USB-based drivers cannot use DMA, so the importing of dma-buf attachments currently fails for udl and gm12u320. This breaks joining/mirroring of displays.
The fix is now a little series. To solve the issue on the importer side (i.e., the affected USB-based driver), patch 1 introduces a new PRIME callback, struct drm_driver.gem_prime_create_object, which creates an object and gives more control to the importing driver. Specifically, udl and gm12u320 can now avoid the creation of a scatter/gather table for the imported pages. Patch 1 is self-contained in the sense that it can be backported into older kernels.
Patches 2 and 3 update SHMEM and CMA helpers to use the new callback. Effectively this moves the sg table setup from the PRIME helpers into the memory managers. SHMEM now supports devices without DMA support, so custom code can be removed from udl and g12u320.
Tested by joining/mirroring displays of udl and radeon under Gnome/X11.
v2: * move fix to importer side (Christian, Daniel) * update SHMEM and CMA helpers for new PRIME callbacks
Thomas Zimmermann (3): drm: Support importing dmabufs into drivers without DMA drm/shmem-helper: Implement struct drm_driver.gem_prime_create_object drm/cma-helper: Implement struct drm_driver.gem_prime_create_object
drivers/gpu/drm/drm_gem_cma_helper.c | 62 ++++++++++++++----------- drivers/gpu/drm/drm_gem_shmem_helper.c | 38 ++++++++++----- drivers/gpu/drm/drm_prime.c | 43 +++++++++++------ drivers/gpu/drm/lima/lima_drv.c | 2 +- drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +- drivers/gpu/drm/panfrost/panfrost_gem.c | 6 +-- drivers/gpu/drm/panfrost/panfrost_gem.h | 4 +- drivers/gpu/drm/pl111/pl111_drv.c | 8 ++-- drivers/gpu/drm/v3d/v3d_bo.c | 6 +-- drivers/gpu/drm/v3d/v3d_drv.c | 2 +- drivers/gpu/drm/v3d/v3d_drv.h | 5 +- include/drm/drm_drv.h | 12 +++++ include/drm/drm_gem_cma_helper.h | 12 ++--- include/drm/drm_gem_shmem_helper.h | 6 +-- 14 files changed, 120 insertions(+), 88 deletions(-)
-- 2.30.1
USB devices cannot perform DMA and hence have no dma_mask set in their device structure. Importing dmabuf into a USB-based driver fails, which break joining and mirroring of display in X11. A corresponding error message is shown below.
[ 60.050199] ------------[ cut here ]------------ [ 60.055092] WARNING: CPU: 0 PID: 1403 at kernel/dma/mapping.c:190 dma_map_sg_attrs+0x8f/0xc0 [ 60.064331] Modules linked in: af_packet(E) rfkill(E) dmi_sysfs(E) intel_rapl_msr(E) intel_rapl_common(E) snd_hda_codec_realtek(E) snd_hda_codec_generic(E) ledtrig_audio(E) snd_hda_codec_hdmi(E) x86_pkg_temp_thermal(E) intel_powerclam) [ 60.064801] wmi(E) video(E) btrfs(E) blake2b_generic(E) libcrc32c(E) crc32c_intel(E) xor(E) raid6_pq(E) sg(E) dm_multipath(E) dm_mod(E) scsi_dh_rdac(E) scsi_dh_emc(E) scsi_dh_alua(E) msr(E) efivarfs(E) [ 60.170778] CPU: 0 PID: 1403 Comm: Xorg.bin Tainted: G E 5.11.0-rc5-1-default+ #747 [ 60.179841] Hardware name: Dell Inc. OptiPlex 9020/0N4YC8, BIOS A24 10/24/2018 [ 60.187145] RIP: 0010:dma_map_sg_attrs+0x8f/0xc0 [ 60.191822] Code: 4d 85 ff 75 2b 49 89 d8 44 89 e1 44 89 f2 4c 89 ee 48 89 ef e8 62 30 00 00 85 c0 78 36 5b 5d 41 5c 41 5d 41 5e 41 5f c3 0f 0b <0f> 0b 31 c0 eb ed 49 8d 7f 50 e8 72 f5 2a 00 49 8b 47 50 49 89 d8 [ 60.210770] RSP: 0018:ffffc90001d6fc18 EFLAGS: 00010246 [ 60.216062] RAX: 0000000000000000 RBX: 0000000000000020 RCX: ffffffffb31e677b [ 60.223274] RDX: dffffc0000000000 RSI: ffff888212c10600 RDI: ffff8881b08a9488 [ 60.230501] RBP: ffff8881b08a9030 R08: 0000000000000020 R09: ffff888212c10600 [ 60.237710] R10: ffffed10425820df R11: 0000000000000001 R12: 0000000000000000 [ 60.244939] R13: ffff888212c10600 R14: 0000000000000008 R15: 0000000000000000 [ 60.252155] FS: 00007f08ac2b2f00(0000) GS:ffff8887cbe00000(0000) knlGS:0000000000000000 [ 60.260333] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 60.266150] CR2: 000055831c899be8 CR3: 000000015372e006 CR4: 00000000001706f0 [ 60.273369] Call Trace: [ 60.275845] drm_gem_map_dma_buf+0xb4/0x120 [ 60.280089] dma_buf_map_attachment+0x15d/0x1e0 [ 60.284688] drm_gem_prime_import_dev.part.0+0x5d/0x140 [ 60.289984] drm_gem_prime_fd_to_handle+0x213/0x280 [ 60.294931] ? drm_prime_destroy_file_private+0x30/0x30 [ 60.300224] drm_ioctl_kernel+0x131/0x180 [ 60.304291] ? drm_setversion+0x230/0x230 [ 60.308366] ? drm_prime_destroy_file_private+0x30/0x30 [ 60.313659] drm_ioctl+0x2f1/0x500 [ 60.317118] ? drm_version+0x150/0x150 [ 60.320903] ? lock_downgrade+0xa0/0xa0 [ 60.324806] ? do_vfs_ioctl+0x5f4/0x680 [ 60.328694] ? __fget_files+0x13e/0x210 [ 60.332591] ? ioctl_fiemap.isra.0+0x1a0/0x1a0 [ 60.337102] ? __fget_files+0x15d/0x210 [ 60.340990] __x64_sys_ioctl+0xb9/0xf0 [ 60.344795] do_syscall_64+0x33/0x80 [ 60.348427] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 60.353542] RIP: 0033:0x7f08ac7b53cb [ 60.357168] Code: 89 d8 49 8d 3c 1c 48 f7 d8 49 39 c4 72 b5 e8 1c ff ff ff 85 c0 78 ba 4c 89 e0 5b 5d 41 5c c3 f3 0f 1e fa b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 75 ba 0c 00 f7 d8 64 89 01 48 [ 60.376108] RSP: 002b:00007ffeabc89fc8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010 [ 60.383758] RAX: ffffffffffffffda RBX: 00007ffeabc8a00c RCX: 00007f08ac7b53cb [ 60.390970] RDX: 00007ffeabc8a00c RSI: 00000000c00c642e RDI: 000000000000000d [ 60.398221] RBP: 00000000c00c642e R08: 000055831c691d00 R09: 000055831b2ec010 [ 60.405446] R10: 00007f08acf6ada0 R11: 0000000000000246 R12: 000055831c691d00 [ 60.412667] R13: 000000000000000d R14: 00000000007e9000 R15: 0000000000000000 [ 60.419903] irq event stamp: 672893 [ 60.423441] hardirqs last enabled at (672913): [<ffffffffb31b796d>] console_unlock+0x44d/0x500 [ 60.432230] hardirqs last disabled at (672922): [<ffffffffb31b7963>] console_unlock+0x443/0x500 [ 60.441021] softirqs last enabled at (672940): [<ffffffffb46003dd>] __do_softirq+0x3dd/0x554 [ 60.449634] softirqs last disabled at (672931): [<ffffffffb44010f2>] asm_call_irq_on_stack+0x12/0x20 [ 60.458871] ---[ end trace f2f88696eb17806c ]---
This patch introduces struct drm_driver.gem_prime_create_object, which creates a GEM object during the import. Drivers should implement this callback and handle DMA S/G table support by themselves. For USB-based drivers, the patch adds an implementation without DMA-related code.
The original interface struct drm_driver.gem_prime_import_sg_table is deprecated. All drivers should switch over.
Tested by joining/mirroring displays of udl and radeon un der Gnome/X11.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices") Cc: Christoph Hellwig hch@lst.de Cc: Greg Kroah-Hartman gregkh@linuxfoundation.org Cc: Alan Stern stern@rowland.harvard.edu Cc: Johan Hovold johan@kernel.org Cc: Andy Shevchenko andriy.shevchenko@linux.intel.com Cc: Sebastian Andrzej Siewior bigeasy@linutronix.de Cc: Mathias Nyman mathias.nyman@linux.intel.com Cc: "Ahmed S. Darwish" a.darwish@linutronix.de Cc: Oliver Neukum oneukum@suse.com Cc: Thomas Gleixner tglx@linutronix.de Cc: Felipe Balbi balbi@kernel.org Cc: stable@vger.kernel.org # v5.10+ --- drivers/gpu/drm/drm_prime.c | 43 +++++++++++++++++++++------------ drivers/gpu/drm/tiny/gm12u320.c | 7 ++++++ drivers/gpu/drm/udl/udl_drv.c | 7 ++++++ include/drm/drm_drv.h | 12 +++++++++ 4 files changed, 54 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 2a54f86856af..9bb30e843a44 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -923,7 +923,8 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev, } }
- if (!dev->driver->gem_prime_import_sg_table) + if (!dev->driver->gem_prime_import_sg_table && + !dev->driver->gem_prime_create_object) return ERR_PTR(-EINVAL);
attach = dma_buf_attach(dma_buf, attach_dev); @@ -932,25 +933,37 @@ struct drm_gem_object *drm_gem_prime_import_dev(struct drm_device *dev,
get_dma_buf(dma_buf);
- sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL); - if (IS_ERR(sgt)) { - ret = PTR_ERR(sgt); - goto fail_detach; - } + if (dev->driver->gem_prime_create_object) { + obj = dev->driver->gem_prime_create_object(dev, attach); + if (IS_ERR(obj)) { + ret = PTR_ERR(obj); + goto fail_detach; + }
- obj = dev->driver->gem_prime_import_sg_table(dev, attach, sgt); - if (IS_ERR(obj)) { - ret = PTR_ERR(obj); - goto fail_unmap; - } + if (!obj->import_attach) + obj->import_attach = attach; + if (!obj->resv) + obj->resv = dma_buf->resv; + } else { + sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL); + if (IS_ERR(sgt)) { + ret = PTR_ERR(sgt); + goto fail_detach; + }
- obj->import_attach = attach; - obj->resv = dma_buf->resv; + obj = dev->driver->gem_prime_import_sg_table(dev, attach, sgt); + if (IS_ERR(obj)) { + ret = PTR_ERR(obj); + dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL); + goto fail_detach; + } + + obj->import_attach = attach; + obj->resv = dma_buf->resv; + }
return obj;
-fail_unmap: - dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL); fail_detach: dma_buf_detach(dma_buf, attach); dma_buf_put(dma_buf); diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c index 0b4f4f2af1ef..9f13a7c7c2da 100644 --- a/drivers/gpu/drm/tiny/gm12u320.c +++ b/drivers/gpu/drm/tiny/gm12u320.c @@ -601,6 +601,12 @@ static const uint64_t gm12u320_pipe_modifiers[] = {
DEFINE_DRM_GEM_FOPS(gm12u320_fops);
+static struct drm_gem_object *gm12u320_gem_prime_create_object(struct drm_device *dev, + struct dma_buf_attachment *attach) +{ + return drm_gem_shmem_prime_import_sg_table(dev, attach, NULL); +} + static const struct drm_driver gm12u320_drm_driver = { .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
@@ -612,6 +618,7 @@ static const struct drm_driver gm12u320_drm_driver = {
.fops = &gm12u320_fops, DRM_GEM_SHMEM_DRIVER_OPS, + .gem_prime_create_object = gm12u320_gem_prime_create_object, };
static const struct drm_mode_config_funcs gm12u320_mode_config_funcs = { diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c index 9269092697d8..fdf37b44a956 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -34,12 +34,19 @@ static int udl_usb_resume(struct usb_interface *interface)
DEFINE_DRM_GEM_FOPS(udl_driver_fops);
+static struct drm_gem_object *udl_gem_prime_create_object(struct drm_device *dev, + struct dma_buf_attachment *attach) +{ + return drm_gem_shmem_prime_import_sg_table(dev, attach, NULL); +} + static const struct drm_driver driver = { .driver_features = DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET,
/* GEM hooks */ .fops = &udl_driver_fops, DRM_GEM_SHMEM_DRIVER_OPS, + .gem_prime_create_object = udl_gem_prime_create_object,
.name = DRIVER_NAME, .desc = DRIVER_DESC, diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h index 827838e0a97e..77657d649ca6 100644 --- a/include/drm/drm_drv.h +++ b/include/drm/drm_drv.h @@ -369,11 +369,23 @@ struct drm_driver { */ struct drm_gem_object * (*gem_prime_import)(struct drm_device *dev, struct dma_buf *dma_buf); + /** + * @gem_prime_create_object: + * + * Optional hook used by the PRIME helper functions + * drm_gem_prime_import() respectively drm_gem_prime_import_dev(). + */ + struct drm_gem_object *(*gem_prime_create_object)( + struct drm_device *dev, + struct dma_buf_attachment *attach); /** * @gem_prime_import_sg_table: * * Optional hook used by the PRIME helper functions * drm_gem_prime_import() respectively drm_gem_prime_import_dev(). + * + * TODO: This function is deprecated in favor of @drm_driver.gem_prime_create_object. + * Drivers should switch over and implement SG-table support by themselves. */ struct drm_gem_object *(*gem_prime_import_sg_table)( struct drm_device *dev,
On Mon, Feb 22, 2021 at 01:43:26PM +0100, Thomas Zimmermann wrote: ...
Copy-pasting the etnire stack trace to the commit log really hurts readability and provides no additional value to the reader, IMHO.
This patch introduces struct drm_driver.gem_prime_create_object, which
^
Documentation/process/submitting-patches.rst:
Describe your changes in imperative mood, e.g. "make xyzzy do frotz" instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders to the codebase to change its behaviour.
That's a *huge* Cc list, and a most of it seems to be generated by "scripts/get_maintainer.pl --git", which can be overly verbose.
Thanks,
-- Ahmed S. Darwish
On Mon, Feb 22, 2021 at 01:43:26PM +0100, Thomas Zimmermann wrote:
If you don't set import_attach then the refcounting and double-import prevention goes all kinds of wrong. So unfortunately it's not that easy. -Daniel
Moves the scatter/gather-table setup from PRIME helpers into SHMEM helpers. USB-based drivers don't support DMA, so make the sg table optional. This cleans up rsp code in udl and gm12u320.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/drm_gem_shmem_helper.c | 38 +++++++++++++++++-------- drivers/gpu/drm/lima/lima_drv.c | 2 +- drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +- drivers/gpu/drm/panfrost/panfrost_gem.c | 6 ++-- drivers/gpu/drm/panfrost/panfrost_gem.h | 4 +-- drivers/gpu/drm/tiny/gm12u320.c | 7 ----- drivers/gpu/drm/udl/udl_drv.c | 7 ----- drivers/gpu/drm/v3d/v3d_bo.c | 6 ++-- drivers/gpu/drm/v3d/v3d_drv.c | 2 +- drivers/gpu/drm/v3d/v3d_drv.h | 5 ++-- include/drm/drm_gem_shmem_helper.h | 6 ++-- 11 files changed, 38 insertions(+), 47 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index c8a6547a1757..d11154ec0db5 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -711,36 +711,50 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_object *obj) EXPORT_SYMBOL_GPL(drm_gem_shmem_get_pages_sgt);
/** - * drm_gem_shmem_prime_import_sg_table - Produce a shmem GEM object from - * another driver's scatter/gather table of pinned pages + * drm_gem_shmem_prime_create_object - Produce a shmem GEM object from + * another driver's DMA-BUF attachment * @dev: Device to import into * @attach: DMA-BUF attachment - * @sgt: Scatter/gather table of pinned pages * - * This function imports a scatter/gather table exported via DMA-BUF by - * another driver. Drivers that use the shmem helpers should set this as their - * &drm_driver.gem_prime_import_sg_table callback. + * This function imports a DMA-BUF attachment exported by another driver. + * If supported, it sets a scatter/gather table of pinned pages. Drivers + * that use the shmem helpers should set this as their + * &drm_driver.gem_prime_create_object callback. * * Returns: * A pointer to a newly created GEM object or an ERR_PTR-encoded negative * error code on failure. */ struct drm_gem_object * -drm_gem_shmem_prime_import_sg_table(struct drm_device *dev, - struct dma_buf_attachment *attach, - struct sg_table *sgt) +drm_gem_shmem_prime_create_object(struct drm_device *dev, + struct dma_buf_attachment *attach) { size_t size = PAGE_ALIGN(attach->dmabuf->size); + struct sg_table *sgt = NULL; struct drm_gem_shmem_object *shmem; + int ret; + + if (dev->dev->dma_mask) { + sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL); + if (IS_ERR(sgt)) + return ERR_CAST(sgt); + }
shmem = __drm_gem_shmem_create(dev, size, true); - if (IS_ERR(shmem)) - return ERR_CAST(shmem); + if (IS_ERR(shmem)) { + ret = PTR_ERR(shmem); + goto err; + }
shmem->sgt = sgt;
DRM_DEBUG_PRIME("size = %zu\n", size);
return &shmem->base; + +err: + if (sgt) + dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL); + return ERR_PTR(ret); } -EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table); +EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_create_object); diff --git a/drivers/gpu/drm/lima/lima_drv.c b/drivers/gpu/drm/lima/lima_drv.c index 7b8d7178d09a..c8090289ecc7 100644 --- a/drivers/gpu/drm/lima/lima_drv.c +++ b/drivers/gpu/drm/lima/lima_drv.c @@ -277,8 +277,8 @@ static const struct drm_driver lima_drm_driver = {
.gem_create_object = lima_gem_create_object, .prime_fd_to_handle = drm_gem_prime_fd_to_handle, - .gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table, .prime_handle_to_fd = drm_gem_prime_handle_to_fd, + .gem_prime_create_object = drm_gem_shmem_prime_create_object, .gem_prime_mmap = drm_gem_prime_mmap, };
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index 83a461bdeea8..8c0979e1926e 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -564,7 +564,7 @@ static const struct drm_driver panfrost_drm_driver = { .gem_create_object = panfrost_gem_create_object, .prime_handle_to_fd = drm_gem_prime_handle_to_fd, .prime_fd_to_handle = drm_gem_prime_fd_to_handle, - .gem_prime_import_sg_table = panfrost_gem_prime_import_sg_table, + .gem_prime_create_object = panfrost_gem_prime_create_object, .gem_prime_mmap = drm_gem_prime_mmap, };
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c index 3e0723bc36bd..69bb70180ac1 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gem.c +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c @@ -269,14 +269,12 @@ panfrost_gem_create_with_handle(struct drm_file *file_priv, }
struct drm_gem_object * -panfrost_gem_prime_import_sg_table(struct drm_device *dev, - struct dma_buf_attachment *attach, - struct sg_table *sgt) +panfrost_gem_prime_create_object(struct drm_device *dev, struct dma_buf_attachment *attach) { struct drm_gem_object *obj; struct panfrost_gem_object *bo;
- obj = drm_gem_shmem_prime_import_sg_table(dev, attach, sgt); + obj = drm_gem_shmem_prime_create_object(dev, attach); if (IS_ERR(obj)) return ERR_CAST(obj);
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h index 8088d5fd8480..37b8cb4c6626 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gem.h +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h @@ -64,9 +64,7 @@ drm_mm_node_to_panfrost_mapping(struct drm_mm_node *node) struct drm_gem_object *panfrost_gem_create_object(struct drm_device *dev, size_t size);
struct drm_gem_object * -panfrost_gem_prime_import_sg_table(struct drm_device *dev, - struct dma_buf_attachment *attach, - struct sg_table *sgt); +panfrost_gem_prime_create_object(struct drm_device *dev, struct dma_buf_attachment *attach);
struct panfrost_gem_object * panfrost_gem_create_with_handle(struct drm_file *file_priv, diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c index 9f13a7c7c2da..0b4f4f2af1ef 100644 --- a/drivers/gpu/drm/tiny/gm12u320.c +++ b/drivers/gpu/drm/tiny/gm12u320.c @@ -601,12 +601,6 @@ static const uint64_t gm12u320_pipe_modifiers[] = {
DEFINE_DRM_GEM_FOPS(gm12u320_fops);
-static struct drm_gem_object *gm12u320_gem_prime_create_object(struct drm_device *dev, - struct dma_buf_attachment *attach) -{ - return drm_gem_shmem_prime_import_sg_table(dev, attach, NULL); -} - static const struct drm_driver gm12u320_drm_driver = { .driver_features = DRIVER_MODESET | DRIVER_GEM | DRIVER_ATOMIC,
@@ -618,7 +612,6 @@ static const struct drm_driver gm12u320_drm_driver = {
.fops = &gm12u320_fops, DRM_GEM_SHMEM_DRIVER_OPS, - .gem_prime_create_object = gm12u320_gem_prime_create_object, };
static const struct drm_mode_config_funcs gm12u320_mode_config_funcs = { diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c index fdf37b44a956..9269092697d8 100644 --- a/drivers/gpu/drm/udl/udl_drv.c +++ b/drivers/gpu/drm/udl/udl_drv.c @@ -34,19 +34,12 @@ static int udl_usb_resume(struct usb_interface *interface)
DEFINE_DRM_GEM_FOPS(udl_driver_fops);
-static struct drm_gem_object *udl_gem_prime_create_object(struct drm_device *dev, - struct dma_buf_attachment *attach) -{ - return drm_gem_shmem_prime_import_sg_table(dev, attach, NULL); -} - static const struct drm_driver driver = { .driver_features = DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET,
/* GEM hooks */ .fops = &udl_driver_fops, DRM_GEM_SHMEM_DRIVER_OPS, - .gem_prime_create_object = udl_gem_prime_create_object,
.name = DRIVER_NAME, .desc = DRIVER_DESC, diff --git a/drivers/gpu/drm/v3d/v3d_bo.c b/drivers/gpu/drm/v3d/v3d_bo.c index 6a8731ab9d7d..37f65cb44703 100644 --- a/drivers/gpu/drm/v3d/v3d_bo.c +++ b/drivers/gpu/drm/v3d/v3d_bo.c @@ -146,14 +146,12 @@ struct v3d_bo *v3d_bo_create(struct drm_device *dev, struct drm_file *file_priv, }
struct drm_gem_object * -v3d_prime_import_sg_table(struct drm_device *dev, - struct dma_buf_attachment *attach, - struct sg_table *sgt) +v3d_prime_create_object(struct drm_device *dev, struct dma_buf_attachment *attach) { struct drm_gem_object *obj; int ret;
- obj = drm_gem_shmem_prime_import_sg_table(dev, attach, sgt); + obj = drm_gem_shmem_prime_create_object(dev, attach); if (IS_ERR(obj)) return obj;
diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c index 99e22beea90b..72d50e240726 100644 --- a/drivers/gpu/drm/v3d/v3d_drv.c +++ b/drivers/gpu/drm/v3d/v3d_drv.c @@ -173,7 +173,7 @@ static const struct drm_driver v3d_drm_driver = { .gem_create_object = v3d_create_object, .prime_handle_to_fd = drm_gem_prime_handle_to_fd, .prime_fd_to_handle = drm_gem_prime_fd_to_handle, - .gem_prime_import_sg_table = v3d_prime_import_sg_table, + .gem_prime_create_object = v3d_prime_create_object, .gem_prime_mmap = drm_gem_prime_mmap,
.ioctls = v3d_drm_ioctls, diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h index 8a390738d65b..6e4f3eb0b9fc 100644 --- a/drivers/gpu/drm/v3d/v3d_drv.h +++ b/drivers/gpu/drm/v3d/v3d_drv.h @@ -310,9 +310,8 @@ int v3d_mmap_bo_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); int v3d_get_bo_offset_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); -struct drm_gem_object *v3d_prime_import_sg_table(struct drm_device *dev, - struct dma_buf_attachment *attach, - struct sg_table *sgt); +struct drm_gem_object *v3d_prime_create_object(struct drm_device *dev, + struct dma_buf_attachment *attach);
/* v3d_debugfs.c */ void v3d_debugfs_init(struct drm_minor *minor); diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_shmem_helper.h index 434328d8a0d9..837f3b5a83af 100644 --- a/include/drm/drm_gem_shmem_helper.h +++ b/include/drm/drm_gem_shmem_helper.h @@ -143,9 +143,7 @@ void drm_gem_shmem_print_info(struct drm_printer *p, unsigned int indent,
struct sg_table *drm_gem_shmem_get_sg_table(struct drm_gem_object *obj); struct drm_gem_object * -drm_gem_shmem_prime_import_sg_table(struct drm_device *dev, - struct dma_buf_attachment *attach, - struct sg_table *sgt); +drm_gem_shmem_prime_create_object(struct drm_device *dev, struct dma_buf_attachment *attach);
struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_object *obj);
@@ -158,7 +156,7 @@ struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_object *obj); #define DRM_GEM_SHMEM_DRIVER_OPS \ .prime_handle_to_fd = drm_gem_prime_handle_to_fd, \ .prime_fd_to_handle = drm_gem_prime_fd_to_handle, \ - .gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table, \ + .gem_prime_create_object = drm_gem_shmem_prime_create_object, \ .gem_prime_mmap = drm_gem_prime_mmap, \ .dumb_create = drm_gem_shmem_dumb_create
Moves the scatter/gather-table setup from PRIME helpers into CMA helpers. No functional changes.
Signed-off-by: Thomas Zimmermann tzimmermann@suse.de --- drivers/gpu/drm/drm_gem_cma_helper.c | 62 ++++++++++++++++------------ drivers/gpu/drm/pl111/pl111_drv.c | 8 ++-- include/drm/drm_gem_cma_helper.h | 12 ++---- 3 files changed, 42 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c b/drivers/gpu/drm/drm_gem_cma_helper.c index 7942cf05cd93..7200d0ae2e38 100644 --- a/drivers/gpu/drm/drm_gem_cma_helper.c +++ b/drivers/gpu/drm/drm_gem_cma_helper.c @@ -404,37 +404,44 @@ struct sg_table *drm_gem_cma_get_sg_table(struct drm_gem_object *obj) EXPORT_SYMBOL_GPL(drm_gem_cma_get_sg_table);
/** - * drm_gem_cma_prime_import_sg_table - produce a CMA GEM object from another - * driver's scatter/gather table of pinned pages + * drm_gem_cma_prime_create_object - produce a CMA GEM object from another + * driver's DMA-BUF attachment * @dev: device to import into * @attach: DMA-BUF attachment - * @sgt: scatter/gather table of pinned pages * - * This function imports a scatter/gather table exported via DMA-BUF by - * another driver. Imported buffers must be physically contiguous in memory - * (i.e. the scatter/gather table must contain a single entry). Drivers that - * use the CMA helpers should set this as their - * &drm_driver.gem_prime_import_sg_table callback. + * This function imports a DMA-BUF exported by another driver. It sets a + * scatter/gather table of pinned pages. Imported buffers must be physically + * contiguous in memory (i.e. the scatter/gather table must contain a single + * entry). Drivers that use the CMA helpers should set this as their + * &drm_driver.gem_prime_create_object callback. * * Returns: * A pointer to a newly created GEM object or an ERR_PTR-encoded negative * error code on failure. */ struct drm_gem_object * -drm_gem_cma_prime_import_sg_table(struct drm_device *dev, - struct dma_buf_attachment *attach, - struct sg_table *sgt) +drm_gem_cma_prime_create_object(struct drm_device *dev, struct dma_buf_attachment *attach) { + struct sg_table *sgt; struct drm_gem_cma_object *cma_obj; + int ret; + + sgt = dma_buf_map_attachment(attach, DMA_BIDIRECTIONAL); + if (IS_ERR(sgt)) + return ERR_CAST(sgt);
/* check if the entries in the sg_table are contiguous */ - if (drm_prime_get_contiguous_size(sgt) < attach->dmabuf->size) - return ERR_PTR(-EINVAL); + if (drm_prime_get_contiguous_size(sgt) < attach->dmabuf->size) { + ret = -EINVAL; + goto err; + }
/* Create a CMA GEM buffer. */ cma_obj = __drm_gem_cma_create(dev, attach->dmabuf->size); - if (IS_ERR(cma_obj)) - return ERR_CAST(cma_obj); + if (IS_ERR(cma_obj)) { + ret = PTR_ERR(cma_obj); + goto err; + }
cma_obj->paddr = sg_dma_address(sgt->sgl); cma_obj->sgt = sgt; @@ -442,8 +449,12 @@ drm_gem_cma_prime_import_sg_table(struct drm_device *dev, DRM_DEBUG_PRIME("dma_addr = %pad, size = %zu\n", &cma_obj->paddr, attach->dmabuf->size);
return &cma_obj->base; + +err: + dma_buf_unmap_attachment(attach, sgt, DMA_BIDIRECTIONAL); + return ERR_PTR(ret); } -EXPORT_SYMBOL_GPL(drm_gem_cma_prime_import_sg_table); +EXPORT_SYMBOL_GPL(drm_gem_cma_prime_create_object);
/** * drm_gem_cma_vmap - map a CMA GEM object into the kernel's virtual @@ -509,18 +520,17 @@ int drm_gem_cma_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
/** - * drm_gem_cma_prime_import_sg_table_vmap - PRIME import another driver's - * scatter/gather table and get the virtual address of the buffer + * drm_gem_cma_prime_create_object_vmap - produce a CMA GEM object from another + * driver's DMA-BUF attachment and gets the virtual address of the buffer * @dev: DRM device * @attach: DMA-BUF attachment - * @sgt: Scatter/gather table of pinned pages * - * This function imports a scatter/gather table using - * drm_gem_cma_prime_import_sg_table() and uses dma_buf_vmap() to get the kernel + * This function imports a DMA-BUF using + * drm_gem_cma_prime_create_object() and uses dma_buf_vmap() to get the kernel * virtual address. This ensures that a CMA GEM object always has its virtual * address set. This address is released when the object is freed. * - * This function can be used as the &drm_driver.gem_prime_import_sg_table + * This function can be used as the &drm_driver.gem_prime_create_object * callback. The &DRM_GEM_CMA_DRIVER_OPS_VMAP macro provides a shortcut to set * the necessary DRM driver operations. * @@ -529,9 +539,7 @@ EXPORT_SYMBOL_GPL(drm_gem_cma_mmap); * error code on failure. */ struct drm_gem_object * -drm_gem_cma_prime_import_sg_table_vmap(struct drm_device *dev, - struct dma_buf_attachment *attach, - struct sg_table *sgt) +drm_gem_cma_prime_create_object_vmap(struct drm_device *dev, struct dma_buf_attachment *attach) { struct drm_gem_cma_object *cma_obj; struct drm_gem_object *obj; @@ -544,7 +552,7 @@ drm_gem_cma_prime_import_sg_table_vmap(struct drm_device *dev, return ERR_PTR(ret); }
- obj = drm_gem_cma_prime_import_sg_table(dev, attach, sgt); + obj = drm_gem_cma_prime_create_object(dev, attach); if (IS_ERR(obj)) { dma_buf_vunmap(attach->dmabuf, &map); return obj; @@ -555,4 +563,4 @@ drm_gem_cma_prime_import_sg_table_vmap(struct drm_device *dev,
return obj; } -EXPORT_SYMBOL(drm_gem_cma_prime_import_sg_table_vmap); +EXPORT_SYMBOL(drm_gem_cma_prime_create_object_vmap); diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c index e4dcaef6c143..3dd6c7e46df0 100644 --- a/drivers/gpu/drm/pl111/pl111_drv.c +++ b/drivers/gpu/drm/pl111/pl111_drv.c @@ -194,9 +194,7 @@ static int pl111_modeset_init(struct drm_device *dev) }
static struct drm_gem_object * -pl111_gem_import_sg_table(struct drm_device *dev, - struct dma_buf_attachment *attach, - struct sg_table *sgt) +pl111_gem_create_object(struct drm_device *dev, struct dma_buf_attachment *attach) { struct pl111_drm_dev_private *priv = dev->dev_private;
@@ -208,7 +206,7 @@ pl111_gem_import_sg_table(struct drm_device *dev, if (priv->use_device_memory) return ERR_PTR(-EINVAL);
- return drm_gem_cma_prime_import_sg_table(dev, attach, sgt); + return drm_gem_cma_prime_create_object(dev, attach); }
DEFINE_DRM_GEM_CMA_FOPS(drm_fops); @@ -227,7 +225,7 @@ static const struct drm_driver pl111_drm_driver = { .dumb_create = drm_gem_cma_dumb_create, .prime_handle_to_fd = drm_gem_prime_handle_to_fd, .prime_fd_to_handle = drm_gem_prime_fd_to_handle, - .gem_prime_import_sg_table = pl111_gem_import_sg_table, + .gem_prime_create_object = pl111_gem_create_object, .gem_prime_mmap = drm_gem_prime_mmap,
#if defined(CONFIG_DEBUG_FS) diff --git a/include/drm/drm_gem_cma_helper.h b/include/drm/drm_gem_cma_helper.h index 0a9711caa3e8..54d7f4b11225 100644 --- a/include/drm/drm_gem_cma_helper.h +++ b/include/drm/drm_gem_cma_helper.h @@ -95,9 +95,7 @@ void drm_gem_cma_print_info(struct drm_printer *p, unsigned int indent,
struct sg_table *drm_gem_cma_get_sg_table(struct drm_gem_object *obj); struct drm_gem_object * -drm_gem_cma_prime_import_sg_table(struct drm_device *dev, - struct dma_buf_attachment *attach, - struct sg_table *sgt); +drm_gem_cma_prime_create_object(struct drm_device *dev, struct dma_buf_attachment *attach); int drm_gem_cma_vmap(struct drm_gem_object *obj, struct dma_buf_map *map); int drm_gem_cma_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma);
@@ -118,7 +116,7 @@ int drm_gem_cma_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma); .dumb_create = (dumb_create_func), \ .prime_handle_to_fd = drm_gem_prime_handle_to_fd, \ .prime_fd_to_handle = drm_gem_prime_fd_to_handle, \ - .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table, \ + .gem_prime_create_object = drm_gem_cma_prime_create_object, \ .gem_prime_mmap = drm_gem_prime_mmap
/** @@ -156,7 +154,7 @@ int drm_gem_cma_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma); .dumb_create = dumb_create_func, \ .prime_handle_to_fd = drm_gem_prime_handle_to_fd, \ .prime_fd_to_handle = drm_gem_prime_fd_to_handle, \ - .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table_vmap, \ + .gem_prime_create_object = drm_gem_cma_prime_create_object_vmap, \ .gem_prime_mmap = drm_gem_prime_mmap
/** @@ -178,8 +176,6 @@ int drm_gem_cma_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma); DRM_GEM_CMA_DRIVER_OPS_VMAP_WITH_DUMB_CREATE(drm_gem_cma_dumb_create)
struct drm_gem_object * -drm_gem_cma_prime_import_sg_table_vmap(struct drm_device *drm, - struct dma_buf_attachment *attach, - struct sg_table *sgt); +drm_gem_cma_prime_create_object_vmap(struct drm_device *drm, struct dma_buf_attachment *attach);
#endif /* __DRM_GEM_CMA_HELPER_H__ */
Am 22.02.21 um 13:43 schrieb Thomas Zimmermann:
Mhm, that sounds like a little overkill to me.
Drivers can already import the DMA-bufs all by them selves without the help of the DRM functions. See amdgpu for an example.
Daniel also already noted to me that he sees the DRM helper as a bit questionable middle layer.
Have you thought about doing that instead?
Christian.
Hi
Am 22.02.21 um 14:09 schrieb Christian König:
And this bug proves that it is. :)
Have you thought about doing that instead?
There appears to be some useful code in drm_gem_prime_import_dev(). But if the general sentiment goes towards removing gem_prime_import_sg_table, we can work towards that as well.
Best regards Thomas
On Mon, Feb 22, 2021 at 2:24 PM Thomas Zimmermann tzimmermann@suse.de wrote:
The trouble here is actually gem_bo->import_attach, which isn't really part of the questionable midlayer, but fairly mandatory (only exception is vmwgfx because not using gem) caching to make sure we don't end up with duped imports and fun stuff like that.
And dma_buf_attach now implicitly creates the sg table already, so we're already in game over land. I think we'd need to make import_attach a union with import_buf or something like that, so that you can do attachment-less importing.
I still think this part is a bit a silly midlayer for no good reason, but I think that's orthogonal to the issue at hand here.
I'd suggest we first try to paper over the issue by using prime_import_dev with the host controller (which hopefully is dma-capable for most systems). And then, at leisure, try to untangle the obj->import_attach issue. -Daniel
Hi
Am 22.02.21 um 17:10 schrieb Daniel Vetter:
Creating the sg table is not the problem; mapping it is. So dma_buf_attach shouldn't be a problem.
I really don't want to do this. My time is also limited, and I''ll spend time papering over the thing. And then more time for the real fix. I'd rather pull drm_gem_prime_import_dev() in to USB drivers and avoid the dma_buf_map().
Best regard Thomas
On Mon, Feb 22, 2021 at 05:25:46PM +0100, Thomas Zimmermann wrote:
dma_buf_attach will create a cached sg-mapping for you if the exporter is dynamic. Currently that's only the case for amdgpu, I guess you didn't test with that.
So yeah dma_buf_attach is a problem already. And if we can't attach, the entire obj->import_attach logic in drm_prime.c falls over, and we get all kinds of fun with double import and re-export.
Yeah I understand, it's just (as usual :-/) more complex than it seems ... -Daniel
Hi
Am 22.02.21 um 17:34 schrieb Daniel Vetter:
OK, I give up. I'll send out the patch with the usb controller later today.
Best regards Thomas
dri-devel@lists.freedesktop.org
-
Ahmed S. Darwish -
Christian König -
Daniel Vetter -
Thomas Zimmermann