[Intel-gfx] 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