| Message ID | 1464766070-31623-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 01, 2016 at 08:27:50AM +0100, Chris Wilson wrote: > If the VBT says that a certain port should be eDP (and hence fused off > from HDMI), but in reality it isn't, we need to try and acquire the HDMI > connection instead. So only trust the VBT edp setting if we can connect > to an eDP device on that port. > > Fixes: d2182a6608 (drm/i915: Don't register HDMI connectors for eDP ports on VLV/CHV) > References: https://bugs.freedesktop.org/show_bug.cgi?id=96288 > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Tested-by: Phidias Chiang <phidias.chiang@canonical.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++---------- > drivers/gpu/drm/i915/intel_dp.c | 13 ++++++------- > drivers/gpu/drm/i915/intel_drv.h | 2 +- > 3 files changed, 17 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 82796d1a87d7..0be8cede60e3 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -14642,6 +14642,8 @@ static void intel_setup_outputs(struct drm_device *dev) > if (I915_READ(PCH_DP_D) & DP_DETECTED) > intel_dp_init(dev, PCH_DP_D, PORT_D); > } else if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) { > + bool has_edp; > + > /* > * The DP_DETECTED bit is the latched state of the DDC > * SDA pin at boot. However since eDP doesn't require DDC > @@ -14651,19 +14653,17 @@ static void intel_setup_outputs(struct drm_device *dev) > * eDP ports. Consult the VBT as well as DP_DETECTED to > * detect eDP ports. > */ > - if (I915_READ(VLV_HDMIB) & SDVO_DETECTED && > - !intel_dp_is_edp(dev, PORT_B)) > + has_edp = intel_dp_is_edp(dev, PORT_B); > + if (I915_READ(VLV_DP_B) & DP_DETECTED || has_edp) > + has_edp &= intel_dp_init(dev, VLV_DP_B, PORT_B); > + if (I915_READ(VLV_HDMIB) & SDVO_DETECTED && !has_edp) > intel_hdmi_init(dev, VLV_HDMIB, PORT_B); > - if (I915_READ(VLV_DP_B) & DP_DETECTED || > - intel_dp_is_edp(dev, PORT_B)) > - intel_dp_init(dev, VLV_DP_B, PORT_B); This will change the order in which the connectors/encoders are registered. Shouldn't really matter. We use the DP,HDMI order on some other platforms as well. I wonder if we should change everything to use that order for consistency? Also if the eDP init fails we'll not register a regular DP port, but that's sort of OK since our eDP init doesn't actually check if the sink is really eDP. Instead it just checks that DPCD can be read. So if we would have a board with a normal user accesible DP connector which the VBT claims to be eDP, we'd run into some hilarity when the user boots w/o a display connected, yanks the cable after boot, or hotplugs in another display. But I'm thinking we can just ignore that scenario until we encounter such a board. I'm happy enough with this as is, so Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > - if (I915_READ(VLV_HDMIC) & SDVO_DETECTED && > - !intel_dp_is_edp(dev, PORT_C)) > + has_edp = intel_dp_is_edp(dev, PORT_C); > + if (I915_READ(VLV_DP_C) & DP_DETECTED || has_edp) > + has_edp &= intel_dp_init(dev, VLV_DP_C, PORT_C); > + if (I915_READ(VLV_HDMIC) & SDVO_DETECTED && !has_edp) > intel_hdmi_init(dev, VLV_HDMIC, PORT_C); > - if (I915_READ(VLV_DP_C) & DP_DETECTED || > - intel_dp_is_edp(dev, PORT_C)) > - intel_dp_init(dev, VLV_DP_C, PORT_C); > > if (IS_CHERRYVIEW(dev)) { > /* eDP not supported on port D, so don't check VBT */ > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 1d3824721c0a..d043b1384345 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -5565,9 +5565,9 @@ fail: > return false; > } > > -void > -intel_dp_init(struct drm_device *dev, > - i915_reg_t output_reg, enum port port) > +bool intel_dp_init(struct drm_device *dev, > + i915_reg_t output_reg, > + enum port port) > { > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_digital_port *intel_dig_port; > @@ -5577,7 +5577,7 @@ intel_dp_init(struct drm_device *dev, > > intel_dig_port = kzalloc(sizeof(*intel_dig_port), GFP_KERNEL); > if (!intel_dig_port) > - return; > + return false; > > intel_connector = intel_connector_alloc(); > if (!intel_connector) > @@ -5634,7 +5634,7 @@ intel_dp_init(struct drm_device *dev, > if (!intel_dp_init_connector(intel_dig_port, intel_connector)) > goto err_init_connector; > > - return; > + return true; > > err_init_connector: > drm_encoder_cleanup(encoder); > @@ -5642,8 +5642,7 @@ err_encoder_init: > kfree(intel_connector); > err_connector_alloc: > kfree(intel_dig_port); > - > - return; > + return false; > } > > void intel_dp_mst_suspend(struct drm_device *dev) > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index bef4ee90ca67..cb9dc596fb7e 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1328,7 +1328,7 @@ void intel_csr_ucode_suspend(struct drm_i915_private *); > void intel_csr_ucode_resume(struct drm_i915_private *); > > /* intel_dp.c */ > -void intel_dp_init(struct drm_device *dev, i915_reg_t output_reg, enum port port); > +bool intel_dp_init(struct drm_device *dev, i915_reg_t output_reg, enum port port); > bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port, > struct intel_connector *intel_connector); > void intel_dp_set_link_params(struct intel_dp *intel_dp, > -- > 2.8.1
On Wed, Jun 01, 2016 at 11:20:04AM +0300, Ville Syrjälä wrote: > On Wed, Jun 01, 2016 at 08:27:50AM +0100, Chris Wilson wrote: > > If the VBT says that a certain port should be eDP (and hence fused off > > from HDMI), but in reality it isn't, we need to try and acquire the HDMI > > connection instead. So only trust the VBT edp setting if we can connect > > to an eDP device on that port. > > > > Fixes: d2182a6608 (drm/i915: Don't register HDMI connectors for eDP ports on VLV/CHV) > > References: https://bugs.freedesktop.org/show_bug.cgi?id=96288 > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Tested-by: Phidias Chiang <phidias.chiang@canonical.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > > --- > > drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++---------- > > drivers/gpu/drm/i915/intel_dp.c | 13 ++++++------- > > drivers/gpu/drm/i915/intel_drv.h | 2 +- > > 3 files changed, 17 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 82796d1a87d7..0be8cede60e3 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -14642,6 +14642,8 @@ static void intel_setup_outputs(struct drm_device *dev) > > if (I915_READ(PCH_DP_D) & DP_DETECTED) > > intel_dp_init(dev, PCH_DP_D, PORT_D); > > } else if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) { > > + bool has_edp; > > + > > /* > > * The DP_DETECTED bit is the latched state of the DDC > > * SDA pin at boot. However since eDP doesn't require DDC > > @@ -14651,19 +14653,17 @@ static void intel_setup_outputs(struct drm_device *dev) > > * eDP ports. Consult the VBT as well as DP_DETECTED to > > * detect eDP ports. > > */ > > - if (I915_READ(VLV_HDMIB) & SDVO_DETECTED && > > - !intel_dp_is_edp(dev, PORT_B)) > > + has_edp = intel_dp_is_edp(dev, PORT_B); > > + if (I915_READ(VLV_DP_B) & DP_DETECTED || has_edp) > > + has_edp &= intel_dp_init(dev, VLV_DP_B, PORT_B); > > + if (I915_READ(VLV_HDMIB) & SDVO_DETECTED && !has_edp) > > intel_hdmi_init(dev, VLV_HDMIB, PORT_B); > > - if (I915_READ(VLV_DP_B) & DP_DETECTED || > > - intel_dp_is_edp(dev, PORT_B)) > > - intel_dp_init(dev, VLV_DP_B, PORT_B); > > This will change the order in which the connectors/encoders are > registered. Shouldn't really matter. We use the DP,HDMI order on > some other platforms as well. I wonder if we should change everything > to use that order for consistency? I'm tempted to, just to solve the same problem of false eDP disabling valid outputs on other platforms, e.g. ivb. However, changing the order feels like a "it can't possibly break anything, but does!" situation, so gradual is best. Certainly, on the older sdvo chipsets, the order is more constrained, but on the ddi platforms where hdmi/dp are interchangeable, it should be flexible. > Also if the eDP init fails we'll not register a regular DP port, but > that's sort of OK since our eDP init doesn't actually check if the sink > is really eDP. Instead it just checks that DPCD can be read. So if we > would have a board with a normal user accesible DP connector which the > VBT claims to be eDP, we'd run into some hilarity when the user boots > w/o a display connected, yanks the cable after boot, or hotplugs in > another display. But I'm thinking we can just ignore that scenario > until we encounter such a board. Right. That we could probably resolve just by dropping the dp->is_edp flag if intel_edp_init_connector fails and just leaving the connector established for DP - but only if it is reported as detected (rather than the VBT override). Then we would change the HDMI setup to only ignore known eDP links (for which we would need a stronger is-eDP verification). The challenge is "who would this regress"? The older bug is https://bugs.freedesktop.org/show_bug.cgi?id=88331 if anyone feels inspired on handling HDMI vs eDP vs DP more consistently between platforms (if that is even possible!). -Chris
On Wed, Jun 01, 2016 at 11:20:04AM +0300, Ville Syrjälä wrote: > On Wed, Jun 01, 2016 at 08:27:50AM +0100, Chris Wilson wrote: > > If the VBT says that a certain port should be eDP (and hence fused off > > from HDMI), but in reality it isn't, we need to try and acquire the HDMI > > connection instead. So only trust the VBT edp setting if we can connect > > to an eDP device on that port. > > > > Fixes: d2182a6608 (drm/i915: Don't register HDMI connectors for eDP ports on VLV/CHV) > > References: https://bugs.freedesktop.org/show_bug.cgi?id=96288 > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Tested-by: Phidias Chiang <phidias.chiang@canonical.com> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> [snip] > I'm happy enough with this as is, so > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> The only way to get enough test coverage to see if this regresses is to push and wait 12-24 months. So here goes... -Chris
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 82796d1a87d7..0be8cede60e3 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14642,6 +14642,8 @@ static void intel_setup_outputs(struct drm_device *dev) if (I915_READ(PCH_DP_D) & DP_DETECTED) intel_dp_init(dev, PCH_DP_D, PORT_D); } else if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) { + bool has_edp; + /* * The DP_DETECTED bit is the latched state of the DDC * SDA pin at boot. However since eDP doesn't require DDC @@ -14651,19 +14653,17 @@ static void intel_setup_outputs(struct drm_device *dev) * eDP ports. Consult the VBT as well as DP_DETECTED to * detect eDP ports. */ - if (I915_READ(VLV_HDMIB) & SDVO_DETECTED && - !intel_dp_is_edp(dev, PORT_B)) + has_edp = intel_dp_is_edp(dev, PORT_B); + if (I915_READ(VLV_DP_B) & DP_DETECTED || has_edp) + has_edp &= intel_dp_init(dev, VLV_DP_B, PORT_B); + if (I915_READ(VLV_HDMIB) & SDVO_DETECTED && !has_edp) intel_hdmi_init(dev, VLV_HDMIB, PORT_B); - if (I915_READ(VLV_DP_B) & DP_DETECTED || - intel_dp_is_edp(dev, PORT_B)) - intel_dp_init(dev, VLV_DP_B, PORT_B); - if (I915_READ(VLV_HDMIC) & SDVO_DETECTED && - !intel_dp_is_edp(dev, PORT_C)) + has_edp = intel_dp_is_edp(dev, PORT_C); + if (I915_READ(VLV_DP_C) & DP_DETECTED || has_edp) + has_edp &= intel_dp_init(dev, VLV_DP_C, PORT_C); + if (I915_READ(VLV_HDMIC) & SDVO_DETECTED && !has_edp) intel_hdmi_init(dev, VLV_HDMIC, PORT_C); - if (I915_READ(VLV_DP_C) & DP_DETECTED || - intel_dp_is_edp(dev, PORT_C)) - intel_dp_init(dev, VLV_DP_C, PORT_C); if (IS_CHERRYVIEW(dev)) { /* eDP not supported on port D, so don't check VBT */ diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 1d3824721c0a..d043b1384345 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -5565,9 +5565,9 @@ fail: return false; } -void -intel_dp_init(struct drm_device *dev, - i915_reg_t output_reg, enum port port) +bool intel_dp_init(struct drm_device *dev, + i915_reg_t output_reg, + enum port port) { struct drm_i915_private *dev_priv = dev->dev_private; struct intel_digital_port *intel_dig_port; @@ -5577,7 +5577,7 @@ intel_dp_init(struct drm_device *dev, intel_dig_port = kzalloc(sizeof(*intel_dig_port), GFP_KERNEL); if (!intel_dig_port) - return; + return false; intel_connector = intel_connector_alloc(); if (!intel_connector) @@ -5634,7 +5634,7 @@ intel_dp_init(struct drm_device *dev, if (!intel_dp_init_connector(intel_dig_port, intel_connector)) goto err_init_connector; - return; + return true; err_init_connector: drm_encoder_cleanup(encoder); @@ -5642,8 +5642,7 @@ err_encoder_init: kfree(intel_connector); err_connector_alloc: kfree(intel_dig_port); - - return; + return false; } void intel_dp_mst_suspend(struct drm_device *dev) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index bef4ee90ca67..cb9dc596fb7e 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1328,7 +1328,7 @@ void intel_csr_ucode_suspend(struct drm_i915_private *); void intel_csr_ucode_resume(struct drm_i915_private *); /* intel_dp.c */ -void intel_dp_init(struct drm_device *dev, i915_reg_t output_reg, enum port port); +bool intel_dp_init(struct drm_device *dev, i915_reg_t output_reg, enum port port); bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port, struct intel_connector *intel_connector); void intel_dp_set_link_params(struct intel_dp *intel_dp,