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@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
