[RFC,3/4] drm/i915: valleyview: Make intel_set_rps get FORCEWAKE_MEDIA
diff mbox

Message ID 20170101201403.12132-4-hdegoede@redhat.com
State RFC
Headers show

Commit Message

Hans de Goede Jan. 1, 2017, 8:14 p.m. UTC
All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except
for intel_set_rps(). Since intel_set_rps can for example be called from
sysfs store functions, there is no guarantee this is already done, so add
an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Chris Wilson Jan. 1, 2017, 8:24 p.m. UTC | #1
On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote:
> All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except
> for intel_set_rps(). Since intel_set_rps can for example be called from
> sysfs store functions, there is no guarantee this is already done, so add
> an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 4b12637..cc4fbd7 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
>  
>  void intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
>  {
> -	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> +		/* Wake up the media well, as that takes a lot less
> +		 * power than the Render well.
> +		 */
> +		intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
>  		valleyview_set_rps(dev_priv, val);

Both powerwells are woken for rps. (Taking one but not the other has no
benefit, and very misleading.) The forcewake is already held by the
lower level routines, taking the wakelock in the caller is an optimisation
that is only interesting if there is a danger from the forcewake being
dropped mid-sequence (due to preemption whatever).
-Chris
Hans de Goede Jan. 1, 2017, 8:48 p.m. UTC | #2
Hi,

On 01-01-17 21:24, Chris Wilson wrote:
> On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote:
>> All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except
>> for intel_set_rps(). Since intel_set_rps can for example be called from
>> sysfs store functions, there is no guarantee this is already done, so add
>> an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 4b12637..cc4fbd7 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
>>
>>  void intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
>>  {
>> -	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
>> +		/* Wake up the media well, as that takes a lot less
>> +		 * power than the Render well.
>> +		 */
>> +		intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
>>  		valleyview_set_rps(dev_priv, val);
>
> Both powerwells are woken for rps. (Taking one but not the other has no
> benefit, and very misleading.)

The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the
existing code in vlv_set_rps_idle().

 > The forcewake is already held by the
> lower level routines, taking the wakelock in the caller is an optimisation
> that is only interesting if there is a danger from the forcewake being
> dropped mid-sequence (due to preemption whatever).

We're also accessing the punit in valleyview_set_rps() and I've seen several
patches (in the android x86 kernel) suggesting that we need to take a wakelock
while doing this.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chris Wilson Jan. 2, 2017, 11:37 a.m. UTC | #3
On Sun, Jan 01, 2017 at 09:48:53PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 01-01-17 21:24, Chris Wilson wrote:
> >On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote:
> >>All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except
> >>for intel_set_rps(). Since intel_set_rps can for example be called from
> >>sysfs store functions, there is no guarantee this is already done, so add
> >>an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps.
> >>
> >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>---
> >> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
> >> 1 file changed, 7 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> >>index 4b12637..cc4fbd7 100644
> >>--- a/drivers/gpu/drm/i915/intel_pm.c
> >>+++ b/drivers/gpu/drm/i915/intel_pm.c
> >>@@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
> >>
> >> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
> >> {
> >>-	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> >>+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> >>+		/* Wake up the media well, as that takes a lot less
> >>+		 * power than the Render well.
> >>+		 */
> >>+		intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
> >> 		valleyview_set_rps(dev_priv, val);
> >
> >Both powerwells are woken for rps. (Taking one but not the other has no
> >benefit, and very misleading.)
> 
> The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the
> existing code in vlv_set_rps_idle().

It's not correct there either...
 
> > The forcewake is already held by the
> >lower level routines, taking the wakelock in the caller is an optimisation
> >that is only interesting if there is a danger from the forcewake being
> >dropped mid-sequence (due to preemption whatever).
> 
> We're also accessing the punit in valleyview_set_rps() and I've seen several
> patches (in the android x86 kernel) suggesting that we need to take a wakelock
> while doing this.

It is quite likely that we do have to guarantee to keep the punit awake
during long sequences. That would be an acceptable rationale (but not in
the caller!).
-Chris
Hans de Goede Jan. 2, 2017, 12:40 p.m. UTC | #4
Hi,

On 02-01-17 12:37, Chris Wilson wrote:
> On Sun, Jan 01, 2017 at 09:48:53PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 01-01-17 21:24, Chris Wilson wrote:
>>> On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote:
>>>> All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except
>>>> for intel_set_rps(). Since intel_set_rps can for example be called from
>>>> sysfs store functions, there is no guarantee this is already done, so add
>>>> an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
>>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>>>> index 4b12637..cc4fbd7 100644
>>>> --- a/drivers/gpu/drm/i915/intel_pm.c
>>>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>>>> @@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
>>>>
>>>> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
>>>> {
>>>> -	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>>>> +	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
>>>> +		/* Wake up the media well, as that takes a lot less
>>>> +		 * power than the Render well.
>>>> +		 */
>>>> +		intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
>>>> 		valleyview_set_rps(dev_priv, val);
>>>
>>> Both powerwells are woken for rps. (Taking one but not the other has no
>>> benefit, and very misleading.)
>>
>> The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the
>> existing code in vlv_set_rps_idle().
>
> It's not correct there either...
>
>>> The forcewake is already held by the
>>> lower level routines, taking the wakelock in the caller is an optimisation
>>> that is only interesting if there is a danger from the forcewake being
>>> dropped mid-sequence (due to preemption whatever).
>>
>> We're also accessing the punit in valleyview_set_rps() and I've seen several
>> patches (in the android x86 kernel) suggesting that we need to take a wakelock
>> while doing this.
>
> It is quite likely that we do have to guarantee to keep the punit awake
> during long sequences. That would be an acceptable rationale (but not in
> the caller!).

So what would be a good approach, take FORCEWAKE_ALL in valleyview_set_rps()
itself (and drop the existing code from vlv_set_rps_idle()) ?

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ville Syrjala Jan. 2, 2017, 2:10 p.m. UTC | #5
On Mon, Jan 02, 2017 at 11:37:59AM +0000, Chris Wilson wrote:
> On Sun, Jan 01, 2017 at 09:48:53PM +0100, Hans de Goede wrote:
> > Hi,
> > 
> > On 01-01-17 21:24, Chris Wilson wrote:
> > >On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote:
> > >>All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except
> > >>for intel_set_rps(). Since intel_set_rps can for example be called from
> > >>sysfs store functions, there is no guarantee this is already done, so add
> > >>an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps.
> > >>
> > >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > >>---
> > >> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
> > >> 1 file changed, 7 insertions(+), 2 deletions(-)
> > >>
> > >>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > >>index 4b12637..cc4fbd7 100644
> > >>--- a/drivers/gpu/drm/i915/intel_pm.c
> > >>+++ b/drivers/gpu/drm/i915/intel_pm.c
> > >>@@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
> > >>
> > >> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
> > >> {
> > >>-	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > >>+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > >>+		/* Wake up the media well, as that takes a lot less
> > >>+		 * power than the Render well.
> > >>+		 */
> > >>+		intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
> > >> 		valleyview_set_rps(dev_priv, val);
> > >
> > >Both powerwells are woken for rps. (Taking one but not the other has no
> > >benefit, and very misleading.)
> > 
> > The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the
> > existing code in vlv_set_rps_idle().
> 
> It's not correct there either...

Why not? If the render well is in rc6 already we don't want to waste
power by waking it. The only reason we have to wake up *something* is
so that the gpll frequency actually gets changed.
Chris Wilson Jan. 2, 2017, 2:21 p.m. UTC | #6
On Mon, Jan 02, 2017 at 04:10:49PM +0200, Ville Syrjälä wrote:
> On Mon, Jan 02, 2017 at 11:37:59AM +0000, Chris Wilson wrote:
> > On Sun, Jan 01, 2017 at 09:48:53PM +0100, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 01-01-17 21:24, Chris Wilson wrote:
> > > >On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote:
> > > >>All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except
> > > >>for intel_set_rps(). Since intel_set_rps can for example be called from
> > > >>sysfs store functions, there is no guarantee this is already done, so add
> > > >>an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps.
> > > >>
> > > >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > >>---
> > > >> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
> > > >> 1 file changed, 7 insertions(+), 2 deletions(-)
> > > >>
> > > >>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > >>index 4b12637..cc4fbd7 100644
> > > >>--- a/drivers/gpu/drm/i915/intel_pm.c
> > > >>+++ b/drivers/gpu/drm/i915/intel_pm.c
> > > >>@@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
> > > >>
> > > >> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
> > > >> {
> > > >>-	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > >>+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > > >>+		/* Wake up the media well, as that takes a lot less
> > > >>+		 * power than the Render well.
> > > >>+		 */
> > > >>+		intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
> > > >> 		valleyview_set_rps(dev_priv, val);
> > > >
> > > >Both powerwells are woken for rps. (Taking one but not the other has no
> > > >benefit, and very misleading.)
> > > 
> > > The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the
> > > existing code in vlv_set_rps_idle().
> > 
> > It's not correct there either...
> 
> Why not? If the render well is in rc6 already we don't want to waste
> power by waking it. The only reason we have to wake up *something* is
> so that the gpll frequency actually gets changed.

If the register write requires the powerwell, the mmio access will take
the powerwell. Manually taking the wakelock has to be for a reason, the
one given is bogus. (That is if the register write only requires meda
access that is taken, but iirc, rps requires both, so both are taken
inside.) If the claimed benefit is indeed real, and there is indeed
freedom of choice, we want that inside the mmio accessor to choose the
cheaper well.
-Chris
Ville Syrjala Jan. 2, 2017, 2:40 p.m. UTC | #7
On Mon, Jan 02, 2017 at 02:21:58PM +0000, Chris Wilson wrote:
> On Mon, Jan 02, 2017 at 04:10:49PM +0200, Ville Syrjälä wrote:
> > On Mon, Jan 02, 2017 at 11:37:59AM +0000, Chris Wilson wrote:
> > > On Sun, Jan 01, 2017 at 09:48:53PM +0100, Hans de Goede wrote:
> > > > Hi,
> > > > 
> > > > On 01-01-17 21:24, Chris Wilson wrote:
> > > > >On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote:
> > > > >>All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except
> > > > >>for intel_set_rps(). Since intel_set_rps can for example be called from
> > > > >>sysfs store functions, there is no guarantee this is already done, so add
> > > > >>an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps.
> > > > >>
> > > > >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > > >>---
> > > > >> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
> > > > >> 1 file changed, 7 insertions(+), 2 deletions(-)
> > > > >>
> > > > >>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > >>index 4b12637..cc4fbd7 100644
> > > > >>--- a/drivers/gpu/drm/i915/intel_pm.c
> > > > >>+++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > >>@@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
> > > > >>
> > > > >> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
> > > > >> {
> > > > >>-	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > >>+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > > > >>+		/* Wake up the media well, as that takes a lot less
> > > > >>+		 * power than the Render well.
> > > > >>+		 */
> > > > >>+		intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
> > > > >> 		valleyview_set_rps(dev_priv, val);
> > > > >
> > > > >Both powerwells are woken for rps. (Taking one but not the other has no
> > > > >benefit, and very misleading.)
> > > > 
> > > > The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the
> > > > existing code in vlv_set_rps_idle().
> > > 
> > > It's not correct there either...
> > 
> > Why not? If the render well is in rc6 already we don't want to waste
> > power by waking it. The only reason we have to wake up *something* is
> > so that the gpll frequency actually gets changed.
> 
> If the register write requires the powerwell, the mmio access will take
> the powerwell.

The register write doesn't require a power well. It's a sideband access.
The punit will simply not change the GPLL frequency if the GPLL is
currently not running (which will/can happen when all power wells are
asleep). That in itself doesn't sound too back (why change the
frequency if the thing isn't even running, right?). But the real problem
is that the punit will not let the voltage on the rail to drop
either until the new frequency gets programmed into the GPLL. Hence if
the GPU goes idle before we've dropped the GPLL frequency to the
minimum value, we will waste power by having a needlessly high voltage.

Originally we tried to avoid this problem via vlv_force_gfx_clock(),
which should just force the GPLL to turn on without powering on
any power wells. But that caused some spurious WARNs or something
IIRC, so we had to come up with another workaround. And since powering
either well is sufficient we chose to use the cheaper media well.

> Manually taking the wakelock has to be for a reason, the
> one given is bogus. (That is if the register write only requires meda
> access that is taken, but iirc, rps requires both, so both are taken
> inside.) If the claimed benefit is indeed real, and there is indeed
> freedom of choice, we want that inside the mmio accessor to choose the
> cheaper well.
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
Chris Wilson Jan. 2, 2017, 2:53 p.m. UTC | #8
On Mon, Jan 02, 2017 at 04:40:05PM +0200, Ville Syrjälä wrote:
> On Mon, Jan 02, 2017 at 02:21:58PM +0000, Chris Wilson wrote:
> > On Mon, Jan 02, 2017 at 04:10:49PM +0200, Ville Syrjälä wrote:
> > > On Mon, Jan 02, 2017 at 11:37:59AM +0000, Chris Wilson wrote:
> > > > On Sun, Jan 01, 2017 at 09:48:53PM +0100, Hans de Goede wrote:
> > > > > Hi,
> > > > > 
> > > > > On 01-01-17 21:24, Chris Wilson wrote:
> > > > > >On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote:
> > > > > >>All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except
> > > > > >>for intel_set_rps(). Since intel_set_rps can for example be called from
> > > > > >>sysfs store functions, there is no guarantee this is already done, so add
> > > > > >>an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps.
> > > > > >>
> > > > > >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > > > >>---
> > > > > >> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
> > > > > >> 1 file changed, 7 insertions(+), 2 deletions(-)
> > > > > >>
> > > > > >>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > > >>index 4b12637..cc4fbd7 100644
> > > > > >>--- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > >>+++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > >>@@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
> > > > > >>
> > > > > >> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
> > > > > >> {
> > > > > >>-	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > > >>+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > > > > >>+		/* Wake up the media well, as that takes a lot less
> > > > > >>+		 * power than the Render well.
> > > > > >>+		 */
> > > > > >>+		intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
> > > > > >> 		valleyview_set_rps(dev_priv, val);
> > > > > >
> > > > > >Both powerwells are woken for rps. (Taking one but not the other has no
> > > > > >benefit, and very misleading.)
> > > > > 
> > > > > The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the
> > > > > existing code in vlv_set_rps_idle().
> > > > 
> > > > It's not correct there either...
> > > 
> > > Why not? If the render well is in rc6 already we don't want to waste
> > > power by waking it. The only reason we have to wake up *something* is
> > > so that the gpll frequency actually gets changed.
> > 
> > If the register write requires the powerwell, the mmio access will take
> > the powerwell.
> 
> The register write doesn't require a power well. It's a sideband access.
> The punit will simply not change the GPLL frequency if the GPLL is
> currently not running (which will/can happen when all power wells are
> asleep). That in itself doesn't sound too back (why change the
> frequency if the thing isn't even running, right?). But the real problem
> is that the punit will not let the voltage on the rail to drop
> either until the new frequency gets programmed into the GPLL. Hence if
> the GPU goes idle before we've dropped the GPLL frequency to the
> minimum value, we will waste power by having a needlessly high voltage.
> 
> Originally we tried to avoid this problem via vlv_force_gfx_clock(),
> which should just force the GPLL to turn on without powering on
> any power wells. But that caused some spurious WARNs or something
> IIRC, so we had to come up with another workaround. And since powering
> either well is sufficient we chose to use the cheaper media well.

That explains set_idle() (and would be a better comment that the one
there). But not set_rps(), since there we don't care if the write is
delayed until the GPU is next active.
-Chris
Ville Syrjala Jan. 2, 2017, 3:02 p.m. UTC | #9
On Mon, Jan 02, 2017 at 02:53:59PM +0000, Chris Wilson wrote:
> On Mon, Jan 02, 2017 at 04:40:05PM +0200, Ville Syrjälä wrote:
> > On Mon, Jan 02, 2017 at 02:21:58PM +0000, Chris Wilson wrote:
> > > On Mon, Jan 02, 2017 at 04:10:49PM +0200, Ville Syrjälä wrote:
> > > > On Mon, Jan 02, 2017 at 11:37:59AM +0000, Chris Wilson wrote:
> > > > > On Sun, Jan 01, 2017 at 09:48:53PM +0100, Hans de Goede wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On 01-01-17 21:24, Chris Wilson wrote:
> > > > > > >On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote:
> > > > > > >>All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except
> > > > > > >>for intel_set_rps(). Since intel_set_rps can for example be called from
> > > > > > >>sysfs store functions, there is no guarantee this is already done, so add
> > > > > > >>an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps.
> > > > > > >>
> > > > > > >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > > > > >>---
> > > > > > >> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
> > > > > > >> 1 file changed, 7 insertions(+), 2 deletions(-)
> > > > > > >>
> > > > > > >>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > >>index 4b12637..cc4fbd7 100644
> > > > > > >>--- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > > >>+++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > >>@@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
> > > > > > >>
> > > > > > >> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
> > > > > > >> {
> > > > > > >>-	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > > > >>+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > > > > > >>+		/* Wake up the media well, as that takes a lot less
> > > > > > >>+		 * power than the Render well.
> > > > > > >>+		 */
> > > > > > >>+		intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
> > > > > > >> 		valleyview_set_rps(dev_priv, val);
> > > > > > >
> > > > > > >Both powerwells are woken for rps. (Taking one but not the other has no
> > > > > > >benefit, and very misleading.)
> > > > > > 
> > > > > > The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the
> > > > > > existing code in vlv_set_rps_idle().
> > > > > 
> > > > > It's not correct there either...
> > > > 
> > > > Why not? If the render well is in rc6 already we don't want to waste
> > > > power by waking it. The only reason we have to wake up *something* is
> > > > so that the gpll frequency actually gets changed.
> > > 
> > > If the register write requires the powerwell, the mmio access will take
> > > the powerwell.
> > 
> > The register write doesn't require a power well. It's a sideband access.
> > The punit will simply not change the GPLL frequency if the GPLL is
> > currently not running (which will/can happen when all power wells are
> > asleep). That in itself doesn't sound too back (why change the
> > frequency if the thing isn't even running, right?). But the real problem
> > is that the punit will not let the voltage on the rail to drop
> > either until the new frequency gets programmed into the GPLL. Hence if
> > the GPU goes idle before we've dropped the GPLL frequency to the
> > minimum value, we will waste power by having a needlessly high voltage.
> > 
> > Originally we tried to avoid this problem via vlv_force_gfx_clock(),
> > which should just force the GPLL to turn on without powering on
> > any power wells. But that caused some spurious WARNs or something
> > IIRC, so we had to come up with another workaround. And since powering
> > either well is sufficient we chose to use the cheaper media well.
> 
> That explains set_idle() (and would be a better comment that the one
> there). But not set_rps(), since there we don't care if the write is
> delayed until the GPU is next active.

I don't see any forcewakes in set_rps()
Ville Syrjala Jan. 2, 2017, 3:08 p.m. UTC | #10
On Mon, Jan 02, 2017 at 05:02:25PM +0200, Ville Syrjälä wrote:
> On Mon, Jan 02, 2017 at 02:53:59PM +0000, Chris Wilson wrote:
> > On Mon, Jan 02, 2017 at 04:40:05PM +0200, Ville Syrjälä wrote:
> > > On Mon, Jan 02, 2017 at 02:21:58PM +0000, Chris Wilson wrote:
> > > > On Mon, Jan 02, 2017 at 04:10:49PM +0200, Ville Syrjälä wrote:
> > > > > On Mon, Jan 02, 2017 at 11:37:59AM +0000, Chris Wilson wrote:
> > > > > > On Sun, Jan 01, 2017 at 09:48:53PM +0100, Hans de Goede wrote:
> > > > > > > Hi,
> > > > > > > 
> > > > > > > On 01-01-17 21:24, Chris Wilson wrote:
> > > > > > > >On Sun, Jan 01, 2017 at 09:14:02PM +0100, Hans de Goede wrote:
> > > > > > > >>All callers of valleyview_set_rps() get at least FORCEWAKE_MEDIA, except
> > > > > > > >>for intel_set_rps(). Since intel_set_rps can for example be called from
> > > > > > > >>sysfs store functions, there is no guarantee this is already done, so add
> > > > > > > >>an intel_uncore_forcewake_get(FORCEWAKE_MEDIA) call to intel_set_rps.
> > > > > > > >>
> > > > > > > >>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > > > > > >>---
> > > > > > > >> drivers/gpu/drm/i915/intel_pm.c | 9 +++++++--
> > > > > > > >> 1 file changed, 7 insertions(+), 2 deletions(-)
> > > > > > > >>
> > > > > > > >>diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > > >>index 4b12637..cc4fbd7 100644
> > > > > > > >>--- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > > > >>+++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > > >>@@ -5096,9 +5096,14 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv,
> > > > > > > >>
> > > > > > > >> void intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
> > > > > > > >> {
> > > > > > > >>-	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > > > > >>+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
> > > > > > > >>+		/* Wake up the media well, as that takes a lot less
> > > > > > > >>+		 * power than the Render well.
> > > > > > > >>+		 */
> > > > > > > >>+		intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
> > > > > > > >> 		valleyview_set_rps(dev_priv, val);
> > > > > > > >
> > > > > > > >Both powerwells are woken for rps. (Taking one but not the other has no
> > > > > > > >benefit, and very misleading.)
> > > > > > > 
> > > > > > > The comment on why FORCEWAKE_MEDIA is used + code is copy pasted from the
> > > > > > > existing code in vlv_set_rps_idle().
> > > > > > 
> > > > > > It's not correct there either...
> > > > > 
> > > > > Why not? If the render well is in rc6 already we don't want to waste
> > > > > power by waking it. The only reason we have to wake up *something* is
> > > > > so that the gpll frequency actually gets changed.
> > > > 
> > > > If the register write requires the powerwell, the mmio access will take
> > > > the powerwell.
> > > 
> > > The register write doesn't require a power well. It's a sideband access.
> > > The punit will simply not change the GPLL frequency if the GPLL is
> > > currently not running (which will/can happen when all power wells are
> > > asleep). That in itself doesn't sound too back (why change the
> > > frequency if the thing isn't even running, right?). But the real problem
> > > is that the punit will not let the voltage on the rail to drop
> > > either until the new frequency gets programmed into the GPLL. Hence if
> > > the GPU goes idle before we've dropped the GPLL frequency to the
> > > minimum value, we will waste power by having a needlessly high voltage.
> > > 
> > > Originally we tried to avoid this problem via vlv_force_gfx_clock(),
> > > which should just force the GPLL to turn on without powering on
> > > any power wells. But that caused some spurious WARNs or something
> > > IIRC, so we had to come up with another workaround. And since powering
> > > either well is sufficient we chose to use the cheaper media well.
> > 
> > That explains set_idle() (and would be a better comment that the one
> > there). But not set_rps(), since there we don't care if the write is
> > delayed until the GPU is next active.
> 
> I don't see any forcewakes in set_rps()

Ah, it was this patch which wants to add that. And indeed that doesn't
make sense.

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4b12637..cc4fbd7 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5096,9 +5096,14 @@  void gen6_rps_boost(struct drm_i915_private *dev_priv,
 
 void intel_set_rps(struct drm_i915_private *dev_priv, u8 val)
 {
-	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
+	if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) {
+		/* Wake up the media well, as that takes a lot less
+		 * power than the Render well.
+		 */
+		intel_uncore_forcewake_get(dev_priv, FORCEWAKE_MEDIA);
 		valleyview_set_rps(dev_priv, val);
-	else
+		intel_uncore_forcewake_put(dev_priv, FORCEWAKE_MEDIA);
+	} else
 		gen6_set_rps(dev_priv, val);
 }