i915: WARN_ON(val > dev_priv->rps.max_freq_softlimit)

i915: WARN_ON(val > dev_priv->rps.max_freq_softlimit)

[Michael Auchter]

Testing out 3.19-rc6 on my 2014 Thinkpad X1 Carbon (Haswell) resulted in
this WARN at boot (and pretty frequently afterwards):

WARNING: CPU: 0 PID: 989 at drivers/gpu/drm/i915/intel_pm.c:4377 gen6_set_rps+0x371/0x3c0()
WARN_ON(val > dev_priv->rps.max_freq_softlimit)
Modules linked in:
CPU: 0 PID: 989 Comm: kworker/0:2 Not tainted 3.19.0-rc6 #31
Hardware name: LENOVO 20A7002WUS/20A7002WUS, BIOS GRET38WW (1.15 ) 05/29/2014
Workqueue: events intel_gen6_powersave_work
 0000000000000000 ffffffff81a82dd0 ffffffff817f099e ffff88021451bd28
 ffffffff8107d107 ffff8802148e0000 0000000000000022 ffff8802148e86f0
 ffff88021498f000 0000000000040000 ffffffff8107d185 ffffffff81a83448
Call Trace:
 [<ffffffff817f099e>] ? dump_stack+0x40/0x50
 [<ffffffff8107d107>] ? warn_slowpath_common+0x77/0xb0
 [<ffffffff8107d185>] ? warn_slowpath_fmt+0x45/0x50
 [<ffffffff813c6ff1>] ? gen6_set_rps+0x371/0x3c0
 [<ffffffff813caa10>] ? intel_gen6_powersave_work+0x780/0x1180
 [<ffffffff81090c50>] ? process_one_work+0x130/0x350
 [<ffffffff81091274>] ? worker_thread+0x114/0x450
 [<ffffffff81091160>] ? rescuer_thread+0x2f0/0x2f0
 [<ffffffff810954ec>] ? kthread+0xbc/0xe0
 [<ffffffff81095430>] ? kthread_create_on_node+0x170/0x170
 [<ffffffff817f86ac>] ? ret_from_fork+0x7c/0xb0
 [<ffffffff81095430>] ? kthread_create_on_node+0x170/0x170
---[ end trace c3ac159c87b9b234 ]---

I bisected this back to:

commit 93ee29203f506582cca2bcec5f05041526d9ab0a
Author: Tom O'Rourke <Tom.O'Rourke@intel.com>
Date:   Wed Nov 19 14:21:52 2014 -0800

    drm/i915: Use efficient frequency for HSW/BDW

    Added gen6_init_rps_frequencies() to initialize
    the rps frequency values.  This function replaces
    parse_rp_state_cap().  In addition to reading RPn,
    RP0, and RP1 from RP_STATE_CAP register, the new
    function reads efficient frequency (aka RPe) from
    pcode for Haswell and Broadwell and sets the turbo
    softlimits.  The turbo minimum frequency softlimit
    is set to RPe for Haswell and Broadwell and to RPn
    otherwise.

    For RPe, the efficiency is based on the frequency/power
    ratio (MHz/W); this is considering GT power and not
    package power.  The efficent frequency is the highest
    frequency for which the frequency/power ratio is within
    some threshold of the highest frequency/power ratio.
    A fixed decrease in frequency results in smaller
    decrease in power at frequencies less than RPe than
    at frequencies above RPe.

    v2: Following suggestions from Chris Wilson and
    Daniel Vetter to extend and rename parse_rp_state_cap
    and to open-code a poorly named function.

    Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
    Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
    [danvet: Remove unused variables.]
    Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I'm not at all familiar with this hardware, but I took a quick look into
what changed with this commit for my laptop. Before the commit,
rps.min_freq_softlimit is 4 (from rps.min_freq) and
rps.max_freq_softlimit is 22.

After the commit, rps.min_freq_softlimit is set to the
rps.efficient_freq value read from pcode, which is 34 on my laptop.
So later when gen6_set_rps() is called with rps.min_freq_softlimit that
warning is hit.

Any thoughts? It certainly seems fishy that this commit causes
rps.min_freq_softlimit to be greater than rps.max_freq_softlimit.

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

Re: i915: WARN_ON(val > dev_priv->rps.max_freq_softlimit)

[Daniel Vetter]

On Wed, Jan 28, 2015 at 12:43:21AM -0500, Michael Auchter wrote:
> Testing out 3.19-rc6 on my 2014 Thinkpad X1 Carbon (Haswell) resulted in
> this WARN at boot (and pretty frequently afterwards):
> 
> WARNING: CPU: 0 PID: 989 at drivers/gpu/drm/i915/intel_pm.c:4377 gen6_set_rps+0x371/0x3c0()
> WARN_ON(val > dev_priv->rps.max_freq_softlimit)
> Modules linked in:
> CPU: 0 PID: 989 Comm: kworker/0:2 Not tainted 3.19.0-rc6 #31
> Hardware name: LENOVO 20A7002WUS/20A7002WUS, BIOS GRET38WW (1.15 ) 05/29/2014
> Workqueue: events intel_gen6_powersave_work
>  0000000000000000 ffffffff81a82dd0 ffffffff817f099e ffff88021451bd28
>  ffffffff8107d107 ffff8802148e0000 0000000000000022 ffff8802148e86f0
>  ffff88021498f000 0000000000040000 ffffffff8107d185 ffffffff81a83448
> Call Trace:
>  [<ffffffff817f099e>] ? dump_stack+0x40/0x50
>  [<ffffffff8107d107>] ? warn_slowpath_common+0x77/0xb0
>  [<ffffffff8107d185>] ? warn_slowpath_fmt+0x45/0x50
>  [<ffffffff813c6ff1>] ? gen6_set_rps+0x371/0x3c0
>  [<ffffffff813caa10>] ? intel_gen6_powersave_work+0x780/0x1180
>  [<ffffffff81090c50>] ? process_one_work+0x130/0x350
>  [<ffffffff81091274>] ? worker_thread+0x114/0x450
>  [<ffffffff81091160>] ? rescuer_thread+0x2f0/0x2f0
>  [<ffffffff810954ec>] ? kthread+0xbc/0xe0
>  [<ffffffff81095430>] ? kthread_create_on_node+0x170/0x170
>  [<ffffffff817f86ac>] ? ret_from_fork+0x7c/0xb0
>  [<ffffffff81095430>] ? kthread_create_on_node+0x170/0x170
> ---[ end trace c3ac159c87b9b234 ]---
> 
> I bisected this back to:
> 
> commit 93ee29203f506582cca2bcec5f05041526d9ab0a
> Author: Tom O'Rourke <Tom.O'Rourke@intel.com>
> Date:   Wed Nov 19 14:21:52 2014 -0800
> 
>     drm/i915: Use efficient frequency for HSW/BDW
> 
>     Added gen6_init_rps_frequencies() to initialize
>     the rps frequency values.  This function replaces
>     parse_rp_state_cap().  In addition to reading RPn,
>     RP0, and RP1 from RP_STATE_CAP register, the new
>     function reads efficient frequency (aka RPe) from
>     pcode for Haswell and Broadwell and sets the turbo
>     softlimits.  The turbo minimum frequency softlimit
>     is set to RPe for Haswell and Broadwell and to RPn
>     otherwise.
> 
>     For RPe, the efficiency is based on the frequency/power
>     ratio (MHz/W); this is considering GT power and not
>     package power.  The efficent frequency is the highest
>     frequency for which the frequency/power ratio is within
>     some threshold of the highest frequency/power ratio.
>     A fixed decrease in frequency results in smaller
>     decrease in power at frequencies less than RPe than
>     at frequencies above RPe.
> 
>     v2: Following suggestions from Chris Wilson and
>     Daniel Vetter to extend and rename parse_rp_state_cap
>     and to open-code a poorly named function.
> 
>     Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
>     Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>     [danvet: Remove unused variables.]
>     Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> I'm not at all familiar with this hardware, but I took a quick look into
> what changed with this commit for my laptop. Before the commit,
> rps.min_freq_softlimit is 4 (from rps.min_freq) and
> rps.max_freq_softlimit is 22.
> 
> After the commit, rps.min_freq_softlimit is set to the
> rps.efficient_freq value read from pcode, which is 34 on my laptop.
> So later when gen6_set_rps() is called with rps.min_freq_softlimit that
> warning is hit.
> 
> Any thoughts? It certainly seems fishy that this commit causes
> rps.min_freq_softlimit to be greater than rps.max_freq_softlimit.

Sounds like the rpe value on your firmware is rather bogus, and we
probably need to sanity-check it. Tom?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: i915: WARN_ON(val > dev_priv->rps.max_freq_softlimit)

[Chris Wilson]

On Wed, Jan 28, 2015 at 12:43:21AM -0500, Michael Auchter wrote:
> Testing out 3.19-rc6 on my 2014 Thinkpad X1 Carbon (Haswell) resulted in
> this WARN at boot (and pretty frequently afterwards):
> 
> WARNING: CPU: 0 PID: 989 at drivers/gpu/drm/i915/intel_pm.c:4377 gen6_set_rps+0x371/0x3c0()
> WARN_ON(val > dev_priv->rps.max_freq_softlimit)

[snip]
 
> I'm not at all familiar with this hardware, but I took a quick look into
> what changed with this commit for my laptop. Before the commit,
> rps.min_freq_softlimit is 4 (from rps.min_freq) and
> rps.max_freq_softlimit is 22.
> 
> After the commit, rps.min_freq_softlimit is set to the
> rps.efficient_freq value read from pcode, which is 34 on my laptop.
> So later when gen6_set_rps() is called with rps.min_freq_softlimit that
> warning is hit.
> 
> Any thoughts? It certainly seems fishy that this commit causes
> rps.min_freq_softlimit to be greater than rps.max_freq_softlimit.

Very fishy indeed. Moral of this story, never trust hw.

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 3e630feb18e4..bbedd2901c54 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4007,7 +4007,10 @@ static void gen6_init_rps_frequencies(struct drm_device *dev)
                                        &ddcc_status);
                if (0 == ret)
                        dev_priv->rps.efficient_freq =
-                               (ddcc_status >> 8) & 0xff;
+                               clamp_t(u8,
+                                       (ddcc_status >> 8) & 0xff,
+                                       dev_priv->rps.min_freq,
+                                       dev_priv->rps.max_freq);
        }
 
        /* Preserve min/max settings in case of re-init */

But really it is probably just best to disable the query for hsw:

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 3e630feb18e4..01bd508e81f6 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4001,7 +4001,7 @@ static void gen6_init_rps_frequencies(struct drm_device *dev)
        dev_priv->rps.max_freq          = dev_priv->rps.rp0_freq;
 
        dev_priv->rps.efficient_freq = dev_priv->rps.rp1_freq;
-       if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
+       if (IS_BROADWELL(dev)) {
                ret = sandybridge_pcode_read(dev_priv,
                                        HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL,
                                        &ddcc_status);

Paranoia says we do both.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: i915: WARN_ON(val > dev_priv->rps.max_freq_softlimit)

[Ville Syrjälä]

On Wed, Jan 28, 2015 at 09:58:15AM +0000, Chris Wilson wrote:
> On Wed, Jan 28, 2015 at 12:43:21AM -0500, Michael Auchter wrote:
> > Testing out 3.19-rc6 on my 2014 Thinkpad X1 Carbon (Haswell) resulted in
> > this WARN at boot (and pretty frequently afterwards):
> > 
> > WARNING: CPU: 0 PID: 989 at drivers/gpu/drm/i915/intel_pm.c:4377 gen6_set_rps+0x371/0x3c0()
> > WARN_ON(val > dev_priv->rps.max_freq_softlimit)
> 
> [snip]
>  
> > I'm not at all familiar with this hardware, but I took a quick look into
> > what changed with this commit for my laptop. Before the commit,
> > rps.min_freq_softlimit is 4 (from rps.min_freq) and
> > rps.max_freq_softlimit is 22.
> > 
> > After the commit, rps.min_freq_softlimit is set to the
> > rps.efficient_freq value read from pcode, which is 34 on my laptop.
> > So later when gen6_set_rps() is called with rps.min_freq_softlimit that
> > warning is hit.
> > 
> > Any thoughts? It certainly seems fishy that this commit causes
> > rps.min_freq_softlimit to be greater than rps.max_freq_softlimit.
> 
> Very fishy indeed. Moral of this story, never trust hw.
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 3e630feb18e4..bbedd2901c54 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4007,7 +4007,10 @@ static void gen6_init_rps_frequencies(struct drm_device *dev)
>                                         &ddcc_status);
>                 if (0 == ret)
>                         dev_priv->rps.efficient_freq =
> -                               (ddcc_status >> 8) & 0xff;
> +                               clamp_t(u8,
> +                                       (ddcc_status >> 8) & 0xff,
> +                                       dev_priv->rps.min_freq,
> +                                       dev_priv->rps.max_freq);

Maybe better to fall back to rp1_freq if this is bogus?

>         }
>  
>         /* Preserve min/max settings in case of re-init */
> 
> But really it is probably just best to disable the query for hsw:
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 3e630feb18e4..01bd508e81f6 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4001,7 +4001,7 @@ static void gen6_init_rps_frequencies(struct drm_device *dev)
>         dev_priv->rps.max_freq          = dev_priv->rps.rp0_freq;
>  
>         dev_priv->rps.efficient_freq = dev_priv->rps.rp1_freq;
> -       if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> +       if (IS_BROADWELL(dev)) {
>                 ret = sandybridge_pcode_read(dev_priv,
>                                         HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL,
>                                         &ddcc_status);
> 
> Paranoia says we do both.
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: i915: WARN_ON(val > dev_priv->rps.max_freq_softlimit)

[O'Rourke, Tom]

On Wed, Jan 28, 2015 at 01:28:58PM +0200, Ville Syrjälä wrote:
> On Wed, Jan 28, 2015 at 09:58:15AM +0000, Chris Wilson wrote:
> > On Wed, Jan 28, 2015 at 12:43:21AM -0500, Michael Auchter wrote:
> > > Testing out 3.19-rc6 on my 2014 Thinkpad X1 Carbon (Haswell) resulted in
> > > this WARN at boot (and pretty frequently afterwards):
> > > 
> > > WARNING: CPU: 0 PID: 989 at drivers/gpu/drm/i915/intel_pm.c:4377 gen6_set_rps+0x371/0x3c0()
> > > WARN_ON(val > dev_priv->rps.max_freq_softlimit)
> > 
> > [snip]
> >  
> > > I'm not at all familiar with this hardware, but I took a quick look into
> > > what changed with this commit for my laptop. Before the commit,
> > > rps.min_freq_softlimit is 4 (from rps.min_freq) and
> > > rps.max_freq_softlimit is 22.
> > > 
> > > After the commit, rps.min_freq_softlimit is set to the
> > > rps.efficient_freq value read from pcode, which is 34 on my laptop.
> > > So later when gen6_set_rps() is called with rps.min_freq_softlimit that
> > > warning is hit.
> > > 
> > > Any thoughts? It certainly seems fishy that this commit causes
> > > rps.min_freq_softlimit to be greater than rps.max_freq_softlimit.
> > 
> > Very fishy indeed. Moral of this story, never trust hw.
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 3e630feb18e4..bbedd2901c54 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4007,7 +4007,10 @@ static void gen6_init_rps_frequencies(struct drm_device *dev)
> >                                         &ddcc_status);
> >                 if (0 == ret)
> >                         dev_priv->rps.efficient_freq =
> > -                               (ddcc_status >> 8) & 0xff;
> > +                               clamp_t(u8,
> > +                                       (ddcc_status >> 8) & 0xff,
> > +                                       dev_priv->rps.min_freq,
> > +                                       dev_priv->rps.max_freq);
> 
> Maybe better to fall back to rp1_freq if this is bogus?
>
[TOR:] Michael, Thank you for bringing this problem to our attention.

Yes, this function needs some range checking to maintain
RPn <= RPe <= RP0.

A value of 34 seems too high for RPe.  
What values does the Carbon X1 (Haswell) have for RPn and RP0?

I agree with Ville that a bogus value from the pcode read should 
not be used.  Simple clamping would set the min_freq to RP0; 
probably incorrect.

Tom O'Rourke
 
> >         }
> >  
> >         /* Preserve min/max settings in case of re-init */
> > 
> > But really it is probably just best to disable the query for hsw:
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 3e630feb18e4..01bd508e81f6 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4001,7 +4001,7 @@ static void gen6_init_rps_frequencies(struct drm_device *dev)
> >         dev_priv->rps.max_freq          = dev_priv->rps.rp0_freq;
> >  
> >         dev_priv->rps.efficient_freq = dev_priv->rps.rp1_freq;
> > -       if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> > +       if (IS_BROADWELL(dev)) {
> >                 ret = sandybridge_pcode_read(dev_priv,
> >                                         HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL,
> >                                         &ddcc_status);
> > 
> > Paranoia says we do both.
> > -Chris
> > 
> > -- 
> > Chris Wilson, Intel Open Source Technology Centre
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: i915: WARN_ON(val > dev_priv->rps.max_freq_softlimit)

[Michael Auchter]

On Wed, Jan 28, 2015 at 10:36:02PM -0800, O'Rourke, Tom wrote:
> On Wed, Jan 28, 2015 at 01:28:58PM +0200, Ville Syrjälä wrote:
> > On Wed, Jan 28, 2015 at 09:58:15AM +0000, Chris Wilson wrote:
> > > On Wed, Jan 28, 2015 at 12:43:21AM -0500, Michael Auchter wrote:
> > > > Testing out 3.19-rc6 on my 2014 Thinkpad X1 Carbon (Haswell) resulted in
> > > > this WARN at boot (and pretty frequently afterwards):
> > > > 
> > > > WARNING: CPU: 0 PID: 989 at drivers/gpu/drm/i915/intel_pm.c:4377 gen6_set_rps+0x371/0x3c0()
> > > > WARN_ON(val > dev_priv->rps.max_freq_softlimit)
> > > 
> > > [snip]
> > >  
> > > > I'm not at all familiar with this hardware, but I took a quick look into
> > > > what changed with this commit for my laptop. Before the commit,
> > > > rps.min_freq_softlimit is 4 (from rps.min_freq) and
> > > > rps.max_freq_softlimit is 22.
> > > > 
> > > > After the commit, rps.min_freq_softlimit is set to the
> > > > rps.efficient_freq value read from pcode, which is 34 on my laptop.
> > > > So later when gen6_set_rps() is called with rps.min_freq_softlimit that
> > > > warning is hit.
> > > > 
> > > > Any thoughts? It certainly seems fishy that this commit causes
> > > > rps.min_freq_softlimit to be greater than rps.max_freq_softlimit.
> > > 
> > > Very fishy indeed. Moral of this story, never trust hw.
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 3e630feb18e4..bbedd2901c54 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -4007,7 +4007,10 @@ static void gen6_init_rps_frequencies(struct drm_device *dev)
> > >                                         &ddcc_status);
> > >                 if (0 == ret)
> > >                         dev_priv->rps.efficient_freq =
> > > -                               (ddcc_status >> 8) & 0xff;
> > > +                               clamp_t(u8,
> > > +                                       (ddcc_status >> 8) & 0xff,
> > > +                                       dev_priv->rps.min_freq,
> > > +                                       dev_priv->rps.max_freq);
> > 
> > Maybe better to fall back to rp1_freq if this is bogus?
> >
> [TOR:] Michael, Thank you for bringing this problem to our attention.
> 
> Yes, this function needs some range checking to maintain
> RPn <= RPe <= RP0.
> 
> A value of 34 seems too high for RPe.  
> What values does the Carbon X1 (Haswell) have for RPn and RP0?
 
RPn is 4, and RP0 is 22.

> I agree with Ville that a bogus value from the pcode read should 
> not be used.  Simple clamping would set the min_freq to RP0; 
> probably incorrect.
> 
> Tom O'Rourke
>  
> > >         }
> > >  
> > >         /* Preserve min/max settings in case of re-init */
> > > 
> > > But really it is probably just best to disable the query for hsw:
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 3e630feb18e4..01bd508e81f6 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -4001,7 +4001,7 @@ static void gen6_init_rps_frequencies(struct drm_device *dev)
> > >         dev_priv->rps.max_freq          = dev_priv->rps.rp0_freq;
> > >  
> > >         dev_priv->rps.efficient_freq = dev_priv->rps.rp1_freq;
> > > -       if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> > > +       if (IS_BROADWELL(dev)) {
> > >                 ret = sandybridge_pcode_read(dev_priv,
> > >                                         HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL,
> > >                                         &ddcc_status);
> > > 
> > > Paranoia says we do both.
> > > -Chris
> > > 
> > > -- 
> > > Chris Wilson, Intel Open Source Technology Centre
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: i915: WARN_ON(val > dev_priv->rps.max_freq_softlimit)

[Daniel Vetter]

On Wed, Jan 28, 2015 at 10:36:02PM -0800, O'Rourke, Tom wrote:
> On Wed, Jan 28, 2015 at 01:28:58PM +0200, Ville Syrjälä wrote:
> > On Wed, Jan 28, 2015 at 09:58:15AM +0000, Chris Wilson wrote:
> > > On Wed, Jan 28, 2015 at 12:43:21AM -0500, Michael Auchter wrote:
> > > > Testing out 3.19-rc6 on my 2014 Thinkpad X1 Carbon (Haswell) resulted in
> > > > this WARN at boot (and pretty frequently afterwards):
> > > > 
> > > > WARNING: CPU: 0 PID: 989 at drivers/gpu/drm/i915/intel_pm.c:4377 gen6_set_rps+0x371/0x3c0()
> > > > WARN_ON(val > dev_priv->rps.max_freq_softlimit)
> > > 
> > > [snip]
> > >  
> > > > I'm not at all familiar with this hardware, but I took a quick look into
> > > > what changed with this commit for my laptop. Before the commit,
> > > > rps.min_freq_softlimit is 4 (from rps.min_freq) and
> > > > rps.max_freq_softlimit is 22.
> > > > 
> > > > After the commit, rps.min_freq_softlimit is set to the
> > > > rps.efficient_freq value read from pcode, which is 34 on my laptop.
> > > > So later when gen6_set_rps() is called with rps.min_freq_softlimit that
> > > > warning is hit.
> > > > 
> > > > Any thoughts? It certainly seems fishy that this commit causes
> > > > rps.min_freq_softlimit to be greater than rps.max_freq_softlimit.
> > > 
> > > Very fishy indeed. Moral of this story, never trust hw.
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 3e630feb18e4..bbedd2901c54 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -4007,7 +4007,10 @@ static void gen6_init_rps_frequencies(struct drm_device *dev)
> > >                                         &ddcc_status);
> > >                 if (0 == ret)
> > >                         dev_priv->rps.efficient_freq =
> > > -                               (ddcc_status >> 8) & 0xff;
> > > +                               clamp_t(u8,
> > > +                                       (ddcc_status >> 8) & 0xff,
> > > +                                       dev_priv->rps.min_freq,
> > > +                                       dev_priv->rps.max_freq);
> > 
> > Maybe better to fall back to rp1_freq if this is bogus?
> >
> [TOR:] Michael, Thank you for bringing this problem to our attention.
> 
> Yes, this function needs some range checking to maintain
> RPn <= RPe <= RP0.
> 
> A value of 34 seems too high for RPe.  
> What values does the Carbon X1 (Haswell) have for RPn and RP0?

4 & 22, already in Micheal's original bug report.

Tom, can you pls polish the clamping into a proper patch with m-l
references?

Micheal, can you please test the first hunk from Chris (the one that adds
the clamp) to make sure it does indeed address the WARN_ON you're seeing?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: i915: WARN_ON(val > dev_priv->rps.max_freq_softlimit)

[Michael Auchter]

On Thu, Jan 29, 2015 at 06:12:31PM +0100, Daniel Vetter wrote:
> On Wed, Jan 28, 2015 at 10:36:02PM -0800, O'Rourke, Tom wrote:
> > On Wed, Jan 28, 2015 at 01:28:58PM +0200, Ville Syrjälä wrote:
> > > On Wed, Jan 28, 2015 at 09:58:15AM +0000, Chris Wilson wrote:
> > > > On Wed, Jan 28, 2015 at 12:43:21AM -0500, Michael Auchter wrote:
> > > > > Testing out 3.19-rc6 on my 2014 Thinkpad X1 Carbon (Haswell) resulted in
> > > > > this WARN at boot (and pretty frequently afterwards):
> > > > > 
> > > > > WARNING: CPU: 0 PID: 989 at drivers/gpu/drm/i915/intel_pm.c:4377 gen6_set_rps+0x371/0x3c0()
> > > > > WARN_ON(val > dev_priv->rps.max_freq_softlimit)
> > > > 
> > > > [snip]
> > > >  
> > > > > I'm not at all familiar with this hardware, but I took a quick look into
> > > > > what changed with this commit for my laptop. Before the commit,
> > > > > rps.min_freq_softlimit is 4 (from rps.min_freq) and
> > > > > rps.max_freq_softlimit is 22.
> > > > > 
> > > > > After the commit, rps.min_freq_softlimit is set to the
> > > > > rps.efficient_freq value read from pcode, which is 34 on my laptop.
> > > > > So later when gen6_set_rps() is called with rps.min_freq_softlimit that
> > > > > warning is hit.
> > > > > 
> > > > > Any thoughts? It certainly seems fishy that this commit causes
> > > > > rps.min_freq_softlimit to be greater than rps.max_freq_softlimit.
> > > > 
> > > > Very fishy indeed. Moral of this story, never trust hw.
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > index 3e630feb18e4..bbedd2901c54 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -4007,7 +4007,10 @@ static void gen6_init_rps_frequencies(struct drm_device *dev)
> > > >                                         &ddcc_status);
> > > >                 if (0 == ret)
> > > >                         dev_priv->rps.efficient_freq =
> > > > -                               (ddcc_status >> 8) & 0xff;
> > > > +                               clamp_t(u8,
> > > > +                                       (ddcc_status >> 8) & 0xff,
> > > > +                                       dev_priv->rps.min_freq,
> > > > +                                       dev_priv->rps.max_freq);
> > > 
> > > Maybe better to fall back to rp1_freq if this is bogus?
> > >
> > [TOR:] Michael, Thank you for bringing this problem to our attention.
> > 
> > Yes, this function needs some range checking to maintain
> > RPn <= RPe <= RP0.
> > 
> > A value of 34 seems too high for RPe.  
> > What values does the Carbon X1 (Haswell) have for RPn and RP0?
> 
> 4 & 22, already in Micheal's original bug report.
> 
> Tom, can you pls polish the clamping into a proper patch with m-l
> references?
> 
> Micheal, can you please test the first hunk from Chris (the one that adds
> the clamp) to make sure it does indeed address the WARN_ON you're seeing?

The clamp suggested by Chris does indeed fix the WARN_ON.

In the case where RPe is greater than RP0, RPe will now be clamped to
RP0. Is this likely to result in increased power consumption?

At a quick glance on my laptop it does not (idling around 5W before and
after) but Ville had suggested earlier to fall back to RP1, which would
be more consistent with previous kernels.

Thanks again for the quick responses,
 Michael
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: i915: WARN_ON(val > dev_priv->rps.max_freq_softlimit)

[O'Rourke, Tom]

On Thu, Jan 29, 2015 at 08:56:06PM -0500, Michael Auchter wrote:
> On Thu, Jan 29, 2015 at 06:12:31PM +0100, Daniel Vetter wrote:
> > On Wed, Jan 28, 2015 at 10:36:02PM -0800, O'Rourke, Tom wrote:
> > > On Wed, Jan 28, 2015 at 01:28:58PM +0200, Ville Syrjälä wrote:
> > > > On Wed, Jan 28, 2015 at 09:58:15AM +0000, Chris Wilson wrote:
> > > > > On Wed, Jan 28, 2015 at 12:43:21AM -0500, Michael Auchter wrote:
> > > > > > Testing out 3.19-rc6 on my 2014 Thinkpad X1 Carbon (Haswell) resulted in
> > > > > > this WARN at boot (and pretty frequently afterwards):
> > > > > > 
> > > > > > WARNING: CPU: 0 PID: 989 at drivers/gpu/drm/i915/intel_pm.c:4377 gen6_set_rps+0x371/0x3c0()
> > > > > > WARN_ON(val > dev_priv->rps.max_freq_softlimit)
> > > > > 
> > > > > [snip]
> > > > >  
> > > > > > I'm not at all familiar with this hardware, but I took a quick look into
> > > > > > what changed with this commit for my laptop. Before the commit,
> > > > > > rps.min_freq_softlimit is 4 (from rps.min_freq) and
> > > > > > rps.max_freq_softlimit is 22.
> > > > > > 
> > > > > > After the commit, rps.min_freq_softlimit is set to the
> > > > > > rps.efficient_freq value read from pcode, which is 34 on my laptop.
> > > > > > So later when gen6_set_rps() is called with rps.min_freq_softlimit that
> > > > > > warning is hit.
> > > > > > 
> > > > > > Any thoughts? It certainly seems fishy that this commit causes
> > > > > > rps.min_freq_softlimit to be greater than rps.max_freq_softlimit.
> > > > > 
> > > > > Very fishy indeed. Moral of this story, never trust hw.
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > > index 3e630feb18e4..bbedd2901c54 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > @@ -4007,7 +4007,10 @@ static void gen6_init_rps_frequencies(struct drm_device *dev)
> > > > >                                         &ddcc_status);
> > > > >                 if (0 == ret)
> > > > >                         dev_priv->rps.efficient_freq =
> > > > > -                               (ddcc_status >> 8) & 0xff;
> > > > > +                               clamp_t(u8,
> > > > > +                                       (ddcc_status >> 8) & 0xff,
> > > > > +                                       dev_priv->rps.min_freq,
> > > > > +                                       dev_priv->rps.max_freq);
> > > > 
> > > > Maybe better to fall back to rp1_freq if this is bogus?
> > > >
> > > [TOR:] Michael, Thank you for bringing this problem to our attention.
> > > 
> > > Yes, this function needs some range checking to maintain
> > > RPn <= RPe <= RP0.
> > > 
> > > A value of 34 seems too high for RPe.  
> > > What values does the Carbon X1 (Haswell) have for RPn and RP0?
> > 
> > 4 & 22, already in Micheal's original bug report.
> > 
> > Tom, can you pls polish the clamping into a proper patch with m-l
> > references?
[TOR:] Yes, I will submit a polished patch with proper range checking.

> > 
> > Micheal, can you please test the first hunk from Chris (the one that adds
> > the clamp) to make sure it does indeed address the WARN_ON you're seeing?
> 
> The clamp suggested by Chris does indeed fix the WARN_ON.
> 
> In the case where RPe is greater than RP0, RPe will now be clamped to
> RP0. Is this likely to result in increased power consumption? 
> 
> At a quick glance on my laptop it does not (idling around 5W before and
> after) but Ville had suggested earlier to fall back to RP1, which would
> be more consistent with previous kernels.
> 
> Thanks again for the quick responses,
>  Michael
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: i915: WARN_ON(val > dev_priv->rps.max_freq_softlimit)

[O'Rourke, Tom]

On Thu, Jan 29, 2015 at 08:56:06PM -0500, Michael Auchter wrote:
> On Thu, Jan 29, 2015 at 06:12:31PM +0100, Daniel Vetter wrote:
> > On Wed, Jan 28, 2015 at 10:36:02PM -0800, O'Rourke, Tom wrote:
> > > On Wed, Jan 28, 2015 at 01:28:58PM +0200, Ville Syrjälä wrote:
> > > > On Wed, Jan 28, 2015 at 09:58:15AM +0000, Chris Wilson wrote:
> > > > > On Wed, Jan 28, 2015 at 12:43:21AM -0500, Michael Auchter wrote:
> > > > > > Testing out 3.19-rc6 on my 2014 Thinkpad X1 Carbon (Haswell) resulted in
> > > > > > this WARN at boot (and pretty frequently afterwards):
> > > > > > 
> > > > > > WARNING: CPU: 0 PID: 989 at drivers/gpu/drm/i915/intel_pm.c:4377 gen6_set_rps+0x371/0x3c0()
> > > > > > WARN_ON(val > dev_priv->rps.max_freq_softlimit)
> > > > > 
> > > > > [snip]
> > > > >  
> > > > > > I'm not at all familiar with this hardware, but I took a quick look into
> > > > > > what changed with this commit for my laptop. Before the commit,
> > > > > > rps.min_freq_softlimit is 4 (from rps.min_freq) and
> > > > > > rps.max_freq_softlimit is 22.
> > > > > > 
> > > > > > After the commit, rps.min_freq_softlimit is set to the
> > > > > > rps.efficient_freq value read from pcode, which is 34 on my laptop.
> > > > > > So later when gen6_set_rps() is called with rps.min_freq_softlimit that
> > > > > > warning is hit.
> > > > > > 
> > > > > > Any thoughts? It certainly seems fishy that this commit causes
> > > > > > rps.min_freq_softlimit to be greater than rps.max_freq_softlimit.
> > > > > 
> > > > > Very fishy indeed. Moral of this story, never trust hw.
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > > index 3e630feb18e4..bbedd2901c54 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > @@ -4007,7 +4007,10 @@ static void gen6_init_rps_frequencies(struct drm_device *dev)
> > > > >                                         &ddcc_status);
> > > > >                 if (0 == ret)
> > > > >                         dev_priv->rps.efficient_freq =
> > > > > -                               (ddcc_status >> 8) & 0xff;
> > > > > +                               clamp_t(u8,
> > > > > +                                       (ddcc_status >> 8) & 0xff,
> > > > > +                                       dev_priv->rps.min_freq,
> > > > > +                                       dev_priv->rps.max_freq);
> > > > 
> > > > Maybe better to fall back to rp1_freq if this is bogus?
> > > >
> > > [TOR:] Michael, Thank you for bringing this problem to our attention.
> > > 
> > > Yes, this function needs some range checking to maintain
> > > RPn <= RPe <= RP0.
> > > 
> > > A value of 34 seems too high for RPe.  
> > > What values does the Carbon X1 (Haswell) have for RPn and RP0?
> > 
> > 4 & 22, already in Micheal's original bug report.
> > 
> > Tom, can you pls polish the clamping into a proper patch with m-l
> > references?
> > 
> > Micheal, can you please test the first hunk from Chris (the one that adds
> > the clamp) to make sure it does indeed address the WARN_ON you're seeing?
> 
> The clamp suggested by Chris does indeed fix the WARN_ON.
> 
> In the case where RPe is greater than RP0, RPe will now be clamped to
> RP0. Is this likely to result in increased power consumption?
> 
> At a quick glance on my laptop it does not (idling around 5W before and
> after) but Ville had suggested earlier to fall back to RP1, which would
> be more consistent with previous kernels.
> 
> Thanks again for the quick responses,
>  Michael

[TOR:] Michael,  I discussed this report with a pcode architect here.

The RPe value is clamped to the [RPn, RP0] range by pcode before
returning the value to the driver on Broadwell but not on Haswell.

On Haswell, an efficient frequency value above RP0 is not a garbage
value and could result from a relatively flat efficiency curve.  In
this situation, leakage power would dominate the efficiency curve
such that running at lower frequencies may not save power overall.
Higher leakage power may result from a higher package temperature.

Running at RP0 may actually save power compared to RP1 by allowing
more time in RC6.  

Thanks,
Tom O'Rourke
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: i915: WARN_ON(val > dev_priv->rps.max_freq_softlimit)

[Daniel Vetter]

On Tue, Feb 10, 2015 at 10:26:02PM -0800, O'Rourke, Tom wrote:
> On Thu, Jan 29, 2015 at 08:56:06PM -0500, Michael Auchter wrote:
> > On Thu, Jan 29, 2015 at 06:12:31PM +0100, Daniel Vetter wrote:
> > > On Wed, Jan 28, 2015 at 10:36:02PM -0800, O'Rourke, Tom wrote:
> > > > On Wed, Jan 28, 2015 at 01:28:58PM +0200, Ville Syrjälä wrote:
> > > > > On Wed, Jan 28, 2015 at 09:58:15AM +0000, Chris Wilson wrote:
> > > > > > On Wed, Jan 28, 2015 at 12:43:21AM -0500, Michael Auchter wrote:
> > > > > > > Testing out 3.19-rc6 on my 2014 Thinkpad X1 Carbon (Haswell) resulted in
> > > > > > > this WARN at boot (and pretty frequently afterwards):
> > > > > > > 
> > > > > > > WARNING: CPU: 0 PID: 989 at drivers/gpu/drm/i915/intel_pm.c:4377 gen6_set_rps+0x371/0x3c0()
> > > > > > > WARN_ON(val > dev_priv->rps.max_freq_softlimit)
> > > > > > 
> > > > > > [snip]
> > > > > >  
> > > > > > > I'm not at all familiar with this hardware, but I took a quick look into
> > > > > > > what changed with this commit for my laptop. Before the commit,
> > > > > > > rps.min_freq_softlimit is 4 (from rps.min_freq) and
> > > > > > > rps.max_freq_softlimit is 22.
> > > > > > > 
> > > > > > > After the commit, rps.min_freq_softlimit is set to the
> > > > > > > rps.efficient_freq value read from pcode, which is 34 on my laptop.
> > > > > > > So later when gen6_set_rps() is called with rps.min_freq_softlimit that
> > > > > > > warning is hit.
> > > > > > > 
> > > > > > > Any thoughts? It certainly seems fishy that this commit causes
> > > > > > > rps.min_freq_softlimit to be greater than rps.max_freq_softlimit.
> > > > > > 
> > > > > > Very fishy indeed. Moral of this story, never trust hw.
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > index 3e630feb18e4..bbedd2901c54 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > @@ -4007,7 +4007,10 @@ static void gen6_init_rps_frequencies(struct drm_device *dev)
> > > > > >                                         &ddcc_status);
> > > > > >                 if (0 == ret)
> > > > > >                         dev_priv->rps.efficient_freq =
> > > > > > -                               (ddcc_status >> 8) & 0xff;
> > > > > > +                               clamp_t(u8,
> > > > > > +                                       (ddcc_status >> 8) & 0xff,
> > > > > > +                                       dev_priv->rps.min_freq,
> > > > > > +                                       dev_priv->rps.max_freq);
> > > > > 
> > > > > Maybe better to fall back to rp1_freq if this is bogus?
> > > > >
> > > > [TOR:] Michael, Thank you for bringing this problem to our attention.
> > > > 
> > > > Yes, this function needs some range checking to maintain
> > > > RPn <= RPe <= RP0.
> > > > 
> > > > A value of 34 seems too high for RPe.  
> > > > What values does the Carbon X1 (Haswell) have for RPn and RP0?
> > > 
> > > 4 & 22, already in Micheal's original bug report.
> > > 
> > > Tom, can you pls polish the clamping into a proper patch with m-l
> > > references?
> > > 
> > > Micheal, can you please test the first hunk from Chris (the one that adds
> > > the clamp) to make sure it does indeed address the WARN_ON you're seeing?
> > 
> > The clamp suggested by Chris does indeed fix the WARN_ON.
> > 
> > In the case where RPe is greater than RP0, RPe will now be clamped to
> > RP0. Is this likely to result in increased power consumption?
> > 
> > At a quick glance on my laptop it does not (idling around 5W before and
> > after) but Ville had suggested earlier to fall back to RP1, which would
> > be more consistent with previous kernels.
> > 
> > Thanks again for the quick responses,
> >  Michael
> 
> [TOR:] Michael,  I discussed this report with a pcode architect here.
> 
> The RPe value is clamped to the [RPn, RP0] range by pcode before
> returning the value to the driver on Broadwell but not on Haswell.
> 
> On Haswell, an efficient frequency value above RP0 is not a garbage
> value and could result from a relatively flat efficiency curve.  In
> this situation, leakage power would dominate the efficiency curve
> such that running at lower frequencies may not save power overall.
> Higher leakage power may result from a higher package temperature.
> 
> Running at RP0 may actually save power compared to RP1 by allowing
> more time in RC6.  

Hm, I'd just go with clamping since that's what bdw does too. So Chris
diff essentially.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: i915: WARN_ON(val > dev_priv->rps.max_freq_softlimit)

[O'Rourke, Tom]

On Wed, Feb 11, 2015 at 08:30:44AM +0100, Daniel Vetter wrote:
> On Tue, Feb 10, 2015 at 10:26:02PM -0800, O'Rourke, Tom wrote:
> > On Thu, Jan 29, 2015 at 08:56:06PM -0500, Michael Auchter wrote:
> > > On Thu, Jan 29, 2015 at 06:12:31PM +0100, Daniel Vetter wrote:
> > > > On Wed, Jan 28, 2015 at 10:36:02PM -0800, O'Rourke, Tom wrote:
> > > > > On Wed, Jan 28, 2015 at 01:28:58PM +0200, Ville Syrjälä wrote:
> > > > > > On Wed, Jan 28, 2015 at 09:58:15AM +0000, Chris Wilson wrote:
> > > > > > > On Wed, Jan 28, 2015 at 12:43:21AM -0500, Michael Auchter wrote:
> > > > > > > > Testing out 3.19-rc6 on my 2014 Thinkpad X1 Carbon (Haswell) resulted in
> > > > > > > > this WARN at boot (and pretty frequently afterwards):
> > > > > > > > 
> > > > > > > > WARNING: CPU: 0 PID: 989 at drivers/gpu/drm/i915/intel_pm.c:4377 gen6_set_rps+0x371/0x3c0()
> > > > > > > > WARN_ON(val > dev_priv->rps.max_freq_softlimit)
> > > > > > > 
> > > > > > > [snip]
> > > > > > >  
> > > > > > > > I'm not at all familiar with this hardware, but I took a quick look into
> > > > > > > > what changed with this commit for my laptop. Before the commit,
> > > > > > > > rps.min_freq_softlimit is 4 (from rps.min_freq) and
> > > > > > > > rps.max_freq_softlimit is 22.
> > > > > > > > 
> > > > > > > > After the commit, rps.min_freq_softlimit is set to the
> > > > > > > > rps.efficient_freq value read from pcode, which is 34 on my laptop.
> > > > > > > > So later when gen6_set_rps() is called with rps.min_freq_softlimit that
> > > > > > > > warning is hit.
> > > > > > > > 
> > > > > > > > Any thoughts? It certainly seems fishy that this commit causes
> > > > > > > > rps.min_freq_softlimit to be greater than rps.max_freq_softlimit.
> > > > > > > 
> > > > > > > Very fishy indeed. Moral of this story, never trust hw.
> > > > > > > 
> > > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > > index 3e630feb18e4..bbedd2901c54 100644
> > > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > > @@ -4007,7 +4007,10 @@ static void gen6_init_rps_frequencies(struct drm_device *dev)
> > > > > > >                                         &ddcc_status);
> > > > > > >                 if (0 == ret)
> > > > > > >                         dev_priv->rps.efficient_freq =
> > > > > > > -                               (ddcc_status >> 8) & 0xff;
> > > > > > > +                               clamp_t(u8,
> > > > > > > +                                       (ddcc_status >> 8) & 0xff,
> > > > > > > +                                       dev_priv->rps.min_freq,
> > > > > > > +                                       dev_priv->rps.max_freq);
> > > > > > 
> > > > > > Maybe better to fall back to rp1_freq if this is bogus?
> > > > > >
> > > > > [TOR:] Michael, Thank you for bringing this problem to our attention.
> > > > > 
> > > > > Yes, this function needs some range checking to maintain
> > > > > RPn <= RPe <= RP0.
> > > > > 
> > > > > A value of 34 seems too high for RPe.  
> > > > > What values does the Carbon X1 (Haswell) have for RPn and RP0?
> > > > 
> > > > 4 & 22, already in Micheal's original bug report.
> > > > 
> > > > Tom, can you pls polish the clamping into a proper patch with m-l
> > > > references?
> > > > 
> > > > Micheal, can you please test the first hunk from Chris (the one that adds
> > > > the clamp) to make sure it does indeed address the WARN_ON you're seeing?
> > > 
> > > The clamp suggested by Chris does indeed fix the WARN_ON.
> > > 
> > > In the case where RPe is greater than RP0, RPe will now be clamped to
> > > RP0. Is this likely to result in increased power consumption?
> > > 
> > > At a quick glance on my laptop it does not (idling around 5W before and
> > > after) but Ville had suggested earlier to fall back to RP1, which would
> > > be more consistent with previous kernels.
> > > 
> > > Thanks again for the quick responses,
> > >  Michael
> > 
> > [TOR:] Michael,  I discussed this report with a pcode architect here.
> > 
> > The RPe value is clamped to the [RPn, RP0] range by pcode before
> > returning the value to the driver on Broadwell but not on Haswell.
> > 
> > On Haswell, an efficient frequency value above RP0 is not a garbage
> > value and could result from a relatively flat efficiency curve.  In
> > this situation, leakage power would dominate the efficiency curve
> > such that running at lower frequencies may not save power overall.
> > Higher leakage power may result from a higher package temperature.
> > 
> > Running at RP0 may actually save power compared to RP1 by allowing
> > more time in RC6.  
> 
> Hm, I'd just go with clamping since that's what bdw does too. So Chris
> diff essentially.
> -Daniel

[TOR:] Yes, we should go with clamping, essentially the diff from Chris.
I am trying to provide an explanation of why that is the right thing to do.

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