[PATCH] drm/etnaviv: Fix flush sequence logic

[PATCH] drm/etnaviv: Fix flush sequence logic

[Tomeu Vizoso]

We should be comparing the last submitted sequence number with that of
the address space we may be switching to.

And we should be using the latter as the last submitted sequence number
afterwards.

Signed-off-by: Tomeu Vizoso <tomeu@tomeuvizoso.net>
---
 drivers/gpu/drm/etnaviv/etnaviv_buffer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_buffer.c b/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
index b13a17276d07..865b07b14b38 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
@@ -347,7 +347,7 @@ void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, u32 exec_state,
 	u32 link_target, link_dwords;
 	bool switch_context = gpu->exec_state != exec_state;
 	bool switch_mmu_context = gpu->mmu_context != mmu_context;
-	unsigned int new_flush_seq = READ_ONCE(gpu->mmu_context->flush_seq);
+	unsigned int new_flush_seq = READ_ONCE(mmu_context->flush_seq);
 	bool need_flush = switch_mmu_context || gpu->flush_seq != new_flush_seq;
 	bool has_blt = !!(gpu->identity.minor_features5 &
 			  chipMinorFeatures5_BLT_ENGINE);
@@ -399,6 +399,7 @@ void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, u32 exec_state,
 			struct etnaviv_iommu_context *old_context = gpu->mmu_context;
 
 			gpu->mmu_context = etnaviv_iommu_context_get(mmu_context);
+			gpu->flush_seq = new_flush_seq;
 			etnaviv_iommu_context_put(old_context);
 		}
 
-- 
2.49.0

Re: [PATCH] drm/etnaviv: Fix flush sequence logic

[Fabio Estevam]

Hi Tomeu,

On Wed, May 7, 2025 at 8:22 AM Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
>
> We should be comparing the last submitted sequence number with that of
> the address space we may be switching to.
>
> And we should be using the latter as the last submitted sequence number
> afterwards.
>
> Signed-off-by: Tomeu Vizoso <tomeu@tomeuvizoso.net>

Does this need a Fixes tag?

Re: [PATCH] drm/etnaviv: Fix flush sequence logic

[Lucas Stach]

Am Mittwoch, dem 07.05.2025 um 13:21 +0200 schrieb Tomeu Vizoso:
> We should be comparing the last submitted sequence number with that of
> the address space we may be switching to.
> 
> And we should be using the latter as the last submitted sequence number
> afterwards.
> 
> Signed-off-by: Tomeu Vizoso <tomeu@tomeuvizoso.net>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_buffer.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_buffer.c b/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
> index b13a17276d07..865b07b14b38 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
> @@ -347,7 +347,7 @@ void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, u32 exec_state,
>  	u32 link_target, link_dwords;
>  	bool switch_context = gpu->exec_state != exec_state;
>  	bool switch_mmu_context = gpu->mmu_context != mmu_context;
> -	unsigned int new_flush_seq = READ_ONCE(gpu->mmu_context->flush_seq);
> +	unsigned int new_flush_seq = READ_ONCE(mmu_context->flush_seq);
>  	bool need_flush = switch_mmu_context || gpu->flush_seq != new_flush_seq;
>  	bool has_blt = !!(gpu->identity.minor_features5 &
>  			  chipMinorFeatures5_BLT_ENGINE);
> @@ -399,6 +399,7 @@ void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, u32 exec_state,
>  			struct etnaviv_iommu_context *old_context = gpu->mmu_context;
>  
>  			gpu->mmu_context = etnaviv_iommu_context_get(mmu_context);
> +			gpu->flush_seq = new_flush_seq;

As I said in IRC: this should not be necessary. If we switch to a new
MMU context, need_flush will also be set. The code under the need_flush
condition already includes this exact line.

Regards,
Lucas

Re: [PATCH] drm/etnaviv: Fix flush sequence logic

[Christian Gmeiner]

> Hi Tomeu,
>
> On Wed, May 7, 2025 at 8:22 AM Tomeu Vizoso <tomeu@tomeuvizoso.net> wrote:
> >
> > We should be comparing the last submitted sequence number with that of
> > the address space we may be switching to.
> >
> > And we should be using the latter as the last submitted sequence number
> > afterwards.
> >
> > Signed-off-by: Tomeu Vizoso <tomeu@tomeuvizoso.net>
>
> Does this need a Fixes tag?

Yes - based on the READ_ONCE(..) line, it should be
Fixes: 27b67278e007 ("drm/etnaviv: rework MMU handling")

-- 
greets
--
Christian Gmeiner, MSc

https://christian-gmeiner.info/privacypolicy

[PATCH v2] drm/etnaviv: Fix flush sequence logic

[Tomeu Vizoso]

We should be comparing the last submitted sequence number with that of
the address space we may be switching to.

Fixes: 27b67278e007 ("drm/etnaviv: rework MMU handling")
Signed-off-by: Tomeu Vizoso <tomeu@tomeuvizoso.net>
---
 drivers/gpu/drm/etnaviv/etnaviv_buffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_buffer.c b/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
index b13a17276d07..88385dc3b30d 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
@@ -347,7 +347,7 @@ void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, u32 exec_state,
 	u32 link_target, link_dwords;
 	bool switch_context = gpu->exec_state != exec_state;
 	bool switch_mmu_context = gpu->mmu_context != mmu_context;
-	unsigned int new_flush_seq = READ_ONCE(gpu->mmu_context->flush_seq);
+	unsigned int new_flush_seq = READ_ONCE(mmu_context->flush_seq);
 	bool need_flush = switch_mmu_context || gpu->flush_seq != new_flush_seq;
 	bool has_blt = !!(gpu->identity.minor_features5 &
 			  chipMinorFeatures5_BLT_ENGINE);
-- 
2.49.0

Re: [PATCH v2] drm/etnaviv: Fix flush sequence logic

[Lucas Stach]

Am Donnerstag, dem 08.05.2025 um 16:56 +0200 schrieb Tomeu Vizoso:
> We should be comparing the last submitted sequence number with that of
> the address space we may be switching to.
> 
This isn't the relevant change here though: if we switch the address
space, the comparison is moot, as we do a full flush on AS switch
anyway. The relevant change is that with the old code we would record
the flush sequence of the AS we switch away from as the current flush
sequence, so we might miss a necessary flush on the next submission if
that one doesn't require a AS switch, but would only flush based on
sequence mismatch.

Mind if I rewrite the commit message along those lines while applying?

Regards,
Lucas

> Fixes: 27b67278e007 ("drm/etnaviv: rework MMU handling")
> Signed-off-by: Tomeu Vizoso <tomeu@tomeuvizoso.net>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_buffer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_buffer.c b/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
> index b13a17276d07..88385dc3b30d 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
> @@ -347,7 +347,7 @@ void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, u32 exec_state,
>  	u32 link_target, link_dwords;
>  	bool switch_context = gpu->exec_state != exec_state;
>  	bool switch_mmu_context = gpu->mmu_context != mmu_context;
> -	unsigned int new_flush_seq = READ_ONCE(gpu->mmu_context->flush_seq);
> +	unsigned int new_flush_seq = READ_ONCE(mmu_context->flush_seq);
>  	bool need_flush = switch_mmu_context || gpu->flush_seq != new_flush_seq;
>  	bool has_blt = !!(gpu->identity.minor_features5 &
>  			  chipMinorFeatures5_BLT_ENGINE);

Re: [PATCH v2] drm/etnaviv: Fix flush sequence logic

[Tomeu Vizoso]

On Thu, May 8, 2025 at 7:08 PM Lucas Stach <l.stach@pengutronix.de> wrote:
>
> Am Donnerstag, dem 08.05.2025 um 16:56 +0200 schrieb Tomeu Vizoso:
> > We should be comparing the last submitted sequence number with that of
> > the address space we may be switching to.
> >
> This isn't the relevant change here though: if we switch the address
> space, the comparison is moot, as we do a full flush on AS switch
> anyway. The relevant change is that with the old code we would record
> the flush sequence of the AS we switch away from as the current flush
> sequence, so we might miss a necessary flush on the next submission if
> that one doesn't require a AS switch, but would only flush based on
> sequence mismatch.

Ah, you are right.

> Mind if I rewrite the commit message along those lines while applying?

Sure, no problem.

Thanks,

Tomeu

> Regards,
> Lucas
>
> > Fixes: 27b67278e007 ("drm/etnaviv: rework MMU handling")
> > Signed-off-by: Tomeu Vizoso <tomeu@tomeuvizoso.net>
> > ---
> >  drivers/gpu/drm/etnaviv/etnaviv_buffer.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_buffer.c b/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
> > index b13a17276d07..88385dc3b30d 100644
> > --- a/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
> > @@ -347,7 +347,7 @@ void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, u32 exec_state,
> >       u32 link_target, link_dwords;
> >       bool switch_context = gpu->exec_state != exec_state;
> >       bool switch_mmu_context = gpu->mmu_context != mmu_context;
> > -     unsigned int new_flush_seq = READ_ONCE(gpu->mmu_context->flush_seq);
> > +     unsigned int new_flush_seq = READ_ONCE(mmu_context->flush_seq);
> >       bool need_flush = switch_mmu_context || gpu->flush_seq != new_flush_seq;
> >       bool has_blt = !!(gpu->identity.minor_features5 &
> >                         chipMinorFeatures5_BLT_ENGINE);
>

Re: [PATCH v2] drm/etnaviv: Fix flush sequence logic

[Christian Gmeiner]

Hi Lucas,

> > > We should be comparing the last submitted sequence number with that of
> > > the address space we may be switching to.
> > >
> > This isn't the relevant change here though: if we switch the address
> > space, the comparison is moot, as we do a full flush on AS switch
> > anyway. The relevant change is that with the old code we would record
> > the flush sequence of the AS we switch away from as the current flush
> > sequence, so we might miss a necessary flush on the next submission if
> > that one doesn't require a AS switch, but would only flush based on
> > sequence mismatch.
>
> Ah, you are right.
>
> > Mind if I rewrite the commit message along those lines while applying?
>

Now that v6.16 has been tagged, I was wondering why this patch didn’t make
it into this release. From the timeline, it seemed like there was
enough time for it
to be included, so I’m just trying to understand if it was overlooked
or deferred
for a reason.

I also haven’t seen any recent activity at
https://git.pengutronix.de/cgit/lst/linux/, which
made me unsure about the current status of patch queue handling.

-- 
Thanks
--
Christian Gmeiner, MSc

https://christian-gmeiner.info/privacypolicy

Re: [PATCH v2] drm/etnaviv: Fix flush sequence logic

[Lucas Stach]

Hi Christian,

Am Montag, dem 28.07.2025 um 00:28 +0200 schrieb Christian Gmeiner:
> Hi Lucas,
> 
> > > > We should be comparing the last submitted sequence number with that of
> > > > the address space we may be switching to.
> > > > 
> > > This isn't the relevant change here though: if we switch the address
> > > space, the comparison is moot, as we do a full flush on AS switch
> > > anyway. The relevant change is that with the old code we would record
> > > the flush sequence of the AS we switch away from as the current flush
> > > sequence, so we might miss a necessary flush on the next submission if
> > > that one doesn't require a AS switch, but would only flush based on
> > > sequence mismatch.
> > 
> > Ah, you are right.
> > 
> > > Mind if I rewrite the commit message along those lines while applying?
> > 
> 
> Now that v6.16 has been tagged, I was wondering why this patch didn’t make
> it into this release. From the timeline, it seemed like there was
> enough time for it
> to be included, so I’m just trying to understand if it was overlooked
> or deferred for a reason.
> 
That's 100% on me. I've applied the patch with the reworded commit
message into my repo, but then didn't push it out as I was planning on
batching this with some other patches. This didn't happen due to other
tasks pushing this down in my priority list.

> I also haven’t seen any recent activity at
> https://git.pengutronix.de/cgit/lst/linux/, which
> made me unsure about the current status of patch queue handling.
> 
Yea, the current workflow of funneling things through this repository
doesn't work too well for the occasional small fix/patch. It works okay
as long as there are some more patches queued, but small patches tend
to get deferred for too long.
I'm pondering the idea of applying small fixes directly into drm-misc
to get them on a more predictable schedule to upstream. This will
require some changes to my workflow and I'll probably announce this via
a appropriate change to MAINTAINERS, as soon as I'm ready to make the
switch.

Regards,
Lucas