Re: [PATCH] drm/i915: fix rps.vlv_work initialization

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Oct 01, 2013 at 08:46:21PM +0300, Imre Deak wrote:
> On Tue, 2013-10-01 at 19:55 +0300, Imre Deak wrote:
> > On Tue, 2013-10-01 at 18:55 +0300, Ville Syrjälä wrote:
> > > On Tue, Oct 01, 2013 at 06:11:26PM +0300, Imre Deak wrote:
> > > > During driver loading we are initializing rps.vlv_work in
> > > > valleyview_enable_rps() via the rps.delayed_resume_work delayed work.
> > > > This is too late since we are using vlv_work already via
> > > > i915_driver_load()->intel_uncore_sanitize()->
> > > > intel_disable_gt_powersave(). This at least leads to the following
> > > > kernel warning:
> > > > 
> > > >  INFO: trying to register non-static key.
> > > >  the code is fine but needs lockdep annotation.
> > > >  turning off the locking correctness validator.
> > > > 
> > > > Fix this by initialzing vlv_work before we call intel_uncore_sanitize().
> > > > 
> > > > The regression was introduced in
> > > > 
> > > > commit 7dcd2677ea912573d9ed4bcd629b0023b2d11505
> > > > Author: Konstantin Khlebnikov <khlebnikov@openvz.org>
> > > > Date:   Wed Jul 17 10:22:58 2013 +0400
> > > > 
> > > >     drm/i915: fix long-standing SNB regression in power consumption
> > > >     after resume
> > > > 
> > > > though there was no good reason to initialize the static vlv_work from
> > > > another delayed work to begin with (especially since this will happen
> > > > multiple times).
> > > > 
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=69397
> > > > 
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_pm.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > index 698257c..2a0a340 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -3894,8 +3894,6 @@ static void valleyview_enable_rps(struct drm_device *dev)
> > > >  				      dev_priv->rps.rpe_delay),
> > > >  			 dev_priv->rps.rpe_delay);
> > > >  
> > > > -	INIT_DELAYED_WORK(&dev_priv->rps.vlv_work, vlv_rps_timer_work);
> > > > -
> > > >  	valleyview_set_rps(dev_priv->dev, dev_priv->rps.rpe_delay);
> > > >  
> > > >  	gen6_enable_rps_interrupts(dev);
> > > > @@ -5805,5 +5803,7 @@ void intel_pm_init(struct drm_device *dev)
> > > >  
> > > >  	INIT_DELAYED_WORK(&dev_priv->rps.delayed_resume_work,
> > > >  			  intel_gen6_powersave_work);
> > > > +
> > > > +	INIT_DELAYED_WORK(&dev_priv->rps.vlv_work, vlv_rps_timer_work);
> > > 
> > > We're initializing rps.work in intel_irq_init(). Maybe we should try to
> > > keep rps related stuff together?
> > 
> > Yes, makes sense. I'll send v2 moving both init work to
> > intel_irq_init().
> 
> Hm, actually for that we'd have to export the two handler functions and
> since all of these really belong to intel_pm.c I'd rather just leave the
> work init there too.

Yeah, this makes imo more sense. Picked up for -fixes (with cc: stable
since the regressing commit is also cc: stable), thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch