>-----Original Message----- >From: Chris Wilson [mailto:chris@xxxxxxxxxxxxxxxxxx] >Sent: Thursday, October 3, 2019 10:19 AM >To: Ruhl, Michael J <michael.j.ruhl@xxxxxxxxx>; intel- >gfx@xxxxxxxxxxxxxxxxxxxxx >Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx >Subject: RE: [Intel-gfx] [PATCH v3] dma-fence: Serialise signal enabling >(dma_fence_enable_sw_signaling) > >Quoting Ruhl, Michael J (2019-10-03 15:12:38) >> >-----Original Message----- >> >From: Intel-gfx [mailto:intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf >Of >> >Chris Wilson >> >Sent: Thursday, October 3, 2019 9:24 AM >> >To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx >> >Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx >> >Subject: [Intel-gfx] [PATCH v3] dma-fence: Serialise signal enabling >> >(dma_fence_enable_sw_signaling) >> > >> >Make dma_fence_enable_sw_signaling() behave like its >> >dma_fence_add_callback() and dma_fence_default_wait() counterparts >and >> >perform the test to enable signaling under the fence->lock, along with >> >the action to do so. This ensure that should an implementation be trying >> >to flush the cb_list (by signaling) on retirement before freeing the >> >fence, it can do so in a race-free manner. >> > >> >See also 0fc89b6802ba ("dma-fence: Simply wrap >dma_fence_signal_locked >> >with dma_fence_signal"). >> > >> >v2: Refactor all 3 enable_signaling paths to use a common function. >> > >> >Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> >> >--- >> >Return false for "could not _enable_ signaling as it was already >> >signaled" >> >--- >> > drivers/dma-buf/dma-fence.c | 78 +++++++++++++++++-------------------- >> > 1 file changed, 35 insertions(+), 43 deletions(-) >> > >> >diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c >> >index 2c136aee3e79..b58528c1cc9d 100644 >> >--- a/drivers/dma-buf/dma-fence.c >> >+++ b/drivers/dma-buf/dma-fence.c >> >@@ -273,6 +273,30 @@ void dma_fence_free(struct dma_fence *fence) >> > } >> > EXPORT_SYMBOL(dma_fence_free); >> > >> >+static bool __dma_fence_enable_signaling(struct dma_fence *fence) >> >+{ >> >+ bool was_set; >> >+ >> >+ lockdep_assert_held(fence->lock); >> >> With this held... >> >> >+ was_set = >> >test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, >> >+ &fence->flags); >> >+ >> >+ if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) >> >+ return false; >> >> Would making these the non-atomic versions be useful (and/or >reasonable)? > >That depends on all modifications to the dword (not just the bit) being >serialised by the same lock (or otherwise guaranteed to be serial and >coherent), which as Tvrtko rediscovered is not the case. dma_fence.flags >is also home to a number of user flags. Ah got it. In the future, I will think a bit outside the patch. (sorry for the pun...) Thanks, Mike >-Chris _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
