Re: [PATCH 3/5] drm: msm: add support for A750 GPU
From: neil . armstrong
Date: Thu Feb 15 2024 - 04:13:02 EST
On 14/02/2024 22:43, Konrad Dybcio wrote:
On 12.02.2024 15:45, Neil Armstrong wrote:
On 12/02/2024 11:46, Konrad Dybcio wrote:
On 12.02.2024 11:37, Neil Armstrong wrote:
Add support for the A750 GPU found on the SM8650 platform
Unlike the the very close A740 GPU on the SM8550 SoC, the A750 GPU
doesn't have an HWCFG block but a separate register set.
The missing registers are added in the a6xx.xml.h file that would
require a subsequent sync and the non-existent hwcfg is handled
in a6xx_set_hwcg().
These should also be submitted to mesa to make sure the next header sync
doesn't wipe them
Ack submitting them right now: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/27576
Thanks
[...]
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -958,10 +958,11 @@ static void a6xx_set_hwcg(struct msm_gpu *gpu, bool state)
struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
struct a6xx_gmu *gmu = &a6xx_gpu->gmu;
const struct adreno_reglist *reg;
+ bool skip_programming = !(adreno_gpu->info->hwcg || adreno_is_a7xx(adreno_gpu));
is_a750?
OK right, I was thinking of the next gpu which will probably also miss an hwcfg
unsigned int i;
u32 val, clock_cntl_on, cgc_mode;
- if (!adreno_gpu->info->hwcg)
+ if (skip_programming)
return;
if (adreno_is_a630(adreno_gpu))
@@ -982,6 +983,25 @@ static void a6xx_set_hwcg(struct msm_gpu *gpu, bool state)
state ? 0x5555 : 0);
}
+ if (!adreno_gpu->info->hwcg) {
I don't think this block of code is reachable now, no?
It is because we didn't skip when adreno_is_a7xx(adreno_gpu)
Ahh I misread the brackets within the assignment
Maybe remove the skip_programming and if_a750 here?
This would require:
- if (!adreno_gpu->info->hwcg || )
+ if (!(adreno_gpu->info->hwcg || adreno_is_a750(adreno_gpu)))
and:
+ if (adreno_is_a750(adreno_gpu)) {
But if the next gpu also doesn't have an hwcfg, we will need to use
the current design...
I just tried with:
====================><===============================
@@ -961,7 +961,7 @@ static void a6xx_set_hwcg(struct msm_gpu *gpu, bool state)
unsigned int i;
u32 val, clock_cntl_on, cgc_mode;
- if (!adreno_gpu->info->hwcg)
+ if (!(adreno_gpu->info->hwcg || adreno_is_a750(adreno_gpu)))
return;
if (adreno_is_a630(adreno_gpu))
@@ -982,6 +982,25 @@ static void a6xx_set_hwcg(struct msm_gpu *gpu, bool state)
state ? 0x5555 : 0);
}
+ if (adreno_is_a750(adreno_gpu)) {
+ gpu_write(gpu, REG_A7XX_RBBM_CLOCK_CNTL_GLOBAL, 1);
+ gpu_write(gpu, REG_A7XX_RBBM_CGC_GLOBAL_LOAD_CMD, state ? 1 : 0);
+
+ if (state) {
+ gpu_write(gpu, REG_A7XX_RBBM_CGC_P2S_TRIG_CMD, 1);
+
+ if (gpu_poll_timeout(gpu, REG_A7XX_RBBM_CGC_P2S_STATUS, val,
+ val & A7XX_RBBM_CGC_P2S_STATUS_TXDONE, 1, 10)) {
+ dev_err(&gpu->pdev->dev, "RBBM_CGC_P2S_STATUS TXDONE Poll failed\n");
+ return;
+ }
+
+ gpu_write(gpu, REG_A7XX_RBBM_CLOCK_CNTL_GLOBAL, 0);
+ }
+
+ return;
+ }
+
val = gpu_read(gpu, REG_A6XX_RBBM_CLOCK_CNTL);
/* Don't re-program the registers if they are already correct */
====================><===============================
And it works fine, does it work it for you ?
Let's keep it as-is in the original submission, as I've mentioned, I had
misread the code
Ack thanks
Neil
Konrad
+ gpu_write(gpu, REG_A7XX_RBBM_CLOCK_CNTL_GLOBAL, 1);
+ gpu_write(gpu, REG_A7XX_RBBM_CGC_GLOBAL_LOAD_CMD, state ? 1 : 0);
+
+ if (state) {
+ gpu_write(gpu, REG_A7XX_RBBM_CGC_P2S_TRIG_CMD, 1);
+
+ if (gpu_poll_timeout(gpu, REG_A7XX_RBBM_CGC_P2S_STATUS, val,
+ val & BIT(0), 1, 10)) {
We should define that bit name (the err suggests it's
REG_A7XX_RBBM_GCC_P2S_STATUS_TXDONE or so)
[...]
+static inline int adreno_is_a750(struct adreno_gpu *gpu)
+{
+ return gpu->info->chip_ids[0] == 0x43051401;
+}
+
/* Placeholder to make future diffs smaller */
Please also remove this comment now that it's invalid
Ack
Konrad
Thanks,
Neil