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@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > >> 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