Re: [PATCH v2 3/4] drm/msm/a6xx: Get HBB dynamically, if available

[Akhil P Oommen]

On 4/17/2025 9:02 PM, Connor Abbott wrote:
> On Thu, Apr 17, 2025 at 3:45 AM Akhil P Oommen <quic_akhilpo@xxxxxxxxxxx> wrote:
>>
>> On 4/10/2025 11:13 PM, Konrad Dybcio wrote:
>>> From: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxxxx>
>>>
>>> The Highest Bank address Bit value can change based on memory type used.
>>>
>>> Attempt to retrieve it dynamically, and fall back to a reasonable
>>> default (the one used prior to this change) on error.
>>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxxxxxxxx>
>>> ---
>>>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 15 ++++++++++++++-
>>>  1 file changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>> index 06465bc2d0b4b128cddfcfcaf1fe4252632b6777..a6232b382bd16319f20ae5f8f5e57f38ecc62d9f 100644
>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>> @@ -13,6 +13,7 @@
>>>  #include <linux/firmware/qcom/qcom_scm.h>
>>>  #include <linux/pm_domain.h>
>>>  #include <linux/soc/qcom/llcc-qcom.h>
>>> +#include <linux/soc/qcom/smem.h>
>>>
>>>  #define GPU_PAS_ID 13
>>>
>>> @@ -587,6 +588,8 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu)
>>>
>>>  static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu)
>>>  {
>>> +     int hbb;
>>> +
>>>       gpu->ubwc_config.rgb565_predicator = 0;
>>>       gpu->ubwc_config.uavflagprd_inv = 0;
>>>       gpu->ubwc_config.min_acc_len = 0;
>>> @@ -635,7 +638,6 @@ static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu)
>>>           adreno_is_a690(gpu) ||
>>>           adreno_is_a730(gpu) ||
>>>           adreno_is_a740_family(gpu)) {
>>> -             /* TODO: get ddr type from bootloader and use 2 for LPDDR4 */
>>>               gpu->ubwc_config.highest_bank_bit = 16;
>>>               gpu->ubwc_config.amsbc = 1;
>>>               gpu->ubwc_config.rgb565_predicator = 1;
>>> @@ -664,6 +666,13 @@ static void a6xx_calc_ubwc_config(struct adreno_gpu *gpu)
>>>               gpu->ubwc_config.highest_bank_bit = 14;
>>>               gpu->ubwc_config.min_acc_len = 1;
>>>       }
>>> +
>>> +     /* Attempt to retrieve the data from SMEM, keep the above defaults in case of error */
>>> +     hbb = qcom_smem_dram_get_hbb();
>>> +     if (hbb < 0)
>>> +             return;
>>> +
>>> +     gpu->ubwc_config.highest_bank_bit = hbb;
>>
>> I am worried about blindly relying on SMEM data directly for HBB for
>> legacy chipsets. There is no guarantee it is accurate on every chipset
>> and every version of firmware. Also, until recently, this value was
>> hardcoded in Mesa which matched the value in KMD.
> 
> To be clear about this, from the moment we introduced host image
> copies in Mesa we added support for querying the HBB from the kernel,
> explicitly so that we could do what this series does without Mesa ever
> breaking. Mesa will never assume the HBB unless the kernel is too old
> to support querying it. So don't let Mesa be the thing that stops us
> here.

Thanks for clarifying about Mesa. I still don't trust a data source that
is unused in production.

I have a related question about HBB. Blob driver doesn't support
host_image_copy, but it still use HBB configuration. I was under the
impression this was required for UMD for compression related
configurations. Is that not true for turnip/freedreno?

-Akhil.

> 
> Connor
> 
>> So it is better to
>> make this opt in, for newer chipsets or those which somebody can verify.
>> We can invert this logic to something like this:
>>
>> if (!gpu->ubwc_config.highest_bank_bit)
>>     gpu->ubwc_config.highest_bank_bit = qcom_smem_dram_get_hbb();
>>
>>>  }
>>>
>>>  static void a6xx_set_ubwc_config(struct msm_gpu *gpu)
>>> @@ -2467,6 +2476,10 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
>>>       bool is_a7xx;
>>>       int ret;
>>>
>>> +     /* We need data from SMEM to retrieve HBB in calc_ubwc_config() */
>>> +     if (!qcom_smem_is_available())
>>> +             return ERR_PTR(-EPROBE_DEFER);
>>> +
>>
>> We should add "depends on QCOM_SMEM" to Kconfig. Is SMEM device present
>> in all Qcom SoC devicetrees? I wonder if there is a scenario where there
>> might be an infinite EPROBE_DEFER here.
>>
>> -Akhil.
>>
>>>       a6xx_gpu = kzalloc(sizeof(*a6xx_gpu), GFP_KERNEL);
>>>       if (!a6xx_gpu)
>>>               return ERR_PTR(-ENOMEM);
>>>
>>