Virtualization - Apr 2019 - [Spice-devel] [PATCH] Revert "drm/qxl: drop prime import/export callbacks"

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 at redhat.com>
---
 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 7:33 AM Gerd Hoffmann <kraxel at 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 at 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
>

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

On 4/26/19 4:21 PM, Daniel Vetter wrote:
> On Fri, Apr 26, 2019 at 7:33 AM Gerd Hoffmann <kraxel at 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 at 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 04:21:37PM +0200, Daniel Vetter
wrote:
> On Fri, Apr 26, 2019 at 7:33 AM Gerd Hoffmann <kraxel at 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 at 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