Re: [PATCH] drm/panfrost: Add governor data with pre-defined thresholds

[Steven Price]

On 22/01/2021 10:11, Lukasz Luba wrote:
> On 1/21/21 5:15 PM, Daniel Lezcano wrote:
> > On 21/01/2021 18:04, Lukasz Luba wrote:
> > > The simple_ondemand devfreq governor uses two thresholds to decide about
> > > the frequency change: upthreshold, downdifferential. These two tunable
> > > change the behavior of the governor decision, e.g. how fast to increase
> > > the frequency or how rapidly limit the frequency. This patch adds needed
> > > governor data with thresholds values gathered experimentally in 
> > > different
> > > workloads.
> > >
> > > Signed-off-by: Lukasz Luba <lukasz.luba@xxxxxxx>
> > > ---
> > > Hi all,
> > >
> > > This patch aims to improve the panfrost performance in various 
> > > workloads,
> > > (benchmarks, games). The simple_ondemand devfreq governor supports
> > > tunables to tweak the behaviour of the internal algorithm. The default
> > > values for these two thresholds (90 and 5) do not work well with 
> > > panfrost.
> > > These new settings should provide good performance, short latency for
> > > rising the frequency due to rapid workload change and decent freq slow
> > > down when the load is decaying. Based on frequency change statistics,
> > > gathered during experiments, all frequencies are used, depending on
> > > the load. This provides some power savings (statistically). The highest
> > > frequency is also used when needed.
> > >
> > > Example glmark2 results:
> > > 1. freq fixed to max: 153
> > > 2. these new thresholds values (w/ patch): 151
> > > 3. default governor values (w/o patch): 114
> > >
> > > In future the devfreq framework would expose via sysfs these two
> > > tunables, so they can be adjusted by the middleware based on currently
> > > running workload (game, desktop, web browser, etc). These new values
> > > should be good enough, though.
> > >
> > > Regards,
> > > Lukasz Luba
> > >
> > >   drivers/gpu/drm/panfrost/panfrost_devfreq.c | 10 +++++++++-
> > >   drivers/gpu/drm/panfrost/panfrost_devfreq.h |  2 ++
> > >   2 files changed, 11 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c 
> > > b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > > index 56b3f5935703..7c5ffc81dce1 100644
> > > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > > @@ -130,8 +130,16 @@ int panfrost_devfreq_init(struct panfrost_device 
> > > *pfdev)
> > >       panfrost_devfreq_profile.initial_freq = cur_freq;
> > >       dev_pm_opp_put(opp);
> > > +    /*
> > > +     * Setup default thresholds for the simple_ondemand governor.
> > > +     * The values are chosen based on experiments.
> > > +     */
> > > +    pfdevfreq->gov_data.upthreshold = 45;
> > > +    pfdevfreq->gov_data.downdifferential = 5;
> > > +
> > >       devfreq = devm_devfreq_add_device(dev, &panfrost_devfreq_profile,
> > > -                      DEVFREQ_GOV_SIMPLE_ONDEMAND, NULL);
> > > +                      DEVFREQ_GOV_SIMPLE_ONDEMAND,
> > > +                      &pfdevfreq->gov_data);
> > >       if (IS_ERR(devfreq)) {
> > >           DRM_DEV_ERROR(dev, "Couldn't initialize GPU devfreq\n");
> > >           ret = PTR_ERR(devfreq);
> > > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h 
> > > b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> > > index db6ea48e21f9..1e2a4de941aa 100644
> > > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> > > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> > > @@ -4,6 +4,7 @@
> > >   #ifndef __PANFROST_DEVFREQ_H__
> > >   #define __PANFROST_DEVFREQ_H__
> > > +#include <linux/devfreq.h>
> > >   #include <linux/spinlock.h>
> > >   #include <linux/ktime.h>
> > > @@ -17,6 +18,7 @@ struct panfrost_devfreq {
> > >       struct devfreq *devfreq;
> > >       struct opp_table *regulators_opp_table;
> > >       struct thermal_cooling_device *cooling;
> > > +    struct devfreq_simple_ondemand_data gov_data;
> > >       bool opp_of_table_added;
> > >       ktime_t busy_time;
> >
> > I think it is simpler to do:
> >
> > +static struct devfreq_simple_ondemand_data panfrost_ondemand_data = {
> > +       .upthreshold = 45,
> > +       .downdifferential = 5,
> > +};
> >
> > [ ... ]
> >
> >         devfreq = devm_devfreq_add_device(dev, &panfrost_devfreq_profile,
> > -                                         DEVFREQ_GOV_SIMPLE_ONDEMAND,
> > NULL);
> > +                                         DEVFREQ_GOV_SIMPLE_ONDEMAND,
> > +                                         &panfrost_ondemand_data);
>
> Yes, it's simpler. The driver would probably never have to serve two
> GPUs. I've tried to keep this thing inside the panfrost struct,
> forgetting about it.

The Juno platform with an FPGA attached is the only example I know of 
where a system has multiple Mali GPUs - so it can happen, but it rare.

As it stands a static structure would work because the values are 
constant - but Lukasz mentioned that they would be exported in sysfs in 
the future, in which case they really should be part of the panfrost struct.

Ultimately having a (non-const) static struct like above would mean 
wasting a few bytes on systems with Panfrost loaded but no Mali GPU. 
Having it in struct panfrost means the cost is only for Mali. Admittedly 
it's only a few bytes in this case and often Panfrost will be a module.

Steve

> Steven already reviewed the patch, so it can probably stay.
> I will keep it in mind. Thank you for the comments.
>
> Regards,
> Lukasz