| Message ID | 1441373176-22302-5-git-send-email-sonika.jindal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Sep 04, 2015 at 06:56:14PM +0530, Sonika Jindal wrote: > The Bspec is very clear that Live status must be checked about before > trying to read EDID over DDC channel. This patch makes sure that HDMI > EDID is read only when live status us up. > > The live status doesn't seem to perform very consistent across various > platforms when tested with different monitors. The reason behind that is > some monitors are late to provide right voltage to set live_status up. > So, after getting the interrupt, for a small duration, live status reg > fluctuates, and then settles down showing the correct staus. > > This is explained here in, in a rough way: > HPD line ________________ > |\ T1 = Monitor Hotplug causing IRQ > | \______________________________________ > | | > | | > | | T2 = Live status is stable > | | _____________________________________ > | | /| > Live status _____________|_|/ | > | | | > | | | > | | | > T0 T1 T2 > > (Between T1 and T2 Live status fluctuates or can be even low, depending on > the monitor) > > After several experiments, we have concluded that a max delay > of 30ms is enough to allow the live status to settle down with > most of the monitors. This total delay of 30ms has been split into > a resolution of 3 retries of 10ms each, for the better cases. > > This delay is kept at 30ms, keeping in consideration that, HDCP compliance > expect the HPD handler to respond a plug out in 100ms, by disabling the port. > > v2: Adding checks for VLV/CHV as well. Reusing old ibx and g4x functions > to check digital port status. Adding a separate function to get bxt live > status (Daniel) > v3: Using intel_encoder->hpd_pin to check the live status (Siva) > Moving the live status read to intel_hdmi_probe and passing parameter > to read/not to read the edid. (me) > v4: > * Added live status check for all platforms using > intel_digital_port_connected. > * Rebased on top of Jani's DP cleanup series > * Some monitors take time in setting the live status. So retry for few > times if this is a connect HPD > > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> Really pretty commit message! Had some minor comments on two other patches in this series while reading through them, but looks good otherwise. Please sign up someone for final review of the details and I'll pull in the patches once that's done. -Daniel > --- > drivers/gpu/drm/i915/intel_hdmi.c | 35 ++++++++++++++++++++++++++++------- > 1 file changed, 28 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index 1eda71a..d82887b 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -1329,22 +1329,23 @@ intel_hdmi_unset_edid(struct drm_connector *connector) > } > > static bool > -intel_hdmi_set_edid(struct drm_connector *connector) > +intel_hdmi_set_edid(struct drm_connector *connector, bool force) > { > struct drm_i915_private *dev_priv = to_i915(connector->dev); > struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); > struct intel_encoder *intel_encoder = > &hdmi_to_dig_port(intel_hdmi)->base; > enum intel_display_power_domain power_domain; > - struct edid *edid; > + struct edid *edid = NULL; > bool connected = false; > > power_domain = intel_display_port_power_domain(intel_encoder); > intel_display_power_get(dev_priv, power_domain); > > - edid = drm_get_edid(connector, > - intel_gmbus_get_adapter(dev_priv, > - intel_hdmi->ddc_bus)); > + if (force) > + edid = drm_get_edid(connector, > + intel_gmbus_get_adapter(dev_priv, > + intel_hdmi->ddc_bus)); > > intel_display_power_put(dev_priv, power_domain); > > @@ -1374,6 +1375,26 @@ void intel_hdmi_probe(struct intel_encoder *intel_encoder) > enc_to_intel_hdmi(&intel_encoder->base); > struct intel_connector *intel_connector = > intel_hdmi->attached_connector; > + struct drm_i915_private *dev_priv = to_i915(intel_encoder->base.dev); > + bool live_status = false; > + unsigned int retry = 3; > + > + live_status = intel_digital_port_connected(dev_priv, > + hdmi_to_dig_port(intel_hdmi)); > + if (!intel_connector->detect_edid && live_status == false) { > + /* > + * Hotplug had occurred and old status was disconnected, > + * so it might be possible that live status is not set, > + * so retry for few times > + */ > + do { > + mdelay(10); > + live_status = intel_digital_port_connected(dev_priv, > + hdmi_to_dig_port(intel_hdmi)); > + if (live_status) > + break; > + } while (retry--); > + } > > /* > * We are here, means there is a hotplug or a force > @@ -1381,7 +1402,7 @@ void intel_hdmi_probe(struct intel_encoder *intel_encoder) > * DDC bus to check the current status of HDMI. > */ > intel_hdmi_unset_edid(&intel_connector->base); > - if (intel_hdmi_set_edid(&intel_connector->base)) > + if (intel_hdmi_set_edid(&intel_connector->base, live_status)) > DRM_DEBUG_DRIVER("DDC probe: got EDID\n"); > else > DRM_DEBUG_DRIVER("DDC probe: no EDID\n"); > @@ -1432,7 +1453,7 @@ intel_hdmi_force(struct drm_connector *connector) > if (connector->status != connector_status_connected) > return; > > - intel_hdmi_set_edid(connector); > + intel_hdmi_set_edid(connector, true); > hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI; > } > > -- > 1.7.10.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Sep 4, 2015 at 7:47 AM Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, Sep 04, 2015 at 06:56:14PM +0530, Sonika Jindal wrote: > > The Bspec is very clear that Live status must be checked about before > > trying to read EDID over DDC channel. This patch makes sure that HDMI > > EDID is read only when live status us up. > s/us/is > > > > The live status doesn't seem to perform very consistent across various > > platforms when tested with different monitors. The reason behind that is > > some monitors are late to provide right voltage to set live_status up. > > So, after getting the interrupt, for a small duration, live status reg > > fluctuates, and then settles down showing the correct staus. > > > > This is explained here in, in a rough way: > > HPD line ________________ > > |\ T1 = Monitor Hotplug causing IRQ > > | \______________________________________ > > | | > > | | > > | | T2 = Live status is stable > > | | _____________________________________ > > | | /| > > Live status _____________|_|/ | > > | | | > > | | | > > | | | > > T0 T1 T2 > > > > (Between T1 and T2 Live status fluctuates or can be even low, depending > on > > the monitor) > > > > After several experiments, we have concluded that a max delay > > of 30ms is enough to allow the live status to settle down with > > most of the monitors. This total delay of 30ms has been split into > > a resolution of 3 retries of 10ms each, for the better cases. > > > > This delay is kept at 30ms, keeping in consideration that, HDCP > compliance > > expect the HPD handler to respond a plug out in 100ms, by disabling the > port. > > > > v2: Adding checks for VLV/CHV as well. Reusing old ibx and g4x functions > > to check digital port status. Adding a separate function to get bxt live > > status (Daniel) > > v3: Using intel_encoder->hpd_pin to check the live status (Siva) > > Moving the live status read to intel_hdmi_probe and passing parameter > > to read/not to read the edid. (me) > > v4: > > * Added live status check for all platforms using > > intel_digital_port_connected. > > * Rebased on top of Jani's DP cleanup series > > * Some monitors take time in setting the live status. So retry for few > > times if this is a connect HPD > > > > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> > > Really pretty commit message! Had some minor comments on two other patches > in this series while reading through them, but looks good otherwise. > Please sign up someone for final review of the details and I'll pull in > the patches once that's done. > -Daniel > > > --- > > drivers/gpu/drm/i915/intel_hdmi.c | 35 > ++++++++++++++++++++++++++++------- > > 1 file changed, 28 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c > b/drivers/gpu/drm/i915/intel_hdmi.c > > index 1eda71a..d82887b 100644 > > --- a/drivers/gpu/drm/i915/intel_hdmi.c > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > > @@ -1329,22 +1329,23 @@ intel_hdmi_unset_edid(struct drm_connector > *connector) > > } > > > > static bool > > -intel_hdmi_set_edid(struct drm_connector *connector) > > +intel_hdmi_set_edid(struct drm_connector *connector, bool force) > > { > > struct drm_i915_private *dev_priv = to_i915(connector->dev); > > struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); > > struct intel_encoder *intel_encoder = > > &hdmi_to_dig_port(intel_hdmi)->base; > > enum intel_display_power_domain power_domain; > > - struct edid *edid; > > + struct edid *edid = NULL; > > bool connected = false; > > > > power_domain = intel_display_port_power_domain(intel_encoder); > > intel_display_power_get(dev_priv, power_domain); > > > > - edid = drm_get_edid(connector, > > - intel_gmbus_get_adapter(dev_priv, > > - intel_hdmi->ddc_bus)); > > + if (force) > The force name is confusing here since you only don't get edid in any other way when it is false... But also there is the risk of this function to get called only with this false and never gets edid.. So I believe we should add a debug message here at least for this case so if we start getting cases where panel is taking more than 30ms to get live we will know why we did't actually read the edid.... > > + edid = drm_get_edid(connector, > > + intel_gmbus_get_adapter(dev_priv, > > + intel_hdmi->ddc_bus)); > > > > intel_display_power_put(dev_priv, power_domain); > > > > @@ -1374,6 +1375,26 @@ void intel_hdmi_probe(struct intel_encoder > *intel_encoder) > > enc_to_intel_hdmi(&intel_encoder->base); > > struct intel_connector *intel_connector = > > intel_hdmi->attached_connector; > > + struct drm_i915_private *dev_priv = > to_i915(intel_encoder->base.dev); > > + bool live_status = false; > > + unsigned int retry = 3; > > + > > + live_status = intel_digital_port_connected(dev_priv, > > + > hdmi_to_dig_port(intel_hdmi)); > > + if (!intel_connector->detect_edid && live_status == false) { > > + /* > > + * Hotplug had occurred and old status was disconnected, > > + * so it might be possible that live status is not set, > > + * so retry for few times > > + */ > > + do { > > + mdelay(10); > > + live_status = > intel_digital_port_connected(dev_priv, > > + > hdmi_to_dig_port(intel_hdmi)); > > + if (live_status) > > + break; > > + } while (retry--); > small and optional bikesheding here: I'd do: while(!live_status && --retry) { mdelay(10); live_status = ... } > > + } > > > > /* > > * We are here, means there is a hotplug or a force > > @@ -1381,7 +1402,7 @@ void intel_hdmi_probe(struct intel_encoder > *intel_encoder) > > * DDC bus to check the current status of HDMI. > > */ > > intel_hdmi_unset_edid(&intel_connector->base); > > - if (intel_hdmi_set_edid(&intel_connector->base)) > > + if (intel_hdmi_set_edid(&intel_connector->base, live_status)) > > DRM_DEBUG_DRIVER("DDC probe: got EDID\n"); > > else > > DRM_DEBUG_DRIVER("DDC probe: no EDID\n"); > > @@ -1432,7 +1453,7 @@ intel_hdmi_force(struct drm_connector *connector) > > if (connector->status != connector_status_connected) > > return; > > > > - intel_hdmi_set_edid(connector); > > + intel_hdmi_set_edid(connector, true); > > hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI; > > } > > > > -- > > 1.7.10.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 1eda71a..d82887b 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1329,22 +1329,23 @@ intel_hdmi_unset_edid(struct drm_connector *connector) } static bool -intel_hdmi_set_edid(struct drm_connector *connector) +intel_hdmi_set_edid(struct drm_connector *connector, bool force) { struct drm_i915_private *dev_priv = to_i915(connector->dev); struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); struct intel_encoder *intel_encoder = &hdmi_to_dig_port(intel_hdmi)->base; enum intel_display_power_domain power_domain; - struct edid *edid; + struct edid *edid = NULL; bool connected = false; power_domain = intel_display_port_power_domain(intel_encoder); intel_display_power_get(dev_priv, power_domain); - edid = drm_get_edid(connector, - intel_gmbus_get_adapter(dev_priv, - intel_hdmi->ddc_bus)); + if (force) + edid = drm_get_edid(connector, + intel_gmbus_get_adapter(dev_priv, + intel_hdmi->ddc_bus)); intel_display_power_put(dev_priv, power_domain); @@ -1374,6 +1375,26 @@ void intel_hdmi_probe(struct intel_encoder *intel_encoder) enc_to_intel_hdmi(&intel_encoder->base); struct intel_connector *intel_connector = intel_hdmi->attached_connector; + struct drm_i915_private *dev_priv = to_i915(intel_encoder->base.dev); + bool live_status = false; + unsigned int retry = 3; + + live_status = intel_digital_port_connected(dev_priv, + hdmi_to_dig_port(intel_hdmi)); + if (!intel_connector->detect_edid && live_status == false) { + /* + * Hotplug had occurred and old status was disconnected, + * so it might be possible that live status is not set, + * so retry for few times + */ + do { + mdelay(10); + live_status = intel_digital_port_connected(dev_priv, + hdmi_to_dig_port(intel_hdmi)); + if (live_status) + break; + } while (retry--); + } /* * We are here, means there is a hotplug or a force @@ -1381,7 +1402,7 @@ void intel_hdmi_probe(struct intel_encoder *intel_encoder) * DDC bus to check the current status of HDMI. */ intel_hdmi_unset_edid(&intel_connector->base); - if (intel_hdmi_set_edid(&intel_connector->base)) + if (intel_hdmi_set_edid(&intel_connector->base, live_status)) DRM_DEBUG_DRIVER("DDC probe: got EDID\n"); else DRM_DEBUG_DRIVER("DDC probe: no EDID\n"); @@ -1432,7 +1453,7 @@ intel_hdmi_force(struct drm_connector *connector) if (connector->status != connector_status_connected) return; - intel_hdmi_set_edid(connector); + intel_hdmi_set_edid(connector, true); hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI; }
The Bspec is very clear that Live status must be checked about before trying to read EDID over DDC channel. This patch makes sure that HDMI EDID is read only when live status us up. The live status doesn't seem to perform very consistent across various platforms when tested with different monitors. The reason behind that is some monitors are late to provide right voltage to set live_status up. So, after getting the interrupt, for a small duration, live status reg fluctuates, and then settles down showing the correct staus. This is explained here in, in a rough way: HPD line ________________ |\ T1 = Monitor Hotplug causing IRQ | \______________________________________ | | | | | | T2 = Live status is stable | | _____________________________________ | | /| Live status _____________|_|/ | | | | | | | | | | T0 T1 T2 (Between T1 and T2 Live status fluctuates or can be even low, depending on the monitor) After several experiments, we have concluded that a max delay of 30ms is enough to allow the live status to settle down with most of the monitors. This total delay of 30ms has been split into a resolution of 3 retries of 10ms each, for the better cases. This delay is kept at 30ms, keeping in consideration that, HDCP compliance expect the HPD handler to respond a plug out in 100ms, by disabling the port. v2: Adding checks for VLV/CHV as well. Reusing old ibx and g4x functions to check digital port status. Adding a separate function to get bxt live status (Daniel) v3: Using intel_encoder->hpd_pin to check the live status (Siva) Moving the live status read to intel_hdmi_probe and passing parameter to read/not to read the edid. (me) v4: * Added live status check for all platforms using intel_digital_port_connected. * Rebased on top of Jani's DP cleanup series * Some monitors take time in setting the live status. So retry for few times if this is a connect HPD Signed-off-by: Sonika Jindal <sonika.jindal@intel.com> --- drivers/gpu/drm/i915/intel_hdmi.c | 35 ++++++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 deletions(-)