[RFC] drm: Allow DRM_IOCTL_MODE_MAP_DUMB for render nodes

[RFC] drm: Allow DRM_IOCTL_MODE_MAP_DUMB for render nodes

[Robert Foss]

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),
-- 
2.17.1

Re: [RFC] drm: Allow DRM_IOCTL_MODE_MAP_DUMB for render nodes

[Martin Fuzzey]

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

Re: [RFC] drm: Allow DRM_IOCTL_MODE_MAP_DUMB for render nodes

[Emil Velikov]

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

Re: [RFC] drm: Allow DRM_IOCTL_MODE_MAP_DUMB for render nodes

[Martin Fuzzey]

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

Re: [RFC] drm: Allow DRM_IOCTL_MODE_MAP_DUMB for render nodes

[Emil Velikov]

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

Re: [RFC] drm: Allow DRM_IOCTL_MODE_MAP_DUMB for render nodes

[Sean Paul]

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

-- 
Sean Paul, Software Engineer, Google / Chromium OS

Re: [RFC] drm: Allow DRM_IOCTL_MODE_MAP_DUMB for render nodes

[Rob Herring]

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

Re: [RFC] drm: Allow DRM_IOCTL_MODE_MAP_DUMB for render nodes

[Martin Fuzzey]

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

Re: [RFC] drm: Allow DRM_IOCTL_MODE_MAP_DUMB for render nodes

[Emil Velikov]

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

Re: [RFC] drm: Allow DRM_IOCTL_MODE_MAP_DUMB for render nodes

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC] drm: Allow DRM_IOCTL_MODE_MAP_DUMB for render nodes

[Emil Velikov]

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

Re: [RFC] drm: Allow DRM_IOCTL_MODE_MAP_DUMB for render nodes

[Emil Velikov]

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