Message ID | 20190426053324.26443-1-kraxel@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Revert "drm/qxl: drop prime import/export callbacks" | expand |
On Fri, Apr 26, 2019 at 7:33 AM Gerd Hoffmann <kraxel@redhat.com> wrote: > > This reverts commit f4c34b1e2a37d5676180901fa6ff188bcb6371f8. > > Simliar to commit a0cecc23cfcb Revert "drm/virtio: drop prime > import/export callbacks". We have to do the same with qxl, > for the same reasons (it breaks DRI3). > > Drop the WARN_ON_ONCE(). > > Fixes: f4c34b1e2a37d5676180901fa6ff188bcb6371f8 > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> Maybe we need some helpers for virtual drivers which only allow self-reimport and nothing else at all? I think there's qxl, virgl, vmwgfx and maybe also vbox one who could use this ... Just a quick idea. -Daniel > --- > drivers/gpu/drm/qxl/qxl_drv.c | 4 ++++ > drivers/gpu/drm/qxl/qxl_prime.c | 12 ++++++++++++ > 2 files changed, 16 insertions(+) > > diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c > index 578d867a81d5..f33e349c4ec5 100644 > --- a/drivers/gpu/drm/qxl/qxl_drv.c > +++ b/drivers/gpu/drm/qxl/qxl_drv.c > @@ -255,10 +255,14 @@ static struct drm_driver qxl_driver = { > #if defined(CONFIG_DEBUG_FS) > .debugfs_init = qxl_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 = qxl_gem_prime_pin, > .gem_prime_unpin = qxl_gem_prime_unpin, > + .gem_prime_get_sg_table = qxl_gem_prime_get_sg_table, > + .gem_prime_import_sg_table = qxl_gem_prime_import_sg_table, > .gem_prime_vmap = qxl_gem_prime_vmap, > .gem_prime_vunmap = qxl_gem_prime_vunmap, > .gem_prime_mmap = qxl_gem_prime_mmap, > diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c > index 8b448eca1cd9..114653b471c6 100644 > --- a/drivers/gpu/drm/qxl/qxl_prime.c > +++ b/drivers/gpu/drm/qxl/qxl_prime.c > @@ -42,6 +42,18 @@ void qxl_gem_prime_unpin(struct drm_gem_object *obj) > qxl_bo_unpin(bo); > } > > +struct sg_table *qxl_gem_prime_get_sg_table(struct drm_gem_object *obj) > +{ > + return ERR_PTR(-ENOSYS); > +} > + > +struct drm_gem_object *qxl_gem_prime_import_sg_table( > + struct drm_device *dev, struct dma_buf_attachment *attach, > + struct sg_table *table) > +{ > + return ERR_PTR(-ENOSYS); > +} > + > void *qxl_gem_prime_vmap(struct drm_gem_object *obj) > { > struct qxl_bo *bo = gem_to_qxl_bo(obj); > -- > 2.18.1 >
On 4/26/19 4:21 PM, Daniel Vetter wrote: > On Fri, Apr 26, 2019 at 7:33 AM Gerd Hoffmann <kraxel@redhat.com> wrote: >> This reverts commit f4c34b1e2a37d5676180901fa6ff188bcb6371f8. >> >> Simliar to commit a0cecc23cfcb Revert "drm/virtio: drop prime >> import/export callbacks". We have to do the same with qxl, >> for the same reasons (it breaks DRI3). >> >> Drop the WARN_ON_ONCE(). >> >> Fixes: f4c34b1e2a37d5676180901fa6ff188bcb6371f8 >> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > Maybe we need some helpers for virtual drivers which only allow > self-reimport and nothing else at all? I think there's qxl, virgl, > vmwgfx and maybe also vbox one who could use this ... Just a quick > idea. > -Daniel I think vmwgfx could, in theory, support the full range of operations, at least for reasonably recent device versions. However, it wouldn't be terribly efficient since the exported dma-buf sglist would basically be a bounce-buffer. /Thomas >> --- >> drivers/gpu/drm/qxl/qxl_drv.c | 4 ++++ >> drivers/gpu/drm/qxl/qxl_prime.c | 12 ++++++++++++ >> 2 files changed, 16 insertions(+) >> >> diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c >> index 578d867a81d5..f33e349c4ec5 100644 >> --- a/drivers/gpu/drm/qxl/qxl_drv.c >> +++ b/drivers/gpu/drm/qxl/qxl_drv.c >> @@ -255,10 +255,14 @@ static struct drm_driver qxl_driver = { >> #if defined(CONFIG_DEBUG_FS) >> .debugfs_init = qxl_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 = qxl_gem_prime_pin, >> .gem_prime_unpin = qxl_gem_prime_unpin, >> + .gem_prime_get_sg_table = qxl_gem_prime_get_sg_table, >> + .gem_prime_import_sg_table = qxl_gem_prime_import_sg_table, >> .gem_prime_vmap = qxl_gem_prime_vmap, >> .gem_prime_vunmap = qxl_gem_prime_vunmap, >> .gem_prime_mmap = qxl_gem_prime_mmap, >> diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c >> index 8b448eca1cd9..114653b471c6 100644 >> --- a/drivers/gpu/drm/qxl/qxl_prime.c >> +++ b/drivers/gpu/drm/qxl/qxl_prime.c >> @@ -42,6 +42,18 @@ void qxl_gem_prime_unpin(struct drm_gem_object *obj) >> qxl_bo_unpin(bo); >> } >> >> +struct sg_table *qxl_gem_prime_get_sg_table(struct drm_gem_object *obj) >> +{ >> + return ERR_PTR(-ENOSYS); >> +} >> + >> +struct drm_gem_object *qxl_gem_prime_import_sg_table( >> + struct drm_device *dev, struct dma_buf_attachment *attach, >> + struct sg_table *table) >> +{ >> + return ERR_PTR(-ENOSYS); >> +} >> + >> void *qxl_gem_prime_vmap(struct drm_gem_object *obj) >> { >> struct qxl_bo *bo = gem_to_qxl_bo(obj); >> -- >> 2.18.1 >> >
On Fri, Apr 26, 2019 at 05:42:23PM +0200, Thomas Hellstrom wrote: > On 4/26/19 4:21 PM, Daniel Vetter wrote: > > On Fri, Apr 26, 2019 at 7:33 AM Gerd Hoffmann <kraxel@redhat.com> wrote: > > > This reverts commit f4c34b1e2a37d5676180901fa6ff188bcb6371f8. > > > > > > Simliar to commit a0cecc23cfcb Revert "drm/virtio: drop prime > > > import/export callbacks". We have to do the same with qxl, > > > for the same reasons (it breaks DRI3). > > > > > > Drop the WARN_ON_ONCE(). > > > > > > Fixes: f4c34b1e2a37d5676180901fa6ff188bcb6371f8 > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > Maybe we need some helpers for virtual drivers which only allow > > self-reimport and nothing else at all? I think there's qxl, virgl, > > vmwgfx and maybe also vbox one who could use this ... Just a quick > > idea. > > -Daniel > > I think vmwgfx could, in theory, support the full range of operations, > at least for reasonably recent device versions. However, it wouldn't be > terribly efficient since the exported dma-buf sglist would basically be a > bounce-buffer. Yeah not sure that makes sense to support really ... -Daniel > > /Thomas > > > > > --- > > > drivers/gpu/drm/qxl/qxl_drv.c | 4 ++++ > > > drivers/gpu/drm/qxl/qxl_prime.c | 12 ++++++++++++ > > > 2 files changed, 16 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c > > > index 578d867a81d5..f33e349c4ec5 100644 > > > --- a/drivers/gpu/drm/qxl/qxl_drv.c > > > +++ b/drivers/gpu/drm/qxl/qxl_drv.c > > > @@ -255,10 +255,14 @@ static struct drm_driver qxl_driver = { > > > #if defined(CONFIG_DEBUG_FS) > > > .debugfs_init = qxl_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 = qxl_gem_prime_pin, > > > .gem_prime_unpin = qxl_gem_prime_unpin, > > > + .gem_prime_get_sg_table = qxl_gem_prime_get_sg_table, > > > + .gem_prime_import_sg_table = qxl_gem_prime_import_sg_table, > > > .gem_prime_vmap = qxl_gem_prime_vmap, > > > .gem_prime_vunmap = qxl_gem_prime_vunmap, > > > .gem_prime_mmap = qxl_gem_prime_mmap, > > > diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c > > > index 8b448eca1cd9..114653b471c6 100644 > > > --- a/drivers/gpu/drm/qxl/qxl_prime.c > > > +++ b/drivers/gpu/drm/qxl/qxl_prime.c > > > @@ -42,6 +42,18 @@ void qxl_gem_prime_unpin(struct drm_gem_object *obj) > > > qxl_bo_unpin(bo); > > > } > > > > > > +struct sg_table *qxl_gem_prime_get_sg_table(struct drm_gem_object *obj) > > > +{ > > > + return ERR_PTR(-ENOSYS); > > > +} > > > + > > > +struct drm_gem_object *qxl_gem_prime_import_sg_table( > > > + struct drm_device *dev, struct dma_buf_attachment *attach, > > > + struct sg_table *table) > > > +{ > > > + return ERR_PTR(-ENOSYS); > > > +} > > > + > > > void *qxl_gem_prime_vmap(struct drm_gem_object *obj) > > > { > > > struct qxl_bo *bo = gem_to_qxl_bo(obj); > > > -- > > > 2.18.1 > > > > > >
On Fri, Apr 26, 2019 at 04:21:37PM +0200, Daniel Vetter wrote: > On Fri, Apr 26, 2019 at 7:33 AM Gerd Hoffmann <kraxel@redhat.com> wrote: > > > > This reverts commit f4c34b1e2a37d5676180901fa6ff188bcb6371f8. > > > > Simliar to commit a0cecc23cfcb Revert "drm/virtio: drop prime > > import/export callbacks". We have to do the same with qxl, > > for the same reasons (it breaks DRI3). > > > > Drop the WARN_ON_ONCE(). > > > > Fixes: f4c34b1e2a37d5676180901fa6ff188bcb6371f8 > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > Maybe we need some helpers for virtual drivers which only allow > self-reimport and nothing else at all? The current gem prime helpers handle this reasonable well, I don't see a need for that. More useful would be some way to signal this self-reimport capability to userspace somehow. See DRM_PRIME_CAP_LOCAL patch. Right now I have the choice to set DRM_PRIME_CAP_{IMPORT,EXPORT}, in which case some userspace assumes it can do cross-driver export/import and trips over that not working. Or I do not set DRM_PRIME_CAP_{IMPORT,EXPORT}, which breaks DRI3 ... > I think there's qxl, virgl, export is implemented for virgl, and import should be possible too. qxl/bochs is simliar to vmgfx, it could be done but dma-bufs would basically be bounce buffers (need copy from/to vram) so probably not worth the effort. cheers, Gerd
On Mon, Apr 29, 2019 at 9:54 AM Gerd Hoffmann <kraxel@redhat.com> wrote: > > On Fri, Apr 26, 2019 at 04:21:37PM +0200, Daniel Vetter wrote: > > On Fri, Apr 26, 2019 at 7:33 AM Gerd Hoffmann <kraxel@redhat.com> wrote: > > > > > > This reverts commit f4c34b1e2a37d5676180901fa6ff188bcb6371f8. > > > > > > Simliar to commit a0cecc23cfcb Revert "drm/virtio: drop prime > > > import/export callbacks". We have to do the same with qxl, > > > for the same reasons (it breaks DRI3). > > > > > > Drop the WARN_ON_ONCE(). > > > > > > Fixes: f4c34b1e2a37d5676180901fa6ff188bcb6371f8 > > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > > > > Maybe we need some helpers for virtual drivers which only allow > > self-reimport and nothing else at all? > > The current gem prime helpers handle this reasonable well, > I don't see a need for that. > > More useful would be some way to signal this self-reimport capability > to userspace somehow. See DRM_PRIME_CAP_LOCAL patch. Userspace is supposed to test whether import/export works for a specific combo, not blindly assume it does and then keel over. I think we need to fix that, not add more flags - there's lots of reasons why a given pair of devices can't share buffers (e.g. all the contiguous allocations stuff on socs). > Right now I have the choice to set DRM_PRIME_CAP_{IMPORT,EXPORT}, in > which case some userspace assumes it can do cross-driver export/import > and trips over that not working. Or I do not set > DRM_PRIME_CAP_{IMPORT,EXPORT}, which breaks DRI3 ... Yeah that's not an option. -Daniel > > I think there's qxl, virgl, > > export is implemented for virgl, and import should be possible too. > > qxl/bochs is simliar to vmgfx, it could be done but dma-bufs would > basically be bounce buffers (need copy from/to vram) so probably not > worth the effort. > > cheers, > Gerd >
Hi, > > More useful would be some way to signal this self-reimport capability > > to userspace somehow. See DRM_PRIME_CAP_LOCAL patch. > > Userspace is supposed to test whether import/export works for a > specific combo, not blindly assume it does and then keel over. I think > we need to fix that, not add more flags - there's lots of reasons why > a given pair of devices can't share buffers (e.g. all the contiguous > allocations stuff on socs). Ok. Lets scratch the DRM_PRIME_CAP_LOCAL idea then and blame userspace instead. > > Right now I have the choice to set DRM_PRIME_CAP_{IMPORT,EXPORT}, in > > which case some userspace assumes it can do cross-driver export/import > > and trips over that not working. Or I do not set > > DRM_PRIME_CAP_{IMPORT,EXPORT}, which breaks DRI3 ... > > Yeah that's not an option. Good. Can I get an ack for this patch then, as it unbreaks DRI3 with qxl? thanks, Gerd
On Mon, Apr 29, 2019 at 4:38 PM Gerd Hoffmann <kraxel@redhat.com> wrote: > > Hi, > > > > More useful would be some way to signal this self-reimport capability > > > to userspace somehow. See DRM_PRIME_CAP_LOCAL patch. > > > > Userspace is supposed to test whether import/export works for a > > specific combo, not blindly assume it does and then keel over. I think > > we need to fix that, not add more flags - there's lots of reasons why > > a given pair of devices can't share buffers (e.g. all the contiguous > > allocations stuff on socs). > > Ok. Lets scratch the DRM_PRIME_CAP_LOCAL idea then and blame userspace > instead. > > > > Right now I have the choice to set DRM_PRIME_CAP_{IMPORT,EXPORT}, in > > > which case some userspace assumes it can do cross-driver export/import > > > and trips over that not working. Or I do not set > > > DRM_PRIME_CAP_{IMPORT,EXPORT}, which breaks DRI3 ... > > > > Yeah that's not an option. > > Good. Can I get an ack for this patch then, as it unbreaks DRI3 with qxl? Oh sure, acked-by: me. Sorry forgot to supply that ... -Daniel
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c index 578d867a81d5..f33e349c4ec5 100644 --- a/drivers/gpu/drm/qxl/qxl_drv.c +++ b/drivers/gpu/drm/qxl/qxl_drv.c @@ -255,10 +255,14 @@ static struct drm_driver qxl_driver = { #if defined(CONFIG_DEBUG_FS) .debugfs_init = qxl_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 = qxl_gem_prime_pin, .gem_prime_unpin = qxl_gem_prime_unpin, + .gem_prime_get_sg_table = qxl_gem_prime_get_sg_table, + .gem_prime_import_sg_table = qxl_gem_prime_import_sg_table, .gem_prime_vmap = qxl_gem_prime_vmap, .gem_prime_vunmap = qxl_gem_prime_vunmap, .gem_prime_mmap = qxl_gem_prime_mmap, diff --git a/drivers/gpu/drm/qxl/qxl_prime.c b/drivers/gpu/drm/qxl/qxl_prime.c index 8b448eca1cd9..114653b471c6 100644 --- a/drivers/gpu/drm/qxl/qxl_prime.c +++ b/drivers/gpu/drm/qxl/qxl_prime.c @@ -42,6 +42,18 @@ void qxl_gem_prime_unpin(struct drm_gem_object *obj) qxl_bo_unpin(bo); } +struct sg_table *qxl_gem_prime_get_sg_table(struct drm_gem_object *obj) +{ + return ERR_PTR(-ENOSYS); +} + +struct drm_gem_object *qxl_gem_prime_import_sg_table( + struct drm_device *dev, struct dma_buf_attachment *attach, + struct sg_table *table) +{ + return ERR_PTR(-ENOSYS); +} + void *qxl_gem_prime_vmap(struct drm_gem_object *obj) { struct qxl_bo *bo = gem_to_qxl_bo(obj);
This reverts commit f4c34b1e2a37d5676180901fa6ff188bcb6371f8. Simliar to commit a0cecc23cfcb Revert "drm/virtio: drop prime import/export callbacks". We have to do the same with qxl, for the same reasons (it breaks DRI3). Drop the WARN_ON_ONCE(). Fixes: f4c34b1e2a37d5676180901fa6ff188bcb6371f8 Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- drivers/gpu/drm/qxl/qxl_drv.c | 4 ++++ drivers/gpu/drm/qxl/qxl_prime.c | 12 ++++++++++++ 2 files changed, 16 insertions(+)