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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@gem_ppgtt@blt-vs-render-ctxn.html

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.html

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
>