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

[Chema Casanova <jmcasanova@igalia.com>]

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