[01/20] drm/i915: Skip barriers inside waits
diff mbox series

Message ID 20200227085723.1961649-1-chris@chris-wilson.co.uk
State New
Headers show
Series
  • [01/20] drm/i915: Skip barriers inside waits
Related show

Commit Message

Chris Wilson Feb. 27, 2020, 8:57 a.m. UTC
Attaching to the i915_active barrier is a two stage process, and a flush
is only effective when the barrier is activation. Thus it is possible
for us to see a barrier, and attempt to flush, only for our flush to
have no effect. As such, before attempting to activate signaling on the
fence we need to double check it is a fence!

Fixes: d13a31770077 ("drm/i915: Flush idle barriers when waiting")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_active.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Andi Shyti Feb. 27, 2020, 10:38 p.m. UTC | #1
Hi Cris,

> Attaching to the i915_active barrier is a two stage process, and a flush
> is only effective when the barrier is activation. Thus it is possible
> for us to see a barrier, and attempt to flush, only for our flush to
> have no effect. As such, before attempting to activate signaling on the
> fence we need to double check it is a fence!
> 
> Fixes: d13a31770077 ("drm/i915: Flush idle barriers when waiting")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Reviewed-by: Andi Shyti <andi.shyti@intel.com>

Andi
Tvrtko Ursulin Feb. 28, 2020, 11:53 a.m. UTC | #2
On 27/02/2020 08:57, Chris Wilson wrote:
> Attaching to the i915_active barrier is a two stage process, and a flush
> is only effective when the barrier is activation. Thus it is possible
> for us to see a barrier, and attempt to flush, only for our flush to
> have no effect. As such, before attempting to activate signaling on the
> fence we need to double check it is a fence!
> 
> Fixes: d13a31770077 ("drm/i915: Flush idle barriers when waiting")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_active.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
> index 0b12d5023800..7b3d6c12ad61 100644
> --- a/drivers/gpu/drm/i915/i915_active.c
> +++ b/drivers/gpu/drm/i915/i915_active.c
> @@ -453,6 +453,9 @@ static void enable_signaling(struct i915_active_fence *active)
>   {
>   	struct dma_fence *fence;
>   
> +	if (unlikely(is_barrier(active)))
> +		return;
> +
>   	fence = i915_active_fence_get(active);
>   	if (!fence)
>   		return;
> 

So that smp_rmb() is not really effective, I mean the race is wider than 
that. I was worried about that.. now I need to figure out where it 
starts and where it ends (the race).

Regards,

Tvrtko
Chris Wilson Feb. 28, 2020, 12:08 p.m. UTC | #3
Quoting Tvrtko Ursulin (2020-02-28 11:53:19)
> 
> On 27/02/2020 08:57, Chris Wilson wrote:
> > Attaching to the i915_active barrier is a two stage process, and a flush
> > is only effective when the barrier is activation. Thus it is possible
> > for us to see a barrier, and attempt to flush, only for our flush to
> > have no effect. As such, before attempting to activate signaling on the
> > fence we need to double check it is a fence!
> > 
> > Fixes: d13a31770077 ("drm/i915: Flush idle barriers when waiting")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_active.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
> > index 0b12d5023800..7b3d6c12ad61 100644
> > --- a/drivers/gpu/drm/i915/i915_active.c
> > +++ b/drivers/gpu/drm/i915/i915_active.c
> > @@ -453,6 +453,9 @@ static void enable_signaling(struct i915_active_fence *active)
> >   {
> >       struct dma_fence *fence;
> >   
> > +     if (unlikely(is_barrier(active)))
> > +             return;
> > +
> >       fence = i915_active_fence_get(active);
> >       if (!fence)
> >               return;
> > 
> 
> So that smp_rmb() is not really effective, I mean the race is wider than 
> that. I was worried about that.. now I need to figure out where it 
> starts and where it ends (the race).

That smp_rmb matches the update when we actually replace the barrier
with the fence. But, yes, the reuse_idle_barrier() has interesting
implications. There's no use after free and the iterator is technically
safe, but the flush isn't quite the flush I had hoped for.
-Chris

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
index 0b12d5023800..7b3d6c12ad61 100644
--- a/drivers/gpu/drm/i915/i915_active.c
+++ b/drivers/gpu/drm/i915/i915_active.c
@@ -453,6 +453,9 @@  static void enable_signaling(struct i915_active_fence *active)
 {
 	struct dma_fence *fence;
 
+	if (unlikely(is_barrier(active)))
+		return;
+
 	fence = i915_active_fence_get(active);
 	if (!fence)
 		return;