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?
[1] https://patchwork.freedesktop.org/series/45966/
drivers/gpu/drm/drm_ioctl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
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),
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 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
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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Emil Velikov -
Martin Fuzzey -
Rob Herring -
Robert Foss -
Sean Paul