On Fri, Feb 19, 2021 at 3:35 PM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
>
> Hi
>
> Am 19.02.21 um 14:45 schrieb Christian König:
> > Well as far as I can see this is a relative clear NAK.
> >
> > When a device can't do DMA and has no DMA mask then why it is requesting
> > an sg-table in the first place?
> >
> > The problem is rather in drm_gem_prime_import_dev() which always tries
> > to create an sg-table to get a GEM object, even when the device doesn't
> > support DMA at all.
>
> Sure, that's the clean solution. But it also needs a bit of work on the
> way the importing works. Our generic code assumes that an SG table is
> being used. Most driver's memory managers as well, I guess.
>
> The regressing patch has already been in upstream kernels for a while.
> The fix at hand is backport-able at least.
>
> Can we add a TODO item for the real thing?
I'm not against a quick&dirt fix for this regression, but this is
kinda on the wrong side:
The issue is that importers did the wrong thing, and now that the
dma-api is a bit more strict, this stopped working. So if we throw a
hack at the problem, it needs to be on the importer's side. If we now
add a hack on the exporter side, we risk that it gets used in more
places, and it's rather hard to get the cat back into the bag.
E.g. all the importers relying on a struct page being there is really
hard to fix. We shouldn't make the same mistake here.
-Daniel
>
> Best regards
> Thomas
>
> >
> > Regards,
> > Christian.
> >
> > Am 19.02.21 um 14:40 schrieb Thomas Zimmermann:
> >> Fixes a regression for udl and probably other USB-based drivers where
> >> joining and mirroring displays fails.
> >>
> >> Joining displays requires importing a dma_buf from another DRM driver.
> >> These devices don't support DMA and therefore have no DMA mask. Trying
> >> to map imported buffers from a DMA-able memory zone fails with an error.
> >> An example 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 ]---
> >>
> >> For the fix, we don't call dma_map_sgtable() for devices without the
> >> DMA mask. Drivers for such devices have to map the imported buffer into
> >> kernel address space and perfom the copy operation in software.
> >>
> >> Tested by joining/mirroring displays of udl and radeon un der Gnome/X11.
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> >> Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB
> >> devices")
> >> Cc: Christoph Hellwig <hch@xxxxxx>
> >> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> >> Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> >> Cc: Johan Hovold <johan@xxxxxxxxxx>
> >> Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> >> Cc: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> >> Cc: "Ahmed S. Darwish" <a.darwish@xxxxxxxxxxxxx>
> >> Cc: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
> >> Cc: Oliver Neukum <oneukum@xxxxxxxx>
> >> Cc: Felipe Balbi <balbi@xxxxxxxxxx>
> >> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> >> Cc: <stable@xxxxxxxxxxxxxxx> # v5.10+
> >> ---
> >> drivers/gpu/drm/drm_prime.c | 27 ++++++++++++++++++---------
> >> 1 file changed, 18 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> >> index 2a54f86856af..d5a39fe76b78 100644
> >> --- a/drivers/gpu/drm/drm_prime.c
> >> +++ b/drivers/gpu/drm/drm_prime.c
> >> @@ -603,10 +603,15 @@ EXPORT_SYMBOL(drm_gem_map_detach);
> >> * @attach: attachment whose scatterlist is to be returned
> >> * @dir: direction of DMA transfer
> >> *
> >> - * Calls &drm_gem_object_funcs.get_sg_table and then maps the
> >> scatterlist. This
> >> - * can be used as the &dma_buf_ops.map_dma_buf callback. Should be
> >> used together
> >> + * Calls &drm_gem_object_funcs.get_sg_table and, if possible, maps
> >> the scatterlist.
> >> + * This can be used as the &dma_buf_ops.map_dma_buf callback. Should
> >> be used together
> >> * with drm_gem_unmap_dma_buf().
> >> *
> >> + * Devices on some peripheral busses, such as USB, cannot use DMA. In
> >> this case,
> >> + * pages in the scatterlist remain unmapped. Drivers for such devices
> >> should acquire
> >> + * a mapping with dma_buf_vmap() and implement copy operation via
> >> bus-specific
> >> + * interfaces.
> >> + *
> >> * Returns:sg_table containing the scatterlist to be returned;
> >> returns ERR_PTR
> >> * on error. May return -EINTR if it is interrupted by a signal.
> >> */
> >> @@ -627,12 +632,14 @@ struct sg_table *drm_gem_map_dma_buf(struct
> >> dma_buf_attachment *attach,
> >> if (IS_ERR(sgt))
> >> return sgt;
> >>
> >> - ret = dma_map_sgtable(attach->dev, sgt, dir,
> >> - DMA_ATTR_SKIP_CPU_SYNC);
> >> - if (ret) {
> >> - sg_free_table(sgt);
> >> - kfree(sgt);
> >> - sgt = ERR_PTR(ret);
> >> + if (attach->dev->dma_mask) {
> >> + ret = dma_map_sgtable(attach->dev, sgt, dir,
> >> + DMA_ATTR_SKIP_CPU_SYNC);
> >> + if (ret) {
> >> + sg_free_table(sgt);
> >> + kfree(sgt);
> >> + sgt = ERR_PTR(ret);
> >> + }
> >> }
> >>
> >> return sgt;
> >> @@ -654,7 +661,9 @@ void drm_gem_unmap_dma_buf(struct
> >> dma_buf_attachment *attach,
> >> if (!sgt)
> >> return;
> >>
> >> - dma_unmap_sgtable(attach->dev, sgt, dir, DMA_ATTR_SKIP_CPU_SYNC);
> >> + if (attach->dev->dma_mask)
> >> + dma_unmap_sgtable(attach->dev, sgt, dir,
> >> DMA_ATTR_SKIP_CPU_SYNC);
> >> +
> >> sg_free_table(sgt);
> >> kfree(sgt);
> >> }
> >> --
> >> 2.30.0
> >>
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> 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
