* [PATCH] drm/i915: Don't del_timer_sync uninitialized timer
@ 2014-03-15 0:21 Ben Widawsky
2014-03-15 8:22 ` Chris Wilson
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Ben Widawsky @ 2014-03-15 0:21 UTC (permalink / raw)
To: Intel GFX; +Cc: Mika Kuoppala, Ben Widawsky, Ben Widawsky
Broken by:
commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date: Thu Mar 13 12:00:29 2014 +0000
drm/i915: Consolidate forcewake resetting to a single function
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
drivers/gpu/drm/i915/intel_uncore.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index e6bb421..7e55ceb 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -362,6 +362,9 @@ void intel_uncore_early_sanitize(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
+ setup_timer(&dev_priv->uncore.force_wake_timer,
+ gen6_force_wake_timer, (unsigned long)dev_priv);
+
if (HAS_FPGA_DBG_UNCLAIMED(dev))
__raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
@@ -724,9 +727,6 @@ void intel_uncore_init(struct drm_device *dev)
{
struct drm_i915_private *dev_priv = dev->dev_private;
- setup_timer(&dev_priv->uncore.force_wake_timer,
- gen6_force_wake_timer, (unsigned long)dev_priv);
-
if (IS_VALLEYVIEW(dev)) {
dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get;
dev_priv->uncore.funcs.force_wake_put = __vlv_force_wake_put;
--
1.9.0
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: Don't del_timer_sync uninitialized timer
2014-03-15 0:21 [PATCH] drm/i915: Don't del_timer_sync uninitialized timer Ben Widawsky
@ 2014-03-15 8:22 ` Chris Wilson
2014-03-15 11:47 ` Daniel Vetter
2014-03-17 7:56 ` Jani Nikula
2 siblings, 0 replies; 11+ messages in thread
From: Chris Wilson @ 2014-03-15 8:22 UTC (permalink / raw)
To: Ben Widawsky; +Cc: Intel GFX, Ben Widawsky, Mika Kuoppala
On Fri, Mar 14, 2014 at 05:21:36PM -0700, Ben Widawsky wrote:
> Broken by:
> commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date: Thu Mar 13 12:00:29 2014 +0000
>
> drm/i915: Consolidate forcewake resetting to a single function
as it is now called by intel_uncore_forcewake_reset() much earlier in
the initialisation sequence.
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Thanks,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: Don't del_timer_sync uninitialized timer
2014-03-15 0:21 [PATCH] drm/i915: Don't del_timer_sync uninitialized timer Ben Widawsky
2014-03-15 8:22 ` Chris Wilson
@ 2014-03-15 11:47 ` Daniel Vetter
2014-03-15 15:20 ` Chris Wilson
2014-03-17 7:56 ` Jani Nikula
2 siblings, 1 reply; 11+ messages in thread
From: Daniel Vetter @ 2014-03-15 11:47 UTC (permalink / raw)
To: Ben Widawsky; +Cc: Intel GFX, Ben Widawsky, Mika Kuoppala
On Fri, Mar 14, 2014 at 05:21:36PM -0700, Ben Widawsky wrote:
> Broken by:
> commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date: Thu Mar 13 12:00:29 2014 +0000
>
> drm/i915: Consolidate forcewake resetting to a single function
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
> drivers/gpu/drm/i915/intel_uncore.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index e6bb421..7e55ceb 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -362,6 +362,9 @@ void intel_uncore_early_sanitize(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> + setup_timer(&dev_priv->uncore.force_wake_timer,
> + gen6_force_wake_timer, (unsigned long)dev_priv);
We call early_sanitize also from our resume code, so this will now
re-setup the timer again. We generally don't do that since if we ever leak
the timer to here in an enabled state it causes havoc.
-Daniel
> +
> if (HAS_FPGA_DBG_UNCLAIMED(dev))
> __raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
>
> @@ -724,9 +727,6 @@ void intel_uncore_init(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> - setup_timer(&dev_priv->uncore.force_wake_timer,
> - gen6_force_wake_timer, (unsigned long)dev_priv);
> -
> if (IS_VALLEYVIEW(dev)) {
> dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get;
> dev_priv->uncore.funcs.force_wake_put = __vlv_force_wake_put;
> --
> 1.9.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: Don't del_timer_sync uninitialized timer
2014-03-15 11:47 ` Daniel Vetter
@ 2014-03-15 15:20 ` Chris Wilson
2014-03-15 18:30 ` Ben Widawsky
0 siblings, 1 reply; 11+ messages in thread
From: Chris Wilson @ 2014-03-15 15:20 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel GFX, Mika Kuoppala, Ben Widawsky, Ben Widawsky
On Sat, Mar 15, 2014 at 12:47:22PM +0100, Daniel Vetter wrote:
> On Fri, Mar 14, 2014 at 05:21:36PM -0700, Ben Widawsky wrote:
> > Broken by:
> > commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd
> > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > Date: Thu Mar 13 12:00:29 2014 +0000
> >
> > drm/i915: Consolidate forcewake resetting to a single function
> >
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> > drivers/gpu/drm/i915/intel_uncore.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > index e6bb421..7e55ceb 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -362,6 +362,9 @@ void intel_uncore_early_sanitize(struct drm_device *dev)
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> >
> > + setup_timer(&dev_priv->uncore.force_wake_timer,
> > + gen6_force_wake_timer, (unsigned long)dev_priv);
>
> We call early_sanitize also from our resume code, so this will now
> re-setup the timer again. We generally don't do that since if we ever leak
> the timer to here in an enabled state it causes havoc.
Gah, really? intel_uncore_early_init()! There must be a clean way to
break this up.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: Don't del_timer_sync uninitialized timer
2014-03-15 15:20 ` Chris Wilson
@ 2014-03-15 18:30 ` Ben Widawsky
2014-03-15 19:13 ` Daniel Vetter
2014-03-17 13:17 ` Chris Wilson
0 siblings, 2 replies; 11+ messages in thread
From: Ben Widawsky @ 2014-03-15 18:30 UTC (permalink / raw)
To: Chris Wilson, Daniel Vetter, Ben Widawsky, Intel GFX,
Mika Kuoppala
On Sat, Mar 15, 2014 at 03:20:23PM +0000, Chris Wilson wrote:
> On Sat, Mar 15, 2014 at 12:47:22PM +0100, Daniel Vetter wrote:
> > On Fri, Mar 14, 2014 at 05:21:36PM -0700, Ben Widawsky wrote:
> > > Broken by:
> > > commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd
> > > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > > Date: Thu Mar 13 12:00:29 2014 +0000
> > >
> > > drm/i915: Consolidate forcewake resetting to a single function
> > >
> > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > ---
> > > drivers/gpu/drm/i915/intel_uncore.c | 6 +++---
> > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > > index e6bb421..7e55ceb 100644
> > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > @@ -362,6 +362,9 @@ void intel_uncore_early_sanitize(struct drm_device *dev)
> > > {
> > > struct drm_i915_private *dev_priv = dev->dev_private;
> > >
> > > + setup_timer(&dev_priv->uncore.force_wake_timer,
> > > + gen6_force_wake_timer, (unsigned long)dev_priv);
> >
> > We call early_sanitize also from our resume code, so this will now
> > re-setup the timer again. We generally don't do that since if we ever leak
> > the timer to here in an enabled state it causes havoc.
>
> Gah, really? intel_uncore_early_init()! There must be a clean way to
> break this up.
> -Chris
At least in the code base I was looking at, we currently do this also,
so I didn't think this was any worse.
With lockdep turned on, the module will not even load, so please either
revert the original, or merge this.
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: Don't del_timer_sync uninitialized timer
2014-03-15 18:30 ` Ben Widawsky
@ 2014-03-15 19:13 ` Daniel Vetter
2014-03-17 13:17 ` Chris Wilson
1 sibling, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2014-03-15 19:13 UTC (permalink / raw)
To: Ben Widawsky; +Cc: Mika Kuoppala, Intel GFX, Ben Widawsky
On Sat, Mar 15, 2014 at 7:30 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
>> > > + setup_timer(&dev_priv->uncore.force_wake_timer,
>> > > + gen6_force_wake_timer, (unsigned long)dev_priv);
>> >
>> > We call early_sanitize also from our resume code, so this will now
>> > re-setup the timer again. We generally don't do that since if we ever leak
>> > the timer to here in an enabled state it causes havoc.
>>
>> Gah, really? intel_uncore_early_init()! There must be a clean way to
>> break this up.
>> -Chris
>
> At least in the code base I was looking at, we currently do this also,
> so I didn't think this was any worse.
Hm, where? Imo such a thing would be a bug.
> With lockdep turned on, the module will not even load, so please either
> revert the original, or merge this.
I think we can just move the call to early_sanitize into uncore_init,
right after the timer setup.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: Don't del_timer_sync uninitialized timer
2014-03-15 0:21 [PATCH] drm/i915: Don't del_timer_sync uninitialized timer Ben Widawsky
2014-03-15 8:22 ` Chris Wilson
2014-03-15 11:47 ` Daniel Vetter
@ 2014-03-17 7:56 ` Jani Nikula
2014-03-17 9:14 ` Jani Nikula
2 siblings, 1 reply; 11+ messages in thread
From: Jani Nikula @ 2014-03-17 7:56 UTC (permalink / raw)
To: Intel GFX; +Cc: Ben Widawsky, Ben Widawsky, Mika Kuoppala
On Sat, 15 Mar 2014, Ben Widawsky <benjamin.widawsky@intel.com> wrote:
> Broken by:
> commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date: Thu Mar 13 12:00:29 2014 +0000
>
> drm/i915: Consolidate forcewake resetting to a single function
>
Does this result in https://bugs.freedesktop.org/show_bug.cgi?id=76242 ?
Backtrace in the commit message would help a lot in pattern matching...
BR,
Jani.
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
> drivers/gpu/drm/i915/intel_uncore.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index e6bb421..7e55ceb 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -362,6 +362,9 @@ void intel_uncore_early_sanitize(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> + setup_timer(&dev_priv->uncore.force_wake_timer,
> + gen6_force_wake_timer, (unsigned long)dev_priv);
> +
> if (HAS_FPGA_DBG_UNCLAIMED(dev))
> __raw_i915_write32(dev_priv, FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
>
> @@ -724,9 +727,6 @@ void intel_uncore_init(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> - setup_timer(&dev_priv->uncore.force_wake_timer,
> - gen6_force_wake_timer, (unsigned long)dev_priv);
> -
> if (IS_VALLEYVIEW(dev)) {
> dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get;
> dev_priv->uncore.funcs.force_wake_put = __vlv_force_wake_put;
> --
> 1.9.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: Don't del_timer_sync uninitialized timer
2014-03-17 7:56 ` Jani Nikula
@ 2014-03-17 9:14 ` Jani Nikula
2014-03-17 9:52 ` Daniel Vetter
0 siblings, 1 reply; 11+ messages in thread
From: Jani Nikula @ 2014-03-17 9:14 UTC (permalink / raw)
To: Intel GFX; +Cc: Ben Widawsky, Ben Widawsky, Mika Kuoppala
On Mon, 17 Mar 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Sat, 15 Mar 2014, Ben Widawsky <benjamin.widawsky@intel.com> wrote:
>> Broken by:
>> commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd
>> Author: Chris Wilson <chris@chris-wilson.co.uk>
>> Date: Thu Mar 13 12:00:29 2014 +0000
>>
>> drm/i915: Consolidate forcewake resetting to a single function
>>
>
> Does this result in https://bugs.freedesktop.org/show_bug.cgi?id=76242 ?
QA confirms this patch fixes the bug.
Jani.
--
Jani Nikula, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: Don't del_timer_sync uninitialized timer
2014-03-17 9:14 ` Jani Nikula
@ 2014-03-17 9:52 ` Daniel Vetter
0 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2014-03-17 9:52 UTC (permalink / raw)
To: Jani Nikula; +Cc: Intel GFX, Mika Kuoppala, Ben Widawsky, Ben Widawsky
On Mon, Mar 17, 2014 at 11:14:25AM +0200, Jani Nikula wrote:
> On Mon, 17 Mar 2014, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> > On Sat, 15 Mar 2014, Ben Widawsky <benjamin.widawsky@intel.com> wrote:
> >> Broken by:
> >> commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd
> >> Author: Chris Wilson <chris@chris-wilson.co.uk>
> >> Date: Thu Mar 13 12:00:29 2014 +0000
> >>
> >> drm/i915: Consolidate forcewake resetting to a single function
> >>
> >
> > Does this result in https://bugs.freedesktop.org/show_bug.cgi?id=76242 ?
>
> QA confirms this patch fixes the bug.
Also the commit message seriously lacks details. Apparantly this only
blows up with lockdep (since most my machines here are still happy), which
is a rather crucial detail.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: Don't del_timer_sync uninitialized timer
2014-03-15 18:30 ` Ben Widawsky
2014-03-15 19:13 ` Daniel Vetter
@ 2014-03-17 13:17 ` Chris Wilson
2014-03-17 13:52 ` Daniel Vetter
1 sibling, 1 reply; 11+ messages in thread
From: Chris Wilson @ 2014-03-17 13:17 UTC (permalink / raw)
To: Ben Widawsky; +Cc: Intel GFX, Mika Kuoppala, Ben Widawsky
On Sat, Mar 15, 2014 at 11:30:18AM -0700, Ben Widawsky wrote:
> On Sat, Mar 15, 2014 at 03:20:23PM +0000, Chris Wilson wrote:
> > On Sat, Mar 15, 2014 at 12:47:22PM +0100, Daniel Vetter wrote:
> > > On Fri, Mar 14, 2014 at 05:21:36PM -0700, Ben Widawsky wrote:
> > > > Broken by:
> > > > commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd
> > > > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Date: Thu Mar 13 12:00:29 2014 +0000
> > > >
> > > > drm/i915: Consolidate forcewake resetting to a single function
> > > >
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > > ---
> > > > drivers/gpu/drm/i915/intel_uncore.c | 6 +++---
> > > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > > > index e6bb421..7e55ceb 100644
> > > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > > @@ -362,6 +362,9 @@ void intel_uncore_early_sanitize(struct drm_device *dev)
> > > > {
> > > > struct drm_i915_private *dev_priv = dev->dev_private;
> > > >
> > > > + setup_timer(&dev_priv->uncore.force_wake_timer,
> > > > + gen6_force_wake_timer, (unsigned long)dev_priv);
> > >
> > > We call early_sanitize also from our resume code, so this will now
> > > re-setup the timer again. We generally don't do that since if we ever leak
> > > the timer to here in an enabled state it causes havoc.
> >
> > Gah, really? intel_uncore_early_init()! There must be a clean way to
> > break this up.
> > -Chris
>
> At least in the code base I was looking at, we currently do this also,
> so I didn't think this was any worse.
>
> With lockdep turned on, the module will not even load, so please either
> revert the original, or merge this.
I think we can just:
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index a62ff21..c9fcc20 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -317,9 +317,10 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
{
struct drm_i915_private *dev_priv = dev->dev_private;
unsigned long irqflags;
- int pending;
+ int pending = 0;
- pending = del_timer_sync(&dev_priv->uncore.force_wake_timer);
+ if (dev_priv->uncore.force_wake_timer.function)
+ pending = del_timer_sync(&dev_priv->uncore.force_wake_timer);
/* Hold uncore.lock across reset to prevent any register access
* with forcewake not set correctly
i.e.
- del_timer_sync(&dev_priv->uncore.force_wake_timer);
+ if (dev_priv->uncore.force_wake_timer.function)
+ del_timer_sync(&dev_priv->uncore.force_wake_timer);
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] drm/i915: Don't del_timer_sync uninitialized timer
2014-03-17 13:17 ` Chris Wilson
@ 2014-03-17 13:52 ` Daniel Vetter
0 siblings, 0 replies; 11+ messages in thread
From: Daniel Vetter @ 2014-03-17 13:52 UTC (permalink / raw)
To: Chris Wilson, Ben Widawsky, Daniel Vetter, Ben Widawsky,
Intel GFX, Mika Kuoppala
On Mon, Mar 17, 2014 at 01:17:32PM +0000, Chris Wilson wrote:
> On Sat, Mar 15, 2014 at 11:30:18AM -0700, Ben Widawsky wrote:
> > On Sat, Mar 15, 2014 at 03:20:23PM +0000, Chris Wilson wrote:
> > > On Sat, Mar 15, 2014 at 12:47:22PM +0100, Daniel Vetter wrote:
> > > > On Fri, Mar 14, 2014 at 05:21:36PM -0700, Ben Widawsky wrote:
> > > > > Broken by:
> > > > > commit 0294ae7b44bba7ab0d4cef9a8736287f38bdb4fd
> > > > > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Date: Thu Mar 13 12:00:29 2014 +0000
> > > > >
> > > > > drm/i915: Consolidate forcewake resetting to a single function
> > > > >
> > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > > > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > > > ---
> > > > > drivers/gpu/drm/i915/intel_uncore.c | 6 +++---
> > > > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> > > > > index e6bb421..7e55ceb 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > > > > @@ -362,6 +362,9 @@ void intel_uncore_early_sanitize(struct drm_device *dev)
> > > > > {
> > > > > struct drm_i915_private *dev_priv = dev->dev_private;
> > > > >
> > > > > + setup_timer(&dev_priv->uncore.force_wake_timer,
> > > > > + gen6_force_wake_timer, (unsigned long)dev_priv);
> > > >
> > > > We call early_sanitize also from our resume code, so this will now
> > > > re-setup the timer again. We generally don't do that since if we ever leak
> > > > the timer to here in an enabled state it causes havoc.
> > >
> > > Gah, really? intel_uncore_early_init()! There must be a clean way to
> > > break this up.
> > > -Chris
> >
> > At least in the code base I was looking at, we currently do this also,
> > so I didn't think this was any worse.
> >
> > With lockdep turned on, the module will not even load, so please either
> > revert the original, or merge this.
>
> I think we can just:
>
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index a62ff21..c9fcc20 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -317,9 +317,10 @@ static void intel_uncore_forcewake_reset(struct drm_device *dev, bool restore)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> unsigned long irqflags;
> - int pending;
> + int pending = 0;
>
> - pending = del_timer_sync(&dev_priv->uncore.force_wake_timer);
> + if (dev_priv->uncore.force_wake_timer.function)
> + pending = del_timer_sync(&dev_priv->uncore.force_wake_timer);
>
> /* Hold uncore.lock across reset to prevent any register access
> * with forcewake not set correctly
>
> i.e.
>
> - del_timer_sync(&dev_priv->uncore.force_wake_timer);
> + if (dev_priv->uncore.force_wake_timer.function)
> + del_timer_sync(&dev_priv->uncore.force_wake_timer);
I've thought of this:
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index e4d2b9f15ae2..9faee49f210d 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1608,8 +1608,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
goto put_bridge;
}
- intel_uncore_early_sanitize(dev);
-
/* This must be called before any calls to HAS_PCH_* */
intel_detect_pch(dev);
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index e2e328d86aff..c3832d9270a6 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -736,6 +736,8 @@ void intel_uncore_init(struct drm_device *dev)
setup_timer(&dev_priv->uncore.force_wake_timer,
gen6_force_wake_timer, (unsigned long)dev_priv);
+ intel_uncore_early_sanitize(dev);
+
if (IS_VALLEYVIEW(dev)) {
dev_priv->uncore.funcs.force_wake_get = __vlv_force_wake_get;
dev_priv->uncore.funcs.force_wake_put = __vlv_force_wake_put;
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply related [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-03-17 13:52 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-03-15 0:21 [PATCH] drm/i915: Don't del_timer_sync uninitialized timer Ben Widawsky
2014-03-15 8:22 ` Chris Wilson
2014-03-15 11:47 ` Daniel Vetter
2014-03-15 15:20 ` Chris Wilson
2014-03-15 18:30 ` Ben Widawsky
2014-03-15 19:13 ` Daniel Vetter
2014-03-17 13:17 ` Chris Wilson
2014-03-17 13:52 ` Daniel Vetter
2014-03-17 7:56 ` Jani Nikula
2014-03-17 9:14 ` Jani Nikula
2014-03-17 9:52 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox