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

[Konrad Dybcio]

On 18.02.2023 17:47, Dmitry Baryshkov wrote:
> 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@xxxxxxxxxx>
>>>> ---
>>>>    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().
Not sure if that's what you meant, but if a device lacks OPP,
devm_pm_opp_set_rate will return -ENODEV.

If you meant "if we can't find an opp table, behave as if we
called clk_set_rate", a discussion on #freedreno with robclark
indicates he'd accept getting rid of non-opp code, provided we
construct a table if need be, since we have the data required
to do so ([FMIN=27MHz, FMAX=fast_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.
Oh okay, I had a feeling like that was dropped at one point..

> 
>>
>> 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().
It's not, but as I mentioned, we can easily work around that.

> 
>>
>> This patch as-is "only" breaks non-OPP a5xx & a6xx (as they have .gpu_busy defined),
>> of which there are none..
...but we want to get devfreq everywhere and it's a few LoC away..

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