| Message ID | 1380640286-26306-1-git-send-email-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? > } > > -- > 1.8.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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(). --Imre
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. --Imre
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
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); }
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(-)