On 06/09/2022 12:21, Tvrtko Ursulin wrote:
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?
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
+
Or simpler OFC:
if (!test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags)) {
WARN_ON_ONCE(!fence->ops->enable_signaling);
return false;
}
You could also run this core change past i915 CI to see if it catches anything.
Just send to our trybot and see what happens? With the debug option enabled of
course. Hope it won't be painful.
Regards,
Tvrtko
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.
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.ya...@amd.com>
With the improved commit message this patch is Reviewed-by:
Christian König <christian.koe...@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;
