Fence signaling must be enabled to make sure that the dma_fence_is_signaled() and dma_fence_is_signaled_locked() function ever returns true. Since drivers and implementations sometimes mess this up, this ensures correct behaviour when DEBUG_WW_MUTEX_SLOWPATH is used during debugging. This should make any implementation bugs resulting in not signaled fences much more obvious.
Arvind Yadav (3): [PATCH 1/3] dma-buf: Remove the signaled bit status check [PATCH 2/3] dma-buf: Enable signaling on fence for sw_sync [PATCH 3/3] dma-buf: Check status of enable-signaling bit on debug
drivers/dma-buf/dma-fence.c | 5 ----- drivers/dma-buf/sw_sync.c | 2 ++ include/linux/dma-fence.h | 5 +++++ 3 files changed, 7 insertions(+), 5 deletions(-)
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 --- 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:
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)) {
Here's enabling software signaling on fence for sw_sync.
Signed-off-by: Arvind Yadav Arvind.Yadav@amd.com --- drivers/dma-buf/sw_sync.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c index 348b3a9170fa..d2a52ceac14e 100644 --- a/drivers/dma-buf/sw_sync.c +++ b/drivers/dma-buf/sw_sync.c @@ -244,6 +244,8 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj, obj->context, value); INIT_LIST_HEAD(&pt->link);
+ dma_fence_enable_sw_signaling(&pt->base); + spin_lock_irq(&obj->lock); if (!dma_fence_is_signaled_locked(&pt->base)) { struct rb_node **p = &obj->pt_tree.rb_node;
Am 27.09.22 um 19:24 schrieb Arvind Yadav:
Here's enabling software signaling on fence for sw_sync.
Signed-off-by: Arvind Yadav Arvind.Yadav@amd.com
Reviewed-by: Christian König christian.koenig@amd.com
drivers/dma-buf/sw_sync.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c index 348b3a9170fa..d2a52ceac14e 100644 --- a/drivers/dma-buf/sw_sync.c +++ b/drivers/dma-buf/sw_sync.c @@ -244,6 +244,8 @@ static struct sync_pt *sync_pt_create(struct sync_timeline *obj, obj->context, value); INIT_LIST_HEAD(&pt->link);
- dma_fence_enable_sw_signaling(&pt->base);
- spin_lock_irq(&obj->lock); if (!dma_fence_is_signaled_locked(&pt->base)) { struct rb_node **p = &obj->pt_tree.rb_node;
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.
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 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;
linaro-mm-sig@lists.linaro.org
-
Arvind Yadav -
Christian König -
Yadav, Arvind