| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 23 Feb 2023 00:38:26 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Konrad Dybcio <konrad.dybcio@...aro.org>,
Rob Clark <robdclark@...il.com>,
Abhinav Kumar <quic_abhinavk@...cinc.com>,
Sean Paul <sean@...rly.run>, David Airlie <airlied@...il.com>,
Daniel Vetter <daniel@...ll.ch>
Cc: linux-arm-msm@...r.kernel.org, dri-devel@...ts.freedesktop.org,
freedreno@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/5] drm/msm/adreno: Use OPP for every GPU generation
On 22/02/2023 23:47, Konrad Dybcio wrote:
> Some older GPUs (namely a2xx with no opp tables at all and a320 with
> downstream-remnants gpu pwrlevels) used not to have OPP tables. They
> both however had just one frequency defined, making it extremely easy
> to construct such an OPP table from within the driver if need be.
>
> Do so and switch all clk_set_rate calls on core_clk to their OPP
> counterparts.
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@...aro.org>
> ---
> drivers/gpu/drm/msm/adreno/adreno_gpu.c | 94 +++++++++++++++------------------
> drivers/gpu/drm/msm/msm_gpu.c | 4 +-
> drivers/gpu/drm/msm/msm_gpu_devfreq.c | 2 +-
> 3 files changed, 45 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> index ce6b76c45b6f..9b940c0f063f 100644
> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
> @@ -922,73 +922,50 @@ void adreno_wait_ring(struct msm_ringbuffer *ring, uint32_t ndwords)
> ring->id);
> }
>
> -/* Get legacy powerlevels from qcom,gpu-pwrlevels and populate the opp table */
> -static int adreno_get_legacy_pwrlevels(struct device *dev)
> -{
> - struct device_node *child, *node;
> - int ret;
> -
> - node = of_get_compatible_child(dev->of_node, "qcom,gpu-pwrlevels");
> - if (!node) {
> - DRM_DEV_DEBUG(dev, "Could not find the GPU powerlevels\n");
> - return -ENXIO;
> - }
> -
> - for_each_child_of_node(node, child) {
> - unsigned int val;
> -
> - ret = of_property_read_u32(child, "qcom,gpu-freq", &val);
> - if (ret)
> - continue;
> -
> - /*
> - * Skip the intentionally bogus clock value found at the bottom
> - * of most legacy frequency tables
> - */
> - if (val != 27000000)
> - dev_pm_opp_add(dev, val, 0);
> - }
> -
> - of_node_put(node);
> -
> - return 0;
> -}
> -
> -static void adreno_get_pwrlevels(struct device *dev,
> +static int adreno_get_pwrlevels(struct device *dev,
> struct msm_gpu *gpu)
> {
> + struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
> unsigned long freq = ULONG_MAX;
> struct dev_pm_opp *opp;
> int ret;
>
> gpu->fast_rate = 0;
>
> - /* You down with OPP? */
> - if (!of_find_property(dev->of_node, "operating-points-v2", NULL))
> - ret = adreno_get_legacy_pwrlevels(dev);
> - else {
> - ret = devm_pm_opp_of_add_table(dev);
> - if (ret)
> - DRM_DEV_ERROR(dev, "Unable to set the OPP table\n");
> - }
> -
> - if (!ret) {
> + /* devm_pm_opp_of_add_table may error out but will still create an OPP table */
> + ret = devm_pm_opp_of_add_table(dev);
> + if (ret == -ENODEV) {
> + /* Special cases for ancient hw with ancient DT bindings */
> + if (adreno_is_a2xx(adreno_gpu)) {
> + dev_warn(dev, "Unable to find the OPP table. Falling back to 200 MHz.\n");
> + dev_pm_opp_add(dev, 200000000, 0);
> + gpu->fast_rate = 200000000;
We can skip setting the fast_rate, dev_pm_opp_find_freq_floor below will
get it from our freshly generated opp table.
> + } else if (adreno_is_a320(adreno_gpu)) {
> + dev_warn(dev, "Unable to find the OPP table. Falling back to 450 MHz.\n");
> + dev_pm_opp_add(dev, 450000000, 0);
> + gpu->fast_rate = 450000000;
> + } else {
> + DRM_DEV_ERROR(dev, "Unable to find the OPP table\n");
> + return -ENODEV;
> + }
> + } else if (ret) {
> + DRM_DEV_ERROR(dev, "Unable to set the OPP table\n");
> + return ret;
> + } else {
> /* Find the fastest defined rate */
> opp = dev_pm_opp_find_freq_floor(dev, &freq);
> - if (!IS_ERR(opp)) {
> +
> + if (IS_ERR(opp))
> + return PTR_ERR(opp);
> + else {
> gpu->fast_rate = freq;
> dev_pm_opp_put(opp);
> }
> }
>
> - if (!gpu->fast_rate) {
> - dev_warn(dev,
> - "Could not find a clock rate. Using a reasonable default\n");
> - /* Pick a suitably safe clock speed for any target */
> - gpu->fast_rate = 200000000;
> - }
> -
> DBG("fast_rate=%u, slow_rate=27000000", gpu->fast_rate);
> +
> + return 0;
> }
>
> int adreno_gpu_ocmem_init(struct device *dev, struct adreno_gpu *adreno_gpu,
> @@ -1046,6 +1023,17 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
> struct adreno_rev *rev = &config->rev;
> const char *gpu_name;
> u32 speedbin;
> + int ret;
> +
> + /* This can only be done here, or devm_pm_opp_set_supported_hw will WARN_ON() */
I'd rephrase this to '...done before devm_pm_opp_of_add_table(), or
dev_pm_opp_set_config() will...'. It took me a while to find correct
limitation.
I wanted to move the code below to msm_gpu_init(), but after digging in
found that it's not possible.
> + if (IS_ERR(devm_clk_get(dev, "core"))) {
> + /*
> + * If "core" is absent, go for the legacy clock name.
> + * If we got this far in probing, it's a given one of them exists.
> + */
> + devm_pm_opp_set_clkname(dev, "core_clk");
> + } else
> + devm_pm_opp_set_clkname(dev, "core");
>
> adreno_gpu->funcs = funcs;
> adreno_gpu->info = adreno_info(config->rev);
> @@ -1070,7 +1058,9 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>
> adreno_gpu_config.nr_rings = nr_rings;
>
> - adreno_get_pwrlevels(dev, gpu);
> + ret = adreno_get_pwrlevels(dev, gpu);
> + if (ret)
> + return ret;
>
> pm_runtime_set_autosuspend_delay(dev,
> adreno_gpu->info->inactive_period);
> diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
> index 380249500325..cdcb00df3f25 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.c
> +++ b/drivers/gpu/drm/msm/msm_gpu.c
> @@ -59,7 +59,7 @@ static int disable_pwrrail(struct msm_gpu *gpu)
> static int enable_clk(struct msm_gpu *gpu)
> {
> if (gpu->core_clk && gpu->fast_rate)
> - clk_set_rate(gpu->core_clk, gpu->fast_rate);
> + dev_pm_opp_set_rate(&gpu->pdev->dev, gpu->fast_rate);
>
> /* Set the RBBM timer rate to 19.2Mhz */
> if (gpu->rbbmtimer_clk)
> @@ -78,7 +78,7 @@ static int disable_clk(struct msm_gpu *gpu)
> * will be rounded down to zero anyway so it all works out.
> */
> if (gpu->core_clk)
> - clk_set_rate(gpu->core_clk, 27000000);
> + dev_pm_opp_set_rate(&gpu->pdev->dev, 27000000);
>
> if (gpu->rbbmtimer_clk)
> clk_set_rate(gpu->rbbmtimer_clk, 0);
> diff --git a/drivers/gpu/drm/msm/msm_gpu_devfreq.c b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> index e27dbf12b5e8..ea70c1c32d94 100644
> --- a/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> +++ b/drivers/gpu/drm/msm/msm_gpu_devfreq.c
> @@ -48,7 +48,7 @@ static int msm_devfreq_target(struct device *dev, unsigned long *freq,
> gpu->funcs->gpu_set_freq(gpu, opp, df->suspended);
> mutex_unlock(&df->lock);
> } else {
> - clk_set_rate(gpu->core_clk, *freq);
> + dev_pm_opp_set_rate(dev, *freq);
> }
>
> dev_pm_opp_put(opp);
>
--
With best wishes
Dmitry
Powered by blists - more mailing lists