| Message ID | 20241108094256.3717-2-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | dma-buf: fix dma_fence_array_signaled | expand |
On 08/11/2024 09:42, Christian König wrote: > The function silently assumed that signaling was already enabled for the > dma_fence_array. This meant that without enabling signaling first we would > never see forward progress. > > Fix that by falling back to testing each individual fence when signaling > isn't enabled yet. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/dma-buf/dma-fence-array.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c > index 46ac42bcfac0..01203796827a 100644 > --- a/drivers/dma-buf/dma-fence-array.c > +++ b/drivers/dma-buf/dma-fence-array.c > @@ -103,10 +103,22 @@ static bool dma_fence_array_enable_signaling(struct dma_fence *fence) > static bool dma_fence_array_signaled(struct dma_fence *fence) > { > struct dma_fence_array *array = to_dma_fence_array(fence); > + unsigned int i, num_pending; > > - if (atomic_read(&array->num_pending) > 0) > + num_pending = atomic_read(&array->num_pending); > + if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &array->base.flags)) { > + if (!num_pending) > + goto signal; > return false; > + } > + > + for (i = 0; i < array->num_fences; ++i) { > + if (dma_fence_is_signaled(array->fences[i]) && !--num_pending) > + goto signal; > + } > + return false; Sampling num_pending (and decrementing) and test_bit from an unlocked path makes one need to think if there are consequences, false negatives, positives or something. Would it be fine to simplify like the below? static bool dma_fence_array_signaled(struct dma_fence *fence) { struct dma_fence_array *array = to_dma_fence_array(fence); unsigned int i; if (atomic_read(&array->num_pending)) { for (i = 0; i < array->num_fences; i++) { if (!dma_fence_is_signaled(array->fences[i])) return false; } } dma_fence_array_clear_pending_error(array); return true; } Or if the optimisation to not walk the array when signalling is already enabled is deemed important, perhaps a less thinking inducing way would be this: static bool dma_fence_array_signaled(struct dma_fence *fence) { struct dma_fence_array *array = to_dma_fence_array(fence); unsigned int i; if (atomic_read(&array->num_pending) == 0) goto signalled; if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &array->base.flags)) return false; for (i = 0; i < array->num_fences; i++) { if (!dma_fence_is_signaled(array->fences[i])) return false; } signalled: dma_fence_array_clear_pending_error(array); return true; } Decrementing locally cached num_pending in the loop I think does not bring anything since when signalling is not enabled it will be stuck at num_fences. So the loop walks the whole array versus bail on first unsignalled, so latter even more efficient. In which case, should dma-fence-chain also be aligned to have the fast path bail out? Regards, Tvrtko > > +signal: > dma_fence_array_clear_pending_error(array); > return true; > }
diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c index 46ac42bcfac0..01203796827a 100644 --- a/drivers/dma-buf/dma-fence-array.c +++ b/drivers/dma-buf/dma-fence-array.c @@ -103,10 +103,22 @@ static bool dma_fence_array_enable_signaling(struct dma_fence *fence) static bool dma_fence_array_signaled(struct dma_fence *fence) { struct dma_fence_array *array = to_dma_fence_array(fence); + unsigned int i, num_pending; - if (atomic_read(&array->num_pending) > 0) + num_pending = atomic_read(&array->num_pending); + if (test_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &array->base.flags)) { + if (!num_pending) + goto signal; return false; + } + + for (i = 0; i < array->num_fences; ++i) { + if (dma_fence_is_signaled(array->fences[i]) && !--num_pending) + goto signal; + } + return false; +signal: dma_fence_array_clear_pending_error(array); return true; }
The function silently assumed that signaling was already enabled for the dma_fence_array. This meant that without enabling signaling first we would never see forward progress. Fix that by falling back to testing each individual fence when signaling isn't enabled yet. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/dma-fence-array.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)