On 4/23/25 4:55 PM, Rob Clark wrote:
> On Tue, Apr 22, 2025 at 4:57 PM Konrad Dybcio
> <konrad.dyb...@oss.qualcomm.com> wrote:
>>
>> On 4/21/25 10:13 PM, Rob Clark wrote:
>>> On Fri, Apr 18, 2025 at 9:00 AM Akhil P Oommen <quic_akhi...@quicinc.com>
>>> wrote:
>>>>
>>>> On 4/18/2025 6:40 AM, Connor Abbott wrote:
>>>>> On Thu, Apr 17, 2025, 1:50 PM Akhil P Oommen <quic_akhi...@quicinc.com>
>>>>> wrote:
>>>>>>
>>>>>> On 4/17/2025 9:02 PM, Connor Abbott wrote:
>>>>>>> On Thu, Apr 17, 2025 at 3:45 AM Akhil P Oommen
>>>>>>> <quic_akhi...@quicinc.com> wrote:
>>>>>>>>
>>>>>>>> On 4/10/2025 11:13 PM, Konrad Dybcio wrote:
>>>>>>>>> From: Konrad Dybcio <konrad.dyb...@oss.qualcomm.com>
>>>>>>>>>
>>>>>>>>> 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.dyb...@oss.qualcomm.com>
>>>>>>>>> ---
>>>>>>>>> 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.
>>>>>
>>>>> Fair enough, I'm not going to argue with that part. Just wanted to
>>>>> clear up any confusion about Mesa.
>>>>>
>>>>> Although, IIRC kgsl did set different values for a650 depending on
>>>>> memory type... do you know what source that used?
>>>>
>>>> KGSL relies on an undocumented devicetree node populated by bootloader
>>>> to detect ddrtype and calculates the HBB value based on that.
>>>
>>> Would it be reasonable to use the smem value, but if we find the
>>> undocumented dt property, WARN_ON() if it's value disagrees with smem?
>>>
>>> That would at least give some confidence, or justified un-confidence
>>> about the smem values
>>
>> The aforementioned value is populated based on the data that this
>> driver reads out, and only on the same range of platforms that this
>> driver happens to cater to
>
> Did I understand that correctly to mean that the dt property is based
> on the same smem value that you are using?
Yes, abl reads it out and modifies the FDT based on what's in there
Konrad
> In that case, there should
> be no argument against using the smem value as the source of truth.
>
> BR,
> -R
