Haswell unclaimed register fixes and power well enabling
Subsystems (2)
cover Haswell unclaimed register fixes and power well enabling
From: Paulo Zanoni <[email protected]>
Hi
This series fixes a few of our current "unclaimed register" messages, then it introduces the code to disable the power well when we're using eDP only, and then it fixes a few more "unclaimed register" messages brought by the power down code. In the last patches we remove the GEN7_ERR_INT checking from I915_WRITE and move it to the interrupt code.
Quoted 1 line
From my tests, after this series I still only see 1 "unclaimed register" message
that seems to be caused by our driver. All the other messages (there are a lot of messages that happen all together when booting) stop when we blacklist snd_hda_intel and its friends. Still, the plan is to get rid of all the remaining messages.
Cheers,
Paulo
Daniel Vetter (1):
drm/i915: dynamic Haswell display power well support
Paulo Zanoni (9):
drm/i915: don't save/restore DSPARB on gen5+ drm/i915: don't read DP_TP_STATUS(PORT_A)
drm/i915: fix intel_init_power_wells
drm/i915: only disable enabled planes on intel_fb_restore_mode drm/i915: check the power down well on assert_pipe() drm/i915: turn on the power well before suspending drm/i915: set TRANSCODER_EDP even earlier
drm/i915: print IVB/HSW display error interrupts drm/i915: remove "unclaimed register" checks from I915_WRITE
drivers/gpu/drm/i915/i915_drv.c | 10 +---- drivers/gpu/drm/i915/i915_irq.c | 56 ++++++++++++++++++++++++++- drivers/gpu/drm/i915/i915_reg.h | 31 ++++++++++++--- drivers/gpu/drm/i915/i915_suspend.c | 6 ++- drivers/gpu/drm/i915/intel_ddi.c | 8 +++- drivers/gpu/drm/i915/intel_display.c | 58 ++++++++++++++++++++++------ drivers/gpu/drm/i915/intel_dp.c | 9 +++-- drivers/gpu/drm/i915/intel_drv.h | 3 +- drivers/gpu/drm/i915/intel_fb.c | 3 +- drivers/gpu/drm/i915/intel_pm.c | 70 +++++++++++++++++++++++++--------- 10 files changed, 202 insertions(+), 52 deletions(-)
Signature · 1 line
1.7.10.4
PATCH 1/10 drm/i915: don't save/restore DSPARB on gen5+
From: Paulo Zanoni <[email protected]>
Because the register does not exist in gen5+.
This patch solves "unclaimed register" messages on Haswell after suspend/resume.
--- drivers/gpu/drm/i915/i915_suspend.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/i915_suspend.c +4 -2
@@ -620,7 +620,8 @@ static void i915_save_display(struct drm_device *dev)struct drm_i915_private *dev_priv = dev->dev_private;/* Display arbitration control */ - dev_priv->regfile.saveDSPARB =I915_READ (DSPARB ); +if (INTEL_INFO (dev)->gen <=4 ) + dev_priv->regfile.saveDSPARB =I915_READ (DSPARB );/* This is only meaningful in non-KMS mode */ /* Don't regfile.save them in KMS mode */
@@ -707,7 +708,8 @@ static void i915_restore_display(struct drm_device *dev)struct drm_i915_private *dev_priv = dev->dev_private;/* Display arbitration */ -I915_WRITE (DSPARB , dev_priv->regfile.saveDSPARB); +if (INTEL_INFO (dev)->gen <=4 ) +I915_WRITE (DSPARB , dev_priv->regfile.saveDSPARB);if (!drm_core_check_feature (dev,DRIVER_MODESET )) {/* Display port ratios (must be done before clock is set) */
Signature · 1 line
1.7.10.4
Re: [PATCH 01/10] drm/i915: don't save/restore DSPARB on gen5+
Quoted 6 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
Because the register does not exist in gen5+.
This patch solves "unclaimed register" messages on Haswell after suspend/resume.
Quoted 37 lines
Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_suspend.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)drivers/gpu/drm/i915/i915_suspend.c +4
-2@@ -620,7 +620,8 @@ static void i915_save_display(struct drm_device *dev)struct drm_i915_private *dev_priv = dev->dev_private;/* Display arbitration control */ - dev_priv->regfile.saveDSPARB =I915_READ (DSPARB ); +if (INTEL_INFO (dev)->gen <=4 ) + dev_priv->regfile.saveDSPARB =I915_READ (DSPARB );/* This is only meaningful in non-KMS mode */ /* Don't regfile.save them in KMS mode */ @@ -707,7 +708,8 @@ static void i915_restore_display(struct drm_device *dev)struct drm_i915_private *dev_priv = dev->dev_private;/* Display arbitration */ -I915_WRITE (DSPARB , dev_priv->regfile.saveDSPARB); +if (INTEL_INFO (dev)->gen <=4 ) +I915_WRITE (DSPARB , dev_priv->regfile.saveDSPARB);if (!drm_core_check_feature (dev,DRIVER_MODESET )) {/* Display port ratios (must be done before clock is set) */ Signature · 6 lines
1.7.10.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter
AppliedRe: [PATCH 01/10] drm/i915: don't save/restore DSPARB on gen5+
Quoted 9 lines · Jani Nikula
Quoted 6 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
Because the register does not exist in gen5+.
This patch solves "unclaimed register" messages on Haswell after suspend/resume.
Reviewed-by Jani Nikula <[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
PATCH 2/10 drm/i915: don't read DP_TP_STATUS(PORT_A)
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
Our documentation is wrong, this register does not exist on port A, so sleep 800us since it's the timeout mentioned on the mode set sequence document.
This fixes error messages, including "unclaimed register".
--- drivers/gpu/drm/i915/intel_dp.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +6 -3
@@ -1788,9 +1788,12 @@ intel_dp_set_link_train(struct intel_dp *intel_dp, temp |=DP_TP_CTL_LINK_TRAIN_IDLE ;I915_WRITE (DP_TP_CTL (port), temp); -if (wait_for ((I915_READ (DP_TP_STATUS (port)) & -DP_TP_STATUS_IDLE_DONE ),1 )) -DRM_ERROR ("Timed out waiting for DP idle patterns\n" ); +if (port ==PORT_A ) +udelay (800 ); +else +if (wait_for ((I915_READ (DP_TP_STATUS (port)) & +DP_TP_STATUS_IDLE_DONE ),1 )) +DRM_ERROR ("Timed out waiting for DP idle patterns\n" ); temp &= ~DP_TP_CTL_LINK_TRAIN_MASK ; temp |=DP_TP_CTL_LINK_TRAIN_NORMAL ;
Signature · 1 line
1.7.10.4
Jani Nikula
maintainerRe: [PATCH 02/10] drm/i915: don't read DP_TP_STATUS(PORT_A)
Quoted 5 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
Our documentation is wrong, this register does not exist on port A, so sleep 800us since it's the timeout mentioned on the mode set sequence document.
Would be nice to have some reference, like documentation, to back up the claim that the documentation is wrong...
Maybe a comment near #define DP_TP_STATUS() in i915_reg.h would be in order too.
Quoted 13 lines
This fixes error messages, including "unclaimed register".
Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/intel_dp.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)drivers/gpu/drm/i915/intel_dp.c +0
-0
drivers/gpu/drm/i915/intel_dp.c intel_dp_set_link_train(struct intel_dp *intel_dp, · line 1792
Quoted 8 lines
drivers/gpu/drm/i915/intel_dp.c +2
-3@@ -1788,9 +1788,12 @@ intel_dp_set_link_train(struct intel_dp *intel_dp, temp |=DP_TP_CTL_LINK_TRAIN_IDLE ;I915_WRITE (DP_TP_CTL (port), temp); -if (wait_for ((I915_READ (DP_TP_STATUS (port)) & -DP_TP_STATUS_IDLE_DONE ),1 )) -DRM_ERROR ("Timed out waiting for DP idle patterns\n" ); +if (port ==PORT_A ) +udelay (800 );
usleep_range()?
Quoted 14 lines
drivers/gpu/drm/i915/intel_dp.c +4
-0+else +if (wait_for ((I915_READ (DP_TP_STATUS (port)) & +DP_TP_STATUS_IDLE_DONE ),1 )) +DRM_ERROR ("Timed out waiting for DP idle patterns\n" ); temp &= ~DP_TP_CTL_LINK_TRAIN_MASK ; temp |=DP_TP_CTL_LINK_TRAIN_NORMAL ;Signature · 6 lines
1.7.10.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 3/10 drm/i915: fix intel_init_power_wells
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
The current code was wrong in many different ways, so this is a full rewrite. We don't have "different power wells for different parts of the GPU", we have a single power well, but we have multiple registers that can be used to request enabling/disabling the power well. So let's be a good citizen and only use the register we're supposed to use, except when we're loading the driver, where we clear the request made by the BIOS.
If any of the registers is requesting the power well to be enabled, it will be enabled. If none of the registers is requesting the power well to be enabled, it will be disabled.
For now we're just forcing the power well to be enabled, but in the next commits we'll change this.
--- drivers/gpu/drm/i915/i915_reg.h | 8 ++-- drivers/gpu/drm/i915/intel_display.c | 5 +-- drivers/gpu/drm/i915/intel_drv.h | 2 +- drivers/gpu/drm/i915/intel_pm.c | 70 +++++++++++++++++++++++++--------- 4 files changed, 59 insertions(+), 26 deletions(-)
drivers/gpu/drm/i915/i915_reg.h +4 -4
@@ -4414,10 +4414,10 @@#define AUDIO_CP_READY_C (1<<9)/* HSW Power Wells */ -#define HSW_PWR_WELL_CTL1 0x45400/* BIOS */ -#define HSW_PWR_WELL_CTL2 0x45404/* Driver */ -#define HSW_PWR_WELL_CTL3 0x45408/* KVMR */ -#define HSW_PWR_WELL_CTL4 0x4540C/* Debug */ +#define HSW_PWR_WELL_BIOS 0x45400/* CTL1 */ +#define HSW_PWR_WELL_DRIVER 0x45404/* CTL2 */ +#define HSW_PWR_WELL_KVMR 0x45408/* CTL3 */ +#define HSW_PWR_WELL_DEBUG 0x4540C/* CTL4 */ #define HSW_PWR_WELL_ENABLE (1<<31)#define HSW_PWR_WELL_STATE (1<<30)#define HSW_PWR_WELL_CTL5 0x45410
drivers/gpu/drm/i915/intel_display.c +1 -4
@@ -8647,10 +8647,7 @@ static void i915_disable_vga(struct drm_device *dev)void intel_modeset_init_hw (struct drm_device *dev) { -/* We attempt to init the necessary power wells early in the initialization -* time, so the subsystems that expect power to be enabled can work. -*/ -intel_init_power_wells (dev); +intel_init_power_well (dev);intel_prepare_ddi (dev);
drivers/gpu/drm/i915/intel_drv.h +1 -1
@@ -666,7 +666,7 @@ extern void intel_update_fbc(struct drm_device *dev);extern void intel_gpu_ips_init (struct drm_i915_private *dev_priv);extern void intel_gpu_ips_teardown (void ); -extern void intel_init_power_wells (struct drm_device *dev); +extern void intel_init_power_well (struct drm_device *dev);extern void intel_enable_gt_powersave (struct drm_device *dev);extern void intel_disable_gt_powersave (struct drm_device *dev);extern void gen6_gt_check_fifodbg (struct drm_i915_private *dev_priv);
drivers/gpu/drm/i915/intel_pm.c +53 -17
86 lines not on this page · raw message
Signature · 1 line
1.7.10.4
Re: [PATCH 03/10] drm/i915: fix intel_init_power_wells
Quoted 112 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
The current code was wrong in many different ways, so this is a full rewrite. We don't have "different power wells for different parts of the GPU", we have a single power well, but we have multiple registers that can be used to request enabling/disabling the power well. So let's be a good citizen and only use the register we're supposed to use, except when we're loading the driver, where we clear the request made by the BIOS.
If any of the registers is requesting the power well to be enabled, it will be enabled. If none of the registers is requesting the power well to be enabled, it will be disabled.
For now we're just forcing the power well to be enabled, but in the next commits we'll change this.
Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_reg.h | 8 ++-- drivers/gpu/drm/i915/intel_display.c | 5 +-- drivers/gpu/drm/i915/intel_drv.h | 2 +- drivers/gpu/drm/i915/intel_pm.c | 70 +++++++++++++++++++++++++--------- 4 files changed, 59 insertions(+), 26 deletions(-)drivers/gpu/drm/i915/i915_reg.h +4
-4@@ -4414,10 +4414,10 @@#define AUDIO_CP_READY_C (1<<9)/* HSW Power Wells */ -#define HSW_PWR_WELL_CTL1 0x45400/* BIOS */ -#define HSW_PWR_WELL_CTL2 0x45404/* Driver */ -#define HSW_PWR_WELL_CTL3 0x45408/* KVMR */ -#define HSW_PWR_WELL_CTL4 0x4540C/* Debug */ +#define HSW_PWR_WELL_BIOS 0x45400/* CTL1 */ +#define HSW_PWR_WELL_DRIVER 0x45404/* CTL2 */ +#define HSW_PWR_WELL_KVMR 0x45408/* CTL3 */ +#define HSW_PWR_WELL_DEBUG 0x4540C/* CTL4 */ #define HSW_PWR_WELL_ENABLE (1<<31)#define HSW_PWR_WELL_STATE (1<<30)#define HSW_PWR_WELL_CTL5 0x45410drivers/gpu/drm/i915/intel_display.c +1
-4@@ -8647,10 +8647,7 @@ static void i915_disable_vga(struct drm_device *dev)void intel_modeset_init_hw (struct drm_device *dev) { -/* We attempt to init the necessary power wells early in the initialization -* time, so the subsystems that expect power to be enabled can work. -*/ -intel_init_power_wells (dev); +intel_init_power_well (dev);intel_prepare_ddi (dev);drivers/gpu/drm/i915/intel_drv.h +1
-1@@ -666,7 +666,7 @@ extern void intel_update_fbc(struct drm_device *dev);extern void intel_gpu_ips_init (struct drm_i915_private *dev_priv);extern void intel_gpu_ips_teardown (void ); -extern void intel_init_power_wells (struct drm_device *dev); +extern void intel_init_power_well (struct drm_device *dev);extern void intel_enable_gt_powersave (struct drm_device *dev);extern void intel_disable_gt_powersave (struct drm_device *dev);extern void gen6_gt_check_fifodbg (struct drm_i915_private *dev_priv);drivers/gpu/drm/i915/intel_pm.c +29
-2@@ -4043,33 +4043,69 @@ void intel_init_clock_gating(struct drm_device *dev) dev_priv->display.init_clock_gating (dev); } -/* Starting with Haswell, we have different power wells for -* different parts of the GPU. This attempts to enable them all. +static void intel_set_power_well (struct drm_device *dev,bool enable) +{ +struct drm_i915_private *dev_priv = dev->dev_private; +bool is_enabled, enable_requested; +uint32_t tmp; + + tmp =I915_READ (HSW_PWR_WELL_DRIVER ); + is_enabled = !!(tmp &HSW_PWR_WELL_STATE ); + enable_requested = !!(tmp &HSW_PWR_WELL_ENABLE ); + +if (enable) { +if (!enable_requested) +I915_WRITE (HSW_PWR_WELL_DRIVER ,HSW_PWR_WELL_ENABLE ); + +if (!is_enabled) { +DRM_DEBUG_KMS ("Enabling power well\n" ); +if (wait_for ((I915_READ (HSW_PWR_WELL_DRIVER ) & +HSW_PWR_WELL_STATE ),20 )) +DRM_ERROR ("Timeout enabling power well\n" ); + } + }else { +if (enable_requested) +I915_WRITE (HSW_PWR_WELL_DRIVER ,0 ); + +if (is_enabled) { +if (I915_READ (HSW_PWR_WELL_BIOS ) &HSW_PWR_WELL_ENABLE ) +DRM_DEBUG_KMS ("Not disabling power well: requested by BIOS\n" ); +else if (I915_READ (HSW_PWR_WELL_KVMR ) &HSW_PWR_WELL_ENABLE ) +DRM_DEBUG_KMS ("Not disabling power well: requested by KVMR\n" );
drivers/gpu/drm/i915/intel_pm.c void intel_init_clock_gating(struct drm_device *dev) · line 4084
Quoted 10 lines
drivers/gpu/drm/i915/intel_pm.c +10
-0+else if (I915_READ (HSW_PWR_WELL_DEBUG ) &HSW_PWR_WELL_ENABLE ) +DRM_DEBUG_KMS ("Not disabling power well: requested by DEBUG\n" ); +else { +DRM_DEBUG_KMS ("Disabling power well\n" ); +if (wait_for ((I915_READ (HSW_PWR_WELL_DRIVER ) & +HSW_PWR_WELL_STATE ) ==0 ,20 )) +DRM_ERROR ("Timeout disabling power well\n" ); + } + } + }
The documentation says not to touch the enable bits if there's a state transition already in progress. I have no idea how we're supposed to do that without all kinds of race conditions due multiple entities making requests simultaneosly. Do you know something about this that's not in the documentation?
Apart from that the patch looks good to me.
Quoted 49 lines
drivers/gpu/drm/i915/intel_pm.c +14
-15+} + +/* +* Starting with Haswell, we have a "Power Down Well" that can be turned off +* when not needed anymore. We have 4 registers that can request the power well +* to be enabled, and it will only be disabled if none of the registers is +* requesting it to be enabled. */ -void intel_init_power_wells (struct drm_device *dev) +void intel_init_power_well (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private; -unsigned long power_wells[] = { -HSW_PWR_WELL_CTL1 , -HSW_PWR_WELL_CTL2 , -HSW_PWR_WELL_CTL4 - }; -int i;if (!IS_HASWELL (dev))return ;mutex_lock (&dev->struct_mutex); -for (i =0 ; i <ARRAY_SIZE (power_wells); i++) { -int well =I915_READ (power_wells[i]); +/* For now, we need the power well to be always enabled. */ +intel_set_power_well (dev, true); -if ((well &HSW_PWR_WELL_STATE ) ==0 ) { -I915_WRITE (power_wells[i], well &HSW_PWR_WELL_ENABLE ); -if (wait_for ((I915_READ (power_wells[i]) &HSW_PWR_WELL_STATE ),20 )) -DRM_ERROR ("Error enabling power well %lx\n" , power_wells[i]); - } - } +/* We're taking over the BIOS, so clear any requests made by it since +* the driver is in charge now. */ +if (I915_READ (HSW_PWR_WELL_BIOS ) &HSW_PWR_WELL_ENABLE ) +I915_WRITE (HSW_PWR_WELL_BIOS ,0 );mutex_unlock (&dev->struct_mutex); }Signature · 6 lines
1.7.10.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 2 lines
Ville Syrjälä Intel OTC
Re: [PATCH 03/10] drm/i915: fix intel_init_power_wells
Quoted 130 lines · Ville Syrjälä
Quoted 112 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
The current code was wrong in many different ways, so this is a full rewrite. We don't have "different power wells for different parts of the GPU", we have a single power well, but we have multiple registers that can be used to request enabling/disabling the power well. So let's be a good citizen and only use the register we're supposed to use, except when we're loading the driver, where we clear the request made by the BIOS.
If any of the registers is requesting the power well to be enabled, it will be enabled. If none of the registers is requesting the power well to be enabled, it will be disabled.
For now we're just forcing the power well to be enabled, but in the next commits we'll change this.
Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_reg.h | 8 ++-- drivers/gpu/drm/i915/intel_display.c | 5 +-- drivers/gpu/drm/i915/intel_drv.h | 2 +- drivers/gpu/drm/i915/intel_pm.c | 70 +++++++++++++++++++++++++--------- 4 files changed, 59 insertions(+), 26 deletions(-)drivers/gpu/drm/i915/i915_reg.h +4
-4@@ -4414,10 +4414,10 @@#define AUDIO_CP_READY_C (1<<9)/* HSW Power Wells */ -#define HSW_PWR_WELL_CTL1 0x45400/* BIOS */ -#define HSW_PWR_WELL_CTL2 0x45404/* Driver */ -#define HSW_PWR_WELL_CTL3 0x45408/* KVMR */ -#define HSW_PWR_WELL_CTL4 0x4540C/* Debug */ +#define HSW_PWR_WELL_BIOS 0x45400/* CTL1 */ +#define HSW_PWR_WELL_DRIVER 0x45404/* CTL2 */ +#define HSW_PWR_WELL_KVMR 0x45408/* CTL3 */ +#define HSW_PWR_WELL_DEBUG 0x4540C/* CTL4 */ #define HSW_PWR_WELL_ENABLE (1<<31)#define HSW_PWR_WELL_STATE (1<<30)#define HSW_PWR_WELL_CTL5 0x45410drivers/gpu/drm/i915/intel_display.c +1
-4@@ -8647,10 +8647,7 @@ static void i915_disable_vga(struct drm_device *dev)void intel_modeset_init_hw (struct drm_device *dev) { -/* We attempt to init the necessary power wells early in the initialization -* time, so the subsystems that expect power to be enabled can work. -*/ -intel_init_power_wells (dev); +intel_init_power_well (dev);intel_prepare_ddi (dev);drivers/gpu/drm/i915/intel_drv.h +1
-1@@ -666,7 +666,7 @@ extern void intel_update_fbc(struct drm_device *dev);extern void intel_gpu_ips_init (struct drm_i915_private *dev_priv);extern void intel_gpu_ips_teardown (void ); -extern void intel_init_power_wells (struct drm_device *dev); +extern void intel_init_power_well (struct drm_device *dev);extern void intel_enable_gt_powersave (struct drm_device *dev);extern void intel_disable_gt_powersave (struct drm_device *dev);extern void gen6_gt_check_fifodbg (struct drm_i915_private *dev_priv);drivers/gpu/drm/i915/intel_pm.c +29
-2@@ -4043,33 +4043,69 @@ void intel_init_clock_gating(struct drm_device *dev) dev_priv->display.init_clock_gating (dev); } -/* Starting with Haswell, we have different power wells for -* different parts of the GPU. This attempts to enable them all. +static void intel_set_power_well (struct drm_device *dev,bool enable) +{ +struct drm_i915_private *dev_priv = dev->dev_private; +bool is_enabled, enable_requested; +uint32_t tmp; + + tmp =I915_READ (HSW_PWR_WELL_DRIVER ); + is_enabled = !!(tmp &HSW_PWR_WELL_STATE ); + enable_requested = !!(tmp &HSW_PWR_WELL_ENABLE ); + +if (enable) { +if (!enable_requested) +I915_WRITE (HSW_PWR_WELL_DRIVER ,HSW_PWR_WELL_ENABLE ); + +if (!is_enabled) { +DRM_DEBUG_KMS ("Enabling power well\n" ); +if (wait_for ((I915_READ (HSW_PWR_WELL_DRIVER ) & +HSW_PWR_WELL_STATE ),20 )) +DRM_ERROR ("Timeout enabling power well\n" ); + } + }else { +if (enable_requested) +I915_WRITE (HSW_PWR_WELL_DRIVER ,0 ); + +if (is_enabled) { +if (I915_READ (HSW_PWR_WELL_BIOS ) &HSW_PWR_WELL_ENABLE ) +DRM_DEBUG_KMS ("Not disabling power well: requested by BIOS\n" ); +else if (I915_READ (HSW_PWR_WELL_KVMR ) &HSW_PWR_WELL_ENABLE ) +DRM_DEBUG_KMS ("Not disabling power well: requested by KVMR\n" );drivers/gpu/drm/i915/intel_pm.c void intel_init_clock_gating(struct drm_device *dev) · line 4084
Quoted 10 lines
drivers/gpu/drm/i915/intel_pm.c +10
-0+else if (I915_READ (HSW_PWR_WELL_DEBUG ) &HSW_PWR_WELL_ENABLE ) +DRM_DEBUG_KMS ("Not disabling power well: requested by DEBUG\n" ); +else { +DRM_DEBUG_KMS ("Disabling power well\n" ); +if (wait_for ((I915_READ (HSW_PWR_WELL_DRIVER ) & +HSW_PWR_WELL_STATE ) ==0 ,20 )) +DRM_ERROR ("Timeout disabling power well\n" ); + } + } + }The documentation says not to touch the enable bits if there's a state transition already in progress. I have no idea how we're supposed to do that without all kinds of race conditions due multiple entities making requests simultaneosly. Do you know something about this that's not in the documentation?
Quoted 1 line
Apart from that the patch looks good to me.
We should be protected by dev->mode_config.mutex in all callsite (safe for maybe setup/resume, but that's just cosmetics). The dev->struct_mutex locking we already have seems to be cargo-culting afaict. Paulo, can you add a follow-up patch to unconfuse matters here a bit?
Thanks, Daniel
Quoted 58 lines
Quoted 50 lines
drivers/gpu/drm/i915/intel_pm.c +14
-15+} + +/* +* Starting with Haswell, we have a "Power Down Well" that can be turned off +* when not needed anymore. We have 4 registers that can request the power well +* to be enabled, and it will only be disabled if none of the registers is +* requesting it to be enabled. */ -void intel_init_power_wells (struct drm_device *dev) +void intel_init_power_well (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private; -unsigned long power_wells[] = { -HSW_PWR_WELL_CTL1 , -HSW_PWR_WELL_CTL2 , -HSW_PWR_WELL_CTL4 - }; -int i;if (!IS_HASWELL (dev))return ;mutex_lock (&dev->struct_mutex); -for (i =0 ; i <ARRAY_SIZE (power_wells); i++) { -int well =I915_READ (power_wells[i]); +/* For now, we need the power well to be always enabled. */ +intel_set_power_well (dev, true); -if ((well &HSW_PWR_WELL_STATE ) ==0 ) { -I915_WRITE (power_wells[i], well &HSW_PWR_WELL_ENABLE ); -if (wait_for ((I915_READ (power_wells[i]) &HSW_PWR_WELL_STATE ),20 )) -DRM_ERROR ("Error enabling power well %lx\n" , power_wells[i]); - } - } +/* We're taking over the BIOS, so clear any requests made by it since +* the driver is in charge now. */ +if (I915_READ (HSW_PWR_WELL_BIOS ) &HSW_PWR_WELL_ENABLE ) +I915_WRITE (HSW_PWR_WELL_BIOS ,0 );mutex_unlock (&dev->struct_mutex); }Signature · 6 lines
1.7.10.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfxSignature · 6 lines
Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Re: [PATCH 03/10] drm/i915: fix intel_init_power_wells
Quoted 131 lines · Daniel Vetter
Quoted 130 lines · Ville Syrjälä
Quoted 112 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
The current code was wrong in many different ways, so this is a full rewrite. We don't have "different power wells for different parts of the GPU", we have a single power well, but we have multiple registers that can be used to request enabling/disabling the power well. So let's be a good citizen and only use the register we're supposed to use, except when we're loading the driver, where we clear the request made by the BIOS.
If any of the registers is requesting the power well to be enabled, it will be enabled. If none of the registers is requesting the power well to be enabled, it will be disabled.
For now we're just forcing the power well to be enabled, but in the next commits we'll change this.
Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_reg.h | 8 ++-- drivers/gpu/drm/i915/intel_display.c | 5 +-- drivers/gpu/drm/i915/intel_drv.h | 2 +- drivers/gpu/drm/i915/intel_pm.c | 70 +++++++++++++++++++++++++--------- 4 files changed, 59 insertions(+), 26 deletions(-)drivers/gpu/drm/i915/i915_reg.h +4
-4@@ -4414,10 +4414,10 @@#define AUDIO_CP_READY_C (1<<9)/* HSW Power Wells */ -#define HSW_PWR_WELL_CTL1 0x45400/* BIOS */ -#define HSW_PWR_WELL_CTL2 0x45404/* Driver */ -#define HSW_PWR_WELL_CTL3 0x45408/* KVMR */ -#define HSW_PWR_WELL_CTL4 0x4540C/* Debug */ +#define HSW_PWR_WELL_BIOS 0x45400/* CTL1 */ +#define HSW_PWR_WELL_DRIVER 0x45404/* CTL2 */ +#define HSW_PWR_WELL_KVMR 0x45408/* CTL3 */ +#define HSW_PWR_WELL_DEBUG 0x4540C/* CTL4 */ #define HSW_PWR_WELL_ENABLE (1<<31)#define HSW_PWR_WELL_STATE (1<<30)#define HSW_PWR_WELL_CTL5 0x45410drivers/gpu/drm/i915/intel_display.c +1
-4@@ -8647,10 +8647,7 @@ static void i915_disable_vga(struct drm_device *dev)void intel_modeset_init_hw (struct drm_device *dev) { -/* We attempt to init the necessary power wells early in the initialization -* time, so the subsystems that expect power to be enabled can work. -*/ -intel_init_power_wells (dev); +intel_init_power_well (dev);intel_prepare_ddi (dev);drivers/gpu/drm/i915/intel_drv.h +1
-1@@ -666,7 +666,7 @@ extern void intel_update_fbc(struct drm_device *dev);extern void intel_gpu_ips_init (struct drm_i915_private *dev_priv);extern void intel_gpu_ips_teardown (void ); -extern void intel_init_power_wells (struct drm_device *dev); +extern void intel_init_power_well (struct drm_device *dev);extern void intel_enable_gt_powersave (struct drm_device *dev);extern void intel_disable_gt_powersave (struct drm_device *dev);extern void gen6_gt_check_fifodbg (struct drm_i915_private *dev_priv);drivers/gpu/drm/i915/intel_pm.c +29
-2@@ -4043,33 +4043,69 @@ void intel_init_clock_gating(struct drm_device *dev) dev_priv->display.init_clock_gating (dev); } -/* Starting with Haswell, we have different power wells for -* different parts of the GPU. This attempts to enable them all. +static void intel_set_power_well (struct drm_device *dev,bool enable) +{ +struct drm_i915_private *dev_priv = dev->dev_private; +bool is_enabled, enable_requested; +uint32_t tmp; + + tmp =I915_READ (HSW_PWR_WELL_DRIVER ); + is_enabled = !!(tmp &HSW_PWR_WELL_STATE ); + enable_requested = !!(tmp &HSW_PWR_WELL_ENABLE ); + +if (enable) { +if (!enable_requested) +I915_WRITE (HSW_PWR_WELL_DRIVER ,HSW_PWR_WELL_ENABLE ); + +if (!is_enabled) { +DRM_DEBUG_KMS ("Enabling power well\n" ); +if (wait_for ((I915_READ (HSW_PWR_WELL_DRIVER ) & +HSW_PWR_WELL_STATE ),20 )) +DRM_ERROR ("Timeout enabling power well\n" ); + } + }else { +if (enable_requested) +I915_WRITE (HSW_PWR_WELL_DRIVER ,0 ); + +if (is_enabled) { +if (I915_READ (HSW_PWR_WELL_BIOS ) &HSW_PWR_WELL_ENABLE ) +DRM_DEBUG_KMS ("Not disabling power well: requested by BIOS\n" ); +else if (I915_READ (HSW_PWR_WELL_KVMR ) &HSW_PWR_WELL_ENABLE ) +DRM_DEBUG_KMS ("Not disabling power well: requested by KVMR\n" );drivers/gpu/drm/i915/intel_pm.c void intel_init_clock_gating(struct drm_device *dev) · line 4084
Quoted 10 lines
drivers/gpu/drm/i915/intel_pm.c +10
-0+else if (I915_READ (HSW_PWR_WELL_DEBUG ) &HSW_PWR_WELL_ENABLE ) +DRM_DEBUG_KMS ("Not disabling power well: requested by DEBUG\n" ); +else { +DRM_DEBUG_KMS ("Disabling power well\n" ); +if (wait_for ((I915_READ (HSW_PWR_WELL_DRIVER ) & +HSW_PWR_WELL_STATE ) ==0 ,20 )) +DRM_ERROR ("Timeout disabling power well\n" ); + } + } + }The documentation says not to touch the enable bits if there's a state transition already in progress. I have no idea how we're supposed to do that without all kinds of race conditions due multiple entities making requests simultaneosly. Do you know something about this that's not in the documentation?
Quoted 6 lines
Quoted 1 line
Apart from that the patch looks good to me.
We should be protected by
dev->mode_config.mutexin all callsite (safe for maybe setup/resume, but that's just cosmetics). Thedev->struct_mutexlocking we already have seems to be cargo-culting afaict. Paulo, can you add a follow-up patch to unconfuse matters here a bit?
I'm not concerned about out driver, but mainly KVMR. It can enable/disable the power well at any time. Unless of course KVMR is never used on Linux machines, in which case my worries are pointless.
Signature · 2 lines
Ville Syrjälä Intel OTC
Re: [PATCH 03/10] drm/i915: fix intel_init_power_wells
In reply to Paulo ZanoniQuoted 110 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
The current code was wrong in many different ways, so this is a full rewrite. We don't have "different power wells for different parts of the GPU", we have a single power well, but we have multiple registers that can be used to request enabling/disabling the power well. So let's be a good citizen and only use the register we're supposed to use, except when we're loading the driver, where we clear the request made by the BIOS.
If any of the registers is requesting the power well to be enabled, it will be enabled. If none of the registers is requesting the power well to be enabled, it will be disabled.
For now we're just forcing the power well to be enabled, but in the next commits we'll change this.
Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_reg.h | 8 ++-- drivers/gpu/drm/i915/intel_display.c | 5 +-- drivers/gpu/drm/i915/intel_drv.h | 2 +- drivers/gpu/drm/i915/intel_pm.c | 70 +++++++++++++++++++++++++--------- 4 files changed, 59 insertions(+), 26 deletions(-)drivers/gpu/drm/i915/i915_reg.h +4
-4@@ -4414,10 +4414,10 @@#define AUDIO_CP_READY_C (1<<9)/* HSW Power Wells */ -#define HSW_PWR_WELL_CTL1 0x45400/* BIOS */ -#define HSW_PWR_WELL_CTL2 0x45404/* Driver */ -#define HSW_PWR_WELL_CTL3 0x45408/* KVMR */ -#define HSW_PWR_WELL_CTL4 0x4540C/* Debug */ +#define HSW_PWR_WELL_BIOS 0x45400/* CTL1 */ +#define HSW_PWR_WELL_DRIVER 0x45404/* CTL2 */ +#define HSW_PWR_WELL_KVMR 0x45408/* CTL3 */ +#define HSW_PWR_WELL_DEBUG 0x4540C/* CTL4 */ #define HSW_PWR_WELL_ENABLE (1<<31)#define HSW_PWR_WELL_STATE (1<<30)#define HSW_PWR_WELL_CTL5 0x45410drivers/gpu/drm/i915/intel_display.c +1
-4@@ -8647,10 +8647,7 @@ static void i915_disable_vga(struct drm_device *dev)void intel_modeset_init_hw (struct drm_device *dev) { -/* We attempt to init the necessary power wells early in the initialization -* time, so the subsystems that expect power to be enabled can work. -*/ -intel_init_power_wells (dev); +intel_init_power_well (dev);intel_prepare_ddi (dev);drivers/gpu/drm/i915/intel_drv.h +1
-1@@ -666,7 +666,7 @@ extern void intel_update_fbc(struct drm_device *dev);extern void intel_gpu_ips_init (struct drm_i915_private *dev_priv);extern void intel_gpu_ips_teardown (void ); -extern void intel_init_power_wells (struct drm_device *dev); +extern void intel_init_power_well (struct drm_device *dev);extern void intel_enable_gt_powersave (struct drm_device *dev);extern void intel_disable_gt_powersave (struct drm_device *dev);extern void gen6_gt_check_fifodbg (struct drm_i915_private *dev_priv);drivers/gpu/drm/i915/intel_pm.c +27
-2@@ -4043,33 +4043,69 @@ void intel_init_clock_gating(struct drm_device *dev) dev_priv->display.init_clock_gating (dev); } -/* Starting with Haswell, we have different power wells for -* different parts of the GPU. This attempts to enable them all. +static void intel_set_power_well (struct drm_device *dev,bool enable) +{ +struct drm_i915_private *dev_priv = dev->dev_private; +bool is_enabled, enable_requested; +uint32_t tmp; + + tmp =I915_READ (HSW_PWR_WELL_DRIVER ); + is_enabled = !!(tmp &HSW_PWR_WELL_STATE ); + enable_requested = !!(tmp &HSW_PWR_WELL_ENABLE ); + +if (enable) { +if (!enable_requested) +I915_WRITE (HSW_PWR_WELL_DRIVER ,HSW_PWR_WELL_ENABLE ); + +if (!is_enabled) { +DRM_DEBUG_KMS ("Enabling power well\n" ); +if (wait_for ((I915_READ (HSW_PWR_WELL_DRIVER ) & +HSW_PWR_WELL_STATE ),20 )) +DRM_ERROR ("Timeout enabling power well\n" ); + } + }else { +if (enable_requested) +I915_WRITE (HSW_PWR_WELL_DRIVER ,0 ); + +if (is_enabled) { +if (I915_READ (HSW_PWR_WELL_BIOS ) &HSW_PWR_WELL_ENABLE ) +DRM_DEBUG_KMS ("Not disabling power well: requested by BIOS\n" );
drivers/gpu/drm/i915/intel_pm.c void intel_init_clock_gating(struct drm_device *dev) · line 4082
Quoted 10 lines
drivers/gpu/drm/i915/intel_pm.c +10
-0+else if (I915_READ (HSW_PWR_WELL_KVMR ) &HSW_PWR_WELL_ENABLE ) +DRM_DEBUG_KMS ("Not disabling power well: requested by KVMR\n" ); +else if (I915_READ (HSW_PWR_WELL_DEBUG ) &HSW_PWR_WELL_ENABLE ) +DRM_DEBUG_KMS ("Not disabling power well: requested by DEBUG\n" ); +else { +DRM_DEBUG_KMS ("Disabling power well\n" ); +if (wait_for ((I915_READ (HSW_PWR_WELL_DRIVER ) & +HSW_PWR_WELL_STATE ) ==0 ,20 )) +DRM_ERROR ("Timeout disabling power well\n" ); + }
After some irc discussions Daniel came to the conclusion that the status bit is likely just a delayed version of the request bit. I tend to agree, nothing else makes much sense. So we should just always poll the status bit after changing the request bit, and completely ignore the KVMR/BIOS/DEBUG registers.
Signature · 2 lines
Ville Syrjälä Intel OTC
Re: [PATCH 03/10] drm/i915: fix intel_init_power_wells
In reply to Paulo ZanoniQuoted 10 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
The current code was wrong in many different ways, so this is a full rewrite. We don't have "different power wells for different parts of the GPU", we have a single power well, but we have multiple registers that can be used to request enabling/disabling the power well. So let's be a good citizen and only use the register we're supposed to use, except when we're loading the driver, where we clear the request made by the BIOS.
Quoted 6 lines
If any of the registers is requesting the power well to be enabled, it will be enabled. If none of the registers is requesting the power well to be enabled, it will be disabled.
For now we're just forcing the power well to be enabled, but in the next commits we'll change this.
Quoted 155 lines
Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_reg.h | 8 ++-- drivers/gpu/drm/i915/intel_display.c | 5 +-- drivers/gpu/drm/i915/intel_drv.h | 2 +- drivers/gpu/drm/i915/intel_pm.c | 70 +++++++++++++++++++++++++--------- 4 files changed, 59 insertions(+), 26 deletions(-)drivers/gpu/drm/i915/i915_reg.h +4
-4@@ -4414,10 +4414,10 @@#define AUDIO_CP_READY_C (1<<9)/* HSW Power Wells */ -#define HSW_PWR_WELL_CTL1 0x45400/* BIOS */ -#define HSW_PWR_WELL_CTL2 0x45404/* Driver */ -#define HSW_PWR_WELL_CTL3 0x45408/* KVMR */ -#define HSW_PWR_WELL_CTL4 0x4540C/* Debug */ +#define HSW_PWR_WELL_BIOS 0x45400/* CTL1 */ +#define HSW_PWR_WELL_DRIVER 0x45404/* CTL2 */ +#define HSW_PWR_WELL_KVMR 0x45408/* CTL3 */ +#define HSW_PWR_WELL_DEBUG 0x4540C/* CTL4 */ #define HSW_PWR_WELL_ENABLE (1<<31)#define HSW_PWR_WELL_STATE (1<<30)#define HSW_PWR_WELL_CTL5 0x45410drivers/gpu/drm/i915/intel_display.c +1
-4@@ -8647,10 +8647,7 @@ static void i915_disable_vga(struct drm_device *dev)void intel_modeset_init_hw (struct drm_device *dev) { -/* We attempt to init the necessary power wells early in the initialization -* time, so the subsystems that expect power to be enabled can work. -*/ -intel_init_power_wells (dev); +intel_init_power_well (dev);intel_prepare_ddi (dev);drivers/gpu/drm/i915/intel_drv.h +1
-1@@ -666,7 +666,7 @@ extern void intel_update_fbc(struct drm_device *dev);extern void intel_gpu_ips_init (struct drm_i915_private *dev_priv);extern void intel_gpu_ips_teardown (void ); -extern void intel_init_power_wells (struct drm_device *dev); +extern void intel_init_power_well (struct drm_device *dev);extern void intel_enable_gt_powersave (struct drm_device *dev);extern void intel_disable_gt_powersave (struct drm_device *dev);extern void gen6_gt_check_fifodbg (struct drm_i915_private *dev_priv);drivers/gpu/drm/i915/intel_pm.c +53
-17@@ -4043,33 +4043,69 @@ void intel_init_clock_gating(struct drm_device *dev) dev_priv->display.init_clock_gating (dev); } -/* Starting with Haswell, we have different power wells for -* different parts of the GPU. This attempts to enable them all. +static void intel_set_power_well (struct drm_device *dev,bool enable) +{ +struct drm_i915_private *dev_priv = dev->dev_private; +bool is_enabled, enable_requested; +uint32_t tmp; + + tmp =I915_READ (HSW_PWR_WELL_DRIVER ); + is_enabled = !!(tmp &HSW_PWR_WELL_STATE ); + enable_requested = !!(tmp &HSW_PWR_WELL_ENABLE ); + +if (enable) { +if (!enable_requested) +I915_WRITE (HSW_PWR_WELL_DRIVER ,HSW_PWR_WELL_ENABLE ); + +if (!is_enabled) { +DRM_DEBUG_KMS ("Enabling power well\n" ); +if (wait_for ((I915_READ (HSW_PWR_WELL_DRIVER ) & +HSW_PWR_WELL_STATE ),20 )) +DRM_ERROR ("Timeout enabling power well\n" ); + } + }else { +if (enable_requested) +I915_WRITE (HSW_PWR_WELL_DRIVER ,0 ); + +if (is_enabled) { +if (I915_READ (HSW_PWR_WELL_BIOS ) &HSW_PWR_WELL_ENABLE ) +DRM_DEBUG_KMS ("Not disabling power well: requested by BIOS\n" ); +else if (I915_READ (HSW_PWR_WELL_KVMR ) &HSW_PWR_WELL_ENABLE ) +DRM_DEBUG_KMS ("Not disabling power well: requested by KVMR\n" ); +else if (I915_READ (HSW_PWR_WELL_DEBUG ) &HSW_PWR_WELL_ENABLE ) +DRM_DEBUG_KMS ("Not disabling power well: requested by DEBUG\n" ); +else { +DRM_DEBUG_KMS ("Disabling power well\n" ); +if (wait_for ((I915_READ (HSW_PWR_WELL_DRIVER ) & +HSW_PWR_WELL_STATE ) ==0 ,20 )) +DRM_ERROR ("Timeout disabling power well\n" ); + } + } + } +} + +/* +* Starting with Haswell, we have a "Power Down Well" that can be turned off +* when not needed anymore. We have 4 registers that can request the power well +* to be enabled, and it will only be disabled if none of the registers is +* requesting it to be enabled. */ -void intel_init_power_wells (struct drm_device *dev) +void intel_init_power_well (struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private; -unsigned long power_wells[] = { -HSW_PWR_WELL_CTL1 , -HSW_PWR_WELL_CTL2 , -HSW_PWR_WELL_CTL4 - }; -int i;if (!IS_HASWELL (dev))return ;mutex_lock (&dev->struct_mutex); -for (i =0 ; i <ARRAY_SIZE (power_wells); i++) { -int well =I915_READ (power_wells[i]); +/* For now, we need the power well to be always enabled. */ +intel_set_power_well (dev, true); -if ((well &HSW_PWR_WELL_STATE ) ==0 ) { -I915_WRITE (power_wells[i], well &HSW_PWR_WELL_ENABLE ); -if (wait_for ((I915_READ (power_wells[i]) &HSW_PWR_WELL_STATE ),20 )) -DRM_ERROR ("Error enabling power well %lx\n" , power_wells[i]); - } - } +/* We're taking over the BIOS, so clear any requests made by it since +* the driver is in charge now. */ +if (I915_READ (HSW_PWR_WELL_BIOS ) &HSW_PWR_WELL_ENABLE ) +I915_WRITE (HSW_PWR_WELL_BIOS ,0 );mutex_unlock (&dev->struct_mutex); }Signature · 6 lines
1.7.10.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 4/10 drm/i915: dynamic Haswell display power well support
In reply to Paulo ZanoniFrom: Daniel Vetter <[email protected]>
We can disable (almost) all the display hw if we only use pipe A, with the integrated edp transcoder on port A. Because we don't set the cpu transcoder that early (yet), we need to help us with a trick to simply check for any edp encoders.
And wrt the old code: Can anyone explain what that struct mutex grabbing was supposed to protect?
v2: Paulo Zanoni pointed out that we also need to configure the eDP cpu transcoder correctly.
v3: Made by Paulo Zanoni
- Rebase patch to be on top of "fix intel_init_power_wells" patch
- Fix typos
- Fix a small bug by adding a "connectors_active" check
- Restore the initial code that unconditionally enables the power well when taking over from the BIOS
--- drivers/gpu/drm/i915/intel_ddi.c | 8 +++++++- drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_pm.c | 2 +- 4 files changed, 40 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/intel_ddi.c +7 -1
@@ -987,7 +987,13 @@ void intel_ddi_enable_pipe_func(struct drm_crtc *crtc)if (cpu_transcoder ==TRANSCODER_EDP ) {switch (pipe) {case PIPE_A : - temp |=TRANS_DDI_EDP_INPUT_A_ONOFF ; +/* Can only use the always-on power well for eDP when +* not using the panel fitter, and when not using motion +* blur mitigation (which we don't support). */ +if (dev_priv->pch_pf_size) + temp |=TRANS_DDI_EDP_INPUT_A_ONOFF ; +else + temp |=TRANS_DDI_EDP_INPUT_A_ON ;break ;case PIPE_B : temp |=TRANS_DDI_EDP_INPUT_B_ONOFF ;
drivers/gpu/drm/i915/intel_display.c +31 -0
@@ -5595,6 +5595,35 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,return fdi_config_ok ? ret : -EINVAL ; } +static void haswell_modeset_global_resources (struct drm_device *dev) +{ +struct drm_i915_private *dev_priv = dev->dev_private; +bool enable = false; +struct intel_crtc *crtc; +struct intel_encoder *encoder; + +list_for_each_entry (crtc, &dev->mode_config.crtc_list, base.head) { +if (crtc->pipe !=PIPE_A && crtc->base.enabled) + enable = true; +/* XXX: Should check for edp transcoder here, but thanks to init +* sequence that's not yet availble. Just in case desktop eDP on +* PORT D is possible on haswell, too. */ + } + +list_for_each_entry (encoder, &dev->mode_config.encoder_list, + base.head) { +if (encoder->type !=INTEL_OUTPUT_EDP && + encoder->connectors_active) + enable = true; + } + +/* Even the eDP panel fitter is outside the always-on well. */ +if (dev_priv->pch_pf_size) + enable = true; + +intel_set_power_well (dev, enable); +} + static int haswell_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode,
@@ -8477,6 +8506,8 @@ static void intel_init_display(struct drm_device *dev) }else if (IS_HASWELL (dev)) { dev_priv->display.fdi_link_train = hsw_fdi_link_train; dev_priv->display.write_eld = haswell_write_eld; + dev_priv->display.modeset_global_resources = + haswell_modeset_global_resources; } }else if (IS_G4X (dev)) { dev_priv->display.write_eld = g4x_write_eld;
drivers/gpu/drm/i915/intel_drv.h +1 -0
@@ -667,6 +667,7 @@ extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);extern void intel_gpu_ips_teardown (void );extern void intel_init_power_well (struct drm_device *dev); +extern void intel_set_power_well (struct drm_device *dev,bool enable);extern void intel_enable_gt_powersave (struct drm_device *dev);extern void intel_disable_gt_powersave (struct drm_device *dev);extern void gen6_gt_check_fifodbg (struct drm_i915_private *dev_priv);
drivers/gpu/drm/i915/intel_pm.c +1 -1
@@ -4043,7 +4043,7 @@ void intel_init_clock_gating(struct drm_device *dev) dev_priv->display.init_clock_gating (dev); } -static void intel_set_power_well (struct drm_device *dev,bool enable) +void intel_set_power_well (struct drm_device *dev,bool enable) {struct drm_i915_private *dev_priv = dev->dev_private;bool is_enabled, enable_requested;
Signature · 1 line
1.7.10.4
Re: [PATCH 04/10] drm/i915: dynamic Haswell display power well support
Quoted 59 lines · Paulo Zanoni
From: Daniel Vetter <[email protected]>
We can disable (almost) all the display hw if we only use pipe A, with the integrated edp transcoder on port A. Because we don't set the cpu transcoder that early (yet), we need to help us with a trick to simply check for any edp encoders.
And wrt the old code: Can anyone explain what that struct mutex grabbing was supposed to protect?
v2: Paulo Zanoni pointed out that we also need to configure the eDP cpu transcoder correctly.
v3: Made by Paulo Zanoni
- Rebase patch to be on top of "fixintel_init_power_wells" patch
- Fix typos
- Fix a small bug by adding a "connectors_active" check
- Restore the initial code that unconditionally enables the power well when taking over from the BIOSSigned-off-by Daniel Vetter <[email protected]>Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/intel_ddi.c | 8 +++++++- drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_pm.c | 2 +- 4 files changed, 40 insertions(+), 2 deletions(-)drivers/gpu/drm/i915/intel_ddi.c +7
-1@@ -987,7 +987,13 @@ void intel_ddi_enable_pipe_func(struct drm_crtc *crtc)if (cpu_transcoder ==TRANSCODER_EDP ) {switch (pipe) {case PIPE_A : - temp |=TRANS_DDI_EDP_INPUT_A_ONOFF ; +/* Can only use the always-on power well for eDP when +* not using the panel fitter, and when not using motion +* blur mitigation (which we don't support). */ +if (dev_priv->pch_pf_size) + temp |=TRANS_DDI_EDP_INPUT_A_ONOFF ; +else + temp |=TRANS_DDI_EDP_INPUT_A_ON ;break ;case PIPE_B : temp |=TRANS_DDI_EDP_INPUT_B_ONOFF ;drivers/gpu/drm/i915/intel_display.c +3
-0@@ -5595,6 +5595,35 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,return fdi_config_ok ? ret : -EINVAL ; } +static void haswell_modeset_global_resources (struct drm_device *dev) +{ +struct drm_i915_private *dev_priv = dev->dev_private;
drivers/gpu/drm/i915/intel_display.c static int ironlake_crtc_mode_set(struct drm_crtc *crtc, · line 5610
Quoted 10 lines
drivers/gpu/drm/i915/intel_display.c +10
-0+bool enable = false; +struct intel_crtc *crtc; +struct intel_encoder *encoder; + +list_for_each_entry (crtc, &dev->mode_config.crtc_list, base.head) { +if (crtc->pipe !=PIPE_A && crtc->base.enabled) + enable = true; +/* XXX: Should check for edp transcoder here, but thanks to init +* sequence that's not yet availble. Just in case desktop eDP on +* PORT D is possible on haswell, too. */
Obligatory nitpick on something: s/availble/available/
Quoted 60 lines
drivers/gpu/drm/i915/intel_display.c +18
-0+ } + +list_for_each_entry (encoder, &dev->mode_config.encoder_list, + base.head) { +if (encoder->type !=INTEL_OUTPUT_EDP && + encoder->connectors_active) + enable = true; + } + +/* Even the eDP panel fitter is outside the always-on well. */ +if (dev_priv->pch_pf_size) + enable = true; + +intel_set_power_well (dev, enable); +} + static int haswell_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode,@@ -8477,6 +8506,8 @@ static void intel_init_display(struct drm_device *dev) }else if (IS_HASWELL (dev)) { dev_priv->display.fdi_link_train = hsw_fdi_link_train; dev_priv->display.write_eld = haswell_write_eld; + dev_priv->display.modeset_global_resources = + haswell_modeset_global_resources; } }else if (IS_G4X (dev)) { dev_priv->display.write_eld = g4x_write_eld;drivers/gpu/drm/i915/intel_drv.h +1
-0@@ -667,6 +667,7 @@ extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);extern void intel_gpu_ips_teardown (void );extern void intel_init_power_well (struct drm_device *dev); +extern void intel_set_power_well (struct drm_device *dev,bool enable);extern void intel_enable_gt_powersave (struct drm_device *dev);extern void intel_disable_gt_powersave (struct drm_device *dev);extern void gen6_gt_check_fifodbg (struct drm_i915_private *dev_priv);drivers/gpu/drm/i915/intel_pm.c +1
-1@@ -4043,7 +4043,7 @@ void intel_init_clock_gating(struct drm_device *dev) dev_priv->display.init_clock_gating (dev); } -static void intel_set_power_well (struct drm_device *dev,bool enable) +void intel_set_power_well (struct drm_device *dev,bool enable) {struct drm_i915_private *dev_priv = dev->dev_private;bool is_enabled, enable_requested;Signature · 6 lines
1.7.10.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 5/10 drm/i915: only disable enabled planes on intel_fb_restore_mode
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
We should avoid touching registers that are on the power down well when we don't need to, because if we touch these registers when the power well is disabled we'll get tons of "unclaimed register" messages. This commit fixes some of these messages.
--- drivers/gpu/drm/i915/intel_fb.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/gpu/drm/i915/intel_fb.c +2 -1
@@ -304,7 +304,8 @@ void intel_fb_restore_mode(struct drm_device *dev)/* Be sure to shut off any planes that may be active */ list_for_each_entry (plane, &config->plane_list, head) - plane->funcs->disable_plane (plane); +if (plane->enabled) + plane->funcs->disable_plane (plane);mutex_unlock (&dev->mode_config.mutex); }
Signature · 1 line
1.7.10.4
PATCH 6/10 drm/i915: check the power down well on assert_pipe()
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
If the power well is disabled, we should not try to read its registers, otherwise we'll get "unclaimed register" messages.
--- drivers/gpu/drm/i915/intel_display.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
drivers/gpu/drm/i915/intel_display.c +9 -3
@@ -1214,9 +1214,15 @@ void assert_pipe(struct drm_i915_private *dev_priv,if (pipe ==PIPE_A && dev_priv->quirks &QUIRK_PIPEA_FORCE ) state = true; - reg =PIPECONF (cpu_transcoder); - val =I915_READ (reg); - cur_state = !!(val &PIPECONF_ENABLE ); +if (cpu_transcoder ==TRANSCODER_EDP || + (I915_READ (HSW_PWR_WELL_DRIVER ) &HSW_PWR_WELL_STATE )) { + reg =PIPECONF (cpu_transcoder); + val =I915_READ (reg); + cur_state = !!(val &PIPECONF_ENABLE ); + }else { + cur_state = false; + } +WARN (cur_state != state,"pipe %c assertion failure (expected %s, current %s)\n" ,pipe_name (pipe),state_string (state),state_string (cur_state));
Signature · 1 line
1.7.10.4
Re: [PATCH 06/10] drm/i915: check the power down well on assert_pipe()
Quoted 15 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
If the power well is disabled, we should not try to read its registers, otherwise we'll get "unclaimed register" messages.
Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/intel_display.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)drivers/gpu/drm/i915/intel_display.c +0
-0
drivers/gpu/drm/i915/intel_display.c void assert_pipe(struct drm_i915_private *dev_priv, · line 1218
Quoted 8 lines
drivers/gpu/drm/i915/intel_display.c +2
-3@@ -1214,9 +1214,15 @@ void assert_pipe(struct drm_i915_private *dev_priv,if (pipe ==PIPE_A && dev_priv->quirks &QUIRK_PIPEA_FORCE ) state = true; - reg =PIPECONF (cpu_transcoder); - val =I915_READ (reg); - cur_state = !!(val &PIPECONF_ENABLE ); +if (cpu_transcoder ==TRANSCODER_EDP || + (I915_READ (HSW_PWR_WELL_DRIVER ) &HSW_PWR_WELL_STATE )) {
Should that also check HSW_PWR_WELL_ENABLE? KVMR might have the well enabled, while the driver has it disabled. But KVMR might have already disabled the well, and it might get disabled just after this check, and then you would hit the unclaimed register issue again.
Quoted 17 lines
drivers/gpu/drm/i915/intel_display.c +7
-0+ reg =PIPECONF (cpu_transcoder); + val =I915_READ (reg); + cur_state = !!(val &PIPECONF_ENABLE ); + }else { + cur_state = false; + } +WARN (cur_state != state,"pipe %c assertion failure (expected %s, current %s)\n" ,pipe_name (pipe),state_string (state),state_string (cur_state));Signature · 6 lines
1.7.10.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 2 lines
Ville Syrjälä Intel OTC
Re: [PATCH 06/10] drm/i915: check the power down well on assert_pipe()
Quoted 19 lines · Ville Syrjälä
Quoted 18 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
If the power well is disabled, we should not try to read its registers, otherwise we'll get "unclaimed register" messages.
Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/intel_display.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)drivers/gpu/drm/i915/intel_display.c +0
-0@@ -1214,9 +1214,15 @@ void assert_pipe(struct drm_i915_private *dev_priv,if (pipe ==PIPE_A && dev_priv->quirks &QUIRK_PIPEA_FORCE ) state = true;
Quoted 10 lines
drivers/gpu/drm/i915/intel_display.c void assert_pipe(struct drm_i915_private *dev_priv, · line 1218
Quoted 5 lines
drivers/gpu/drm/i915/intel_display.c +2
-3- reg =PIPECONF (cpu_transcoder); - val =I915_READ (reg); - cur_state = !!(val &PIPECONF_ENABLE ); +if (cpu_transcoder ==TRANSCODER_EDP || + (I915_READ (HSW_PWR_WELL_DRIVER ) &HSW_PWR_WELL_STATE )) {Should that also check
HSW_PWR_WELL_ENABLE? KVMR might have the well enabled, while the driver has it disabled. But KVMR might have already disabled the well, and it might get disabled just after this check, and then you would hit the unclaimed register issue again.
The important matter is to not read registers in the power well if it's off, for which checking just one of the three bits is enough. If the kvm keeps the power well on, we just avoid checking the pipe state if we don't need the power well, but otherwise no side effect. Otoh just one set bit makes sure that the power well is on and we can read the regs).
So looks good to me.
-Daniel
Quoted 26 lines
Quoted 18 lines
drivers/gpu/drm/i915/intel_display.c +7
-0+ reg =PIPECONF (cpu_transcoder); + val =I915_READ (reg); + cur_state = !!(val &PIPECONF_ENABLE ); + }else { + cur_state = false; + } +WARN (cur_state != state,"pipe %c assertion failure (expected %s, current %s)\n" ,pipe_name (pipe),state_string (state),state_string (cur_state));Signature · 6 lines
1.7.10.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfxSignature · 6 lines
Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Re: [PATCH 06/10] drm/i915: check the power down well on assert_pipe()
Quoted 31 lines · Daniel Vetter
Quoted 19 lines · Ville Syrjälä
Quoted 18 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
If the power well is disabled, we should not try to read its registers, otherwise we'll get "unclaimed register" messages.
Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/intel_display.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)drivers/gpu/drm/i915/intel_display.c +0
-0@@ -1214,9 +1214,15 @@ void assert_pipe(struct drm_i915_private *dev_priv,if (pipe ==PIPE_A && dev_priv->quirks &QUIRK_PIPEA_FORCE ) state = true;Quoted 10 lines
drivers/gpu/drm/i915/intel_display.c void assert_pipe(struct drm_i915_private *dev_priv, · line 1218
Quoted 5 lines
drivers/gpu/drm/i915/intel_display.c +2
-3- reg =PIPECONF (cpu_transcoder); - val =I915_READ (reg); - cur_state = !!(val &PIPECONF_ENABLE ); +if (cpu_transcoder ==TRANSCODER_EDP || + (I915_READ (HSW_PWR_WELL_DRIVER ) &HSW_PWR_WELL_STATE )) {Should that also check
HSW_PWR_WELL_ENABLE? KVMR might have the well enabled, while the driver has it disabled. But KVMR might have already disabled the well, and it might get disabled just after this check, and then you would hit the unclaimed register issue again.
Quoted 5 lines
The important matter is to not read registers in the power well if it's off, for which checking just one of the three bits is enough. If the kvm keeps the power well on, we just avoid checking the pipe state if we don't need the power well, but otherwise no side effect. Otoh just one set bit makes sure that the power well is on and we can read the regs).
The power well may be on when you're reading the status bit, but assuming it was KVMR who caused the power well to be powered on, we can't be sure the power well will remain powered long enough to read the registers.
Signature · 2 lines
Ville Syrjälä Intel OTC
Re: [PATCH 06/10] drm/i915: check the power down well on assert_pipe()
In reply to Ville SyrjäläHi
2013/1/21 Ville Syrjälä <[email protected]>:
Quoted 19 lines · Paulo Zanoni
Quoted 18 lines
From: Paulo Zanoni <[email protected]>
If the power well is disabled, we should not try to read its registers, otherwise we'll get "unclaimed register" messages.
Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/intel_display.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)drivers/gpu/drm/i915/intel_display.c +0
-0@@ -1214,9 +1214,15 @@ void assert_pipe(struct drm_i915_private *dev_priv,if (pipe ==PIPE_A && dev_priv->quirks &QUIRK_PIPEA_FORCE ) state = true;
Quoted 10 lines
drivers/gpu/drm/i915/intel_display.c void assert_pipe(struct drm_i915_private *dev_priv, · line 1218
Quoted 5 lines
drivers/gpu/drm/i915/intel_display.c +2
-3- reg =PIPECONF (cpu_transcoder); - val =I915_READ (reg); - cur_state = !!(val &PIPECONF_ENABLE ); +if (cpu_transcoder ==TRANSCODER_EDP || + (I915_READ (HSW_PWR_WELL_DRIVER ) &HSW_PWR_WELL_STATE )) {Should that also check
HSW_PWR_WELL_ENABLE? KVMR might have the well enabled, while the driver has it disabled. But KVMR might have already disabled the well, and it might get disabled just after this check, and then you would hit the unclaimed register issue again.
By reading your text it seems you think that if bit 31 is 0 then bit 30 will necessarily be 0. This is not true. If any entity (including KVMr) needs the power well enabled (and is requesting so), then bit 30 of HSW_PWR_WELL_DRIVER will be 1, no matter the status of bit 31. Another thing to mention is that no entity can force the power well to be "off", it can only force to be "on".
Still, your text does contain an example of a problem with the current code. Let me explicitly write it:
- Our driver doesn't need it to be enabled, so bit 31 of HSW_PWR_WELL_DRIVER is 0
- KVMr needs it enabled, so HSW_PWR_WELL_DRIVER bit 30 is 1
- We do the I915_READ above
- After the read, KVMr says it doesn't need the power well to be enabled anymore, so it gets disabled because no one else is requesting it
- Then we touch an unclaimed register.
I think the only policy to avoid these race conditions should be "if I don't need the power well to be enabled, then I won't touch registers that require it to be enabled, no matter what the real state of the power well is".This should prevent any kinds of problems, because if I need the power well to be enabled, then I requested it to be enabled and it is enabled because I asked so. I'll send a second version of the patch with this.
Quoted 22 lines
Quoted 14 lines
drivers/gpu/drm/i915/intel_display.c +7
-1+ reg =PIPECONF (cpu_transcoder); + val =I915_READ (reg); + cur_state = !!(val &PIPECONF_ENABLE ); + }else { + cur_state = false; + } +WARN (cur_state != state,"pipe %c assertion failure (expected %s, current %s)\n" ,pipe_name (pipe),state_string (state),state_string (cur_state)); --1.7.10.4
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
Re: [PATCH 06/10] drm/i915: check the power down well on assert_pipe()
Quoted 33 lines · Paulo Zanoni
Hi
2013/1/21 Ville Syrjälä <[email protected]>:
Quoted 19 lines · Paulo Zanoni
Quoted 18 lines
From: Paulo Zanoni <[email protected]>
If the power well is disabled, we should not try to read its registers, otherwise we'll get "unclaimed register" messages.
Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/intel_display.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)drivers/gpu/drm/i915/intel_display.c +0
-0@@ -1214,9 +1214,15 @@ void assert_pipe(struct drm_i915_private *dev_priv,if (pipe ==PIPE_A && dev_priv->quirks &QUIRK_PIPEA_FORCE ) state = true;Quoted 10 lines
drivers/gpu/drm/i915/intel_display.c void assert_pipe(struct drm_i915_private *dev_priv, · line 1218
Quoted 5 lines
drivers/gpu/drm/i915/intel_display.c +2
-3- reg =PIPECONF (cpu_transcoder); - val =I915_READ (reg); - cur_state = !!(val &PIPECONF_ENABLE ); +if (cpu_transcoder ==TRANSCODER_EDP || + (I915_READ (HSW_PWR_WELL_DRIVER ) &HSW_PWR_WELL_STATE )) {Should that also check
HSW_PWR_WELL_ENABLE? KVMR might have the well enabled, while the driver has it disabled. But KVMR might have already disabled the well, and it might get disabled just after this check, and then you would hit the unclaimed register issue again.
Quoted 4 lines
By reading your text it seems you think that if bit 31 is 0 then bit 30 will necessarily be 0. This is not true. If any entity (including KVMr) needs the power well enabled (and is requesting so), then bit 30 of
HSW_PWR_WELL_DRIVERwill be 1, no matter the status of bit 31.
Did you verify that on real HW?
It makes little sense when you consider that the spec says not to toggle the request bit when there's a state transition currently happening. If all the status bits read as the same thing, then there's no way to know when a transition is actually happening, and hence the text in the spec is complete nonsense.
The only sensible interpretation of that text I could think of is that you're not supposed to abort a state transition you yourself caused. That would be achieved by polling the status bit after toggling the request bit, assuming that the status bit is just a delayed version of request bit.
Quoted 49 lines
Another thing to mention is that no entity can force the power well to be "off", it can only force to be "on".
Still, your text does contain an example of a problem with the current code. Let me explicitly write it:
- Our driver doesn't need it to be enabled, so bit 31 ofHSW_PWR_WELL_DRIVERis 0
- KVMr needs it enabled, soHSW_PWR_WELL_DRIVERbit 30 is 1
- We do theI915_READabove
- After the read, KVMr says it doesn't need the power well to be enabled anymore, so it gets disabled because no one else is requesting it
- Then we touch an unclaimed register.I think the only policy to avoid these race conditions should be "if I don't need the power well to be enabled, then I won't touch registers that require it to be enabled, no matter what the real state of the power well is".This should prevent any kinds of problems, because if I need the power well to be enabled, then I requested it to be enabled and it is enabled because I asked so. I'll send a second version of the patch with this.
Quoted 22 lines
Quoted 14 lines
drivers/gpu/drm/i915/intel_display.c +7
-1+ reg =PIPECONF (cpu_transcoder); + val =I915_READ (reg); + cur_state = !!(val &PIPECONF_ENABLE ); + }else { + cur_state = false; + } +WARN (cur_state != state,"pipe %c assertion failure (expected %s, current %s)\n" ,pipe_name (pipe),state_string (state),state_string (cur_state)); --1.7.10.4
Quoted 4 lines
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx--
Ville Syrjälä
Intel OTCSignature · 1 line
Paulo Zanoni
Signature · 2 lines
Ville Syrjälä Intel OTC
Re: [PATCH 06/10] drm/i915: check the power down well on assert_pipe()
Hi
2013/1/25 Ville Syrjälä <[email protected]>:
Quoted 34 lines · Paulo Zanoni
Quoted 33 lines
Hi
2013/1/21 Ville Syrjälä <[email protected]>:
Quoted 29 lines · Paulo Zanoni
Quoted 15 lines
From: Paulo Zanoni <[email protected]>
If the power well is disabled, we should not try to read its registers, otherwise we'll get "unclaimed register" messages.
Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/intel_display.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)drivers/gpu/drm/i915/intel_display.c +0
-0drivers/gpu/drm/i915/intel_display.c void assert_pipe(struct drm_i915_private *dev_priv, · line 1218
Quoted 8 lines
drivers/gpu/drm/i915/intel_display.c +2
-3@@ -1214,9 +1214,15 @@ void assert_pipe(struct drm_i915_private *dev_priv,if (pipe ==PIPE_A && dev_priv->quirks &QUIRK_PIPEA_FORCE ) state = true; - reg =PIPECONF (cpu_transcoder); - val =I915_READ (reg); - cur_state = !!(val &PIPECONF_ENABLE ); +if (cpu_transcoder ==TRANSCODER_EDP || + (I915_READ (HSW_PWR_WELL_DRIVER ) &HSW_PWR_WELL_STATE )) {Should that also check
HSW_PWR_WELL_ENABLE? KVMR might have the well enabled, while the driver has it disabled. But KVMR might have already disabled the well, and it might get disabled just after this check, and then you would hit the unclaimed register issue again.
Quoted 6 lines
Quoted 4 lines
By reading your text it seems you think that if bit 31 is 0 then bit 30 will necessarily be 0. This is not true. If any entity (including KVMr) needs the power well enabled (and is requesting so), then bit 30 of
HSW_PWR_WELL_DRIVERwill be 1, no matter the status of bit 31.Did you verify that on real HW?
Yes.
Quoted 7 lines
It makes little sense when you consider that the spec says not to toggle the request bit when there's a state transition currently happening. If all the status bits read as the same thing, then there's no way to know when a transition is actually happening, and hence the text in the spec is complete nonsense.
I agree... I'm investigating this problem. That's why there is still no new version of patch 3/10 here :)
Quoted 5 lines
The only sensible interpretation of that text I could think of is that you're not supposed to abort a state transition you yourself caused. That would be achieved by polling the status bit after toggling the request bit, assuming that the status bit is just a delayed version of request bit.
If we're enabling then it's fine: we requested it to enable, then at some point it will be enabled, then we can start using the registers. We can always avoid touching the registers when the well is still being enabled. The only situation where the "restriction" doesn't make sense is on the "disable" case.
OTOH, if we requested it to be disabled, the fact that we can't predict whether it's really going to be disabled or not shouldn't really matter that much since we're not supposed to touch the "possibly disabled" registers, and if anybody needs to touch those registers they will have to request the well to be enabled first... This still doesn't explain the "Restriction", but maybe that restriction is just for the "enable" case.
Quoted 54 lines
Quoted 42 lines
Another thing to mention is that no entity can force the power well to be "off", it can only force to be "on".
Still, your text does contain an example of a problem with the current code. Let me explicitly write it:
- Our driver doesn't need it to be enabled, so bit 31 ofHSW_PWR_WELL_DRIVERis 0
- KVMr needs it enabled, soHSW_PWR_WELL_DRIVERbit 30 is 1
- We do theI915_READabove
- After the read, KVMr says it doesn't need the power well to be enabled anymore, so it gets disabled because no one else is requesting it
- Then we touch an unclaimed register.I think the only policy to avoid these race conditions should be "if I don't need the power well to be enabled, then I won't touch registers that require it to be enabled, no matter what the real state of the power well is".This should prevent any kinds of problems, because if I need the power well to be enabled, then I requested it to be enabled and it is enabled because I asked so. I'll send a second version of the patch with this.
Quoted 19 lines
Quoted 18 lines
drivers/gpu/drm/i915/intel_display.c +7
-1+ reg =PIPECONF (cpu_transcoder); + val =I915_READ (reg); + cur_state = !!(val &PIPECONF_ENABLE ); + }else { + cur_state = false; + } +WARN (cur_state != state,"pipe %c assertion failure (expected %s, current %s)\n" ,pipe_name (pipe),state_string (state),state_string (cur_state)); --1.7.10.4
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfxQuoted 8 lines
Quoted 3 lines
--
Ville Syrjälä
Intel OTC--
Paulo Zanoni--
Ville Syrjälä
Intel OTC
Signature · 1 line
Paulo Zanoni
Re: [PATCH 06/10] drm/i915: check the power down well on assert_pipe()
Quoted 63 lines · Paulo Zanoni
Hi
2013/1/25 Ville Syrjälä <[email protected]>:
Quoted 34 lines · Paulo Zanoni
Quoted 33 lines
Hi
2013/1/21 Ville Syrjälä <[email protected]>:
Quoted 29 lines · Paulo Zanoni
Quoted 15 lines
From: Paulo Zanoni <[email protected]>
If the power well is disabled, we should not try to read its registers, otherwise we'll get "unclaimed register" messages.
Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/intel_display.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)drivers/gpu/drm/i915/intel_display.c +0
-0drivers/gpu/drm/i915/intel_display.c void assert_pipe(struct drm_i915_private *dev_priv, · line 1218
Quoted 8 lines
drivers/gpu/drm/i915/intel_display.c +2
-3@@ -1214,9 +1214,15 @@ void assert_pipe(struct drm_i915_private *dev_priv,if (pipe ==PIPE_A && dev_priv->quirks &QUIRK_PIPEA_FORCE ) state = true; - reg =PIPECONF (cpu_transcoder); - val =I915_READ (reg); - cur_state = !!(val &PIPECONF_ENABLE ); +if (cpu_transcoder ==TRANSCODER_EDP || + (I915_READ (HSW_PWR_WELL_DRIVER ) &HSW_PWR_WELL_STATE )) {Should that also check
HSW_PWR_WELL_ENABLE? KVMR might have the well enabled, while the driver has it disabled. But KVMR might have already disabled the well, and it might get disabled just after this check, and then you would hit the unclaimed register issue again.Quoted 6 lines
Quoted 4 lines
By reading your text it seems you think that if bit 31 is 0 then bit 30 will necessarily be 0. This is not true. If any entity (including KVMr) needs the power well enabled (and is requesting so), then bit 30 of
HSW_PWR_WELL_DRIVERwill be 1, no matter the status of bit 31.Did you verify that on real HW?
Yes.
Quoted 7 lines
It makes little sense when you consider that the spec says not to toggle the request bit when there's a state transition currently happening. If all the status bits read as the same thing, then there's no way to know when a transition is actually happening, and hence the text in the spec is complete nonsense.
I agree... I'm investigating this problem. That's why there is still no new version of patch 3/10 here :)
Quoted 5 lines
The only sensible interpretation of that text I could think of is that you're not supposed to abort a state transition you yourself caused. That would be achieved by polling the status bit after toggling the request bit, assuming that the status bit is just a delayed version of request bit.
Quoted 4 lines
If we're enabling then it's fine: we requested it to enable, then at some point it will be enabled, then we can start using the registers. We can always avoid touching the registers when the well is still being enabled.
I think you can only assume this if the status bit goes to 0 immediately when the well starts powering down. And it won't go back to 1 until the well is fully powered again. Well, I guess that's the only design that would make sense anyway. But even then you can't tell the fully powered down case apart from the currently going down case.
Quoted 70 lines
The only situation where the "restriction" doesn't make sense is on the "disable" case.
OTOH, if we requested it to be disabled, the fact that we can't predict whether it's really going to be disabled or not shouldn't really matter that much since we're not supposed to touch the "possibly disabled" registers, and if anybody needs to touch those registers they will have to request the well to be enabled first... This still doesn't explain the "Restriction", but maybe that restriction is just for the "enable" case.
Quoted 54 lines
Quoted 42 lines
Another thing to mention is that no entity can force the power well to be "off", it can only force to be "on".
Still, your text does contain an example of a problem with the current code. Let me explicitly write it:
- Our driver doesn't need it to be enabled, so bit 31 ofHSW_PWR_WELL_DRIVERis 0
- KVMr needs it enabled, soHSW_PWR_WELL_DRIVERbit 30 is 1
- We do theI915_READabove
- After the read, KVMr says it doesn't need the power well to be enabled anymore, so it gets disabled because no one else is requesting it
- Then we touch an unclaimed register.I think the only policy to avoid these race conditions should be "if I don't need the power well to be enabled, then I won't touch registers that require it to be enabled, no matter what the real state of the power well is".This should prevent any kinds of problems, because if I need the power well to be enabled, then I requested it to be enabled and it is enabled because I asked so. I'll send a second version of the patch with this.
Quoted 19 lines
Quoted 18 lines
drivers/gpu/drm/i915/intel_display.c +7
-1+ reg =PIPECONF (cpu_transcoder); + val =I915_READ (reg); + cur_state = !!(val &PIPECONF_ENABLE ); + }else { + cur_state = false; + } +WARN (cur_state != state,"pipe %c assertion failure (expected %s, current %s)\n" ,pipe_name (pipe),state_string (state),state_string (cur_state)); --1.7.10.4
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfxQuoted 8 lines
Quoted 3 lines
--
Ville Syrjälä
Intel OTC--
Paulo Zanoni--
Ville Syrjälä
Intel OTCSignature · 1 line
Paulo Zanoni
Signature · 2 lines
Ville Syrjälä Intel OTC
PATCH 7/10 drm/i915: turn on the power well before suspending
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
Our suspend code touches a lot of registers all over the place, so we need to enable the power well before suspending.
--- drivers/gpu/drm/i915/i915_drv.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/gpu/drm/i915/i915_drv.c +2 -0
@@ -468,6 +468,8 @@ static int i915_drm_freeze(struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private; +intel_set_power_well (dev, true); +drm_kms_helper_poll_disable (dev);pci_save_state (dev->pdev);
Signature · 1 line
1.7.10.4
Re: [PATCH 07/10] drm/i915: turn on the power well before suspending
Quoted 4 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
Our suspend code touches a lot of registers all over the place, so we need to enable the power well before suspending.
Quoted 26 lines
Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_drv.c | 2 ++ 1 file changed, 2 insertions(+)drivers/gpu/drm/i915/i915_drv.c +2
-0@@ -468,6 +468,8 @@ static int i915_drm_freeze(struct drm_device *dev) {struct drm_i915_private *dev_priv = dev->dev_private; +intel_set_power_well (dev, true); +drm_kms_helper_poll_disable (dev);pci_save_state (dev->pdev);Signature · 6 lines
1.7.10.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 8/10 drm/i915: set TRANSCODER_EDP even earlier
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
Instead of setting it at the beginning of haswell_crtc_mode_set, let's set it at the beginning of intel_crtc_mode_set. When intel_crt_mode_set calls drm_vblank_pre_modeset we already need to have the transcoder_edp correctly set, because eventually drm_vblank_pre_modeset calls functions that call i915_pipe_enabled from i915_irq.c, which will read PIPECONF(cpu_transcoder).
This is a bug that affects us since we added support for TRANSCODER_EDP, but I was only able to see the problem after suspending a machine with the power well disabled (got an "unclaimed register" error.
--- drivers/gpu/drm/i915/intel_display.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/gpu/drm/i915/intel_display.c +5 -5
@@ -5662,11 +5662,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc, num_connectors++; } -if (is_cpu_edp) - intel_crtc->cpu_transcoder =TRANSCODER_EDP ; -else - intel_crtc->cpu_transcoder = pipe; -/* We are not sure yet this won't happen. */ WARN (!HAS_PCH_LPT (dev),"Unexpected PCH type %d\n" ,INTEL_PCH_TYPE (dev));
@@ -5731,6 +5726,11 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,int pipe = intel_crtc->pipe;int ret; +if (IS_HASWELL (dev) &&intel_pipe_has_type (crtc,INTEL_OUTPUT_EDP )) + intel_crtc->cpu_transcoder =TRANSCODER_EDP ; +else + intel_crtc->cpu_transcoder = pipe; +drm_vblank_pre_modeset (dev, pipe); ret = dev_priv->display.crtc_mode_set(crtc, mode, adjusted_mode,
Signature · 1 line
1.7.10.4
Re: [PATCH 08/10] drm/i915: set TRANSCODER_EDP even earlier
Quoted 8 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
Instead of setting it at the beginning of
haswell_crtc_mode_set, let's set it at the beginning ofintel_crtc_mode_set. Whenintel_crt_mode_setcallsdrm_vblank_pre_modesetwe already need to have thetranscoder_edpcorrectly set, because eventuallydrm_vblank_pre_modesetcalls functions that calli915_pipe_enabledfromi915_irq.c, which will readPIPECONF(cpu_transcoder).
Side note, we confusingly have two i915_pipe_enabled() functions, the one referred to here in i915_irq.c and the other in i915_suspend.c.
Quoted 46 lines
This is a bug that affects us since we added support for
TRANSCODER_EDP, but I was only able to see the problem after suspending a machine with the power well disabled (got an "unclaimed register" error.Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/intel_display.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)drivers/gpu/drm/i915/intel_display.c +5
-5@@ -5662,11 +5662,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc, num_connectors++; } -if (is_cpu_edp) - intel_crtc->cpu_transcoder =TRANSCODER_EDP ; -else - intel_crtc->cpu_transcoder = pipe; -/* We are not sure yet this won't happen. */ WARN (!HAS_PCH_LPT (dev),"Unexpected PCH type %d\n" ,INTEL_PCH_TYPE (dev));@@ -5731,6 +5726,11 @@ static int intel_crtc_mode_set(struct drm_crtc *crtc,int pipe = intel_crtc->pipe;int ret; +if (IS_HASWELL (dev) &&intel_pipe_has_type (crtc,INTEL_OUTPUT_EDP )) + intel_crtc->cpu_transcoder =TRANSCODER_EDP ; +else + intel_crtc->cpu_transcoder = pipe; +drm_vblank_pre_modeset (dev, pipe); ret = dev_priv->display.crtc_mode_set(crtc, mode, adjusted_mode,Signature · 6 lines
1.7.10.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 9/10 drm/i915: print IVB/HSW display error interrupts
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
If we get one of these messages it means we did something wrong, and the first step to fix wrong things is to detect them and recognize they exist.
For now, leave these messages as DRM_DEBUG_KMS. After we conclude that a certain message should not be print since our code is correct, then we can promote that specific message to DRM_ERROR.
--- drivers/gpu/drm/i915/i915_irq.c | 56 ++++++++++++++++++++++++++++++++++++++- drivers/gpu/drm/i915/i915_reg.h | 23 ++++++++++++++-- 2 files changed, 76 insertions(+), 3 deletions(-)
drivers/gpu/drm/i915/i915_irq.c +55 -1
@@ -697,6 +697,54 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir) I915_READ(FDI_RX_IIR(pipe))); } +static void ivb_err_int_handler (struct drm_device *dev) +{ +drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; +u32 err_int =I915_READ (GEN7_ERR_INT ); + +if (err_int &ERR_INT_POISON ) +DRM_DEBUG_KMS ("Poison interrupt\n" ); +if (err_int &ERR_INT_INVALID_GTT_PTE ) +DRM_DEBUG_KMS ("Invalid GTT PTE\n" ); +if (err_int &ERR_INT_INVALID_PTE_DATA ) +DRM_DEBUG_KMS ("Invalid GTT PTE data\n" ); +if (err_int &ERR_INT_SPRITE_GTT_FAULT_C ) +DRM_DEBUG_KMS ("Sprite C GTT fault\n" ); +if (err_int &ERR_INT_PRIMARY_GTT_FAULT_C ) +DRM_DEBUG_KMS ("Primary place C GTT fault\n" ); +if (err_int &ERR_INT_CURSOR_GTT_FAULT_C ) +DRM_DEBUG_KMS ("Cursor C GTT fault\n" ); +if (err_int &ERR_INT_SPRITE_GTT_FAULT_B ) +DRM_DEBUG_KMS ("Sprite B GTT fault\n" ); +if (err_int &ERR_INT_PRIMARY_GTT_FAULT_B ) +DRM_DEBUG_KMS ("Primary place B GTT fault\n" ); +if (err_int &ERR_INT_CURSOR_GTT_FAULT_B ) +DRM_DEBUG_KMS ("Cursor B GTT fault\n" ); +if (err_int &ERR_INT_SPRITE_GTT_FAULT_A ) +DRM_DEBUG_KMS ("Sprite A GTT fault\n" ); +if (err_int &ERR_INT_PRIMARY_GTT_FAULT_A ) +DRM_DEBUG_KMS ("Primary place A GTT fault\n" ); +if (err_int &ERR_INT_CURSOR_GTT_FAULT_A ) +DRM_DEBUG_KMS ("Cursor A GTT fault\n" ); +if (err_int &ERR_INT_PIPE_CRR_ERROR_C ) +DRM_DEBUG_KMS ("Pipe C CRC error\n" ); +if (err_int &ERR_INT_PIPE_FIFO_UNDERRRUN_C ) +DRM_DEBUG_KMS ("Pipe C FIFO underrun\n" ); +if (err_int &ERR_INT_PIPE_CRR_ERROR_B ) +DRM_DEBUG_KMS ("Pipe B CRC error\n" ); +if (err_int &ERR_INT_PIPE_FIFO_UNDERRRUN_B ) +DRM_DEBUG_KMS ("Pipe B FIFO underrun\n" ); +if (err_int &ERR_INT_PIPE_CRR_ERROR_A ) +DRM_DEBUG_KMS ("Pipe A CRC error\n" ); +if (err_int &ERR_INT_PIPE_FIFO_UNDERRRUN_A ) +DRM_DEBUG_KMS ("Pipe A FIFO underrun\n" ); + +if (IS_HASWELL (dev) && (err_int &ERR_INT_MMIO_UNCLAIMED )) +DRM_DEBUG_KMS ("MMIO cycle not claimed\n" ); + +I915_WRITE (GEN7_ERR_INT , err_int); +} +static irqreturn_t ivybridge_irq_handler (int irq,void *arg) {struct drm_device *dev = (struct drm_device *) arg;
@@ -720,6 +768,9 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg) de_iir =I915_READ (DEIIR );if (de_iir) { +if (de_iir &DE_ERR_INT_IVB ) +ivb_err_int_handler (dev); +if (de_iir &DE_AUX_CHANNEL_A_IVB )dp_aux_irq_handler (dev);
@@ -1964,7 +2015,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)DE_PLANEC_FLIP_DONE_IVB |DE_PLANEB_FLIP_DONE_IVB |DE_PLANEA_FLIP_DONE_IVB | -DE_AUX_CHANNEL_A_IVB ; +DE_AUX_CHANNEL_A_IVB |DE_ERR_INT_IVB ;u32 render_irqs;u32 hotplug_mask;u32 pch_irq_mask;
@@ -1972,6 +2023,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev) dev_priv->irq_mask = ~display_mask;/* should always can generate irq */ +I915_WRITE (GEN7_ERR_INT ,I915_READ (GEN7_ERR_INT ));I915_WRITE (DEIIR ,I915_READ (DEIIR ));I915_WRITE (DEIMR , dev_priv->irq_mask); I915_WRITE(DEIER,
@@ -2135,6 +2187,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)I915_WRITE (DEIMR ,0xffffffff );I915_WRITE (DEIER ,0x0 );I915_WRITE (DEIIR ,I915_READ (DEIIR )); +if (IS_GEN7 (dev)) +I915_WRITE (GEN7_ERR_INT ,I915_READ (GEN7_ERR_INT ));I915_WRITE (GTIMR ,0xffffffff );I915_WRITE (GTIER ,0x0 );
drivers/gpu/drm/i915/i915_reg.h +21 -2
@@ -511,7 +511,26 @@#define ERROR_GEN6 0x040a0#define GEN7_ERR_INT 0x44040 -#define ERR_INT_MMIO_UNCLAIMED (1<<13) +#define ERR_INT_POISON (1<<31) +#define ERR_INT_INVALID_GTT_PTE (1<<29) +#define ERR_INT_INVALID_PTE_DATA (1<<28) +#define ERR_INT_SPRITE_GTT_FAULT_C (1<<23) +#define ERR_INT_PRIMARY_GTT_FAULT_C (1<<22) +#define ERR_INT_CURSOR_GTT_FAULT_C (1<<21) +#define ERR_INT_SPRITE_GTT_FAULT_B (1<<20) +#define ERR_INT_PRIMARY_GTT_FAULT_B (1<<19) +#define ERR_INT_CURSOR_GTT_FAULT_B (1<<18) +#define ERR_INT_SPRITE_GTT_FAULT_A (1<<17) +#define ERR_INT_PRIMARY_GTT_FAULT_A (1<<16) +#define ERR_INT_CURSOR_GTT_FAULT_A (1<<15) +#define ERR_INT_PIPE_CRR_ERROR_C (1<<7) +#define ERR_INT_PIPE_FIFO_UNDERRRUN_C (1<<6) +#define ERR_INT_PIPE_CRR_ERROR_B (1<<4) +#define ERR_INT_PIPE_FIFO_UNDERRRUN_B (1<<3) +#define ERR_INT_PIPE_CRR_ERROR_A (1<<1) +#define ERR_INT_PIPE_FIFO_UNDERRRUN_A (1<<0) +/* Haswell only: */ +#define ERR_INT_MMIO_UNCLAIMED (1<<13)/* GM45+ chicken bits -- debug workaround bits that may be required * for various sorts of correct behavior. The top 16 bits of each are
@@ -3374,7 +3393,7 @@#define DE_PIPEA_FIFO_UNDERRUN (1 << 0)/* More Ivybridge lolz */ -#define DE_ERR_DEBUG_IVB (1<<30) +#define DE_ERR_INT_IVB (1<<30)#define DE_GSE_IVB (1<<29)#define DE_PCH_EVENT_IVB (1<<28)#define DE_DP_A_HOTPLUG_IVB (1<<27)
Signature · 1 line
1.7.10.4
Re: [PATCH 09/10] drm/i915: print IVB/HSW display error interrupts
Quoted 11 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
If we get one of these messages it means we did something wrong, and the first step to fix wrong things is to detect them and recognize they exist.
For now, leave these messages as
DRM_DEBUG_KMS. After we conclude that a certain message should not be print since our code is correct, then we can promote that specific message toDRM_ERROR.Signed-off-by Paulo Zanoni <[email protected]>
A couple of recommendations below, but either way it is
Quoted 51 lines
--- drivers/gpu/drm/i915/i915_irq.c | 56 ++++++++++++++++++++++++++++++++++++++- drivers/gpu/drm/i915/i915_reg.h | 23 ++++++++++++++-- 2 files changed, 76 insertions(+), 3 deletions(-)drivers/gpu/drm/i915/i915_irq.c +38
-0@@ -697,6 +697,54 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir) I915_READ(FDI_RX_IIR(pipe))); } +static void ivb_err_int_handler (struct drm_device *dev) +{ +drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; +u32 err_int =I915_READ (GEN7_ERR_INT ); + +if (err_int &ERR_INT_POISON ) +DRM_DEBUG_KMS ("Poison interrupt\n" ); +if (err_int &ERR_INT_INVALID_GTT_PTE ) +DRM_DEBUG_KMS ("Invalid GTT PTE\n" ); +if (err_int &ERR_INT_INVALID_PTE_DATA ) +DRM_DEBUG_KMS ("Invalid GTT PTE data\n" ); +if (err_int &ERR_INT_SPRITE_GTT_FAULT_C ) +DRM_DEBUG_KMS ("Sprite C GTT fault\n" ); +if (err_int &ERR_INT_PRIMARY_GTT_FAULT_C ) +DRM_DEBUG_KMS ("Primary place C GTT fault\n" ); +if (err_int &ERR_INT_CURSOR_GTT_FAULT_C ) +DRM_DEBUG_KMS ("Cursor C GTT fault\n" ); +if (err_int &ERR_INT_SPRITE_GTT_FAULT_B ) +DRM_DEBUG_KMS ("Sprite B GTT fault\n" ); +if (err_int &ERR_INT_PRIMARY_GTT_FAULT_B ) +DRM_DEBUG_KMS ("Primary place B GTT fault\n" ); +if (err_int &ERR_INT_CURSOR_GTT_FAULT_B ) +DRM_DEBUG_KMS ("Cursor B GTT fault\n" ); +if (err_int &ERR_INT_SPRITE_GTT_FAULT_A ) +DRM_DEBUG_KMS ("Sprite A GTT fault\n" ); +if (err_int &ERR_INT_PRIMARY_GTT_FAULT_A ) +DRM_DEBUG_KMS ("Primary place A GTT fault\n" ); +if (err_int &ERR_INT_CURSOR_GTT_FAULT_A ) +DRM_DEBUG_KMS ("Cursor A GTT fault\n" ); +if (err_int &ERR_INT_PIPE_CRR_ERROR_C ) +DRM_DEBUG_KMS ("Pipe C CRC error\n" ); +if (err_int &ERR_INT_PIPE_FIFO_UNDERRRUN_C ) +DRM_DEBUG_KMS ("Pipe C FIFO underrun\n" ); +if (err_int &ERR_INT_PIPE_CRR_ERROR_B ) +DRM_DEBUG_KMS ("Pipe B CRC error\n" ); +if (err_int &ERR_INT_PIPE_FIFO_UNDERRRUN_B ) +DRM_DEBUG_KMS ("Pipe B FIFO underrun\n" ); +if (err_int &ERR_INT_PIPE_CRR_ERROR_A )
drivers/gpu/drm/i915/i915_irq.c static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir) · line 747
Quoted 10 lines
drivers/gpu/drm/i915/i915_irq.c +10
-0+DRM_DEBUG_KMS ("Pipe A CRC error\n" ); +if (err_int &ERR_INT_PIPE_FIFO_UNDERRRUN_A ) +DRM_DEBUG_KMS ("Pipe A FIFO underrun\n" ); + +if (IS_HASWELL (dev) && (err_int &ERR_INT_MMIO_UNCLAIMED )) +DRM_DEBUG_KMS ("MMIO cycle not claimed\n" ); + +I915_WRITE (GEN7_ERR_INT , err_int); +} +
We probably don't need the IS_HASWELL() check, but it shouldn't hurt either.
I shutter to think of debugging what will happen if this handler itself generates ERR_INT_MMIO_UNCLAIMED. Hopefully the DRM_DEBUG_KMS will get a chance to get spewed out.
Since the only thing you're doing is printing, you could do something like:
u32 err_int = I915_READ(GEN7_ERR_INT);
if (drm_debug & DRM_UT_KMS == 0) {
I915_WRITE(GEN7_ERR_INT, err_int);
return;
}Quoted 87 lines
drivers/gpu/drm/i915/i915_irq.c +7
-1static irqreturn_t ivybridge_irq_handler (int irq,void *arg) {struct drm_device *dev = (struct drm_device *) arg;@@ -720,6 +768,9 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg) de_iir =I915_READ (DEIIR );if (de_iir) { +if (de_iir &DE_ERR_INT_IVB ) +ivb_err_int_handler (dev); +if (de_iir &DE_AUX_CHANNEL_A_IVB )dp_aux_irq_handler (dev);@@ -1964,7 +2015,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)DE_PLANEC_FLIP_DONE_IVB |DE_PLANEB_FLIP_DONE_IVB |DE_PLANEA_FLIP_DONE_IVB | -DE_AUX_CHANNEL_A_IVB ; +DE_AUX_CHANNEL_A_IVB |DE_ERR_INT_IVB ;u32 render_irqs;u32 hotplug_mask;u32 pch_irq_mask;@@ -1972,6 +2023,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev) dev_priv->irq_mask = ~display_mask;/* should always can generate irq */ +I915_WRITE (GEN7_ERR_INT ,I915_READ (GEN7_ERR_INT ));I915_WRITE (DEIIR ,I915_READ (DEIIR ));I915_WRITE (DEIMR , dev_priv->irq_mask); I915_WRITE(DEIER,@@ -2135,6 +2187,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)I915_WRITE (DEIMR ,0xffffffff );I915_WRITE (DEIER ,0x0 );I915_WRITE (DEIIR ,I915_READ (DEIIR )); +if (IS_GEN7 (dev)) +I915_WRITE (GEN7_ERR_INT ,I915_READ (GEN7_ERR_INT ));I915_WRITE (GTIMR ,0xffffffff );I915_WRITE (GTIER ,0x0 );drivers/gpu/drm/i915/i915_reg.h +21
-2@@ -511,7 +511,26 @@#define ERROR_GEN6 0x040a0#define GEN7_ERR_INT 0x44040 -#define ERR_INT_MMIO_UNCLAIMED (1<<13) +#define ERR_INT_POISON (1<<31) +#define ERR_INT_INVALID_GTT_PTE (1<<29) +#define ERR_INT_INVALID_PTE_DATA (1<<28) +#define ERR_INT_SPRITE_GTT_FAULT_C (1<<23) +#define ERR_INT_PRIMARY_GTT_FAULT_C (1<<22) +#define ERR_INT_CURSOR_GTT_FAULT_C (1<<21) +#define ERR_INT_SPRITE_GTT_FAULT_B (1<<20) +#define ERR_INT_PRIMARY_GTT_FAULT_B (1<<19) +#define ERR_INT_CURSOR_GTT_FAULT_B (1<<18) +#define ERR_INT_SPRITE_GTT_FAULT_A (1<<17) +#define ERR_INT_PRIMARY_GTT_FAULT_A (1<<16) +#define ERR_INT_CURSOR_GTT_FAULT_A (1<<15) +#define ERR_INT_PIPE_CRR_ERROR_C (1<<7) +#define ERR_INT_PIPE_FIFO_UNDERRRUN_C (1<<6) +#define ERR_INT_PIPE_CRR_ERROR_B (1<<4) +#define ERR_INT_PIPE_FIFO_UNDERRRUN_B (1<<3) +#define ERR_INT_PIPE_CRR_ERROR_A (1<<1) +#define ERR_INT_PIPE_FIFO_UNDERRRUN_A (1<<0) +/* Haswell only: */ +#define ERR_INT_MMIO_UNCLAIMED (1<<13)/* GM45+ chicken bits -- debug workaround bits that may be required * for various sorts of correct behavior. The top 16 bits of each are @@ -3374,7 +3393,7 @@#define DE_PIPEA_FIFO_UNDERRUN (1 << 0)/* More Ivybridge lolz */ -#define DE_ERR_DEBUG_IVB (1<<30) +#define DE_ERR_INT_IVB (1<<30)#define DE_GSE_IVB (1<<29)#define DE_PCH_EVENT_IVB (1<<28)#define DE_DP_A_HOTPLUG_IVB (1<<27)Signature · 6 lines
1.7.10.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 1 line
Ben Widawsky, Intel Open Source Technology Center
Jani Nikula
maintainerRe: [PATCH 09/10] drm/i915: print IVB/HSW display error interrupts
Quoted 81 lines · Ben Widawsky
Quoted 11 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
If we get one of these messages it means we did something wrong, and the first step to fix wrong things is to detect them and recognize they exist.
For now, leave these messages as
DRM_DEBUG_KMS. After we conclude that a certain message should not be print since our code is correct, then we can promote that specific message toDRM_ERROR.Signed-off-by Paulo Zanoni <[email protected]>A couple of recommendations below, but either way it is
Reviewed-by Ben Widawsky <[email protected]>Quoted 51 lines
--- drivers/gpu/drm/i915/i915_irq.c | 56 ++++++++++++++++++++++++++++++++++++++- drivers/gpu/drm/i915/i915_reg.h | 23 ++++++++++++++-- 2 files changed, 76 insertions(+), 3 deletions(-)drivers/gpu/drm/i915/i915_irq.c +38
-0@@ -697,6 +697,54 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir) I915_READ(FDI_RX_IIR(pipe))); } +static void ivb_err_int_handler (struct drm_device *dev) +{ +drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; +u32 err_int =I915_READ (GEN7_ERR_INT ); + +if (err_int &ERR_INT_POISON ) +DRM_DEBUG_KMS ("Poison interrupt\n" ); +if (err_int &ERR_INT_INVALID_GTT_PTE ) +DRM_DEBUG_KMS ("Invalid GTT PTE\n" ); +if (err_int &ERR_INT_INVALID_PTE_DATA ) +DRM_DEBUG_KMS ("Invalid GTT PTE data\n" ); +if (err_int &ERR_INT_SPRITE_GTT_FAULT_C ) +DRM_DEBUG_KMS ("Sprite C GTT fault\n" ); +if (err_int &ERR_INT_PRIMARY_GTT_FAULT_C ) +DRM_DEBUG_KMS ("Primary place C GTT fault\n" ); +if (err_int &ERR_INT_CURSOR_GTT_FAULT_C ) +DRM_DEBUG_KMS ("Cursor C GTT fault\n" ); +if (err_int &ERR_INT_SPRITE_GTT_FAULT_B ) +DRM_DEBUG_KMS ("Sprite B GTT fault\n" ); +if (err_int &ERR_INT_PRIMARY_GTT_FAULT_B ) +DRM_DEBUG_KMS ("Primary place B GTT fault\n" ); +if (err_int &ERR_INT_CURSOR_GTT_FAULT_B ) +DRM_DEBUG_KMS ("Cursor B GTT fault\n" ); +if (err_int &ERR_INT_SPRITE_GTT_FAULT_A ) +DRM_DEBUG_KMS ("Sprite A GTT fault\n" ); +if (err_int &ERR_INT_PRIMARY_GTT_FAULT_A ) +DRM_DEBUG_KMS ("Primary place A GTT fault\n" ); +if (err_int &ERR_INT_CURSOR_GTT_FAULT_A ) +DRM_DEBUG_KMS ("Cursor A GTT fault\n" ); +if (err_int &ERR_INT_PIPE_CRR_ERROR_C ) +DRM_DEBUG_KMS ("Pipe C CRC error\n" ); +if (err_int &ERR_INT_PIPE_FIFO_UNDERRRUN_C ) +DRM_DEBUG_KMS ("Pipe C FIFO underrun\n" ); +if (err_int &ERR_INT_PIPE_CRR_ERROR_B ) +DRM_DEBUG_KMS ("Pipe B CRC error\n" ); +if (err_int &ERR_INT_PIPE_FIFO_UNDERRRUN_B ) +DRM_DEBUG_KMS ("Pipe B FIFO underrun\n" ); +if (err_int &ERR_INT_PIPE_CRR_ERROR_A )drivers/gpu/drm/i915/i915_irq.c static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir) · line 747
Quoted 10 lines
drivers/gpu/drm/i915/i915_irq.c +10
-0+DRM_DEBUG_KMS ("Pipe A CRC error\n" ); +if (err_int &ERR_INT_PIPE_FIFO_UNDERRRUN_A ) +DRM_DEBUG_KMS ("Pipe A FIFO underrun\n" ); + +if (IS_HASWELL (dev) && (err_int &ERR_INT_MMIO_UNCLAIMED )) +DRM_DEBUG_KMS ("MMIO cycle not claimed\n" ); + +I915_WRITE (GEN7_ERR_INT , err_int); +} +We probably don't need the
IS_HASWELL()check, but it shouldn't hurt either.
Quoted 8 lines
I shutter to think of debugging what will happen if this handler itself generates
ERR_INT_MMIO_UNCLAIMED. Hopefully theDRM_DEBUG_KMSwill get a chance to get spewed out.Since the only thing you're doing is printing, you could do something like:
u32err_int=I915_READ(GEN7_ERR_INT);
if (drm_debug&DRM_UT_KMS== 0) {
Review-of-review, that needs braces or it's always false.
BR,
Jani.
Quoted 98 lines
I915_WRITE(GEN7_ERR_INT, err_int); return; }Quoted 87 lines
drivers/gpu/drm/i915/i915_irq.c +7
-1static irqreturn_t ivybridge_irq_handler (int irq,void *arg) {struct drm_device *dev = (struct drm_device *) arg;@@ -720,6 +768,9 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg) de_iir =I915_READ (DEIIR );if (de_iir) { +if (de_iir &DE_ERR_INT_IVB ) +ivb_err_int_handler (dev); +if (de_iir &DE_AUX_CHANNEL_A_IVB )dp_aux_irq_handler (dev);@@ -1964,7 +2015,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)DE_PLANEC_FLIP_DONE_IVB |DE_PLANEB_FLIP_DONE_IVB |DE_PLANEA_FLIP_DONE_IVB | -DE_AUX_CHANNEL_A_IVB ; +DE_AUX_CHANNEL_A_IVB |DE_ERR_INT_IVB ;u32 render_irqs;u32 hotplug_mask;u32 pch_irq_mask;@@ -1972,6 +2023,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev) dev_priv->irq_mask = ~display_mask;/* should always can generate irq */ +I915_WRITE (GEN7_ERR_INT ,I915_READ (GEN7_ERR_INT ));I915_WRITE (DEIIR ,I915_READ (DEIIR ));I915_WRITE (DEIMR , dev_priv->irq_mask); I915_WRITE(DEIER,@@ -2135,6 +2187,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)I915_WRITE (DEIMR ,0xffffffff );I915_WRITE (DEIER ,0x0 );I915_WRITE (DEIIR ,I915_READ (DEIIR )); +if (IS_GEN7 (dev)) +I915_WRITE (GEN7_ERR_INT ,I915_READ (GEN7_ERR_INT ));I915_WRITE (GTIMR ,0xffffffff );I915_WRITE (GTIER ,0x0 );drivers/gpu/drm/i915/i915_reg.h +21
-2@@ -511,7 +511,26 @@#define ERROR_GEN6 0x040a0#define GEN7_ERR_INT 0x44040 -#define ERR_INT_MMIO_UNCLAIMED (1<<13) +#define ERR_INT_POISON (1<<31) +#define ERR_INT_INVALID_GTT_PTE (1<<29) +#define ERR_INT_INVALID_PTE_DATA (1<<28) +#define ERR_INT_SPRITE_GTT_FAULT_C (1<<23) +#define ERR_INT_PRIMARY_GTT_FAULT_C (1<<22) +#define ERR_INT_CURSOR_GTT_FAULT_C (1<<21) +#define ERR_INT_SPRITE_GTT_FAULT_B (1<<20) +#define ERR_INT_PRIMARY_GTT_FAULT_B (1<<19) +#define ERR_INT_CURSOR_GTT_FAULT_B (1<<18) +#define ERR_INT_SPRITE_GTT_FAULT_A (1<<17) +#define ERR_INT_PRIMARY_GTT_FAULT_A (1<<16) +#define ERR_INT_CURSOR_GTT_FAULT_A (1<<15) +#define ERR_INT_PIPE_CRR_ERROR_C (1<<7) +#define ERR_INT_PIPE_FIFO_UNDERRRUN_C (1<<6) +#define ERR_INT_PIPE_CRR_ERROR_B (1<<4) +#define ERR_INT_PIPE_FIFO_UNDERRRUN_B (1<<3) +#define ERR_INT_PIPE_CRR_ERROR_A (1<<1) +#define ERR_INT_PIPE_FIFO_UNDERRRUN_A (1<<0) +/* Haswell only: */ +#define ERR_INT_MMIO_UNCLAIMED (1<<13)/* GM45+ chicken bits -- debug workaround bits that may be required * for various sorts of correct behavior. The top 16 bits of each are @@ -3374,7 +3393,7 @@#define DE_PIPEA_FIFO_UNDERRUN (1 << 0)/* More Ivybridge lolz */ -#define DE_ERR_DEBUG_IVB (1<<30) +#define DE_ERR_INT_IVB (1<<30)#define DE_GSE_IVB (1<<29)#define DE_PCH_EVENT_IVB (1<<28)#define DE_DP_A_HOTPLUG_IVB (1<<27)Signature · 6 lines
1.7.10.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfxSignature · 5 lines
Ben Widawsky, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH 09/10] drm/i915: print IVB/HSW display error interrupts
In reply to Paulo ZanoniQuoted 21 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
If we get one of these messages it means we did something wrong, and the first step to fix wrong things is to detect them and recognize they exist.
For now, leave these messages as
DRM_DEBUG_KMS. After we conclude that a certain message should not be print since our code is correct, then we can promote that specific message toDRM_ERROR.Signed-off-by Paulo Zanoni <[email protected]>--- drivers/gpu/drm/i915/i915_irq.c | 56 ++++++++++++++++++++++++++++++++++++++- drivers/gpu/drm/i915/i915_reg.h | 23 ++++++++++++++-- 2 files changed, 76 insertions(+), 3 deletions(-)drivers/gpu/drm/i915/i915_irq.c +0
-0
drivers/gpu/drm/i915/i915_irq.c static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir) · line 702
Quoted 6 lines
drivers/gpu/drm/i915/i915_irq.c +3
-0@@ -697,6 +697,54 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir) I915_READ(FDI_RX_IIR(pipe))); } +static void ivb_err_int_handler (struct drm_device *dev) +{ +drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
dev->dev_private being a void *, the cast is unnecessary.
Quoted 17 lines
drivers/gpu/drm/i915/i915_irq.c +17
-0+u32 err_int =I915_READ (GEN7_ERR_INT ); + +if (err_int &ERR_INT_POISON ) +DRM_DEBUG_KMS ("Poison interrupt\n" ); +if (err_int &ERR_INT_INVALID_GTT_PTE ) +DRM_DEBUG_KMS ("Invalid GTT PTE\n" ); +if (err_int &ERR_INT_INVALID_PTE_DATA ) +DRM_DEBUG_KMS ("Invalid GTT PTE data\n" ); +if (err_int &ERR_INT_SPRITE_GTT_FAULT_C ) +DRM_DEBUG_KMS ("Sprite C GTT fault\n" ); +if (err_int &ERR_INT_PRIMARY_GTT_FAULT_C ) +DRM_DEBUG_KMS ("Primary place C GTT fault\n" ); +if (err_int &ERR_INT_CURSOR_GTT_FAULT_C ) +DRM_DEBUG_KMS ("Cursor C GTT fault\n" ); +if (err_int &ERR_INT_SPRITE_GTT_FAULT_B ) +DRM_DEBUG_KMS ("Sprite B GTT fault\n" ); +if (err_int &ERR_INT_PRIMARY_GTT_FAULT_B )
drivers/gpu/drm/i915/i915_irq.c static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir) · line 729
Quoted 10 lines
drivers/gpu/drm/i915/i915_irq.c +10
-0+DRM_DEBUG_KMS ("Primary place B GTT fault\n" ); +if (err_int &ERR_INT_CURSOR_GTT_FAULT_B ) +DRM_DEBUG_KMS ("Cursor B GTT fault\n" ); +if (err_int &ERR_INT_SPRITE_GTT_FAULT_A ) +DRM_DEBUG_KMS ("Sprite A GTT fault\n" ); +if (err_int &ERR_INT_PRIMARY_GTT_FAULT_A ) +DRM_DEBUG_KMS ("Primary place A GTT fault\n" ); +if (err_int &ERR_INT_CURSOR_GTT_FAULT_A ) +DRM_DEBUG_KMS ("Cursor A GTT fault\n" ); +if (err_int &ERR_INT_PIPE_CRR_ERROR_C )
CRR? Should it be CRC? Ditto for the others below.
Also, how about trading some clarity of error messages to less duplication of info with something like:
#define err_print(status, bit) do { \
if ((status) & (bit)) \
DRM_DEBUG_KMS("Error interrupt %s\n", #bit);\
} while (0)
err_print(err_int, ERR_INT_POISON);
err_print(err_int, ERR_INT_INVALID_GTT_PTE);
err_print(err_int, ERR_INT_INVALID_PTE_DATA);
...#undef err_print
This is just bikeshedding though, up to you.
Quoted 105 lines
drivers/gpu/drm/i915/i915_irq.c +25
-1+DRM_DEBUG_KMS ("Pipe C CRC error\n" ); +if (err_int &ERR_INT_PIPE_FIFO_UNDERRRUN_C ) +DRM_DEBUG_KMS ("Pipe C FIFO underrun\n" ); +if (err_int &ERR_INT_PIPE_CRR_ERROR_B ) +DRM_DEBUG_KMS ("Pipe B CRC error\n" ); +if (err_int &ERR_INT_PIPE_FIFO_UNDERRRUN_B ) +DRM_DEBUG_KMS ("Pipe B FIFO underrun\n" ); +if (err_int &ERR_INT_PIPE_CRR_ERROR_A ) +DRM_DEBUG_KMS ("Pipe A CRC error\n" ); +if (err_int &ERR_INT_PIPE_FIFO_UNDERRRUN_A ) +DRM_DEBUG_KMS ("Pipe A FIFO underrun\n" ); + +if (IS_HASWELL (dev) && (err_int &ERR_INT_MMIO_UNCLAIMED )) +DRM_DEBUG_KMS ("MMIO cycle not claimed\n" ); + +I915_WRITE (GEN7_ERR_INT , err_int); +} +static irqreturn_t ivybridge_irq_handler (int irq,void *arg) {struct drm_device *dev = (struct drm_device *) arg;@@ -720,6 +768,9 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg) de_iir =I915_READ (DEIIR );if (de_iir) { +if (de_iir &DE_ERR_INT_IVB ) +ivb_err_int_handler (dev); +if (de_iir &DE_AUX_CHANNEL_A_IVB )dp_aux_irq_handler (dev);@@ -1964,7 +2015,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)DE_PLANEC_FLIP_DONE_IVB |DE_PLANEB_FLIP_DONE_IVB |DE_PLANEA_FLIP_DONE_IVB | -DE_AUX_CHANNEL_A_IVB ; +DE_AUX_CHANNEL_A_IVB |DE_ERR_INT_IVB ;u32 render_irqs;u32 hotplug_mask;u32 pch_irq_mask;@@ -1972,6 +2023,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev) dev_priv->irq_mask = ~display_mask;/* should always can generate irq */ +I915_WRITE (GEN7_ERR_INT ,I915_READ (GEN7_ERR_INT ));I915_WRITE (DEIIR ,I915_READ (DEIIR ));I915_WRITE (DEIMR , dev_priv->irq_mask); I915_WRITE(DEIER,@@ -2135,6 +2187,8 @@ static void ironlake_irq_uninstall(struct drm_device *dev)I915_WRITE (DEIMR ,0xffffffff );I915_WRITE (DEIER ,0x0 );I915_WRITE (DEIIR ,I915_READ (DEIIR )); +if (IS_GEN7 (dev)) +I915_WRITE (GEN7_ERR_INT ,I915_READ (GEN7_ERR_INT ));I915_WRITE (GTIMR ,0xffffffff );I915_WRITE (GTIER ,0x0 );drivers/gpu/drm/i915/i915_reg.h +21
-2@@ -511,7 +511,26 @@#define ERROR_GEN6 0x040a0#define GEN7_ERR_INT 0x44040 -#define ERR_INT_MMIO_UNCLAIMED (1<<13) +#define ERR_INT_POISON (1<<31) +#define ERR_INT_INVALID_GTT_PTE (1<<29) +#define ERR_INT_INVALID_PTE_DATA (1<<28) +#define ERR_INT_SPRITE_GTT_FAULT_C (1<<23) +#define ERR_INT_PRIMARY_GTT_FAULT_C (1<<22) +#define ERR_INT_CURSOR_GTT_FAULT_C (1<<21) +#define ERR_INT_SPRITE_GTT_FAULT_B (1<<20) +#define ERR_INT_PRIMARY_GTT_FAULT_B (1<<19) +#define ERR_INT_CURSOR_GTT_FAULT_B (1<<18) +#define ERR_INT_SPRITE_GTT_FAULT_A (1<<17) +#define ERR_INT_PRIMARY_GTT_FAULT_A (1<<16) +#define ERR_INT_CURSOR_GTT_FAULT_A (1<<15) +#define ERR_INT_PIPE_CRR_ERROR_C (1<<7) +#define ERR_INT_PIPE_FIFO_UNDERRRUN_C (1<<6) +#define ERR_INT_PIPE_CRR_ERROR_B (1<<4) +#define ERR_INT_PIPE_FIFO_UNDERRRUN_B (1<<3) +#define ERR_INT_PIPE_CRR_ERROR_A (1<<1) +#define ERR_INT_PIPE_FIFO_UNDERRRUN_A (1<<0) +/* Haswell only: */ +#define ERR_INT_MMIO_UNCLAIMED (1<<13)/* GM45+ chicken bits -- debug workaround bits that may be required * for various sorts of correct behavior. The top 16 bits of each are @@ -3374,7 +3393,7 @@#define DE_PIPEA_FIFO_UNDERRUN (1 << 0)/* More Ivybridge lolz */ -#define DE_ERR_DEBUG_IVB (1<<30) +#define DE_ERR_INT_IVB (1<<30)#define DE_GSE_IVB (1<<29)#define DE_PCH_EVENT_IVB (1<<28)#define DE_DP_A_HOTPLUG_IVB (1<<27)Signature · 6 lines
1.7.10.4 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 10/10 drm/i915: remove "unclaimed register" checks from I915_WRITE
In reply to Paulo ZanoniFrom: Paulo Zanoni <[email protected]>
Some developers don't really like this code polluting I915_WRITE, and we've never really measured its negative impacts. So now that we properly print ERR_INT interrupts, let's remove the I915_WRITE code and promote the interrupt error message to DRM_ERROR.
The downside of this change is that we lose the ability to check the register and print nice backtraces, but at this point most of the errors have already been fixed and we're investigating the few remaining cases.
--- drivers/gpu/drm/i915/i915_drv.c | 8 -------- drivers/gpu/drm/i915/i915_irq.c | 2 +- 2 files changed, 1 insertion(+), 9 deletions(-)
drivers/gpu/drm/i915/i915_drv.c +0 -8
@@ -1260,10 +1260,6 @@ void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val) { \ } \if (IS_GEN5 (dev_priv->dev)) \ilk_dummy_write (dev_priv); \ -if (IS_HASWELL (dev_priv->dev) && (I915_READ_NOTRACE (GEN7_ERR_INT ) &ERR_INT_MMIO_UNCLAIMED )) { \ -DRM_ERROR ("Unknown unclaimed register before writing to %x\n" , reg); \ -I915_WRITE_NOTRACE (GEN7_ERR_INT ,ERR_INT_MMIO_UNCLAIMED ); \ - } \if (IS_VALLEYVIEW (dev_priv->dev) &&IS_DISPLAYREG (reg)) { \ write##y(val, dev_priv->regs + reg + 0x180000); \ } else { \
@@ -1272,10 +1268,6 @@ void i915_write##x(struct drm_i915_private *dev_priv, u32 reg, u##x val) { \if (unlikely (__fifo_ret)) { \gen6_gt_check_fifodbg (dev_priv); \ } \ -if (IS_HASWELL (dev_priv->dev) && (I915_READ_NOTRACE (GEN7_ERR_INT ) &ERR_INT_MMIO_UNCLAIMED )) { \ -DRM_ERROR ("Unclaimed write to %x\n" , reg); \ -writel (ERR_INT_MMIO_UNCLAIMED , dev_priv->regs +GEN7_ERR_INT ); \ - } \ } __i915_write(8, b)__i915_write (16 , w)
drivers/gpu/drm/i915/i915_irq.c +1 -1
@@ -740,7 +740,7 @@ static void ivb_err_int_handler(struct drm_device *dev)DRM_DEBUG_KMS ("Pipe A FIFO underrun\n" );if (IS_HASWELL (dev) && (err_int &ERR_INT_MMIO_UNCLAIMED )) -DRM_DEBUG_KMS ("MMIO cycle not claimed\n" ); +DRM_ERROR ("MMIO cycle not claimed\n" );I915_WRITE (GEN7_ERR_INT , err_int); }
Signature · 1 line
1.7.10.4
Re: [PATCH 10/10] drm/i915: remove "unclaimed register" checks from I915_WRITE
Quoted 7 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
Some developers don't really like this code polluting
I915_WRITE, and we've never really measured its negative impacts. So now that we properly printERR_INTinterrupts, let's remove theI915_WRITEcode and promote the interrupt error message toDRM_ERROR.
Quoted 6 lines
The downside of this change is that we lose the ability to check the register and print nice backtraces, but at this point most of the errors have already been fixed and we're investigating the few remaining cases.
Signed-off-by Paulo Zanoni <[email protected]>
I'm really sad to see this go. Especially since our time between new platform bring-up is decreasing so much. If I were to guess right, every developer working on a new platform would want this. So while HSW may be in the clear, HSW+1 suffers.
Could I request that you measure the negative impacts?
Re: [PATCH 10/10] drm/i915: remove "unclaimed register" checks from I915_WRITE
Quoted 13 lines · Ben Widawsky
Quoted 12 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
Some developers don't really like this code polluting
I915_WRITE, and we've never really measured its negative impacts. So now that we properly printERR_INTinterrupts, let's remove theI915_WRITEcode and promote the interrupt error message toDRM_ERROR.The downside of this change is that we lose the ability to check the register and print nice backtraces, but at this point most of the errors have already been fixed and we're investigating the few remaining cases.
Quoted 6 lines
Quoted 1 line
Signed-off-by Paulo Zanoni <[email protected]>I'm really sad to see this go. Especially since our time between new platform bring-up is decreasing so much. If I were to guess right, every developer working on a new platform would want this. So while HSW may be in the clear, HSW+1 suffers.
I don't like the extra work per iowrite32, but I can live with for the error-detection. I would rather remove it for special cases where it has demonstrable impact, perhaps in execbuffer. However, you can equally lay the blame there for execbuffer hitting i915_write32 too often. So even there I am not convinced that speeding up i915_write32 is the best approach.
So NAKed-by: Chris Wilson <[email protected]> unless you can find a way to work out the faulting register address in the interrupt or that is the only method going forward.
-Chris
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre