On Fri, Nov 19, 2021 at 4:21 PM Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
>
> Hi,
>
> On Fri, Nov 19, 2021 at 2:47 PM Rob Clark <robdclark@xxxxxxxxx> wrote:
> >
> > +void msm_devfreq_boost(struct msm_gpu *gpu, unsigned factor)
> > +{
> > + struct msm_gpu_devfreq *df = &gpu->devfreq;
> > + unsigned long freq;
> > +
> > + freq = get_freq(gpu);
> > + freq *= factor;
> > + freq /= HZ_PER_KHZ;
>
> Should it do the divide first? I don't know for sure, but it feels
> like GPU frequency could conceivably be near-ish the u32 overflow? (~4
> GHz). Better to be safe and do the / 1000 first?
>
It looks like on 8998 we have some GPU OPPs that are not integer # of
KHz.. although that would not change the integer math result unless
factor > 10.
We are a bit aways for 32b overflow (highest freq for current things
is 825MHz, but I guess we could see things closer to 1GHz in the
future.. generally GPUs aren't clocked nearly as high as CPUs.. slow
but wide, and all that).. but maybe this should just be 64b math
instead to be safe?
>
> > @@ -201,26 +217,14 @@ static void msm_devfreq_idle_work(struct kthread_work *work)
> > struct msm_gpu_devfreq *df = container_of(work,
> > struct msm_gpu_devfreq, idle_work.work);
> > struct msm_gpu *gpu = container_of(df, struct msm_gpu, devfreq);
> > - unsigned long idle_freq, target_freq = 0;
> >
> > if (!df->devfreq)
> > return;
>
> Why does the msm_devfreq_idle_work() need a check for "!df->devfreq"
> but the boost work doesn't? Maybe you don't need it anymore now that
> you're not reaching into the mutex? ...or maybe the boost work does
> need it?
>
> ...and if "df->devfreq" is NULL then doesn't it mean that
> msm_hrtimer_work_init() was never called? That seems bad...
Looks like 658f4c829688 ("drm/msm/devfreq: Add 1ms delay before
clamping freq") was badly rebased on top of efb8a170a367 ("drm/msm:
Fix devfreq NULL pointer dereference on a3xx").. I'll send a separate
patch to fix that
BR,
-R
