[PATCH v3 11/12] drm/virtio: implement context init: add virtio_gpu_fence_event

Thu Nov 18 08:53:34 UTC 2021

On Tue, Nov 16, 2021 at 06:31:10PM -0800, Gurchetan Singh wrote:
> On Tue, Nov 16, 2021 at 7:43 AM Daniel Vetter <daniel at ffwll.ch> wrote:
> 
> > On Mon, Nov 15, 2021 at 07:26:14PM +0000, Kasireddy, Vivek wrote:
> > > Hi Daniel, Greg,
> > >
> > > If it is the same or a similar crash reported here:
> > >
> > https://lists.freedesktop.org/archives/dri-devel/2021-November/330018.html
> > > and here:
> > https://lists.freedesktop.org/archives/dri-devel/2021-November/330212.html
> > > then the fix is already merged:
> > >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d89c0c8322ecdc9a2ec84b959b6f766be082da76
> 
> Yeah but that still leaves the problem of why exaxtly virtgpu is
> > reinventing drm_poll here?
> 
> 
> > Can you please replace it with drm_poll like all other drivers, including
> > the ones that have private events?
> >
> 
> Hi Daniel,
> 
> Allow me to explain the use case a bit.  It's for when virtgpu KMS is not
> used, but a special Wayland compositor does wayland passthrough instead:
> 
> https://www.youtube.com/watch?v=WwrXqDERFm8https://www.youtube.com/watch?v=EkNBsBx501Q
> 
> This technique has gained much popularity in the virtualized laptop
> space, where it offers better performance/user experience than virtgpu
> KMS.  The relevant paravirtualized userspace is "Sommelier":
> 
> https://chromium.googlesource.com/chromiumos/platform2/+/main/vm_tools/sommelier/
> https://chromium.googlesource.com/chromiumos/platform2/+/main/vm_tools/sommelier/virtualization/virtgpu_channel.cc
> 
> Previously, we were using the out-of-tree virtio-wl device and there
> were many discussions on how we could get this upstream:
> 
> https://lists.freedesktop.org/archives/dri-devel/2017-December/160309.html
> https://lists.oasis-open.org/archives/virtio-dev/202002/msg00005.html
> 
> Extending virtgpu was deemed the least intrusive option:
> 
> https://www.spinics.net/lists/kvm/msg159206.html
> 
> We ultimately settled on the context type abstraction and used
> virtio_gpu_poll to tell the guest "hey, we have a Wayland event".  The
> host response is actually in a buffer of type BLOB_MEM_GUEST.
> 
> It is possible to use drm_poll(..), but that would have to be
> accompanied by a drm_read(..).  You'll need to define a dummy
> VIRTGPU_EVENT_FENCE_SIGNALED in the uapi too.
> 
> That's originally how I did it, but some pointed out that's
> unnecessary since the host response is in the BLOB_MEM_GUEST buffer
> and virtgpu event is a dummy event.  So we decided just to modify
> virtio_gpu_poll(..) to have the desired semantics in that case.
> 
> For the regular virtio-gpu KMS path, things remain unchanged.
> 
> There are of course other ways to do it (perhaps polling a dma_fence),
> but that was the cleanest way we could find.
> 
> It's not rare for virtio to "special things" (see virtio_dma_buf_ops,
> virtio_dma_ops), since they are in fake devices.

These are all internal interfaces, not uapi.

> We're open to other ideas, but hopefully that answers some of your
> questions.

Well for one, why does the commit message not explain any of this. You're
building uapi, which is forever, it's paramount all considerations are
properly explained.

Second, I really don't like that youre redefining poll semantics in
incompatible ways from all other drm drivers. If you want special poll
semantics then just create a sperate fd for that (or a dma_fence or
whatever, maybe that saves some typing), but bending the drm fd semantics
is no good at all. We have tons of different fd with their dedicated
semantics in drm, trying to shoehorn it all into one just isn't very good
design.

Or do the dummy event which is just the event code, but does not contain
any data. Either is fine with me.

Can you pls do this asap? I really don't want to bake this in as uapi
which we then have to quirk and support forever. I'd say revert for -rc2
of these two and then maybe sort it out properly in -next.

Cheers, Daniel
> 
> 
> > Thanks, Daniel
> >
> > >
> > > Thanks,
> > > Vivek
> > >
> > > > On Sat, Nov 13, 2021 at 03:51:48PM +0100, Greg KH wrote:
> > > > > On Tue, Sep 21, 2021 at 04:20:23PM -0700, Gurchetan Singh wrote:
> > > > > > Similar to DRM_VMW_EVENT_FENCE_SIGNALED.  Sends a pollable event
> > > > > > to the DRM file descriptor when a fence on a specific ring is
> > > > > > signaled.
> > > > > >
> > > > > > One difference is the event is not exposed via the UAPI -- this is
> > > > > > because host responses are on a shared memory buffer of type
> > > > > > BLOB_MEM_GUEST [this is the common way to receive responses with
> > > > > > virtgpu].  As such, there is no context specific read(..)
> > > > > > implementation either -- just a poll(..) implementation.
> > > > > >
> > > > > > Signed-off-by: Gurchetan Singh <gurchetansingh at chromium.org>
> > > > > > Acked-by: Nicholas Verne <nverne at chromium.org>
> > > > > > ---
> > > > > >  drivers/gpu/drm/virtio/virtgpu_drv.c   | 43
> > +++++++++++++++++++++++++-
> > > > > >  drivers/gpu/drm/virtio/virtgpu_drv.h   |  7 +++++
> > > > > >  drivers/gpu/drm/virtio/virtgpu_fence.c | 10 ++++++
> > > > > >  drivers/gpu/drm/virtio/virtgpu_ioctl.c | 34 ++++++++++++++++++++
> > > > > >  4 files changed, 93 insertions(+), 1 deletion(-)
> > > > >
> > > > > This commit seems to cause a crash in a virtual drm gpu driver for
> > > > > Android.  I have reverted this, and the next commit in the series
> > from
> > > > > Linus's tree and all is good again.
> > > > >
> > > > > Any ideas?
> > > >
> > > > Well no, but also this patch looks very questionable of hand-rolling
> > > > drm_poll. Yes you can do driver private events like
> > > > DRM_VMW_EVENT_FENCE_SIGNALED, that's fine. But you really should not
> > need
> > > > to hand-roll the poll callback. vmwgfx (which generally is a very old
> > > > driver which has lots of custom stuff, so not a great example) doesn't
> > do
> > > > that either.
> > > >
> > > > So that part should go no matter what I think.
> > > > -Daniel
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> >

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch