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@xxxxxxxxxxxxxxx>
> Sent: Monday, July 24, 2023 5:57 AM
> To: Intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Cc: Ursulin, Tvrtko <tvrtko.ursulin@xxxxxxxxx>; Sripada, Radhakrishna
> <radhakrishna.sripada@xxxxxxxxx>; stable@xxxxxxxxxxxxxxx
> Subject: [PATCH] drm/i915: Avoid GGTT flushing on non-GGTT paths of
> i915_vma_pin_iomap
>
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> 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@xxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx> # 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
