Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

15 participants
2903 discussions

Re: [PATCH v6 1/4] drm/panthor: Introduce BO labeling

by Liviu Dudau

On Wed, Apr 09, 2025 at 10:22:19PM +0100, Adrián Larumbe wrote: > Add a new character string Panthor BO field, and a function that allows > setting it from within the driver. > > Driver takes care of freeing the string when it's replaced or no longer > needed at object destruction time, but allocating it is the responsibility > of callers. > > Signed-off-by: Adrián Larumbe <adrian.larumbe(a)collabora.com> > Reviewed-by: Boris Brezillon <boris.brezillon(a)collabora.com> Reviewed-by: Liviu Dudau <liviu.dudau(a)arm.com> Best regards, Liviu > --- > drivers/gpu/drm/panthor/panthor_gem.c | 39 +++++++++++++++++++++++++++ > drivers/gpu/drm/panthor/panthor_gem.h | 17 ++++++++++++ > 2 files changed, 56 insertions(+) > > diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c > index 8244a4e6c2a2..af0ac17f357f 100644 > --- a/drivers/gpu/drm/panthor/panthor_gem.c > +++ b/drivers/gpu/drm/panthor/panthor_gem.c > @@ -2,6 +2,7 @@ > /* Copyright 2019 Linaro, Ltd, Rob Herring <robh(a)kernel.org> */ > /* Copyright 2023 Collabora ltd. */ > > +#include <linux/cleanup.h> > #include <linux/dma-buf.h> > #include <linux/dma-mapping.h> > #include <linux/err.h> > @@ -18,6 +19,14 @@ static void panthor_gem_free_object(struct drm_gem_object *obj) > struct panthor_gem_object *bo = to_panthor_bo(obj); > struct drm_gem_object *vm_root_gem = bo->exclusive_vm_root_gem; > > + /* > + * Label might have been allocated with kstrdup_const(), > + * we need to take that into account when freeing the memory > + */ > + kfree_const(bo->label.str); > + > + mutex_destroy(&bo->label.lock); > + > drm_gem_free_mmap_offset(&bo->base.base); > mutex_destroy(&bo->gpuva_list_lock); > drm_gem_shmem_free(&bo->base); > @@ -196,6 +205,7 @@ struct drm_gem_object *panthor_gem_create_object(struct drm_device *ddev, size_t > obj->base.map_wc = !ptdev->coherent; > mutex_init(&obj->gpuva_list_lock); > drm_gem_gpuva_set_lock(&obj->base.base, &obj->gpuva_list_lock); > + mutex_init(&obj->label.lock); > > return &obj->base.base; > } > @@ -247,3 +257,32 @@ panthor_gem_create_with_handle(struct drm_file *file, > > return ret; > } > + > +void > +panthor_gem_bo_set_label(struct drm_gem_object *obj, const char *label) > +{ > + struct panthor_gem_object *bo = to_panthor_bo(obj); > + const char *old_label; > + > + scoped_guard(mutex, &bo->label.lock) { > + old_label = bo->label.str; > + bo->label.str = label; > + } > + > + kfree(old_label); > +} > + > +void > +panthor_gem_kernel_bo_set_label(struct panthor_kernel_bo *bo, const char *label) > +{ > + const char *str; > + > + str = kstrdup_const(label, GFP_KERNEL); > + if (!str) { > + /* Failing to allocate memory for a label isn't a fatal condition */ > + drm_warn(bo->obj->dev, "Not enough memory to allocate BO label"); > + return; > + } > + > + panthor_gem_bo_set_label(bo->obj, str); > +} > diff --git a/drivers/gpu/drm/panthor/panthor_gem.h b/drivers/gpu/drm/panthor/panthor_gem.h > index 1a363bb814f4..af0d77338860 100644 > --- a/drivers/gpu/drm/panthor/panthor_gem.h > +++ b/drivers/gpu/drm/panthor/panthor_gem.h > @@ -46,6 +46,20 @@ struct panthor_gem_object { > > /** @flags: Combination of drm_panthor_bo_flags flags. */ > u32 flags; > + > + /** > + * @label: BO tagging fields. The label can be assigned within the > + * driver itself or through a specific IOCTL. > + */ > + struct { > + /** > + * @label.str: Pointer to NULL-terminated string, > + */ > + const char *str; > + > + /** @lock.str: Protects access to the @label.str field. */ > + struct mutex lock; > + } label; > }; > > /** > @@ -91,6 +105,9 @@ panthor_gem_create_with_handle(struct drm_file *file, > struct panthor_vm *exclusive_vm, > u64 *size, u32 flags, uint32_t *handle); > > +void panthor_gem_bo_set_label(struct drm_gem_object *obj, const char *label); > +void panthor_gem_kernel_bo_set_label(struct panthor_kernel_bo *bo, const char *label); > + > static inline u64 > panthor_kernel_bo_gpuva(struct panthor_kernel_bo *bo) > { > -- > 2.48.1 > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯

2 months, 2 weeks

Re: [PATCH 1/3] drm/nouveau: Prevent signaled fences in pending list

by Christian König

Am 10.04.25 um 11:24 schrieb Philipp Stanner: > Nouveau currently relies on the assumption that dma_fences will only > ever get signaled through nouveau_fence_signal(), which takes care of > removing a signaled fence from the list nouveau_fence_chan.pending. > > This self-imposed rule is violated in nouveau_fence_done(), where > dma_fence_is_signaled() (somewhat surprisingly, considering its name) > can signal the fence without removing it from the list. This enables > accesses to already signaled fences through the list, which is a bug. > > In particular, it can race with nouveau_fence_context_kill(), which > would then attempt to set an error code on an already signaled fence, > which is illegal. > > In nouveau_fence_done(), the call to nouveau_fence_update() already > ensures to signal all ready fences. Thus, the signaling potentially > performed by dma_fence_is_signaled() is actually not necessary. Ah, I now got what you are trying to do here! But that won't help. The problem is it is perfectly valid for somebody external (e.g. other driver, TTM etc...) to call dma_fence_is_signaled() on a nouveau fence. This will then in turn still signal the fence and leave it on the pending list and creating the problem you have. Regards, Christian. > > Replace the call to dma_fence_is_signaled() with > nouveau_fence_base_is_signaled(). > > Cc: <stable(a)vger.kernel.org> # 4.10+, precise commit not to be determined > Signed-off-by: Philipp Stanner <phasta(a)kernel.org> > --- > drivers/gpu/drm/nouveau/nouveau_fence.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c > index 7cc84472cece..33535987d8ed 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_fence.c > +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c > @@ -274,7 +274,7 @@ nouveau_fence_done(struct nouveau_fence *fence) > nvif_event_block(&fctx->event); > spin_unlock_irqrestore(&fctx->lock, flags); > } > - return dma_fence_is_signaled(&fence->base); > + return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->base.flags); > } > > static long

2 months, 2 weeks

Re: [PATCH 1/3] drm/nouveau: Prevent signaled fences in pending list

by Christian König

Am 10.04.25 um 14:21 schrieb Danilo Krummrich: > On Thu, Apr 10, 2025 at 02:13:34PM +0200, Christian König wrote: >> Am 10.04.25 um 11:24 schrieb Philipp Stanner: >>> Nouveau currently relies on the assumption that dma_fences will only >>> ever get signaled through nouveau_fence_signal(), which takes care of >>> removing a signaled fence from the list nouveau_fence_chan.pending. >>> >>> This self-imposed rule is violated in nouveau_fence_done(), where >>> dma_fence_is_signaled() (somewhat surprisingly, considering its name) >>> can signal the fence without removing it from the list. This enables >>> accesses to already signaled fences through the list, which is a bug. >>> >>> In particular, it can race with nouveau_fence_context_kill(), which >>> would then attempt to set an error code on an already signaled fence, >>> which is illegal. >>> >>> In nouveau_fence_done(), the call to nouveau_fence_update() already >>> ensures to signal all ready fences. Thus, the signaling potentially >>> performed by dma_fence_is_signaled() is actually not necessary. >>> >>> Replace the call to dma_fence_is_signaled() with >>> nouveau_fence_base_is_signaled(). >>> >>> Cc: <stable(a)vger.kernel.org> # 4.10+, precise commit not to be determined >>> Signed-off-by: Philipp Stanner <phasta(a)kernel.org> >>> --- >>> drivers/gpu/drm/nouveau/nouveau_fence.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c >>> index 7cc84472cece..33535987d8ed 100644 >>> --- a/drivers/gpu/drm/nouveau/nouveau_fence.c >>> +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c >>> @@ -274,7 +274,7 @@ nouveau_fence_done(struct nouveau_fence *fence) >>> nvif_event_block(&fctx->event); >>> spin_unlock_irqrestore(&fctx->lock, flags); >>> } >>> - return dma_fence_is_signaled(&fence->base); >>> + return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->base.flags); >> See the code above that: >> >> if (fence->base.ops == &nouveau_fence_ops_legacy || >> fence->base.ops == &nouveau_fence_ops_uevent) { > I think this check is a bit pointless given that fence is already a struct > nouveau_fence. :) Oh, good point. I totally missed that. In this case that indeed doesn't make any sense at all. (Unless somebody just blindly upcasted the structure, but I really hope that this isn't the case here). Regards, Christian.

2 months, 2 weeks

Re: [PATCH 0/3] drm/nouveau: Fix & improve nouveau_fence_done()

by Christian König

Am 10.04.25 um 11:51 schrieb Philipp Stanner: > On Thu, 2025-04-10 at 11:24 +0200, Philipp Stanner wrote: >> Contains two patches improving nouveau_fence_done(), and one >> addressing >> an actual bug (race): > Oops, that's the wrong calltrace. Here we go: > > [ 85.791794] Call Trace: [ 85.791796] <TASK> [ 85.791797] ? nouveau_fence_context_kill (/home/imperator/linux/./include/linux/dma-fence.h:587 (discriminator 9) /home/imperator/linux/drivers/gpu/drm/nouveau/nouveau_fence.c:94 (discriminator 9)) nouveau [ 85.791874] ? __warn.cold (/home/imperator/linux/kernel/panic.c:748) [ 85.791878] ? nouveau_fence_context_kill (/home/imperator/linux/./include/linux/dma-fence.h:587 (discriminator 9) /home/imperator/linux/drivers/gpu/drm/nouveau/nouveau_fence.c:94 (discriminator 9)) nouveau [ 85.791950] ? report_bug (/home/imperator/linux/lib/bug.c:180 /home/imperator/linux/lib/bug.c:219) [ 85.791953] ? handle_bug (/home/imperator/linux/arch/x86/kernel/traps.c:260) [ 85.791956] ? exc_invalid_op (/home/imperator/linux/arch/x86/kernel/traps.c:309 (discriminator 1)) [ 85.791957] ? asm_exc_invalid_op (/home/imperator/linux/./arch/x86/include/asm/idtentry.h:621) [ 85.791960] ? nouveau_fence_context_kill (/home/imperator/linux/./include/linux/dma-fence.h:587 (discriminator 9) /home/imperator/linux/drivers/gpu/drm/nouveau/nouveau_fence.c:94 (discriminator 9)) nouveau [ 85.792028] drm_sched_fini.cold (/home/imperator/linux/./include/trace/../../drivers/gpu/drm/scheduler/gpu_scheduler_trace.h:72 (discriminator 1)) gpu_sched [ 85.792033] ? drm_sched_entity_kill.part.0 (/home/imperator/linux/drivers/gpu/drm/scheduler/sched_entity.c:243 (discriminator 2)) gpu_sched [ 85.792037] nouveau_sched_destroy (/home/imperator/linux/drivers/gpu/drm/nouveau/nouveau_sched.c:509 /home/imperator/linux/drivers/gpu/drm/nouveau/nouveau_sched.c:518) nouveau [ 85.792122] nouveau_abi16_chan_fini.isra.0 (/home/imperator/linux/drivers/gpu/drm/nouveau/nouveau_abi16.c:188) nouveau [ 85.792191] nouveau_abi16_fini (/home/imperator/linux/drivers/gpu/drm/nouveau/nouveau_abi16.c:224 (discriminator 3)) nouveau [ 85.792263] nouveau_drm_postclose (/home/imperator/linux/drivers/gpu/drm/nouveau/nouveau_drm.c:1240) nouveau [ 85.792349] drm_file_free (/home/imperator/linux/drivers/gpu/drm/drm_file.c:255) [ 85.792353] drm_release (/home/imperator/linux/./arch/x86/include/asm/atomic.h:67 (discriminator 1) /home/imperator/linux/./include/linux/atomic/atomic-arch-fallback.h:2278 (discriminator 1) /home/imperator/linux/./include/linux/atomic/atomic-instrumented.h:1384 (discriminator 1) /home/imperator/linux/drivers/gpu/drm/drm_file.c:428 (discriminator 1)) [ 85.792355] __fput (/home/imperator/linux/fs/file_table.c:464) [ 85.792357] task_work_run (/home/imperator/linux/kernel/task_work.c:227) [ 85.792360] do_exit (/home/imperator/linux/kernel/exit.c:939) [ 85.792362] do_group_exit (/home/imperator/linux/kernel/exit.c:1069) [ 85.792364] get_signal (/home/imperator/linux/kernel/signal.c:3036) [ 85.792366] arch_do_signal_or_restart (/home/imperator/linux/./arch/x86/include/asm/syscall.h:38 /home/imperator/linux/arch/x86/kernel/signal.c:264 /home/imperator/linux/arch/x86/kernel/signal.c:339) [ 85.792369] syscall_exit_to_user_mode (/home/imperator/linux/kernel/entry/common.c:113 /home/imperator/linux/./include/linux/entry-common.h:329 /home/imperator/linux/kernel/entry/common.c:207 /home/imperator/linux/kernel/entry/common.c:218) [ 85.792372] do_syscall_64 (/home/imperator/linux/./arch/x86/include/asm/cpufeature.h:172 /home/imperator/linux/arch/x86/entry/common.c:98) [ 85.792373] ? syscall_exit_to_user_mode_prepare (/home/imperator/linux/./include/linux/audit.h:357 /home/imperator/linux/kernel/entry/common.c:166 /home/imperator/linux/kernel/entry/common.c:200) [ 85.792376] ? syscall_exit_to_user_mode (/home/imperator/linux/./arch/x86/include/asm/paravirt.h:686 /home/imperator/linux/./include/linux/entry-common.h:232 /home/imperator/linux/kernel/entry/common.c:206 /home/imperator/linux/kernel/entry/common.c:218) [ 85.792377] ? do_syscall_64 (/home/imperator/linux/./arch/x86/include/asm/cpufeature.h:172 /home/imperator/linux/arch/x86/entry/common.c:98) [ 85.792378] entry_SYSCALL_64_after_hwframe (/home/imperator/linux/arch/x86/entry/entry_64.S:130) [ 85.792381] RIP: 0033:0x7ff950b6af70 [ 85.792383] Code: Unable to access opcode bytes at 0x7ff950b6af46. objdump: '/tmp/tmp.sfPRl5k2te.o': No such file Code starting with the faulting instruction =========================================== [ 85.792383] RSP: 002b:00007ff93cdfb6f0 EFLAGS: 00000293 ORIG_RAX: 000000000000010f [ 85.792385] RAX: fffffffffffffdfe RBX: 000055d386d61870 RCX: 00007ff950b6af70 [ 85.792386] RDX: 0000000000000000 RSI: 0000000000000001 RDI: 00007ff928000b90 [ 85.792387] RBP: 00007ff93cdfb740 R08: 0000000000000008 R09: 0000000000000000 [ 85.792388] R10: 0000000000000000 R11: 0000000000000293 R12: 0000000000000001 [ 85.792388] R13: 0000000000000000 R14: 0000000000000000 R15: 00007ff951b10b40 [ 85.792390] </TASK> [ 85.792391] ---[ end trace 0000000000000000 ]--- I think I understand the problem now as well, but that backtrace is completely mangled in the mail. It would be nice if you could send that out again. Thanks, Christian. > > By the way, for reference: > I did try whether it could be done to have nouveau_fence_signal() > incorporated into nouveau_fence_update() and nouveau_fence_done(). > This, however, would then cause a race with the list_del() in > nouveau_fence_no_signaling(), WARNing because of the list poison. > > So the "solution" space is: > * A cleanup callback on the dma_fence. > * Keeping the current race or > * replacing it with another race with another function. > * Just preventing nouveau_fence_done() from signaling fences other > than through nouveau_fence_update/signal > > The later seems clearly like the cleanest solution to me. Alternative > would be a work-intensive rework of all the misdesigns broken in > nouveau_fence.c > > > P. > >> [ 39.848463] WARNING: CPU: 21 PID: 1734 at >> drivers/gpu/drm/nouveau/nouveau_fence.c:509 >> nouveau_fence_no_signaling+0xac/0xd0 [nouveau] >> [ 39.848551] Modules linked in: snd_seq_dummy snd_hrtimer >> nf_conntrack_netbios_ns nf_conntrack_broadcast nft_fib_inet >> nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_ine >> t nf_reject_ipv4 nf_reject_ipv6 nft_reject nft_ct nft_chain_nat >> nf_nat nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 rfkill ip_set >> nf_tables qrtr sunrpc snd_sof_pci_intel_ >> tgl snd_sof_pci_intel_cnl snd_sof_intel_hda_generic snd_sof_pci >> snd_sof_xtensa_dsp snd_sof_intel_hda_common snd_soc_hdac_hda >> snd_sof_intel_hda snd_sof snd_sof_utils snd >> _soc_acpi_intel_match snd_soc_acpi snd_soc_acpi_intel_sdca_quirks >> snd_sof_intel_hda_mlink snd_soc_sdca snd_soc_avs snd_ctl_led >> snd_soc_hda_codec intel_rapl_msr snd_hda_ >> codec_realtek snd_hda_ext_core intel_rapl_common >> snd_hda_codec_generic snd_soc_core snd_hda_scodec_component >> intel_uncore_frequency intel_uncore_frequency_common snd_hd >> a_codec_hdmi intel_ifs snd_compress i10nm_edac skx_edac_common nfit >> snd_hda_intel snd_intel_dspcfg libnvdimm snd_hda_codec binfmt_misc >> snd_hwdep snd_hda_core snd_seq sn >> d_seq_device dell_wmi >> [ 39.848575] dell_pc x86_pkg_temp_thermal spi_nor platform_profile >> sparse_keymap intel_powerclamp dax_hmem snd_pcm cxl_acpi coretemp >> cxl_port iTCO_wdt mtd rapl intel >> _pmc_bxt pmt_telemetry cxl_core dell_wmi_sysman pmt_class >> iTCO_vendor_support snd_timer isst_if_mmio vfat intel_cstate >> dell_smbios dcdbas fat dell_wmi_ddv dell_smm_hwmo >> n dell_wmi_descriptor firmware_attributes_class wmi_bmof intel_uncore >> einj pcspkr isst_if_mbox_pci atlantic snd isst_if_common intel_vsec >> e1000e macsec mei_me i2c_i801 >> spi_intel_pci soundcore i2c_smbus spi_intel mei joydev loop nfnetlink >> zram nouveau drm_ttm_helper ttm polyval_clmulni iaa_crypto gpu_sched >> polyval_generic rtsx_pci_sdmm >> c ghash_clmulni_intel i2c_algo_bit mmc_core drm_gpuvm sha512_ssse3 >> nvme drm_exec drm_display_helper sha256_ssse3 idxd sha1_ssse3 cec >> nvme_core idxd_bus rtsx_pci nvme_au >> th pinctrl_alderlake ip6_tables ip_tables fuse >> [ 39.848603] CPU: 21 UID: 42 PID: 1734 Comm: gnome-shell Tainted: >> G W 6.14.0-rc4+ #11 >> [ 39.848605] Tainted: [W]=WARN >> [ 39.848606] Hardware name: Dell Inc. Precision 7960 Tower/01G0M6, >> BIOS 2.7.0 12/17/2024 >> [ 39.848607] RIP: 0010:nouveau_fence_no_signaling+0xac/0xd0 >> [nouveau] >> [ 39.848688] Code: db 74 17 48 8d 7b 38 b8 ff ff ff ff f0 0f c1 43 >> 38 83 f8 01 74 29 85 c0 7e 17 31 c0 5b 5d c3 cc cc cc cc e8 76 b2 c5 >> f0 eb 96 <0f> 0b e9 67 ff ff f >> f be 03 00 00 00 e8 83 76 33 f1 31 c0 eb dd e8 >> [ 39.848690] RSP: 0018:ff1cc1ffc5c039f0 EFLAGS: 00010046 >> [ 39.848691] RAX: 0000000000000001 RBX: ff175a3b504da980 RCX: >> ff175a3b4801e008 >> [ 39.848692] RDX: ff175a3b43e7bad0 RSI: ffffffffc09d3fda RDI: >> ff175a3b504da980 >> [ 39.848693] RBP: ff175a3b504da9c0 R08: ffffffffc09e39df R09: >> 0000000000000001 >> [ 39.848694] R10: 0000000000000001 R11: 0000000000000000 R12: >> ff175a3b6d97de00 >> [ 39.848695] R13: 0000000000000246 R14: ff1cc1ffc5c03c60 R15: >> 0000000000000001 >> [ 39.848696] FS: 00007fc5477846c0(0000) GS:ff175a5a50280000(0000) >> knlGS:0000000000000000 >> [ 39.848698] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >> [ 39.848699] CR2: 000055cb7613d1a8 CR3: 000000012e5ce004 CR4: >> 0000000000f71ef0 >> [ 39.848700] DR0: 0000000000000000 DR1: 0000000000000000 DR2: >> 0000000000000000 >> [ 39.848701] DR3: 0000000000000000 DR6: 00000000fffe07f0 DR7: >> 0000000000000400 >> [ 39.848702] PKRU: 55555554 >> [ 39.848703] Call Trace: >> [ 39.848704] <TASK> >> [ 39.848705] ? nouveau_fence_no_signaling+0xac/0xd0 [nouveau] >> [ 39.848782] ? __warn.cold+0x93/0xfa >> [ 39.848785] ? nouveau_fence_no_signaling+0xac/0xd0 [nouveau] >> [ 39.848861] ? report_bug+0xff/0x140 >> [ 39.848863] ? handle_bug+0x58/0x90 >> [ 39.848865] ? exc_invalid_op+0x17/0x70 >> [ 39.848866] ? asm_exc_invalid_op+0x1a/0x20 >> [ 39.848870] ? nouveau_fence_no_signaling+0xac/0xd0 [nouveau] >> [ 39.848943] nouveau_fence_enable_signaling+0x32/0x80 [nouveau] >> [ 39.849016] ? __pfx_nouveau_fence_cleanup_cb+0x10/0x10 [nouveau] >> [ 39.849088] __dma_fence_enable_signaling+0x33/0xc0 >> [ 39.849090] dma_fence_add_callback+0x4b/0xd0 >> [ 39.849093] nouveau_fence_emit+0xa3/0x260 [nouveau] >> [ 39.849166] nouveau_fence_new+0x7d/0xf0 [nouveau] >> [ 39.849242] nouveau_gem_ioctl_pushbuf+0xe8f/0x1300 [nouveau] >> [ 39.849338] ? __pfx_nouveau_gem_ioctl_pushbuf+0x10/0x10 [nouveau] >> [ 39.849431] drm_ioctl_kernel+0xad/0x100 >> [ 39.849433] drm_ioctl+0x288/0x550 >> [ 39.849435] ? __pfx_nouveau_gem_ioctl_pushbuf+0x10/0x10 [nouveau] >> [ 39.849526] nouveau_drm_ioctl+0x57/0xb0 [nouveau] >> [ 39.849620] __x64_sys_ioctl+0x94/0xc0 >> [ 39.849621] do_syscall_64+0x82/0x160 >> [ 39.849623] ? drm_ioctl+0x2b7/0x550 >> [ 39.849625] ? __pfx_nouveau_gem_ioctl_pushbuf+0x10/0x10 [nouveau] >> [ 39.849719] ? ktime_get_mono_fast_ns+0x38/0xd0 >> [ 39.849721] ? __pm_runtime_suspend+0x69/0xc0 >> [ 39.849724] ? syscall_exit_to_user_mode_prepare+0x15e/0x1a0 >> [ 39.849726] ? syscall_exit_to_user_mode+0x10/0x200 >> [ 39.849729] ? do_syscall_64+0x8e/0x160 >> [ 39.849730] ? exc_page_fault+0x7e/0x1a0 >> [ 39.849733] entry_SYSCALL_64_after_hwframe+0x76/0x7e >> [ 39.849735] RIP: 0033:0x7fc5576fe0ad >> [ 39.849736] Code: 04 25 28 00 00 00 48 89 45 c8 31 c0 48 8d 45 10 >> c7 45 b0 10 00 00 00 48 89 45 b8 48 8d 45 d0 48 89 45 c0 b8 10 00 00 >> 00 0f 05 <89> c2 3d 00 f0 ff ff 77 1a 48 8b 45 c8 64 48 2b 04 25 28 >> 00 00 00 >> [ 39.849737] RSP: 002b:00007ffc002688a0 EFLAGS: 00000246 ORIG_RAX: >> 0000000000000010 >> [ 39.849739] RAX: ffffffffffffffda RBX: 000055cb74e316c0 RCX: >> 00007fc5576fe0ad >> [ 39.849740] RDX: 00007ffc00268960 RSI: 00000000c0406481 RDI: >> 000000000000000e >> [ 39.849741] RBP: 00007ffc002688f0 R08: 0000000000000000 R09: >> 000055cb74e35560 >> [ 39.849742] R10: 0000000000000014 R11: 0000000000000246 R12: >> 00007ffc00268960 >> [ 39.849744] R13: 00000000c0406481 R14: 000000000000000e R15: >> 000055cb74e3cd10 >> [ 39.849746] </TASK> >> [ 39.849746] ---[ end trace 0000000000000000 ]--- >> [ 39.849776] ------------[ cut here ]------------ >> >> >> This is the first WARN_ON() in dma_fence_set_error(), called by >> nouveau_fence_context_kill(). >> >> It's rare, but it is a bug, or rather: the archetype of a race, since >> (as Christian pointed out) nouveau_fence_update() later at some point >> will remove the signaled fence (by signaling it again). >> >> >> P. >> >> >> Philipp Stanner (3): >> drm/nouveau: Prevent signaled fences in pending list >> drm/nouveau: Remove surplus if-branch >> drm/nouveau: Add helper to check base fence >> >> drivers/gpu/drm/nouveau/nouveau_fence.c | 32 ++++++++++++++--------- >> -- >> 1 file changed, 18 insertions(+), 14 deletions(-) >>

2 months, 2 weeks

Re: [PATCH 2/3] drm/nouveau: Remove surplus if-branch

by Christian König

Am 10.04.25 um 11:24 schrieb Philipp Stanner: > nouveau_fence_done() contains an if-branch which checks for the > existence of either of two fence backend ops. Those two are the only > backend ops existing in Nouveau, however; and at least one backend ops > must be in use for the entire driver to be able to work. The if branch > is, therefore, surplus. > > Remove the if-branch. What happens here is that nouveau checks if the fence comes from itself or some external source. So when you remove that check you potentially illegally uses nouveau_fctx() on a non-nouveau fence. Regards, Christian. > > Signed-off-by: Philipp Stanner <phasta(a)kernel.org> > --- > drivers/gpu/drm/nouveau/nouveau_fence.c | 24 +++++++++++------------- > 1 file changed, 11 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c > index 33535987d8ed..db6f4494405c 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_fence.c > +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c > @@ -259,21 +259,19 @@ nouveau_fence_emit(struct nouveau_fence *fence) > bool > nouveau_fence_done(struct nouveau_fence *fence) > { > - if (fence->base.ops == &nouveau_fence_ops_legacy || > - fence->base.ops == &nouveau_fence_ops_uevent) { > - struct nouveau_fence_chan *fctx = nouveau_fctx(fence); > - struct nouveau_channel *chan; > - unsigned long flags; > + struct nouveau_fence_chan *fctx = nouveau_fctx(fence); > + struct nouveau_channel *chan; > + unsigned long flags; > > - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->base.flags)) > - return true; > + if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->base.flags)) > + return true; > + > + spin_lock_irqsave(&fctx->lock, flags); > + chan = rcu_dereference_protected(fence->channel, lockdep_is_held(&fctx->lock)); > + if (chan && nouveau_fence_update(chan, fctx)) > + nvif_event_block(&fctx->event); > + spin_unlock_irqrestore(&fctx->lock, flags); > > - spin_lock_irqsave(&fctx->lock, flags); > - chan = rcu_dereference_protected(fence->channel, lockdep_is_held(&fctx->lock)); > - if (chan && nouveau_fence_update(chan, fctx)) > - nvif_event_block(&fctx->event); > - spin_unlock_irqrestore(&fctx->lock, flags); > - } > return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->base.flags); > } >

2 months, 2 weeks

Re: [PATCH 1/3] drm/nouveau: Prevent signaled fences in pending list

by Christian König

Am 10.04.25 um 11:24 schrieb Philipp Stanner: > Nouveau currently relies on the assumption that dma_fences will only > ever get signaled through nouveau_fence_signal(), which takes care of > removing a signaled fence from the list nouveau_fence_chan.pending. > > This self-imposed rule is violated in nouveau_fence_done(), where > dma_fence_is_signaled() (somewhat surprisingly, considering its name) > can signal the fence without removing it from the list. This enables > accesses to already signaled fences through the list, which is a bug. > > In particular, it can race with nouveau_fence_context_kill(), which > would then attempt to set an error code on an already signaled fence, > which is illegal. > > In nouveau_fence_done(), the call to nouveau_fence_update() already > ensures to signal all ready fences. Thus, the signaling potentially > performed by dma_fence_is_signaled() is actually not necessary. > > Replace the call to dma_fence_is_signaled() with > nouveau_fence_base_is_signaled(). > > Cc: <stable(a)vger.kernel.org> # 4.10+, precise commit not to be determined > Signed-off-by: Philipp Stanner <phasta(a)kernel.org> > --- > drivers/gpu/drm/nouveau/nouveau_fence.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c > index 7cc84472cece..33535987d8ed 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_fence.c > +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c > @@ -274,7 +274,7 @@ nouveau_fence_done(struct nouveau_fence *fence) > nvif_event_block(&fctx->event); > spin_unlock_irqrestore(&fctx->lock, flags); > } > - return dma_fence_is_signaled(&fence->base); > + return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->base.flags); See the code above that: if (fence->base.ops == &nouveau_fence_ops_legacy || fence->base.ops == &nouveau_fence_ops_uevent) { .... Nouveau first tests if it's one of it's own fences, and if yes does some special handling. E.g. checking the fence status bits etc... So this dma_fence_is_signaled() is for all non-nouveau fences and then not touching the internal flags is perfectly correct as far as I can see. Regards, Christian. > } > > static long

2 months, 2 weeks

Re: [PATCH 1/2] dma-fence: Rename dma_fence_is_signaled()

by Christian König

Am 09.04.25 um 17:04 schrieb Philipp Stanner: > On Wed, 2025-04-09 at 16:10 +0200, Christian König wrote: >>> I only see improvement by making things more obvious. >>> >>> In any case, how would you call a wrapper that just does >>> test_bit(IS_SIGNALED, …) ? >> Broken, that was very intentionally removed quite shortly after we >> created the framework. >> >> We have a few cases were implementations do check that for their >> fences, but consumers should never be allowed to touch such >> internals. > There is theory and there is practice. In practice, those internals are > being used by Nouveau, i915, Xe, vmgfx and radeon. What do you mean? I only skimmed over the use cases, but as far as I can see those are all valid. You can test the flag if you know what the fence means to you, that is not a problem at all. > So it seems that we failed quite a bit at communicating clearly how the > interface should be used. > > And, to repeat myself, with both name and docu of that function, I > think it is very easy to misunderstand what it's doing. You say that it > shouldn't matter – and maybe that's true, in theory. In practice, it > does matter. In practice, APIs get misused and have side-effects. And > making that harder is desirable. That sounds like I didn't used the right wording. It *must* not matter to the consumer. See the purpose of the DMA-fence framework is to make it irrelevant for the consumer how the provider has implemented it's fences. This means that things like if polling or interrupt driven signaling is used, 32bit vs 64bit seq numbers, etc... should all be hidden by the framework from the consumer of the fences. BTW I'm actually not sure if nouveau has a bug here. As far as I can see nouveau_fence_signal() will be called later eventually and do the necessary cleanup. But on the other hand it wouldn't surprise me if nouveau has a bug with that. The driver has been basically only barely maintained for quite a while. > In any case, I might have to add another such call to Nouveau, because > the solution preferred by you over the callback causes another race. > Certainly one could solve this in a clean way, but someone has to do > the work, and we're talking about more than a few hours here. Well this is not my preferred solution, it's just the technical correct solution as far as I can see. > In any case, be so kind and look at patch 2 and tell me there if you're > at least OK with making the documentation more detailed. As far as I can see that is clearly the wrong place to document that stuff. Regards, Christian. > > P.

2 months, 2 weeks

Re: [PATCH v6 05/10] tee: implement restricted DMA-heap

by Jens Wiklander

On Wed, Apr 9, 2025 at 2:50 PM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > On Tue, Apr 08, 2025 at 03:28:45PM +0200, Jens Wiklander wrote: > > On Tue, Apr 8, 2025 at 11:14 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > > > > > On Tue, Apr 01, 2025 at 10:33:04AM +0200, Jens Wiklander wrote: > > > > On Tue, Apr 1, 2025 at 9:58 AM Sumit Garg <sumit.garg(a)kernel.org> wrote: > > > > > > > > > > On Tue, Mar 25, 2025 at 11:55:46AM +0100, Jens Wiklander wrote: > > > > > > Hi Sumit, > > > > > > > > > > > > > > > > <snip> > > > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > > +#include "tee_private.h" > > > > > > > > + > > > > > > > > +struct tee_dma_heap { > > > > > > > > + struct dma_heap *heap; > > > > > > > > + enum tee_dma_heap_id id; > > > > > > > > + struct tee_rstmem_pool *pool; > > > > > > > > + struct tee_device *teedev; > > > > > > > > + /* Protects pool and teedev above */ > > > > > > > > + struct mutex mu; > > > > > > > > +}; > > > > > > > > + > > > > > > > > +struct tee_heap_buffer { > > > > > > > > + struct tee_rstmem_pool *pool; > > > > > > > > + struct tee_device *teedev; > > > > > > > > + size_t size; > > > > > > > > + size_t offs; > > > > > > > > + struct sg_table table; > > > > > > > > +}; > > > > > > > > + > > > > > > > > +struct tee_heap_attachment { > > > > > > > > + struct sg_table table; > > > > > > > > + struct device *dev; > > > > > > > > +}; > > > > > > > > + > > > > > > > > +struct tee_rstmem_static_pool { > > > > > > > > + struct tee_rstmem_pool pool; > > > > > > > > + struct gen_pool *gen_pool; > > > > > > > > + phys_addr_t pa_base; > > > > > > > > +}; > > > > > > > > + > > > > > > > > +#if !IS_MODULE(CONFIG_TEE) && IS_ENABLED(CONFIG_DMABUF_HEAPS) > > > > > > > > > > > > > > Can this dependency rather be better managed via Kconfig? > > > > > > > > > > > > This was the easiest yet somewhat flexible solution I could find. If > > > > > > you have something better, let's use that instead. > > > > > > > > > > > > > > > > --- a/drivers/tee/optee/Kconfig > > > > > +++ b/drivers/tee/optee/Kconfig > > > > > @@ -5,6 +5,7 @@ config OPTEE > > > > > depends on HAVE_ARM_SMCCC > > > > > depends on MMU > > > > > depends on RPMB || !RPMB > > > > > + select DMABUF_HEAPS > > > > > help > > > > > This implements the OP-TEE Trusted Execution Environment (TEE) > > > > > driver. > > > > > > > > I wanted to avoid that since there are plenty of use cases where > > > > DMABUF_HEAPS aren't needed. > > > > > > Yeah, but how the users will figure out the dependency to enable DMA > > > heaps with TEE subsystem. > > > > I hope, without too much difficulty. They are after all looking for a > > way to allocate memory from a DMA heap. > > > > > So it's better we provide a generic kernel > > > Kconfig which enables all the default features. > > > > I disagree, it should be possible to configure without DMABUF_HEAPS if desired. > > It's hard to see a use-case for that additional compile time option. If > you are worried about kernel size then those can be built as modules. On > the other hand the benifit is that we avoid ifdefery and providing sane > TEE defaults where features can be detected and enabled at runtime > instead. My primary concern isn't kernel size, even if it shouldn't be irrelevant. It doesn't seem right to enable features that are not asked for casually. In this case, it's not unreasonable or unexpected that DMABUF_HEAPS must be explicitly enabled in the config if a heap interface is needed. It's the same as before this patch set. > > > > > > > > > > This seems to do the job: > > > > +config TEE_DMABUF_HEAP > > > > + bool > > > > + depends on TEE = y && DMABUF_HEAPS > > > > > > > > We can only use DMABUF_HEAPS if the TEE subsystem is compiled into the kernel. > > > > > > Ah, I see. So we aren't exporting the DMA heaps APIs for TEE subsystem > > > to use. We should do that such that there isn't a hard dependency to > > > compile them into the kernel. > > > > I was saving that for a later patch set as a later problem. We may > > save some time by not doing it now. > > > > But I think it's not a correct way to just reuse internal APIs from DMA > heaps subsystem without exporting them. It can be seen as a inter > subsystem API contract breach. I hope it won't be an issue with DMA heap > maintainers regarding export of those APIs. Fair enough. I'll add a patch in the next patch set for that. I guess the same goes for CMA. Cheers, Jens

2 months, 2 weeks

Re: [PATCH 1/2] dma-fence: Rename dma_fence_is_signaled()

by Christian König

Am 09.04.25 um 16:01 schrieb Philipp Stanner: > On Wed, 2025-04-09 at 15:14 +0200, Christian König wrote: >> Am 09.04.25 um 14:56 schrieb Philipp Stanner: >>> On Wed, 2025-04-09 at 14:51 +0200, Philipp Stanner wrote: >>>> On Wed, 2025-04-09 at 14:39 +0200, Boris Brezillon wrote: >>>>> Hi Philipp, >>>>> >>>>> On Wed, 9 Apr 2025 14:06:37 +0200 >>>>> Philipp Stanner <phasta(a)kernel.org> wrote: >>>>> >>>>>> dma_fence_is_signaled()'s name strongly reads as if this >>>>>> function >>>>>> were >>>>>> intended for checking whether a fence is already signaled. >>>>>> Also >>>>>> the >>>>>> boolean it returns hints at that. >>>>>> >>>>>> The function's behavior, however, is more complex: it can >>>>>> check >>>>>> with a >>>>>> driver callback whether the hardware's sequence number >>>>>> indicates >>>>>> that >>>>>> the fence can already be treated as signaled, although the >>>>>> hardware's / >>>>>> driver's interrupt handler has not signaled it yet. If that's >>>>>> the >>>>>> case, >>>>>> the function also signals the fence. >>>>>> >>>>>> (Presumably) this has caused a bug in Nouveau (unknown >>>>>> commit), >>>>>> where >>>>>> nouveau_fence_done() uses the function to check a fence, >>>>>> which >>>>>> causes a >>>>>> race. >>>>>> >>>>>> Give the function a more obvious name. >>>>> This is just my personal view on this, but I find the new name >>>>> just >>>>> as >>>>> confusing as the old one. It sounds like something is checked, >>>>> but >>>>> it's >>>>> clear what, and then the fence is forcibly signaled like it >>>>> would >>>>> be >>>>> if >>>>> you call drm_fence_signal(). Of course, this clarified by the >>>>> doc, >>>>> but >>>>> given the goal was to make the function name clearly reflect >>>>> what >>>>> it >>>>> does, I'm not convinced it's significantly better. >>>>> >>>>> Maybe dma_fence_check_hw_state_and_propagate(), though it might >>>>> be >>>>> too long of name. Oh well, feel free to ignore this comments if >>>>> a >>>>> majority is fine with the new name. >>>> Yoa, the name isn't perfect (the perfect name describing the >>>> whole >>>> behavior would be >>>> dma_fence_check_if_already_signaled_then_check_hardware_state_and >>>> _pro >>>> pa >>>> gate() ^^' >>>> >>>> My intention here is to have the reader realize "watch out, the >>>> fence >>>> might get signaled here!", which is probably the most important >>>> event >>>> regarding fences, which can race, invoke the callbacks and so on. >>>> >>>> For details readers will then check the documentation. >>>> >>>> But I'm of course open to see if there's a majority for this or >>>> that >>>> name. >>> how about: >>> >>> dma_fence_check_hw_and_signal() ? >> I don't think that renaming the function is a good idea in the first >> place. >> >> What the function does internally is an implementation detail of the >> framework. >> >> For the code using this function it's completely irrelevant if the >> function might also signal the fence, what matters for the caller is >> the returned status of the fence. I think this also counts for the >> dma_fence_is_signaled() documentation. > It does obviously matter. As it's currently implemented, a lot of > important things happen implicitly. Yeah, but that's ok. The code who calls this is the consumer of the interface and so shouldn't need to know this. That's why we have created the DMA fence framework in the first place. For the provider side when a driver or similar implements the interface the relevant documentation is the dma_fence_ops structure. > I only see improvement by making things more obvious. > > In any case, how would you call a wrapper that just does > test_bit(IS_SIGNALED, …) ? Broken, that was very intentionally removed quite shortly after we created the framework. We have a few cases were implementations do check that for their fences, but consumers should never be allowed to touch such internals. Regards, Christian. > > P. > >> What we should improve is the documentation of the dma_fence_ops- >>> enable_signaling and dma_fence_ops->signaled callbacks. >> Especially see the comment about reference counts on enable_signaling >> which is missing on the signaled callback. That is most likely the >> root cause why nouveau implemented enable_signaling correctly but not >> the other one. >> >> But putting that aside I think we should make nails with heads and >> let the framework guarantee that the fences stay alive until they are >> signaled (one way or another). This completely removes the burden to >> keep a reference on unsignaled fences from the drivers / >> implementations and make things more over all more defensive. >> >> Regards, >> Christian. >> >>> P. >>> >>>> P. >>>> >>>> >>>>> Regards, >>>>> >>>>> Boris

2 months, 2 weeks

Re: [PATCH 1/2] dma-fence: Rename dma_fence_is_signaled()

by Christian König

Am 09.04.25 um 14:56 schrieb Philipp Stanner: > On Wed, 2025-04-09 at 14:51 +0200, Philipp Stanner wrote: >> On Wed, 2025-04-09 at 14:39 +0200, Boris Brezillon wrote: >>> Hi Philipp, >>> >>> On Wed, 9 Apr 2025 14:06:37 +0200 >>> Philipp Stanner <phasta(a)kernel.org> wrote: >>> >>>> dma_fence_is_signaled()'s name strongly reads as if this function >>>> were >>>> intended for checking whether a fence is already signaled. Also >>>> the >>>> boolean it returns hints at that. >>>> >>>> The function's behavior, however, is more complex: it can check >>>> with a >>>> driver callback whether the hardware's sequence number indicates >>>> that >>>> the fence can already be treated as signaled, although the >>>> hardware's / >>>> driver's interrupt handler has not signaled it yet. If that's the >>>> case, >>>> the function also signals the fence. >>>> >>>> (Presumably) this has caused a bug in Nouveau (unknown commit), >>>> where >>>> nouveau_fence_done() uses the function to check a fence, which >>>> causes a >>>> race. >>>> >>>> Give the function a more obvious name. >>> This is just my personal view on this, but I find the new name just >>> as >>> confusing as the old one. It sounds like something is checked, but >>> it's >>> clear what, and then the fence is forcibly signaled like it would >>> be >>> if >>> you call drm_fence_signal(). Of course, this clarified by the doc, >>> but >>> given the goal was to make the function name clearly reflect what >>> it >>> does, I'm not convinced it's significantly better. >>> >>> Maybe dma_fence_check_hw_state_and_propagate(), though it might be >>> too long of name. Oh well, feel free to ignore this comments if a >>> majority is fine with the new name. >> Yoa, the name isn't perfect (the perfect name describing the whole >> behavior would be >> dma_fence_check_if_already_signaled_then_check_hardware_state_and_pro >> pa >> gate() ^^' >> >> My intention here is to have the reader realize "watch out, the fence >> might get signaled here!", which is probably the most important event >> regarding fences, which can race, invoke the callbacks and so on. >> >> For details readers will then check the documentation. >> >> But I'm of course open to see if there's a majority for this or that >> name. > how about: > > dma_fence_check_hw_and_signal() ? I don't think that renaming the function is a good idea in the first place. What the function does internally is an implementation detail of the framework. For the code using this function it's completely irrelevant if the function might also signal the fence, what matters for the caller is the returned status of the fence. I think this also counts for the dma_fence_is_signaled() documentation. What we should improve is the documentation of the dma_fence_ops->enable_signaling and dma_fence_ops->signaled callbacks. Especially see the comment about reference counts on enable_signaling which is missing on the signaled callback. That is most likely the root cause why nouveau implemented enable_signaling correctly but not the other one. But putting that aside I think we should make nails with heads and let the framework guarantee that the fences stay alive until they are signaled (one way or another). This completely removes the burden to keep a reference on unsignaled fences from the drivers / implementations and make things more over all more defensive. Regards, Christian. > > P. > >> P. >> >> >>> Regards, >>> >>> Boris

2 months, 2 weeks

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig