dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel@ffwll.ch>
To: Gurchetan Singh <gurchetansingh@chromium.org>
Cc: "virtio-dev@lists.oasis-open.org"
	<virtio-dev@lists.oasis-open.org>,
	Greg KH <gregkh@linuxfoundation.org>,
	"Kasireddy, Vivek" <vivek.kasireddy@intel.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	"kraxel@redhat.com" <kraxel@redhat.com>
Subject: Re: [PATCH v3 11/12] drm/virtio: implement context init: add virtio_gpu_fence_event
Date: Thu, 18 Nov 2021 09:53:34 +0100	[thread overview]
Message-ID: <YZYUjh73sPYISNKc@phenom.ffwll.local> (raw)
In-Reply-To: <CAAfnVBms1Bi8MnaCZVv=4dgoG+REVzZ-zFq-hRQ-4tCzYBrDdA@mail.gmail.com>

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@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@chromium.org>
> > > > > > Acked-by: Nicholas Verne <nverne@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

  reply	other threads:[~2021-11-18  8:53 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-21 23:20 [PATCH v3 00/12] Context types, v3 Gurchetan Singh
2021-09-21 23:20 ` [PATCH v3 01/12] virtio-gpu api: multiple context types with explicit initialization Gurchetan Singh
2021-09-21 23:20 ` [PATCH v3 02/12] drm/virtgpu api: create context init feature Gurchetan Singh
2021-09-21 23:20 ` [PATCH v3 03/12] drm/virtio: implement context init: track valid capabilities in a mask Gurchetan Singh
2021-09-21 23:20 ` [PATCH v3 04/12] drm/virtio: implement context init: probe for feature Gurchetan Singh
2021-09-21 23:20 ` [PATCH v3 05/12] drm/virtio: implement context init: support init ioctl Gurchetan Singh
2021-09-21 23:20 ` [PATCH v3 06/12] drm/virtio: implement context init: track {ring_idx, emit_fence_info} in virtio_gpu_fence Gurchetan Singh
2021-09-21 23:20 ` [PATCH v3 07/12] drm/virtio: implement context init: plumb {base_fence_ctx, ring_idx} to virtio_gpu_fence_alloc Gurchetan Singh
2021-09-21 23:20 ` [PATCH v3 08/12] drm/virtio: implement context init: stop using drv->context when creating fence Gurchetan Singh
2021-09-21 23:20 ` [PATCH v3 09/12] drm/virtio: implement context init: allocate an array of fence contexts Gurchetan Singh
2021-09-21 23:20 ` [PATCH v3 10/12] drm/virtio: implement context init: handle VIRTGPU_CONTEXT_PARAM_POLL_RINGS_MASK Gurchetan Singh
2021-09-21 23:20 ` [PATCH v3 11/12] drm/virtio: implement context init: add virtio_gpu_fence_event Gurchetan Singh
2021-11-13 14:51   ` Greg KH
2021-11-15 14:16     ` Daniel Vetter
2021-11-15 19:26       ` Kasireddy, Vivek
2021-11-16 15:43         ` Daniel Vetter
2021-11-17  2:31           ` Gurchetan Singh
2021-11-18  8:53             ` Daniel Vetter [this message]
2021-11-19 17:43               ` Rob Clark
2021-11-20  1:38                 ` Gurchetan Singh
2021-09-21 23:20 ` [PATCH v3 12/12] drm/virtio: implement context init: advertise feature to userspace Gurchetan Singh
2021-09-29  7:33 ` [PATCH v3 00/12] Context types, v3 Gerd Hoffmann

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YZYUjh73sPYISNKc@phenom.ffwll.local \
    --to=daniel@ffwll.ch \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=gurchetansingh@chromium.org \
    --cc=kraxel@redhat.com \
    --cc=virtio-dev@lists.oasis-open.org \
    --cc=vivek.kasireddy@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).