[v3,04/15] drm/msm/a6xx: Extend and explain UBWC config

  Rename lower_bit to hbb_lo and explain what it signifies.
Add explanations (wherever possible to other tunables).

Sort the variable definition and assignment alphabetically.

Port setting min_access_length, ubwc_mode and hbb_hi from downstream.
Set default values for all of the tunables to zero, as they should be.

Values were validated against downstream and will be fixed up in
separate commits so as not to make this one even more messy.

A618 remains untouched (left at hw defaults) in this patch.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 55 ++++++++++++++++++++++++++++-------
 1 file changed, 45 insertions(+), 10 deletions(-)
  

On 2/23/2023 5:36 PM, Konrad Dybcio wrote:
> Rename lower_bit to hbb_lo and explain what it signifies.
> Add explanations (wherever possible to other tunables).
>
> Sort the variable definition and assignment alphabetically.
Sorting based on decreasing order of line length is more readable, isn't it?
>
> Port setting min_access_length, ubwc_mode and hbb_hi from downstream.
> Set default values for all of the tunables to zero, as they should be.
>
> Values were validated against downstream and will be fixed up in
> separate commits so as not to make this one even more messy.
>
> A618 remains untouched (left at hw defaults) in this patch.
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 55 ++++++++++++++++++++++++++++-------
>  1 file changed, 45 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index c5f5d0bb3fdc..bdae341e0a7c 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -786,39 +786,74 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu)
>  static void a6xx_set_ubwc_config(struct msm_gpu *gpu)
>  {
>  	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> -	u32 lower_bit = 2;
> +	/* Unknown, introduced with A640/680 */
>  	u32 amsbc = 0;
> +	/*
> +	 * The Highest Bank Bit value represents the bit of the highest DDR bank.
> +	 * We then subtract 13 from it (13 is the minimum value allowed by hw) and
> +	 * write the lowest two bits of the remaining value as hbb_lo and the
> +	 * one above it as hbb_hi to the hardware. The default values (when HBB is
> +	 * not specified) are 0, 0.
> +	 */
> +	u32 hbb_hi = 0;
> +	u32 hbb_lo = 0;
> +	/* Whether the minimum access length is 64 bits */
> +	u32 min_acc_len = 0;
> +	/* Unknown, introduced with A650 family, related to UBWC mode/ver 4 */
>  	u32 rgb565_predicator = 0;
> +	/* Unknown, introduced with A650 family */
>  	u32 uavflagprd_inv = 0;
> +	/* Entirely magic, per-GPU-gen value */
> +	u32 ubwc_mode = 0;
>  
>  	/* a618 is using the hw default values */
>  	if (adreno_is_a618(adreno_gpu))
>  		return;
>  
> -	if (adreno_is_a640_family(adreno_gpu))
> +	if (adreno_is_a619(adreno_gpu)) {
> +		/* HBB = 14 */
> +		hbb_lo = 1;
> +	}
> +
> +	if (adreno_is_a630(adreno_gpu)) {
> +		/* HBB = 15 */
> +		hbb_lo = 2;
> +	}
> +
> +	if (adreno_is_a640_family(adreno_gpu)) {
>  		amsbc = 1;
> +		/* HBB = 15 */
> +		hbb_lo = 2;
> +	}
>  
>  	if (adreno_is_a650(adreno_gpu) || adreno_is_a660(adreno_gpu)) {
> -		/* TODO: get ddr type from bootloader and use 2 for LPDDR4 */
> -		lower_bit = 3;
>  		amsbc = 1;
> +		/* TODO: get ddr type from bootloader and use 2 for LPDDR4 */
> +		/* HBB = 16 */
> +		hbb_lo = 3;
>  		rgb565_predicator = 1;
>  		uavflagprd_inv = 2;
>  	}
>  
>  	if (adreno_is_7c3(adreno_gpu)) {
> -		lower_bit = 1;
>  		amsbc = 1;
> +		/* HBB is unset in downstream DTS, defaulting to 0 */
This is incorrect. For 7c3 hbb value is 14. So hbb_lo should be 1. FYI, hbb configurations were moved to the driver from DT in recent downstream kernels.

-Akhil.
>  		rgb565_predicator = 1;
>  		uavflagprd_inv = 2;
>  	}
>  
>  	gpu_write(gpu, REG_A6XX_RB_NC_MODE_CNTL,
> -		rgb565_predicator << 11 | amsbc << 4 | lower_bit << 1);
> -	gpu_write(gpu, REG_A6XX_TPL1_NC_MODE_CNTL, lower_bit << 1);
> -	gpu_write(gpu, REG_A6XX_SP_NC_MODE_CNTL,
> -		uavflagprd_inv << 4 | lower_bit << 1);
> -	gpu_write(gpu, REG_A6XX_UCHE_MODE_CNTL, lower_bit << 21);
> +		  rgb565_predicator << 11 | hbb_hi << 10 | amsbc << 4 |
> +		  min_acc_len << 3 | hbb_lo << 1 | ubwc_mode);
> +
> +	gpu_write(gpu, REG_A6XX_TPL1_NC_MODE_CNTL, hbb_hi << 4 |
> +		  min_acc_len << 3 | hbb_lo << 1 | ubwc_mode);
> +
> +	gpu_write(gpu, REG_A6XX_SP_NC_MODE_CNTL, hbb_hi << 10 |
> +		  uavflagprd_inv << 4 | min_acc_len << 3 |
> +		  hbb_lo << 1 | ubwc_mode);
> +
> +	gpu_write(gpu, REG_A6XX_UCHE_MODE_CNTL, min_acc_len << 23 | hbb_lo << 21);
>  }
>  
>  static int a6xx_cp_init(struct msm_gpu *gpu)
>
  

On 28.02.2023 21:23, Akhil P Oommen wrote:
> On 2/23/2023 5:36 PM, Konrad Dybcio wrote:
>> Rename lower_bit to hbb_lo and explain what it signifies.
>> Add explanations (wherever possible to other tunables).
>>
>> Sort the variable definition and assignment alphabetically.
> Sorting based on decreasing order of line length is more readable, isn't it?
I can do that.

>>
>> Port setting min_access_length, ubwc_mode and hbb_hi from downstream.
>> Set default values for all of the tunables to zero, as they should be.
>>
>> Values were validated against downstream and will be fixed up in
>> separate commits so as not to make this one even more messy.
>>
>> A618 remains untouched (left at hw defaults) in this patch.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 55 ++++++++++++++++++++++++++++-------
>>  1 file changed, 45 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> index c5f5d0bb3fdc..bdae341e0a7c 100644
>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>> @@ -786,39 +786,74 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu)
>>  static void a6xx_set_ubwc_config(struct msm_gpu *gpu)
>>  {
>>  	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>> -	u32 lower_bit = 2;
>> +	/* Unknown, introduced with A640/680 */
>>  	u32 amsbc = 0;
>> +	/*
>> +	 * The Highest Bank Bit value represents the bit of the highest DDR bank.
>> +	 * We then subtract 13 from it (13 is the minimum value allowed by hw) and
>> +	 * write the lowest two bits of the remaining value as hbb_lo and the
>> +	 * one above it as hbb_hi to the hardware. The default values (when HBB is
>> +	 * not specified) are 0, 0.
>> +	 */
>> +	u32 hbb_hi = 0;
>> +	u32 hbb_lo = 0;
>> +	/* Whether the minimum access length is 64 bits */
>> +	u32 min_acc_len = 0;
>> +	/* Unknown, introduced with A650 family, related to UBWC mode/ver 4 */
>>  	u32 rgb565_predicator = 0;
>> +	/* Unknown, introduced with A650 family */
>>  	u32 uavflagprd_inv = 0;
>> +	/* Entirely magic, per-GPU-gen value */
>> +	u32 ubwc_mode = 0;
>>  
>>  	/* a618 is using the hw default values */
>>  	if (adreno_is_a618(adreno_gpu))
>>  		return;
>>  
>> -	if (adreno_is_a640_family(adreno_gpu))
>> +	if (adreno_is_a619(adreno_gpu)) {
>> +		/* HBB = 14 */
>> +		hbb_lo = 1;
>> +	}
>> +
>> +	if (adreno_is_a630(adreno_gpu)) {
>> +		/* HBB = 15 */
>> +		hbb_lo = 2;
>> +	}
>> +
>> +	if (adreno_is_a640_family(adreno_gpu)) {
>>  		amsbc = 1;
>> +		/* HBB = 15 */
>> +		hbb_lo = 2;
>> +	}
>>  
>>  	if (adreno_is_a650(adreno_gpu) || adreno_is_a660(adreno_gpu)) {
>> -		/* TODO: get ddr type from bootloader and use 2 for LPDDR4 */
>> -		lower_bit = 3;
>>  		amsbc = 1;
>> +		/* TODO: get ddr type from bootloader and use 2 for LPDDR4 */
>> +		/* HBB = 16 */
>> +		hbb_lo = 3;
>>  		rgb565_predicator = 1;
>>  		uavflagprd_inv = 2;
>>  	}
>>  
>>  	if (adreno_is_7c3(adreno_gpu)) {
>> -		lower_bit = 1;
>>  		amsbc = 1;
>> +		/* HBB is unset in downstream DTS, defaulting to 0 */
> This is incorrect. For 7c3 hbb value is 14. So hbb_lo should be 1. FYI, hbb configurations were moved to the driver from DT in recent downstream kernels.
Right, seems to have happened with msm-5.10. Though a random kernel I
grabbed seems to suggest it's 15 and not 14?

https://github.com/sonyxperiadev/kernel/blob/aosp/K.P.1.0.r1/drivers/gpu/msm/adreno-gpulist.h#L1710

Konrad
> 
> -Akhil.
>>  		rgb565_predicator = 1;
>>  		uavflagprd_inv = 2;
>>  	}
>>  
>>  	gpu_write(gpu, REG_A6XX_RB_NC_MODE_CNTL,
>> -		rgb565_predicator << 11 | amsbc << 4 | lower_bit << 1);
>> -	gpu_write(gpu, REG_A6XX_TPL1_NC_MODE_CNTL, lower_bit << 1);
>> -	gpu_write(gpu, REG_A6XX_SP_NC_MODE_CNTL,
>> -		uavflagprd_inv << 4 | lower_bit << 1);
>> -	gpu_write(gpu, REG_A6XX_UCHE_MODE_CNTL, lower_bit << 21);
>> +		  rgb565_predicator << 11 | hbb_hi << 10 | amsbc << 4 |
>> +		  min_acc_len << 3 | hbb_lo << 1 | ubwc_mode);
>> +
>> +	gpu_write(gpu, REG_A6XX_TPL1_NC_MODE_CNTL, hbb_hi << 4 |
>> +		  min_acc_len << 3 | hbb_lo << 1 | ubwc_mode);
>> +
>> +	gpu_write(gpu, REG_A6XX_SP_NC_MODE_CNTL, hbb_hi << 10 |
>> +		  uavflagprd_inv << 4 | min_acc_len << 3 |
>> +		  hbb_lo << 1 | ubwc_mode);
>> +
>> +	gpu_write(gpu, REG_A6XX_UCHE_MODE_CNTL, min_acc_len << 23 | hbb_lo << 21);
>>  }
>>  
>>  static int a6xx_cp_init(struct msm_gpu *gpu)
>>
>
  

On 3/1/2023 2:10 AM, Konrad Dybcio wrote:
>
> On 28.02.2023 21:23, Akhil P Oommen wrote:
>> On 2/23/2023 5:36 PM, Konrad Dybcio wrote:
>>> Rename lower_bit to hbb_lo and explain what it signifies.
>>> Add explanations (wherever possible to other tunables).
>>>
>>> Sort the variable definition and assignment alphabetically.
>> Sorting based on decreasing order of line length is more readable, isn't it?
> I can do that.
>
>>> Port setting min_access_length, ubwc_mode and hbb_hi from downstream.
>>> Set default values for all of the tunables to zero, as they should be.
>>>
>>> Values were validated against downstream and will be fixed up in
>>> separate commits so as not to make this one even more messy.
>>>
>>> A618 remains untouched (left at hw defaults) in this patch.
>>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 55 ++++++++++++++++++++++++++++-------
>>>  1 file changed, 45 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>> index c5f5d0bb3fdc..bdae341e0a7c 100644
>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>> @@ -786,39 +786,74 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu)
>>>  static void a6xx_set_ubwc_config(struct msm_gpu *gpu)
>>>  {
>>>  	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>> -	u32 lower_bit = 2;
>>> +	/* Unknown, introduced with A640/680 */
>>>  	u32 amsbc = 0;
>>> +	/*
>>> +	 * The Highest Bank Bit value represents the bit of the highest DDR bank.
>>> +	 * We then subtract 13 from it (13 is the minimum value allowed by hw) and
>>> +	 * write the lowest two bits of the remaining value as hbb_lo and the
>>> +	 * one above it as hbb_hi to the hardware. The default values (when HBB is
>>> +	 * not specified) are 0, 0.
>>> +	 */
>>> +	u32 hbb_hi = 0;
>>> +	u32 hbb_lo = 0;
>>> +	/* Whether the minimum access length is 64 bits */
>>> +	u32 min_acc_len = 0;
>>> +	/* Unknown, introduced with A650 family, related to UBWC mode/ver 4 */
>>>  	u32 rgb565_predicator = 0;
>>> +	/* Unknown, introduced with A650 family */
>>>  	u32 uavflagprd_inv = 0;
>>> +	/* Entirely magic, per-GPU-gen value */
>>> +	u32 ubwc_mode = 0;
>>>  
>>>  	/* a618 is using the hw default values */
>>>  	if (adreno_is_a618(adreno_gpu))
>>>  		return;
>>>  
>>> -	if (adreno_is_a640_family(adreno_gpu))
>>> +	if (adreno_is_a619(adreno_gpu)) {
>>> +		/* HBB = 14 */
>>> +		hbb_lo = 1;
>>> +	}
>>> +
>>> +	if (adreno_is_a630(adreno_gpu)) {
>>> +		/* HBB = 15 */
>>> +		hbb_lo = 2;
>>> +	}
>>> +
>>> +	if (adreno_is_a640_family(adreno_gpu)) {
>>>  		amsbc = 1;
>>> +		/* HBB = 15 */
>>> +		hbb_lo = 2;
>>> +	}
>>>  
>>>  	if (adreno_is_a650(adreno_gpu) || adreno_is_a660(adreno_gpu)) {
>>> -		/* TODO: get ddr type from bootloader and use 2 for LPDDR4 */
>>> -		lower_bit = 3;
>>>  		amsbc = 1;
>>> +		/* TODO: get ddr type from bootloader and use 2 for LPDDR4 */
>>> +		/* HBB = 16 */
>>> +		hbb_lo = 3;
>>>  		rgb565_predicator = 1;
>>>  		uavflagprd_inv = 2;
>>>  	}
>>>  
>>>  	if (adreno_is_7c3(adreno_gpu)) {
>>> -		lower_bit = 1;
>>>  		amsbc = 1;
>>> +		/* HBB is unset in downstream DTS, defaulting to 0 */
>> This is incorrect. For 7c3 hbb value is 14. So hbb_lo should be 1. FYI, hbb configurations were moved to the driver from DT in recent downstream kernels.
> Right, seems to have happened with msm-5.10. Though a random kernel I
> grabbed seems to suggest it's 15 and not 14?
>
> https://github.com/sonyxperiadev/kernel/blob/aosp/K.P.1.0.r1/drivers/gpu/msm/adreno-gpulist.h#L1710
We override that with 14 in a6xx_init() for LP4 platforms dynamically. Since 7c3 is only supported on LP4, we can hardcode 14 here.
In the downstream kernel, there is an api (of_fdt_get_ddrtype()) to detect ddrtype. If we can get something like that in upstream, we should implement a similar logic here.

-Akhil.
>
> Konrad
>> -Akhil.
>>>  		rgb565_predicator = 1;
>>>  		uavflagprd_inv = 2;
>>>  	}
>>>  
>>>  	gpu_write(gpu, REG_A6XX_RB_NC_MODE_CNTL,
>>> -		rgb565_predicator << 11 | amsbc << 4 | lower_bit << 1);
>>> -	gpu_write(gpu, REG_A6XX_TPL1_NC_MODE_CNTL, lower_bit << 1);
>>> -	gpu_write(gpu, REG_A6XX_SP_NC_MODE_CNTL,
>>> -		uavflagprd_inv << 4 | lower_bit << 1);
>>> -	gpu_write(gpu, REG_A6XX_UCHE_MODE_CNTL, lower_bit << 21);
>>> +		  rgb565_predicator << 11 | hbb_hi << 10 | amsbc << 4 |
>>> +		  min_acc_len << 3 | hbb_lo << 1 | ubwc_mode);
>>> +
>>> +	gpu_write(gpu, REG_A6XX_TPL1_NC_MODE_CNTL, hbb_hi << 4 |
>>> +		  min_acc_len << 3 | hbb_lo << 1 | ubwc_mode);
>>> +
>>> +	gpu_write(gpu, REG_A6XX_SP_NC_MODE_CNTL, hbb_hi << 10 |
>>> +		  uavflagprd_inv << 4 | min_acc_len << 3 |
>>> +		  hbb_lo << 1 | ubwc_mode);
>>> +
>>> +	gpu_write(gpu, REG_A6XX_UCHE_MODE_CNTL, min_acc_len << 23 | hbb_lo << 21);
>>>  }
>>>  
>>>  static int a6xx_cp_init(struct msm_gpu *gpu)
>>>
  

On 3/1/2023 2:14 AM, Akhil P Oommen wrote:
> On 3/1/2023 2:10 AM, Konrad Dybcio wrote:
>> On 28.02.2023 21:23, Akhil P Oommen wrote:
>>> On 2/23/2023 5:36 PM, Konrad Dybcio wrote:
>>>> Rename lower_bit to hbb_lo and explain what it signifies.
>>>> Add explanations (wherever possible to other tunables).
>>>>
>>>> Sort the variable definition and assignment alphabetically.
>>> Sorting based on decreasing order of line length is more readable, isn't it?
>> I can do that.
>>
>>>> Port setting min_access_length, ubwc_mode and hbb_hi from downstream.
>>>> Set default values for all of the tunables to zero, as they should be.
>>>>
>>>> Values were validated against downstream and will be fixed up in
>>>> separate commits so as not to make this one even more messy.
>>>>
>>>> A618 remains untouched (left at hw defaults) in this patch.
>>>>
>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>> ---
>>>>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 55 ++++++++++++++++++++++++++++-------
>>>>  1 file changed, 45 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>>> index c5f5d0bb3fdc..bdae341e0a7c 100644
>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>>> @@ -786,39 +786,74 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu)
>>>>  static void a6xx_set_ubwc_config(struct msm_gpu *gpu)
>>>>  {
>>>>  	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>>> -	u32 lower_bit = 2;
>>>> +	/* Unknown, introduced with A640/680 */
>>>>  	u32 amsbc = 0;
>>>> +	/*
>>>> +	 * The Highest Bank Bit value represents the bit of the highest DDR bank.
>>>> +	 * We then subtract 13 from it (13 is the minimum value allowed by hw) and
>>>> +	 * write the lowest two bits of the remaining value as hbb_lo and the
>>>> +	 * one above it as hbb_hi to the hardware. The default values (when HBB is
>>>> +	 * not specified) are 0, 0.
>>>> +	 */
>>>> +	u32 hbb_hi = 0;
>>>> +	u32 hbb_lo = 0;
>>>> +	/* Whether the minimum access length is 64 bits */
>>>> +	u32 min_acc_len = 0;
>>>> +	/* Unknown, introduced with A650 family, related to UBWC mode/ver 4 */
>>>>  	u32 rgb565_predicator = 0;
>>>> +	/* Unknown, introduced with A650 family */
>>>>  	u32 uavflagprd_inv = 0;
>>>> +	/* Entirely magic, per-GPU-gen value */
>>>> +	u32 ubwc_mode = 0;
>>>>  
>>>>  	/* a618 is using the hw default values */
>>>>  	if (adreno_is_a618(adreno_gpu))
>>>>  		return;
>>>>  
>>>> -	if (adreno_is_a640_family(adreno_gpu))
>>>> +	if (adreno_is_a619(adreno_gpu)) {
>>>> +		/* HBB = 14 */
>>>> +		hbb_lo = 1;
>>>> +	}
>>>> +
>>>> +	if (adreno_is_a630(adreno_gpu)) {
>>>> +		/* HBB = 15 */
>>>> +		hbb_lo = 2;
>>>> +	}
>>>> +
>>>> +	if (adreno_is_a640_family(adreno_gpu)) {
>>>>  		amsbc = 1;
>>>> +		/* HBB = 15 */
>>>> +		hbb_lo = 2;
>>>> +	}
>>>>  
>>>>  	if (adreno_is_a650(adreno_gpu) || adreno_is_a660(adreno_gpu)) {
>>>> -		/* TODO: get ddr type from bootloader and use 2 for LPDDR4 */
>>>> -		lower_bit = 3;
>>>>  		amsbc = 1;
>>>> +		/* TODO: get ddr type from bootloader and use 2 for LPDDR4 */
>>>> +		/* HBB = 16 */
>>>> +		hbb_lo = 3;
>>>>  		rgb565_predicator = 1;
>>>>  		uavflagprd_inv = 2;
>>>>  	}
>>>>  
>>>>  	if (adreno_is_7c3(adreno_gpu)) {
>>>> -		lower_bit = 1;
>>>>  		amsbc = 1;
>>>> +		/* HBB is unset in downstream DTS, defaulting to 0 */
>>> This is incorrect. For 7c3 hbb value is 14. So hbb_lo should be 1. FYI, hbb configurations were moved to the driver from DT in recent downstream kernels.
>> Right, seems to have happened with msm-5.10. Though a random kernel I
>> grabbed seems to suggest it's 15 and not 14?
>>
>> https://github.com/sonyxperiadev/kernel/blob/aosp/K.P.1.0.r1/drivers/gpu/msm/adreno-gpulist.h#L1710
> We override that with 14 in a6xx_init() for LP4 platforms dynamically. Since 7c3 is only supported on LP4, we can hardcode 14 here.
> In the downstream kernel, there is an api (of_fdt_get_ddrtype()) to detect ddrtype. If we can get something like that in upstream, we should implement a similar logic here.
>
> -Akhil.
Also, I haven't closely reviewed other targets configuration you updated, but it is a good idea to leave the existing configurations here as it in this refactor patch. Any update should be a separate patch.

-Akhil.
>> Konrad
>>> -Akhil.
>>>>  		rgb565_predicator = 1;
>>>>  		uavflagprd_inv = 2;
>>>>  	}
>>>>  
>>>>  	gpu_write(gpu, REG_A6XX_RB_NC_MODE_CNTL,
>>>> -		rgb565_predicator << 11 | amsbc << 4 | lower_bit << 1);
>>>> -	gpu_write(gpu, REG_A6XX_TPL1_NC_MODE_CNTL, lower_bit << 1);
>>>> -	gpu_write(gpu, REG_A6XX_SP_NC_MODE_CNTL,
>>>> -		uavflagprd_inv << 4 | lower_bit << 1);
>>>> -	gpu_write(gpu, REG_A6XX_UCHE_MODE_CNTL, lower_bit << 21);
>>>> +		  rgb565_predicator << 11 | hbb_hi << 10 | amsbc << 4 |
>>>> +		  min_acc_len << 3 | hbb_lo << 1 | ubwc_mode);
>>>> +
>>>> +	gpu_write(gpu, REG_A6XX_TPL1_NC_MODE_CNTL, hbb_hi << 4 |
>>>> +		  min_acc_len << 3 | hbb_lo << 1 | ubwc_mode);
>>>> +
>>>> +	gpu_write(gpu, REG_A6XX_SP_NC_MODE_CNTL, hbb_hi << 10 |
>>>> +		  uavflagprd_inv << 4 | min_acc_len << 3 |
>>>> +		  hbb_lo << 1 | ubwc_mode);
>>>> +
>>>> +	gpu_write(gpu, REG_A6XX_UCHE_MODE_CNTL, min_acc_len << 23 | hbb_lo << 21);
>>>>  }
>>>>  
>>>>  static int a6xx_cp_init(struct msm_gpu *gpu)
>>>>
  

On 28.02.2023 21:48, Akhil P Oommen wrote:
> On 3/1/2023 2:14 AM, Akhil P Oommen wrote:
>> On 3/1/2023 2:10 AM, Konrad Dybcio wrote:
>>> On 28.02.2023 21:23, Akhil P Oommen wrote:
>>>> On 2/23/2023 5:36 PM, Konrad Dybcio wrote:
>>>>> Rename lower_bit to hbb_lo and explain what it signifies.
>>>>> Add explanations (wherever possible to other tunables).
>>>>>
>>>>> Sort the variable definition and assignment alphabetically.
>>>> Sorting based on decreasing order of line length is more readable, isn't it?
>>> I can do that.
>>>
>>>>> Port setting min_access_length, ubwc_mode and hbb_hi from downstream.
>>>>> Set default values for all of the tunables to zero, as they should be.
>>>>>
>>>>> Values were validated against downstream and will be fixed up in
>>>>> separate commits so as not to make this one even more messy.
>>>>>
>>>>> A618 remains untouched (left at hw defaults) in this patch.
>>>>>
>>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>>> ---
>>>>>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 55 ++++++++++++++++++++++++++++-------
>>>>>  1 file changed, 45 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>>>> index c5f5d0bb3fdc..bdae341e0a7c 100644
>>>>> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>>>> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
>>>>> @@ -786,39 +786,74 @@ static void a6xx_set_cp_protect(struct msm_gpu *gpu)
>>>>>  static void a6xx_set_ubwc_config(struct msm_gpu *gpu)
>>>>>  {
>>>>>  	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
>>>>> -	u32 lower_bit = 2;
>>>>> +	/* Unknown, introduced with A640/680 */
>>>>>  	u32 amsbc = 0;
>>>>> +	/*
>>>>> +	 * The Highest Bank Bit value represents the bit of the highest DDR bank.
>>>>> +	 * We then subtract 13 from it (13 is the minimum value allowed by hw) and
>>>>> +	 * write the lowest two bits of the remaining value as hbb_lo and the
>>>>> +	 * one above it as hbb_hi to the hardware. The default values (when HBB is
>>>>> +	 * not specified) are 0, 0.
>>>>> +	 */
>>>>> +	u32 hbb_hi = 0;
>>>>> +	u32 hbb_lo = 0;
>>>>> +	/* Whether the minimum access length is 64 bits */
>>>>> +	u32 min_acc_len = 0;
>>>>> +	/* Unknown, introduced with A650 family, related to UBWC mode/ver 4 */
>>>>>  	u32 rgb565_predicator = 0;
>>>>> +	/* Unknown, introduced with A650 family */
>>>>>  	u32 uavflagprd_inv = 0;
>>>>> +	/* Entirely magic, per-GPU-gen value */
>>>>> +	u32 ubwc_mode = 0;
>>>>>  
>>>>>  	/* a618 is using the hw default values */
>>>>>  	if (adreno_is_a618(adreno_gpu))
>>>>>  		return;
>>>>>  
>>>>> -	if (adreno_is_a640_family(adreno_gpu))
>>>>> +	if (adreno_is_a619(adreno_gpu)) {
>>>>> +		/* HBB = 14 */
>>>>> +		hbb_lo = 1;
>>>>> +	}
>>>>> +
>>>>> +	if (adreno_is_a630(adreno_gpu)) {
>>>>> +		/* HBB = 15 */
>>>>> +		hbb_lo = 2;
>>>>> +	}
>>>>> +
>>>>> +	if (adreno_is_a640_family(adreno_gpu)) {
>>>>>  		amsbc = 1;
>>>>> +		/* HBB = 15 */
>>>>> +		hbb_lo = 2;
>>>>> +	}
>>>>>  
>>>>>  	if (adreno_is_a650(adreno_gpu) || adreno_is_a660(adreno_gpu)) {
>>>>> -		/* TODO: get ddr type from bootloader and use 2 for LPDDR4 */
>>>>> -		lower_bit = 3;
>>>>>  		amsbc = 1;
>>>>> +		/* TODO: get ddr type from bootloader and use 2 for LPDDR4 */
>>>>> +		/* HBB = 16 */
>>>>> +		hbb_lo = 3;
>>>>>  		rgb565_predicator = 1;
>>>>>  		uavflagprd_inv = 2;
>>>>>  	}
>>>>>  
>>>>>  	if (adreno_is_7c3(adreno_gpu)) {
>>>>> -		lower_bit = 1;
>>>>>  		amsbc = 1;
>>>>> +		/* HBB is unset in downstream DTS, defaulting to 0 */
>>>> This is incorrect. For 7c3 hbb value is 14. So hbb_lo should be 1. FYI, hbb configurations were moved to the driver from DT in recent downstream kernels.
>>> Right, seems to have happened with msm-5.10. Though a random kernel I
>>> grabbed seems to suggest it's 15 and not 14?
>>>
>>> https://github.com/sonyxperiadev/kernel/blob/aosp/K.P.1.0.r1/drivers/gpu/msm/adreno-gpulist.h#L1710
>> We override that with 14 in a6xx_init() for LP4 platforms dynamically. Since 7c3 is only supported on LP4, we can hardcode 14 here.
Okay, I see.

>> In the downstream kernel, there is an api (of_fdt_get_ddrtype()) to detect ddrtype. If we can get something like that in upstream, we should implement a similar logic here.
Yeah, I mentioned it here [1], but I doubt it'd be implemented,
given what Krzysztof pointed out.

>>
>> -Akhil.
> Also, I haven't closely reviewed other targets configuration you updated, but it is a good idea to leave the existing configurations here as it in this refactor patch. Any update should be a separate patch.
Sure, will do.

Konrad

[1] https://github.com/devicetree-org/devicetree-specification/issues/62
> 
> -Akhil.
>>> Konrad
>>>> -Akhil.
>>>>>  		rgb565_predicator = 1;
>>>>>  		uavflagprd_inv = 2;
>>>>>  	}
>>>>>  
>>>>>  	gpu_write(gpu, REG_A6XX_RB_NC_MODE_CNTL,
>>>>> -		rgb565_predicator << 11 | amsbc << 4 | lower_bit << 1);
>>>>> -	gpu_write(gpu, REG_A6XX_TPL1_NC_MODE_CNTL, lower_bit << 1);
>>>>> -	gpu_write(gpu, REG_A6XX_SP_NC_MODE_CNTL,
>>>>> -		uavflagprd_inv << 4 | lower_bit << 1);
>>>>> -	gpu_write(gpu, REG_A6XX_UCHE_MODE_CNTL, lower_bit << 21);
>>>>> +		  rgb565_predicator << 11 | hbb_hi << 10 | amsbc << 4 |
>>>>> +		  min_acc_len << 3 | hbb_lo << 1 | ubwc_mode);
>>>>> +
>>>>> +	gpu_write(gpu, REG_A6XX_TPL1_NC_MODE_CNTL, hbb_hi << 4 |
>>>>> +		  min_acc_len << 3 | hbb_lo << 1 | ubwc_mode);
>>>>> +
>>>>> +	gpu_write(gpu, REG_A6XX_SP_NC_MODE_CNTL, hbb_hi << 10 |
>>>>> +		  uavflagprd_inv << 4 | min_acc_len << 3 |
>>>>> +		  hbb_lo << 1 | ubwc_mode);
>>>>> +
>>>>> +	gpu_write(gpu, REG_A6XX_UCHE_MODE_CNTL, min_acc_len << 23 | hbb_lo << 21);
>>>>>  }
>>>>>  
>>>>>  static int a6xx_cp_init(struct msm_gpu *gpu)
>>>>>
>