[PATCH v2 06/14] drm/msm/gpu: Use dev_pm_opp_set_rate for non-GMU GPUs

[Dmitry Baryshkov]

On 18/02/2023 13:04, Konrad Dybcio wrote:
> 
> 
> On 17.02.2023 22:07, Dmitry Baryshkov wrote:
>> On 14/02/2023 19:31, Konrad Dybcio wrote:
>>> Currently we only utilize the OPP table connected to the GPU for
>>> getting (available) frequencies. We do however need to scale the
>>> voltage rail(s) accordingly to ensure that we aren't trying to
>>> run the GPU at 1GHz with a VDD_LOW vote, as that would result in
>>> an otherwise inexplainable hang.
>>>
>>> Tell the OPP framework that we want to scale the "core" clock
>>> and swap out the clk_set_rate to a dev_pm_opp_set_rate in
>>> msm_devfreq_target() to enable usage of required-opps and by
>>> extension proper voltage level/corner scaling.
>>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio at linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/adreno/adreno_gpu.c | 4 ++++
>>>    drivers/gpu/drm/msm/msm_gpu_devfreq.c   | 2 +-
>>>    2 files changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>> index ce6b76c45b6f..15e405e4f977 100644
>>> --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>> +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c
>>> @@ -1047,6 +1047,10 @@ int adreno_gpu_init(struct drm_device *drm, struct platform_device *pdev,
>>>        const char *gpu_name;
>>>        u32 speedbin;
>>>    +    /* This can only be done here, or devm_pm_opp_set_supported_hw will WARN_ON() */
>>> +    if (!IS_ERR(devm_clk_get(dev, "core")))
>>> +        devm_pm_opp_set_clkname(dev, "core");
>>
>> Can we instead move a call to a6xx_set_supported_hw() / check_speed_bin after the adreno_gpu_init() ? It will call msm_gpu_init, which in turn sets gpu->core_clk.
>>
>> Ideally you can call devm_pm_opp_set_clkname() from that function.
> 
> 
>> Or maybe completely drop gpu->core_clk and always use devm_pm_opp_set_clk_rate().
> That would break non-OPP targets, last of which were probably added N=big years ago..

No. In the lack of OPP tables, dev_pm_opp_clk_set_rate() should behave 
exactly like the clk_set_rate().

> I'm not sure these would still work, as I think we've got rid of some ugly
> clock getters that were looking for both "core" and "core_clk" etc.

We still support core vs core_clk, see the get_clocks() at msm_gpu.c and 
then msm_clk_bulk_get_clock(). However we might mimick this function and 
call devm_pm_opp_set_clkname() with the proper name ("core" or "core_clk").

> 
> See 8db0b6c7b636376789e356d861c3c6c35dcb6913 for what seems to be the most recent
> example of non-OPP.
> 
> IMX51/53 also have no OPP tables and are using the (AFAIK) now-defunct _clk-suffixed
> clock-names.

It works, I tested it during this cycle.

> 
> I'd be more than happy to rip out some of this legacy code and convert it
> to something modern like OPP, but I'm not sure you guys would like it considering
> the breakage on (arguably ancient and borderline retired) platforms.

I think, we should try switching to OPP-for-everybody, granted the 
promise of dev_pm_opp_set_clk_rate() being backwards compatible with 
bare clk_set_rate().

> 
> This patch as-is "only" breaks non-OPP a5xx & a6xx (as they have .gpu_busy defined),
> of which there are none..
> 
>>
>>> +
>>>        adreno_gpu->funcs = funcs;
>>>        adreno_gpu->info = adreno_info(config->rev);
>>>        adreno_gpu->gmem = adreno_gpu->info->gmem;
>>> 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);
>>
>> This is not enough, there are calls to clk_set_rate(gpu->core_clk) in msm_gpu.c which are called from the suspend/resume path.
> Right, good catch.
> 
> Konrad
>>
>>>        }
>>>          dev_pm_opp_put(opp);
>>

-- 
With best wishes
Dmitry