| Message ID | 20250508-topic-ubwc_central-v1-10-035c4c5cbe50@oss.qualcomm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add a single source of truth for UBWC configuration data | expand |
On 5/8/25 8:33 PM, Connor Abbott wrote: > On Thu, May 8, 2025 at 2:14 PM Konrad Dybcio <konradybcio@kernel.org> wrote: >> >> From: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com> >> >> SC8180X (A680) and SA8775P (A663) require a write to that register, >> while other SKUs are fine with the default value. Don't overwrite it >> needlessly, requiring the developer to read the value back from >> hardware just to put it in the driver again, introducing much more room >> for error. > > I'm not sure I understand that last sentence. The original reason I > always wrote it was that for host image copy we need to know the value > of macrotile_mode, so again the value exposed to userspace must match > what's set in the HW. We can't read the value from the HW and send it > to userspace, because userspace queries this when creating the > physical device during device enumeration and we really don't want to > spuriously turn on the device then. That means the safest thing is to > always program it, guaranteeing that it always matches. Otherwise we > just have to hope that the default value matches what we expect it to > be. > > I know you're copying this from kgsl, but kgsl doesn't expose the > macrotile_mode to userspace. I expect that HIC was added afterwards > and only works via hacks there (if it's even supported at all on the > relevant SoCs). Alright, I think I'll include it in the common UBWC config (even though it only concerns the GPU), as IIUC it may differ between platforms implementing the same GPU SKU Konrad
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c index 60f89a2d851a5c383fc14cce4c483f630132a9a6..bee7e9685aa3ea282fb20ef479e4d243d28418f7 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c @@ -594,7 +594,6 @@ static int a6xx_calc_ubwc_config(struct adreno_gpu *gpu) gpu->ubwc_config.min_acc_len = 0; gpu->ubwc_config.ubwc_swizzle = 0x6; - gpu->ubwc_config.macrotile_mode = 0; gpu->ubwc_config.highest_bank_bit = 2; if (adreno_is_a610(gpu)) { @@ -616,13 +615,8 @@ static int a6xx_calc_ubwc_config(struct adreno_gpu *gpu) if (adreno_is_a621(gpu)) gpu->ubwc_config.highest_bank_bit = 0; - if (adreno_is_a623(gpu)) { + if (adreno_is_a623(gpu)) gpu->ubwc_config.highest_bank_bit = 3; - gpu->ubwc_config.macrotile_mode = 1; - } - - if (adreno_is_a680(gpu)) - gpu->ubwc_config.macrotile_mode = 1; if (adreno_is_a650(gpu) || adreno_is_a660(gpu) || @@ -631,19 +625,15 @@ static int a6xx_calc_ubwc_config(struct adreno_gpu *gpu) adreno_is_a740_family(gpu)) { /* TODO: get ddr type from bootloader and use 2 for LPDDR4 */ gpu->ubwc_config.highest_bank_bit = 3; - gpu->ubwc_config.macrotile_mode = 1; } if (adreno_is_a663(gpu)) { gpu->ubwc_config.highest_bank_bit = 0; - gpu->ubwc_config.macrotile_mode = 1; gpu->ubwc_config.ubwc_swizzle = 0x4; } - if (adreno_is_7c3(gpu)) { + if (adreno_is_7c3(gpu)) gpu->ubwc_config.highest_bank_bit = 1; - gpu->ubwc_config.macrotile_mode = 1; - } if (adreno_is_a702(gpu)) { gpu->ubwc_config.highest_bank_bit = 1; @@ -691,8 +681,9 @@ static void a6xx_set_ubwc_config(struct msm_gpu *gpu) gpu_write(gpu, REG_A6XX_UCHE_MODE_CNTL, adreno_gpu->ubwc_config.min_acc_len << 23 | hbb_lo << 21); - gpu_write(gpu, REG_A6XX_RBBM_NC_MODE_CNTL, - adreno_gpu->ubwc_config.macrotile_mode); + /* The reset value only needs altering in some cases */ + if (adreno_is_a680(adreno_gpu) || adreno_is_a663(adreno_gpu)) + gpu_write(gpu, REG_A6XX_RBBM_NC_MODE_CNTL, BIT(0)); } static void a7xx_patch_pwrup_reglist(struct msm_gpu *gpu)