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