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@xxxxxxxxx> Thanks
