drm/i915: properly reenable gen8 pipe IRQs
Subsystems (2)
PATCH 1/2 drm/i915: properly reenable gen8 pipe IRQs
From: Paulo Zanoni <[email protected]>
We were missing the pipe B/C vblank bits! Take a look at gen8_de_irq_postinstall for a comparison.
This should fix a bunch of IGT tests.
There are a few more things we could improve on this code, but this should be the minimal fix to unblock us.
Bugzilla:https://bugs.freedesktop.org/show_bug.cgi?id=83640
Testcase: igt/*
--- drivers/gpu/drm/i915/i915_irq.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +4 -2
@@ -3166,11 +3166,13 @@ static void gen8_irq_reset(struct drm_device *dev)void gen8_irq_power_well_post_enable (struct drm_i915_private *dev_priv) { +uint32_t extra_iir =GEN8_PIPE_VBLANK |GEN8_PIPE_FIFO_UNDERRUN ; +spin_lock_irq (&dev_priv->irq_lock);GEN8_IRQ_INIT_NDX (DE_PIPE ,PIPE_B , dev_priv->de_irq_mask[PIPE_B ], - ~dev_priv->de_irq_mask[PIPE_B ]); + ~dev_priv->de_irq_mask[PIPE_B ] | extra_iir);GEN8_IRQ_INIT_NDX (DE_PIPE ,PIPE_C , dev_priv->de_irq_mask[PIPE_C ], - ~dev_priv->de_irq_mask[PIPE_C ]); + ~dev_priv->de_irq_mask[PIPE_C ] | extra_iir);spin_unlock_irq (&dev_priv->irq_lock); }
Signature · 1 line
2.1.1
PATCH 2/2 drm/i915: only run hsw_power_well_post_enable when really needed
From: Paulo Zanoni <[email protected]>
Only run it after we actually enable the power well. When we're booting the machine there are cases where we run hsw_power_well_post_enable without really needing, and even though this is not causing any real bugs, it is unneeded and causes confusion to people debugging interrupts.
--- drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_runtime_pm.c +1 -1
@@ -221,9 +221,9 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,if (wait_for ((I915_READ (HSW_PWR_WELL_DRIVER ) &HSW_PWR_WELL_STATE_ENABLED ),20 ))DRM_ERROR ("Timeout enabling power well\n" ); +hsw_power_well_post_enable (dev_priv); } -hsw_power_well_post_enable (dev_priv); }else {if (enable_requested) {I915_WRITE (HSW_PWR_WELL_DRIVER ,0 );
Signature · 1 line
2.1.1
Re: [PATCH 2/2] drm/i915: only run hsw_power_well_post_enable when really needed
Quoted 9 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
Only run it after we actually enable the power well. When we're booting the machine there are cases where we run
hsw_power_well_post_enablewithout really needing, and even though this is not causing any real bugs, it is unneeded and causes confusion to people debugging interrupts.Signed-off-by Paulo Zanoni <[email protected]>
Seems perfectly sensible.
Quoted 26 lines
--- drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/gpu/drm/i915/intel_runtime_pm.c +1
-1@@ -221,9 +221,9 @@ static void hsw_set_power_well(struct drm_i915_private *dev_priv,if (wait_for ((I915_READ (HSW_PWR_WELL_DRIVER ) &HSW_PWR_WELL_STATE_ENABLED ),20 ))DRM_ERROR ("Timeout enabling power well\n" ); +hsw_power_well_post_enable (dev_priv); } -hsw_power_well_post_enable (dev_priv); }else {if (enable_requested) {I915_WRITE (HSW_PWR_WELL_DRIVER ,0 );Signature · 6 lines
2.1.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 2 lines
Ville Syrjälä Intel OTC
Daniel Vetter
AppliedRe: [PATCH 2/2] drm/i915: only run hsw_power_well_post_enable when really needed
Quoted 9 lines · Ville Syrjälä
Quoted 8 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
Only run it after we actually enable the power well. When we're booting the machine there are cases where we run
hsw_power_well_post_enablewithout really needing, and even though this is not causing any real bugs, it is unneeded and causes confusion to people debugging interrupts.
Quoted 5 lines
Quoted 1 line
Signed-off-by Paulo Zanoni <[email protected]>Seems perfectly sensible.
Reviewed-by Ville Syrjälä <[email protected]>
Queued for -next, thanks for the patch.
-Daniel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Quoted 23 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
We were missing the pipe B/C vblank bits! Take a look at
gen8_de_irq_postinstallfor a comparison.This should fix a bunch of IGT tests.
There are a few more things we could improve on this code, but this should be the minimal fix to unblock us.
Bugzilla:https://bugs.freedesktop.org/show_bug.cgi?id=83640
Testcase: igt/*Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_irq.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)drivers/gpu/drm/i915/i915_irq.c +0
-0
drivers/gpu/drm/i915/i915_irq.c static void gen8_irq_reset(struct drm_device *dev) · line 3169
Quoted 4 lines
drivers/gpu/drm/i915/i915_irq.c +1
-0@@ -3166,11 +3166,13 @@ static void gen8_irq_reset(struct drm_device *dev)void gen8_irq_power_well_post_enable (struct drm_i915_private *dev_priv) { +uint32_t extra_iir =GEN8_PIPE_VBLANK |GEN8_PIPE_FIFO_UNDERRUN ;
extra_iir seems a bit weird as a name when this is about IER. Maybe
uint32_t de_pipe_enables = ~dev_priv->de_irq_mask | ...;
to better match gen8_de_irq_postinstall()?
In any case now those reported vblank timeout failures at least make some sense, so even w/o that change:
But IIRC we had some vblank timeout reports on other platforms too (PNV maybe?). Wouldn't it be cute if this patch cured those too :)
And I'll end with the rant of the day:
Boy do I hate that stupid ~ we have to sprinkle around everywhere. Almost makes me wish someone would go and hide that in some special IMR register access macro so we wouldn't have to invert bits all the time.
Quoted 18 lines
drivers/gpu/drm/i915/i915_irq.c +3
-2+spin_lock_irq (&dev_priv->irq_lock);GEN8_IRQ_INIT_NDX (DE_PIPE ,PIPE_B , dev_priv->de_irq_mask[PIPE_B ], - ~dev_priv->de_irq_mask[PIPE_B ]); + ~dev_priv->de_irq_mask[PIPE_B ] | extra_iir);GEN8_IRQ_INIT_NDX (DE_PIPE ,PIPE_C , dev_priv->de_irq_mask[PIPE_C ], - ~dev_priv->de_irq_mask[PIPE_C ]); + ~dev_priv->de_irq_mask[PIPE_C ] | extra_iir);spin_unlock_irq (&dev_priv->irq_lock); }Signature · 6 lines
2.1.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 2 lines
Ville Syrjälä Intel OTC
2014-10-07 16:58 GMT-03:00 Ville Syrjälä <[email protected]>:
Quoted 25 lines · Paulo Zanoni
Quoted 24 lines
From: Paulo Zanoni <[email protected]>
We were missing the pipe B/C vblank bits! Take a look at
gen8_de_irq_postinstallfor a comparison.This should fix a bunch of IGT tests.
There are a few more things we could improve on this code, but this should be the minimal fix to unblock us.
Bugzilla:https://bugs.freedesktop.org/show_bug.cgi?id=83640
Testcase: igt/*Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_irq.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)drivers/gpu/drm/i915/i915_irq.c +0
-0@@ -3166,11 +3166,13 @@ static void gen8_irq_reset(struct drm_device *dev)
Quoted 5 lines
drivers/gpu/drm/i915/i915_irq.c static void gen8_irq_reset(struct drm_device *dev) · line 3169
Quoted 3 lines
drivers/gpu/drm/i915/i915_irq.c +1
-0void gen8_irq_power_well_post_enable (struct drm_i915_private *dev_priv) { +uint32_t extra_iir =GEN8_PIPE_VBLANK |GEN8_PIPE_FIFO_UNDERRUN ;
extra_iirseems a bit weird as a name when this is about IER. Maybe
In my head, this was supposed to be named extra_ier... Why would I name it extra_iir?
Quoted 2 lines
uint32_tde_pipe_enables= ~dev_priv->de_irq_mask | ...;
The problem is that since we have de_irq_mask for both pipes B and C, I'd have to set de_pipe_enables twice. Even if they're supposed to be the same thing today, I prefer to not depend on this fact.
I'll send a new version with another variable name.
Quoted 10 lines
to better match
gen8_de_irq_postinstall()?In any case now those reported vblank timeout failures at least make some sense, so even w/o that change:
Reviewed-by Ville Syrjälä <[email protected]>But IIRC we had some vblank timeout reports on other platforms too (PNV maybe?). Wouldn't it be cute if this patch cured those too :)
For the eventual readers unfamiliar with the hardware: he knows this patch won't fix PNV.
Quoted 5 lines
And I'll end with the rant of the day:
Boy do I hate that stupid ~ we have to sprinkle around everywhere. Almost makes me wish someone would go and hide that in some special IMR register access macro so we wouldn't have to invert bits all the time.
Yeah, it is confusing...
Quoted 23 lines
Quoted 15 lines
drivers/gpu/drm/i915/i915_irq.c +3
-3+spin_lock_irq (&dev_priv->irq_lock);GEN8_IRQ_INIT_NDX (DE_PIPE ,PIPE_B , dev_priv->de_irq_mask[PIPE_B ], - ~dev_priv->de_irq_mask[PIPE_B ]); + ~dev_priv->de_irq_mask[PIPE_B ] | extra_iir);GEN8_IRQ_INIT_NDX (DE_PIPE ,PIPE_C , dev_priv->de_irq_mask[PIPE_C ], - ~dev_priv->de_irq_mask[PIPE_C ]); + ~dev_priv->de_irq_mask[PIPE_C ] | extra_iir);spin_unlock_irq (&dev_priv->irq_lock); } --2.1.1
Quoted 4 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx--
Ville Syrjälä
Intel OTC
Signature · 1 line
Paulo Zanoni
PATCH 1/1 drm/i915: properly reenable gen8 pipe IRQs
From: Paulo Zanoni <[email protected]>
We were missing the pipe B/C vblank bits! Take a look at gen8_de_irq_postinstall for a comparison.
This should fix a bunch of IGT tests.
There are a few more things we could improve on this code, but this should be the minimal fix to unblock us.
v2: s/extra_iir/extra_ier/ because IIR doesn't make sense (Ville)
Bugzilla:https://bugs.freedesktop.org/show_bug.cgi?id=83640
Testcase: igt/*
--- drivers/gpu/drm/i915/i915_irq.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +4 -2
@@ -3166,11 +3166,13 @@ static void gen8_irq_reset(struct drm_device *dev)void gen8_irq_power_well_post_enable (struct drm_i915_private *dev_priv) { +uint32_t extra_ier =GEN8_PIPE_VBLANK |GEN8_PIPE_FIFO_UNDERRUN ; +spin_lock_irq (&dev_priv->irq_lock);GEN8_IRQ_INIT_NDX (DE_PIPE ,PIPE_B , dev_priv->de_irq_mask[PIPE_B ], - ~dev_priv->de_irq_mask[PIPE_B ]); + ~dev_priv->de_irq_mask[PIPE_B ] | extra_ier);GEN8_IRQ_INIT_NDX (DE_PIPE ,PIPE_C , dev_priv->de_irq_mask[PIPE_C ], - ~dev_priv->de_irq_mask[PIPE_C ]); + ~dev_priv->de_irq_mask[PIPE_C ] | extra_ier);spin_unlock_irq (&dev_priv->irq_lock); }
Signature · 1 line
2.1.1
Jani Nikula
maintainerQuoted 5 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
We were missing the pipe B/C vblank bits! Take a look at
gen8_de_irq_postinstallfor a comparison.
Quoted 8 lines
This should fix a bunch of IGT tests.
There are a few more things we could improve on this code, but this should be the minimal fix to unblock us.
v2: s/extra_iir/extra_ier/ because IIR doesn't make sense (Ville)
Maybe https://bugs.freedesktop.org/show_bug.cgi?id=84217 too, and others?
BR,
Jani.
Quoted 34 lines
Testcase: igt/*
Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_irq.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)drivers/gpu/drm/i915/i915_irq.c +4
-2@@ -3166,11 +3166,13 @@ static void gen8_irq_reset(struct drm_device *dev)void gen8_irq_power_well_post_enable (struct drm_i915_private *dev_priv) { +uint32_t extra_ier =GEN8_PIPE_VBLANK |GEN8_PIPE_FIFO_UNDERRUN ; +spin_lock_irq (&dev_priv->irq_lock);GEN8_IRQ_INIT_NDX (DE_PIPE ,PIPE_B , dev_priv->de_irq_mask[PIPE_B ], - ~dev_priv->de_irq_mask[PIPE_B ]); + ~dev_priv->de_irq_mask[PIPE_B ] | extra_ier);GEN8_IRQ_INIT_NDX (DE_PIPE ,PIPE_C , dev_priv->de_irq_mask[PIPE_C ], - ~dev_priv->de_irq_mask[PIPE_C ]); + ~dev_priv->de_irq_mask[PIPE_C ] | extra_ier);spin_unlock_irq (&dev_priv->irq_lock); }Signature · 6 lines
2.1.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 1 line
Jani Nikula, Intel Open Source Technology Center
2014-10-08 5:25 GMT-03:00 Jani Nikula <[email protected]>:
Quoted 11 lines · Paulo Zanoni
Quoted 10 lines
From: Paulo Zanoni <[email protected]>
We were missing the pipe B/C vblank bits! Take a look at
gen8_de_irq_postinstallfor a comparison.This should fix a bunch of IGT tests.
There are a few more things we could improve on this code, but this should be the minimal fix to unblock us.
Quoted 6 lines
Quoted 3 lines
v2: s/extra_iir/extra_ier/ because IIR doesn't make sense (Ville)
Maybe https://bugs.freedesktop.org/show_bug.cgi?id=84217 too, and others?
Yeah, I can confirm that this patch fixes at least 84162, 83969, 84217 and 83640 from bugzilla.freedesktop.org on my machine.
Quoted 42 lines
BR,
Jani.Quoted 30 lines
Testcase: igt/*
Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_irq.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)drivers/gpu/drm/i915/i915_irq.c +4
-3@@ -3166,11 +3166,13 @@ static void gen8_irq_reset(struct drm_device *dev)void gen8_irq_power_well_post_enable (struct drm_i915_private *dev_priv) { +uint32_t extra_ier =GEN8_PIPE_VBLANK |GEN8_PIPE_FIFO_UNDERRUN ; +spin_lock_irq (&dev_priv->irq_lock);GEN8_IRQ_INIT_NDX (DE_PIPE ,PIPE_B , dev_priv->de_irq_mask[PIPE_B ], - ~dev_priv->de_irq_mask[PIPE_B ]); + ~dev_priv->de_irq_mask[PIPE_B ] | extra_ier);GEN8_IRQ_INIT_NDX (DE_PIPE ,PIPE_C , dev_priv->de_irq_mask[PIPE_C ], - ~dev_priv->de_irq_mask[PIPE_C ]); + ~dev_priv->de_irq_mask[PIPE_C ] | extra_ier);spin_unlock_irq (&dev_priv->irq_lock); } --2.1.1
Quoted 4 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx--
Jani Nikula, Intel Open Source Technology Center
Signature · 1 line
Paulo Zanoni
Quoted 10 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
We were missing the pipe B/C vblank bits! Take a look at
gen8_de_irq_postinstallfor a comparison.This should fix a bunch of IGT tests.
There are a few more things we could improve on this code, but this should be the minimal fix to unblock us.
Quoted 6 lines
v2: s/extra_iir/extra_ier/ because IIR doesn't make sense (Ville)
Bugzilla:https://bugs.freedesktop.org/show_bug.cgi?id=83640
Testcase: igt/*Signed-off-by Paulo Zanoni <[email protected]>
Pushed to drm-intel-next-fixes, thanks for the patch and review.
Paulo, please ask for retests on all relevant bug reports.
BR,
Jani.
Quoted 31 lines
--- drivers/gpu/drm/i915/i915_irq.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)drivers/gpu/drm/i915/i915_irq.c +4
-2@@ -3166,11 +3166,13 @@ static void gen8_irq_reset(struct drm_device *dev)void gen8_irq_power_well_post_enable (struct drm_i915_private *dev_priv) { +uint32_t extra_ier =GEN8_PIPE_VBLANK |GEN8_PIPE_FIFO_UNDERRUN ; +spin_lock_irq (&dev_priv->irq_lock);GEN8_IRQ_INIT_NDX (DE_PIPE ,PIPE_B , dev_priv->de_irq_mask[PIPE_B ], - ~dev_priv->de_irq_mask[PIPE_B ]); + ~dev_priv->de_irq_mask[PIPE_B ] | extra_ier);GEN8_IRQ_INIT_NDX (DE_PIPE ,PIPE_C , dev_priv->de_irq_mask[PIPE_C ], - ~dev_priv->de_irq_mask[PIPE_C ]); + ~dev_priv->de_irq_mask[PIPE_C ] | extra_ier);spin_unlock_irq (&dev_priv->irq_lock); }Signature · 6 lines
2.1.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 1 line
Jani Nikula, Intel Open Source Technology Center