[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 at 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 at 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 at 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