[PATCH 1/2] drm/v3d: fix client obtained from axi_ids on V3D 4.1

[PATCH 1/2] drm/v3d: fix client obtained from axi_ids on V3D 4.1

[Jose Maria Casanova Crespo]

The client that causes an MMU error is expected to be reported.
But in the case of MMU TFU errors, a non existing client
was being reported. This happened because  because the client
calculation was taking into account more than the bits 0-7
from the axi_id that were representing the client.

This patch masks the proper bits to do the calculation and
limits the returned clients to the expected axi_id ranges that
V3D 4.1 and 4.2 use.

Fixes: 38c2c7917adc ("drm/v3d: Fix and extend MMU error handling.")
Signed-off-by: Jose Maria Casanova Crespo <jmcasanova@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_irq.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_irq.c b/drivers/gpu/drm/v3d/v3d_irq.c
index 29f63f572d35..1810743ea7b8 100644
--- a/drivers/gpu/drm/v3d/v3d_irq.c
+++ b/drivers/gpu/drm/v3d/v3d_irq.c
@@ -186,24 +186,33 @@ v3d_hub_irq(int irq, void *arg)
 		u32 axi_id = V3D_READ(V3D_MMU_VIO_ID);
 		u64 vio_addr = ((u64)V3D_READ(V3D_MMU_VIO_ADDR) <<
 				(v3d->va_width - 32));
-		static const char *const v3d41_axi_ids[] = {
-			"L2T",
-			"PTB",
-			"PSE",
-			"TLB",
-			"CLE",
-			"TFU",
-			"MMU",
-			"GMP",
+		static const struct {
+			u32 begin;
+			u32 end;
+			const char *client;
+		} v3d41_axi_ids[] = {
+			{0x00, 0x20, "L2T"},
+			{0x20, 0x21, "PTB"},
+			{0x40, 0x41, "PSE"},
+			{0x60, 0x80, "TLB"},
+			{0x80, 0x88, "CLE"},
+			{0xA0, 0xA1, "TFU"},
+			{0xC0, 0xE0, "MMU"},
+			{0xE0, 0xE1, "GMP"},
 		};
 		const char *client = "?";
 
 		V3D_WRITE(V3D_MMU_CTL, V3D_READ(V3D_MMU_CTL));
 
 		if (v3d->ver >= V3D_GEN_41) {
-			axi_id = axi_id >> 5;
-			if (axi_id < ARRAY_SIZE(v3d41_axi_ids))
-				client = v3d41_axi_ids[axi_id];
+			axi_id = axi_id & 0xFF;
+			for (size_t i = 0; i < ARRAY_SIZE(v3d41_axi_ids); i++) {
+				if (axi_id >= v3d41_axi_ids[i].begin &&
+				    axi_id < v3d41_axi_ids[i].end) {
+					client = v3d41_axi_ids[i].client;
+					break;
+				}
+			}
 		}
 
 		dev_err(v3d->drm.dev, "MMU error from client %s (%d) at 0x%llx%s%s%s\n",
-- 
2.49.0

[PATCH 2/2] drm/v3d: client ranges from axi_ids are different with V3D 7.1

[Jose Maria Casanova Crespo]

The client mask has been reduced from 8 bits on V3D 4.1 to 7 bits
on V3d 7.1, so the ranges for each client are not compatible.

A new CSD client can now report MMU errors on 7.1

Signed-off-by: Jose Maria Casanova Crespo <jmcasanova@igalia.com>
---
 drivers/gpu/drm/v3d/v3d_irq.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/v3d/v3d_irq.c b/drivers/gpu/drm/v3d/v3d_irq.c
index 1810743ea7b8..0cc1c7e5b412 100644
--- a/drivers/gpu/drm/v3d/v3d_irq.c
+++ b/drivers/gpu/drm/v3d/v3d_irq.c
@@ -199,12 +199,31 @@ v3d_hub_irq(int irq, void *arg)
 			{0xA0, 0xA1, "TFU"},
 			{0xC0, 0xE0, "MMU"},
 			{0xE0, 0xE1, "GMP"},
+		}, v3d71_axi_ids[] = {
+			{0x00, 0x30, "L2T"},
+			{0x30, 0x38, "CLE"},
+			{0x38, 0x39, "PTB"},
+			{0x39, 0x3A, "PSE"},
+			{0x3A, 0x3B, "CSD"},
+			{0x40, 0x60, "TLB"},
+			{0x60, 0x70, "MMU"},
+			{0x7C, 0x7E, "TFU"},
+			{0x7F, 0x80, "GMP"},
 		};
 		const char *client = "?";
 
 		V3D_WRITE(V3D_MMU_CTL, V3D_READ(V3D_MMU_CTL));
 
-		if (v3d->ver >= V3D_GEN_41) {
+		if (v3d->ver >= V3D_GEN_71) {
+			axi_id = axi_id & 0x7F;
+			for (size_t i = 0; i < ARRAY_SIZE(v3d71_axi_ids); i++) {
+				if (axi_id >= v3d71_axi_ids[i].begin &&
+				    axi_id < v3d71_axi_ids[i].end) {
+					client = v3d71_axi_ids[i].client;
+					break;
+				}
+			}
+		} else if (v3d->ver >= V3D_GEN_41) {
 			axi_id = axi_id & 0xFF;
 			for (size_t i = 0; i < ARRAY_SIZE(v3d41_axi_ids); i++) {
 				if (axi_id >= v3d41_axi_ids[i].begin &&
-- 
2.49.0

Re: [PATCH 2/2] drm/v3d: client ranges from axi_ids are different with V3D 7.1

[Maíra Canal]

Hi Chema,

On 09/04/25 12:55, Jose Maria Casanova Crespo wrote:
> The client mask has been reduced from 8 bits on V3D 4.1 to 7 bits
> on V3d 7.1, so the ranges for each client are not compatible.

s/V3d/V3D

> 
> A new CSD client can now report MMU errors on 7.1

How about "On V3D 7.1, the CSD client can also report MMU errors.
Therefore, add its AXI ID to the IDs list."?

Note that a commit message should use the imperative mood:

"Describe your changes in imperative mood, e.g. “make xyzzy do frotz”
instead of “[This patch] makes xyzzy do frotz” or “[I] changed xyzzy to
do frotz”, as if you are giving orders to the codebase to change its
behaviour." [1]

I miss such imperative description in this commit message.

Also, you could add a "Fixes:" tag pointing to the commit that
introduced V3D 7.1. This will allow this commit to go to the stable
trees.

[1] https://docs.kernel.org/process/submitting-patches.html

> 
> Signed-off-by: Jose Maria Casanova Crespo <jmcasanova@igalia.com>
> ---
>   drivers/gpu/drm/v3d/v3d_irq.c | 21 ++++++++++++++++++++-
>   1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/v3d/v3d_irq.c b/drivers/gpu/drm/v3d/v3d_irq.c
> index 1810743ea7b8..0cc1c7e5b412 100644
> --- a/drivers/gpu/drm/v3d/v3d_irq.c
> +++ b/drivers/gpu/drm/v3d/v3d_irq.c
> @@ -199,12 +199,31 @@ v3d_hub_irq(int irq, void *arg)
>   			{0xA0, 0xA1, "TFU"},
>   			{0xC0, 0xE0, "MMU"},
>   			{0xE0, 0xE1, "GMP"},
> +		}, v3d71_axi_ids[] = {
> +			{0x00, 0x30, "L2T"},
> +			{0x30, 0x38, "CLE"},
> +			{0x38, 0x39, "PTB"},
> +			{0x39, 0x3A, "PSE"},
> +			{0x3A, 0x3B, "CSD"},
> +			{0x40, 0x60, "TLB"},
> +			{0x60, 0x70, "MMU"},
> +			{0x7C, 0x7E, "TFU"},
> +			{0x7F, 0x80, "GMP"},
>   		};
>   		const char *client = "?";
>   
>   		V3D_WRITE(V3D_MMU_CTL, V3D_READ(V3D_MMU_CTL));
>   
> -		if (v3d->ver >= V3D_GEN_41) {
> +		if (v3d->ver >= V3D_GEN_71) {
> +			axi_id = axi_id & 0x7F;
> +			for (size_t i = 0; i < ARRAY_SIZE(v3d71_axi_ids); i++) {
> +				if (axi_id >= v3d71_axi_ids[i].begin &&
> +				    axi_id < v3d71_axi_ids[i].end) {
> +					client = v3d71_axi_ids[i].client;
> +					break;
> +				}
> +			}

What do you think about assigning v3d71_axi_ids or v3d41_axi_ids to an 
temporary variable and move this loop below? Something like,

if (v3d->ver >= V3D_GEN_71) {
	axi_id = axi_id & 0x7F;
	v3d_axi_ids = v3d71_axi_ids;
} else if ... {
	...
}

for (size_t i = 0; i < ARRAY_SIZE(v3d_axi_ids); i++) {
	if (axi_id >= v3d_axi_ids[i].begin
	    && axi_id < v3d_axi_ids[i].end) {
		client = v3d_axi_ids[i].client;
		break;
	}
}

Best Regards,
- Maíra

> +		} else if (v3d->ver >= V3D_GEN_41) {
>   			axi_id = axi_id & 0xFF;
>   			for (size_t i = 0; i < ARRAY_SIZE(v3d41_axi_ids); i++) {
>   				if (axi_id >= v3d41_axi_ids[i].begin &&

Re: [PATCH 2/2] drm/v3d: client ranges from axi_ids are different with V3D 7.1

[Chema Casanova]

El 10/4/25 a las 14:40, Maíra Canal escribió:
> Hi Chema,
>
> On 09/04/25 12:55, Jose Maria Casanova Crespo wrote:
>> The client mask has been reduced from 8 bits on V3D 4.1 to 7 bits
>> on V3d 7.1, so the ranges for each client are not compatible.
>
> s/V3d/V3D
>
Fixed.
>>
>> A new CSD client can now report MMU errors on 7.1
>
> How about "On V3D 7.1, the CSD client can also report MMU errors.
> Therefore, add its AXI ID to the IDs list."?
>
> Note that a commit message should use the imperative mood:
>
> "Describe your changes in imperative mood, e.g. “make xyzzy do frotz”
> instead of “[This patch] makes xyzzy do frotz” or “[I] changed xyzzy to
> do frotz”, as if you are giving orders to the codebase to change its
> behaviour." [1]
>
> I miss such imperative description in this commit message.
>
I tried for v2.
>
> Also, you could add a "Fixes:" tag pointing to the commit that
> introduced V3D 7.1. This will allow this commit to go to the stable
> trees.
>
> [1] https://docs.kernel.org/process/submitting-patches.html
I already included the fixes tag for v2. Initially, I had doubts because
as I thought that the fix was not critical, as at the end was only
affecting to debug message.
>
>>
>> Signed-off-by: Jose Maria Casanova Crespo <jmcasanova@igalia.com>
>> ---
>>   drivers/gpu/drm/v3d/v3d_irq.c | 21 ++++++++++++++++++++-
>>   1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/v3d/v3d_irq.c 
>> b/drivers/gpu/drm/v3d/v3d_irq.c
>> index 1810743ea7b8..0cc1c7e5b412 100644
>> --- a/drivers/gpu/drm/v3d/v3d_irq.c
>> +++ b/drivers/gpu/drm/v3d/v3d_irq.c
>> @@ -199,12 +199,31 @@ v3d_hub_irq(int irq, void *arg)
>>               {0xA0, 0xA1, "TFU"},
>>               {0xC0, 0xE0, "MMU"},
>>               {0xE0, 0xE1, "GMP"},
>> +        }, v3d71_axi_ids[] = {
>> +            {0x00, 0x30, "L2T"},
>> +            {0x30, 0x38, "CLE"},
>> +            {0x38, 0x39, "PTB"},
>> +            {0x39, 0x3A, "PSE"},
>> +            {0x3A, 0x3B, "CSD"},
>> +            {0x40, 0x60, "TLB"},
>> +            {0x60, 0x70, "MMU"},
>> +            {0x7C, 0x7E, "TFU"},
>> +            {0x7F, 0x80, "GMP"},
>>           };
>>           const char *client = "?";
>>             V3D_WRITE(V3D_MMU_CTL, V3D_READ(V3D_MMU_CTL));
>>   -        if (v3d->ver >= V3D_GEN_41) {
>> +        if (v3d->ver >= V3D_GEN_71) {
>> +            axi_id = axi_id & 0x7F;
>> +            for (size_t i = 0; i < ARRAY_SIZE(v3d71_axi_ids); i++) {
>> +                if (axi_id >= v3d71_axi_ids[i].begin &&
>> +                    axi_id < v3d71_axi_ids[i].end) {
>> +                    client = v3d71_axi_ids[i].client;
>> +                    break;
>> +                }
>> +            }
>
> What do you think about assigning v3d71_axi_ids or v3d41_axi_ids to an 
> temporary variable and move this loop below? Something like,
>
> if (v3d->ver >= V3D_GEN_71) {
>     axi_id = axi_id & 0x7F;
>     v3d_axi_ids = v3d71_axi_ids;
> } else if ... {
>     ...
> }
>
> for (size_t i = 0; i < ARRAY_SIZE(v3d_axi_ids); i++) {
>     if (axi_id >= v3d_axi_ids[i].begin
>         && axi_id < v3d_axi_ids[i].end) {
>         client = v3d_axi_ids[i].client;
>         break;
>     }
> }
After checking with Maíra we agree that was simpler the original
approach, as we would need to include an extra of the number
of elements in the arrays as ARRAY_SIZE and the compiler would
need to thread the size of the arrays as dynamic.

Kind regards,
Chema Casanova

Re: [PATCH 1/2] drm/v3d: fix client obtained from axi_ids on V3D 4.1

[Maíra Canal]

Hi Chema,

On 09/04/25 12:55, Jose Maria Casanova Crespo wrote:
> The client that causes an MMU error is expected to be reported.
> But in the case of MMU TFU errors, a non existing client

"In the case of MMU errors caused by the TFU unit, [...]"

> was being reported. This happened because  because the client

There are two "because" in the sentence. Could you add an example of the
a MMU error with a non-existing client in the commit message?

> calculation was taking into account more than the bits 0-7
> from the axi_id that were representing the client.
> 
> This patch masks the proper bits to do the calculation and
> limits the returned clients to the expected axi_id ranges that
> V3D 4.1 and 4.2 use.
> 
> Fixes: 38c2c7917adc ("drm/v3d: Fix and extend MMU error handling.")
> Signed-off-by: Jose Maria Casanova Crespo <jmcasanova@igalia.com>
> ---
>   drivers/gpu/drm/v3d/v3d_irq.c | 33 +++++++++++++++++++++------------
>   1 file changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/v3d/v3d_irq.c b/drivers/gpu/drm/v3d/v3d_irq.c
> index 29f63f572d35..1810743ea7b8 100644
> --- a/drivers/gpu/drm/v3d/v3d_irq.c
> +++ b/drivers/gpu/drm/v3d/v3d_irq.c
> @@ -186,24 +186,33 @@ v3d_hub_irq(int irq, void *arg)
>   		u32 axi_id = V3D_READ(V3D_MMU_VIO_ID);
>   		u64 vio_addr = ((u64)V3D_READ(V3D_MMU_VIO_ADDR) <<
>   				(v3d->va_width - 32));
> -		static const char *const v3d41_axi_ids[] = {
> -			"L2T",
> -			"PTB",
> -			"PSE",
> -			"TLB",
> -			"CLE",
> -			"TFU",
> -			"MMU",
> -			"GMP",
> +		static const struct {
> +			u32 begin;
> +			u32 end;
> +			const char *client;
> +		} v3d41_axi_ids[] = {
> +			{0x00, 0x20, "L2T"},
> +			{0x20, 0x21, "PTB"},
> +			{0x40, 0x41, "PSE"},
> +			{0x60, 0x80, "TLB"},
> +			{0x80, 0x88, "CLE"},
> +			{0xA0, 0xA1, "TFU"},
> +			{0xC0, 0xE0, "MMU"},
> +			{0xE0, 0xE1, "GMP"},
>   		};
>   		const char *client = "?";
>   
>   		V3D_WRITE(V3D_MMU_CTL, V3D_READ(V3D_MMU_CTL));
>   
>   		if (v3d->ver >= V3D_GEN_41) {
> -			axi_id = axi_id >> 5;
> -			if (axi_id < ARRAY_SIZE(v3d41_axi_ids))
> -				client = v3d41_axi_ids[axi_id];
> +			axi_id = axi_id & 0xFF;
> +			for (size_t i = 0; i < ARRAY_SIZE(v3d41_axi_ids); i++) {
> +				if (axi_id >= v3d41_axi_ids[i].begin &&
> +				    axi_id < v3d41_axi_ids[i].end) {
> +					client = v3d41_axi_ids[i].client;
> +					break;
> +				}
> +			}
>   		}
>   
>   		dev_err(v3d->drm.dev, "MMU error from client %s (%d) at 0x%llx%s%s%s\n",

As we are declaring `begin` and `end` as hexadecimal numbers in the
code, could we display AXI ID as a hexadecimal as well? Just to ease
future debugging. You would need to change "(%d)".

Please, don't forget to mention this change in the commit message of v2.

Thanks for your patch!

Best Regards,
- Maíra

Re: [PATCH 1/2] drm/v3d: fix client obtained from axi_ids on V3D 4.1

[Chema Casanova]

El 10/4/25 a las 14:31, Maíra Canal escribió:
> Hi Chema,
>
> On 09/04/25 12:55, Jose Maria Casanova Crespo wrote:
>> The client that causes an MMU error is expected to be reported.
>> But in the case of MMU TFU errors, a non existing client
>
> "In the case of MMU errors caused by the TFU unit, [...]"

Message updated.

>
>> was being reported. This happened because because the client
>
> There are two "because" in the sentence. Could you add an example of the
> a MMU error with a non-existing client in the commit message?
>
Example added.

[...]
>>           }
>>             dev_err(v3d->drm.dev, "MMU error from client %s (%d) at 
>> 0x%llx%s%s%s\n",
>
> As we are declaring `begin` and `end` as hexadecimal numbers in the
> code, could we display AXI ID as a hexadecimal as well? Just to ease
> future debugging. You would need to change "(%d)".
>
Change done.
>
> Please, don't forget to mention this change in the commit message of v2.
>
Already included in v2.

Thanks for your review Maíra.

Kind regards.
Chema Casanova