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);@@ -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