| Message ID | 20220927172409.484061-1-Arvind.Yadav@amd.com |
|---|---|
| Headers | show |
| Series | dma-buf: Check status of enable-signaling bit on debug | expand |
Am 27.09.22 um 19:24 schrieb Arvind Yadav: > Remove the extra signaled bit status check because > it is returning early when the fence is already signaled and > __dma_fence_enable_signaling is checking the status of signaled > bit again. > > Signed-off-by: Arvind Yadav <Arvind.Yadav@amd.com> Reviewed-by: Christian König <christian.koenig@amd.com> > --- > drivers/dma-buf/dma-fence.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c > index 406b4e26f538..11ef20f70ee0 100644 > --- a/drivers/dma-buf/dma-fence.c > +++ b/drivers/dma-buf/dma-fence.c > @@ -648,11 +648,6 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb, > if (WARN_ON(!fence || !func)) > return -EINVAL; > > - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) { > - INIT_LIST_HEAD(&cb->node); > - return -ENOENT; > - } > - > spin_lock_irqsave(fence->lock, flags); > > if (__dma_fence_enable_signaling(fence)) {
Am 27.09.22 um 19:24 schrieb Arvind Yadav: > Fence signaling must be enabled to make sure that > the dma_fence_is_signaled_locked() function ever returns true. > Since drivers and implementations sometimes mess this up, > this ensures correct behaviour when DMABUF_DEBUG_ENABLE_SIGNALING > is used during debugging. > This should make any implementation bugs resulting in not > signaled fences much more obvious. Are all IGT tests now passing with this? That would be a bit unfortunate because it means we still have missed the bug in drm_syncobj. Christian. > > Signed-off-by: Arvind Yadav <Arvind.Yadav@amd.com> > --- > 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..5156dc6be0a6 100644 > --- a/include/linux/dma-fence.h > +++ b/include/linux/dma-fence.h > @@ -398,6 +398,11 @@ void dma_fence_enable_sw_signaling(struct dma_fence *fence); > static inline bool > dma_fence_is_signaled_locked(struct dma_fence *fence) > { > +#ifdef CONFIG_DMABUF_DEBUG_ENABLE_SIGNALING > + 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; >
On 9/29/2022 11:48 PM, Christian König wrote: > Am 27.09.22 um 19:24 schrieb Arvind Yadav: >> Fence signaling must be enabled to make sure that >> the dma_fence_is_signaled_locked() function ever returns true. >> Since drivers and implementations sometimes mess this up, >> this ensures correct behaviour when DMABUF_DEBUG_ENABLE_SIGNALING >> is used during debugging. >> This should make any implementation bugs resulting in not >> signaled fences much more obvious. > > Are all IGT tests now passing with this? That would be a bit > unfortunate because it means we still have missed the bug in drm_syncobj. > IGT has these test cases related to syncobj (syncobj_basic, syncobj_timeline and syncobj_wait)and all are passing. I will check syncobj and let you know. ~Arvind > Christian. > >> >> Signed-off-by: Arvind Yadav <Arvind.Yadav@amd.com> >> --- >> 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..5156dc6be0a6 100644 >> --- a/include/linux/dma-fence.h >> +++ b/include/linux/dma-fence.h >> @@ -398,6 +398,11 @@ void dma_fence_enable_sw_signaling(struct >> dma_fence *fence); >> static inline bool >> dma_fence_is_signaled_locked(struct dma_fence *fence) >> { >> +#ifdef CONFIG_DMABUF_DEBUG_ENABLE_SIGNALING >> + 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; >
Am 29.09.22 um 20:30 schrieb Yadav, Arvind: > > On 9/29/2022 11:48 PM, Christian König wrote: >> Am 27.09.22 um 19:24 schrieb Arvind Yadav: >>> Fence signaling must be enabled to make sure that >>> the dma_fence_is_signaled_locked() function ever returns true. >>> Since drivers and implementations sometimes mess this up, >>> this ensures correct behaviour when DMABUF_DEBUG_ENABLE_SIGNALING >>> is used during debugging. >>> This should make any implementation bugs resulting in not >>> signaled fences much more obvious. >> >> Are all IGT tests now passing with this? That would be a bit >> unfortunate because it means we still have missed the bug in >> drm_syncobj. >> > IGT has these test cases related to syncobj (syncobj_basic, > syncobj_timeline and syncobj_wait)and all are passing. > > I will check syncobj and let you know. Maybe CC the Intel list and let their CI systems take a look. That's usually rather valuable. Thanks, Christian. > > ~Arvind > >> Christian. >> >>> >>> Signed-off-by: Arvind Yadav <Arvind.Yadav@amd.com> >>> --- >>> 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..5156dc6be0a6 100644 >>> --- a/include/linux/dma-fence.h >>> +++ b/include/linux/dma-fence.h >>> @@ -398,6 +398,11 @@ void dma_fence_enable_sw_signaling(struct >>> dma_fence *fence); >>> static inline bool >>> dma_fence_is_signaled_locked(struct dma_fence *fence) >>> { >>> +#ifdef CONFIG_DMABUF_DEBUG_ENABLE_SIGNALING >>> + 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; >>
On 9/30/2022 12:02 AM, Christian König wrote: > Am 29.09.22 um 20:30 schrieb Yadav, Arvind: >> >> On 9/29/2022 11:48 PM, Christian König wrote: >>> Am 27.09.22 um 19:24 schrieb Arvind Yadav: >>>> Fence signaling must be enabled to make sure that >>>> the dma_fence_is_signaled_locked() function ever returns true. >>>> Since drivers and implementations sometimes mess this up, >>>> this ensures correct behaviour when DMABUF_DEBUG_ENABLE_SIGNALING >>>> is used during debugging. >>>> This should make any implementation bugs resulting in not >>>> signaled fences much more obvious. >>> >>> Are all IGT tests now passing with this? That would be a bit >>> unfortunate because it means we still have missed the bug in >>> drm_syncobj. >>> >> IGT has these test cases related to syncobj (syncobj_basic, >> syncobj_timeline and syncobj_wait)and all are passing. >> >> I will check syncobj and let you know. > > Maybe CC the Intel list and let their CI systems take a look. That's > usually rather valuable. There is one IGT subtest is failing which is related to syncobj. I will fix this and submit the patch. igt_subtest("host-signal-points") test_host_signal_points(fd); > > Thanks, > Christian. > >> >> ~Arvind >> >>> Christian. >>> >>>> >>>> Signed-off-by: Arvind Yadav <Arvind.Yadav@amd.com> >>>> --- >>>> 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..5156dc6be0a6 100644 >>>> --- a/include/linux/dma-fence.h >>>> +++ b/include/linux/dma-fence.h >>>> @@ -398,6 +398,11 @@ void dma_fence_enable_sw_signaling(struct >>>> dma_fence *fence); >>>> static inline bool >>>> dma_fence_is_signaled_locked(struct dma_fence *fence) >>>> { >>>> +#ifdef CONFIG_DMABUF_DEBUG_ENABLE_SIGNALING >>>> + 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; >>> >