Re: [Intel-gfx] [PATCH] drm/i915/slpc: Let's fix the PCODE min freq table setup for SLPC

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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@intel.com>

Thanks