Unclaimed registers and power well V2
Subsystems (2)
cover Unclaimed registers and power well V2
From: Paulo Zanoni <[email protected]>
Hi
So here's the second version of the patches that fix some "unclaimed register" errors, making our driver good enough to start disabling the power well when possible (we're still not 100% "unclaimed register" free, recent patches added some new unclaimed register problems when the power well is disabled, but IMHO we're good enough).
The changes are based on the reviews and also on some information provided by the hardware engineers regarding the places where our specification was confusing.
The patches that were previously patches 9 and 10 were removed, they will be part of a future patch series only for them.
Thanks,
Paulo
Daniel Vetter (1):
drm/i915: dynamic Haswell display power well support
Paulo Zanoni (6):
drm/i915: don't send DP idle pattern before normal pattern on HSW 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
drivers/gpu/drm/i915/i915_drv.c | 2 ++ drivers/gpu/drm/i915/i915_reg.h | 8 ++--- 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 | 57 +++++++++++++++++++++++---------- 8 files changed, 103 insertions(+), 45 deletions(-)
Signature · 1 line
1.7.10.4
PATCH 1/7 drm/i915: don't send DP idle pattern before normal pattern on HSW
From: Paulo Zanoni <[email protected]>
Previously I sent "drm/i915: don't read DP_TP_STATUS(PORT_A)", but after some more discussion I was told by a hardware engineer that we don't really need to send the idle patterns before the normal pattern in our current code: we only need this for a DP mode that we currently don't support. So for now, just kill the whole code. I've already asked for an update on the documentation, so at some point this code should match the docs.
This solves "Timed out waiting for DP idle patterns" and "unclaimed register" messages on eDP.
--- drivers/gpu/drm/i915/intel_dp.c | 9 --------- 1 file changed, 9 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +0 -9
@@ -1785,16 +1785,7 @@ intel_dp_set_link_train(struct intel_dp *intel_dp, temp &= ~DP_TP_CTL_LINK_TRAIN_MASK ;switch (dp_train_pat &DP_TRAINING_PATTERN_MASK ) {case DP_TRAINING_PATTERN_DISABLE : - 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" ); - - temp &= ~DP_TP_CTL_LINK_TRAIN_MASK ; temp |=DP_TP_CTL_LINK_TRAIN_NORMAL ; -break ;case DP_TRAINING_PATTERN_1 : temp |=DP_TP_CTL_LINK_TRAIN_PAT1 ;
Signature · 1 line
1.7.10.4
Re: [PATCH 1/7] drm/i915: don't send DP idle pattern before normal pattern on HSW
Quoted 10 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
Previously I sent "drm/i915: don't read
DP_TP_STATUS(PORT_A)", but after some more discussion I was told by a hardware engineer that we don't really need to send the idle patterns before the normal pattern in our current code: we only need this for a DP mode that we currently don't support. So for now, just kill the whole code. I've already asked for an update on the documentation, so at some point this code should match the docs.
Quoted 4 lines
This solves "Timed out waiting for DP idle patterns" and "unclaimed register" messages on eDP.
Signed-off-by Paulo Zanoni <[email protected]>
Since we already have the code, and it should be harmless to run this on DP ports B-D, can't we just disable this on port A? I fear that if we ever get around to enable that funky other DP mode, we'll miss this and suffer through some ugly bugs ...
-Daniel
Quoted 32 lines
--- drivers/gpu/drm/i915/intel_dp.c | 9 --------- 1 file changed, 9 deletions(-)drivers/gpu/drm/i915/intel_dp.c +0
-9@@ -1785,16 +1785,7 @@ intel_dp_set_link_train(struct intel_dp *intel_dp, temp &= ~DP_TP_CTL_LINK_TRAIN_MASK ;switch (dp_train_pat &DP_TRAINING_PATTERN_MASK ) {case DP_TRAINING_PATTERN_DISABLE : - 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" ); - - temp &= ~DP_TP_CTL_LINK_TRAIN_MASK ; temp |=DP_TP_CTL_LINK_TRAIN_NORMAL ; -break ;case DP_TRAINING_PATTERN_1 : temp |=DP_TP_CTL_LINK_TRAIN_PAT1 ;Signature · 6 lines
1.7.10.4 _______________________________________________ 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
PATCH 2/7 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 suppose 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.
V2:
- Remove debug messages that could be misleading due to possible race conditions with KVMr, Debug and BIOS.
- Don't wait on disabling: after a conversaion with a hardware engineer we discovered that the "restriction" on bit 31 is just for the "enable" case, and we don't even need to wait on the "disable" case.
--- 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 | 57 ++++++++++++++++++++++++---------- 4 files changed, 46 insertions(+), 26 deletions(-)
drivers/gpu/drm/i915/i915_reg.h +4 -4
@@ -4416,10 +4416,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
@@ -8664,10 +8664,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
@@ -667,7 +667,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 +40 -17
73 lines not on this page · raw message
Signature · 1 line
1.7.10.4
Daniel Vetter
AppliedRe: [PATCH 2/7] drm/i915: fix intel_init_power_wells
Quoted 25 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 suppose 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.
V2:
- Remove debug messages that could be misleading due to possible race conditions with KVMr, Debug and BIOS.
- Don't wait on disabling: after a conversaion with a hardware engineer we discovered that the "restriction" on bit 31 is just for the "enable" case, and we don't even need to wait on the "disable" case.
Quoted 1 line
Signed-off-by Paulo Zanoni <[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 3/7 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
v4: Made by Paulo Zanoni
- One more typo spotted by Jani Nikula
--- 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
@@ -988,7 +988,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
@@ -5601,6 +5601,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 available. 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,
@@ -8485,6 +8514,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
@@ -668,6 +668,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
PATCH 4/7 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 5/7 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.
V2: Don't check whether the power well is enabled or not, just check whether we asked it to be enabled or not: if we asked to disable the power well, don't use the registers on it, even if it's still enabled.
--- 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_ENABLE )) { + 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 5/7] drm/i915: check the power down well on assert_pipe()
Quoted 23 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.
V2: Don't check whether the power well is enabled or not, just check whether we asked it to be enabled or not: if we asked to disable the power well, don't use the registers on it, even if it's still enabled.
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
-1@@ -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);
drivers/gpu/drm/i915/intel_display.c void assert_pipe(struct drm_i915_private *dev_priv, · line 1224
Quoted 10 lines
drivers/gpu/drm/i915/intel_display.c +8
-2- val =I915_READ (reg); - cur_state = !!(val &PIPECONF_ENABLE ); +if (cpu_transcoder ==TRANSCODER_EDP || + (I915_READ (HSW_PWR_WELL_DRIVER ) &HSW_PWR_WELL_ENABLE )) { + reg =PIPECONF (cpu_transcoder); + val =I915_READ (reg); + cur_state = !!(val &PIPECONF_ENABLE ); + }else { + cur_state = false; + }
This blows up on !hsw in big ways, since it makes the paranoid modeset state checker really unhappy. I've dropped the patch for now again from dinq. Thanks to Sedat Dilek for reporting.
-Daniel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
PATCH 6/7 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
@@ -470,6 +470,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
PATCH 7/7 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
@@ -5668,11 +5668,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));
@@ -5737,6 +5732,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 7/7] drm/i915: set TRANSCODER_EDP even earlier
Quoted 9 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).
Quoted 7 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]>Reviewed-by Jani Nikula <[email protected]>
I've picked up patches 4-7 from this series. I'm leaving patch 3 out for now until that entire "unclaimed register write business" with the dynamic power down well code is settled.
-Daniel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch