Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
> ---
> 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?
> 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.
Or that we go for 'fence_reg' always when talking about
preallocated reg slots.
> - 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.
For making sure that for next pin won't drop the write?
> - GEM_BUG_ON(i915_vma_has_userfault(vma));
> -
> - list_move(®->link, &dev_priv->mm.fence_list);
This makes life easier.
> - vma->fence = NULL;
> + if (vma && !i915_gem_object_is_tiled(vma->obj))
> vma = NULL;
> - }
>
> fence_write(reg, vma);
> - reg->vma = vma;
> }
> + rcu_read_unlock();
> }
>
> /**
> --
> 2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
