Re: [PATCH v2 4/4] dma-buf: Check status of enable-signaling bit on debug

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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.Yadav@xxxxxxx>

With the improved commit message this patch is Reviewed-by: Christian König <christian.koenig@xxxxxxx>

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;





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux