>-----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)? Mike >+ >+ if (!was_set && fence->ops->enable_signaling) { >+ if (!fence->ops->enable_signaling(fence)) { >+ dma_fence_signal_locked(fence); >+ return false; >+ } >+ >+ trace_dma_fence_enable_signal(fence); >+ } >+ >+ return true; >+} >+ > /** > * dma_fence_enable_sw_signaling - enable signaling on fence > * @fence: the fence to enable >@@ -285,19 +309,12 @@ void dma_fence_enable_sw_signaling(struct >dma_fence *fence) > { > unsigned long flags; > >- if (!test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, >- &fence->flags) && >- !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) && >- fence->ops->enable_signaling) { >- trace_dma_fence_enable_signal(fence); >- >- spin_lock_irqsave(fence->lock, flags); >- >- if (!fence->ops->enable_signaling(fence)) >- dma_fence_signal_locked(fence); >+ if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) >+ return; > >- spin_unlock_irqrestore(fence->lock, flags); >- } >+ spin_lock_irqsave(fence->lock, flags); >+ __dma_fence_enable_signaling(fence); >+ spin_unlock_irqrestore(fence->lock, flags); > } > EXPORT_SYMBOL(dma_fence_enable_sw_signaling); > >@@ -331,7 +348,6 @@ int dma_fence_add_callback(struct dma_fence >*fence, struct dma_fence_cb *cb, > { > unsigned long flags; > int ret = 0; >- bool was_set; > > if (WARN_ON(!fence || !func)) > return -EINVAL; >@@ -343,25 +359,14 @@ int dma_fence_add_callback(struct dma_fence >*fence, struct dma_fence_cb *cb, > > spin_lock_irqsave(fence->lock, flags); > >- was_set = >test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, >- &fence->flags); >- >- if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) >- ret = -ENOENT; >- else if (!was_set && fence->ops->enable_signaling) { >- trace_dma_fence_enable_signal(fence); >- >- if (!fence->ops->enable_signaling(fence)) { >- dma_fence_signal_locked(fence); >- ret = -ENOENT; >- } >- } >- >- if (!ret) { >+ if (__dma_fence_enable_signaling(fence)) { > cb->func = func; > list_add_tail(&cb->node, &fence->cb_list); >- } else >+ } else { > INIT_LIST_HEAD(&cb->node); >+ ret = -ENOENT; >+ } >+ > spin_unlock_irqrestore(fence->lock, flags); > > return ret; >@@ -461,7 +466,6 @@ dma_fence_default_wait(struct dma_fence *fence, >bool intr, signed long timeout) > struct default_wait_cb cb; > unsigned long flags; > signed long ret = timeout ? timeout : 1; >- bool was_set; > > if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) > return ret; >@@ -473,21 +477,9 @@ dma_fence_default_wait(struct dma_fence *fence, >bool intr, signed long timeout) > goto out; > } > >- was_set = >test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, >- &fence->flags); >- >- if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) >+ if (!__dma_fence_enable_signaling(fence)) > goto out; > >- if (!was_set && fence->ops->enable_signaling) { >- trace_dma_fence_enable_signal(fence); >- >- if (!fence->ops->enable_signaling(fence)) { >- dma_fence_signal_locked(fence); >- goto out; >- } >- } >- > if (!timeout) { > ret = 0; > goto out; >-- >2.23.0 > >_______________________________________________ >Intel-gfx mailing list >Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
