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

[Andi Shyti <andi.shyti@linux.intel.com>]

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@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@intel.com>
> > > Cc: <stable@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. */
> > 
> > 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@linux.intel.com>
> 
> 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