public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* [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