| Message ID | 1457720460-11164-3-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 11, 2016 at 11:51:00PM +0530, Shashank Sharma wrote: > This patch restricts usage of live status check for HDMI detection. > While testing certain (monitor + cable) combinations with various > intel platforms, it seems that live status register is not reliable > on some older devices. So limit the live_status check from VLV onwards. > > This fixes a regression introduced in: > commit 237ed86c693d8a8e4db476976aeb30df4deac74b > Author: Sonika Jindal <sonika.jindal@intel.com> > Date: Tue Sep 15 09:44:20 2015 +0530 > drm/i915: Check live status before reading edid > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > --- > drivers/gpu/drm/i915/intel_hdmi.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index b523a2f..ca1fb57 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -1397,6 +1397,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) > enum drm_connector_status status; > struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); > struct drm_i915_private *dev_priv = to_i915(connector->dev); > + struct drm_device *dev = connector->dev; > bool live_status = true; > unsigned int try = 8; > > @@ -1404,14 +1405,21 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) > connector->base.id, connector->name); > > intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); > - live_status = intel_digital_port_connected(dev_priv, > - hdmi_to_dig_port(intel_hdmi)); > - while (!live_status && try--) { > - msleep(10); > + > + /* > + * The live status register doesn't work reliably with certain > + * cables/monitors, on old platforms. So restrict the live status > + * check to be applied from VLV onwards. > + */ > + if (INTEL_INFO(dev)->gen >= 7 && !IS_IVYBRIDGE(dev)) { > live_status = intel_digital_port_connected(dev_priv, > hdmi_to_dig_port(intel_hdmi)); > + while (!live_status && try--) { > + msleep(10); > + live_status = intel_digital_port_connected(dev_priv, > + hdmi_to_dig_port(intel_hdmi)); > + } > } > - I would avoid all that churn and just do if (whatever) live_status = true; after we're done with the loop. That might at least hang on to some of the benefits of the live status check for these platforms, whether or not the live status really works or not. Not that I particularly like this random choice of platforms approach either. > intel_hdmi_unset_edid(connector); > > if (intel_hdmi_set_edid(connector, live_status)) { > -- > 1.9.1
Right now live status check is helping in many scenarios, including: - HDMI compliance - HDMI connector status to be shown to user space - While running HDCP compliance, we pass a train of hot-plugs/unplugs via analyzers, and without live status check and HDMI optimization patches, our software state machine is in a soup. I don't see any other way of solving all the above problems, without this, and fixing the regression too on the old devices. So if you think we should remove this, we should also have some alternative solution for above listed problems. Our hotplug handling is not the best way it should be done (both in HW and SW), and we will end up something like this only. We can't say that we are not going to bother about these problem, as we shouldn't focus only on the upstream use cases, but Android and Chrome cases also, which are commercial targets, where meeting the product quality gets even more difficult. If you agree, we can discus more about how to make this work, else the only thing I can come up with is we will revert the HDMI optimization and live status check from upstream, and keep them local on Android branches. Regards Shashank -----Original Message----- From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] Sent: Saturday, March 12, 2016 12:54 AM To: Sharma, Shashank Cc: intel-gfx@lists.freedesktop.org; Jindal, Sonika; Kumar, Shobhit; Wang, Gary C; Vetter, Daniel Subject: Re: [PATCH 2/2] drm/i915: Restrict usage of live status check On Fri, Mar 11, 2016 at 11:51:00PM +0530, Shashank Sharma wrote: > This patch restricts usage of live status check for HDMI detection. > While testing certain (monitor + cable) combinations with various > intel platforms, it seems that live status register is not reliable > on some older devices. So limit the live_status check from VLV onwards. > > This fixes a regression introduced in: > commit 237ed86c693d8a8e4db476976aeb30df4deac74b > Author: Sonika Jindal <sonika.jindal@intel.com> > Date: Tue Sep 15 09:44:20 2015 +0530 > drm/i915: Check live status before reading edid > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > --- > drivers/gpu/drm/i915/intel_hdmi.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c > b/drivers/gpu/drm/i915/intel_hdmi.c > index b523a2f..ca1fb57 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -1397,6 +1397,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) > enum drm_connector_status status; > struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); > struct drm_i915_private *dev_priv = to_i915(connector->dev); > + struct drm_device *dev = connector->dev; > bool live_status = true; > unsigned int try = 8; > > @@ -1404,14 +1405,21 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) > connector->base.id, connector->name); > > intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); > - live_status = intel_digital_port_connected(dev_priv, > - hdmi_to_dig_port(intel_hdmi)); > - while (!live_status && try--) { > - msleep(10); > + > + /* > + * The live status register doesn't work reliably with certain > + * cables/monitors, on old platforms. So restrict the live status > + * check to be applied from VLV onwards. > + */ > + if (INTEL_INFO(dev)->gen >= 7 && !IS_IVYBRIDGE(dev)) { > live_status = intel_digital_port_connected(dev_priv, > hdmi_to_dig_port(intel_hdmi)); > + while (!live_status && try--) { > + msleep(10); > + live_status = intel_digital_port_connected(dev_priv, > + hdmi_to_dig_port(intel_hdmi)); > + } > } > - I would avoid all that churn and just do if (whatever) live_status = true; after we're done with the loop. That might at least hang on to some of the benefits of the live status check for these platforms, whether or not the live status really works or not. Not that I particularly like this random choice of platforms approach either. > intel_hdmi_unset_edid(connector); > > if (intel_hdmi_set_edid(connector, live_status)) { > -- > 1.9.1 -- Ville Syrjälä Intel OTC
On Fri, 11 Mar 2016, Shashank Sharma <shashank.sharma@intel.com> wrote: > [ text/plain ] > This patch restricts usage of live status check for HDMI detection. > While testing certain (monitor + cable) combinations with various > intel platforms, it seems that live status register is not reliable > on some older devices. So limit the live_status check from VLV onwards. > > This fixes a regression introduced in: > commit 237ed86c693d8a8e4db476976aeb30df4deac74b > Author: Sonika Jindal <sonika.jindal@intel.com> > Date: Tue Sep 15 09:44:20 2015 +0530 > drm/i915: Check live status before reading edid This is in v4.4, which means the fix will need to be backported too, so please send a patch fixing the bug *first*, instead of depending on an optimization patch. BR, Jani. > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > --- > drivers/gpu/drm/i915/intel_hdmi.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index b523a2f..ca1fb57 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -1397,6 +1397,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) > enum drm_connector_status status; > struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); > struct drm_i915_private *dev_priv = to_i915(connector->dev); > + struct drm_device *dev = connector->dev; > bool live_status = true; > unsigned int try = 8; > > @@ -1404,14 +1405,21 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) > connector->base.id, connector->name); > > intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); > - live_status = intel_digital_port_connected(dev_priv, > - hdmi_to_dig_port(intel_hdmi)); > - while (!live_status && try--) { > - msleep(10); > + > + /* > + * The live status register doesn't work reliably with certain > + * cables/monitors, on old platforms. So restrict the live status > + * check to be applied from VLV onwards. > + */ > + if (INTEL_INFO(dev)->gen >= 7 && !IS_IVYBRIDGE(dev)) { > live_status = intel_digital_port_connected(dev_priv, > hdmi_to_dig_port(intel_hdmi)); > + while (!live_status && try--) { > + msleep(10); > + live_status = intel_digital_port_connected(dev_priv, > + hdmi_to_dig_port(intel_hdmi)); > + } > } > - > intel_hdmi_unset_edid(connector); > > if (intel_hdmi_set_edid(connector, live_status)) {
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index b523a2f..ca1fb57 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1397,6 +1397,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) enum drm_connector_status status; struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector); struct drm_i915_private *dev_priv = to_i915(connector->dev); + struct drm_device *dev = connector->dev; bool live_status = true; unsigned int try = 8; @@ -1404,14 +1405,21 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) connector->base.id, connector->name); intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); - live_status = intel_digital_port_connected(dev_priv, - hdmi_to_dig_port(intel_hdmi)); - while (!live_status && try--) { - msleep(10); + + /* + * The live status register doesn't work reliably with certain + * cables/monitors, on old platforms. So restrict the live status + * check to be applied from VLV onwards. + */ + if (INTEL_INFO(dev)->gen >= 7 && !IS_IVYBRIDGE(dev)) { live_status = intel_digital_port_connected(dev_priv, hdmi_to_dig_port(intel_hdmi)); + while (!live_status && try--) { + msleep(10); + live_status = intel_digital_port_connected(dev_priv, + hdmi_to_dig_port(intel_hdmi)); + } } - intel_hdmi_unset_edid(connector); if (intel_hdmi_set_edid(connector, live_status)) {
This patch restricts usage of live status check for HDMI detection. While testing certain (monitor + cable) combinations with various intel platforms, it seems that live status register is not reliable on some older devices. So limit the live_status check from VLV onwards. This fixes a regression introduced in: commit 237ed86c693d8a8e4db476976aeb30df4deac74b Author: Sonika Jindal <sonika.jindal@intel.com> Date: Tue Sep 15 09:44:20 2015 +0530 drm/i915: Check live status before reading edid Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> --- drivers/gpu/drm/i915/intel_hdmi.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)