[PATCH v2 1/2] drm/prime: reject DMA-BUF attach when get_sg_table is missing

[PATCH v2 1/2] drm/prime: reject DMA-BUF attach when get_sg_table is missing

[Simon Ser]

drm_gem_map_dma_buf() requires drm_gem_object_funcs.get_sg_table
to be implemented, or else WARNs.

Allow drivers to leave this hook unimplemented to implement purely
local DMA-BUFs (ie, DMA-BUFs which cannot be imported anywhere
else but the device which allocated them). In that case, reject
imports to other devices in drm_gem_map_attach().

v2: new patch

Signed-off-by: Simon Ser <contact@emersion.fr>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Tian Tao <tiantao6@hisilicon.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Christian König <christian.koenig@amd.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/drm_prime.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index f924b8b4ab6b..ab1d21d63a03 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -544,7 +544,8 @@ int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
  * Optional pinning of buffers is handled at dma-buf attach and detach time in
  * drm_gem_map_attach() and drm_gem_map_detach(). Backing storage itself is
  * handled by drm_gem_map_dma_buf() and drm_gem_unmap_dma_buf(), which relies on
- * &drm_gem_object_funcs.get_sg_table.
+ * &drm_gem_object_funcs.get_sg_table. If &drm_gem_object_funcs.get_sg_table is
+ * unimplemented, exports into another device are rejected.
  *
  * For kernel-internal access there's drm_gem_dmabuf_vmap() and
  * drm_gem_dmabuf_vunmap(). Userspace mmap support is provided by
@@ -583,6 +584,9 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
 {
 	struct drm_gem_object *obj = dma_buf->priv;
 
+	if (!obj->funcs->get_sg_table)
+		return -EOPNOTSUPP;
+
 	return drm_gem_pin(obj);
 }
 EXPORT_SYMBOL(drm_gem_map_attach);
-- 
2.39.2

[PATCH v2 2/2] drm/vram-helper: turn on PRIME import/export

[Simon Ser]

We don't populate drm_driver.gem_prime_import_sg_table so only
DMA-BUFs exported from our own device can be imported. We don't
populate drm_gem_object_funcs.get_sg_table so DMA-BUFs cannot be
imported into another device. Still, this is useful to user-space
to share buffers between processes and between API boundaries
(e.g. wlroots hard-requires PRIME import/export support).

v2: expand commit message

Signed-off-by: Simon Ser <contact@emersion.fr>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Tian Tao <tiantao6@hisilicon.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Christian König <christian.koenig@amd.com>
Cc: Hans de Goede <hdegoede@redhat.com>
---
 include/drm/drm_gem_vram_helper.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
index d3e8920c0b64..f4aab64411d8 100644
--- a/include/drm/drm_gem_vram_helper.h
+++ b/include/drm/drm_gem_vram_helper.h
@@ -160,7 +160,9 @@ void drm_gem_vram_simple_display_pipe_cleanup_fb(
 	.debugfs_init             = drm_vram_mm_debugfs_init, \
 	.dumb_create		  = drm_gem_vram_driver_dumb_create, \
 	.dumb_map_offset	  = drm_gem_ttm_dumb_map_offset, \
-	.gem_prime_mmap		  = drm_gem_prime_mmap
+	.gem_prime_mmap		  = drm_gem_prime_mmap, \
+	.prime_handle_to_fd	  = drm_gem_prime_handle_to_fd, \
+	.prime_fd_to_handle	  = drm_gem_prime_fd_to_handle
 
 /*
  *  VRAM memory manager
-- 
2.39.2

Re: [PATCH v2 1/2] drm/prime: reject DMA-BUF attach when get_sg_table is missing

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2249 bytes --]

Hi

Am 02.03.23 um 15:35 schrieb Simon Ser:
> drm_gem_map_dma_buf() requires drm_gem_object_funcs.get_sg_table
> to be implemented, or else WARNs.
> 
> Allow drivers to leave this hook unimplemented to implement purely
> local DMA-BUFs (ie, DMA-BUFs which cannot be imported anywhere
> else but the device which allocated them). In that case, reject
> imports to other devices in drm_gem_map_attach().
> 
> v2: new patch
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Tian Tao <tiantao6@hisilicon.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/gpu/drm/drm_prime.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index f924b8b4ab6b..ab1d21d63a03 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -544,7 +544,8 @@ int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
>    * Optional pinning of buffers is handled at dma-buf attach and detach time in
>    * drm_gem_map_attach() and drm_gem_map_detach(). Backing storage itself is
>    * handled by drm_gem_map_dma_buf() and drm_gem_unmap_dma_buf(), which relies on
> - * &drm_gem_object_funcs.get_sg_table.
> + * &drm_gem_object_funcs.get_sg_table. If &drm_gem_object_funcs.get_sg_table is
> + * unimplemented, exports into another device are rejected.
>    *
>    * For kernel-internal access there's drm_gem_dmabuf_vmap() and
>    * drm_gem_dmabuf_vunmap(). Userspace mmap support is provided by
> @@ -583,6 +584,9 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
>   {
>   	struct drm_gem_object *obj = dma_buf->priv;
>   
> +	if (!obj->funcs->get_sg_table)
> +		return -EOPNOTSUPP;

-ENOSYS please.

Best regards
Thomas

> +
>   	return drm_gem_pin(obj);
>   }
>   EXPORT_SYMBOL(drm_gem_map_attach);

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 1/2] drm/prime: reject DMA-BUF attach when get_sg_table is missing

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2514 bytes --]

Hi

Am 02.03.23 um 16:03 schrieb Thomas Zimmermann:
> Hi
> 
> Am 02.03.23 um 15:35 schrieb Simon Ser:
>> drm_gem_map_dma_buf() requires drm_gem_object_funcs.get_sg_table
>> to be implemented, or else WARNs.
>>
>> Allow drivers to leave this hook unimplemented to implement purely
>> local DMA-BUFs (ie, DMA-BUFs which cannot be imported anywhere
>> else but the device which allocated them). In that case, reject
>> imports to other devices in drm_gem_map_attach().
>>
>> v2: new patch
>>
>> Signed-off-by: Simon Ser <contact@emersion.fr>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Tian Tao <tiantao6@hisilicon.com>
>> Cc: Maxime Ripard <maxime@cerno.tech>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/gpu/drm/drm_prime.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>> index f924b8b4ab6b..ab1d21d63a03 100644
>> --- a/drivers/gpu/drm/drm_prime.c
>> +++ b/drivers/gpu/drm/drm_prime.c
>> @@ -544,7 +544,8 @@ int drm_prime_handle_to_fd_ioctl(struct drm_device 
>> *dev, void *data,
>>    * Optional pinning of buffers is handled at dma-buf attach and 
>> detach time in
>>    * drm_gem_map_attach() and drm_gem_map_detach(). Backing storage 
>> itself is
>>    * handled by drm_gem_map_dma_buf() and drm_gem_unmap_dma_buf(), 
>> which relies on
>> - * &drm_gem_object_funcs.get_sg_table.
>> + * &drm_gem_object_funcs.get_sg_table. If 
>> &drm_gem_object_funcs.get_sg_table is
>> + * unimplemented, exports into another device are rejected.
>>    *
>>    * For kernel-internal access there's drm_gem_dmabuf_vmap() and
>>    * drm_gem_dmabuf_vunmap(). Userspace mmap support is provided by
>> @@ -583,6 +584,9 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
>>   {
>>       struct drm_gem_object *obj = dma_buf->priv;
>> +    if (!obj->funcs->get_sg_table)
>> +        return -EOPNOTSUPP;
> 
> -ENOSYS please.

With this changed:

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

> 
> Best regards
> Thomas
> 
>> +
>>       return drm_gem_pin(obj);
>>   }
>>   EXPORT_SYMBOL(drm_gem_map_attach);
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 2/2] drm/vram-helper: turn on PRIME import/export

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1870 bytes --]



Am 02.03.23 um 15:35 schrieb Simon Ser:
> We don't populate drm_driver.gem_prime_import_sg_table so only
> DMA-BUFs exported from our own device can be imported. We don't
> populate drm_gem_object_funcs.get_sg_table so DMA-BUFs cannot be
> imported into another device. Still, this is useful to user-space
> to share buffers between processes and between API boundaries
> (e.g. wlroots hard-requires PRIME import/export support).
> 
> v2: expand commit message
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Tian Tao <tiantao6@hisilicon.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> ---
>   include/drm/drm_gem_vram_helper.h | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/include/drm/drm_gem_vram_helper.h b/include/drm/drm_gem_vram_helper.h
> index d3e8920c0b64..f4aab64411d8 100644
> --- a/include/drm/drm_gem_vram_helper.h
> +++ b/include/drm/drm_gem_vram_helper.h
> @@ -160,7 +160,9 @@ void drm_gem_vram_simple_display_pipe_cleanup_fb(
>   	.debugfs_init             = drm_vram_mm_debugfs_init, \
>   	.dumb_create		  = drm_gem_vram_driver_dumb_create, \
>   	.dumb_map_offset	  = drm_gem_ttm_dumb_map_offset, \
> -	.gem_prime_mmap		  = drm_gem_prime_mmap
> +	.gem_prime_mmap		  = drm_gem_prime_mmap, \
> +	.prime_handle_to_fd	  = drm_gem_prime_handle_to_fd, \
> +	.prime_fd_to_handle	  = drm_gem_prime_fd_to_handle

Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

>   
>   /*
>    *  VRAM memory manager

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 1/2] drm/prime: reject DMA-BUF attach when get_sg_table is missing

[Christian König]

Am 02.03.23 um 15:35 schrieb Simon Ser:
> drm_gem_map_dma_buf() requires drm_gem_object_funcs.get_sg_table
> to be implemented, or else WARNs.
>
> Allow drivers to leave this hook unimplemented to implement purely
> local DMA-BUFs (ie, DMA-BUFs which cannot be imported anywhere
> else but the device which allocated them). In that case, reject
> imports to other devices in drm_gem_map_attach().
>
> v2: new patch
>
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Tian Tao <tiantao6@hisilicon.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Hans de Goede <hdegoede@redhat.com>

With Thomas comment addressed: Reviewed-by: Christian König 
<christian.koenig@amd.com> for the series.

> ---
>   drivers/gpu/drm/drm_prime.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index f924b8b4ab6b..ab1d21d63a03 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -544,7 +544,8 @@ int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
>    * Optional pinning of buffers is handled at dma-buf attach and detach time in
>    * drm_gem_map_attach() and drm_gem_map_detach(). Backing storage itself is
>    * handled by drm_gem_map_dma_buf() and drm_gem_unmap_dma_buf(), which relies on
> - * &drm_gem_object_funcs.get_sg_table.
> + * &drm_gem_object_funcs.get_sg_table. If &drm_gem_object_funcs.get_sg_table is
> + * unimplemented, exports into another device are rejected.
>    *
>    * For kernel-internal access there's drm_gem_dmabuf_vmap() and
>    * drm_gem_dmabuf_vunmap(). Userspace mmap support is provided by
> @@ -583,6 +584,9 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
>   {
>   	struct drm_gem_object *obj = dma_buf->priv;
>   
> +	if (!obj->funcs->get_sg_table)
> +		return -EOPNOTSUPP;
> +
>   	return drm_gem_pin(obj);
>   }
>   EXPORT_SYMBOL(drm_gem_map_attach);

Re: [PATCH v2 1/2] drm/prime: reject DMA-BUF attach when get_sg_table is missing

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2357 bytes --]

Hi Simon

Am 02.03.23 um 15:35 schrieb Simon Ser:
> drm_gem_map_dma_buf() requires drm_gem_object_funcs.get_sg_table
> to be implemented, or else WARNs.
> 
> Allow drivers to leave this hook unimplemented to implement purely
> local DMA-BUFs (ie, DMA-BUFs which cannot be imported anywhere
> else but the device which allocated them). In that case, reject
> imports to other devices in drm_gem_map_attach().
> 
> v2: new patch

Is there a v3 of this patchset?  It was Acked with the one errno code 
changed.

Best regards
Thomas

> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Tian Tao <tiantao6@hisilicon.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> ---
>   drivers/gpu/drm/drm_prime.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index f924b8b4ab6b..ab1d21d63a03 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -544,7 +544,8 @@ int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
>    * Optional pinning of buffers is handled at dma-buf attach and detach time in
>    * drm_gem_map_attach() and drm_gem_map_detach(). Backing storage itself is
>    * handled by drm_gem_map_dma_buf() and drm_gem_unmap_dma_buf(), which relies on
> - * &drm_gem_object_funcs.get_sg_table.
> + * &drm_gem_object_funcs.get_sg_table. If &drm_gem_object_funcs.get_sg_table is
> + * unimplemented, exports into another device are rejected.
>    *
>    * For kernel-internal access there's drm_gem_dmabuf_vmap() and
>    * drm_gem_dmabuf_vunmap(). Userspace mmap support is provided by
> @@ -583,6 +584,9 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
>   {
>   	struct drm_gem_object *obj = dma_buf->priv;
>   
> +	if (!obj->funcs->get_sg_table)
> +		return -EOPNOTSUPP;
> +
>   	return drm_gem_pin(obj);
>   }
>   EXPORT_SYMBOL(drm_gem_map_attach);

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 1/2] drm/prime: reject DMA-BUF attach when get_sg_table is missing

[Simon Ser]

Hi,

On Friday, June 9th, 2023 at 13:31, Thomas Zimmermann <tzimmermann@suse.de> wrote:

> Is there a v3 of this patchset? It was Acked with the one errno code
> changed.

Since this was a minor change, I did it locally and pushed the patch
to drm-misc-next already.

Simon

Re: [PATCH v2 1/2] drm/prime: reject DMA-BUF attach when get_sg_table is missing

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 664 bytes --]

Hi

Am 09.06.23 um 15:04 schrieb Simon Ser:
> Hi,
> 
> On Friday, June 9th, 2023 at 13:31, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> 
>> Is there a v3 of this patchset? It was Acked with the one errno code
>> changed.
> 
> Since this was a minor change, I did it locally and pushed the patch
> to drm-misc-next already.

Found it. Thanks for committing and sorry for the noise.

Best regards
Thomas

> 
> Simon

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2 1/2] drm/prime: reject DMA-BUF attach when get_sg_table is missing

[Rob Clark]

On Thu, Mar 2, 2023 at 6:35 AM Simon Ser <contact@emersion.fr> wrote:
>
> drm_gem_map_dma_buf() requires drm_gem_object_funcs.get_sg_table
> to be implemented, or else WARNs.
>
> Allow drivers to leave this hook unimplemented to implement purely
> local DMA-BUFs (ie, DMA-BUFs which cannot be imported anywhere
> else but the device which allocated them). In that case, reject
> imports to other devices in drm_gem_map_attach().
>
> v2: new patch
>
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Tian Tao <tiantao6@hisilicon.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/drm_prime.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index f924b8b4ab6b..ab1d21d63a03 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -544,7 +544,8 @@ int drm_prime_handle_to_fd_ioctl(struct drm_device *dev, void *data,
>   * Optional pinning of buffers is handled at dma-buf attach and detach time in
>   * drm_gem_map_attach() and drm_gem_map_detach(). Backing storage itself is
>   * handled by drm_gem_map_dma_buf() and drm_gem_unmap_dma_buf(), which relies on
> - * &drm_gem_object_funcs.get_sg_table.
> + * &drm_gem_object_funcs.get_sg_table. If &drm_gem_object_funcs.get_sg_table is
> + * unimplemented, exports into another device are rejected.
>   *
>   * For kernel-internal access there's drm_gem_dmabuf_vmap() and
>   * drm_gem_dmabuf_vunmap(). Userspace mmap support is provided by
> @@ -583,6 +584,9 @@ int drm_gem_map_attach(struct dma_buf *dma_buf,
>  {
>         struct drm_gem_object *obj = dma_buf->priv;
>
> +       if (!obj->funcs->get_sg_table)
> +               return -EOPNOTSUPP;

This breaks virtgpu, where buffers may not necessarily have guest
backing pages, but still may be shared with other virtual devices
(because the host side buffer _does_ have backing pages)

BR,
-R

> +
>         return drm_gem_pin(obj);
>  }
>  EXPORT_SYMBOL(drm_gem_map_attach);
> --
> 2.39.2
>
>