Re: [PATCH 1/5] drm/msm/adreno: Use OPP for every GPU generation

[Dmitry Baryshkov]

On 23/02/2023 00:40, Konrad Dybcio wrote:
> 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.

I see. I got lost in all the ifs. What do you think about turning it 
into the main code path, since after this code block we always have a 
valid OPP table?

> > > +        } 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,


--
With best wishes
Dmitry