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

[Dmitry Baryshkov]

On Mon, 20 Feb 2023 at 11:59, Konrad Dybcio <konrad.dybcio@xxxxxxxxxx> 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@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 was referring to a comment at dev_pm_opp_set_rate():

/*
* For IO devices which require an OPP on some platforms/SoCs
* while just needing to scale the clock on some others
* we look for empty OPP tables with just a clock handle and
* scale only the clk. This makes dev_pm_opp_set_rate()
* equivalent to a clk_set_rate()
*/

Maybe we just need to make sure that the OPP table exists
(devm_pm_opp_of_add_table) to prevent the function from bailing out
early.

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



-- 
With best wishes
Dmitry