Message-ID: <de1ceb35-0b9b-b99d-49f4-f93cc8332032@linaro.org>
Date:   Mon, 20 Feb 2023 11:12:34 +0100
From:   Konrad Dybcio <konrad.dybcio@...aro.org>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        linux-arm-msm@...r.kernel.org, andersson@...nel.org,
        agross@...nel.org
Cc:     marijn.suijten@...ainline.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>,
        Akhil P Oommen <quic_akhilpo@...cinc.com>,
        Emma Anholt <emma@...olt.net>, Chia-I Wu <olvaffe@...il.com>,
        Dan Carpenter <error27@...il.com>,
        dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 06/14] drm/msm/gpu: Use dev_pm_opp_set_rate for non-GMU
 GPUs



On 20.02.2023 10:59, Konrad Dybcio wrote:
> 
> 
> 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@...aro.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().
> 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]).
Actually.. that's what happens for gpu-pwrlevels users already..
Well, use>r<, as apq8064 seems to have been the only user of
that upstream, ever..

And for A2XX it looks like it just unconditionally selects 200
MHz..

I think this could be simplified to:

if (opp exists)
    // use opp
else if (adreno_is_a2xx)
    dev_pm_opp_add(dev, 200000000, 0) //device, freq_hz, volt_uV
else if (adreno_is_a320)
    dev_pm_opp_add(dev, 450000000, 0)
else
    // for now the driver sets 200mhz here, but i don't think
    // it's reasonable to keep carrying that behavior for >a2xx
    return -EINVAL


And then we can yank out all clk_set_rate calls just like that!

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

Powered by blists - more mailing lists