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

[Deepak S <deepak.s@linux.intel.com>]

On Friday 27 March 2015 03:13 AM, Chris Wilson wrote:
> 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

Thanks Chris. I will address comments & rebase the patch.

>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx