* Re: [PATCH] drm/i915: vlv: fix IRQ masking when uninstalling interrupts
2014-11-20 14:05 [PATCH] drm/i915: vlv: fix IRQ masking when uninstalling interrupts Imre Deak
@ 2014-11-20 14:11 ` Daniel Vetter
2014-11-20 14:19 ` Imre Deak
2014-11-22 10:35 ` [PATCH] drm/i915: vlv: fix IRQ masking when shuang.he
2014-12-10 17:37 ` [PATCH] drm/i915: vlv: fix IRQ masking when uninstalling interrupts Ville Syrjälä
2 siblings, 1 reply; 6+ messages in thread
From: Daniel Vetter @ 2014-11-20 14:11 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx, v3.15+
On Thu, Nov 20, 2014 at 04:05:55PM +0200, Imre Deak wrote:
> irq_mask should include all IRQ bits that we want to mask, but atm we
> set it incorrectly to the inverse of this. If the mask is used
> subsequently to enable/disable some IRQ bits, we may unintentionally
> unmask unrelated IRQs. I can't see any way that this can lead to a real
> problem in the current -nightly code, since the first place the mask
> will be used next (after a suspend/resume cycle) is in
> valleyview_irq_postinstall(), but the mask is reset there to its proper
> value.
>
> This causes a problem in the upstream kernel though, where - due to another
> issue - the mask is used in the above way to disable only the display IRQs.
> This other issue is fixed by:
>
> commit 950eabaf5a87257040e0c207be09487954113f54
> Author: Imre Deak <imre.deak@intel.com>
> Date: Mon Sep 8 15:21:09 2014 +0300
>
> drm/i915: vlv: fix display IRQ enable/disable
>
> Interestingly, even with the above two bugs, we shouldn't in theory have
> any real problems (arguably a famous last sentence:). That's because
> even if we unmask something unintentionally via the VLV_IMR/VLV_IER
> register the master IRQ masking bit in VLV_MASTER_IER is still set and
> should prevent all i915 interrupts. According to my testing on an ASUS
> T100 with DSI output this isn't the case at least with the
> MIPIA_INTERRUPT. Leaving this one unmasked in IMR/IER, while having
> VLV_MASTER_IER set to 0 may lead to a lockup during system suspend as
> shown in the bugzilla ticket below. This fix should get rid of the
> problem reported there in upstream and older kernels.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85920
> Cc: stable@vger.kernel.org (v3.15+)
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
> drivers/gpu/drm/i915/i915_irq.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 8d169e1..4601f53 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3597,7 +3597,7 @@ static void vlv_display_irq_uninstall(struct drm_i915_private *dev_priv)
>
> vlv_display_irq_reset(dev_priv);
>
> - dev_priv->irq_mask = 0;
> + dev_priv->irq_mask = ~0;
vlv seems to be the only platform which resets irq masks in the uninstall
hook. That smells fishy - can't we just remove this completely?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: vlv: fix IRQ masking when uninstalling interrupts
2014-11-20 14:11 ` Daniel Vetter
@ 2014-11-20 14:19 ` Imre Deak
0 siblings, 0 replies; 6+ messages in thread
From: Imre Deak @ 2014-11-20 14:19 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx, v3.15+
On Thu, 2014-11-20 at 15:11 +0100, Daniel Vetter wrote:
> On Thu, Nov 20, 2014 at 04:05:55PM +0200, Imre Deak wrote:
> > irq_mask should include all IRQ bits that we want to mask, but atm we
> > set it incorrectly to the inverse of this. If the mask is used
> > subsequently to enable/disable some IRQ bits, we may unintentionally
> > unmask unrelated IRQs. I can't see any way that this can lead to a real
> > problem in the current -nightly code, since the first place the mask
> > will be used next (after a suspend/resume cycle) is in
> > valleyview_irq_postinstall(), but the mask is reset there to its proper
> > value.
> >
> > This causes a problem in the upstream kernel though, where - due to another
> > issue - the mask is used in the above way to disable only the display IRQs.
> > This other issue is fixed by:
> >
> > commit 950eabaf5a87257040e0c207be09487954113f54
> > Author: Imre Deak <imre.deak@intel.com>
> > Date: Mon Sep 8 15:21:09 2014 +0300
> >
> > drm/i915: vlv: fix display IRQ enable/disable
> >
> > Interestingly, even with the above two bugs, we shouldn't in theory have
> > any real problems (arguably a famous last sentence:). That's because
> > even if we unmask something unintentionally via the VLV_IMR/VLV_IER
> > register the master IRQ masking bit in VLV_MASTER_IER is still set and
> > should prevent all i915 interrupts. According to my testing on an ASUS
> > T100 with DSI output this isn't the case at least with the
> > MIPIA_INTERRUPT. Leaving this one unmasked in IMR/IER, while having
> > VLV_MASTER_IER set to 0 may lead to a lockup during system suspend as
> > shown in the bugzilla ticket below. This fix should get rid of the
> > problem reported there in upstream and older kernels.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85920
> > Cc: stable@vger.kernel.org (v3.15+)
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_irq.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 8d169e1..4601f53 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -3597,7 +3597,7 @@ static void vlv_display_irq_uninstall(struct drm_i915_private *dev_priv)
> >
> > vlv_display_irq_reset(dev_priv);
> >
> > - dev_priv->irq_mask = 0;
> > + dev_priv->irq_mask = ~0;
>
> vlv seems to be the only platform which resets irq masks in the uninstall
> hook. That smells fishy - can't we just remove this completely?
Yes, we can look into removing it. But on older kernels the irq_mask was
used in the way I described in the commit log, so I'd suggest this fix
for -stable at least, together with commit 950eabaf5 which isn't
upstream yet.
> -Daniel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: vlv: fix IRQ masking when
2014-11-20 14:05 [PATCH] drm/i915: vlv: fix IRQ masking when uninstalling interrupts Imre Deak
2014-11-20 14:11 ` Daniel Vetter
@ 2014-11-22 10:35 ` shuang.he
2014-12-10 17:37 ` [PATCH] drm/i915: vlv: fix IRQ masking when uninstalling interrupts Ville Syrjälä
2 siblings, 0 replies; 6+ messages in thread
From: shuang.he @ 2014-11-22 10:35 UTC (permalink / raw)
To: shuang.he, intel-gfx, imre.deak
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
PNV 367/367 367/367
ILK +2 373/375 375/375
SNB -1 450/450 449/450
IVB -2 503/503 501/503
BYT 289/289 289/289
HSW -3 567/567 564/567
BDW 417/417 417/417
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
ILK igt_kms_flip_flip-vs-panning NSPT(1, M26) PASS(2, M37)
ILK igt_kms_flip_nonexisting-fb DMESG_WARN(1, M26) PASS(2, M37)
SNB igt_kms_rotation_crc_sprite-rotation PASS(1, M35) DMESG_WARN(1, M22)PASS(1, M22)
IVB igt_gem_bad_reloc_negative-reloc PASS(1, M21) NSPT(2, M21)
IVB igt_gem_bad_reloc_negative-reloc-lut PASS(1, M21) NSPT(1, M21)PASS(1, M21)
HSW igt_gem_bad_reloc_negative-reloc-lut PASS(1, M19) NSPT(2, M40)
HSW igt_kms_rotation_crc_primary-rotation PASS(1, M19) DMESG_WARN(1, M40)PASS(1, M40)
HSW igt_pm_rc6_residency_rc6-accuracy PASS(1, M19) FAIL(1, M40)PASS(1, M40)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: vlv: fix IRQ masking when uninstalling interrupts
2014-11-20 14:05 [PATCH] drm/i915: vlv: fix IRQ masking when uninstalling interrupts Imre Deak
2014-11-20 14:11 ` Daniel Vetter
2014-11-22 10:35 ` [PATCH] drm/i915: vlv: fix IRQ masking when shuang.he
@ 2014-12-10 17:37 ` Ville Syrjälä
2014-12-11 14:09 ` [Intel-gfx] " Jani Nikula
2 siblings, 1 reply; 6+ messages in thread
From: Ville Syrjälä @ 2014-12-10 17:37 UTC (permalink / raw)
To: Imre Deak; +Cc: intel-gfx, v3.15+
On Thu, Nov 20, 2014 at 04:05:55PM +0200, Imre Deak wrote:
> irq_mask should include all IRQ bits that we want to mask, but atm we
> set it incorrectly to the inverse of this. If the mask is used
> subsequently to enable/disable some IRQ bits, we may unintentionally
> unmask unrelated IRQs. I can't see any way that this can lead to a real
> problem in the current -nightly code, since the first place the mask
> will be used next (after a suspend/resume cycle) is in
> valleyview_irq_postinstall(), but the mask is reset there to its proper
> value.
>
> This causes a problem in the upstream kernel though, where - due to another
> issue - the mask is used in the above way to disable only the display IRQs.
> This other issue is fixed by:
>
> commit 950eabaf5a87257040e0c207be09487954113f54
> Author: Imre Deak <imre.deak@intel.com>
> Date: Mon Sep 8 15:21:09 2014 +0300
>
> drm/i915: vlv: fix display IRQ enable/disable
>
> Interestingly, even with the above two bugs, we shouldn't in theory have
> any real problems (arguably a famous last sentence:). That's because
> even if we unmask something unintentionally via the VLV_IMR/VLV_IER
> register the master IRQ masking bit in VLV_MASTER_IER is still set and
> should prevent all i915 interrupts. According to my testing on an ASUS
> T100 with DSI output this isn't the case at least with the
> MIPIA_INTERRUPT. Leaving this one unmasked in IMR/IER, while having
> VLV_MASTER_IER set to 0 may lead to a lockup during system suspend as
> shown in the bugzilla ticket below. This fix should get rid of the
> problem reported there in upstream and older kernels.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85920
> Cc: stable@vger.kernel.org (v3.15+)
> Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_irq.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 8d169e1..4601f53 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3597,7 +3597,7 @@ static void vlv_display_irq_uninstall(struct drm_i915_private *dev_priv)
>
> vlv_display_irq_reset(dev_priv);
>
> - dev_priv->irq_mask = 0;
> + dev_priv->irq_mask = ~0;
> }
>
> static void valleyview_irq_uninstall(struct drm_device *dev)
> --
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915: vlv: fix IRQ masking when uninstalling interrupts
2014-12-10 17:37 ` [PATCH] drm/i915: vlv: fix IRQ masking when uninstalling interrupts Ville Syrjälä
@ 2014-12-11 14:09 ` Jani Nikula
0 siblings, 0 replies; 6+ messages in thread
From: Jani Nikula @ 2014-12-11 14:09 UTC (permalink / raw)
To: Ville Syrjälä, Imre Deak; +Cc: intel-gfx, v3.15+
On Wed, 10 Dec 2014, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Nov 20, 2014 at 04:05:55PM +0200, Imre Deak wrote:
>> irq_mask should include all IRQ bits that we want to mask, but atm we
>> set it incorrectly to the inverse of this. If the mask is used
>> subsequently to enable/disable some IRQ bits, we may unintentionally
>> unmask unrelated IRQs. I can't see any way that this can lead to a real
>> problem in the current -nightly code, since the first place the mask
>> will be used next (after a suspend/resume cycle) is in
>> valleyview_irq_postinstall(), but the mask is reset there to its proper
>> value.
>>
>> This causes a problem in the upstream kernel though, where - due to another
>> issue - the mask is used in the above way to disable only the display IRQs.
>> This other issue is fixed by:
>>
>> commit 950eabaf5a87257040e0c207be09487954113f54
>> Author: Imre Deak <imre.deak@intel.com>
>> Date: Mon Sep 8 15:21:09 2014 +0300
>>
>> drm/i915: vlv: fix display IRQ enable/disable
>>
>> Interestingly, even with the above two bugs, we shouldn't in theory have
>> any real problems (arguably a famous last sentence:). That's because
>> even if we unmask something unintentionally via the VLV_IMR/VLV_IER
>> register the master IRQ masking bit in VLV_MASTER_IER is still set and
>> should prevent all i915 interrupts. According to my testing on an ASUS
>> T100 with DSI output this isn't the case at least with the
>> MIPIA_INTERRUPT. Leaving this one unmasked in IMR/IER, while having
>> VLV_MASTER_IER set to 0 may lead to a lockup during system suspend as
>> shown in the bugzilla ticket below. This fix should get rid of the
>> problem reported there in upstream and older kernels.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85920
>> Cc: stable@vger.kernel.org (v3.15+)
>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Pushed to drm-intel-next-fixes, thanks for the patch and review.
BR,
Jani.
>
>> ---
>> drivers/gpu/drm/i915/i915_irq.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index 8d169e1..4601f53 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -3597,7 +3597,7 @@ static void vlv_display_irq_uninstall(struct drm_i915_private *dev_priv)
>>
>> vlv_display_irq_reset(dev_priv);
>>
>> - dev_priv->irq_mask = 0;
>> + dev_priv->irq_mask = ~0;
>> }
>>
>> static void valleyview_irq_uninstall(struct drm_device *dev)
>> --
>> 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
> _______________________________________________
> 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] 6+ messages in thread