Messages in this thread | | | Date | Tue, 6 Sep 2022 13:35:49 +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 13:21 schrieb Tvrtko Ursulin: > > On 06/09/2022 11:43, Christian König wrote: >> 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. > > Cool, lets see about the specifics below and then decide. > >> 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. > > By backend you mean drivers/dma-buf/dma-fence.c and by front end > driver specific implementations? Or simply users for dma-fence?
Backend are the driver specific implementation of the dma_fence_ops.
Middleware is the framework in drivers/dma-buf.
Frontend is the users of the dma_fence interface, e.g. either drivers or components (drm_syncobj, drm_scheduler etc...).
> > Could be that I got confused.. I was reading these two: > > > * This callback is optional. If this callback is not present, > then the > * driver must always have signaling enabled. > */ > bool (*enable_signaling)(struct dma_fence *fence); > > And dma_fence_is_signaled: > > * Returns true if the fence was already signaled, false if not. Since > this > * function doesn't enable signaling, it is not guaranteed to ever return > * true if dma_fence_add_callback(), dma_fence_wait() or > * dma_fence_enable_sw_signaling() haven't been called before. > > Right, I think I did get confused, apologies. What I was thinking was > probably two separate conditions: > > static inline bool > dma_fence_is_signaled(struct dma_fence *fence) > { > +#ifdef CONFIG_DEBUG_DMAFENCE > + if (WARN_ON_ONCE(!fence->ops->enable_signaling && > + !test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags))) > + return false; > + > + 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; > > Not sure "is signaled" is the best place for the first one or that it > should definitely be added.
I was thinking about adding this WARN_ON() as well, but then decided against it.
There are a couple of cases where calling dma_fence_is_signaled() without previously calling dma_fence_enable_sw_signaling() is valid because it is done just for optimization purposes and we guarantee that dma_fence_enable_sw_signaling() is called just a little bit later.
But yes, in general it's the same idea I already had as well.
Regards, Christian.
> > Regards, > > Tvrtko > >> 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; >>>> >>
| |