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:

> 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>
> 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?

>  			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(&reg->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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx