Re: [PATCH 2/8] drm/i915: Defer removing fence register tracking to rpm wakeup

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2019-02-07 13:22:45)
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> 
>> > Currently, we may simultaneously release the fence register from both
>> > fence_update() and i915_gem_restore_fences(). This is dangerous, so
>> > defer the bookkeeping entirely to i915_gem_restore_fences() when the
>> > device is asleep.
>> >
>> > Reported-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

A tad overstatement but fine. I feel like I was wandering
in these hoods, being lost and confused and bumping into fences.

>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_gem_fence_reg.c | 62 ++++++++++++-----------
>> >  1 file changed, 32 insertions(+), 30 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
>> > index e037e94792f3..be89bd95ab7c 100644
>> > --- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c
>> > +++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
>> > @@ -210,6 +210,7 @@ static int fence_update(struct drm_i915_fence_reg *fence,
>> >                       struct i915_vma *vma)
>> >  {
>> >       intel_wakeref_t wakeref;
>> > +     struct i915_vma *old;
>> >       int ret;
>> >  
>> >       if (vma) {
>> > @@ -229,49 +230,55 @@ static int fence_update(struct drm_i915_fence_reg *fence,
>> >                       return ret;
>> >       }
>> >  
>> > -     if (fence->vma) {
>> > -             struct i915_vma *old = fence->vma;
>> > -
>> > +     old = xchg(&fence->vma, NULL);
>> 
>> So this is for restore seeing fence consistently.
>> 
>> > +     if (old) {
>> >               ret = i915_active_request_retire(&old->last_fence,
>> >                                            &old->obj->base.dev->struct_mutex);
>> > -             if (ret)
>> > +             if (ret) {
>> > +                     fence->vma = old;
>> 
>> And this then won't matter as the restore fences had zeroed
>> the fence reg before error. We get back to exact state
>> later but when?
>
> This operation is under the mutex guarding the fences, and the previous
> fence was unpinned so not in used. Prior to being used, all is
> consistent.

Ah didn't get the unpinning part.

>
>> >                       return ret;
>> > +             }
>> >  
>> >               i915_vma_flush_writes(old);
>> > -     }
>> >  
>> > -     if (fence->vma && fence->vma != vma) {
>> > -             /* Ensure that all userspace CPU access is completed before
>> > +             /*
>> > +              * Ensure that all userspace CPU access is completed before
>> >                * stealing the fence.
>> >                */
>> > -             GEM_BUG_ON(fence->vma->fence != fence);
>> > -             i915_vma_revoke_mmap(fence->vma);
>> > -
>> > -             fence->vma->fence = NULL;
>> > -             fence->vma = NULL;
>> > +             if (old != vma) {
>> > +                     GEM_BUG_ON(old->fence != fence);
>> > +                     i915_vma_revoke_mmap(old);
>> > +                     old->fence = NULL;
>> > +             }
>> >  
>> >               list_move(&fence->link, &fence->i915->mm.fence_list);
>> >       }
>> >  
>> > -     /* We only need to update the register itself if the device is awake.
>> > +     /*
>> > +      * We only need to update the register itself if the device is awake.
>> >        * If the device is currently powered down, we will defer the write
>> >        * to the runtime resume, see i915_gem_restore_fences().
>> > +      *
>> > +      * This only works for removing the fence register, on acquisition
>> > +      * the caller must hold the rpm wakeref. The fence register must
>> > +      * be cleared before we can use any other fences to ensure that
>> > +      * the new fences do not overlap the elided clears, confusing HW.
>> >        */
>> >       wakeref = intel_runtime_pm_get_if_in_use(fence->i915);
>> > -     if (wakeref) {
>> > -             fence_write(fence, vma);
>> > -             intel_runtime_pm_put(fence->i915, wakeref);
>> > +     if (!wakeref) {
>> > +             GEM_BUG_ON(vma);
>> > +             return 0;
>> >       }
>> >  
>> > -     if (vma) {
>> > -             if (fence->vma != vma) {
>> > -                     vma->fence = fence;
>> > -                     fence->vma = vma;
>> > -             }
>> > +     fence_write(fence, vma);
>> > +     fence->vma = vma;
>> >  
>> > +     if (vma) {
>> > +             vma->fence = fence;
>> >               list_move_tail(&fence->link, &fence->i915->mm.fence_list);
>> >       }
>> >  
>> > +     intel_runtime_pm_put(fence->i915, wakeref);
>> >       return 0;
>> >  }
>> >  
>> > @@ -473,9 +480,10 @@ void i915_gem_restore_fences(struct drm_i915_private *dev_priv)
>> >  {
>> >       int i;
>> >  
>> > +     rcu_read_lock(); /* keep obj alive as we dereference */
>> >       for (i = 0; i < dev_priv->num_fence_regs; i++) {
>> >               struct drm_i915_fence_reg *reg = &dev_priv->fence_regs[i];
>> 
>> I do have spent some amount of time to try to figure out if
>> there is a reasoning of sometimes calling the fence reg as 'fence'
>> and in other cases 'reg'.
>> 
>> If there is a reason, help me out. If there is not, I
>> politely ask to follow the same naming than in revoke_fences.
>
> The hw is known as fences, but so are other things. reg is too general,
> and the use here is inconsistent with every other use of reg. In short,
> it really doesn't matter...
>
>> Or that we go for 'fence_reg' always when talking about
>> preallocated reg slots.
>
> Except now you are pulling my leg.

Nope!

>
>> > -             struct i915_vma *vma = reg->vma;
>> > +             struct i915_vma *vma = READ_ONCE(reg->vma);
>> >  
>> >               GEM_BUG_ON(vma && vma->fence != reg);
>> >  
>> > @@ -483,18 +491,12 @@ void i915_gem_restore_fences(struct drm_i915_private *dev_priv)
>> >                * Commit delayed tiling changes if we have an object still
>> >                * attached to the fence, otherwise just clear the fence.
>> >                */
>> > -             if (vma && !i915_gem_object_is_tiled(vma->obj)) {
>> > -                     GEM_BUG_ON(!reg->dirty);
>> 
>> You omit the dirty check here. If the reasoning is
>> that we can't sample due to inconstency wrt rest of fence reg,
>> does it then lead to need to make a __fence_write()
>> that does not write the dirty flag.
>
> Because it doesn't matter, we are just flushing the register back to the
> known state.

Ah I see, well perhaps there is then possiblity to throw out
the dirty trickery out from suspend/restore side in future.

The issue I was concerned is addressed so,
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx