Linaro-mm-sig February 2021

linaro-mm-sig@lists.linaro.org

19 participants
33 discussions

Re: [Linaro-mm-sig] [PATCH] drm/ttm/ttm_bo: make ttm_bo_glob_use_count static

by Christian König

Am 23.02.21 um 09:54 schrieb Jiapeng Chong: > Fix the following sparse warning: > > drivers/gpu/drm/ttm/ttm_bo.c:53:10: warning: symbol > 'ttm_bo_glob_use_count' was not declared. Should it be static? IIRC we already have a patch for this on the mailing list and the mutex can be static as well. Christian. > > Reported-by: Abaci Robot <abaci(a)linux.alibaba.com> > Signed-off-by: Jiapeng Chong <jiapeng.chong(a)linux.alibaba.com> > --- > drivers/gpu/drm/ttm/ttm_bo.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c > index b65f4b1..107dd13 100644 > --- a/drivers/gpu/drm/ttm/ttm_bo.c > +++ b/drivers/gpu/drm/ttm/ttm_bo.c > @@ -50,7 +50,7 @@ > * ttm_global_mutex - protecting the global BO state > */ > DEFINE_MUTEX(ttm_global_mutex); > -unsigned ttm_bo_glob_use_count; > +static unsigned ttm_bo_glob_use_count; > struct ttm_bo_global ttm_bo_glob; > EXPORT_SYMBOL(ttm_bo_glob); >

3 years, 10 months

Re: [Linaro-mm-sig] [PATCH] drm/amdgpu/sdma5.2: Remove unnecessary conversion to bool

by Alex Deucher

Applied. Thanks! Alex On Sat, Feb 20, 2021 at 3:35 AM Jiapeng Chong <jiapeng.chong(a)linux.alibaba.com> wrote: > > Fix the following coccicheck warnings: > > ./drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c:1621:40-45: WARNING: conversion > to bool not needed here. > > ./drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c:1619:40-45: WARNING: conversion > to bool not needed here. > > Reported-by: Abaci Robot <abaci(a)linux.alibaba.com> > Signed-off-by: Jiapeng Chong <jiapeng.chong(a)linux.alibaba.com> > --- > drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c > index f1ba36a..9ed79a9 100644 > --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c > +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c > @@ -1616,9 +1616,9 @@ static int sdma_v5_2_set_clockgating_state(void *handle, > case CHIP_VANGOGH: > case CHIP_DIMGREY_CAVEFISH: > sdma_v5_2_update_medium_grain_clock_gating(adev, > - state == AMD_CG_STATE_GATE ? true : false); > + state == AMD_CG_STATE_GATE); > sdma_v5_2_update_medium_grain_light_sleep(adev, > - state == AMD_CG_STATE_GATE ? true : false); > + state == AMD_CG_STATE_GATE); > break; > default: > break; > -- > 1.8.3.1 > > _______________________________________________ > dri-devel mailing list > dri-devel(a)lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel

3 years, 10 months

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

by 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(a)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

3 years, 10 months

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

by 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(a)suse.de> > Fixes: 6eb0233ec2d0 ("usb: don't inherity DMA properties for USB devices") > Cc: Christoph Hellwig <hch(a)lst.de> > Cc: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> > Cc: Alan Stern <stern(a)rowland.harvard.edu> > Cc: Johan Hovold <johan(a)kernel.org> > Cc: Andy Shevchenko <andriy.shevchenko(a)linux.intel.com> > Cc: Sebastian Andrzej Siewior <bigeasy(a)linutronix.de> > Cc: Mathias Nyman <mathias.nyman(a)linux.intel.com> > Cc: "Ahmed S. Darwish" <a.darwish(a)linutronix.de> > Cc: Oliver Neukum <oneukum(a)suse.com> > Cc: Thomas Gleixner <tglx(a)linutronix.de> > Cc: Felipe Balbi <balbi(a)kernel.org> > Cc: <stable(a)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

3 years, 10 months

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

by Daniel Vetter

On Mon, Feb 22, 2021 at 2:24 PM Thomas Zimmermann <tzimmermann(a)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

3 years, 10 months

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

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

3 years, 10 months

[PATCH] drm-buf: Add debug option

by Daniel Vetter

We have too many people abusing the struct page they can get at but really shouldn't in importers. Aside from that the backing page might simply not exist (for dynamic p2p mappings) looking at it and using it e.g. for mmap can also wreak the page handling of the exporter completely. Importers really must go through the proper interface like dma_buf_mmap for everything. Just an RFC to see whether this idea has some stickiness. default y for now to make sure intel-gfx-ci picks it up too. I'm semi-tempted to enforce this for dynamic importers since those really have no excuse at all to break the rules. Unfortuantely we can't store the right pointers somewhere safe to make sure we oops on something recognizable, so best is to just wrangle them a bit by flipping all the bits. At least on x86 kernel addresses have all their high bits sets and the struct page array is fairly low in the kernel mapping, so flipping all the bits gives us a very high pointer in userspace and hence excellent chances for an invalid dereference. Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: David Stevens <stevensd(a)chromium.org> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org --- drivers/dma-buf/Kconfig | 8 +++++++ drivers/dma-buf/dma-buf.c | 49 +++++++++++++++++++++++++++++++++++---- 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig index 4f8224a6ac95..cddb549e5e59 100644 --- a/drivers/dma-buf/Kconfig +++ b/drivers/dma-buf/Kconfig @@ -50,6 +50,14 @@ config DMABUF_MOVE_NOTIFY This is marked experimental because we don't yet have a consistent execution context and memory management between drivers. +config DMABUF_DEBUG + bool "DMA-BUF debug checks" + default y + help + This option enables additional checks for DMA-BUF importers and + exporters. Specifically it validates that importers do not peek at the + underlying struct page when they import a buffer. + config DMABUF_SELFTESTS tristate "Selftests for the dma-buf interfaces" default n diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 1c9bd51db110..6e4725f7dfde 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -666,6 +666,30 @@ void dma_buf_put(struct dma_buf *dmabuf) } EXPORT_SYMBOL_GPL(dma_buf_put); +static struct sg_table * __map_dma_buf(struct dma_buf_attachment *attach, + enum dma_data_direction direction) +{ + struct sg_table *sg_table; + + sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction); + +#if CONFIG_DMABUF_DEBUG + if (sg_table) { + int i; + struct scatterlist *sg; + + /* To catch abuse of the underlying struct page by importers mix + * up the bits, but take care to preserve the low SG_ bits to + * not corrupt the sgt. The mixing is undone in __unmap_dma_buf + * before passing the sgt back to the exporter. */ + for_each_sgtable_sg(sg_table, sg, i) + sg->page_link ^= ~0xffUL; + } +#endif + + return sg_table; +} + /** * dma_buf_dynamic_attach - Add the device to dma_buf's attachments list * @dmabuf: [in] buffer to attach device to. @@ -737,7 +761,7 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, goto err_unlock; } - sgt = dmabuf->ops->map_dma_buf(attach, DMA_BIDIRECTIONAL); + sgt = __map_dma_buf(attach, DMA_BIDIRECTIONAL); if (!sgt) sgt = ERR_PTR(-ENOMEM); if (IS_ERR(sgt)) { @@ -784,6 +808,23 @@ struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, } EXPORT_SYMBOL_GPL(dma_buf_attach); +static void __unmap_dma_buf(struct dma_buf_attachment *attach, + struct sg_table *sg_table, + enum dma_data_direction direction) +{ + +#if CONFIG_DMABUF_DEBUG + if (sg_table) { + int i; + struct scatterlist *sg; + + for_each_sgtable_sg(sg_table, sg, i) + sg->page_link ^= ~0xffUL; + } +#endif + attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction); +} + /** * dma_buf_detach - Remove the given attachment from dmabuf's attachments list * @dmabuf: [in] buffer to detach from. @@ -802,7 +843,7 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) if (dma_buf_is_dynamic(attach->dmabuf)) dma_resv_lock(attach->dmabuf->resv, NULL); - dmabuf->ops->unmap_dma_buf(attach, attach->sgt, attach->dir); + __unmap_dma_buf(attach, attach->sgt, attach->dir); if (dma_buf_is_dynamic(attach->dmabuf)) { dma_buf_unpin(attach); @@ -924,7 +965,7 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, } } - sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction); + sg_table = __map_dma_buf(attach, direction); if (!sg_table) sg_table = ERR_PTR(-ENOMEM); @@ -987,7 +1028,7 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, if (dma_buf_is_dynamic(attach->dmabuf)) dma_resv_assert_held(attach->dmabuf->resv); - attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction); + __unmap_dma_buf(attach, sg_table, direction); if (dma_buf_is_dynamic(attach->dmabuf) && !IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY)) -- 2.29.2

3 years, 10 months

Re: [Linaro-mm-sig] DMA-buf and uncached system memory

by Christian König

Am 15.02.21 um 10:06 schrieb Simon Ser: > On Monday, February 15th, 2021 at 9:58 AM, Christian König <christian.koenig(a)amd.com> wrote: > >> we are currently working an Freesync and direct scan out from system >> memory on AMD APUs in A+A laptops. >> >> On problem we stumbled over is that our display hardware needs to scan >> out from uncached system memory and we currently don't have a way to >> communicate that through DMA-buf. >> >> For our specific use case at hand we are going to implement something >> driver specific, but the question is should we have something more >> generic for this? >> >> After all the system memory access pattern is a PCIe extension and as >> such something generic. > Intel also needs uncached system memory if I'm not mistaken? No idea, that's why I'm asking. Could be that this is also interesting for I+A systems. > Where are the buffers allocated? If GBM, then it needs to allocate memory that > can be scanned out if the USE_SCANOUT flag is set or if a scanout-capable > modifier is picked. > > If this is about communicating buffer constraints between different components > of the stack, there were a few proposals about it. The most recent one is [1]. Well the problem here is on a different level of the stack. See resolution, pitch etc:.. can easily communicated in userspace without involvement of the kernel. The worst thing which can happen is that you draw garbage into your own application window. But if you get the caching attributes in the page tables (both CPU as well as IOMMU, device etc...) wrong then ARM for example has the tendency to just spontaneously reboot X86 is fortunately a bit more gracefully and you only end up with random data corruption, but that is only marginally better. So to sum it up that is not something which we can leave in the hands of userspace. I think that exporters in the DMA-buf framework should have the ability to tell importers if the system memory snooping is necessary or not. Userspace components can then of course tell the exporter what the importer needs, but validation if that stuff is correct and doesn't crash the system must happen in the kernel. Regards, Christian. > > Simon > > [1]: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fxdc2020.x…

3 years, 10 months

Re: [Linaro-mm-sig] DMA-buf and uncached system memory

by Christian König

Am 15.02.21 um 13:00 schrieb Thomas Zimmermann: > Hi > > Am 15.02.21 um 10:49 schrieb Thomas Zimmermann: >> Hi >> >> Am 15.02.21 um 09:58 schrieb Christian König: >>> Hi guys, >>> >>> we are currently working an Freesync and direct scan out from system >>> memory on AMD APUs in A+A laptops. >>> >>> On problem we stumbled over is that our display hardware needs to >>> scan out from uncached system memory and we currently don't have a >>> way to communicate that through DMA-buf. > > Re-reading this paragrah, it sounds more as if you want to let the > exporter know where to move the buffer. Is this another case of the > missing-pin-flag problem? No, your original interpretation was correct. Maybe my writing is a bit unspecific. The real underlying issue is that our display hardware has a problem with latency when accessing system memory. So the question is if that also applies to for example Intel hardware or other devices as well or if it is just something AMD specific? Regards, Christian. > > Best regards > Thomas > >>> >>> For our specific use case at hand we are going to implement >>> something driver specific, but the question is should we have >>> something more generic for this? >> >> For vmap operations, we return the address as struct dma_buf_map, >> which contains additional information about the memory buffer. In >> vram helpers, we have the interface drm_gem_vram_offset() that >> returns the offset of the GPU device memory. >> >> Would it be feasible to combine both concepts into a dma-buf >> interface that returns the device-memory offset plus the additional >> caching flag? >> >> There'd be a structure and a getter function returning the structure. >> >> struct dma_buf_offset { >> bool cached; >> u64 address; >> }; >> >> // return offset in *off >> int dma_buf_offset(struct dma_buf *buf, struct dma_buf_off *off); >> >> Whatever settings are returned by dma_buf_offset() are valid while >> the dma_buf is pinned. >> >> Best regards >> Thomas >> >>> >>> After all the system memory access pattern is a PCIe extension and >>> as such something generic. >>> >>> Regards, >>> Christian. >>> _______________________________________________ >>> dri-devel mailing list >>> dri-devel(a)lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel(a)lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> >

3 years, 10 months

Re: [Linaro-mm-sig] [PATCH] RFC: dma-fence: Document recoverable page fault implications

by Daniel Vetter

On Tue, Feb 9, 2021 at 4:13 AM Bas Nieuwenhuizen <bas(a)basnieuwenhuizen.nl> wrote: > > On Thu, Jan 28, 2021 at 4:40 PM Felix Kuehling <felix.kuehling(a)amd.com> wrote: > > > > Am 2021-01-28 um 2:39 a.m. schrieb Christian König: > > > Am 27.01.21 um 23:00 schrieb Felix Kuehling: > > >> Am 2021-01-27 um 7:16 a.m. schrieb Christian König: > > >>> Am 27.01.21 um 13:11 schrieb Maarten Lankhorst: > > >>>> Op 27-01-2021 om 01:22 schreef Felix Kuehling: > > >>>>> Am 2021-01-21 um 2:40 p.m. schrieb Daniel Vetter: > > >>>>>> Recently there was a fairly long thread about recoreable hardware > > >>>>>> page > > >>>>>> faults, how they can deadlock, and what to do about that. > > >>>>>> > > >>>>>> While the discussion is still fresh I figured good time to try and > > >>>>>> document the conclusions a bit. > > >>>>>> > > >>>>>> References: > > >>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kern… > > >>>>>> > > >>>>>> Cc: Maarten Lankhorst <maarten.lankhorst(a)linux.intel.com> > > >>>>>> Cc: Thomas Hellström <thomas.hellstrom(a)intel.com> > > >>>>>> Cc: "Christian König" <christian.koenig(a)amd.com> > > >>>>>> Cc: Jerome Glisse <jglisse(a)redhat.com> > > >>>>>> Cc: Felix Kuehling <felix.kuehling(a)amd.com> > > >>>>>> Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> > > >>>>>> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> > > >>>>>> Cc: linux-media(a)vger.kernel.org > > >>>>>> Cc: linaro-mm-sig(a)lists.linaro.org > > >>>>>> -- > > >>>>>> I'll be away next week, but figured I'll type this up quickly for > > >>>>>> some > > >>>>>> comments and to check whether I got this all roughly right. > > >>>>>> > > >>>>>> Critique very much wanted on this, so that we can make sure hw which > > >>>>>> can't preempt (with pagefaults pending) like gfx10 has a clear > > >>>>>> path to > > >>>>>> support page faults in upstream. So anything I missed, got wrong or > > >>>>>> like that would be good. > > >>>>>> -Daniel > > >>>>>> --- > > >>>>>> Documentation/driver-api/dma-buf.rst | 66 > > >>>>>> ++++++++++++++++++++++++++++ > > >>>>>> 1 file changed, 66 insertions(+) > > >>>>>> > > >>>>>> diff --git a/Documentation/driver-api/dma-buf.rst > > >>>>>> b/Documentation/driver-api/dma-buf.rst > > >>>>>> index a2133d69872c..e924c1e4f7a3 100644 > > >>>>>> --- a/Documentation/driver-api/dma-buf.rst > > >>>>>> +++ b/Documentation/driver-api/dma-buf.rst > > >>>>>> @@ -257,3 +257,69 @@ fences in the kernel. This means: > > >>>>>> userspace is allowed to use userspace fencing or long running > > >>>>>> compute > > >>>>>> workloads. This also means no implicit fencing for shared > > >>>>>> buffers in these > > >>>>>> cases. > > >>>>>> + > > >>>>>> +Recoverable Hardware Page Faults Implications > > >>>>>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > >>>>>> + > > >>>>>> +Modern hardware supports recoverable page faults, which has a > > >>>>>> lot of > > >>>>>> +implications for DMA fences. > > >>>>>> + > > >>>>>> +First, a pending page fault obviously holds up the work that's > > >>>>>> running on the > > >>>>>> +accelerator and a memory allocation is usually required to resolve > > >>>>>> the fault. > > >>>>>> +But memory allocations are not allowed to gate completion of DMA > > >>>>>> fences, which > > >>>>>> +means any workload using recoverable page faults cannot use DMA > > >>>>>> fences for > > >>>>>> +synchronization. Synchronization fences controlled by userspace > > >>>>>> must be used > > >>>>>> +instead. > > >>>>>> + > > >>>>>> +On GPUs this poses a problem, because current desktop compositor > > >>>>>> protocols on > > >>>>>> +Linus rely on DMA fences, which means without an entirely new > > >>>>>> userspace stack > > >>>>>> +built on top of userspace fences, they cannot benefit from > > >>>>>> recoverable page > > >>>>>> +faults. The exception is when page faults are only used as > > >>>>>> migration hints and > > >>>>>> +never to on-demand fill a memory request. For now this means > > >>>>>> recoverable page > > >>>>>> +faults on GPUs are limited to pure compute workloads. > > >>>>>> + > > >>>>>> +Furthermore GPUs usually have shared resources between the 3D > > >>>>>> rendering and > > >>>>>> +compute side, like compute units or command submission engines. If > > >>>>>> both a 3D > > >>>>>> +job with a DMA fence and a compute workload using recoverable page > > >>>>>> faults are > > >>>>>> +pending they could deadlock: > > >>>>>> + > > >>>>>> +- The 3D workload might need to wait for the compute job to finish > > >>>>>> and release > > >>>>>> + hardware resources first. > > >>>>>> + > > >>>>>> +- The compute workload might be stuck in a page fault, because the > > >>>>>> memory > > >>>>>> + allocation is waiting for the DMA fence of the 3D workload to > > >>>>>> complete. > > >>>>>> + > > >>>>>> +There are a few ways to prevent this problem: > > >>>>>> + > > >>>>>> +- Compute workloads can always be preempted, even when a page > > >>>>>> fault is pending > > >>>>>> + and not yet repaired. Not all hardware supports this. > > >>>>>> + > > >>>>>> +- DMA fence workloads and workloads which need page fault handling > > >>>>>> have > > >>>>>> + independent hardware resources to guarantee forward progress. > > >>>>>> This could be > > >>>>>> + achieved through e.g. through dedicated engines and minimal > > >>>>>> compute unit > > >>>>>> + reservations for DMA fence workloads. > > >>>>>> + > > >>>>>> +- The reservation approach could be further refined by only > > >>>>>> reserving the > > >>>>>> + hardware resources for DMA fence workloads when they are > > >>>>>> in-flight. This must > > >>>>>> + cover the time from when the DMA fence is visible to other > > >>>>>> threads up to > > >>>>>> + moment when fence is completed through dma_fence_signal(). > > >>>>>> + > > >>>>>> +- As a last resort, if the hardware provides no useful reservation > > >>>>>> mechanics, > > >>>>>> + all workloads must be flushed from the GPU when switching > > >>>>>> between jobs > > >>>>>> + requiring DMA fences or jobs requiring page fault handling: This > > >>>>>> means all DMA > > >>>>>> + fences must complete before a compute job with page fault > > >>>>>> handling can be > > >>>>>> + inserted into the scheduler queue. And vice versa, before a DMA > > >>>>>> fence can be > > >>>>>> + made visible anywhere in the system, all compute workloads must > > >>>>>> be preempted > > >>>>>> + to guarantee all pending GPU page faults are flushed. > > >>>>> I thought of another possible workaround: > > >>>>> > > >>>>> * Partition the memory. Servicing of page faults will use a > > >>>>> separate > > >>>>> memory pool that can always be allocated from without > > >>>>> waiting for > > >>>>> fences. This includes memory for page tables and memory for > > >>>>> migrating data to. You may steal memory from other processes > > >>>>> that > > >>>>> can page fault, so no fence waiting is necessary. Being able to > > >>>>> steal memory at any time also means there are basically no > > >>>>> out-of-memory situations you need to worry about. Even page > > >>>>> tables > > >>>>> (except the root page directory of each process) can be > > >>>>> stolen in > > >>>>> the worst case. > > >>>> I think 'overcommit' would be a nice way to describe this. But I'm not > > >>>> sure how easy this is to implement in practice. You would basically > > >>>> need > > >>>> to create your own memory manager for this. > > >>> Well you would need a completely separate pool for both device as well > > >>> as system memory. > > >>> > > >>> E.g. on boot we say we steal X GB system memory only for HMM. > > >> Why? The GPU driver doesn't need to allocate system memory for HMM. > > >> Migrations to system memory are handled by the kernel's handle_mm_fault > > >> and page allocator and swap logic. > > > > > > And that one depends on dma_fence completion because you can easily > > > need to wait for an MMU notifier callback. > > > > I see, the GFX MMU notifier for userpointers in amdgpu currently waits > > for fences. For the KFD MMU notifier I am planning to fix this by > > causing GPU page faults instead of preempting the queues. Can we limit > > userptrs in amdgpu to engines that can page fault. Basically make it > > illegal to attach userptr BOs to graphics CS BO lists, so they can only > > be used in user mode command submissions, which can page fault. Then the > > GFX MMU notifier could invalidate PTEs and would not have to wait for > > fences. > > sadly graphics + userptr is already exposed via Mesa. This is not about userptr, we fake userptr entirely in software. It's about exposing recoverable gpu page faults (which would make userptr maybe more efficient since we could do on-demand paging). userptr itself isn't a problem, but it is part of the reasons why this is tricky. Christian/Felix, I think for kernel folks this is clear enough that I don't need to clarify this in the text? -Daniel > > > > > > > > > > > As Maarten wrote when you want to go down this route you need a > > > complete separate memory management parallel to the one of the kernel. > > > > Not really. I'm trying to make the GPU memory management more similar to > > what the kernel does for system memory. > > > > I understood Maarten's comment as "I'm creating a new memory manager and > > not using TTM any more". This is true. The idea is that this portion of > > VRAM would be managed more like system memory. > > > > Regards, > > Felix > > > > > > > > > > Regards, > > > Christian. > > > > > >> It doesn't depend on any fences, so > > >> it cannot deadlock with any GPU driver-managed memory. The GPU driver > > >> gets involved in the MMU notifier to invalidate device page tables. But > > >> that also doesn't need to wait for any fences. > > >> > > >> And if the kernel runs out of pageable memory, you're in trouble anyway. > > >> The OOM killer will step in, nothing new there. > > >> > > >> Regards, > > >> Felix > > >> > > >> > > >>>> But from a design point of view, definitely a valid solution. > > >>> I think the restriction above makes it pretty much unusable. > > >>> > > >>>> But this looks good, those solutions are definitely the valid > > >>>> options we > > >>>> can choose from. > > >>> It's certainly worth noting, yes. And just to make sure that nobody > > >>> has the idea to reserve only device memory. > > >>> > > >>> Christian. > > >>> > > >>>> ~Maarten > > >>>> > > >> _______________________________________________ > > >> Linaro-mm-sig mailing list > > >> Linaro-mm-sig(a)lists.linaro.org > > >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.lin… > > >> > > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel(a)lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 10 months

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig February 2021