Quoting Mika Kuoppala (2019-02-07 13:22:45)
> Chris Wilson <[email protected]> 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 <[email protected]>
> > Signed-off-by: Chris Wilson <[email protected]>
> > Cc: Mika Kuoppala <[email protected]>
> > ---
> > 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.
> > 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.
> > - 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.
-Chris
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
