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

Deepak S
2015-05-02T07:30:00Z

On Thursday 30 April 2015 07:35 PM, Ville Syrjälä wrote:
> On Thu, Apr 30, 2015 at 02:19:07PM +0300, Ville Syrjälä wrote:
>> On Thu, Apr 30, 2015 at 03:42:42PM +0530, Deepak S wrote:
>>> As you suggested it would be better to extend the VLV WA to
>>> CHV also to make sure we drop the voltage when idle.
>>>
>>> Below is the sequence I think we should follow (based on your comments).
>>> 1. forcewake power wells
>>> 2. do gfx force clock on
>>> 3. request freq to punit
>>> 4. release gfx force clock on
>>> 5. release forcewake of power wells.
>>>
>>> Please share your thoughts?
>> I'm thinking we shouldn't need the gfx clock force since forcewake
>> should already cause the clock to be enabled.
>>
>> I've also not verified what happens if we drop the forcewake before
>> Punit has actually finished the frequency change. I'll try to hack
>> up some kind of test to see if I can make that happen.
> OK, after some hacking I see that the forcewake is enough, and also
> we don't seem to need to wait for the Punit to finish the frequency
> change before dropping forcewake. It seems to finish the change even
> after forcewake has been dropped.
>
> Also for a bit of extra micro optimization we should perhaps wake up
> the media well only, as that takes a lot less power than the render
> well.

Thanks Ville. I will submit new patch to extend the WA to CHV

Thanks
Deepak

>>> Thanks
>>> Deepak
>>>
>>>>> So based on my tests this patch feels a bit wrong.
>>>>>
>>>>>> Since Punit validates the rps range [RPe, RP0]. This patch
>>>>>> removes unused cherryview_rps_min_freq function.
>>>>>>
>>>>>> v2: Change commit message
>>>>>>
>>>>>> v3: set min_freq before idle_freq (chris)
>>>>>>
>>>>>> v4: Squash 'Remove unused rps min function' patch
>>>>>>
>>>>>> Signed-off-by: Deepak S <deepak.s@linux.intel.com>
>>>>>> Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>>> ---
>>>>>> drivers/gpu/drm/i915/intel_pm.c | 21 ++-------------------
>>>>>> 1 file changed, 2 insertions(+), 19 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>>>>> index a7516ed..78c89ff 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>>>> @@ -4715,24 +4715,6 @@ static int cherryview_rps_guar_freq(struct drm_i915_private *dev_priv)
>>>>>> return rp1;
>>>>>> }
>>>>>>
>>>>>> -static int cherryview_rps_min_freq(struct drm_i915_private *dev_priv)
>>>>>> -{
>>>>>> - struct drm_device *dev = dev_priv->dev;
>>>>>> - u32 val, rpn;
>>>>>> -
>>>>>> - if (dev->pdev->revision >= 0x20) {
>>>>>> - val = vlv_punit_read(dev_priv, FB_GFX_FMIN_AT_VMIN_FUSE);
>>>>>> - rpn = ((val >> FB_GFX_FMIN_AT_VMIN_FUSE_SHIFT) &
>>>>>> - FB_GFX_FREQ_FUSE_MASK);
>>>>>> - } else { /* For pre-production hardware */
>>>>>> - val = vlv_punit_read(dev_priv, PUNIT_GPU_STATUS_REG);
>>>>>> - rpn = ((val >> PUNIT_GPU_STATIS_GFX_MIN_FREQ_SHIFT) &
>>>>>> - PUNIT_GPU_STATUS_GFX_MIN_FREQ_MASK);
>>>>>> - }
>>>>>> -
>>>>>> - return rpn;
>>>>>> -}
>>>>>> -
>>>>>> static int valleyview_rps_guar_freq(struct drm_i915_private *dev_priv)
>>>>>> {
>>>>>> u32 val, rp1;
>>>>>> @@ -4984,7 +4966,8 @@ static void cherryview_init_gt_powersave(struct drm_device *dev)
>>>>>> intel_gpu_freq(dev_priv, dev_priv->rps.rp1_freq),
>>>>>> dev_priv->rps.rp1_freq);
>>>>>>
>>>>>> - dev_priv->rps.min_freq = cherryview_rps_min_freq(dev_priv);
>>>>>> + /* PUnit validated range is only [RPe, RP0] */
>>>>>> + dev_priv->rps.min_freq = dev_priv->rps.efficient_freq;
>>>>>> DRM_DEBUG_DRIVER("min GPU freq: %d MHz (%u)\n",
>>>>>> intel_gpu_freq(dev_priv, dev_priv->rps.min_freq),
>>>>>> dev_priv->rps.min_freq);
>>>>>> --
>>>>>> 1.9.1
>>>>> --
>>>>> Ville Syrjälä
>>>>> Intel OTC
>>>>> _______________________________________________
>>>>> Intel-gfx mailing list
>>>>> Intel-gfx@lists.freedesktop.org
>>>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> --
>> Ville Syrjälä
>> Intel OTC