| Message ID | 20180724082213.25677-1-robert.foss@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] drm: Allow DRM_IOCTL_MODE_MAP_DUMB for render nodes | expand |
Hi,
I am running into the same problem using etnaviv on i.MX6 under Android 8.1
The mesa etnaviv code uses CREATE_DUMB / MAP_DUMB for the scanout buffers
and is called from the surfaceflinger process.
But, under Android 8.1 drm_hwcomposer runs in a seperate process to surfaceflinger.
Since drm_hwcomposer needs to use the KMS API it must be the DRM master,
that means that surface flinger cannot be DRM master too.
Adding a render node to the imx drm device and configuring mesa to user fixes
the problem but requires the render node to have access rights to use CREATE_DUMB
/ MAP_DUMB.
Here is my full patch:
Make imx-drm export a render node so that mesa can use it to allocate
From: Martin Fuzzey <martin.fuzzey@flowbird.group>
Date: 2018-07-26 15:37:28 +0200
dumb memory rather than requiring the master node.
The problem with using the master node is that, on android 8.1, drm_hwcomposer
is a seperate process and must be the master node (to use the KMS API),
since only a single process may be master surfaceflinger cannot be master too.
With this change surfaceflinger can use just a rendernode.
Note that we also have to modify the permissions table to allow render nodes
to use the dumb allocation functions. This is a hack and unlikely to be accepted
upstream but it's better than the huge security hole of allowing everything we were
using before.
Need to discuss an upstream acceptable way for this...
---
drivers/gpu/drm/drm_ioctl.c | 6 +++---
drivers/gpu/drm/imx/imx-drm-core.c | 2 +-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index a9ae6dd..31c4c86 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -639,9 +639,9 @@ int drm_ioctl_permit(u32 flags, struct drm_file *file_priv)
DRM_IOCTL_DEF(DRM_IOCTL_MODE_RMFB, drm_mode_rmfb, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_PAGE_FLIP, drm_mode_page_flip_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_DIRTYFB, drm_mode_dirtyfb_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
- DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_DUMB, drm_mode_create_dumb_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
- DRM_IOCTL_DEF(DRM_IOCTL_MODE_MAP_DUMB, drm_mode_mmap_dumb_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
- DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROY_DUMB, drm_mode_destroy_dumb_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
+ DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_DUMB, drm_mode_create_dumb_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED|DRM_RENDER_ALLOW),
+ DRM_IOCTL_DEF(DRM_IOCTL_MODE_MAP_DUMB, drm_mode_mmap_dumb_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED|DRM_RENDER_ALLOW),
+ DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROY_DUMB, drm_mode_destroy_dumb_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED|DRM_RENDER_ALLOW),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_OBJ_GETPROPERTIES, drm_mode_obj_get_properties_ioctl, DRM_CONTROL_ALLOW|DRM_UNLOCKED),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_OBJ_SETPROPERTY, drm_mode_obj_set_property_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
DRM_IOCTL_DEF(DRM_IOCTL_MODE_CURSOR2, drm_mode_cursor2_ioctl, DRM_MASTER|DRM_CONTROL_ALLOW|DRM_UNLOCKED),
diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index f91cb72..0c34306 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -177,7 +177,7 @@ int imx_drm_encoder_parse_of(struct drm_device *drm,
static struct drm_driver imx_drm_driver = {
.driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_PRIME |
- DRIVER_ATOMIC,
+ DRIVER_ATOMIC | DRIVER_RENDER,
.lastclose = imx_drm_driver_lastclose,
.gem_free_object_unlocked = drm_gem_cma_free_object,
.gem_vm_ops = &drm_gem_cma_vm_ops,
If it is not acceptable to modify the access rights for the DUMB allocation functions how should this be done?
Best regards,
Martin
Hi all, Sharing some ideas on the topic. Personally I'm neither for, nor against this patch. On 24 July 2018 at 09:22, Robert Foss <robert.foss@collabora.com> wrote: > From: Tomasz Figa <tfiga@chromium.org> > > There is no particular reason to prevent userspace for using this IOCTL, > considering that it already has access to other, platform-specific > IOCTLs. This patch makes is possible to have the same userspace code > work regardless on the underlying platform, which significantly > simplifies the stack. > > Signed-off-by: Tomasz Figa <tfiga@chromium.org> > Reviewed-by: Zach Reizner <zachr@chromium.org> > Signed-off-by: Nicolas Norvez <norvez@chromium.org> > Reviewed-by: Tomasz Figa <tfiga@chromium.org> > Signed-off-by: Robert Foss <robert.foss@collabora.com> > --- > > I've been looking into enabling a kms_swrast based driver for mesa on > the Android platform[1]. > > But have come up against the issue of dumb buffer related ioctls below > not being flagged with DRM_RENDER_ALLOW. > > DRM_IOCTL_MODE_CREATE_DUMB > DRM_IOCTL_MODE_MAP_DUMB > > To be more precise, I've been seeing a failure due to DRM_IOCTL_MODE_MAP_DUMB > not being allowed for /dev/dri/renderD* nodes, and used in mesa > kms_sw_displaytarget_map(). > > > As I understand it the DRM_RENDER_ALLOW flag being unset is a very intentional > restriction placed on dumb buffers in order to minimize its use. > But as far as alternative solutions for software renderers there seems to only > be VGEM and mmap()-ing DMABUFs. > > While it would be convenient from the point of view of software renderers if > dumb buffers had more promiscuous permissions, it may be a hard sell upstream. > > If dumb buffers aren't the way forward, what is? VGEM? Or are there any other > preferable ways? > The description of VGEM says "... as used by Mesa's software renderer for enhanced performance." Although that hasn't been the case really, since we're opening an arbitrary GPU node with kms_swrast. I think we should fix that. On top of that we could also use the card node, which will remove the need for this patch. Yet again, there may be reasonable usecases where one needs the render node to support the dumb buffer ioctls. HTH Emil
Hi Martin, On 1 August 2018 at 15:24, Martin Fuzzey <martin.fuzzey@flowbird.group> wrote: > Hi, > > I am running into the same problem using etnaviv on i.MX6 under Android 8.1 > > The mesa etnaviv code uses CREATE_DUMB / MAP_DUMB for the scanout buffers > and is called from the surfaceflinger process. > > But, under Android 8.1 drm_hwcomposer runs in a seperate process to > surfaceflinger. > Since drm_hwcomposer needs to use the KMS API it must be the DRM master, > that means that surface flinger cannot be DRM master too. > > Adding a render node to the imx drm device and configuring mesa to user > fixes > the problem but requires the render node to have access rights to use > CREATE_DUMB > / MAP_DUMB. > > > Here is my full patch: > > Make imx-drm export a render node so that mesa can use it to allocate > Let's start with the not-so obvious question: Why does one open the imx as render node? Of the top of my head: There is nothing in egl/android that should require an authenticated device. Hence, using a card node should be fine - the etnaviv code opens the render node it needs. It's been a while since I looked at the imx/etna code, so I may be missing something. HTH Emil
Hi Emil, On 03/08/18 14:35, Emil Velikov wrote: > Hi Martin, > > On 1 August 2018 at 15:24, Martin Fuzzey <martin.fuzzey@flowbird.group> wrote: > > Let's start with the not-so obvious question: > Why does one open the imx as render node? > > Of the top of my head: > There is nothing in egl/android that should require an authenticated device. > Hence, using a card node should be fine - the etnaviv code opens the > render node it needs. Yes, the problem is not in egl/android but in the scanout buffer allocation code. etnaviv opens the render node on the *GPU* (for submitting GPU commands), that part is fine. But scanout buffers need to be allocated from imx-drm not etnaviv. This done by renderonly_create_kms_dumb_buffer_for_resource() [src/gallium/auxiliary/renderonly/renderonly.c] Which uses DRM_IOCTL_MODE_CREATE_DUMB followed by DRM_IOCTL_PRIME_FD_TO_HANDLE on the "kms_fd" (probably poorly named because it's not actually used for modesetting) see imx_drm_screen_create()[ src/gallium/winsys/imx/drm/imx_drm_winsys.c] If the card node is used DRM_IOCTL_MODE_CREATE_DUMB works but DRM_IOCTL_PRIME_FD_TO_HANDLE fails, because the permissions are DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW In android 8.1 the hardware composer runs in a seperate process and it has to use the card node and be drm master (to use the KMS API), therefore, when the surface flinger calls renderonly_create_kms_dumb_buffer_for_resource() it is not authenticated. Making surface flinger use a render node fixes the problem for DRM_IOCTL_PRIME_FD_TO_HANDLE (because that already has DRM_RENDER_ALLOW), but DRM_IOCTL_MODE_CREATE_DUMB now fails without the patch. This probably worked in previous versions of Android where surface flinger and hwc were all in the same process. Regards, Martin
On 3 August 2018 at 16:06, Martin Fuzzey <martin.fuzzey@flowbird.group> wrote: > Hi Emil, > > On 03/08/18 14:35, Emil Velikov wrote: >> >> Hi Martin, >> >> On 1 August 2018 at 15:24, Martin Fuzzey <martin.fuzzey@flowbird.group> >> wrote: >> >> Let's start with the not-so obvious question: >> Why does one open the imx as render node? >> >> Of the top of my head: >> There is nothing in egl/android that should require an authenticated >> device. >> Hence, using a card node should be fine - the etnaviv code opens the >> render node it needs. > > > Yes, the problem is not in egl/android but in the scanout buffer allocation > code. > > etnaviv opens the render node on the *GPU* (for submitting GPU commands), > that part is fine. > > But scanout buffers need to be allocated from imx-drm not etnaviv. > > This done by renderonly_create_kms_dumb_buffer_for_resource() > [src/gallium/auxiliary/renderonly/renderonly.c] > Which uses DRM_IOCTL_MODE_CREATE_DUMB followed by > DRM_IOCTL_PRIME_FD_TO_HANDLE > on the "kms_fd" (probably poorly named because it's not actually used for > modesetting) > see imx_drm_screen_create()[ src/gallium/winsys/imx/drm/imx_drm_winsys.c] > > > If the card node is used DRM_IOCTL_MODE_CREATE_DUMB works but > DRM_IOCTL_PRIME_FD_TO_HANDLE fails, because the permissions are > DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW > Right I missed the DRM_AUTH, in the fd <> handle IOCTLs. So in order for things to work, we'd need to either: - allow dumb buffers for render nodes, or - drop the DRM_AUTH for fd <> handle imports Pointing an alternative solution, for kernel developers to analyse and make a decision. > > In android 8.1 the hardware composer runs in a seperate process and it has > to use the card node and be drm master (to use the KMS API), > therefore, when the surface flinger calls > renderonly_create_kms_dumb_buffer_for_resource() it is not authenticated. > > Making surface flinger use a render node fixes the problem for > DRM_IOCTL_PRIME_FD_TO_HANDLE (because that already has DRM_RENDER_ALLOW), > but DRM_IOCTL_MODE_CREATE_DUMB now fails without the patch. > > > This probably worked in previous versions of Android where surface flinger > and hwc were all in the same process. > There has been varying hacks for Android through the years. Bringing details into the discussion will result in a significant diversion. Something we could avoid, for the time being ;-) Thanks Emil
On Fri, Aug 03, 2018 at 06:03:50PM +0100, Emil Velikov wrote: > On 3 August 2018 at 16:06, Martin Fuzzey <martin.fuzzey@flowbird.group> wrote: > > Hi Emil, > > > > On 03/08/18 14:35, Emil Velikov wrote: > >> > >> Hi Martin, > >> > >> On 1 August 2018 at 15:24, Martin Fuzzey <martin.fuzzey@flowbird.group> > >> wrote: > >> > >> Let's start with the not-so obvious question: > >> Why does one open the imx as render node? > >> > >> Of the top of my head: > >> There is nothing in egl/android that should require an authenticated > >> device. > >> Hence, using a card node should be fine - the etnaviv code opens the > >> render node it needs. > > > > > > Yes, the problem is not in egl/android but in the scanout buffer allocation > > code. > > > > etnaviv opens the render node on the *GPU* (for submitting GPU commands), > > that part is fine. > > > > But scanout buffers need to be allocated from imx-drm not etnaviv. > > > > This done by renderonly_create_kms_dumb_buffer_for_resource() > > [src/gallium/auxiliary/renderonly/renderonly.c] > > Which uses DRM_IOCTL_MODE_CREATE_DUMB followed by > > DRM_IOCTL_PRIME_FD_TO_HANDLE > > on the "kms_fd" (probably poorly named because it's not actually used for > > modesetting) > > see imx_drm_screen_create()[ src/gallium/winsys/imx/drm/imx_drm_winsys.c] > > > > > > If the card node is used DRM_IOCTL_MODE_CREATE_DUMB works but > > DRM_IOCTL_PRIME_FD_TO_HANDLE fails, because the permissions are > > DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW > > > Right I missed the DRM_AUTH, in the fd <> handle IOCTLs. > So in order for things to work, we'd need to either: > - allow dumb buffers for render nodes, or > - drop the DRM_AUTH for fd <> handle imports > > Pointing an alternative solution, for kernel developers to analyse and > make a decision. > > > > > In android 8.1 the hardware composer runs in a seperate process and it has > > to use the card node and be drm master (to use the KMS API), > > therefore, when the surface flinger calls > > renderonly_create_kms_dumb_buffer_for_resource() it is not authenticated. > > > > Making surface flinger use a render node fixes the problem for > > DRM_IOCTL_PRIME_FD_TO_HANDLE (because that already has DRM_RENDER_ALLOW), > > but DRM_IOCTL_MODE_CREATE_DUMB now fails without the patch. > > > > > > This probably worked in previous versions of Android where surface flinger > > and hwc were all in the same process. > > > There has been varying hacks for Android through the years. Bringing > details into the discussion will result in a significant diversion. > Something we could avoid, for the time being ;-) Did someone say diversion?!? The way this was handled prior to using render/control nodes in drm_hwc/[drm/gbm]_gralloc is that all modesetting was done via gralloc which was master. The hwc implementation was basically a proxy backchanneling all of the work to gralloc. Anyways, we probably don't want to go back there. Fwiw, I'd lean towards allowing DUMB allocation from the render nodes. I understand it limits use cases that are undesirable, but it is also limiting usecases that are desirable. So, given that people are going to get "creative" regardless of how many safety railings we put up, we shouldn't make things unnecessarily hard on other trying to Get Work Done. Sean [Disclaimer: I'm totally and completely biased on this issue] > > Thanks > Emil
On Fri, Aug 3, 2018 at 1:50 PM Sean Paul <seanpaul@chromium.org> wrote: > > On Fri, Aug 03, 2018 at 06:03:50PM +0100, Emil Velikov wrote: > > On 3 August 2018 at 16:06, Martin Fuzzey <martin.fuzzey@flowbird.group> wrote: > > > Hi Emil, > > > > > > On 03/08/18 14:35, Emil Velikov wrote: > > >> > > >> Hi Martin, > > >> > > >> On 1 August 2018 at 15:24, Martin Fuzzey <martin.fuzzey@flowbird.group> > > >> wrote: > > >> > > >> Let's start with the not-so obvious question: > > >> Why does one open the imx as render node? > > >> > > >> Of the top of my head: > > >> There is nothing in egl/android that should require an authenticated > > >> device. > > >> Hence, using a card node should be fine - the etnaviv code opens the > > >> render node it needs. > > > > > > > > > Yes, the problem is not in egl/android but in the scanout buffer allocation > > > code. > > > > > > etnaviv opens the render node on the *GPU* (for submitting GPU commands), > > > that part is fine. > > > > > > But scanout buffers need to be allocated from imx-drm not etnaviv. > > > > > > This done by renderonly_create_kms_dumb_buffer_for_resource() > > > [src/gallium/auxiliary/renderonly/renderonly.c] > > > Which uses DRM_IOCTL_MODE_CREATE_DUMB followed by > > > DRM_IOCTL_PRIME_FD_TO_HANDLE > > > on the "kms_fd" (probably poorly named because it's not actually used for > > > modesetting) > > > see imx_drm_screen_create()[ src/gallium/winsys/imx/drm/imx_drm_winsys.c] > > > > > > > > > If the card node is used DRM_IOCTL_MODE_CREATE_DUMB works but > > > DRM_IOCTL_PRIME_FD_TO_HANDLE fails, because the permissions are > > > DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW > > > > > Right I missed the DRM_AUTH, in the fd <> handle IOCTLs. > > So in order for things to work, we'd need to either: > > - allow dumb buffers for render nodes, or > > - drop the DRM_AUTH for fd <> handle imports > > > > Pointing an alternative solution, for kernel developers to analyse and > > make a decision. > > > > > > > > In android 8.1 the hardware composer runs in a seperate process and it has > > > to use the card node and be drm master (to use the KMS API), > > > therefore, when the surface flinger calls > > > renderonly_create_kms_dumb_buffer_for_resource() it is not authenticated. > > > > > > Making surface flinger use a render node fixes the problem for > > > DRM_IOCTL_PRIME_FD_TO_HANDLE (because that already has DRM_RENDER_ALLOW), > > > but DRM_IOCTL_MODE_CREATE_DUMB now fails without the patch. > > > > > > > > > This probably worked in previous versions of Android where surface flinger > > > and hwc were all in the same process. > > > > > There has been varying hacks for Android through the years. Bringing > > details into the discussion will result in a significant diversion. > > Something we could avoid, for the time being ;-) > > Did someone say diversion?!? The way this was handled prior to using > render/control nodes in drm_hwc/[drm/gbm]_gralloc is that all modesetting was > done via gralloc which was master. The hwc implementation was basically a proxy > backchanneling all of the work to gralloc. > > Anyways, we probably don't want to go back there. > > Fwiw, I'd lean towards allowing DUMB allocation from the render nodes. I > understand it limits use cases that are undesirable, but it is also limiting > usecases that are desirable. So, given that people are going to get "creative" > regardless of how many safety railings we put up, we shouldn't make things > unnecessarily hard on other trying to Get Work Done. The problem with using render nodes is what if there isn't one? We require VGEM (and make VGEM allow dumb buffers) in that case? Rob
On 06/08/18 21:05, Rob Herring wrote: > On Fri, Aug 3, 2018 at 1:50 PM Sean Paul <seanpaul@chromium.org> wrote: >> Fwiw, I'd lean towards allowing DUMB allocation from the render nodes. I >> understand it limits use cases that are undesirable, but it is also limiting >> usecases that are desirable. So, given that people are going to get "creative" >> regardless of how many safety railings we put up, we shouldn't make things >> unnecessarily hard on other trying to Get Work Done. > The problem with using render nodes is what if there isn't one? We > require VGEM (and make VGEM allow dumb buffers) in that case? Try to open the render node and fall back to the card node if it doesn't exist? AFAICT VGEM doesn't provide contiguous buffers so it won't work for the imx-drm case. Regards, Martin
On 3 August 2018 at 20:50, Sean Paul <seanpaul@chromium.org> wrote: > On Fri, Aug 03, 2018 at 06:03:50PM +0100, Emil Velikov wrote: >> On 3 August 2018 at 16:06, Martin Fuzzey <martin.fuzzey@flowbird.group> wrote: >> > Hi Emil, >> > >> > On 03/08/18 14:35, Emil Velikov wrote: >> >> >> >> Hi Martin, >> >> >> >> On 1 August 2018 at 15:24, Martin Fuzzey <martin.fuzzey@flowbird.group> >> >> wrote: >> >> >> >> Let's start with the not-so obvious question: >> >> Why does one open the imx as render node? >> >> >> >> Of the top of my head: >> >> There is nothing in egl/android that should require an authenticated >> >> device. >> >> Hence, using a card node should be fine - the etnaviv code opens the >> >> render node it needs. >> > >> > >> > Yes, the problem is not in egl/android but in the scanout buffer allocation >> > code. >> > >> > etnaviv opens the render node on the *GPU* (for submitting GPU commands), >> > that part is fine. >> > >> > But scanout buffers need to be allocated from imx-drm not etnaviv. >> > >> > This done by renderonly_create_kms_dumb_buffer_for_resource() >> > [src/gallium/auxiliary/renderonly/renderonly.c] >> > Which uses DRM_IOCTL_MODE_CREATE_DUMB followed by >> > DRM_IOCTL_PRIME_FD_TO_HANDLE >> > on the "kms_fd" (probably poorly named because it's not actually used for >> > modesetting) >> > see imx_drm_screen_create()[ src/gallium/winsys/imx/drm/imx_drm_winsys.c] >> > >> > >> > If the card node is used DRM_IOCTL_MODE_CREATE_DUMB works but >> > DRM_IOCTL_PRIME_FD_TO_HANDLE fails, because the permissions are >> > DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW >> > >> Right I missed the DRM_AUTH, in the fd <> handle IOCTLs. >> So in order for things to work, we'd need to either: >> - allow dumb buffers for render nodes, or >> - drop the DRM_AUTH for fd <> handle imports >> >> Pointing an alternative solution, for kernel developers to analyse and >> make a decision. >> >> > >> > In android 8.1 the hardware composer runs in a seperate process and it has >> > to use the card node and be drm master (to use the KMS API), >> > therefore, when the surface flinger calls >> > renderonly_create_kms_dumb_buffer_for_resource() it is not authenticated. >> > >> > Making surface flinger use a render node fixes the problem for >> > DRM_IOCTL_PRIME_FD_TO_HANDLE (because that already has DRM_RENDER_ALLOW), >> > but DRM_IOCTL_MODE_CREATE_DUMB now fails without the patch. >> > >> > >> > This probably worked in previous versions of Android where surface flinger >> > and hwc were all in the same process. >> > >> There has been varying hacks for Android through the years. Bringing >> details into the discussion will result in a significant diversion. >> Something we could avoid, for the time being ;-) > > Did someone say diversion?!? The way this was handled prior to using > render/control nodes in drm_hwc/[drm/gbm]_gralloc is that all modesetting was > done via gralloc which was master. The hwc implementation was basically a proxy > backchanneling all of the work to gralloc. > > Anyways, we probably don't want to go back there. > Now that we got the diversion out of the way, any input on my proposal to drop the DRM_AUTH for fd <> imports. Am I missing something pretty obvious that makes the idea a no-go? Thanks Emil
On Tue, Aug 07, 2018 at 12:01:50PM +0100, Emil Velikov wrote: > On 3 August 2018 at 20:50, Sean Paul <seanpaul@chromium.org> wrote: > > On Fri, Aug 03, 2018 at 06:03:50PM +0100, Emil Velikov wrote: > >> On 3 August 2018 at 16:06, Martin Fuzzey <martin.fuzzey@flowbird.group> wrote: > >> > Hi Emil, > >> > > >> > On 03/08/18 14:35, Emil Velikov wrote: > >> >> > >> >> Hi Martin, > >> >> > >> >> On 1 August 2018 at 15:24, Martin Fuzzey <martin.fuzzey@flowbird.group> > >> >> wrote: > >> >> > >> >> Let's start with the not-so obvious question: > >> >> Why does one open the imx as render node? > >> >> > >> >> Of the top of my head: > >> >> There is nothing in egl/android that should require an authenticated > >> >> device. > >> >> Hence, using a card node should be fine - the etnaviv code opens the > >> >> render node it needs. > >> > > >> > > >> > Yes, the problem is not in egl/android but in the scanout buffer allocation > >> > code. > >> > > >> > etnaviv opens the render node on the *GPU* (for submitting GPU commands), > >> > that part is fine. > >> > > >> > But scanout buffers need to be allocated from imx-drm not etnaviv. > >> > > >> > This done by renderonly_create_kms_dumb_buffer_for_resource() > >> > [src/gallium/auxiliary/renderonly/renderonly.c] > >> > Which uses DRM_IOCTL_MODE_CREATE_DUMB followed by > >> > DRM_IOCTL_PRIME_FD_TO_HANDLE > >> > on the "kms_fd" (probably poorly named because it's not actually used for > >> > modesetting) > >> > see imx_drm_screen_create()[ src/gallium/winsys/imx/drm/imx_drm_winsys.c] > >> > > >> > > >> > If the card node is used DRM_IOCTL_MODE_CREATE_DUMB works but > >> > DRM_IOCTL_PRIME_FD_TO_HANDLE fails, because the permissions are > >> > DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW > >> > > >> Right I missed the DRM_AUTH, in the fd <> handle IOCTLs. > >> So in order for things to work, we'd need to either: > >> - allow dumb buffers for render nodes, or > >> - drop the DRM_AUTH for fd <> handle imports > >> > >> Pointing an alternative solution, for kernel developers to analyse and > >> make a decision. > >> > >> > > >> > In android 8.1 the hardware composer runs in a seperate process and it has > >> > to use the card node and be drm master (to use the KMS API), > >> > therefore, when the surface flinger calls > >> > renderonly_create_kms_dumb_buffer_for_resource() it is not authenticated. > >> > > >> > Making surface flinger use a render node fixes the problem for > >> > DRM_IOCTL_PRIME_FD_TO_HANDLE (because that already has DRM_RENDER_ALLOW), > >> > but DRM_IOCTL_MODE_CREATE_DUMB now fails without the patch. > >> > > >> > > >> > This probably worked in previous versions of Android where surface flinger > >> > and hwc were all in the same process. > >> > > >> There has been varying hacks for Android through the years. Bringing > >> details into the discussion will result in a significant diversion. > >> Something we could avoid, for the time being ;-) > > > > Did someone say diversion?!? The way this was handled prior to using > > render/control nodes in drm_hwc/[drm/gbm]_gralloc is that all modesetting was > > done via gralloc which was master. The hwc implementation was basically a proxy > > backchanneling all of the work to gralloc. > > > > Anyways, we probably don't want to go back there. > > > Now that we got the diversion out of the way, any input on my proposal > to drop the DRM_AUTH for fd <> imports. > Am I missing something pretty obvious that makes the idea a no-go? Dropping DRM_AUTH is only relevant for the card node. And a card node might not be sufficiently isolated against concurrent other clients, which is why we don't allow it. What we could do is essentially check whether your driver supports render nodes (indicating sufficient amounts of separation), and then allow anything for unauthenicated clients if DRM_RENDER_ALLOW is set on the ioctl. But that's just reinventing render nodes on top of legacy card nodes, and I'm not clear on what that exactly gains us. I think the proposal for dumb render nodes (for drivers which only do dumb kms buffers and no rendering at all) that's been discusson on irc a bit makes a lot more sense. -Daniel
On 7 August 2018 at 13:28, Daniel Vetter <daniel@ffwll.ch> wrote: > On Tue, Aug 07, 2018 at 12:01:50PM +0100, Emil Velikov wrote: >> On 3 August 2018 at 20:50, Sean Paul <seanpaul@chromium.org> wrote: >> > On Fri, Aug 03, 2018 at 06:03:50PM +0100, Emil Velikov wrote: >> >> On 3 August 2018 at 16:06, Martin Fuzzey <martin.fuzzey@flowbird.group> wrote: >> >> > Hi Emil, >> >> > >> >> > On 03/08/18 14:35, Emil Velikov wrote: >> >> >> >> >> >> Hi Martin, >> >> >> >> >> >> On 1 August 2018 at 15:24, Martin Fuzzey <martin.fuzzey@flowbird.group> >> >> >> wrote: >> >> >> >> >> >> Let's start with the not-so obvious question: >> >> >> Why does one open the imx as render node? >> >> >> >> >> >> Of the top of my head: >> >> >> There is nothing in egl/android that should require an authenticated >> >> >> device. >> >> >> Hence, using a card node should be fine - the etnaviv code opens the >> >> >> render node it needs. >> >> > >> >> > >> >> > Yes, the problem is not in egl/android but in the scanout buffer allocation >> >> > code. >> >> > >> >> > etnaviv opens the render node on the *GPU* (for submitting GPU commands), >> >> > that part is fine. >> >> > >> >> > But scanout buffers need to be allocated from imx-drm not etnaviv. >> >> > >> >> > This done by renderonly_create_kms_dumb_buffer_for_resource() >> >> > [src/gallium/auxiliary/renderonly/renderonly.c] >> >> > Which uses DRM_IOCTL_MODE_CREATE_DUMB followed by >> >> > DRM_IOCTL_PRIME_FD_TO_HANDLE >> >> > on the "kms_fd" (probably poorly named because it's not actually used for >> >> > modesetting) >> >> > see imx_drm_screen_create()[ src/gallium/winsys/imx/drm/imx_drm_winsys.c] >> >> > >> >> > >> >> > If the card node is used DRM_IOCTL_MODE_CREATE_DUMB works but >> >> > DRM_IOCTL_PRIME_FD_TO_HANDLE fails, because the permissions are >> >> > DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW >> >> > >> >> Right I missed the DRM_AUTH, in the fd <> handle IOCTLs. >> >> So in order for things to work, we'd need to either: >> >> - allow dumb buffers for render nodes, or >> >> - drop the DRM_AUTH for fd <> handle imports >> >> >> >> Pointing an alternative solution, for kernel developers to analyse and >> >> make a decision. >> >> >> >> > >> >> > In android 8.1 the hardware composer runs in a seperate process and it has >> >> > to use the card node and be drm master (to use the KMS API), >> >> > therefore, when the surface flinger calls >> >> > renderonly_create_kms_dumb_buffer_for_resource() it is not authenticated. >> >> > >> >> > Making surface flinger use a render node fixes the problem for >> >> > DRM_IOCTL_PRIME_FD_TO_HANDLE (because that already has DRM_RENDER_ALLOW), >> >> > but DRM_IOCTL_MODE_CREATE_DUMB now fails without the patch. >> >> > >> >> > >> >> > This probably worked in previous versions of Android where surface flinger >> >> > and hwc were all in the same process. >> >> > >> >> There has been varying hacks for Android through the years. Bringing >> >> details into the discussion will result in a significant diversion. >> >> Something we could avoid, for the time being ;-) >> > >> > Did someone say diversion?!? The way this was handled prior to using >> > render/control nodes in drm_hwc/[drm/gbm]_gralloc is that all modesetting was >> > done via gralloc which was master. The hwc implementation was basically a proxy >> > backchanneling all of the work to gralloc. >> > >> > Anyways, we probably don't want to go back there. >> > >> Now that we got the diversion out of the way, any input on my proposal >> to drop the DRM_AUTH for fd <> imports. >> Am I missing something pretty obvious that makes the idea a no-go? > > Dropping DRM_AUTH is only relevant for the card node. And a card node > might not be sufficiently isolated against concurrent other clients, which > is why we don't allow it. > Right. I did not spot anything that would make a distinction based on the card vs render node used. > What we could do is essentially check whether your driver supports render > nodes (indicating sufficient amounts of separation), and then allow > anything for unauthenicated clients if DRM_RENDER_ALLOW is set on the > ioctl. > > But that's just reinventing render nodes on top of legacy card nodes, and > I'm not clear on what that exactly gains us. > As more of a userspace person, it makes sense to keep render nodes for GPU specifics and card ones - KMS/Display Controller. > I think the proposal for dumb render nodes (for drivers which only do dumb > kms buffers and no rendering at all) that's been discusson on irc a bit > makes a lot more sense. Ack. Thanks for shedding some light. -Emil
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c index 0d4cfb232576..ef716246baf6 100644 --- a/drivers/gpu/drm/drm_ioctl.c +++ b/drivers/gpu/drm/drm_ioctl.c @@ -642,8 +642,8 @@ static const struct drm_ioctl_desc drm_ioctls[] = { DRM_IOCTL_DEF(DRM_IOCTL_MODE_RMFB, drm_mode_rmfb, DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_PAGE_FLIP, drm_mode_page_flip_ioctl, DRM_MASTER|DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_DIRTYFB, drm_mode_dirtyfb_ioctl, DRM_MASTER|DRM_UNLOCKED), - DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_DUMB, drm_mode_create_dumb_ioctl, DRM_UNLOCKED), - DRM_IOCTL_DEF(DRM_IOCTL_MODE_MAP_DUMB, drm_mode_mmap_dumb_ioctl, DRM_UNLOCKED), + DRM_IOCTL_DEF(DRM_IOCTL_MODE_CREATE_DUMB, drm_mode_create_dumb_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW), + DRM_IOCTL_DEF(DRM_IOCTL_MODE_MAP_DUMB, drm_mode_mmap_dumb_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW), DRM_IOCTL_DEF(DRM_IOCTL_MODE_DESTROY_DUMB, drm_mode_destroy_dumb_ioctl, DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_OBJ_GETPROPERTIES, drm_mode_obj_get_properties_ioctl, DRM_UNLOCKED), DRM_IOCTL_DEF(DRM_IOCTL_MODE_OBJ_SETPROPERTY, drm_mode_obj_set_property_ioctl, DRM_MASTER|DRM_UNLOCKED),