[2/2] drm/panfrost: Fix incorrect updating of current device frequency
Commit Message
It was noticed when setting the Panfrost's DVFS device to the performant
governor, GPU frequency as reported by fdinfo had dropped to 0 permamently.
There are two separate issues causing this behaviour:
- Not initialising the device's current_frequency variable to its original
value during device probe().
- Updating said variable in Panfrost devfreq's get_dev_status() rather
than after the new OPP's frequency had been retrieved in target(), which
meant the old frequency would be assigned instead.
Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
Fixes: f11b0417eec2 ("drm/panfrost: Add fdinfo support GPU load metrics")
---
drivers/gpu/drm/panfrost/panfrost_devfreq.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
Comments
On 25/11/2023 20:52, Adrián Larumbe wrote:
> It was noticed when setting the Panfrost's DVFS device to the performant
> governor, GPU frequency as reported by fdinfo had dropped to 0 permamently.
>
> There are two separate issues causing this behaviour:
> - Not initialising the device's current_frequency variable to its original
> value during device probe().
> - Updating said variable in Panfrost devfreq's get_dev_status() rather
> than after the new OPP's frequency had been retrieved in target(), which
> meant the old frequency would be assigned instead.
Good catch - I'd not looked at the performance governor. I'd assumed
that one was "too simple to be wrong" ;)
Reviewed-by: Steven Price <steven.price@arm.com>
>
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> Fixes: f11b0417eec2 ("drm/panfrost: Add fdinfo support GPU load metrics")
> ---
> drivers/gpu/drm/panfrost/panfrost_devfreq.c | 17 +++++++++++++++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index f59c82ea8870..2d30da38c2c3 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -29,14 +29,20 @@ static void panfrost_devfreq_update_utilization(struct panfrost_devfreq *pfdevfr
> static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
> u32 flags)
> {
> + struct panfrost_device *ptdev = dev_get_drvdata(dev);
> struct dev_pm_opp *opp;
> + int err;
>
> opp = devfreq_recommended_opp(dev, freq, flags);
> if (IS_ERR(opp))
> return PTR_ERR(opp);
> dev_pm_opp_put(opp);
>
> - return dev_pm_opp_set_rate(dev, *freq);
> + err = dev_pm_opp_set_rate(dev, *freq);
> + if (!err)
> + ptdev->pfdevfreq.current_frequency = *freq;
> +
> + return err;
> }
>
> static void panfrost_devfreq_reset(struct panfrost_devfreq *pfdevfreq)
> @@ -58,7 +64,6 @@ static int panfrost_devfreq_get_dev_status(struct device *dev,
> spin_lock_irqsave(&pfdevfreq->lock, irqflags);
>
> panfrost_devfreq_update_utilization(pfdevfreq);
> - pfdevfreq->current_frequency = status->current_frequency;
>
> status->total_time = ktime_to_ns(ktime_add(pfdevfreq->busy_time,
> pfdevfreq->idle_time));
> @@ -164,6 +169,14 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>
> panfrost_devfreq_profile.initial_freq = cur_freq;
>
> + /*
> + * We could wait until panfrost_devfreq_target() to set this value, but
> + * since the simple_ondemand governor works asynchronously, there's a
> + * chance by the time someone opens the device's fdinfo file, current
> + * frequency hasn't been updated yet, so let's just do an early set.
> + */
> + pfdevfreq->current_frequency = cur_freq;
> +
> /*
> * Set the recommend OPP this will enable and configure the regulator
> * if any and will avoid a switch off by regulator_late_cleanup()
@@ -29,14 +29,20 @@ static void panfrost_devfreq_update_utilization(struct panfrost_devfreq *pfdevfr
static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
u32 flags)
{
+ struct panfrost_device *ptdev = dev_get_drvdata(dev);
struct dev_pm_opp *opp;
+ int err;
opp = devfreq_recommended_opp(dev, freq, flags);
if (IS_ERR(opp))
return PTR_ERR(opp);
dev_pm_opp_put(opp);
- return dev_pm_opp_set_rate(dev, *freq);
+ err = dev_pm_opp_set_rate(dev, *freq);
+ if (!err)
+ ptdev->pfdevfreq.current_frequency = *freq;
+
+ return err;
}
static void panfrost_devfreq_reset(struct panfrost_devfreq *pfdevfreq)
@@ -58,7 +64,6 @@ static int panfrost_devfreq_get_dev_status(struct device *dev,
spin_lock_irqsave(&pfdevfreq->lock, irqflags);
panfrost_devfreq_update_utilization(pfdevfreq);
- pfdevfreq->current_frequency = status->current_frequency;
status->total_time = ktime_to_ns(ktime_add(pfdevfreq->busy_time,
pfdevfreq->idle_time));
@@ -164,6 +169,14 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
panfrost_devfreq_profile.initial_freq = cur_freq;
+ /*
+ * We could wait until panfrost_devfreq_target() to set this value, but
+ * since the simple_ondemand governor works asynchronously, there's a
+ * chance by the time someone opens the device's fdinfo file, current
+ * frequency hasn't been updated yet, so let's just do an early set.
+ */
+ pfdevfreq->current_frequency = cur_freq;
+
/*
* Set the recommend OPP this will enable and configure the regulator
* if any and will avoid a switch off by regulator_late_cleanup()