Messages in this thread | | | Date | Tue, 6 Sep 2022 12:43:39 +0200 | Subject | Re: [PATCH v2 4/4] dma-buf: Check status of enable-signaling bit on debug | From | Christian König <> |
| |
Am 06.09.22 um 12:20 schrieb Tvrtko Ursulin: > > On 06/09/2022 09:39, Christian König wrote: >> Am 05.09.22 um 18:35 schrieb Arvind Yadav: >>> The core DMA-buf framework needs to enable signaling >>> before the fence is signaled. The core DMA-buf framework >>> can forget to enable signaling before the fence is signaled. >> >> This sentence is a bit confusing. I'm not a native speaker of English >> either, but I suggest something like: >> >> "Fence signaling must be enable to make sure that the >> dma_fence_is_signaled() function ever returns true." >> >>> To avoid this scenario on the debug kernel, check the >>> DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT status bit before checking >>> the signaling bit status to confirm that enable_signaling >>> is enabled. >> >> This describes the implementation, but we should rather describe the >> background of the change. The implementation should be obvious. >> Something like this maybe: >> >> " >> Since drivers and implementations sometimes mess this up enforce >> correct behavior when DEBUG_WW_MUTEX_SLOWPATH is used during debugging. >> >> This should make any implementations bugs resulting in not signaled >> fences much more obvious. >> " > > I think I follow the idea but am not sure coupling (well "coupling".. > not really, but cross-contaminating in a way) dma-fence.c with a > foreign and effectively unrelated concept of a ww mutex is the best way. > > Instead, how about a dma-buf specific debug kconfig option?
Yeah, I was thinking about that as well.
The slowpath config option was just at hand because when you want to test the slowpath you want to test this here as well.
> > Condition would then be, according to my understanding of the rules > and expectations, along the lines of: > > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h > index 775cdc0b4f24..147a9df2c9d0 100644 > --- a/include/linux/dma-fence.h > +++ b/include/linux/dma-fence.h > @@ -428,6 +428,17 @@ dma_fence_is_signaled_locked(struct dma_fence > *fence) > static inline bool > dma_fence_is_signaled(struct dma_fence *fence) > { > +#ifdef CONFIG_DEBUG_DMAFENCE > + /* > + * Implementations not providing the enable_signaling callback > are > + * required to always have signaling enabled or fences are not > + * guaranteed to ever signal. > + */
Well that comment is a bit misleading. The intention of the extra check is to find bugs in the frontend and not the backend.
In other words somewhere in the drm_syncobj code we have a dma_fence_is_signaled() call without matching dma_fence_enable_sw_signaling().
Regards, Christian.
> + if (!fence->ops->enable_signaling && > + !test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags)) > + return false; > +#endif > + > if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) > return true; > > Thoughts? > > Regards, > > Tvrtko > >> >>> >>> Signed-off-by: Arvind Yadav <Arvind.Yadav@amd.com> >> >> With the improved commit message this patch is Reviewed-by: Christian >> König <christian.koenig@amd.com> >> >> Regards, >> Christian. >> >>> --- >>> >>> Changes in v1 : >>> 1- Addressing Christian's comment to replace >>> CONFIG_DEBUG_WW_MUTEX_SLOWPATH instead of CONFIG_DEBUG_FS. >>> 2- As per Christian's comment moving this patch at last so >>> The version of this patch is also changed and previously >>> it was [PATCH 1/4] >>> >>> >>> --- >>> include/linux/dma-fence.h | 5 +++++ >>> 1 file changed, 5 insertions(+) >>> >>> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h >>> index 775cdc0b4f24..ba1ddc14c5d4 100644 >>> --- a/include/linux/dma-fence.h >>> +++ b/include/linux/dma-fence.h >>> @@ -428,6 +428,11 @@ dma_fence_is_signaled_locked(struct dma_fence >>> *fence) >>> static inline bool >>> dma_fence_is_signaled(struct dma_fence *fence) >>> { >>> +#ifdef CONFIG_DEBUG_WW_MUTEX_SLOWPATH >>> + if (!test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags)) >>> + return false; >>> +#endif >>> + >>> if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) >>> return true; >>
| |