[Intel-gfx] [PATCH] drm/i915/slpc: Let's fix the PCODE min freq table setup for SLPC
Rodrigo Vivi
rodrigo.vivi at intel.com
Thu Sep 1 16:53:45 UTC 2022
On Wed, Aug 31, 2022 at 03:17:26PM -0700, Dixit, Ashutosh wrote:
> On Wed, 31 Aug 2022 14:45:38 -0700, Rodrigo Vivi wrote:
> >
>
> Hi Rodrigo,
>
> > We need to inform PCODE of a desired ring frequencies so PCODE update
> > the memory frequencies to us. rps->min_freq and rps->max_freq are the
> > frequencies used in that request. However they were unset when SLPC was
> > enabled and PCODE never updated the memory freq.
> >
> > v2 (as Suggested by Ashutosh): if SLPC is in use, let's pick the right
> > frequencies from the get_ia_constants instead of the fake init of
> > rps' min and max.
> >
> > v3: don't forget the max <= min return
> >
> > v4: Move all the freq conversion to intel_rps.c. And the max <= min
> > check to where it belongs.
> >
> > v5: (Ashutosh) Fix old comment s/50 HZ/50 MHz and add a doc explaining
> > the "raw format"
>
> I think we both agree that mostly the way this patch is written it is to
> add SLPC but not risk disturbing host turbo, specially old platforms
> (CHV/VLV/ILK and pre-Gen 6). Also these freq units (sometimes 16.67 MHz
> units, sometimes 50 MHz, sometime MHz) in different places in the driver
> and different product generations is hugely confusing to say the least. For
> old platform we don't really know what units the freq's are in, we only
> know intel_gpu_freq will magically convert freq's to MHz. In any case let's
> work with what we have.
yeap!
>
> > @@ -130,6 +123,12 @@ static void gen6_update_ring_freq(struct intel_llc *llc)
> > if (!get_ia_constants(llc, &consts))
> > return;
> >
> > + /*
> > + * Although this is unlikely on any platform during initialization,
> > + * let's ensure we don't get accidentally into infinite loop
> > + */
> > + if (consts.max_gpu_freq <= consts.min_gpu_freq)
> > + return;
>
> As I said I would remove reference to "infinite loop", I am not seeing any
> infinite loop, maybe just delete the comment.
>
> Also as I said I see the check above should be completely removed (so it is
> actually a pre-existing bug in the code). However since you want to carry
> it forward in order not to risk disturbing legacy behavior that's fine.
I know we can get the infinit loop because I faced it here on a bad config
where min = max. os if min >= max, the for loop will never close.
And in case we have some fused parts with min = max we will take a while
to figure out what's going on during po. and who knows about older platforms
and skus out there as well.
I will keep the comment so we don't end up removing it from here.
>
> Rest LGTM:
>
> Reviewed-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
Thanks
More information about the Intel-gfx
mailing list