Re: [PATCH 04/17] drm/i915/gt: Make intel_ring_unpin() safe for concurrent pint

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

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

concurrent pint? Sure, thirsty already.

But I am afraid you just mean concurrent pin :O

> In order to avoid some nasty mutex inversions, commit 09c5ab384f6f
> ("drm/i915: Keep rings pinned while the context is active") allowed the
> intel_ring unpinning to be run concurrently with the next context
> pinning it. Thus each step in intel_ring_unpin() needed to be atomic and
> ordered in a nice onion with intel_ring_pin() so that the lifetimes
> overlapped and were always safe.
>
> Sadly, a few steps in intel_ring_unpin() were overlooked, such as
> closing the read/write pointers of the ring and discarding the
> intel_ring.vaddr, as these steps were not serialised with
> intel_ring_pin() and so could leave the ring in disarray.
>
> Fixes: 09c5ab384f6f ("drm/i915: Keep rings pinned while the context is active")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/gt/intel_ring.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_ring.c b/drivers/gpu/drm/i915/gt/intel_ring.c
> index ece20504d240..374b28f13ca0 100644
> --- a/drivers/gpu/drm/i915/gt/intel_ring.c
> +++ b/drivers/gpu/drm/i915/gt/intel_ring.c
> @@ -57,9 +57,10 @@ int intel_ring_pin(struct intel_ring *ring)
>  
>  	i915_vma_make_unshrinkable(vma);
>  
> -	GEM_BUG_ON(ring->vaddr);
> -	ring->vaddr = addr;
> +	/* Discard any unused bytes beyond that submitted to hw. */
> +	intel_ring_reset(ring, ring->emit);
>  
> +	ring->vaddr = addr;
>  	return 0;
>  
>  err_ring:
> @@ -85,20 +86,14 @@ void intel_ring_unpin(struct intel_ring *ring)
>  	if (!atomic_dec_and_test(&ring->pin_count))
>  		return;
>  
> -	/* Discard any unused bytes beyond that submitted to hw. */
> -	intel_ring_reset(ring, ring->emit);
> -
>  	i915_vma_unset_ggtt_write(vma);
>  	if (i915_vma_is_map_and_fenceable(vma))
>  		i915_vma_unpin_iomap(vma);
>  	else
>  		i915_gem_object_unpin_map(vma->obj);
>  
> -	GEM_BUG_ON(!ring->vaddr);
> -	ring->vaddr = NULL;
> -
> -	i915_vma_unpin(vma);
>  	i915_vma_make_purgeable(vma);
> +	i915_vma_unpin(vma);

Removing the BUG_ONs made me grepping.

One GEM_BUG_ON(&ring->pin_count) on a strategic location
would feel comforting.

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

>  }
>  
>  static struct i915_vma *create_ring_vma(struct i915_ggtt *ggtt, int size)
> -- 
> 2.24.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx