| Message ID | 20190710221500.11576-2-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4,1/2] drm/i915: Add support for retrying hotplug | expand |
On Wed, Jul 10, 2019 at 03:15:00PM -0700, José Roberto de Souza wrote: > Right now we are aware of two cases that needs another hotplug retry: > - Unpowered type-c dongles > - HDMI slow unplug > > Both have a complete explanation in the code to schedule another run > of the hotplug handler. > > It could have more checks to just trigger the retry in those two > specific cases but why would sink signal a long pulse if there is > no change? Also the drawback of running the hotplug handler again > is really low and that could fix another cases that we are not > aware. > > Also retrying for old DP ports(non-DDI) to make it consistent and not > cause CI failures if those systems are connected to chamelium boards > that will be used to simulate the issues reported in here. > > v2: Also retrying for old DP ports(non-DDI)(Imre) > > v4: Renamed INTEL_HOTPLUG_NOCHANGE to INTEL_HOTPLUG_UNCHANGED to keep > it consistent(Rodrigo) > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Imre Deak <imre.deak@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Reviewed-by: Imre Deak <imre.deak@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/display/intel_ddi.c | 21 +++++++++++++++++ > drivers/gpu/drm/i915/display/intel_dp.c | 7 ++++++ > drivers/gpu/drm/i915/display/intel_hdmi.c | 28 ++++++++++++++++++++++- > 3 files changed, 55 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > index 734c004800f8..ea6d1873f6cb 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > @@ -4052,6 +4052,7 @@ intel_ddi_hotplug(struct intel_encoder *encoder, > struct intel_connector *connector, > bool irq_received) > { > + struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); > struct drm_modeset_acquire_ctx ctx; > enum intel_hotplug_state state; > int ret; > @@ -4078,6 +4079,26 @@ intel_ddi_hotplug(struct intel_encoder *encoder, > drm_modeset_acquire_fini(&ctx); > WARN(ret, "Acquiring modeset locks failed with %i\n", ret); > > + /* > + * Unpowered type-c dongles can take some time to boot and be > + * responsible, so here giving some time to those dongles to power up > + * and then retrying the probe. > + * > + * On many platforms the HDMI live state signal is known to be > + * unreliable, so we can't use it to detect if a sink is connected or > + * not. Instead we detect if it's connected based on whether we can > + * read the EDID or not. That in turn has a problem during disconnect, > + * since the HPD interrupt may be raised before the DDC lines get > + * disconnected (due to how the required length of DDC vs. HPD > + * connector pins are specified) and so we'll still be able to get a > + * valid EDID. To solve this schedule another detection cycle if this > + * time around we didn't detect any change in the sink's connection > + * status. > + */ > + if (state == INTEL_HOTPLUG_UNCHANGED && irq_received && > + !dig_port->dp.is_mst) > + state = INTEL_HOTPLUG_RETRY; > + > return state; > } > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index 4423abbc7907..7106a2d80f79 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -4881,6 +4881,13 @@ intel_dp_hotplug(struct intel_encoder *encoder, > drm_modeset_acquire_fini(&ctx); > WARN(ret, "Acquiring modeset locks failed with %i\n", ret); > > + /* > + * Keeping it consistent with intel_ddi_hotplug() and > + * intel_hdmi_hotplug(). > + */ > + if (state == INTEL_HOTPLUG_UNCHANGED && irq_received) > + state = INTEL_HOTPLUG_RETRY; > + > return state; > } > > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c > index 0ebec69bbbfc..26c8556f6980 100644 > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c > @@ -3143,6 +3143,32 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port, > DRM_DEBUG_KMS("CEC notifier get failed\n"); > } > > +static enum intel_hotplug_state > +intel_hdmi_hotplug(struct intel_encoder *encoder, > + struct intel_connector *connector, bool irq_received) > +{ > + enum intel_hotplug_state state; > + > + state = intel_encoder_hotplug(encoder, connector, irq_received); > + > + /* > + * On many platforms the HDMI live state signal is known to be > + * unreliable, so we can't use it to detect if a sink is connected or > + * not. Instead we detect if it's connected based on whether we can > + * read the EDID or not. That in turn has a problem during disconnect, > + * since the HPD interrupt may be raised before the DDC lines get > + * disconnected (due to how the required length of DDC vs. HPD > + * connector pins are specified) and so we'll still be able to get a > + * valid EDID. To solve this schedule another detection cycle if this > + * time around we didn't detect any change in the sink's connection > + * status. > + */ > + if (state == INTEL_HOTPLUG_UNCHANGED && irq_received) > + state = INTEL_HOTPLUG_RETRY; > + > + return state; > +} > + > void intel_hdmi_init(struct drm_i915_private *dev_priv, > i915_reg_t hdmi_reg, enum port port) > { > @@ -3166,7 +3192,7 @@ void intel_hdmi_init(struct drm_i915_private *dev_priv, > &intel_hdmi_enc_funcs, DRM_MODE_ENCODER_TMDS, > "HDMI %c", port_name(port)); > > - intel_encoder->hotplug = intel_encoder_hotplug; > + intel_encoder->hotplug = intel_hdmi_hotplug; > intel_encoder->compute_config = intel_hdmi_compute_config; > if (HAS_PCH_SPLIT(dev_priv)) { > intel_encoder->disable = pch_disable_hdmi; > -- > 2.22.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Jul 11, 2019 at 12:49:35PM -0700, Nathan Ciobanu wrote: > On Wed, Jul 10, 2019 at 03:15:00PM -0700, José Roberto de Souza wrote: > > Right now we are aware of two cases that needs another hotplug retry: > > - Unpowered type-c dongles > > - HDMI slow unplug > > > > Both have a complete explanation in the code to schedule another run > > of the hotplug handler. > > > > It could have more checks to just trigger the retry in those two > > specific cases but why would sink signal a long pulse if there is > > no change? Also the drawback of running the hotplug handler again > > is really low and that could fix another cases that we are not > > aware. > > > > Also retrying for old DP ports(non-DDI) to make it consistent and not > > cause CI failures if those systems are connected to chamelium boards > > that will be used to simulate the issues reported in here. > > > > v2: Also retrying for old DP ports(non-DDI)(Imre) > > > > v4: Renamed INTEL_HOTPLUG_NOCHANGE to INTEL_HOTPLUG_UNCHANGED to keep > > it consistent(Rodrigo) > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Imre Deak <imre.deak@intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> Tested-by: Shane McKee <shane.mckee@intel.com> > > Reviewed-by: Imre Deak <imre.deak@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_ddi.c | 21 +++++++++++++++++ > > drivers/gpu/drm/i915/display/intel_dp.c | 7 ++++++ > > drivers/gpu/drm/i915/display/intel_hdmi.c | 28 ++++++++++++++++++++++- > > 3 files changed, 55 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > > index 734c004800f8..ea6d1873f6cb 100644 > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > @@ -4052,6 +4052,7 @@ intel_ddi_hotplug(struct intel_encoder *encoder, > > struct intel_connector *connector, > > bool irq_received) > > { > > + struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); > > struct drm_modeset_acquire_ctx ctx; > > enum intel_hotplug_state state; > > int ret; > > @@ -4078,6 +4079,26 @@ intel_ddi_hotplug(struct intel_encoder *encoder, > > drm_modeset_acquire_fini(&ctx); > > WARN(ret, "Acquiring modeset locks failed with %i\n", ret); > > > > + /* > > + * Unpowered type-c dongles can take some time to boot and be > > + * responsible, so here giving some time to those dongles to power up > > + * and then retrying the probe. > > + * > > + * On many platforms the HDMI live state signal is known to be > > + * unreliable, so we can't use it to detect if a sink is connected or > > + * not. Instead we detect if it's connected based on whether we can > > + * read the EDID or not. That in turn has a problem during disconnect, > > + * since the HPD interrupt may be raised before the DDC lines get > > + * disconnected (due to how the required length of DDC vs. HPD > > + * connector pins are specified) and so we'll still be able to get a > > + * valid EDID. To solve this schedule another detection cycle if this > > + * time around we didn't detect any change in the sink's connection > > + * status. > > + */ > > + if (state == INTEL_HOTPLUG_UNCHANGED && irq_received && > > + !dig_port->dp.is_mst) > > + state = INTEL_HOTPLUG_RETRY; > > + > > return state; > > } > > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > > index 4423abbc7907..7106a2d80f79 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > > @@ -4881,6 +4881,13 @@ intel_dp_hotplug(struct intel_encoder *encoder, > > drm_modeset_acquire_fini(&ctx); > > WARN(ret, "Acquiring modeset locks failed with %i\n", ret); > > > > + /* > > + * Keeping it consistent with intel_ddi_hotplug() and > > + * intel_hdmi_hotplug(). > > + */ > > + if (state == INTEL_HOTPLUG_UNCHANGED && irq_received) > > + state = INTEL_HOTPLUG_RETRY; > > + > > return state; > > } > > > > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c > > index 0ebec69bbbfc..26c8556f6980 100644 > > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c > > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c > > @@ -3143,6 +3143,32 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port, > > DRM_DEBUG_KMS("CEC notifier get failed\n"); > > } > > > > +static enum intel_hotplug_state > > +intel_hdmi_hotplug(struct intel_encoder *encoder, > > + struct intel_connector *connector, bool irq_received) > > +{ > > + enum intel_hotplug_state state; > > + > > + state = intel_encoder_hotplug(encoder, connector, irq_received); > > + > > + /* > > + * On many platforms the HDMI live state signal is known to be > > + * unreliable, so we can't use it to detect if a sink is connected or > > + * not. Instead we detect if it's connected based on whether we can > > + * read the EDID or not. That in turn has a problem during disconnect, > > + * since the HPD interrupt may be raised before the DDC lines get > > + * disconnected (due to how the required length of DDC vs. HPD > > + * connector pins are specified) and so we'll still be able to get a > > + * valid EDID. To solve this schedule another detection cycle if this > > + * time around we didn't detect any change in the sink's connection > > + * status. > > + */ > > + if (state == INTEL_HOTPLUG_UNCHANGED && irq_received) > > + state = INTEL_HOTPLUG_RETRY; > > + > > + return state; > > +} > > + > > void intel_hdmi_init(struct drm_i915_private *dev_priv, > > i915_reg_t hdmi_reg, enum port port) > > { > > @@ -3166,7 +3192,7 @@ void intel_hdmi_init(struct drm_i915_private *dev_priv, > > &intel_hdmi_enc_funcs, DRM_MODE_ENCODER_TMDS, > > "HDMI %c", port_name(port)); > > > > - intel_encoder->hotplug = intel_encoder_hotplug; > > + intel_encoder->hotplug = intel_hdmi_hotplug; > > intel_encoder->compute_config = intel_hdmi_compute_config; > > if (HAS_PCH_SPLIT(dev_priv)) { > > intel_encoder->disable = pch_disable_hdmi; > > -- > > 2.22.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index 734c004800f8..ea6d1873f6cb 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -4052,6 +4052,7 @@ intel_ddi_hotplug(struct intel_encoder *encoder, struct intel_connector *connector, bool irq_received) { + struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); struct drm_modeset_acquire_ctx ctx; enum intel_hotplug_state state; int ret; @@ -4078,6 +4079,26 @@ intel_ddi_hotplug(struct intel_encoder *encoder, drm_modeset_acquire_fini(&ctx); WARN(ret, "Acquiring modeset locks failed with %i\n", ret); + /* + * Unpowered type-c dongles can take some time to boot and be + * responsible, so here giving some time to those dongles to power up + * and then retrying the probe. + * + * On many platforms the HDMI live state signal is known to be + * unreliable, so we can't use it to detect if a sink is connected or + * not. Instead we detect if it's connected based on whether we can + * read the EDID or not. That in turn has a problem during disconnect, + * since the HPD interrupt may be raised before the DDC lines get + * disconnected (due to how the required length of DDC vs. HPD + * connector pins are specified) and so we'll still be able to get a + * valid EDID. To solve this schedule another detection cycle if this + * time around we didn't detect any change in the sink's connection + * status. + */ + if (state == INTEL_HOTPLUG_UNCHANGED && irq_received && + !dig_port->dp.is_mst) + state = INTEL_HOTPLUG_RETRY; + return state; } diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 4423abbc7907..7106a2d80f79 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -4881,6 +4881,13 @@ intel_dp_hotplug(struct intel_encoder *encoder, drm_modeset_acquire_fini(&ctx); WARN(ret, "Acquiring modeset locks failed with %i\n", ret); + /* + * Keeping it consistent with intel_ddi_hotplug() and + * intel_hdmi_hotplug(). + */ + if (state == INTEL_HOTPLUG_UNCHANGED && irq_received) + state = INTEL_HOTPLUG_RETRY; + return state; } diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index 0ebec69bbbfc..26c8556f6980 100644 --- a/drivers/gpu/drm/i915/display/intel_hdmi.c +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c @@ -3143,6 +3143,32 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port, DRM_DEBUG_KMS("CEC notifier get failed\n"); } +static enum intel_hotplug_state +intel_hdmi_hotplug(struct intel_encoder *encoder, + struct intel_connector *connector, bool irq_received) +{ + enum intel_hotplug_state state; + + state = intel_encoder_hotplug(encoder, connector, irq_received); + + /* + * On many platforms the HDMI live state signal is known to be + * unreliable, so we can't use it to detect if a sink is connected or + * not. Instead we detect if it's connected based on whether we can + * read the EDID or not. That in turn has a problem during disconnect, + * since the HPD interrupt may be raised before the DDC lines get + * disconnected (due to how the required length of DDC vs. HPD + * connector pins are specified) and so we'll still be able to get a + * valid EDID. To solve this schedule another detection cycle if this + * time around we didn't detect any change in the sink's connection + * status. + */ + if (state == INTEL_HOTPLUG_UNCHANGED && irq_received) + state = INTEL_HOTPLUG_RETRY; + + return state; +} + void intel_hdmi_init(struct drm_i915_private *dev_priv, i915_reg_t hdmi_reg, enum port port) { @@ -3166,7 +3192,7 @@ void intel_hdmi_init(struct drm_i915_private *dev_priv, &intel_hdmi_enc_funcs, DRM_MODE_ENCODER_TMDS, "HDMI %c", port_name(port)); - intel_encoder->hotplug = intel_encoder_hotplug; + intel_encoder->hotplug = intel_hdmi_hotplug; intel_encoder->compute_config = intel_hdmi_compute_config; if (HAS_PCH_SPLIT(dev_priv)) { intel_encoder->disable = pch_disable_hdmi;