drm/i915: Avoid GGTT flushing on non-GGTT paths of i915_vma_pin_iomap

Commit Message

Tvrtko Ursulin July 24, 2023, 12:56 p.m. UTC

From: Tvrtko Ursulin <[e-mail]>

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 <[e-mail]>
Fixes: 4bc91dbde0da ("drm/i915/lmem: Bypass aperture when lmem is available")
References: d976521a995a ("drm/i915: extend i915_vma_pin_iomap()")
Cc: Radhakrishna Sripada <[e-mail]>
Cc: <[e-mail]> # v5.14+
---
 drivers/gpu/drm/i915/i915_vma.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Andi Shyti July 24, 2023, 8:16 p.m. UTC | #1

Hi Tvrtko,

On Mon, Jul 24, 2023 at 01:56:33PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <[e-mail]>
> 
> 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 <[e-mail]>
> Fixes: 4bc91dbde0da ("drm/i915/lmem: Bypass aperture when lmem is available")
> References: d976521a995a ("drm/i915: extend i915_vma_pin_iomap()")
> Cc: Radhakrishna Sripada <[e-mail]>
> Cc: <[e-mail]> # 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).

Anyway, it looks good:

Reviewed-by: Andi Shyti <[e-mail]> 

Andi

>  }
>  
>  void i915_vma_unpin_iomap(struct i915_vma *vma)
> -- 
> 2.39.2

Sripada, Radhakrishna July 24, 2023, 11:38 p.m. UTC | #2

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.

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?

--Radhakrishna(RK) Sripada 

> -----Original Message-----
> From: Tvrtko Ursulin <[e-mail]>
> Sent: Monday, July 24, 2023 5:57 AM
> To: [e-mail]; [e-mail]
> Cc: Ursulin, Tvrtko <[e-mail]>; Sripada, Radhakrishna
> <[e-mail]>; [e-mail]
> Subject: [PATCH] drm/i915: Avoid GGTT flushing on non-GGTT paths of
> i915_vma_pin_iomap
> 
> From: Tvrtko Ursulin <[e-mail]>
> 
> 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 <[e-mail]>
> Fixes: 4bc91dbde0da ("drm/i915/lmem: Bypass aperture when lmem is
> available")
> References: d976521a995a ("drm/i915: extend i915_vma_pin_iomap()")
> Cc: Radhakrishna Sripada <[e-mail]>
> Cc: <[e-mail]> # 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

Tvrtko Ursulin July 25, 2023, 11:07 a.m. UTC | #3

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@[e-mail]

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@[e-mail]

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 <[e-mail]>
>> Sent: Monday, July 24, 2023 5:57 AM
>> To: [e-mail]; [e-mail]
>> Cc: Ursulin, Tvrtko <[e-mail]>; Sripada, Radhakrishna
>> <[e-mail]>; [e-mail]
>> Subject: [PATCH] drm/i915: Avoid GGTT flushing on non-GGTT paths of
>> i915_vma_pin_iomap
>>
>> From: Tvrtko Ursulin <[e-mail]>
>>
>> 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 <[e-mail]>
>> Fixes: 4bc91dbde0da ("drm/i915/lmem: Bypass aperture when lmem is
>> available")
>> References: d976521a995a ("drm/i915: extend i915_vma_pin_iomap()")
>> Cc: Radhakrishna Sripada <[e-mail]>
>> Cc: <[e-mail]> # 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
>

Tvrtko Ursulin July 25, 2023, 11:52 a.m. UTC | #4

On 24/07/2023 21:16, Andi Shyti wrote:
> Hi Tvrtko,
> 
> On Mon, Jul 24, 2023 at 01:56:33PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <[e-mail]>
>>
>> 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 <[e-mail]>
>> Fixes: 4bc91dbde0da ("drm/i915/lmem: Bypass aperture when lmem is available")
>> References: d976521a995a ("drm/i915: extend i915_vma_pin_iomap()")
>> Cc: Radhakrishna Sripada <[e-mail]>
>> Cc: <[e-mail]> # 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 <[e-mail]>

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.

Regards,

Tvrtko

> 
> Andi
> 
>>   }
>>   
>>   void i915_vma_unpin_iomap(struct i915_vma *vma)
>> -- 
>> 2.39.2

Andi Shyti July 25, 2023, 12:45 p.m. UTC | #5

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 <[e-mail]>
> > > Fixes: 4bc91dbde0da ("drm/i915/lmem: Bypass aperture when lmem is available")
> > > References: d976521a995a ("drm/i915: extend i915_vma_pin_iomap()")
> > > Cc: Radhakrishna Sripada <[e-mail]>
> > > Cc: <[e-mail]> # 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 <[e-mail]>
> 
> 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

Patch

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)