On Fri, Nov 19, 2021 at 4:21 PM Doug Anderson dianders@chromium.org wrote:
Hi,
On Fri, Nov 19, 2021 at 2:47 PM Rob Clark robdclark@gmail.com 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