Re: [PATCH v3 05/15] drm/msm/a6xx: Introduce GMU wrapper support

[Dmitry Baryshkov]

On Thu, 23 Feb 2023 at 14:06, Konrad Dybcio <konrad.dybcio@xxxxxxxxxx> wrote:
>
> Some (particularly SMD_RPM, a.k.a non-RPMh) SoCs implement A6XX GPUs
> but don't implement the associated GMUs. This is due to the fact that
> the GMU directly pokes at RPMh. Sadly, this means we have to take care
> of enabling & scaling power rails, clocks and bandwidth ourselves.
>
> Reuse existing Adreno-common code and modify the deeply-GMU-infused
> A6XX code to facilitate these GPUs. This involves if-ing out lots
> of GMU callbacks and introducing a new type of GMU - GMU wrapper (it's
> the actual name that Qualcomm uses in their downstream kernels).
>
> This is essentially a register region which is convenient to model
> as a device. We'll use it for managing the GDSCs. The register
> layout matches the actual GMU_CX/GX regions on the "real GMU" devices
> and lets us reuse quite a bit of gmu_read/write/rmw calls.
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>
> ---
>  drivers/gpu/drm/msm/adreno/a6xx_gmu.c       |  53 +++++-
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c       | 244 +++++++++++++++++++++++++---
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.h       |   1 +
>  drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c |  14 +-
>  drivers/gpu/drm/msm/adreno/adreno_gpu.h     |   6 +
>  5 files changed, 282 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> index 90e636dcdd5b..b2c56561cde6 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c

[skipped]

>  struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
>  {
>         struct msm_drm_private *priv = dev->dev_private;
> @@ -2063,18 +2235,36 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
>
>         adreno_gpu->registers = NULL;
>
> +       /* Check if there is a GMU phandle and set it up */
> +       node = of_parse_phandle(pdev->dev.of_node, "qcom,gmu", 0);
> +       /* FIXME: How do we gracefully handle this? */
> +       BUG_ON(!node);

I thought that we should fix it, but then I noticed that this code was
just moved from the part below.

> +
> +       adreno_gpu->gmu_is_wrapper = of_device_is_compatible(node, "qcom,adreno-gmu-wrapper");
> +
>         /*
>          * We need to know the platform type before calling into adreno_gpu_init
>          * so that the hw_apriv flag can be correctly set. Snoop into the info
>          * and grab the revision number
>          */
>         info = adreno_info(config->rev);
> -
> -       if (info && (info->revn == 650 || info->revn == 660 ||
> -                       adreno_cmp_rev(ADRENO_REV(6, 3, 5, ANY_ID), info->rev)))

Are we losing A635 here? I don't see it in the condition below.

> +       if (!info)
> +               return ERR_PTR(-EINVAL);
> +
> +       /* Assign these early so that we can use the is_aXYZ helpers */
> +       /* Numeric revision IDs (e.g. 630) */
> +       adreno_gpu->revn = info->revn;
> +       /* New-style ADRENO_REV()-only */
> +       adreno_gpu->rev = info->rev;
> +       /* Quirk data */
> +       adreno_gpu->info = info;
> +
> +       if (adreno_is_a650(adreno_gpu) || adreno_is_a660_family(adreno_gpu))
>                 adreno_gpu->base.hw_apriv = true;
>
> -       a6xx_llc_slices_init(pdev, a6xx_gpu);
> +       /* No LLCC on non-RPMh (and by extension, non-GMU) SoCs */
> +       if (!adreno_has_gmu_wrapper(adreno_gpu))
> +               a6xx_llc_slices_init(pdev, a6xx_gpu);
>
>         ret = a6xx_set_supported_hw(&pdev->dev, config->rev);
>         if (ret) {
> @@ -2082,7 +2272,10 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
>                 return ERR_PTR(ret);
>         }
>
> -       ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, 1);
> +       if (adreno_has_gmu_wrapper(adreno_gpu))
> +               ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs_gmuwrapper, 1);
> +       else
> +               ret = adreno_gpu_init(dev, pdev, adreno_gpu, &funcs, 1);
>         if (ret) {
>                 a6xx_destroy(&(a6xx_gpu->base.base));
>                 return ERR_PTR(ret);
> @@ -2095,13 +2288,10 @@ struct msm_gpu *a6xx_gpu_init(struct drm_device *dev)
>         if (adreno_is_a618(adreno_gpu) || adreno_is_7c3(adreno_gpu))
>                 priv->gpu_clamp_to_idle = true;
>
> -       /* Check if there is a GMU phandle and set it up */
> -       node = of_parse_phandle(pdev->dev.of_node, "qcom,gmu", 0);
> -
> -       /* FIXME: How do we gracefully handle this? */
> -       BUG_ON(!node);
> -
> -       ret = a6xx_gmu_init(a6xx_gpu, node);
> +       if (adreno_has_gmu_wrapper(adreno_gpu))
> +               ret = a6xx_gmu_wrapper_init(a6xx_gpu, node);
> +       else
> +               ret = a6xx_gmu_init(a6xx_gpu, node);
>         of_node_put(node);
>         if (ret) {
>                 a6xx_destroy(&(a6xx_gpu->base.base));
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> index eea2e60ce3b7..51a7656072fa 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.h
> @@ -76,6 +76,7 @@ int a6xx_gmu_set_oob(struct a6xx_gmu *gmu, enum a6xx_gmu_oob_state state);
>  void a6xx_gmu_clear_oob(struct a6xx_gmu *gmu, enum a6xx_gmu_oob_state state);
>
>  int a6xx_gmu_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node);
> +int a6xx_gmu_wrapper_init(struct a6xx_gpu *a6xx_gpu, struct device_node *node);
>  void a6xx_gmu_remove(struct a6xx_gpu *a6xx_gpu);
>
>  void a6xx_gmu_set_freq(struct msm_gpu *gpu, struct dev_pm_opp *opp,
> diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> index b7e217d00a22..e11e8a02ac22 100644
> --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c
> @@ -1041,16 +1041,18 @@ struct msm_gpu_state *a6xx_gpu_state_get(struct msm_gpu *gpu)
>         /* Get the generic state from the adreno core */
>         adreno_gpu_state_get(gpu, &a6xx_state->base);
>
> -       a6xx_get_gmu_registers(gpu, a6xx_state);
> +       if (!adreno_has_gmu_wrapper(adreno_gpu)) {
> +               a6xx_get_gmu_registers(gpu, a6xx_state);
>
> -       a6xx_state->gmu_log = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.log);
> -       a6xx_state->gmu_hfi = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.hfi);
> -       a6xx_state->gmu_debug = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.debug);
> +               a6xx_state->gmu_log = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.log);
> +               a6xx_state->gmu_hfi = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.hfi);
> +               a6xx_state->gmu_debug = a6xx_snapshot_gmu_bo(a6xx_state, &a6xx_gpu->gmu.debug);
>
> -       a6xx_snapshot_gmu_hfi_history(gpu, a6xx_state);
> +               a6xx_snapshot_gmu_hfi_history(gpu, a6xx_state);
> +       }
>
>         /* If GX isn't on the rest of the data isn't going to be accessible */
> -       if (!a6xx_gmu_gx_is_on(&a6xx_gpu->gmu))
> +       if (!adreno_has_gmu_wrapper(adreno_gpu) && !a6xx_gmu_gx_is_on(&a6xx_gpu->gmu))
>                 return &a6xx_state->base;
>
>         /* Get the banks of indexed registers */
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.h b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> index b4f9b1343d63..2c0f0ef094cb 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.h
> @@ -115,6 +115,7 @@ struct adreno_gpu {
>          * code (a3xx_gpu.c) and stored in this common location.
>          */
>         const unsigned int *reg_offsets;
> +       bool gmu_is_wrapper;
>  };
>  #define to_adreno_gpu(x) container_of(x, struct adreno_gpu, base)
>
> @@ -145,6 +146,11 @@ struct adreno_platform_config {
>
>  bool adreno_cmp_rev(struct adreno_rev rev1, struct adreno_rev rev2);
>
> +static inline bool adreno_has_gmu_wrapper(struct adreno_gpu *gpu)
> +{
> +       return gpu->gmu_is_wrapper;
> +}
> +
>  static inline bool adreno_is_a2xx(struct adreno_gpu *gpu)
>  {
>         return (gpu->revn < 300);
>
> --
> 2.39.2
>


-- 
With best wishes
Dmitry