| Message ID | 20190419155709.19608-1-marcandre.lureau@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Revert "drm/virtio: drop prime import/export callbacks" | expand |
On Fri, 19 Apr 2019 at 16:57, Marc-André Lureau <marcandre.lureau@redhat.com> wrote: > > This patch does more harm than good, as it breaks both Xwayland and > gnome-shell with X11. > > Xwayland requires DRI3 & DRI3 requires PRIME. > > X11 crash for obscure double-free reason which are hard to debug > (starting X11 by hand doesn't trigger the crash). > > I don't see an apparent problem implementing those stub prime > functions, they may return an error at run-time, and it seems to be > handled fine by GNOME at least. > > Please consider a revert. > > This reverts commit b318e3ff7ca065d6b107e424c85a63d7a6798a69. > > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: Dave Airlie <airlied@redhat.com> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> The DRI3 code extensively uses prime_handle_to_fd and prime_fd_to_handle for self-import. Fwiw the patch is: Reviewed-by: Emil Velikov <emil.velikov@collabora.com> -Emil
On Fri, Apr 19, 2019 at 9:21 AM Emil Velikov <emil.l.velikov@gmail.com> wrote: > > On Fri, 19 Apr 2019 at 16:57, Marc-André Lureau > <marcandre.lureau@redhat.com> wrote: > > > > This patch does more harm than good, as it breaks both Xwayland and > > gnome-shell with X11. > > > > Xwayland requires DRI3 & DRI3 requires PRIME. > > > > X11 crash for obscure double-free reason which are hard to debug > > (starting X11 by hand doesn't trigger the crash). > > > > I don't see an apparent problem implementing those stub prime > > functions, they may return an error at run-time, and it seems to be > > handled fine by GNOME at least. > > > > Please consider a revert. > > > > This reverts commit b318e3ff7ca065d6b107e424c85a63d7a6798a69. > > > > Cc: Gerd Hoffmann <kraxel@redhat.com> > > Cc: Dave Airlie <airlied@redhat.com> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > The DRI3 code extensively uses prime_handle_to_fd and > prime_fd_to_handle for self-import. > > Fwiw the patch is: > Reviewed-by: Emil Velikov <emil.velikov@collabora.com> We also use Xwayland and will be helped by the revert. I believe the cap was removed because the driver only supports self-import. But the driver only runs inside VMs where there is no(?) other dma-buf clients, and it handles runtime errors fine even if there are, I would also like to see this revert be considered. > > -Emil > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Apr 19, 2019 at 05:57:09PM +0200, Marc-André Lureau wrote: > This patch does more harm than good, as it breaks both Xwayland and > gnome-shell with X11. > > Xwayland requires DRI3 & DRI3 requires PRIME. > > X11 crash for obscure double-free reason which are hard to debug > (starting X11 by hand doesn't trigger the crash). > > I don't see an apparent problem implementing those stub prime > functions, they may return an error at run-time, and it seems to be > handled fine by GNOME at least. Well, problem is they are *not* handled fine, this is why the patch is there in the first place. Problem case: gnome-shell (in wayland display server mode) fails to come up with a working desktop in case both a intel vgpu and an emulated display device are present in the system. Looks like wayland tries to render using the intel vgpu, then import the rendered buffers to the emulated display device (qxl or virtio-gpu), and it does not handle the failure. I've tried to add a DRM_PRIME_CAP_LOCAL flag for self-import support: https://patchwork.freedesktop.org/patch/297672/ But looks like Daniel Vetter (Cc'ed) thinks userspace is to blame here for not handling the import failure properly. > +struct sg_table *virtgpu_gem_prime_get_sg_table(struct drm_gem_object *obj) > +{ > + WARN_ONCE(1, "not implemented"); > + return ERR_PTR(-ENODEV); > +} That is actually implemented meanwhile (branch drm-misc-next, will land in 5.2, commit "98f41dc3b3ee drm/virtio: implement prime export". > +struct drm_gem_object *virtgpu_gem_prime_import_sg_table( > + struct drm_device *dev, struct dma_buf_attachment *attach, > + struct sg_table *table) > +{ > + WARN_ONCE(1, "not implemented"); I don't think we should re-add this. Either drop this completely, or replace by a less noisy printk_once("can do self-import only"). cheers, Gerd
Hi, > > The DRI3 code extensively uses prime_handle_to_fd and > > prime_fd_to_handle for self-import. The callbacks are not used for self-import. > I believe the cap was removed because the driver only supports > self-import. But the driver only runs inside VMs where there is no(?) > other dma-buf clients, There can be other dma-buf clients even in a VM. cheers, Gerd
On Tue, 23 Apr 2019 at 09:05, Gerd Hoffmann <kraxel@redhat.com> wrote: > > Hi, > > > > The DRI3 code extensively uses prime_handle_to_fd and > > > prime_fd_to_handle for self-import. > > The callbacks are not used for self-import. > Userspace converts from it's own handles to a dmabuf fd and vice-versa. I assumed that's called self-import, is it not? Can you please enlighten me? Thanks -Emil
On Tue, Apr 23, 2019 at 12:55:07PM +0100, Emil Velikov wrote: > On Tue, 23 Apr 2019 at 09:05, Gerd Hoffmann <kraxel@redhat.com> wrote: > > > > Hi, > > > > > > The DRI3 code extensively uses prime_handle_to_fd and > > > > prime_fd_to_handle for self-import. > > > > The callbacks are not used for self-import. > > > Userspace converts from it's own handles to a dmabuf fd and vice-versa. > I assumed that's called self-import, is it not? self-import is importing a dma-buf into the driver which created it. drm_gem_prime.c has a shortcut for that, it'll just increase the gem reference count instead of going though a full-blown export/import. The prime_handle_to_fd and prime_fd_to_handle callbacks are only needed for the non-self-import case, where the other driver needs/provides a scatter list. cheers, Gerd
On Tue, 23 Apr 2019 at 15:06, Gerd Hoffmann <kraxel@redhat.com> wrote: > > On Tue, Apr 23, 2019 at 12:55:07PM +0100, Emil Velikov wrote: > > On Tue, 23 Apr 2019 at 09:05, Gerd Hoffmann <kraxel@redhat.com> wrote: > > > > > > Hi, > > > > > > > > The DRI3 code extensively uses prime_handle_to_fd and > > > > > prime_fd_to_handle for self-import. > > > > > > The callbacks are not used for self-import. > > > > > Userspace converts from it's own handles to a dmabuf fd and vice-versa. > > I assumed that's called self-import, is it not? > > self-import is importing a dma-buf into the driver which created it. > > drm_gem_prime.c has a shortcut for that, it'll just increase the gem > reference count instead of going though a full-blown export/import. > > The prime_handle_to_fd and prime_fd_to_handle callbacks are only needed > for the non-self-import case, where the other driver needs/provides a > scatter list. > Looking at kernel.org and drm-misc-next (as of a couple of hours ago) there is no drm_gem_prime.c file. I would imagine you meant drm_prime.c. Even then, the callbacks "prime_handle_to_fd" and "prime_fd_to_handle" are used by the ioctls themselves [1] [2]. There is no fall-back when the callbacks are missing - we error out with ENOSYS. I assume that by "callbacks are only needed" you meant that we should fix the ioctls to use the callbacks only for the non-self-import case? If so, that should land first before pulling out the prime callbacks, right? HTH Emil [1] https://cgit.freedesktop.org/drm/drm-misc/tree/drivers/gpu/drm/drm_prime.c?id=9158e3c31163488364c76bf6948507e7640d511f#n870 [2] https://cgit.freedesktop.org/drm/drm-misc/tree/drivers/gpu/drm/drm_prime.c?id=9158e3c31163488364c76bf6948507e7640d511f#n889
On Wed, 24 Apr 2019 at 02:18, Emil Velikov <emil.l.velikov@gmail.com> wrote: > > On Tue, 23 Apr 2019 at 15:06, Gerd Hoffmann <kraxel@redhat.com> wrote: > > > > On Tue, Apr 23, 2019 at 12:55:07PM +0100, Emil Velikov wrote: > > > On Tue, 23 Apr 2019 at 09:05, Gerd Hoffmann <kraxel@redhat.com> wrote: > > > > > > > > Hi, > > > > > > > > > > The DRI3 code extensively uses prime_handle_to_fd and > > > > > > prime_fd_to_handle for self-import. > > > > > > > > The callbacks are not used for self-import. Btw for drm-fixes I'm reverting this as it is a userspace regression. I've dropped the warn prints in the revert, I think userspace should be fixed, and/or kernel space but without regression behaviour. Dave.
diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.c b/drivers/gpu/drm/virtio/virtgpu_drv.c index b996ac1d4fcc..af92964b6889 100644 --- a/drivers/gpu/drm/virtio/virtgpu_drv.c +++ b/drivers/gpu/drm/virtio/virtgpu_drv.c @@ -205,10 +205,14 @@ static struct drm_driver driver = { #if defined(CONFIG_DEBUG_FS) .debugfs_init = virtio_gpu_debugfs_init, #endif + .prime_handle_to_fd = drm_gem_prime_handle_to_fd, + .prime_fd_to_handle = drm_gem_prime_fd_to_handle, .gem_prime_export = drm_gem_prime_export, .gem_prime_import = drm_gem_prime_import, .gem_prime_pin = virtgpu_gem_prime_pin, .gem_prime_unpin = virtgpu_gem_prime_unpin, + .gem_prime_get_sg_table = virtgpu_gem_prime_get_sg_table, + .gem_prime_import_sg_table = virtgpu_gem_prime_import_sg_table, .gem_prime_vmap = virtgpu_gem_prime_vmap, .gem_prime_vunmap = virtgpu_gem_prime_vunmap, .gem_prime_mmap = virtgpu_gem_prime_mmap, diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h b/drivers/gpu/drm/virtio/virtgpu_drv.h index 3238fdf58eb4..d577cb76f5ad 100644 --- a/drivers/gpu/drm/virtio/virtgpu_drv.h +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h @@ -354,6 +354,10 @@ int virtio_gpu_object_wait(struct virtio_gpu_object *bo, bool no_wait); /* virtgpu_prime.c */ int virtgpu_gem_prime_pin(struct drm_gem_object *obj); void virtgpu_gem_prime_unpin(struct drm_gem_object *obj); +struct sg_table *virtgpu_gem_prime_get_sg_table(struct drm_gem_object *obj); +struct drm_gem_object *virtgpu_gem_prime_import_sg_table( + struct drm_device *dev, struct dma_buf_attachment *attach, + struct sg_table *sgt); void *virtgpu_gem_prime_vmap(struct drm_gem_object *obj); void virtgpu_gem_prime_vunmap(struct drm_gem_object *obj, void *vaddr); int virtgpu_gem_prime_mmap(struct drm_gem_object *obj, diff --git a/drivers/gpu/drm/virtio/virtgpu_prime.c b/drivers/gpu/drm/virtio/virtgpu_prime.c index c59ec34c80a5..86ce0ae93f59 100644 --- a/drivers/gpu/drm/virtio/virtgpu_prime.c +++ b/drivers/gpu/drm/virtio/virtgpu_prime.c @@ -39,6 +39,20 @@ void virtgpu_gem_prime_unpin(struct drm_gem_object *obj) WARN_ONCE(1, "not implemented"); } +struct sg_table *virtgpu_gem_prime_get_sg_table(struct drm_gem_object *obj) +{ + WARN_ONCE(1, "not implemented"); + return ERR_PTR(-ENODEV); +} + +struct drm_gem_object *virtgpu_gem_prime_import_sg_table( + struct drm_device *dev, struct dma_buf_attachment *attach, + struct sg_table *table) +{ + WARN_ONCE(1, "not implemented"); + return ERR_PTR(-ENODEV); +} + void *virtgpu_gem_prime_vmap(struct drm_gem_object *obj) { struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
This patch does more harm than good, as it breaks both Xwayland and gnome-shell with X11. Xwayland requires DRI3 & DRI3 requires PRIME. X11 crash for obscure double-free reason which are hard to debug (starting X11 by hand doesn't trigger the crash). I don't see an apparent problem implementing those stub prime functions, they may return an error at run-time, and it seems to be handled fine by GNOME at least. Please consider a revert. This reverts commit b318e3ff7ca065d6b107e424c85a63d7a6798a69. Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Dave Airlie <airlied@redhat.com> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- drivers/gpu/drm/virtio/virtgpu_drv.c | 4 ++++ drivers/gpu/drm/virtio/virtgpu_drv.h | 4 ++++ drivers/gpu/drm/virtio/virtgpu_prime.c | 14 ++++++++++++++ 3 files changed, 22 insertions(+)