Re: [Freedreno] [PATCH 07/12] drm/msm/adreno: Move speedbin mapping to device table

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 10/07/2023 22:56, Rob Clark wrote:
> On Thu, Jul 6, 2023 at 7:54 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>>
>> On 07/07/2023 00:10, Rob Clark wrote:
>>> From: Rob Clark <robdclark@chromium.org>
>>>
>>> This simplifies the code.
>>>
>>> Signed-off-by: Rob Clark <robdclark@chromium.org>
>>> ---
>>>    drivers/gpu/drm/msm/adreno/a6xx_gpu.c      | 171 ++-------------------
>>>    drivers/gpu/drm/msm/adreno/adreno_device.c |  51 ++++++
>>>    drivers/gpu/drm/msm/adreno/adreno_gpu.h    |  25 +++
>>>    3 files changed, 92 insertions(+), 155 deletions(-)
>>
>>
>> Interesting hack, I'd say.
>>
>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>
>> Minor nit below.
>>
>>>
>>
>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
>>> index d5335b99c64c..994ac26ce731 100644
>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
>>> @@ -72,8 +72,33 @@ struct adreno_info {
>>>        u32 inactive_period;
>>>        const struct adreno_reglist *hwcg;
>>>        u64 address_space_size;
>>> +     /**
>>> +      * @speedbins: Optional table of fuse to speedbin mappings
>>> +      *
>>> +      * Consists of pairs of fuse, index mappings, terminated with
>>> +      * UINT_MAX sentinal.
>>> +      */
>>> +     uint32_t *speedbins;
>>
>> Would it be better to explicitly list this as pairs of uint32_t? And
>> then use braces in ADRENO_SPEEDBIN initialisation.
> 
> It would mean the sentinel would take 8 bytes instead of 4.. maybe
> that is over-thinking it, but it was the reason I just stuck with a
> flat table

Guessed so. But we are wasting so much memory already... I think that 
the paired structure would better reflect the data - it's not a flat 
list, but a list of nvmem <-> speedbin pairs.

> 
> BR,
> -R
> 
>>>    };
>>>
>>> +/*
>>> + * Helper to build a speedbin table, ie. the table:
>>> + *      fuse | speedbin
>>> + *      -----+---------
>>> + *        0  |   0
>>> + *       169 |   1
>>> + *       174 |   2
>>> + *
>>> + * would be declared as:
>>> + *
>>> + *     .speedbins = ADRENO_SPEEDBINS(
>>> + *                      0,   0,
>>> + *                      169, 1,
>>> + *                      174, 2
>>> + *                  ),
>>> + */
>>> +#define ADRENO_SPEEDBINS(tbl...) (uint32_t[]) { tbl, UINT_MAX }
>>> +
>>>    const struct adreno_info *adreno_info(struct adreno_rev rev);
>>>
>>>    struct adreno_gpu {
>>
>> --
>> With best wishes
>> Dmitry
>>

-- 
With best wishes
Dmitry