Re: [PATCH 1/5] drm/msm/adreno: Use OPP for every GPU generation

[Konrad Dybcio]

On 22.02.2023 23:38, Dmitry Baryshkov wrote:
> 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@xxxxxxxxxx>
>> ---
>>   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.
It's not reached in this code path.

> 
>> +        } 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.
Ack, I wasted some time on this too..

Konrad
> 
> 
>> +    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);
>>
>