| Subject | Re: [PATCH 05/15] drm/panfrost: use spinlock instead of atomic | From | Robin Murphy <> | Date | Fri, 29 May 2020 13:20:24 +0100 |
| |
On 2020-05-10 17:55, Clément Péron wrote: > Convert busy_count to a simple int protected by spinlock.
A little more reasoning might be nice.
> Signed-off-by: Clément Péron <peron.clem@gmail.com> > --- [...] > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h > index 0697f8d5aa34..e6629900a618 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/spinlock.h> > #include <linux/ktime.h> > > struct devfreq; > @@ -14,10 +15,17 @@ struct panfrost_device; > struct panfrost_devfreq { > struct devfreq *devfreq; > struct thermal_cooling_device *cooling; > + > ktime_t busy_time; > ktime_t idle_time; > ktime_t time_last_update; > - atomic_t busy_count; > + int busy_count; > + /* > + * Protect busy_time, idle_time, time_last_update and busy_count > + * because these can be updated concurrently, for example by the GP > + * and PP interrupts. > + */
Nit: this comment is clearly wrong, since we only have Job, GPU and MMU interrupts here. I guess if there is a race it would be between submission/completion/timeout on different job slots.
Given that, should this actually be considered a fix for 9e62b885f715 ("drm/panfrost: Simplify devfreq utilisation tracking")?
Robin.
|