On 25/07/2023 00:38, Sripada, Radhakrishna wrote:
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.
This failure: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_121236v1/shard-snb7/igt@g...
Test or machine is not entirely stable looking at it's history, but with a couple different failure signatures:
https://intel-gfx-ci.01.org/tree/drm-tip/igt@gem_ppgtt@blt-vs-render-ctxn.ht...
But agreed that we need to be careful. I requested a re-run for a start.
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?
You mean:
intel_gt_flush_ggtt_writes() { ... wmb(); ... intel_gt_chipset_flush(); wmb();
?
I'd guess it is not needed twice on the intel_gt_flush_ggtt_writes() path, but happens to be like that for direct callers of intel_gt_chipset_flush().
Maybe there is scope to tidy this all, for instance the first direct caller I opened does this:
rpcs_query_batch() { ... __i915_gem_object_flush_map(rpcs, 0, 64); i915_gem_object_unpin_map(rpcs);
intel_gt_chipset_flush(vma->vm->gt);
Where I think __i915_gem_object_flush_map() could actually do the right thing and issue a flush appropriate for the mapping that was used. But it is work and double flush does not really harm. I don't think it does at least.
Regards,
Tvrtko
--Radhakrishna(RK) Sripada
-----Original Message----- From: Tvrtko Ursulin tvrtko.ursulin@linux.intel.com Sent: Monday, July 24, 2023 5:57 AM To: Intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org Cc: Ursulin, Tvrtko tvrtko.ursulin@intel.com; Sripada, Radhakrishna radhakrishna.sripada@intel.com; stable@vger.kernel.org Subject: [PATCH] drm/i915: Avoid GGTT flushing on non-GGTT paths of i915_vma_pin_iomap
From: Tvrtko Ursulin tvrtko.ursulin@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@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. */}
void i915_vma_unpin_iomap(struct i915_vma *vma)
-- 2.39.2