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/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.

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*)

Konrad