[PATCH 1/5] drm/i915: Use dp_detect_common in hotplug helper [PATCH 2/5] drm/i915: Rename i915_dp_detect_common to [PATCH 3/5] drm/i915: In intel_dp_init, replace read of DPCD with
These three are simple cleanups to centralize all places where the DPCD block was read from the device. Now everyone shares the same function, and that function retries the reads.
[PATCH 4/5] drm/i915: Delay 250ms before running the hotplug code
I was experimenting with DP hotplugging -- moving the plug in and out of the jack very slowly and discovered that the hotplug interrupt occurred well before or after the link for the aux data channel was connected or disconnected. The result of this was that a sufficiently rapid cycle back through user mode could easily beat the motion of the plug and cause the hotplug detection to get the wrong status. Sticking a 250ms delay before doing anything gives the user sufficient time to actually get the plug connected or disconnected.
[PATCH 5/5] drm/i915: DP_PIPE_ENABLED must check transcoder on CPT
This is a fairly nice bug fix to finally have. The symptom I saw was that going from one two-head configuration to another would sometimes turn off the monitor which was *not* being modified. For instance, I would do:
$ xrandr --output LVDS1 --below DP2
This would always turn off the DP2 monitor, and sometimes it wouldn't turn back on.
Turns out the bug wasn't that the mode setting code was doing it wrong and turning the DP2 output off intentionally as a part of the mode change. Instead, the intel driver was trying to adjust the PCH link for the LVDS1 output and thought it needed to turn the DP2 output off because it mistakenly believed the DP2 output was sharing the same pipe as the LVDS1 output. Just a matter of using the wrong mechanism to detect which pipe the DP2 output was connected to.
In any case, review and testing appreciated.
-keith
This uses the common dpcd reading routine, i915_dp_detect_common, instead of open-coding a call to intel_dp_aux_native_read. Besides reducing duplicated code, this also gains the read retries which may be necessary when a cable is first plugged back in and the link needs to be retrained.
Signed-off-by: Keith Packard keithp@keithp.com --- drivers/gpu/drm/i915/intel_dp.c | 39 +++++++++++++++++++-------------------- 1 files changed, 19 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index dcc7ae6..45db810 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1567,6 +1567,20 @@ intel_dp_link_down(struct intel_dp *intel_dp) POSTING_READ(intel_dp->output_reg); }
+static enum drm_connector_status +i915_dp_detect_common(struct intel_dp *intel_dp) +{ + enum drm_connector_status status = connector_status_disconnected; + + if (intel_dp_aux_native_read_retry(intel_dp, 0x000, intel_dp->dpcd, + sizeof (intel_dp->dpcd)) && + (intel_dp->dpcd[DP_DPCD_REV] != 0)) { + status = connector_status_connected; + } + + return status; +} + /* * According to DP spec * 5.1.2: @@ -1579,45 +1593,30 @@ intel_dp_link_down(struct intel_dp *intel_dp) static void intel_dp_check_link_status(struct intel_dp *intel_dp) { - int ret; - if (!intel_dp->base.base.crtc) return;
+ /* Try to read receiver status if the link appears to be up */ if (!intel_dp_get_link_status(intel_dp)) { intel_dp_link_down(intel_dp); return; }
- /* Try to read receiver status if the link appears to be up */ - ret = intel_dp_aux_native_read(intel_dp, - 0x000, intel_dp->dpcd, - sizeof (intel_dp->dpcd)); - if (ret != sizeof(intel_dp->dpcd)) { + /* Now read the DPCD to see if it's actually running */ + if (i915_dp_detect_common(intel_dp) != connector_status_connected) { intel_dp_link_down(intel_dp); return; }
if (!intel_channel_eq_ok(intel_dp)) { + DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n", + drm_get_encoder_name(&intel_dp->base.base)); intel_dp_start_link_train(intel_dp); intel_dp_complete_link_train(intel_dp); } }
static enum drm_connector_status -i915_dp_detect_common(struct intel_dp *intel_dp) -{ - enum drm_connector_status status = connector_status_disconnected; - - if (intel_dp_aux_native_read_retry(intel_dp, 0x000, intel_dp->dpcd, - sizeof (intel_dp->dpcd)) && - (intel_dp->dpcd[DP_DPCD_REV] != 0)) - status = connector_status_connected; - - return status; -} - -static enum drm_connector_status ironlake_dp_detect(struct intel_dp *intel_dp) { enum drm_connector_status status;
On Mon, 25 Jul 2011 23:36:30 -0700 Keith Packard keithp@keithp.com wrote:
This uses the common dpcd reading routine, i915_dp_detect_common, instead of open-coding a call to intel_dp_aux_native_read. Besides reducing duplicated code, this also gains the read retries which may be necessary when a cable is first plugged back in and the link needs to be retrained.
Signed-off-by: Keith Packard keithp@keithp.com
drivers/gpu/drm/i915/intel_dp.c | 39 +++++++++++++++++++-------------------- 1 files changed, 19 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index dcc7ae6..45db810 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1567,6 +1567,20 @@ intel_dp_link_down(struct intel_dp *intel_dp) POSTING_READ(intel_dp->output_reg); }
+static enum drm_connector_status +i915_dp_detect_common(struct intel_dp *intel_dp)
Maybe you can fix the prefix of this function while you're at it since it's not i915 or even i9xx specific?
This describes the function better, allowing it to be used where the DPCD value is relevant.
Signed-off-by: Keith Packard keithp@keithp.com --- drivers/gpu/drm/i915/intel_dp.c | 24 +++++++++++++++--------- 1 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 45db810..41674e1 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1567,18 +1567,16 @@ intel_dp_link_down(struct intel_dp *intel_dp) POSTING_READ(intel_dp->output_reg); }
-static enum drm_connector_status -i915_dp_detect_common(struct intel_dp *intel_dp) +static bool +intel_dp_get_dpcd(struct intel_dp *intel_dp) { - enum drm_connector_status status = connector_status_disconnected; - if (intel_dp_aux_native_read_retry(intel_dp, 0x000, intel_dp->dpcd, sizeof (intel_dp->dpcd)) && (intel_dp->dpcd[DP_DPCD_REV] != 0)) { - status = connector_status_connected; + return true; }
- return status; + return false; }
/* @@ -1603,7 +1601,7 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) }
/* Now read the DPCD to see if it's actually running */ - if (i915_dp_detect_common(intel_dp) != connector_status_connected) { + if (!intel_dp_get_dpcd(intel_dp)) { intel_dp_link_down(intel_dp); return; } @@ -1617,6 +1615,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) }
static enum drm_connector_status +intel_dp_detect_dpcd(struct intel_dp *intel_dp) +{ + if (intel_dp_get_dpcd(intel_dp)) + return connector_status_connected; + return connector_status_disconnected; +} + +static enum drm_connector_status ironlake_dp_detect(struct intel_dp *intel_dp) { enum drm_connector_status status; @@ -1629,7 +1635,7 @@ ironlake_dp_detect(struct intel_dp *intel_dp) return status; }
- return i915_dp_detect_common(intel_dp); + return intel_dp_detect_dpcd(intel_dp); }
static enum drm_connector_status @@ -1658,7 +1664,7 @@ g4x_dp_detect(struct intel_dp *intel_dp) if ((temp & bit) == 0) return connector_status_disconnected;
- return i915_dp_detect_common(intel_dp); + return intel_dp_detect_dpcd(intel_dp); }
/**
On Mon, 25 Jul 2011 23:36:31 -0700 Keith Packard keithp@keithp.com wrote:
This describes the function better, allowing it to be used where the DPCD value is relevant.
Signed-off-by: Keith Packard keithp@keithp.com
Ah I see you've addressed my previous comment already. :)
You can add my Reviewed-by: Jesse Barnes jbarnes@virtuousgeek.org to 1/5 and 2/5.
Eliminates an open-coded read and also gains the retry behaviour of intel_dp_get_dpcd, which seems like a good idea.
Signed-off-by: Keith Packard keithp@keithp.com --- drivers/gpu/drm/i915/intel_dp.c | 8 +++----- 1 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 41674e1..9f134d2 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -2015,7 +2015,7 @@ intel_dp_init(struct drm_device *dev, int output_reg)
/* Cache some DPCD data in the eDP case */ if (is_edp(intel_dp)) { - int ret; + bool ret; u32 pp_on, pp_div;
pp_on = I915_READ(PCH_PP_ON_DELAYS); @@ -2028,11 +2028,9 @@ intel_dp_init(struct drm_device *dev, int output_reg) dev_priv->panel_t12 *= 100; /* t12 in 100ms units */
ironlake_edp_panel_vdd_on(intel_dp); - ret = intel_dp_aux_native_read(intel_dp, DP_DPCD_REV, - intel_dp->dpcd, - sizeof(intel_dp->dpcd)); + ret = intel_dp_get_dpcd(intel_dp); ironlake_edp_panel_vdd_off(intel_dp); - if (ret == sizeof(intel_dp->dpcd)) { + if (ret) { if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11) dev_priv->no_aux_handshake = intel_dp->dpcd[DP_MAX_DOWNSPREAD] &
On Mon, 25 Jul 2011 23:36:32 -0700 Keith Packard keithp@keithp.com wrote:
Eliminates an open-coded read and also gains the retry behaviour of intel_dp_get_dpcd, which seems like a good idea.
Signed-off-by: Keith Packard keithp@keithp.com
drivers/gpu/drm/i915/intel_dp.c | 8 +++----- 1 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 41674e1..9f134d2 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -2015,7 +2015,7 @@ intel_dp_init(struct drm_device *dev, int output_reg)
/* Cache some DPCD data in the eDP case */ if (is_edp(intel_dp)) {
int ret;
bool ret;u32 pp_on, pp_div;
pp_on = I915_READ(PCH_PP_ON_DELAYS);
@@ -2028,11 +2028,9 @@ intel_dp_init(struct drm_device *dev, int output_reg) dev_priv->panel_t12 *= 100; /* t12 in 100ms units */
ironlake_edp_panel_vdd_on(intel_dp);
ret = intel_dp_aux_native_read(intel_dp, DP_DPCD_REV,intel_dp->dpcd,sizeof(intel_dp->dpcd));
ret = intel_dp_get_dpcd(intel_dp);
if (ret == sizeof(intel_dp->dpcd)) {
if (ret) { if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11) dev_priv->no_aux_handshake = intel_dp->dpcd[DP_MAX_DOWNSPREAD] &
Reviewed-by: Jesse Barnes jbarnes@virtuousgeek.org
Now we just have to enable fast link training in the eDP case (and optionally when we know the DP monitor hasn't changed, just DPMS'd).
If the connector is inserted or removed slowly, the hotplug line may well change state before the data lines do. So, assume the user isn't trying to fool us and give them 250ms to get the connector plugged or unplugged.
Signed-off-by: Keith Packard keithp@keithp.com --- drivers/gpu/drm/i915/i915_irq.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 9da2a2c..e3ce1c3 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -306,6 +306,8 @@ static void i915_hotplug_work_func(struct work_struct *work) struct drm_mode_config *mode_config = &dev->mode_config; struct intel_encoder *encoder;
+ /* Wait a bit so that the connector change can be completed */ + msleep(250); mutex_lock(&mode_config->mutex); DRM_DEBUG_KMS("running encoder hotplug functions\n");
queue_delayed_work? Plays nicer with other workqueue-items.
-Daniel
PS: Scrap my other mail, just noticed that the merged patched r-b'ed by Jesse uses the mode_config mutex.
On Tue, 26 Jul 2011 09:44:32 +0200, Daniel Vetter daniel@ffwll.ch wrote:
queue_delayed_work? Plays nicer with other workqueue-items.
Yeah, I'll change this.
Display port pipe selection on CPT is not done with a bit in the output register, rather it is controlled by a couple of bits in the separate transcoder register which indicate which display port output is connected to the transcoder.
This patch replaces the simplistic macro DP_PIPE_ENABLED with the rather more complicated function dp_pipe_enabled which checks the output register to see if that is enabled, and then goes on to either check the output register pipe selection bit (on non-CPT) or the transcoder DP selection bits (on CPT).
Before this patch, any time the mode of pipe A was changed, any display port outputs on pipe B would get disabled as intel_disable_pch_ports would ensure that the mode setting operation could occur on pipe A without interference from other outputs connected to that pch port
Signed-off-by: Keith Packard keithp@keithp.com --- drivers/gpu/drm/i915/i915_reg.h | 3 -- drivers/gpu/drm/i915/intel_display.c | 45 +++++++++++++++++++++++++-------- 2 files changed, 34 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 5d5def7..f731565 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -2083,9 +2083,6 @@ #define DP_PIPEB_SELECT (1 << 30) #define DP_PIPE_MASK (1 << 30)
-#define DP_PIPE_ENABLED(V, P) \ - (((V) & (DP_PIPE_MASK | DP_PORT_EN)) == ((P) << 30 | DP_PORT_EN)) - /* Link training mode - select a suitable mode for each stage */ #define DP_LINK_TRAIN_PAT_1 (0 << 28) #define DP_LINK_TRAIN_PAT_2 (1 << 28) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 5609c06..fc26cb4 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -979,11 +979,29 @@ static void assert_transcoder_disabled(struct drm_i915_private *dev_priv, pipe_name(pipe)); }
+static bool dp_pipe_enabled(struct drm_i915_private *dev_priv, enum pipe pipe, + int reg, u32 port_sel, u32 val) +{ + if ((val & DP_PORT_EN) == 0) + return false; + + if (HAS_PCH_CPT(dev_priv->dev)) { + u32 trans_dp_ctl_reg = TRANS_DP_CTL(pipe); + u32 trans_dp_ctl = I915_READ(trans_dp_ctl_reg); + if ((trans_dp_ctl & TRANS_DP_PORT_SEL_MASK) != port_sel) + return false; + } else { + if ((val & DP_PIPE_MASK) != (pipe << 30)) + return false; + } + return true; +} + static void assert_pch_dp_disabled(struct drm_i915_private *dev_priv, - enum pipe pipe, int reg) + enum pipe pipe, int reg, u32 port_sel) { u32 val = I915_READ(reg); - WARN(DP_PIPE_ENABLED(val, pipe), + WARN(dp_pipe_enabled(dev_priv, pipe, reg, port_sel, val), "PCH DP (0x%08x) enabled on transcoder %c, should be disabled\n", reg, pipe_name(pipe)); } @@ -1003,9 +1021,9 @@ static void assert_pch_ports_disabled(struct drm_i915_private *dev_priv, int reg; u32 val;
- assert_pch_dp_disabled(dev_priv, pipe, PCH_DP_B); - assert_pch_dp_disabled(dev_priv, pipe, PCH_DP_C); - assert_pch_dp_disabled(dev_priv, pipe, PCH_DP_D); + assert_pch_dp_disabled(dev_priv, pipe, PCH_DP_B, TRANS_DP_PORT_SEL_B); + assert_pch_dp_disabled(dev_priv, pipe, PCH_DP_C, TRANS_DP_PORT_SEL_C); + assert_pch_dp_disabled(dev_priv, pipe, PCH_DP_D, TRANS_DP_PORT_SEL_D);
reg = PCH_ADPA; val = I915_READ(reg); @@ -1334,19 +1352,24 @@ static void intel_disable_plane(struct drm_i915_private *dev_priv, }
static void disable_pch_dp(struct drm_i915_private *dev_priv, - enum pipe pipe, int reg) + enum pipe pipe, int reg, u32 port_sel) { u32 val = I915_READ(reg); - if (DP_PIPE_ENABLED(val, pipe)) + if (dp_pipe_enabled(dev_priv, pipe, reg, port_sel, val)) { + DRM_DEBUG_KMS("Disabling pch dp %x on pipe %d\n", reg, pipe); I915_WRITE(reg, val & ~DP_PORT_EN); + } }
static void disable_pch_hdmi(struct drm_i915_private *dev_priv, enum pipe pipe, int reg) { u32 val = I915_READ(reg); - if (HDMI_PIPE_ENABLED(val, pipe)) + if (HDMI_PIPE_ENABLED(val, pipe)) { + DRM_DEBUG_KMS("Disabling pch HDMI %x on pipe %d\n", + reg, pipe); I915_WRITE(reg, val & ~PORT_ENABLE); + } }
/* Disable any ports connected to this transcoder */ @@ -1358,9 +1381,9 @@ static void intel_disable_pch_ports(struct drm_i915_private *dev_priv, val = I915_READ(PCH_PP_CONTROL); I915_WRITE(PCH_PP_CONTROL, val | PANEL_UNLOCK_REGS);
- disable_pch_dp(dev_priv, pipe, PCH_DP_B); - disable_pch_dp(dev_priv, pipe, PCH_DP_C); - disable_pch_dp(dev_priv, pipe, PCH_DP_D); + disable_pch_dp(dev_priv, pipe, PCH_DP_B, TRANS_DP_PORT_SEL_B); + disable_pch_dp(dev_priv, pipe, PCH_DP_C, TRANS_DP_PORT_SEL_C); + disable_pch_dp(dev_priv, pipe, PCH_DP_D, TRANS_DP_PORT_SEL_D);
reg = PCH_ADPA; val = I915_READ(reg);
On Mon, 25 Jul 2011 23:36:34 -0700 Keith Packard keithp@keithp.com wrote:
Display port pipe selection on CPT is not done with a bit in the output register, rather it is controlled by a couple of bits in the separate transcoder register which indicate which display port output is connected to the transcoder.
This patch replaces the simplistic macro DP_PIPE_ENABLED with the rather more complicated function dp_pipe_enabled which checks the output register to see if that is enabled, and then goes on to either check the output register pipe selection bit (on non-CPT) or the transcoder DP selection bits (on CPT).
Before this patch, any time the mode of pipe A was changed, any display port outputs on pipe B would get disabled as intel_disable_pch_ports would ensure that the mode setting operation could occur on pipe A without interference from other outputs connected to that pch port
Signed-off-by: Keith Packard keithp@keithp.com
Ah nice catch. I expect one day we'll have all the chipset and PCH differences coded...
Reviewed-by: Jesse Barnes jbarnes@virtuousgeek.org
On Mon, 2011-07-25 at 23:36 -0700, Keith Packard wrote:
[PATCH 1/5] drm/i915: Use dp_detect_common in hotplug helper [PATCH 2/5] drm/i915: Rename i915_dp_detect_common to [PATCH 3/5] drm/i915: In intel_dp_init, replace read of DPCD with
These three are simple cleanups to centralize all places where the DPCD block was read from the device. Now everyone shares the same function, and that function retries the reads.
Reviewed-by: Adam Jackson ajax@redhat.com
[PATCH 4/5] drm/i915: Delay 250ms before running the hotplug code
I was experimenting with DP hotplugging -- moving the plug in and out of the jack very slowly and discovered that the hotplug interrupt occurred well before or after the link for the aux data channel was connected or disconnected. The result of this was that a sufficiently rapid cycle back through user mode could easily beat the motion of the plug and cause the hotplug detection to get the wrong status. Sticking a 250ms delay before doing anything gives the user sufficient time to actually get the plug connected or disconnected.
At the very least this should instead be queue_delayed_work(). But if we're going to delay, can we instead set up PCH_PORT_HOTPLUG to do a 100ms delay for us? I'll try this locally, at any rate.
[PATCH 5/5] drm/i915: DP_PIPE_ENABLED must check transcoder on CPT
<snip>
Turns out the bug wasn't that the mode setting code was doing it wrong and turning the DP2 output off intentionally as a part of the mode change. Instead, the intel driver was trying to adjust the PCH link for the LVDS1 output and thought it needed to turn the DP2 output off because it mistakenly believed the DP2 output was sharing the same pipe as the LVDS1 output. Just a matter of using the wrong mechanism to detect which pipe the DP2 output was connected to.
Reviewed-by: Adam Jackson ajax@redhat.com
- ajax
dri-devel@lists.freedesktop.org
-
Adam Jackson -
Daniel Vetter -
Jesse Barnes -
Keith Packard