[Intel-gfx] [PATCH] drm/i915: Avoid GGTT flushing on non-GGTT paths of i915_vma_pin_iomap

[Sripada, Radhakrishna]

Hi Tvrtko,

The changes makes sense and based on the description looks good.
I am bit skeptical about the exec buffer failure reported by ci hence,
withholding the r-b for now. If you believe the CI failure is unrelated
please feel free to add my r-b.

On a side note on platforms with non-coherent ggtt do we really
need to use the barriers twice under intel_gt_flush_ggtt_writes?

--Radhakrishna(RK) Sripada 

> -----Original Message-----
> From: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> Sent: Monday, July 24, 2023 5:57 AM
> To: Intel-gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org
> Cc: Ursulin, Tvrtko <tvrtko.ursulin at intel.com>; Sripada, Radhakrishna
> <radhakrishna.sripada at intel.com>; stable at vger.kernel.org
> Subject: [PATCH] drm/i915: Avoid GGTT flushing on non-GGTT paths of
> i915_vma_pin_iomap
> 
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> Commit 4bc91dbde0da ("drm/i915/lmem: Bypass aperture when lmem is
> available")
> added a code path which does not map via GGTT, but was still setting the
> ggtt write bit, and so triggering the GGTT flushing.
> 
> Fix it by not setting that bit unless the GGTT mapping path was used, and
> replace the flush with wmb() in i915_vma_flush_writes().
> 
> This also works for the i915_gem_object_pin_map path added in
> d976521a995a ("drm/i915: extend i915_vma_pin_iomap()").
> 
> It is hard to say if the fix has any observable effect, given that the
> write-combine buffer gets flushed from intel_gt_flush_ggtt_writes too, but
> apart from code clarity, skipping the needless GGTT flushing could be
> beneficial on platforms with non-coherent GGTT. (See the code flow in
> intel_gt_flush_ggtt_writes().)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Fixes: 4bc91dbde0da ("drm/i915/lmem: Bypass aperture when lmem is
> available")
> References: d976521a995a ("drm/i915: extend i915_vma_pin_iomap()")
> Cc: Radhakrishna Sripada <radhakrishna.sripada at intel.com>
> Cc: <stable at vger.kernel.org> # v5.14+
> ---
>  drivers/gpu/drm/i915/i915_vma.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_vma.c
> b/drivers/gpu/drm/i915/i915_vma.c
> index ffb425ba591c..f2b626cd2755 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -602,7 +602,9 @@ void __iomem *i915_vma_pin_iomap(struct i915_vma
> *vma)
>  	if (err)
>  		goto err_unpin;
> 
> -	i915_vma_set_ggtt_write(vma);
> +	if (!i915_gem_object_is_lmem(vma->obj) &&
> +	    i915_vma_is_map_and_fenceable(vma))
> +		i915_vma_set_ggtt_write(vma);
> 
>  	/* NB Access through the GTT requires the device to be awake. */
>  	return page_mask_bits(ptr);
> @@ -617,6 +619,8 @@ void i915_vma_flush_writes(struct i915_vma *vma)
>  {
>  	if (i915_vma_unset_ggtt_write(vma))
>  		intel_gt_flush_ggtt_writes(vma->vm->gt);
> +	else
> +		wmb(); /* Just flush the write-combine buffer. */
>  }
> 
>  void i915_vma_unpin_iomap(struct i915_vma *vma)
> --
> 2.39.2