[PATCH v2 0/3] drm/prime: Only call dma_map_sgtable() for devices with DMA support

[PATCH v2 0/3] drm/prime: Only call dma_map_sgtable() for devices with DMA support

[Thomas Zimmermann]

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

[PATCH v2 0/3] drm/prime: Only call dma_map_sgtable() for devices with DMA support

[Thomas Zimmermann]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 1/3] drm: Support importing dmabufs into drivers without DMA

[Thomas Zimmermann]

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,
-- 
2.30.1

[PATCH v2 1/3] drm: Support importing dmabufs into drivers without DMA

[Thomas Zimmermann]

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,
-- 
2.30.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/3] drm/shmem-helper: Implement struct drm_driver.gem_prime_create_object

[Thomas Zimmermann]

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
 
-- 
2.30.1

[PATCH v2 2/3] drm/shmem-helper: Implement struct drm_driver.gem_prime_create_object

[Thomas Zimmermann]

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
 
-- 
2.30.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 3/3] drm/cma-helper: Implement struct drm_driver.gem_prime_create_object

[Thomas Zimmermann]

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__ */
-- 
2.30.1

[PATCH v2 3/3] drm/cma-helper: Implement struct drm_driver.gem_prime_create_object

[Thomas Zimmermann]

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__ */
-- 
2.30.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/3] drm/prime: Only call dma_map_sgtable() for devices with DMA support

[Christian König]

Am 22.02.21 um 13:43 schrieb Thomas Zimmermann:
> 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.

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.

>
> 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
>

Re: [PATCH v2 0/3] drm/prime: Only call dma_map_sgtable() for devices with DMA support

[Christian König]

Am 22.02.21 um 13:43 schrieb Thomas Zimmermann:
> 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.

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.

>
> 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
>

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/3] drm: Support importing dmabufs into drivers without DMA

[Ahmed S. Darwish]

On Mon, Feb 22, 2021 at 01:43:26PM +0100, Thomas Zimmermann wrote:
...
>
> [   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 ]---
>

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.

> 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>

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

Re: [PATCH v2 1/3] drm: Support importing dmabufs into drivers without DMA

[Ahmed S. Darwish]

On Mon, Feb 22, 2021 at 01:43:26PM +0100, Thomas Zimmermann wrote:
...
>
> [   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 ]---
>

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.

> 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>

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/3] drm/prime: Only call dma_map_sgtable() for devices with DMA support

[Thomas Zimmermann]

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

Hi

Am 22.02.21 um 14:09 schrieb Christian König:
> 
> 
> Am 22.02.21 um 13:43 schrieb Thomas Zimmermann:
>> 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.
> 
> 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.

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

> 
> Christian.
> 
>>
>> 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
>>
> 

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


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

Re: [PATCH v2 0/3] drm/prime: Only call dma_map_sgtable() for devices with DMA support

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 3373 bytes --]

Hi

Am 22.02.21 um 14:09 schrieb Christian König:
> 
> 
> Am 22.02.21 um 13:43 schrieb Thomas Zimmermann:
>> 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.
> 
> 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.

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

> 
> Christian.
> 
>>
>> 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
>>
> 

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


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

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/3] drm/prime: Only call dma_map_sgtable() for devices with DMA support

[Daniel Vetter]

On Mon, Feb 22, 2021 at 2:24 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 22.02.21 um 14:09 schrieb Christian König:
> >
> >
> > Am 22.02.21 um 13:43 schrieb Thomas Zimmermann:
> >> 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.
> >
> > 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.
>
> And this bug proves that it is. :)

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.

> > 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.

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

>
> Best regards
> Thomas
>
> >
> > Christian.
> >
> >>
> >> 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
> >>
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 0/3] drm/prime: Only call dma_map_sgtable() for devices with DMA support

[Daniel Vetter]

On Mon, Feb 22, 2021 at 2:24 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 22.02.21 um 14:09 schrieb Christian König:
> >
> >
> > Am 22.02.21 um 13:43 schrieb Thomas Zimmermann:
> >> 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.
> >
> > 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.
>
> And this bug proves that it is. :)

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.

> > 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.

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

>
> Best regards
> Thomas
>
> >
> > Christian.
> >
> >>
> >> 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
> >>
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/3] drm/prime: Only call dma_map_sgtable() for devices with DMA support

[Thomas Zimmermann]

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

Hi

Am 22.02.21 um 17:10 schrieb Daniel Vetter:
> On Mon, Feb 22, 2021 at 2:24 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> Hi
>>
>> Am 22.02.21 um 14:09 schrieb Christian König:
>>>
>>>
>>> Am 22.02.21 um 13:43 schrieb Thomas Zimmermann:
>>>> 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.
>>>
>>> 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.
>>
>> And this bug proves that it is. :)
> 
> 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.

Creating the sg table is not the problem; mapping it is. So 
dma_buf_attach shouldn't be a problem.

> 
>>> 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.
> 
> 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.

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

> -Daniel
> 
>>
>> Best regards
>> Thomas
>>
>>>
>>> Christian.
>>>
>>>>
>>>> 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
>>>>
>>>
>>
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Felix Imendörffer
>>
> 
> 

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


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

Re: [PATCH v2 0/3] drm/prime: Only call dma_map_sgtable() for devices with DMA support

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 5058 bytes --]

Hi

Am 22.02.21 um 17:10 schrieb Daniel Vetter:
> On Mon, Feb 22, 2021 at 2:24 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> Hi
>>
>> Am 22.02.21 um 14:09 schrieb Christian König:
>>>
>>>
>>> Am 22.02.21 um 13:43 schrieb Thomas Zimmermann:
>>>> 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.
>>>
>>> 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.
>>
>> And this bug proves that it is. :)
> 
> 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.

Creating the sg table is not the problem; mapping it is. So 
dma_buf_attach shouldn't be a problem.

> 
>>> 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.
> 
> 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.

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

> -Daniel
> 
>>
>> Best regards
>> Thomas
>>
>>>
>>> Christian.
>>>
>>>>
>>>> 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
>>>>
>>>
>>
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Felix Imendörffer
>>
> 
> 

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


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

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/3] drm: Support importing dmabufs into drivers without DMA

[Daniel Vetter]

On Mon, Feb 22, 2021 at 01:43:26PM +0100, Thomas Zimmermann wrote:
> 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 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

> +		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,
> -- 
> 2.30.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 1/3] drm: Support importing dmabufs into drivers without DMA

[Daniel Vetter]

On Mon, Feb 22, 2021 at 01:43:26PM +0100, Thomas Zimmermann wrote:
> 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 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

> +		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,
> -- 
> 2.30.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/3] drm/prime: Only call dma_map_sgtable() for devices with DMA support

[Daniel Vetter]

On Mon, Feb 22, 2021 at 05:25:46PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 22.02.21 um 17:10 schrieb Daniel Vetter:
> > On Mon, Feb 22, 2021 at 2:24 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> > > 
> > > Hi
> > > 
> > > Am 22.02.21 um 14:09 schrieb Christian König:
> > > > 
> > > > 
> > > > Am 22.02.21 um 13:43 schrieb Thomas Zimmermann:
> > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > And this bug proves that it is. :)
> > 
> > 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.
> 
> Creating the sg table is not the problem; mapping it is. So dma_buf_attach
> shouldn't be a problem.

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.

> > > > 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.
> > 
> > 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.
> 
> 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().

Yeah I understand, it's just (as usual :-/) more complex than it seems ...
-Daniel

> 
> Best regard
> Thomas
> 
> > -Daniel
> > 
> > > 
> > > Best regards
> > > Thomas
> > > 
> > > > 
> > > > Christian.
> > > > 
> > > > > 
> > > > > 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
> > > > > 
> > > > 
> > > 
> > > --
> > > Thomas Zimmermann
> > > Graphics Driver Developer
> > > SUSE Software Solutions Germany GmbH
> > > Maxfeldstr. 5, 90409 Nürnberg, Germany
> > > (HRB 36809, AG Nürnberg)
> > > Geschäftsführer: Felix Imendörffer
> > > 
> > 
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
> 




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v2 0/3] drm/prime: Only call dma_map_sgtable() for devices with DMA support

[Daniel Vetter]

On Mon, Feb 22, 2021 at 05:25:46PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> Am 22.02.21 um 17:10 schrieb Daniel Vetter:
> > On Mon, Feb 22, 2021 at 2:24 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> > > 
> > > Hi
> > > 
> > > Am 22.02.21 um 14:09 schrieb Christian König:
> > > > 
> > > > 
> > > > Am 22.02.21 um 13:43 schrieb Thomas Zimmermann:
> > > > > 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.
> > > > 
> > > > 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.
> > > 
> > > And this bug proves that it is. :)
> > 
> > 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.
> 
> Creating the sg table is not the problem; mapping it is. So dma_buf_attach
> shouldn't be a problem.

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.

> > > > 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.
> > 
> > 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.
> 
> 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().

Yeah I understand, it's just (as usual :-/) more complex than it seems ...
-Daniel

> 
> Best regard
> Thomas
> 
> > -Daniel
> > 
> > > 
> > > Best regards
> > > Thomas
> > > 
> > > > 
> > > > Christian.
> > > > 
> > > > > 
> > > > > 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
> > > > > 
> > > > 
> > > 
> > > --
> > > Thomas Zimmermann
> > > Graphics Driver Developer
> > > SUSE Software Solutions Germany GmbH
> > > Maxfeldstr. 5, 90409 Nürnberg, Germany
> > > (HRB 36809, AG Nürnberg)
> > > Geschäftsführer: Felix Imendörffer
> > > 
> > 
> > 
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Maxfeldstr. 5, 90409 Nürnberg, Germany
> (HRB 36809, AG Nürnberg)
> Geschäftsführer: Felix Imendörffer
> 




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 0/3] drm/prime: Only call dma_map_sgtable() for devices with DMA support

[Thomas Zimmermann]

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

Hi

Am 22.02.21 um 17:34 schrieb Daniel Vetter:
> On Mon, Feb 22, 2021 at 05:25:46PM +0100, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 22.02.21 um 17:10 schrieb Daniel Vetter:
>>> On Mon, Feb 22, 2021 at 2:24 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>>
>>>> Hi
>>>>
>>>> Am 22.02.21 um 14:09 schrieb Christian König:
>>>>>
>>>>>
>>>>> Am 22.02.21 um 13:43 schrieb Thomas Zimmermann:
>>>>>> 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.
>>>>>
>>>>> 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.
>>>>
>>>> And this bug proves that it is. :)
>>>
>>> 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.
>>
>> Creating the sg table is not the problem; mapping it is. So dma_buf_attach
>> shouldn't be a problem.
> 
> 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.

OK, I give up. I'll send out the patch with the usb controller later today.

Best regards
Thomas

> 
>>>>> 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.
>>>
>>> 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.
>>
>> 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().
> 
> Yeah I understand, it's just (as usual :-/) more complex than it seems ...
> -Daniel
> 
>>
>> Best regard
>> Thomas
>>
>>> -Daniel
>>>
>>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>>>
>>>>> Christian.
>>>>>
>>>>>>
>>>>>> 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
>>>>>>
>>>>>
>>>>
>>>> --
>>>> Thomas Zimmermann
>>>> Graphics Driver Developer
>>>> SUSE Software Solutions Germany GmbH
>>>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>>>> (HRB 36809, AG Nürnberg)
>>>> Geschäftsführer: Felix Imendörffer
>>>>
>>>
>>>
>>
>> -- 
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Felix Imendörffer
>>
> 
> 
> 
> 

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


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

Re: [PATCH v2 0/3] drm/prime: Only call dma_map_sgtable() for devices with DMA support

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 6268 bytes --]

Hi

Am 22.02.21 um 17:34 schrieb Daniel Vetter:
> On Mon, Feb 22, 2021 at 05:25:46PM +0100, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 22.02.21 um 17:10 schrieb Daniel Vetter:
>>> On Mon, Feb 22, 2021 at 2:24 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>>>
>>>> Hi
>>>>
>>>> Am 22.02.21 um 14:09 schrieb Christian König:
>>>>>
>>>>>
>>>>> Am 22.02.21 um 13:43 schrieb Thomas Zimmermann:
>>>>>> 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.
>>>>>
>>>>> 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.
>>>>
>>>> And this bug proves that it is. :)
>>>
>>> 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.
>>
>> Creating the sg table is not the problem; mapping it is. So dma_buf_attach
>> shouldn't be a problem.
> 
> 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.

OK, I give up. I'll send out the patch with the usb controller later today.

Best regards
Thomas

> 
>>>>> 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.
>>>
>>> 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.
>>
>> 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().
> 
> Yeah I understand, it's just (as usual :-/) more complex than it seems ...
> -Daniel
> 
>>
>> Best regard
>> Thomas
>>
>>> -Daniel
>>>
>>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>>>
>>>>> Christian.
>>>>>
>>>>>>
>>>>>> 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
>>>>>>
>>>>>
>>>>
>>>> --
>>>> Thomas Zimmermann
>>>> Graphics Driver Developer
>>>> SUSE Software Solutions Germany GmbH
>>>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>>>> (HRB 36809, AG Nürnberg)
>>>> Geschäftsführer: Felix Imendörffer
>>>>
>>>
>>>
>>
>> -- 
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Felix Imendörffer
>>
> 
> 
> 
> 

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


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

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel