On Tue, 4 Jun 2024 at 18:48, Rob Clark <robdclark@xxxxxxxxx> wrote:
>
> From: Rob Clark <robdclark@xxxxxxxxxxxx>
>
> Add the SQE fw version to dmesg and devcoredump.
>
> Signed-off-by: Rob Clark <robdclark@xxxxxxxxxxxx>
> ---
> drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 32 +++++++++++++++++++--
> drivers/gpu/drm/msm/adreno/a6xx_gpu.h | 1 +
> drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 2 ++
> 3 files changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> index 56bfb228808d..5a2a005003c8 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
> @@ -665,6 +665,32 @@ static int a7xx_cp_init(struct msm_gpu *gpu)
> return a6xx_idle(gpu, ring) ? 0 : -EINVAL;
> }
>
> +static uint32_t get_ucode_version(const uint32_t *data)
> +{
> + uint32_t version;
> +
> + /* NOTE: compared to kgsl, we've already stripped off the first dword: */
> + version = data[0];
> +
> + if ((version & 0xf) != 0xa)
> + return version;
> +
> + version &= ~0xfff;
> + return version | ((data[2] & 0xfff000) >> 12);
> +}
> +
> +uint32_t a6xx_get_sqe_version(struct msm_gpu *gpu)
> +{
> + struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> + struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
> + uint32_t *buf = msm_gem_get_vaddr(a6xx_gpu->sqe_bo);
> + uint32_t version = get_ucode_version(buf);
> +
> + msm_gem_put_vaddr(a6xx_gpu->sqe_bo);
Wouldn't it be easier to save the SQE version when the firmware is
loaded and then just return it?
Or the point is about reading the SQE version at runtime?
> +
> + return version;
> +}
> +
> /*
> * Check that the microcode version is new enough to include several key
> * security fixes. Return true if the ucode is safe.
> @@ -681,6 +707,8 @@ static bool a6xx_ucode_check_version(struct a6xx_gpu *a6xx_gpu,
> if (IS_ERR(buf))
> return false;
>
> + DRM_DEV_INFO(&gpu->pdev->dev, "Have SQE version %03x\n", get_ucode_version(buf));
I'd suggest drm_dbg_driver() instead. I think the motto is to keep the
kernel more or less quiet.
> +
> /* A7xx is safe! */
> if (adreno_is_a7xx(adreno_gpu) || adreno_is_a702(adreno_gpu))
> return true;
> @@ -714,7 +742,7 @@ static bool a6xx_ucode_check_version(struct a6xx_gpu *a6xx_gpu,
> }
>
> DRM_DEV_ERROR(&gpu->pdev->dev,
> - "a630 SQE ucode is too old. Have version %x need at least %x\n",
> + "a630 SQE ucode is too old. Have version %03x need at least %03x\n",
> buf[0] & 0xfff, 0x190);
> } else if (!strcmp(sqe_name, "a650_sqe.fw")) {
> if ((buf[0] & 0xfff) >= 0x095) {
> @@ -723,7 +751,7 @@ static bool a6xx_ucode_check_version(struct a6xx_gpu *a6xx_gpu,
> }
>
> DRM_DEV_ERROR(&gpu->pdev->dev,
> - "a650 SQE ucode is too old. Have version %x need at least %x\n",
> + "a650 SQE ucode is too old. Have version %03x need at least %03x\n",
> buf[0] & 0xfff, 0x095);
> } else if (!strcmp(sqe_name, "a660_sqe.fw")) {
> ret = true;
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> index 1c3cc6df70fe..c206dab8bc08 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> @@ -109,6 +109,7 @@ void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp,
> bool suspended);
> unsigned long a6xx_gmu_get_freq(struct msm_gpu *gpu);
>
> +uint32_t a6xx_get_sqe_version(struct msm_gpu *gpu);
> void a6xx_show(struct msm_gpu *gpu, struct msm_gpu_state *state,
> struct drm_printer *p);
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> index 0a7717a4fc2f..1acfe39eb8e0 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> @@ -1957,6 +1957,8 @@ void a6xx_show(struct msm_gpu *gpu, struct msm_gpu_state *state,
>
> adreno_show(gpu, state, p);
>
> + drm_printf(p, "sqe-version: 0x%08x\n", a6xx_get_sqe_version(gpu));
> +
> drm_puts(p, "gmu-log:\n");
> if (a6xx_state->gmu_log) {
> struct msm_gpu_state_bo *gmu_log = a6xx_state->gmu_log;
> --
> 2.45.1
>
--
With best wishes
Dmitry
