Message ID | 20200330095425.29113-2-imre.deak@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [1/2] drm/i915: Add a retry counter for hotplug detect retries | expand |
On Mon, 2020-03-30 at 12:54 +0300, Imre Deak wrote: > On TypeC ports if a sink deasserts/reasserts its HPD signal, > generating > a hotplug interrupt without the sink getting unplugged/replugged from > the connector, there can be an up to 3 seconds delay until the AUX > channel gets functional. To avoid detection failures this delay > causes > retry the detection for 5 seconds. 5 seconds? would it be 5 tries? Other than that both patches looks good. Reviewed-by: José Roberto de Souza <jose.souza@intel.com> > > I noticed this on ICL/TGL RVPs and a DELL XPS 13 7390 ICL laptop. > > References: https://gitlab.freedesktop.org/drm/intel/issues/1067 > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/display/intel_ddi.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > b/drivers/gpu/drm/i915/display/intel_ddi.c > index 4f508bf70f3b..2d947ff83488 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > @@ -4371,7 +4371,10 @@ static enum intel_hotplug_state > intel_ddi_hotplug(struct intel_encoder *encoder, > struct intel_connector *connector) > { > + struct drm_i915_private *i915 = to_i915(encoder->base.dev); > struct intel_digital_port *dig_port = enc_to_dig_port(encoder); > + enum phy phy = intel_port_to_phy(i915, encoder->port); > + bool is_tc = intel_phy_is_tc(i915, phy); > struct drm_modeset_acquire_ctx ctx; > enum intel_hotplug_state state; > int ret; > @@ -4414,8 +4417,15 @@ intel_ddi_hotplug(struct intel_encoder > *encoder, > * 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. > + * > + * Type-c connectors which get their HPD signal deasserted then > + * reasserted, without unplugging/replugging the sink from the > + * connector, introduce a delay until the AUX channel > communication > + * becomes functional. Retry the detection for 5 seconds on > type-c > + * connectors to account for this delay. > */ > - if (state == INTEL_HOTPLUG_UNCHANGED && !connector- > >hotplug_retries && > + if (state == INTEL_HOTPLUG_UNCHANGED && > + connector->hotplug_retries < (is_tc ? 5 : 1) && > !dig_port->dp.is_mst) > state = INTEL_HOTPLUG_RETRY; >
On 2020-03-30 at 15:24:25 +0530, Imre Deak wrote: > On TypeC ports if a sink deasserts/reasserts its HPD signal, generating > a hotplug interrupt without the sink getting unplugged/replugged from > the connector, there can be an up to 3 seconds delay until the AUX > channel gets functional. To avoid detection failures this delay causes > retry the detection for 5 seconds. > > I noticed this on ICL/TGL RVPs and a DELL XPS 13 7390 ICL laptop. > > References: https://gitlab.freedesktop.org/drm/intel/issues/1067 > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/display/intel_ddi.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > index 4f508bf70f3b..2d947ff83488 100644 > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > @@ -4371,7 +4371,10 @@ static enum intel_hotplug_state > intel_ddi_hotplug(struct intel_encoder *encoder, > struct intel_connector *connector) > { > + struct drm_i915_private *i915 = to_i915(encoder->base.dev); > struct intel_digital_port *dig_port = enc_to_dig_port(encoder); > + enum phy phy = intel_port_to_phy(i915, encoder->port); > + bool is_tc = intel_phy_is_tc(i915, phy); > struct drm_modeset_acquire_ctx ctx; > enum intel_hotplug_state state; > int ret; > @@ -4414,8 +4417,15 @@ intel_ddi_hotplug(struct intel_encoder *encoder, > * 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. > + * > + * Type-c connectors which get their HPD signal deasserted then > + * reasserted, without unplugging/replugging the sink from the > + * connector, introduce a delay until the AUX channel communication > + * becomes functional. Retry the detection for 5 seconds on type-c > + * connectors to account for this delay. > */ > - if (state == INTEL_HOTPLUG_UNCHANGED && !connector->hotplug_retries && > + if (state == INTEL_HOTPLUG_UNCHANGED && > + connector->hotplug_retries < (is_tc ? 5 : 1) && I had observed that intel_dp_detect may race between user spece invoked get connector call and intel_encoder_hotplug(), that may leave connector status to be UNCHANGED in actual hotplug flow as intel_dp_detect() already called from drm_helper_probe_single_connector_modes(), this may results in 5 retries for type-C ports for normal HPD assertion. Please correct me if i am wrong. Thanks, Anshuman Gupta. > !dig_port->dp.is_mst) > state = INTEL_HOTPLUG_RETRY; > > -- > 2.23.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Apr 01, 2020 at 04:50:23PM +0530, Anshuman Gupta wrote: > On 2020-03-30 at 15:24:25 +0530, Imre Deak wrote: > > On TypeC ports if a sink deasserts/reasserts its HPD signal, generating > > a hotplug interrupt without the sink getting unplugged/replugged from > > the connector, there can be an up to 3 seconds delay until the AUX > > channel gets functional. To avoid detection failures this delay causes > > retry the detection for 5 seconds. > > > > I noticed this on ICL/TGL RVPs and a DELL XPS 13 7390 ICL laptop. > > > > References: https://gitlab.freedesktop.org/drm/intel/issues/1067 > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_ddi.c | 12 +++++++++++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c > > index 4f508bf70f3b..2d947ff83488 100644 > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > @@ -4371,7 +4371,10 @@ static enum intel_hotplug_state > > intel_ddi_hotplug(struct intel_encoder *encoder, > > struct intel_connector *connector) > > { > > + struct drm_i915_private *i915 = to_i915(encoder->base.dev); > > struct intel_digital_port *dig_port = enc_to_dig_port(encoder); > > + enum phy phy = intel_port_to_phy(i915, encoder->port); > > + bool is_tc = intel_phy_is_tc(i915, phy); > > struct drm_modeset_acquire_ctx ctx; > > enum intel_hotplug_state state; > > int ret; > > @@ -4414,8 +4417,15 @@ intel_ddi_hotplug(struct intel_encoder *encoder, > > * 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. > > + * > > + * Type-c connectors which get their HPD signal deasserted then > > + * reasserted, without unplugging/replugging the sink from the > > + * connector, introduce a delay until the AUX channel communication > > + * becomes functional. Retry the detection for 5 seconds on type-c > > + * connectors to account for this delay. > > */ > > - if (state == INTEL_HOTPLUG_UNCHANGED && !connector->hotplug_retries && > > + if (state == INTEL_HOTPLUG_UNCHANGED && > > + connector->hotplug_retries < (is_tc ? 5 : 1) && > > I had observed that intel_dp_detect may race between user spece > invoked get connector call and intel_encoder_hotplug(), that may leave > connector status to be UNCHANGED in actual hotplug flow as > intel_dp_detect() already called from > drm_helper_probe_single_connector_modes(), this may results in 5 > retries for type-C ports for normal HPD assertion. Please correct me > if i am wrong. Yes, it's possible the retries will be unneccessary, but I don't think we can do much about avoiding a race between a hotplug event and a detect call. > Thanks, > Anshuman Gupta. > > !dig_port->dp.is_mst) > > state = INTEL_HOTPLUG_RETRY; > > > > -- > > 2.23.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Mar 30, 2020 at 10:13:02PM +0300, Souza, Jose wrote: > On Mon, 2020-03-30 at 12:54 +0300, Imre Deak wrote: > > On TypeC ports if a sink deasserts/reasserts its HPD signal, > > generating > > a hotplug interrupt without the sink getting unplugged/replugged from > > the connector, there can be an up to 3 seconds delay until the AUX > > channel gets functional. To avoid detection failures this delay > > causes > > retry the detection for 5 seconds. > > 5 seconds? would it be 5 tries? Yes, 5 tries with a 1 second delay in-between them. Thanks for the review, patchset pushed to -dinq. > Other than that both patches looks good. > > Reviewed-by: José Roberto de Souza <jose.souza@intel.com> > > > > > I noticed this on ICL/TGL RVPs and a DELL XPS 13 7390 ICL laptop. > > > > References: https://gitlab.freedesktop.org/drm/intel/issues/1067 > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_ddi.c | 12 +++++++++++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > > b/drivers/gpu/drm/i915/display/intel_ddi.c > > index 4f508bf70f3b..2d947ff83488 100644 > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > @@ -4371,7 +4371,10 @@ static enum intel_hotplug_state > > intel_ddi_hotplug(struct intel_encoder *encoder, > > struct intel_connector *connector) > > { > > + struct drm_i915_private *i915 = to_i915(encoder->base.dev); > > struct intel_digital_port *dig_port = enc_to_dig_port(encoder); > > + enum phy phy = intel_port_to_phy(i915, encoder->port); > > + bool is_tc = intel_phy_is_tc(i915, phy); > > struct drm_modeset_acquire_ctx ctx; > > enum intel_hotplug_state state; > > int ret; > > @@ -4414,8 +4417,15 @@ intel_ddi_hotplug(struct intel_encoder > > *encoder, > > * 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. > > + * > > + * Type-c connectors which get their HPD signal deasserted then > > + * reasserted, without unplugging/replugging the sink from the > > + * connector, introduce a delay until the AUX channel > > communication > > + * becomes functional. Retry the detection for 5 seconds on > > type-c > > + * connectors to account for this delay. > > */ > > - if (state == INTEL_HOTPLUG_UNCHANGED && !connector- > > >hotplug_retries && > > + if (state == INTEL_HOTPLUG_UNCHANGED && > > + connector->hotplug_retries < (is_tc ? 5 : 1) && > > !dig_port->dp.is_mst) > > state = INTEL_HOTPLUG_RETRY; > >
diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c index 4f508bf70f3b..2d947ff83488 100644 --- a/drivers/gpu/drm/i915/display/intel_ddi.c +++ b/drivers/gpu/drm/i915/display/intel_ddi.c @@ -4371,7 +4371,10 @@ static enum intel_hotplug_state intel_ddi_hotplug(struct intel_encoder *encoder, struct intel_connector *connector) { + struct drm_i915_private *i915 = to_i915(encoder->base.dev); struct intel_digital_port *dig_port = enc_to_dig_port(encoder); + enum phy phy = intel_port_to_phy(i915, encoder->port); + bool is_tc = intel_phy_is_tc(i915, phy); struct drm_modeset_acquire_ctx ctx; enum intel_hotplug_state state; int ret; @@ -4414,8 +4417,15 @@ intel_ddi_hotplug(struct intel_encoder *encoder, * 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. + * + * Type-c connectors which get their HPD signal deasserted then + * reasserted, without unplugging/replugging the sink from the + * connector, introduce a delay until the AUX channel communication + * becomes functional. Retry the detection for 5 seconds on type-c + * connectors to account for this delay. */ - if (state == INTEL_HOTPLUG_UNCHANGED && !connector->hotplug_retries && + if (state == INTEL_HOTPLUG_UNCHANGED && + connector->hotplug_retries < (is_tc ? 5 : 1) && !dig_port->dp.is_mst) state = INTEL_HOTPLUG_RETRY;
On TypeC ports if a sink deasserts/reasserts its HPD signal, generating a hotplug interrupt without the sink getting unplugged/replugged from the connector, there can be an up to 3 seconds delay until the AUX channel gets functional. To avoid detection failures this delay causes retry the detection for 5 seconds. I noticed this on ICL/TGL RVPs and a DELL XPS 13 7390 ICL laptop. References: https://gitlab.freedesktop.org/drm/intel/issues/1067 Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/display/intel_ddi.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)