Re: [PATCH v2 3/4] drm/i915/chv: Set min freq to efficient frequency on chv

Chris Wilson
2015-03-26T21:43:54Z

On Thu, Mar 26, 2015 at 06:32:15PM -0300, Paulo Zanoni wrote:
> 2015-03-19 11:14 GMT-03:00 <deepak.s@linux.intel.com>:
> > From: Deepak S <deepak.s@linux.intel.com>
> >
> > After feedback from the hardware team, now we set the GPU min/idel freq to RPe.
> > Punit is expecting us to operate GPU between Rpe & Rp0. If we drop the
> > frequency to RPn, punit is failing to change the input voltage to
> > minimum :(
>
> Since this is far away from the obvious, I am imagining some
> programmer from the future looking at this code and imagining
> min_freq_softlimit was "accidentally" set to RPe instead of RPn. Can't
> we add a comment in the code - not just the commit message -, to make
> it clear that we're doing this since the punit is weird?
>
> Another thing which I noticed is that your patch title mentions CHV,
> but your patch touches the VLV function instead of the CHV one. This
> also leads me to think that maybe the power measurement experiments
> you did were done using the non-patched CHV code... Can you please
> clarify your intentions here? And also maybe redo the power
> measurements if needed.
>
> Also, I think we need at least an ACK from Chris here, especially
> since he was already discussing the previous version of this patch.

If you include a comment like (and note we want to set
dev_priv->rps.min_freq not dev_priv->rps.min_freq_softlimit):

/* PUnit validated range is only [RPe, RP0] */
dev_priv->rps.min_freq = dev_priv->rps.efficient_freq;

and make sure that is set before we derive dev_priv->rps.idle_freq.

You can have my
Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

--
Chris Wilson, Intel Open Source Technology Centre