[Mesa-dev] [PATCH v2 0/4] Android kms_swrast support

[Rob Herring]

On Thu, Jul 19, 2018 at 9:52 AM Robert Foss <robert.foss at collabora.com> wrote:
>
> Hey Rob,
>
> On 2018-07-19 09:26, Tomasz Figa wrote:
> > On Thu, Jul 19, 2018 at 12:08 AM Robert Foss <robert.foss at collabora.com> wrote:
> >>
> >> Hey Rob,
> >>
> >> On 2018-07-18 15:30, Rob Herring wrote:
> >>> On Tue, Jul 17, 2018 at 4:33 AM Robert Foss <robert.foss at collabora.com> wrote:
> >>>>
> >>>> This series implements kms_swrast support for the Android
> >>>> platform. And since having to debug a null pointer dereference,
> >>>> simplify that process for the next guy.
> >>>
> >>> So is this working for you now?
> >>
> >> I'm seeing page-flips happen in the logs, but have no graphical output on the
> >> Qemu-based setup I'm using now.
> >>
> >> When using virgl I'm seeing the same page-flipping in the logs, but no graphical
> >> output.
> >>
> >>>
> >>>> As it stands now, any kernel must have the following ioctls flagged with
> >>>> DRM_RENDER_ALLOW[1], which isn't the case in the mainline kernel.
> >>>>
> >>>> DRM_IOCTL_MODE_CREATE_DUMB
> >>>> DRM_IOCTL_MODE_MAP_DUMB
> >>>
> >>> Ah, sorry. I should have mentioned this. We have discussed this issue
> >>> in the past, but to no further conclusion.
> >>>
> >>> But as I recall, I thought the issue was also allowing import and
> >>> export of dumb buffers?
> >>
> >> Yeah, it's a two-parter for any AOSP Treble build.
> >> 1) Allow dumb buffer ioctls fom render nodes
> >> 2) Support moving buffers across processes.
> >
> > Wouldn't 2) be automatically solved by 1), since we should be able to
> > run drmPrimeHandleToFD for dumb buffers already?
> >
>
> I thought, perhaps wrongly that drmPrimeHandleToFD was only applicable to dmabufs.
>
> I think I've misunderstood the restrictions of dumb buffers, if they're
> shareable across processes using drmPrimeHandleToFD then only 1) is in our way.

I thought the issue was either:
1) a card node can't export dumb buffers
2) a render node can't allocate dumb buffers

My approach had been disabling the permission check for 1. Tomasz's
approach had been to allow VGEM to allocate dumb buffers.

Either way, we either need more permissions on dumb buffers or define
some new s/w rendering scanout buffers which are just dumb buffers,
but not called that because upstream doesn't want to extend dumb
buffers.

> >>>> While it would be possible to open a non-render node to pass the
> >>>> authentication check, this would still cause authentication issues
> >>>> when the /dev/dri/cardX node needs to be opened as master by both mesa
> >>>> and the compositor.
> >>>
> >>> Right. We've pretty much stripped the support that was there out. Plus
> >>> I don't think it will work with Treble.
> >>>
> >>>> I don't know how acceptable this series is for upstreaming, while relying on
> >>>> a non-mainline kernel. I think the policy is to not accept changes that
> >>>> don't have both a user and kernel space solution in place.
> >>>>
> >>>> Like I noted yesterday[2] the alternative to using dumb buffers and having
> >>>> authentication issues is using VGEM, which is new territory to me, and it would
> >>>> take me a little bit of time to figure exactly how it fits into the current
> >>>> kms_swrast approach.
> >>>> Input, like noted before, is very much welcome.
> >>>
> >>> I'm very much in favor of the former approach. VGEM seems like an
> >>> overly complicated solution when there's a very simple solution.
> >>>
> >>
> >> The former solution being what we have now, dumb buffers?
> >> I don't think dumb buffers are a viable path due to 2) listed above.
> >
> > I don't understand what 2) is about. Could you elaborate on it?
>
> See above!
>
> >
> > I'd personally be for dropping those strange restrictions from render
> > nodes. I don't see why a render node couldn't allocate and map a dumb
> > buffer (for software rendering) and share it with another process that
> > opened a control node (to display it).
>
>  From my understanding the wider communitys idea is to minimize the use of dumb
> buffers.
> A part of not allowing render nodes to use map dumb buffers is meant to
> incentivize proprietary drivers to not do the simplest thing that could possibly
> work, as far as I understand.
>
> So while I'm happy to push that change upstream, if for no other reason than to
> generate a dialogue, maybe it's not all that likely that it will be accepted.

I think we have a valid usecase and it's worth the discussion at least
to force some guidance on direction upstream would like.

> >> If there are any other options I'm not aware of, I'm very much listening.
> >
> > One could just call mmap() on DMA-buf FDs directly rather than
> > importing them, but that could open another can of worms, because FDs
> > don't give us any way to deduplicate buffers (you might be given
> > several FDs pointing to the same buffer, which in case of importing to
> > DRM would end up with the same GEM handle every time).
> >
>
> So mmap()ing dmabuf FDs dealing with that can of worms is preferable to looking
> into a VGEM based approach?

We have to have the duplicate buffer tracking. Android doesn't work without it.

Rob