Re: [PATCH RFT 04/14] drm/msm/a6xx: Get a handle to the common UBWC config

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 5/14/25 12:06 AM, Rob Clark wrote:
> On Fri, May 9, 2025 at 10:00 AM Konrad Dybcio
> <konrad.dybcio@oss.qualcomm.com> wrote:
>>
>> On 5/9/25 3:52 PM, Rob Clark wrote:
>>> On Fri, May 9, 2025 at 5:31 AM Konrad Dybcio
>>> <konrad.dybcio@oss.qualcomm.com> wrote:
>>>>
>>>> On 5/8/25 8:41 PM, Rob Clark wrote:
>>>>> On Thu, May 8, 2025 at 11:13 AM Konrad Dybcio <konradybcio@kernel.org> wrote:
>>>>>>
>>>>>> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>>>>
>>>>>> Start the great despaghettification by getting a pointer to the common
>>>>>> UBWC configuration, which houses e.g. UBWC versions that we need to
>>>>>> make decisions.
>>>>>>
>>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c   | 16 ++++++++++++++--
>>>>>>  drivers/gpu/drm/msm/adreno/adreno_gpu.c |  6 ++++++
>>>>>>  drivers/gpu/drm/msm/adreno/adreno_gpu.h |  3 +++
>>>>>>  3 files changed, 23 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>>>>> index b161b5cd991fc645dfcd69754b82be9691775ffe..89eb725f0950f3679d6214366cfbd22d5bcf4bc7 100644
>>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>>>>> @@ -585,8 +585,13 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu)
>>>>>>         gpu_write(gpu, REG_A6XX_CP_PROTECT(protect->count_max - 1), protect->regs[i]);
>>>>>>  }
>>>>>>
>>>>>> -static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu)
>>>>>> +static int a6xx_calc_ubwc_config(struct adreno_gpu *gpu)
>>>>>>  {
>>>>>> +       /* Inherit the common config and make some necessary fixups */
>>>>>> +       gpu->common_ubwc_cfg = qcom_ubwc_config_get_data();
>>>>>
>>>>> This does look a bit funny given the devm_kzalloc() below.. I guess
>>>>> just so that the ptr is never NULL?
>>>>
>>>> Yeah, would you prefer this is changed?
>>>
>>> I think having an all zeros ubwc cfg isn't really going to work
>>> anyways, so probably drop the kzalloc().  Or if there is a case that
>>> I'm not thinking of offhand where it makes sense to have an all 0's
>>> cfg, then add a comment to avoid future head scratching, since
>>> otherwise it looks like a bug to be fixed.
>>
>> So my own lack of comments bit me.
>>
>> Without the allocation this will fall apart badly..
>> I added this hunk:
>>
>> ---------------------
>> /* Inherit the common config and make some necessary fixups */
>> common_cfg = if (IS_ERR(common_cfg))
>>         return ERR_PTR(-EINVAL);
>>
>> *adreno_gpu->ubwc_config = *common_cfg;
>> ---------------------
>>
>> to get the common data but take away the const qualifier.. because
>> we still override some HBB values and we can't yet fully trust the
>> common config, as the smem getter is not yet plumbed up.
> 
> So I get that common_ubwc_cfg is the const thing without fixups (and
> agree that it should say const), and ubwc_config is the fixed up
> thing.  But don't see how that necessitates the zeroalloc.  Couldn't
> you just:
> 
> 
>   if (!IS_ERR_OR_NULL(adreno_gpu->common_ubwc_cfg)
>     adreno_gpu->ubwc_config = *adreno_gpu->common_ubwc_cfg;

Aaaah I read into what me-a-week-ago thought and realized I did that so
that I can still make overrides in a5xx_gpu.c (where this data is
*always* hardcoded up until now) - I can simply squash the last patch
with this one and we should be gtg without the zeroalloc

>> I can add a commit discarding all the HBB overrides (matching or not)
>> or we can keep the zeroalloc around for some time (i'd rather keep
>> the function returning const so that when things are ready nobody gets
>> to poke at the source of *truth*)
> 
> We can keep the overrides to start (although the goal should be to
> remove them).. but qcom_ubwc_config_get_data() not finding anything
> seems like more or less a fatal condition.

Indeed

Konrad