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/?i...
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=E...
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/somme... https://chromium.googlesource.com/chromiumos/platform2/+/main/vm_tools/somme...
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