Hi Tvrtko,
> > > 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. */
> >
> > is flush the right word? Can you expand more the explanation in
> > this comment and why this point of synchronization is needed
> > here? (I am even wondering if it is really needed).
>
> If you are hinting flush isn't the right word then I am not remembering what
> else do we use for it?
>
> It is needed because i915_flush_writes()'s point AFAIU is to make sure CPU
> writes after i915_vma_pin_iomap() have landed in RAM. All three methods the
> latter can map the buffer are WC, therefore "flushing" of the WC buffer is
> needed for former to do something (what it promises).
>
> Currently the wmb() is in intel_gt_flush_ggtt_writes(). But only one of the
> three mapping paths is via GGTT. So my logic is that calling it for paths
> not interacting with GGTT is confusing and not needed.
>
> > Anyway, it looks good:
> >
> > Reviewed-by: Andi Shyti <andi.shyti@xxxxxxxxxxxxxxx>
>
> Thanks. If you don't see a hole in my logic I can improve the comment. I
> considered it initially but then thought it is obvious enough from looking
> at the i915_vma_pin_iomap. I can comment it more.
The logic looks linear... my questions were more aiming at
confirming my understanding and improving the comment around
wmb().
Thanks,
Andi
