| Message ID | 20190222210834.17580-2-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915: Add support for retrying hotplug | expand |
On Fri, Feb 22, 2019 at 01:08:34PM -0800, José Roberto de Souza wrote: > Unpowered type-c dongles can take some time to boot and be > responsible, causing the probe to fail and sink never be detected > without further actions from userspace. > > It was not a issue for older platforms because there was a hardware > bridge between DDI/DP ports and type-c controller adding a implicit > delay that hid this issue but ICL have type-c controllers integrated > to the SOC bring this issue to users. > > So here if the probe failed when coming from a IRQ it returns > INTEL_HOTPLUG_RETRY that will schedule another run of > i915_hotplug_work_func() after 1 second what is time enough for > those type-c dongles to boot. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Imre Deak <imre.deak@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 1676a87f18cb..96bbcf5c9787 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -4056,6 +4056,8 @@ intel_ddi_hotplug(struct intel_encoder *encoder, > struct intel_connector *connector, > bool irq_received) > { > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > + 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; > @@ -4082,6 +4084,17 @@ 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 type to those dongles to power up > + * and then retrying the probe. > + */ > + if (state == INTEL_HOTPLUG_NOCHANGE && > + connector->base.status != connector_status_connected && > + irq_received && intel_port_is_tc(dev_priv, encoder->port) && > + !dig_port->tc_legacy_port && !dig_port->dp.is_mst) Based on the investigation by Jani et al, we have the similar problem with HDMI, only during disconnect. So I think we could generalize by retrying any time there is no change (except for MST where the driver always keeps the connector in a disconnected state), regardless of the type of the sink, since a no-change is suspect in any case: why would the sink signal (with a long pulse) if there is no change? For HDMI we'd also need to handle this in intel_hdmi.c. Then Ville suggested to add a Chamelium test for this to IGT, could you Jose look into that as well? Both the connect and disconnect races could be tested, both on HDMI and DP, by generating the HPD early/late wrt. to AUX/DDC starting/stopping to return valid data. I don't know if Chamelium can do this, you'd have to find that out first. --Imre > + state = INTEL_HOTPLUG_RETRY; > + > return state; > } > > -- > 2.20.1 >
On Tue, 26 Feb 2019, Imre Deak <imre.deak@intel.com> wrote: > On Fri, Feb 22, 2019 at 01:08:34PM -0800, José Roberto de Souza wrote: >> Unpowered type-c dongles can take some time to boot and be >> responsible, causing the probe to fail and sink never be detected >> without further actions from userspace. >> >> It was not a issue for older platforms because there was a hardware >> bridge between DDI/DP ports and type-c controller adding a implicit >> delay that hid this issue but ICL have type-c controllers integrated >> to the SOC bring this issue to users. >> >> So here if the probe failed when coming from a IRQ it returns >> INTEL_HOTPLUG_RETRY that will schedule another run of >> i915_hotplug_work_func() after 1 second what is time enough for >> those type-c dongles to boot. >> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Cc: Imre Deak <imre.deak@intel.com> >> Cc: Jani Nikula <jani.nikula@intel.com> >> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> >> --- >> drivers/gpu/drm/i915/intel_ddi.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >> index 1676a87f18cb..96bbcf5c9787 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -4056,6 +4056,8 @@ intel_ddi_hotplug(struct intel_encoder *encoder, >> struct intel_connector *connector, >> bool irq_received) >> { >> + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); >> + 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; >> @@ -4082,6 +4084,17 @@ 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 type to those dongles to power up >> + * and then retrying the probe. >> + */ >> + if (state == INTEL_HOTPLUG_NOCHANGE && >> + connector->base.status != connector_status_connected && >> + irq_received && intel_port_is_tc(dev_priv, encoder->port) && >> + !dig_port->tc_legacy_port && !dig_port->dp.is_mst) > > Based on the investigation by Jani et al, we have the similar problem with > HDMI, only during disconnect. So I think we could generalize by retrying > any time there is no change (except for MST where the driver always keeps > the connector in a disconnected state), regardless of the type of the > sink, since a no-change is suspect in any case: why would the sink signal > (with a long pulse) if there is no change? > > For HDMI we'd also need to handle this in intel_hdmi.c. > > Then Ville suggested to add a Chamelium test for this to IGT, could you > Jose look into that as well? Both the connect and disconnect races could > be tested, both on HDMI and DP, by generating the HPD early/late wrt. to > AUX/DDC starting/stopping to return valid data. I don't know if > Chamelium can do this, you'd have to find that out first. Guang Bai also kept referencing a pathological customer test case which we're not privy to. BR, Jani. > > --Imre > >> + state = INTEL_HOTPLUG_RETRY; >> + >> return state; >> } >> >> -- >> 2.20.1 >> > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, 2019-02-26 at 16:08 +0200, Imre Deak wrote: > On Fri, Feb 22, 2019 at 01:08:34PM -0800, José Roberto de Souza > wrote: > > Unpowered type-c dongles can take some time to boot and be > > responsible, causing the probe to fail and sink never be detected > > without further actions from userspace. > > > > It was not a issue for older platforms because there was a hardware > > bridge between DDI/DP ports and type-c controller adding a implicit > > delay that hid this issue but ICL have type-c controllers > > integrated > > to the SOC bring this issue to users. > > > > So here if the probe failed when coming from a IRQ it returns > > INTEL_HOTPLUG_RETRY that will schedule another run of > > i915_hotplug_work_func() after 1 second what is time enough for > > those type-c dongles to boot. > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Imre Deak <imre.deak@intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > b/drivers/gpu/drm/i915/intel_ddi.c > > index 1676a87f18cb..96bbcf5c9787 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -4056,6 +4056,8 @@ intel_ddi_hotplug(struct intel_encoder > > *encoder, > > struct intel_connector *connector, > > bool irq_received) > > { > > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); > > + 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; > > @@ -4082,6 +4084,17 @@ 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 type to those dongles to s/type/time So I don't loose it when doing the next version. > > power up > > + * and then retrying the probe. > > + */ > > + if (state == INTEL_HOTPLUG_NOCHANGE && > > + connector->base.status != connector_status_connected && > > + irq_received && intel_port_is_tc(dev_priv, encoder->port) > > && > > + !dig_port->tc_legacy_port && !dig_port->dp.is_mst) > > Based on the investigation by Jani et al, we have the similar problem > with > HDMI, only during disconnect. So I think we could generalize by > retrying > any time there is no change (except for MST where the driver always > keeps > the connector in a disconnected state), regardless of the type of the > sink, since a no-change is suspect in any case: why would the sink > signal > (with a long pulse) if there is no change? The only case that I can think of is the cases were sink do a short pulse but we don't handle it in the short pulse handler and schedule a long pulse handler but it should not cause any drawback retry everytime there is no change in the state and irq_received is set. What comment could we add about the HDMI here? Something like this would be fine? "HDMI sinks are reported as connected by hardware right after unpluging it, so here also giving some time for hardware to process the unplug so driver can read it and do the unplug sequence and notify userspace about the absence of the HDMI sink" > > For HDMI we'd also need to handle this in intel_hdmi.c. It happens in older gens that don't have DDI? Otherwise just the update above should take care of DP and HDMI DDI ports. > > Then Ville suggested to add a Chamelium test for this to IGT, could > you > Jose look into that as well? Both the connect and disconnect races > could > be tested, both on HDMI and DP, by generating the HPD early/late wrt. > to > AUX/DDC starting/stopping to return valid data. I don't know if > Chamelium can do this, you'd have to find that out first. I will try put my hands in a Chamelium board otherwise I will play with trybot to add this tests. > > --Imre > > > + state = INTEL_HOTPLUG_RETRY; > > + > > return state; > > } > > > > -- > > 2.20.1 > >
On Thu, Feb 28, 2019 at 02:32:38AM +0200, Souza, Jose wrote: > [...] > > > + * and then retrying the probe. > > > + */ > > > + if (state == INTEL_HOTPLUG_NOCHANGE && > > > + connector->base.status != connector_status_connected && > > > + irq_received && intel_port_is_tc(dev_priv, encoder->port) > > > && > > > + !dig_port->tc_legacy_port && !dig_port->dp.is_mst) > > > > Based on the investigation by Jani et al, we have the similar problem > > with > > HDMI, only during disconnect. So I think we could generalize by > > retrying > > any time there is no change (except for MST where the driver always > > keeps > > the connector in a disconnected state), regardless of the type of the > > sink, since a no-change is suspect in any case: why would the sink > > signal > > (with a long pulse) if there is no change? > > The only case that I can think of is the cases were sink do a short > pulse but we don't handle it in the short pulse handler and schedule a > long pulse handler but it should not cause any drawback retry everytime > there is no change in the state and irq_received is set. > > What comment could we add about the HDMI here? Something like this > would be fine? > > "HDMI sinks are reported as connected by hardware right after unpluging > it, so here also giving some time for hardware to process the unplug so > driver can read it and do the unplug sequence and notify userspace > about the absence of the HDMI sink" It could be more specific, something like: 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. > > > > > For HDMI we'd also need to handle this in intel_hdmi.c. > > It happens in older gens that don't have DDI? Otherwise just the update > above should take care of DP and HDMI DDI ports. I've been told it happens both on old and new HW. > > Then Ville suggested to add a Chamelium test for this to IGT, could > > you > > Jose look into that as well? Both the connect and disconnect races > > could > > be tested, both on HDMI and DP, by generating the HPD early/late wrt. > > to > > AUX/DDC starting/stopping to return valid data. I don't know if > > Chamelium can do this, you'd have to find that out first. > > I will try put my hands in a Chamelium board otherwise I will play with > trybot to add this tests. Okay, thanks. --Imre
On Thu, 2019-02-28 at 18:06 +0200, Imre Deak wrote: > On Thu, Feb 28, 2019 at 02:32:38AM +0200, Souza, Jose wrote: > > [...] > > > > + * and then retrying the probe. > > > > + */ > > > > + if (state == INTEL_HOTPLUG_NOCHANGE && > > > > + connector->base.status != > > > > connector_status_connected && > > > > + irq_received && intel_port_is_tc(dev_priv, encoder- > > > > >port) > > > > && > > > > + !dig_port->tc_legacy_port && !dig_port->dp.is_mst) > > > > > > Based on the investigation by Jani et al, we have the similar > > > problem > > > with > > > HDMI, only during disconnect. So I think we could generalize by > > > retrying > > > any time there is no change (except for MST where the driver > > > always > > > keeps > > > the connector in a disconnected state), regardless of the type of > > > the > > > sink, since a no-change is suspect in any case: why would the > > > sink > > > signal > > > (with a long pulse) if there is no change? > > > > The only case that I can think of is the cases were sink do a short > > pulse but we don't handle it in the short pulse handler and > > schedule a > > long pulse handler but it should not cause any drawback retry > > everytime > > there is no change in the state and irq_received is set. > > > > What comment could we add about the HDMI here? Something like this > > would be fine? > > > > "HDMI sinks are reported as connected by hardware right after > > unpluging > > it, so here also giving some time for hardware to process the > > unplug so > > driver can read it and do the unplug sequence and notify userspace > > about the absence of the HDMI sink" > > It could be more specific, something like: > > 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. > > > > For HDMI we'd also need to handle this in intel_hdmi.c. > > > > It happens in older gens that don't have DDI? Otherwise just the > > update > > above should take care of DP and HDMI DDI ports. > > I've been told it happens both on old and new HW. > > > > Then Ville suggested to add a Chamelium test for this to IGT, > > > could > > > you > > > Jose look into that as well? Both the connect and disconnect > > > races > > > could > > > be tested, both on HDMI and DP, by generating the HPD early/late > > > wrt. > > > to > > > AUX/DDC starting/stopping to return valid data. I don't know if > > > Chamelium can do this, you'd have to find that out first. > > > > I will try put my hands in a Chamelium board otherwise I will play > > with > > trybot to add this tests. > > Okay, thanks. Just sent the new kernel patches and the IGT test for the type-c bug, still working in the HDMI test. > > --Imre
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 1676a87f18cb..96bbcf5c9787 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -4056,6 +4056,8 @@ intel_ddi_hotplug(struct intel_encoder *encoder, struct intel_connector *connector, bool irq_received) { + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); + 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; @@ -4082,6 +4084,17 @@ 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 type to those dongles to power up + * and then retrying the probe. + */ + if (state == INTEL_HOTPLUG_NOCHANGE && + connector->base.status != connector_status_connected && + irq_received && intel_port_is_tc(dev_priv, encoder->port) && + !dig_port->tc_legacy_port && !dig_port->dp.is_mst) + state = INTEL_HOTPLUG_RETRY; + return state; }
Unpowered type-c dongles can take some time to boot and be responsible, causing the probe to fail and sink never be detected without further actions from userspace. It was not a issue for older platforms because there was a hardware bridge between DDI/DP ports and type-c controller adding a implicit delay that hid this issue but ICL have type-c controllers integrated to the SOC bring this issue to users. So here if the probe failed when coming from a IRQ it returns INTEL_HOTPLUG_RETRY that will schedule another run of i915_hotplug_work_func() after 1 second what is time enough for those type-c dongles to boot. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Jani Nikula <jani.nikula@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)