Chris Wilson <ch...@chris-wilson.co.uk> writes:
> Quoting Mika Kuoppala (2019-02-07 13:22:45)
>> Chris Wilson <ch...@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.kuopp...@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 <ch...@chris-wilson.co.uk>
>> > Cc: Mika Kuoppala <mika.kuopp...@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.kuopp...@linux.intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
