[PATCH] drm/i915: WARN if interrupts aren't on in en/disable_pipestat

[PATCH] drm/i915: WARN if interrupts aren't on in en/disable_pipestat

[Daniel Vetter]

Now that vlv has runtime pm we kinda should check for that like on the
pch split platforms. Looks like this was simply lost in the vlv rpm
enabling.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 9eb303c1b621..76bc4d0de5a4 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -589,6 +589,7 @@ __i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
 	u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
 
 	assert_spin_locked(&dev_priv->irq_lock);
+	WARN_ON(!intel_irqs_enabled(dev_priv));
 
 	if (WARN_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
 		      status_mask & ~PIPESTAT_INT_STATUS_MASK,
@@ -615,6 +616,7 @@ __i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
 	u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
 
 	assert_spin_locked(&dev_priv->irq_lock);
+	WARN_ON(!intel_irqs_enabled(dev_priv));
 
 	if (WARN_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
 		      status_mask & ~PIPESTAT_INT_STATUS_MASK,
-- 
2.0.1

Re: [PATCH] drm/i915: WARN if interrupts aren't on in en/disable_pipestat

[Chris Wilson]

On Wed, Aug 27, 2014 at 10:43:37AM +0200, Daniel Vetter wrote:
> Now that vlv has runtime pm we kinda should check for that like on the
> pch split platforms. Looks like this was simply lost in the vlv rpm
> enabling.

Is there a reason why setting up the pipestat prior to enabling
interrupts is verboten, or is it just not done today?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: WARN if interrupts aren't on in en/disable_pipestat

[Daniel Vetter]

On Wed, Aug 27, 2014 at 09:51:58AM +0100, Chris Wilson wrote:
> On Wed, Aug 27, 2014 at 10:43:37AM +0200, Daniel Vetter wrote:
> > Now that vlv has runtime pm we kinda should check for that like on the
> > pch split platforms. Looks like this was simply lost in the vlv rpm
> > enabling.
> 
> Is there a reason why setting up the pipestat prior to enabling
> interrupts is verboten, or is it just not done today?

The similar warning on pch platforms was added to catch runtime pm bugs
where someone enabled some interrupt source, but didn't hold a runtime pm
reference. That's just not going to work. So motivation for this patch is
vlv/chv runtime pm really.

But I also think that this check provides value for driver
load/resume/suspend since if we change interrupt settings after the
interrupts have been disabled or before enabling something is at least
fishy. At least insofar as that our irq install hooks nowadays overwrite
all of them with default values.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: WARN if interrupts aren't on in en/disable_pipestat

[Chris Wilson]

On Wed, Aug 27, 2014 at 12:23:47PM +0200, Daniel Vetter wrote:
> On Wed, Aug 27, 2014 at 09:51:58AM +0100, Chris Wilson wrote:
> > On Wed, Aug 27, 2014 at 10:43:37AM +0200, Daniel Vetter wrote:
> > > Now that vlv has runtime pm we kinda should check for that like on the
> > > pch split platforms. Looks like this was simply lost in the vlv rpm
> > > enabling.
> > 
> > Is there a reason why setting up the pipestat prior to enabling
> > interrupts is verboten, or is it just not done today?
> 
> The similar warning on pch platforms was added to catch runtime pm bugs
> where someone enabled some interrupt source, but didn't hold a runtime pm
> reference. That's just not going to work. So motivation for this patch is
> vlv/chv runtime pm really.
> 
> But I also think that this check provides value for driver
> load/resume/suspend since if we change interrupt settings after the
> interrupts have been disabled or before enabling something is at least
> fishy. At least insofar as that our irq install hooks nowadays overwrite
> all of them with default values.

That's a better blurp. But doesn't it sound a little overly generic just
to keep whether or not pipestat_enable is called after uninstall  or
before install? Since we track the expected register value, could we
just semi-randomly check that the register matches that value?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: WARN if interrupts aren't on in en/disable_pipestat

[Daniel Vetter]

On Wed, Aug 27, 2014 at 1:28 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed, Aug 27, 2014 at 12:23:47PM +0200, Daniel Vetter wrote:
>> On Wed, Aug 27, 2014 at 09:51:58AM +0100, Chris Wilson wrote:
>> > On Wed, Aug 27, 2014 at 10:43:37AM +0200, Daniel Vetter wrote:
>> > > Now that vlv has runtime pm we kinda should check for that like on the
>> > > pch split platforms. Looks like this was simply lost in the vlv rpm
>> > > enabling.
>> >
>> > Is there a reason why setting up the pipestat prior to enabling
>> > interrupts is verboten, or is it just not done today?
>>
>> The similar warning on pch platforms was added to catch runtime pm bugs
>> where someone enabled some interrupt source, but didn't hold a runtime pm
>> reference. That's just not going to work. So motivation for this patch is
>> vlv/chv runtime pm really.
>>
>> But I also think that this check provides value for driver
>> load/resume/suspend since if we change interrupt settings after the
>> interrupts have been disabled or before enabling something is at least
>> fishy. At least insofar as that our irq install hooks nowadays overwrite
>> all of them with default values.
>
> That's a better blurp. But doesn't it sound a little overly generic just
> to keep whether or not pipestat_enable is called after uninstall  or
> before install? Since we track the expected register value, could we
> just semi-randomly check that the register matches that value?

I guess we could do both, but I don't really see a good place. Maybe
in the enable/disable functions, but I'm not even sure we track the
expected state for everyone really.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: WARN if interrupts aren't on in en/disable_pipestat

[Jesse Barnes]

On Wed, 27 Aug 2014 10:43:37 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> Now that vlv has runtime pm we kinda should check for that like on the
> pch split platforms. Looks like this was simply lost in the vlv rpm
> enabling.
> 
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 9eb303c1b621..76bc4d0de5a4 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -589,6 +589,7 @@ __i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
>  	u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
>  
>  	assert_spin_locked(&dev_priv->irq_lock);
> +	WARN_ON(!intel_irqs_enabled(dev_priv));
>  
>  	if (WARN_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
>  		      status_mask & ~PIPESTAT_INT_STATUS_MASK,
> @@ -615,6 +616,7 @@ __i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
>  	u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
>  
>  	assert_spin_locked(&dev_priv->irq_lock);
> +	WARN_ON(!intel_irqs_enabled(dev_priv));
>  
>  	if (WARN_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
>  		      status_mask & ~PIPESTAT_INT_STATUS_MASK,

Yeah looks good, wonder if it'll trigger any new warnings.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: WARN if interrupts aren't on in en/disable_pipestat

[Daniel Vetter]

On Thu, Aug 28, 2014 at 1:00 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index 9eb303c1b621..76bc4d0de5a4 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -589,6 +589,7 @@ __i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
>>       u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
>>
>>       assert_spin_locked(&dev_priv->irq_lock);
>> +     WARN_ON(!intel_irqs_enabled(dev_priv));
>>
>>       if (WARN_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
>>                     status_mask & ~PIPESTAT_INT_STATUS_MASK,
>> @@ -615,6 +616,7 @@ __i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
>>       u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
>>
>>       assert_spin_locked(&dev_priv->irq_lock);
>> +     WARN_ON(!intel_irqs_enabled(dev_priv));
>>
>>       if (WARN_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
>>                     status_mask & ~PIPESTAT_INT_STATUS_MASK,
>
> Yeah looks good, wonder if it'll trigger any new warnings.

It will blow up in a bunch of postinstall hooks, just like the one for
ilk. At least without my patch to shuffle the pm._irqs_disabled
assignment around.

> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

... so does that count as an implicit r-b on my other patch?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: WARN if interrupts aren't on in en/disable_pipestat

[Jesse Barnes]

On Thu, 4 Sep 2014 17:59:18 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> On Thu, Aug 28, 2014 at 1:00 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >> index 9eb303c1b621..76bc4d0de5a4 100644
> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> @@ -589,6 +589,7 @@ __i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
> >>       u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
> >>
> >>       assert_spin_locked(&dev_priv->irq_lock);
> >> +     WARN_ON(!intel_irqs_enabled(dev_priv));
> >>
> >>       if (WARN_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
> >>                     status_mask & ~PIPESTAT_INT_STATUS_MASK,
> >> @@ -615,6 +616,7 @@ __i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
> >>       u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
> >>
> >>       assert_spin_locked(&dev_priv->irq_lock);
> >> +     WARN_ON(!intel_irqs_enabled(dev_priv));
> >>
> >>       if (WARN_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
> >>                     status_mask & ~PIPESTAT_INT_STATUS_MASK,
> >
> > Yeah looks good, wonder if it'll trigger any new warnings.
> 
> It will blow up in a bunch of postinstall hooks, just like the one for
> ilk. At least without my patch to shuffle the pm._irqs_disabled
> assignment around.
> 
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> ... so does that count as an implicit r-b on my other patch?

Sure, though didn't Jani find some issues with it?

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: WARN if interrupts aren't on in en/disable_pipestat

[Daniel Vetter]

On Thu, Sep 4, 2014 at 6:24 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Thu, 4 Sep 2014 17:59:18 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
>> On Thu, Aug 28, 2014 at 1:00 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> >> index 9eb303c1b621..76bc4d0de5a4 100644
>> >> --- a/drivers/gpu/drm/i915/i915_irq.c
>> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> >> @@ -589,6 +589,7 @@ __i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
>> >>       u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
>> >>
>> >>       assert_spin_locked(&dev_priv->irq_lock);
>> >> +     WARN_ON(!intel_irqs_enabled(dev_priv));
>> >>
>> >>       if (WARN_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
>> >>                     status_mask & ~PIPESTAT_INT_STATUS_MASK,
>> >> @@ -615,6 +616,7 @@ __i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
>> >>       u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
>> >>
>> >>       assert_spin_locked(&dev_priv->irq_lock);
>> >> +     WARN_ON(!intel_irqs_enabled(dev_priv));
>> >>
>> >>       if (WARN_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
>> >>                     status_mask & ~PIPESTAT_INT_STATUS_MASK,
>> >
>> > Yeah looks good, wonder if it'll trigger any new warnings.
>>
>> It will blow up in a bunch of postinstall hooks, just like the one for
>> ilk. At least without my patch to shuffle the pm._irqs_disabled
>> assignment around.
>>
>> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>>
>> ... so does that count as an implicit r-b on my other patch?
>
> Sure, though didn't Jani find some issues with it?

QA claims that both my and your patch break the display on hsw, bdw
and snb or something like that. Which either means I'm blind (since
either patch should only affect ilk in a functional way) or they're
doing something really strange. I have no idea what's actually going
on there, but we have a regular stream of reports of this one here ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: WARN if interrupts aren't on in en/disable_pipestat

[Jesse Barnes]

On Thu, 4 Sep 2014 18:59:55 +0200
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> On Thu, Sep 4, 2014 at 6:24 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > On Thu, 4 Sep 2014 17:59:18 +0200
> > Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >
> >> On Thu, Aug 28, 2014 at 1:00 AM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> >> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> >> >> index 9eb303c1b621..76bc4d0de5a4 100644
> >> >> --- a/drivers/gpu/drm/i915/i915_irq.c
> >> >> +++ b/drivers/gpu/drm/i915/i915_irq.c
> >> >> @@ -589,6 +589,7 @@ __i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
> >> >>       u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
> >> >>
> >> >>       assert_spin_locked(&dev_priv->irq_lock);
> >> >> +     WARN_ON(!intel_irqs_enabled(dev_priv));
> >> >>
> >> >>       if (WARN_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
> >> >>                     status_mask & ~PIPESTAT_INT_STATUS_MASK,
> >> >> @@ -615,6 +616,7 @@ __i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
> >> >>       u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
> >> >>
> >> >>       assert_spin_locked(&dev_priv->irq_lock);
> >> >> +     WARN_ON(!intel_irqs_enabled(dev_priv));
> >> >>
> >> >>       if (WARN_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
> >> >>                     status_mask & ~PIPESTAT_INT_STATUS_MASK,
> >> >
> >> > Yeah looks good, wonder if it'll trigger any new warnings.
> >>
> >> It will blow up in a bunch of postinstall hooks, just like the one for
> >> ilk. At least without my patch to shuffle the pm._irqs_disabled
> >> assignment around.
> >>
> >> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> >>
> >> ... so does that count as an implicit r-b on my other patch?
> >
> > Sure, though didn't Jani find some issues with it?
> 
> QA claims that both my and your patch break the display on hsw, bdw
> and snb or something like that. Which either means I'm blind (since
> either patch should only affect ilk in a functional way) or they're
> doing something really strange. I have no idea what's actually going
> on there, but we have a regular stream of reports of this one here ...

Yeah I agree the results are weird; don't see how we'd affect BDW
display, esp with my patch, so I'd say go ahead with either.  Though if
you do mine I guess you'd drop the above.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: WARN if interrupts aren't on in en/disable_pipestat

[Daniel Vetter]

On Wed, Aug 27, 2014 at 04:00:22PM -0700, Jesse Barnes wrote:
> On Wed, 27 Aug 2014 10:43:37 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > Now that vlv has runtime pm we kinda should check for that like on the
> > pch split platforms. Looks like this was simply lost in the vlv rpm
> > enabling.
> > 
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 9eb303c1b621..76bc4d0de5a4 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -589,6 +589,7 @@ __i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
> >  	u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
> >  
> >  	assert_spin_locked(&dev_priv->irq_lock);
> > +	WARN_ON(!intel_irqs_enabled(dev_priv));
> >  
> >  	if (WARN_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
> >  		      status_mask & ~PIPESTAT_INT_STATUS_MASK,
> > @@ -615,6 +616,7 @@ __i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
> >  	u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
> >  
> >  	assert_spin_locked(&dev_priv->irq_lock);
> > +	WARN_ON(!intel_irqs_enabled(dev_priv));
> >  
> >  	if (WARN_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
> >  		      status_mask & ~PIPESTAT_INT_STATUS_MASK,
> 
> Yeah looks good, wonder if it'll trigger any new warnings.
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Since Jani just merged the driver load ordering patch I've pulled this one
into dinq to maximise testing coverage. Let's see what happens ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch