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.
-Akhil.
>
>>
>> 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.
>
> AFAIK the HBB (as well as other UBWC config parameters) doesn't have
> any impact on layout configuration, so the UMD can ignore it except
> when it's doing CPU texture uploads/downloads. We certainly do in
> freedreno/turnip. You'd have to ask that team what they use HBB for,
> but my best guess is that the GLES driver uses it for CPU texture
> uploads sometimes. That is, the GLES driver might be using
> functionality similar to host_image_copy "under the hood". It's
> something we'd probably want for freedreno too.
>
> Connor
>
>>
>>>
>>> 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);
>>>>>
>>>>
>>
