* [PATCH] drm/i915: fix rps.vlv_work initialization
@ 2013-10-01 15:11 Imre Deak
2013-10-01 15:55 ` Ville Syrjälä
0 siblings, 1 reply; 5+ messages in thread
From: Imre Deak @ 2013-10-01 15:11 UTC (permalink / raw)
To: intel-gfx
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);
}
--
1.8.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: fix rps.vlv_work initialization
2013-10-01 15:11 [PATCH] drm/i915: fix rps.vlv_work initialization Imre Deak
@ 2013-10-01 15:55 ` Ville Syrjälä
2013-10-01 16:55 ` Imre Deak
0 siblings, 1 reply; 5+ messages in thread
From: Ville Syrjälä @ 2013-10-01 15:55 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx
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
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: fix rps.vlv_work initialization
2013-10-01 15:55 ` Ville Syrjälä
@ 2013-10-01 16:55 ` Imre Deak
2013-10-01 17:46 ` Imre Deak
0 siblings, 1 reply; 5+ messages in thread
From: Imre Deak @ 2013-10-01 16:55 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
[-- Attachment #1.1: Type: text/plain, Size: 2468 bytes --]
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
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: fix rps.vlv_work initialization
2013-10-01 16:55 ` Imre Deak
@ 2013-10-01 17:46 ` Imre Deak
2013-10-01 19:12 ` Daniel Vetter
0 siblings, 1 reply; 5+ messages in thread
From: Imre Deak @ 2013-10-01 17:46 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
[-- Attachment #1.1: Type: text/plain, Size: 2809 bytes --]
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
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: fix rps.vlv_work initialization
2013-10-01 17:46 ` Imre Deak
@ 2013-10-01 19:12 ` Daniel Vetter
0 siblings, 0 replies; 5+ messages in thread
From: Daniel Vetter @ 2013-10-01 19:12 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-10-01 19:12 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-01 15:11 [PATCH] drm/i915: fix rps.vlv_work initialization Imre Deak
2013-10-01 15:55 ` Ville Syrjälä
2013-10-01 16:55 ` Imre Deak
2013-10-01 17:46 ` Imre Deak
2013-10-01 19:12 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox