在 2025/5/9 14:51, Thomas Zimmermann 写道:
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
+
Name DRM_GEM_SHMEM_DRIVER_OPS_NO_MAP or
DRM_GEM_SHMEM_DRIVER_OPS_NO_MAP_SGT ?
Best regards
Shixiong Ou.
#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);
