Hi Am 09.05.25 um 08:22 schrieb Christian König:
On 5/9/25 05:20, oushixiong1...@163.com wrote:From: Shixiong Ou <oushixi...@kylinos.cn>[WHY] 1. Drivers using DRM_GEM_SHADOW_PLANE_HELPER_FUNCS and DRM_GEM_SHMEM_DRIVER_OPS (e.g., udl, ast) do not require sg_table import. They only need dma_buf_vmap() to access the shared buffer's kernel virtual address. 2. On certain Aspeed-based boards, a dma_mask of 0xffff_ffff may trigger SWIOTLB during dmabuf import. However, IO_TLB_SEGSIZE restricts the maximum DMA streaming mapping memory, resulting in errors like: ast 0000:07:00.0: swiotlb buffer is full (sz: 3145728 bytes), total 32768 (slots), used 0 (slots) [HOW] Provide a gem_prime_import implementation without sg_table mapping to avoid issues (e.g., "swiotlb buffer is full"). Drivers that do not require sg_table can adopt this. Signed-off-by: Shixiong Ou <oushixi...@kylinos.cn> --- v1->v2: Patch rebase. v2->v3: Rename the import callback function. Remove drm_gem_shmem_prime_export() and separate some codes to drm_gem_prime_import_self(). v3->v4: Separate the test from the policy. Rename the macro. drivers/gpu/drm/drm_gem_shmem_helper.c | 57 ++++++++++++++++++++++++++ drivers/gpu/drm/drm_prime.c | 36 ++++++++++++---- include/drm/drm_gem_shmem_helper.h | 15 +++++++ include/drm/drm_prime.h | 3 ++ 4 files changed, 102 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index aa43265f4f4f..8fa160c3635e 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -800,6 +800,63 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev, } EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table);+/**+ * drm_gem_shmem_prime_import_no_sgt - Import dmabuf without mapping its sg_table + * @dev: Device to import into + * @dma_buf: dma-buf object to import + * + * Drivers that use the shmem helpers but also wants to import dmabuf without + * mapping its sg_table can use this as their &drm_driver.gem_prime_import + * implementation. + */ +struct drm_gem_object *drm_gem_shmem_prime_import_no_sgt(struct drm_device *dev, + struct dma_buf *dma_buf)Please don't mention "no sgt" in the name, that we use sgtable is an implementation detail. Maybe use something like "no map" or similar.
To be fair, I asked to not named it something like _vmap(), but rather _no_sgt(). These vmap-only names purely describe a use case. I'd be OK with any name that refers to the difference between the various functions; not just their effect.
+{ + struct dma_buf_attachment *attach; + struct drm_gem_shmem_object *shmem; + struct drm_gem_object *obj; + size_t size; + int ret; + + if (drm_gem_prime_exported_dma_buf(dev, dma_buf)) { + /* + * Importing dmabuf exported from our own gem increases + * refcount on gem itself instead of f_count of dmabuf. + */ + obj = dma_buf->priv; + drm_gem_object_get(obj); + return obj; + } + + attach = dma_buf_attach(dma_buf, dev->dev); + if (IS_ERR(attach)) + return ERR_CAST(attach); + + get_dma_buf(dma_buf);
+ + size = PAGE_ALIGN(attach->dmabuf->size); + + shmem = __drm_gem_shmem_create(dev, size, true, NULL); + if (IS_ERR(shmem)) { + ret = PTR_ERR(shmem); + goto fail_detach; + }
Unrelated to this series, we might reconsider drm_driver.gem_prime_import_sg_table. If we move the call to dma_buf_map_attachment_unlocked() into the callback and rename it to gem_prime_import_attachment, using sg tables would become optional for all drivers. SHMEM would be able to create the object without SG table without having to reimplement the prime boiler-plate code. Thoughts?
+ + drm_dbg_prime(dev, "size = %zu\n", size); + + shmem->base.import_attach = attach; + shmem->base.resv = dma_buf->resv; + + return &shmem->base; + +fail_detach: + dma_buf_detach(dma_buf, attach); + dma_buf_put(dma_buf); + + return ERR_PTR(ret); +} +EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_no_sgt); + MODULE_DESCRIPTION("DRM SHMEM memory-management helpers"); MODULE_IMPORT_NS("DMA_BUF"); MODULE_LICENSE("GPL v2"); diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index d828502268b8..5bcf483520b8 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -910,6 +910,26 @@ struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj, } EXPORT_SYMBOL(drm_gem_prime_export);++/** + * drm_gem_prime_exported_dma_buf - + * checks if the DMA-BUF was exported from a GEM object belonging to @dev. + * @dev: drm_device to check against + * @dma_buf: dma-buf object to import + * + * Return: true if the DMA-BUF was exported from a GEM object belonging + * to @dev, false otherwise. + */ + +bool drm_gem_prime_exported_dma_buf(struct drm_device *dev, + struct dma_buf *dma_buf)If I remember the naming conventions correctly this should be something like drm_gem_is_prime_exported_dma_buf().
Again my fault. But the name you suggested sounds correct. Best regards Thomas
Regards, Christian.+{ + struct drm_gem_object *obj = dma_buf->priv; + + return (dma_buf->ops == &drm_gem_prime_dmabuf_ops) && (obj->dev == dev); +} +EXPORT_SYMBOL(drm_gem_prime_exported_dma_buf); + /** * drm_gem_prime_import_dev - core implementation of the import callback * @dev: drm_device to import into @@ -933,16 +953,14 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev, struct drm_gem_object *obj; int ret;- if (dma_buf->ops == &drm_gem_prime_dmabuf_ops) {+ if (drm_gem_prime_exported_dma_buf(dev, dma_buf)) { + /* + * Importing dmabuf exported from our own gem increases + * refcount on gem itself instead of f_count of dmabuf. + */ obj = dma_buf->priv; - if (obj->dev == dev) { - /* - * Importing dmabuf exported from our own gem increases - * refcount on gem itself instead of f_count of dmabuf. - */ - drm_gem_object_get(obj); - return obj; - } + drm_gem_object_get(obj); + return obj; }if (!dev->driver->gem_prime_import_sg_table)diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h index b4f993da3cae..9ee697ff52ea 100644 --- a/include/drm/drm_gem_shmem_helper.h +++ b/include/drm/drm_gem_shmem_helper.h @@ -287,6 +287,8 @@ drm_gem_shmem_prime_import_sg_table(struct drm_device *dev, struct sg_table *sgt); int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev, struct drm_mode_create_dumb *args); +struct drm_gem_object *drm_gem_shmem_prime_import_no_sgt(struct drm_device *dev, + struct dma_buf *buf);/*** DRM_GEM_SHMEM_DRIVER_OPS - Default shmem GEM operations @@ -298,4 +300,17 @@ int drm_gem_shmem_dumb_create(struct drm_file *file, struct drm_device *dev, .gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table, \ .dumb_create = drm_gem_shmem_dumb_create+/**+ * DRM_GEM_SHMEM_DRIVER_OPS_NO_SGT - shmem GEM operations + * without mapping sg_table on + * imported buffer. + * + * This macro provides a shortcut for setting the shmem GEM operations in + * the &drm_driver structure for drivers that do not require a sg_table on + * imported buffers. + */ +#define DRM_GEM_SHMEM_DRIVER_OPS_NO_SGT \ + .gem_prime_import = drm_gem_shmem_prime_import_no_sgt, \ + .dumb_create = drm_gem_shmem_dumb_create + #endif /* __DRM_GEM_SHMEM_HELPER_H__ */ diff --git a/include/drm/drm_prime.h b/include/drm/drm_prime.h index fa085c44d4ca..ec83f1c077a4 100644 --- a/include/drm/drm_prime.h +++ b/include/drm/drm_prime.h @@ -100,6 +100,9 @@ struct dma_buf *drm_gem_prime_export(struct drm_gem_object *obj, unsigned long drm_prime_get_contiguous_size(struct sg_table *sgt);/* helper functions for importing */+bool drm_gem_prime_exported_dma_buf(struct drm_device *dev, + struct dma_buf *dma_buf); + struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev, struct dma_buf *dma_buf, struct device *attach_dev);
-- -- 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)
