| Message ID | 1458201565-10128-1-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 17 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: 237ed86 "drm/i915: Check live status" > Author: Sonika Jindal <sonika.jindal@intel.com> > Date: Tue Sep 15 09:44:20 2015 +0530 > Fixes: 237ed86c693d ("drm/i915: Check live status before reading edid") Cc: stable@vger.kernel.org # v4.4 Bugzilla: ? > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > --- > drivers/gpu/drm/i915/intel_hdmi.c | 22 ++++++++++++++-------- > 1 file changed, 14 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index e2dab48..d24d18a 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -1397,7 +1397,8 @@ 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); > - bool live_status = false; > + struct drm_device *dev = connector->dev; This is unnecessary. > + bool live_status = true; > unsigned int try; > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > @@ -1405,16 +1406,21 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) > > intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); > > - for (try = 0; !live_status && try < 9; try++) { > - if (try) > - msleep(10); > - live_status = intel_digital_port_connected(dev_priv, > + /* > + * Live status check for HDMI detection is not very > + * reliable on older platforms. So insist the live > + * status check for EDID read from VLV onwards. > + */ > + if (INTEL_INFO(dev)->gen >= 7 && !IS_IVYBRIDGE(dev)) { Replace dev with dev_priv. Both work, it's magic, but we prefer using dev_priv where possible. Move try variable declaration here. > + for (try = 0; !live_status && try < 9; try++) { You'll now ignore live_status on all platforms. (But please do not turn this into a do-while, keep it as a for loop.) > + if (try) > + msleep(10); > + live_status = intel_digital_port_connected(dev_priv, > hdmi_to_dig_port(intel_hdmi)); > + } > + DRM_DEBUG_KMS("Live status %s\n", live_status ? "up" : "down"); Consider using onoff(live_status) instead of live_status ? "up" : "down". > } > > - if (!live_status) > - DRM_DEBUG_KMS("Live status not up!"); > - > intel_hdmi_unset_edid(connector); > > if (intel_hdmi_set_edid(connector, live_status)) {
Regards Shashank On 3/17/2016 2:55 PM, Jani Nikula wrote: > On Thu, 17 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: 237ed86 "drm/i915: Check live status" >> Author: Sonika Jindal <sonika.jindal@intel.com> >> Date: Tue Sep 15 09:44:20 2015 +0530 >> > > Fixes: 237ed86c693d ("drm/i915: Check live status before reading edid") > Cc: stable@vger.kernel.org # v4.4 > Bugzilla: ? > Let me dig for it ... Do you have a link ? If yes, please share. >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >> --- >> drivers/gpu/drm/i915/intel_hdmi.c | 22 ++++++++++++++-------- >> 1 file changed, 14 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c >> index e2dab48..d24d18a 100644 >> --- a/drivers/gpu/drm/i915/intel_hdmi.c >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >> @@ -1397,7 +1397,8 @@ 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); >> - bool live_status = false; >> + struct drm_device *dev = connector->dev; > > This is unnecessary. > >> + bool live_status = true; >> unsigned int try; >> >> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", >> @@ -1405,16 +1406,21 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) >> >> intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); >> >> - for (try = 0; !live_status && try < 9; try++) { >> - if (try) >> - msleep(10); >> - live_status = intel_digital_port_connected(dev_priv, >> + /* >> + * Live status check for HDMI detection is not very >> + * reliable on older platforms. So insist the live >> + * status check for EDID read from VLV onwards. >> + */ >> + if (INTEL_INFO(dev)->gen >= 7 && !IS_IVYBRIDGE(dev)) { > > Replace dev with dev_priv. Both work, it's magic, but we prefer using > dev_priv where possible. > Interesting, will try that out. > Move try variable declaration here. > Got it. >> + for (try = 0; !live_status && try < 9; try++) { > > You'll now ignore live_status on all platforms. (But please do not turn > this into a do-while, keep it as a for loop.) > >> + if (try) >> + msleep(10); >> + live_status = intel_digital_port_connected(dev_priv, >> hdmi_to_dig_port(intel_hdmi)); >> + } >> + DRM_DEBUG_KMS("Live status %s\n", live_status ? "up" : "down"); > > Consider using onoff(live_status) instead of live_status ? "up" : > "down". > Sounds cool, I dint know about onoff yet, will check. >> } >> >> - if (!live_status) >> - DRM_DEBUG_KMS("Live status not up!"); >> - >> intel_hdmi_unset_edid(connector); >> >> if (intel_hdmi_set_edid(connector, live_status)) { >
On Thu, Mar 17, 2016 at 01:29:25PM +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: 237ed86 "drm/i915: Check live status" > Author: Sonika Jindal <sonika.jindal@intel.com> > Date: Tue Sep 15 09:44:20 2015 +0530 > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > --- > drivers/gpu/drm/i915/intel_hdmi.c | 22 ++++++++++++++-------- > 1 file changed, 14 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index e2dab48..d24d18a 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -1397,7 +1397,8 @@ 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); > - bool live_status = false; > + struct drm_device *dev = connector->dev; > + bool live_status = true; > unsigned int try; > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > @@ -1405,16 +1406,21 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) > > intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); > > - for (try = 0; !live_status && try < 9; try++) { > - if (try) > - msleep(10); > - live_status = intel_digital_port_connected(dev_priv, > + /* > + * Live status check for HDMI detection is not very > + * reliable on older platforms. So insist the live > + * status check for EDID read from VLV onwards. > + */ > + if (INTEL_INFO(dev)->gen >= 7 && !IS_IVYBRIDGE(dev)) { > + for (try = 0; !live_status && try < 9; try++) { > + if (try) > + msleep(10); > + live_status = intel_digital_port_connected(dev_priv, > hdmi_to_dig_port(intel_hdmi)); > + } > + DRM_DEBUG_KMS("Live status %s\n", live_status ? "up" : "down"); > } > > - if (!live_status) > - DRM_DEBUG_KMS("Live status not up!"); > - As I said before, I think this whole thing could be solved with a simple two-liner here: + if (...) + live_status = true; > intel_hdmi_unset_edid(connector); > > if (intel_hdmi_set_edid(connector, live_status)) { > -- > 1.9.1
Regards Shashank On 3/17/2016 6:34 PM, Ville Syrjälä wrote: > On Thu, Mar 17, 2016 at 01:29:25PM +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: 237ed86 "drm/i915: Check live status" >> Author: Sonika Jindal <sonika.jindal@intel.com> >> Date: Tue Sep 15 09:44:20 2015 +0530 >> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >> --- >> drivers/gpu/drm/i915/intel_hdmi.c | 22 ++++++++++++++-------- >> 1 file changed, 14 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c >> index e2dab48..d24d18a 100644 >> --- a/drivers/gpu/drm/i915/intel_hdmi.c >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >> @@ -1397,7 +1397,8 @@ 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); >> - bool live_status = false; >> + struct drm_device *dev = connector->dev; >> + bool live_status = true; >> unsigned int try; >> >> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", >> @@ -1405,16 +1406,21 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) >> >> intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); >> >> - for (try = 0; !live_status && try < 9; try++) { >> - if (try) >> - msleep(10); >> - live_status = intel_digital_port_connected(dev_priv, >> + /* >> + * Live status check for HDMI detection is not very >> + * reliable on older platforms. So insist the live >> + * status check for EDID read from VLV onwards. >> + */ >> + if (INTEL_INFO(dev)->gen >= 7 && !IS_IVYBRIDGE(dev)) { >> + for (try = 0; !live_status && try < 9; try++) { >> + if (try) >> + msleep(10); >> + live_status = intel_digital_port_connected(dev_priv, >> hdmi_to_dig_port(intel_hdmi)); >> + } >> + DRM_DEBUG_KMS("Live status %s\n", live_status ? "up" : "down"); >> } >> >> - if (!live_status) >> - DRM_DEBUG_KMS("Live status not up!"); >> - > > As I said before, I think this whole thing could be solved with a simple > two-liner here: > > + if (...) > + live_status = true; > Yes I remember, and I replied on that already that why we would like to keep the live status check. In fact I would be ok to remove this check if you can suggest some other way of making this work for other operating systems/sw platforms. >> intel_hdmi_unset_edid(connector); >> >> if (intel_hdmi_set_edid(connector, live_status)) { >> -- >> 1.9.1 >
On Thu, Mar 17, 2016 at 09:15:39PM +0530, Sharma, Shashank wrote: > Regards > Shashank > > On 3/17/2016 6:34 PM, Ville Syrjälä wrote: > >On Thu, Mar 17, 2016 at 01:29:25PM +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: 237ed86 "drm/i915: Check live status" > >> Author: Sonika Jindal <sonika.jindal@intel.com> > >> Date: Tue Sep 15 09:44:20 2015 +0530 > >> > >>Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > >>--- > >> drivers/gpu/drm/i915/intel_hdmi.c | 22 ++++++++++++++-------- > >> 1 file changed, 14 insertions(+), 8 deletions(-) > >> > >>diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > >>index e2dab48..d24d18a 100644 > >>--- a/drivers/gpu/drm/i915/intel_hdmi.c > >>+++ b/drivers/gpu/drm/i915/intel_hdmi.c > >>@@ -1397,7 +1397,8 @@ 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); > >>- bool live_status = false; > >>+ struct drm_device *dev = connector->dev; > >>+ bool live_status = true; > >> unsigned int try; > >> > >> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > >>@@ -1405,16 +1406,21 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) > >> > >> intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); > >> > >>- for (try = 0; !live_status && try < 9; try++) { > >>- if (try) > >>- msleep(10); > >>- live_status = intel_digital_port_connected(dev_priv, > >>+ /* > >>+ * Live status check for HDMI detection is not very > >>+ * reliable on older platforms. So insist the live > >>+ * status check for EDID read from VLV onwards. > >>+ */ > >>+ if (INTEL_INFO(dev)->gen >= 7 && !IS_IVYBRIDGE(dev)) { > >>+ for (try = 0; !live_status && try < 9; try++) { > >>+ if (try) > >>+ msleep(10); > >>+ live_status = intel_digital_port_connected(dev_priv, > >> hdmi_to_dig_port(intel_hdmi)); > >>+ } > >>+ DRM_DEBUG_KMS("Live status %s\n", live_status ? "up" : "down"); > >> } > >> > >>- if (!live_status) > >>- DRM_DEBUG_KMS("Live status not up!"); > >>- > > > >As I said before, I think this whole thing could be solved with a simple > >two-liner here: > > > >+ if (...) > >+ live_status = true; > > > Yes I remember, and I replied on that already that why we would like > to keep the live status check. In fact I would be ok to remove this > check if you can suggest some other way of making this work for > other operating systems/sw platforms. The question is what is the safest approach for stable@ and for that just doing as we historically did and ignore live_status makes the most sense. Once the regressions have been rectified, then you can decide how best to approach -next. -Chris
On Thu, Mar 17, 2016 at 09:15:39PM +0530, Sharma, Shashank wrote: > Regards > Shashank > > On 3/17/2016 6:34 PM, Ville Syrjälä wrote: > > On Thu, Mar 17, 2016 at 01:29:25PM +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: 237ed86 "drm/i915: Check live status" > >> Author: Sonika Jindal <sonika.jindal@intel.com> > >> Date: Tue Sep 15 09:44:20 2015 +0530 > >> > >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_hdmi.c | 22 ++++++++++++++-------- > >> 1 file changed, 14 insertions(+), 8 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > >> index e2dab48..d24d18a 100644 > >> --- a/drivers/gpu/drm/i915/intel_hdmi.c > >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c > >> @@ -1397,7 +1397,8 @@ 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); > >> - bool live_status = false; > >> + struct drm_device *dev = connector->dev; > >> + bool live_status = true; > >> unsigned int try; > >> > >> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > >> @@ -1405,16 +1406,21 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) > >> > >> intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); > >> > >> - for (try = 0; !live_status && try < 9; try++) { > >> - if (try) > >> - msleep(10); > >> - live_status = intel_digital_port_connected(dev_priv, > >> + /* > >> + * Live status check for HDMI detection is not very > >> + * reliable on older platforms. So insist the live > >> + * status check for EDID read from VLV onwards. > >> + */ > >> + if (INTEL_INFO(dev)->gen >= 7 && !IS_IVYBRIDGE(dev)) { > >> + for (try = 0; !live_status && try < 9; try++) { > >> + if (try) > >> + msleep(10); > >> + live_status = intel_digital_port_connected(dev_priv, > >> hdmi_to_dig_port(intel_hdmi)); > >> + } > >> + DRM_DEBUG_KMS("Live status %s\n", live_status ? "up" : "down"); > >> } > >> > >> - if (!live_status) > >> - DRM_DEBUG_KMS("Live status not up!"); > >> - > > > > As I said before, I think this whole thing could be solved with a simple > > two-liner here: > > > > + if (...) > > + live_status = true; > > > Yes I remember, and I replied on that already that why we would like to > keep the live status check. In fact I would be ok to remove this check > if you can suggest some other way of making this work for other > operating systems/sw platforms. My two liner would keep the check. > >> intel_hdmi_unset_edid(connector); > >> > >> if (intel_hdmi_set_edid(connector, live_status)) { > >> -- > >> 1.9.1 > >
Regards Shashank On 3/17/2016 9:31 PM, Ville Syrjälä wrote: > On Thu, Mar 17, 2016 at 09:15:39PM +0530, Sharma, Shashank wrote: >> Regards >> Shashank >> >> On 3/17/2016 6:34 PM, Ville Syrjälä wrote: >>> On Thu, Mar 17, 2016 at 01:29:25PM +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: 237ed86 "drm/i915: Check live status" >>>> Author: Sonika Jindal <sonika.jindal@intel.com> >>>> Date: Tue Sep 15 09:44:20 2015 +0530 >>>> >>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_hdmi.c | 22 ++++++++++++++-------- >>>> 1 file changed, 14 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c >>>> index e2dab48..d24d18a 100644 >>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c >>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >>>> @@ -1397,7 +1397,8 @@ 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); >>>> - bool live_status = false; >>>> + struct drm_device *dev = connector->dev; >>>> + bool live_status = true; >>>> unsigned int try; >>>> >>>> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", >>>> @@ -1405,16 +1406,21 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) >>>> >>>> intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); >>>> >>>> - for (try = 0; !live_status && try < 9; try++) { >>>> - if (try) >>>> - msleep(10); >>>> - live_status = intel_digital_port_connected(dev_priv, >>>> + /* >>>> + * Live status check for HDMI detection is not very >>>> + * reliable on older platforms. So insist the live >>>> + * status check for EDID read from VLV onwards. >>>> + */ >>>> + if (INTEL_INFO(dev)->gen >= 7 && !IS_IVYBRIDGE(dev)) { >>>> + for (try = 0; !live_status && try < 9; try++) { >>>> + if (try) >>>> + msleep(10); >>>> + live_status = intel_digital_port_connected(dev_priv, >>>> hdmi_to_dig_port(intel_hdmi)); >>>> + } >>>> + DRM_DEBUG_KMS("Live status %s\n", live_status ? "up" : "down"); >>>> } >>>> >>>> - if (!live_status) >>>> - DRM_DEBUG_KMS("Live status not up!"); >>>> - >>> >>> As I said before, I think this whole thing could be solved with a simple >>> two-liner here: >>> >>> + if (...) >>> + live_status = true; >>> >> Yes I remember, and I replied on that already that why we would like to >> keep the live status check. In fact I would be ok to remove this check >> if you can suggest some other way of making this work for other >> operating systems/sw platforms. > > My two liner would keep the check. > Sorry, I might have not understood you properly. Do you mean: if (INTEL_INFO(dev)->gen < 7 && IS_IVYBRIDGE(dev)) { live_status = true; } else { do the same looping for retry; } >>>> intel_hdmi_unset_edid(connector); >>>> >>>> if (intel_hdmi_set_edid(connector, live_status)) { >>>> -- >>>> 1.9.1 >>> >
On Thu, Mar 17, 2016 at 09:35:30PM +0530, Sharma, Shashank wrote: > Regards > Shashank > > On 3/17/2016 9:31 PM, Ville Syrjälä wrote: > > On Thu, Mar 17, 2016 at 09:15:39PM +0530, Sharma, Shashank wrote: > >> Regards > >> Shashank > >> > >> On 3/17/2016 6:34 PM, Ville Syrjälä wrote: > >>> On Thu, Mar 17, 2016 at 01:29:25PM +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: 237ed86 "drm/i915: Check live status" > >>>> Author: Sonika Jindal <sonika.jindal@intel.com> > >>>> Date: Tue Sep 15 09:44:20 2015 +0530 > >>>> > >>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > >>>> --- > >>>> drivers/gpu/drm/i915/intel_hdmi.c | 22 ++++++++++++++-------- > >>>> 1 file changed, 14 insertions(+), 8 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > >>>> index e2dab48..d24d18a 100644 > >>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c > >>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c > >>>> @@ -1397,7 +1397,8 @@ 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); > >>>> - bool live_status = false; > >>>> + struct drm_device *dev = connector->dev; > >>>> + bool live_status = true; > >>>> unsigned int try; > >>>> > >>>> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > >>>> @@ -1405,16 +1406,21 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) > >>>> > >>>> intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); > >>>> > >>>> - for (try = 0; !live_status && try < 9; try++) { > >>>> - if (try) > >>>> - msleep(10); > >>>> - live_status = intel_digital_port_connected(dev_priv, > >>>> + /* > >>>> + * Live status check for HDMI detection is not very > >>>> + * reliable on older platforms. So insist the live > >>>> + * status check for EDID read from VLV onwards. > >>>> + */ > >>>> + if (INTEL_INFO(dev)->gen >= 7 && !IS_IVYBRIDGE(dev)) { > >>>> + for (try = 0; !live_status && try < 9; try++) { > >>>> + if (try) > >>>> + msleep(10); > >>>> + live_status = intel_digital_port_connected(dev_priv, > >>>> hdmi_to_dig_port(intel_hdmi)); > >>>> + } > >>>> + DRM_DEBUG_KMS("Live status %s\n", live_status ? "up" : "down"); > >>>> } > >>>> > >>>> - if (!live_status) > >>>> - DRM_DEBUG_KMS("Live status not up!"); > >>>> - > >>> > >>> As I said before, I think this whole thing could be solved with a simple > >>> two-liner here: > >>> > >>> + if (...) > >>> + live_status = true; > >>> > >> Yes I remember, and I replied on that already that why we would like to > >> keep the live status check. In fact I would be ok to remove this check > >> if you can suggest some other way of making this work for other > >> operating systems/sw platforms. > > > > My two liner would keep the check. > > > Sorry, I might have not understood you properly. > Do you mean: > if (INTEL_INFO(dev)->gen < 7 && IS_IVYBRIDGE(dev)) { > live_status = true; > } else { > do the same looping for retry; > } No, I mean { ... do loop; if (!live_status) DRM_DEBUG_KMS("Live status not up!"); + if (don't trust live status) + live_status = true; intel_hdmi_unset_edid(); if (intel_hdmi_set_edid(live_status)) { ... } > > >>>> intel_hdmi_unset_edid(connector); > >>>> > >>>> if (intel_hdmi_set_edid(connector, live_status)) { > >>>> -- > >>>> 1.9.1 > >>> > >
Hey Chris, I added comments for both Ville and you, please help me to understand this. Regards Shashank On 3/17/2016 9:51 PM, Ville Syrjälä wrote: > On Thu, Mar 17, 2016 at 09:35:30PM +0530, Sharma, Shashank wrote: >> Regards >> Shashank >> >> On 3/17/2016 9:31 PM, Ville Syrjälä wrote: >>> On Thu, Mar 17, 2016 at 09:15:39PM +0530, Sharma, Shashank wrote: >>>> Regards >>>> Shashank >>>> >>>> On 3/17/2016 6:34 PM, Ville Syrjälä wrote: >>>>> On Thu, Mar 17, 2016 at 01:29:25PM +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: 237ed86 "drm/i915: Check live status" >>>>>> Author: Sonika Jindal <sonika.jindal@intel.com> >>>>>> Date: Tue Sep 15 09:44:20 2015 +0530 >>>>>> >>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >>>>>> --- >>>>>> drivers/gpu/drm/i915/intel_hdmi.c | 22 ++++++++++++++-------- >>>>>> 1 file changed, 14 insertions(+), 8 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c >>>>>> index e2dab48..d24d18a 100644 >>>>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c >>>>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >>>>>> @@ -1397,7 +1397,8 @@ 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); >>>>>> - bool live_status = false; >>>>>> + struct drm_device *dev = connector->dev; >>>>>> + bool live_status = true; >>>>>> unsigned int try; >>>>>> >>>>>> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", >>>>>> @@ -1405,16 +1406,21 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) >>>>>> >>>>>> intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); >>>>>> >>>>>> - for (try = 0; !live_status && try < 9; try++) { >>>>>> - if (try) >>>>>> - msleep(10); >>>>>> - live_status = intel_digital_port_connected(dev_priv, >>>>>> + /* >>>>>> + * Live status check for HDMI detection is not very >>>>>> + * reliable on older platforms. So insist the live >>>>>> + * status check for EDID read from VLV onwards. >>>>>> + */ >>>>>> + if (INTEL_INFO(dev)->gen >= 7 && !IS_IVYBRIDGE(dev)) { >>>>>> + for (try = 0; !live_status && try < 9; try++) { >>>>>> + if (try) >>>>>> + msleep(10); >>>>>> + live_status = intel_digital_port_connected(dev_priv, >>>>>> hdmi_to_dig_port(intel_hdmi)); >>>>>> + } >>>>>> + DRM_DEBUG_KMS("Live status %s\n", live_status ? "up" : "down"); >>>>>> } >>>>>> >>>>>> - if (!live_status) >>>>>> - DRM_DEBUG_KMS("Live status not up!"); >>>>>> - >>>>> >>>>> As I said before, I think this whole thing could be solved with a simple >>>>> two-liner here: >>>>> >>>>> + if (...) >>>>> + live_status = true; >>>>> >>>> Yes I remember, and I replied on that already that why we would like to >>>> keep the live status check. In fact I would be ok to remove this check >>>> if you can suggest some other way of making this work for other >>>> operating systems/sw platforms. >>> >>> My two liner would keep the check. >>> >> Sorry, I might have not understood you properly. >> Do you mean: >> if (INTEL_INFO(dev)->gen < 7 && IS_IVYBRIDGE(dev)) { >> live_status = true; >> } else { >> do the same looping for retry; >> } > > No, I mean > > { > ... > do loop; > > if (!live_status) > DRM_DEBUG_KMS("Live status not up!"); > > + if (don't trust live status) > + live_status = true; > > intel_hdmi_unset_edid(); > > if (intel_hdmi_set_edid(live_status)) { > ... > } > > In fact, this is what I have done. If you see the change in this patch, /* Lets make live status true for < VLV platforms */ bool live_status = true; blah.... blah.... /* Check live status only for newer platforms */ if (this is VLV or later platforms) { live_status = read_real_live_status(); } intel_hdmi_unset_edid(); intel_hdmi_set_edid(live_status); Now, my question is, do you want to remove live_status check for VLV and other platforms too ? or this is good enough ? >> >>>>>> intel_hdmi_unset_edid(connector); >>>>>> >>>>>> if (intel_hdmi_set_edid(connector, live_status)) { >>>>>> -- >>>>>> 1.9.1 >>>>> >>> >
On Thu, Mar 17, 2016 at 10:03:04PM +0530, Sharma, Shashank wrote: > Hey Chris, > I added comments for both Ville and you, please help me to understand this. > > Regards > Shashank > > On 3/17/2016 9:51 PM, Ville Syrjälä wrote: > > On Thu, Mar 17, 2016 at 09:35:30PM +0530, Sharma, Shashank wrote: > >> Regards > >> Shashank > >> > >> On 3/17/2016 9:31 PM, Ville Syrjälä wrote: > >>> On Thu, Mar 17, 2016 at 09:15:39PM +0530, Sharma, Shashank wrote: > >>>> Regards > >>>> Shashank > >>>> > >>>> On 3/17/2016 6:34 PM, Ville Syrjälä wrote: > >>>>> On Thu, Mar 17, 2016 at 01:29:25PM +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: 237ed86 "drm/i915: Check live status" > >>>>>> Author: Sonika Jindal <sonika.jindal@intel.com> > >>>>>> Date: Tue Sep 15 09:44:20 2015 +0530 > >>>>>> > >>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > >>>>>> --- > >>>>>> drivers/gpu/drm/i915/intel_hdmi.c | 22 ++++++++++++++-------- > >>>>>> 1 file changed, 14 insertions(+), 8 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > >>>>>> index e2dab48..d24d18a 100644 > >>>>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c > >>>>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c > >>>>>> @@ -1397,7 +1397,8 @@ 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); > >>>>>> - bool live_status = false; > >>>>>> + struct drm_device *dev = connector->dev; > >>>>>> + bool live_status = true; > >>>>>> unsigned int try; > >>>>>> > >>>>>> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > >>>>>> @@ -1405,16 +1406,21 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) > >>>>>> > >>>>>> intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); > >>>>>> > >>>>>> - for (try = 0; !live_status && try < 9; try++) { > >>>>>> - if (try) > >>>>>> - msleep(10); > >>>>>> - live_status = intel_digital_port_connected(dev_priv, > >>>>>> + /* > >>>>>> + * Live status check for HDMI detection is not very > >>>>>> + * reliable on older platforms. So insist the live > >>>>>> + * status check for EDID read from VLV onwards. > >>>>>> + */ > >>>>>> + if (INTEL_INFO(dev)->gen >= 7 && !IS_IVYBRIDGE(dev)) { > >>>>>> + for (try = 0; !live_status && try < 9; try++) { > >>>>>> + if (try) > >>>>>> + msleep(10); > >>>>>> + live_status = intel_digital_port_connected(dev_priv, > >>>>>> hdmi_to_dig_port(intel_hdmi)); > >>>>>> + } > >>>>>> + DRM_DEBUG_KMS("Live status %s\n", live_status ? "up" : "down"); > >>>>>> } > >>>>>> > >>>>>> - if (!live_status) > >>>>>> - DRM_DEBUG_KMS("Live status not up!"); > >>>>>> - > >>>>> > >>>>> As I said before, I think this whole thing could be solved with a simple > >>>>> two-liner here: > >>>>> > >>>>> + if (...) > >>>>> + live_status = true; > >>>>> > >>>> Yes I remember, and I replied on that already that why we would like to > >>>> keep the live status check. In fact I would be ok to remove this check > >>>> if you can suggest some other way of making this work for other > >>>> operating systems/sw platforms. > >>> > >>> My two liner would keep the check. > >>> > >> Sorry, I might have not understood you properly. > >> Do you mean: > >> if (INTEL_INFO(dev)->gen < 7 && IS_IVYBRIDGE(dev)) { > >> live_status = true; > >> } else { > >> do the same looping for retry; > >> } > > > > No, I mean > > > > { > > ... > > do loop; > > > > if (!live_status) > > DRM_DEBUG_KMS("Live status not up!"); > > > > + if (don't trust live status) > > + live_status = true; > > > > intel_hdmi_unset_edid(); > > > > if (intel_hdmi_set_edid(live_status)) { > > ... > > } > > > > > In fact, this is what I have done. > If you see the change in this patch, > /* Lets make live status true for < VLV platforms */ > bool live_status = true; > > blah.... > blah.... > > /* Check live status only for newer platforms */ > if (this is VLV or later platforms) { > live_status = read_real_live_status(); > } > intel_hdmi_unset_edid(); > intel_hdmi_set_edid(live_status); > > Now, my question is, do you want to remove live_status check for VLV and > other platforms too ? or this is good enough ? No, I'm objecting to changing the entire code when you could just add two lines. Also my way has the extra benefit that we keep the live status check mostly working on these presumed "broken" platforms. So the only difference to the current situation is that we would still attempt the EDID read even if live_status came out as false, but thanks to the extra delay from the live status polling we would hopefully avoid spurious detection results since the EDID read gets delayed a bit. > > >> > >>>>>> intel_hdmi_unset_edid(connector); > >>>>>> > >>>>>> if (intel_hdmi_set_edid(connector, live_status)) { > >>>>>> -- > >>>>>> 1.9.1 > >>>>> > >>> > >
On Thu, 17 Mar 2016, "Sharma, Shashank" <shashank.sharma@intel.com> wrote: > [ text/plain ] > Hey Chris, > I added comments for both Ville and you, please help me to understand this. > > Regards > Shashank > > On 3/17/2016 9:51 PM, Ville Syrjälä wrote: >> On Thu, Mar 17, 2016 at 09:35:30PM +0530, Sharma, Shashank wrote: >>> Regards >>> Shashank >>> >>> On 3/17/2016 9:31 PM, Ville Syrjälä wrote: >>>> On Thu, Mar 17, 2016 at 09:15:39PM +0530, Sharma, Shashank wrote: >>>>> Regards >>>>> Shashank >>>>> >>>>> On 3/17/2016 6:34 PM, Ville Syrjälä wrote: >>>>>> On Thu, Mar 17, 2016 at 01:29:25PM +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: 237ed86 "drm/i915: Check live status" >>>>>>> Author: Sonika Jindal <sonika.jindal@intel.com> >>>>>>> Date: Tue Sep 15 09:44:20 2015 +0530 >>>>>>> >>>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >>>>>>> --- >>>>>>> drivers/gpu/drm/i915/intel_hdmi.c | 22 ++++++++++++++-------- >>>>>>> 1 file changed, 14 insertions(+), 8 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c >>>>>>> index e2dab48..d24d18a 100644 >>>>>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c >>>>>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >>>>>>> @@ -1397,7 +1397,8 @@ 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); >>>>>>> - bool live_status = false; >>>>>>> + struct drm_device *dev = connector->dev; >>>>>>> + bool live_status = true; >>>>>>> unsigned int try; >>>>>>> >>>>>>> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", >>>>>>> @@ -1405,16 +1406,21 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) >>>>>>> >>>>>>> intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); >>>>>>> >>>>>>> - for (try = 0; !live_status && try < 9; try++) { >>>>>>> - if (try) >>>>>>> - msleep(10); >>>>>>> - live_status = intel_digital_port_connected(dev_priv, >>>>>>> + /* >>>>>>> + * Live status check for HDMI detection is not very >>>>>>> + * reliable on older platforms. So insist the live >>>>>>> + * status check for EDID read from VLV onwards. >>>>>>> + */ >>>>>>> + if (INTEL_INFO(dev)->gen >= 7 && !IS_IVYBRIDGE(dev)) { >>>>>>> + for (try = 0; !live_status && try < 9; try++) { >>>>>>> + if (try) >>>>>>> + msleep(10); >>>>>>> + live_status = intel_digital_port_connected(dev_priv, >>>>>>> hdmi_to_dig_port(intel_hdmi)); >>>>>>> + } >>>>>>> + DRM_DEBUG_KMS("Live status %s\n", live_status ? "up" : "down"); >>>>>>> } >>>>>>> >>>>>>> - if (!live_status) >>>>>>> - DRM_DEBUG_KMS("Live status not up!"); >>>>>>> - >>>>>> >>>>>> As I said before, I think this whole thing could be solved with a simple >>>>>> two-liner here: >>>>>> >>>>>> + if (...) >>>>>> + live_status = true; >>>>>> >>>>> Yes I remember, and I replied on that already that why we would like to >>>>> keep the live status check. In fact I would be ok to remove this check >>>>> if you can suggest some other way of making this work for other >>>>> operating systems/sw platforms. >>>> >>>> My two liner would keep the check. >>>> >>> Sorry, I might have not understood you properly. >>> Do you mean: >>> if (INTEL_INFO(dev)->gen < 7 && IS_IVYBRIDGE(dev)) { >>> live_status = true; >>> } else { >>> do the same looping for retry; >>> } >> >> No, I mean >> >> { >> ... >> do loop; >> >> if (!live_status) >> DRM_DEBUG_KMS("Live status not up!"); >> >> + if (don't trust live status) >> + live_status = true; >> >> intel_hdmi_unset_edid(); >> >> if (intel_hdmi_set_edid(live_status)) { >> ... >> } >> >> > In fact, this is what I have done. > If you see the change in this patch, Actually, not. You set live_status to true in the beginning, and your loop condition has !live_status, i.e. you won't check live status even once on any platform. Which is what I said in my review. BR, Jani. > /* Lets make live status true for < VLV platforms */ > bool live_status = true; > > blah.... > blah.... > > /* Check live status only for newer platforms */ > if (this is VLV or later platforms) { > live_status = read_real_live_status(); > } > intel_hdmi_unset_edid(); > intel_hdmi_set_edid(live_status); > > Now, my question is, do you want to remove live_status check for VLV and > other platforms too ? or this is good enough ? > >>> >>>>>>> intel_hdmi_unset_edid(connector); >>>>>>> >>>>>>> if (intel_hdmi_set_edid(connector, live_status)) { >>>>>>> -- >>>>>>> 1.9.1 >>>>>> >>>> >>
Regards Shashank On 3/17/2016 10:17 PM, Ville Syrjälä wrote: > On Thu, Mar 17, 2016 at 10:03:04PM +0530, Sharma, Shashank wrote: >> Hey Chris, >> I added comments for both Ville and you, please help me to understand this. >> >> Regards >> Shashank >> >> On 3/17/2016 9:51 PM, Ville Syrjälä wrote: >>> On Thu, Mar 17, 2016 at 09:35:30PM +0530, Sharma, Shashank wrote: >>>> Regards >>>> Shashank >>>> >>>> On 3/17/2016 9:31 PM, Ville Syrjälä wrote: >>>>> On Thu, Mar 17, 2016 at 09:15:39PM +0530, Sharma, Shashank wrote: >>>>>> Regards >>>>>> Shashank >>>>>> >>>>>> On 3/17/2016 6:34 PM, Ville Syrjälä wrote: >>>>>>> On Thu, Mar 17, 2016 at 01:29:25PM +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: 237ed86 "drm/i915: Check live status" >>>>>>>> Author: Sonika Jindal <sonika.jindal@intel.com> >>>>>>>> Date: Tue Sep 15 09:44:20 2015 +0530 >>>>>>>> >>>>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >>>>>>>> --- >>>>>>>> drivers/gpu/drm/i915/intel_hdmi.c | 22 ++++++++++++++-------- >>>>>>>> 1 file changed, 14 insertions(+), 8 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c >>>>>>>> index e2dab48..d24d18a 100644 >>>>>>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c >>>>>>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >>>>>>>> @@ -1397,7 +1397,8 @@ 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); >>>>>>>> - bool live_status = false; >>>>>>>> + struct drm_device *dev = connector->dev; >>>>>>>> + bool live_status = true; >>>>>>>> unsigned int try; >>>>>>>> >>>>>>>> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", >>>>>>>> @@ -1405,16 +1406,21 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) >>>>>>>> >>>>>>>> intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); >>>>>>>> >>>>>>>> - for (try = 0; !live_status && try < 9; try++) { >>>>>>>> - if (try) >>>>>>>> - msleep(10); >>>>>>>> - live_status = intel_digital_port_connected(dev_priv, >>>>>>>> + /* >>>>>>>> + * Live status check for HDMI detection is not very >>>>>>>> + * reliable on older platforms. So insist the live >>>>>>>> + * status check for EDID read from VLV onwards. >>>>>>>> + */ >>>>>>>> + if (INTEL_INFO(dev)->gen >= 7 && !IS_IVYBRIDGE(dev)) { >>>>>>>> + for (try = 0; !live_status && try < 9; try++) { >>>>>>>> + if (try) >>>>>>>> + msleep(10); >>>>>>>> + live_status = intel_digital_port_connected(dev_priv, >>>>>>>> hdmi_to_dig_port(intel_hdmi)); >>>>>>>> + } >>>>>>>> + DRM_DEBUG_KMS("Live status %s\n", live_status ? "up" : "down"); >>>>>>>> } >>>>>>>> >>>>>>>> - if (!live_status) >>>>>>>> - DRM_DEBUG_KMS("Live status not up!"); >>>>>>>> - >>>>>>> >>>>>>> As I said before, I think this whole thing could be solved with a simple >>>>>>> two-liner here: >>>>>>> >>>>>>> + if (...) >>>>>>> + live_status = true; >>>>>>> >>>>>> Yes I remember, and I replied on that already that why we would like to >>>>>> keep the live status check. In fact I would be ok to remove this check >>>>>> if you can suggest some other way of making this work for other >>>>>> operating systems/sw platforms. >>>>> >>>>> My two liner would keep the check. >>>>> >>>> Sorry, I might have not understood you properly. >>>> Do you mean: >>>> if (INTEL_INFO(dev)->gen < 7 && IS_IVYBRIDGE(dev)) { >>>> live_status = true; >>>> } else { >>>> do the same looping for retry; >>>> } >>> >>> No, I mean >>> >>> { >>> ... >>> do loop; >>> >>> if (!live_status) >>> DRM_DEBUG_KMS("Live status not up!"); >>> >>> + if (don't trust live status) >>> + live_status = true; >>> >>> intel_hdmi_unset_edid(); >>> >>> if (intel_hdmi_set_edid(live_status)) { >>> ... >>> } >>> >>> >> In fact, this is what I have done. >> If you see the change in this patch, >> /* Lets make live status true for < VLV platforms */ >> bool live_status = true; >> >> blah.... >> blah.... >> >> /* Check live status only for newer platforms */ >> if (this is VLV or later platforms) { >> live_status = read_real_live_status(); >> } >> intel_hdmi_unset_edid(); >> intel_hdmi_set_edid(live_status); >> >> Now, my question is, do you want to remove live_status check for VLV and >> other platforms too ? or this is good enough ? > > No, I'm objecting to changing the entire code when you could just add > two lines. Also my way has the extra benefit that we keep the live > status check mostly working on these presumed "broken" platforms. > > So the only difference to the current situation is that we would still > attempt the EDID read even if live_status came out as false, but thanks > to the extra delay from the live status polling we would hopefully > avoid spurious detection results since the EDID read gets delayed a bit. > Ok, thanks for this suggestion. I will do the change. I was thinking of adding a warning, if we are going to read the EDID without live status being set, sounds ok ? >> >>>> >>>>>>>> intel_hdmi_unset_edid(connector); >>>>>>>> >>>>>>>> if (intel_hdmi_set_edid(connector, live_status)) { >>>>>>>> -- >>>>>>>> 1.9.1 >>>>>>> >>>>> >>> >
Regards Shashank On 3/17/2016 11:29 PM, Jani Nikula wrote: > On Thu, 17 Mar 2016, "Sharma, Shashank" <shashank.sharma@intel.com> wrote: >> [ text/plain ] >> Hey Chris, >> I added comments for both Ville and you, please help me to understand this. >> >> Regards >> Shashank >> >> On 3/17/2016 9:51 PM, Ville Syrjälä wrote: >>> On Thu, Mar 17, 2016 at 09:35:30PM +0530, Sharma, Shashank wrote: >>>> Regards >>>> Shashank >>>> >>>> On 3/17/2016 9:31 PM, Ville Syrjälä wrote: >>>>> On Thu, Mar 17, 2016 at 09:15:39PM +0530, Sharma, Shashank wrote: >>>>>> Regards >>>>>> Shashank >>>>>> >>>>>> On 3/17/2016 6:34 PM, Ville Syrjälä wrote: >>>>>>> On Thu, Mar 17, 2016 at 01:29:25PM +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: 237ed86 "drm/i915: Check live status" >>>>>>>> Author: Sonika Jindal <sonika.jindal@intel.com> >>>>>>>> Date: Tue Sep 15 09:44:20 2015 +0530 >>>>>>>> >>>>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >>>>>>>> --- >>>>>>>> drivers/gpu/drm/i915/intel_hdmi.c | 22 ++++++++++++++-------- >>>>>>>> 1 file changed, 14 insertions(+), 8 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c >>>>>>>> index e2dab48..d24d18a 100644 >>>>>>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c >>>>>>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >>>>>>>> @@ -1397,7 +1397,8 @@ 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); >>>>>>>> - bool live_status = false; >>>>>>>> + struct drm_device *dev = connector->dev; >>>>>>>> + bool live_status = true; >>>>>>>> unsigned int try; >>>>>>>> >>>>>>>> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", >>>>>>>> @@ -1405,16 +1406,21 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) >>>>>>>> >>>>>>>> intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); >>>>>>>> >>>>>>>> - for (try = 0; !live_status && try < 9; try++) { >>>>>>>> - if (try) >>>>>>>> - msleep(10); >>>>>>>> - live_status = intel_digital_port_connected(dev_priv, >>>>>>>> + /* >>>>>>>> + * Live status check for HDMI detection is not very >>>>>>>> + * reliable on older platforms. So insist the live >>>>>>>> + * status check for EDID read from VLV onwards. >>>>>>>> + */ >>>>>>>> + if (INTEL_INFO(dev)->gen >= 7 && !IS_IVYBRIDGE(dev)) { >>>>>>>> + for (try = 0; !live_status && try < 9; try++) { >>>>>>>> + if (try) >>>>>>>> + msleep(10); >>>>>>>> + live_status = intel_digital_port_connected(dev_priv, >>>>>>>> hdmi_to_dig_port(intel_hdmi)); >>>>>>>> + } >>>>>>>> + DRM_DEBUG_KMS("Live status %s\n", live_status ? "up" : "down"); >>>>>>>> } >>>>>>>> >>>>>>>> - if (!live_status) >>>>>>>> - DRM_DEBUG_KMS("Live status not up!"); >>>>>>>> - >>>>>>> >>>>>>> As I said before, I think this whole thing could be solved with a simple >>>>>>> two-liner here: >>>>>>> >>>>>>> + if (...) >>>>>>> + live_status = true; >>>>>>> >>>>>> Yes I remember, and I replied on that already that why we would like to >>>>>> keep the live status check. In fact I would be ok to remove this check >>>>>> if you can suggest some other way of making this work for other >>>>>> operating systems/sw platforms. >>>>> >>>>> My two liner would keep the check. >>>>> >>>> Sorry, I might have not understood you properly. >>>> Do you mean: >>>> if (INTEL_INFO(dev)->gen < 7 && IS_IVYBRIDGE(dev)) { >>>> live_status = true; >>>> } else { >>>> do the same looping for retry; >>>> } >>> >>> No, I mean >>> >>> { >>> ... >>> do loop; >>> >>> if (!live_status) >>> DRM_DEBUG_KMS("Live status not up!"); >>> >>> + if (don't trust live status) >>> + live_status = true; >>> >>> intel_hdmi_unset_edid(); >>> >>> if (intel_hdmi_set_edid(live_status)) { >>> ... >>> } >>> >>> >> In fact, this is what I have done. >> If you see the change in this patch, > > Actually, not. You set live_status to true in the beginning, and your > loop condition has !live_status, i.e. you won't check live status even > once on any platform. Which is what I said in my review. > > BR, > Jani. Ah.. damn. While splitting the patch into two, I missed this part. Thanks for pointing this out. > > >> /* Lets make live status true for < VLV platforms */ >> bool live_status = true; >> >> blah.... >> blah.... >> >> /* Check live status only for newer platforms */ >> if (this is VLV or later platforms) { >> live_status = read_real_live_status(); >> } >> intel_hdmi_unset_edid(); >> intel_hdmi_set_edid(live_status); >> >> Now, my question is, do you want to remove live_status check for VLV and >> other platforms too ? or this is good enough ? >> >>>> >>>>>>>> intel_hdmi_unset_edid(connector); >>>>>>>> >>>>>>>> if (intel_hdmi_set_edid(connector, live_status)) { >>>>>>>> -- >>>>>>>> 1.9.1 >>>>>>> >>>>> >>> >
On Fri, Mar 18, 2016 at 08:51:29AM +0530, Sharma, Shashank wrote: > Regards > Shashank > > On 3/17/2016 10:17 PM, Ville Syrjälä wrote: > > On Thu, Mar 17, 2016 at 10:03:04PM +0530, Sharma, Shashank wrote: > >> Hey Chris, > >> I added comments for both Ville and you, please help me to understand this. > >> > >> Regards > >> Shashank > >> > >> On 3/17/2016 9:51 PM, Ville Syrjälä wrote: > >>> On Thu, Mar 17, 2016 at 09:35:30PM +0530, Sharma, Shashank wrote: > >>>> Regards > >>>> Shashank > >>>> > >>>> On 3/17/2016 9:31 PM, Ville Syrjälä wrote: > >>>>> On Thu, Mar 17, 2016 at 09:15:39PM +0530, Sharma, Shashank wrote: > >>>>>> Regards > >>>>>> Shashank > >>>>>> > >>>>>> On 3/17/2016 6:34 PM, Ville Syrjälä wrote: > >>>>>>> On Thu, Mar 17, 2016 at 01:29:25PM +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: 237ed86 "drm/i915: Check live status" > >>>>>>>> Author: Sonika Jindal <sonika.jindal@intel.com> > >>>>>>>> Date: Tue Sep 15 09:44:20 2015 +0530 > >>>>>>>> > >>>>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > >>>>>>>> --- > >>>>>>>> drivers/gpu/drm/i915/intel_hdmi.c | 22 ++++++++++++++-------- > >>>>>>>> 1 file changed, 14 insertions(+), 8 deletions(-) > >>>>>>>> > >>>>>>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > >>>>>>>> index e2dab48..d24d18a 100644 > >>>>>>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c > >>>>>>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c > >>>>>>>> @@ -1397,7 +1397,8 @@ 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); > >>>>>>>> - bool live_status = false; > >>>>>>>> + struct drm_device *dev = connector->dev; > >>>>>>>> + bool live_status = true; > >>>>>>>> unsigned int try; > >>>>>>>> > >>>>>>>> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > >>>>>>>> @@ -1405,16 +1406,21 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) > >>>>>>>> > >>>>>>>> intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); > >>>>>>>> > >>>>>>>> - for (try = 0; !live_status && try < 9; try++) { > >>>>>>>> - if (try) > >>>>>>>> - msleep(10); > >>>>>>>> - live_status = intel_digital_port_connected(dev_priv, > >>>>>>>> + /* > >>>>>>>> + * Live status check for HDMI detection is not very > >>>>>>>> + * reliable on older platforms. So insist the live > >>>>>>>> + * status check for EDID read from VLV onwards. > >>>>>>>> + */ > >>>>>>>> + if (INTEL_INFO(dev)->gen >= 7 && !IS_IVYBRIDGE(dev)) { > >>>>>>>> + for (try = 0; !live_status && try < 9; try++) { > >>>>>>>> + if (try) > >>>>>>>> + msleep(10); > >>>>>>>> + live_status = intel_digital_port_connected(dev_priv, > >>>>>>>> hdmi_to_dig_port(intel_hdmi)); > >>>>>>>> + } > >>>>>>>> + DRM_DEBUG_KMS("Live status %s\n", live_status ? "up" : "down"); > >>>>>>>> } > >>>>>>>> > >>>>>>>> - if (!live_status) > >>>>>>>> - DRM_DEBUG_KMS("Live status not up!"); > >>>>>>>> - > >>>>>>> > >>>>>>> As I said before, I think this whole thing could be solved with a simple > >>>>>>> two-liner here: > >>>>>>> > >>>>>>> + if (...) > >>>>>>> + live_status = true; > >>>>>>> > >>>>>> Yes I remember, and I replied on that already that why we would like to > >>>>>> keep the live status check. In fact I would be ok to remove this check > >>>>>> if you can suggest some other way of making this work for other > >>>>>> operating systems/sw platforms. > >>>>> > >>>>> My two liner would keep the check. > >>>>> > >>>> Sorry, I might have not understood you properly. > >>>> Do you mean: > >>>> if (INTEL_INFO(dev)->gen < 7 && IS_IVYBRIDGE(dev)) { > >>>> live_status = true; > >>>> } else { > >>>> do the same looping for retry; > >>>> } > >>> > >>> No, I mean > >>> > >>> { > >>> ... > >>> do loop; > >>> > >>> if (!live_status) > >>> DRM_DEBUG_KMS("Live status not up!"); > >>> > >>> + if (don't trust live status) > >>> + live_status = true; > >>> > >>> intel_hdmi_unset_edid(); > >>> > >>> if (intel_hdmi_set_edid(live_status)) { > >>> ... > >>> } > >>> > >>> > >> In fact, this is what I have done. > >> If you see the change in this patch, > >> /* Lets make live status true for < VLV platforms */ > >> bool live_status = true; > >> > >> blah.... > >> blah.... > >> > >> /* Check live status only for newer platforms */ > >> if (this is VLV or later platforms) { > >> live_status = read_real_live_status(); > >> } > >> intel_hdmi_unset_edid(); > >> intel_hdmi_set_edid(live_status); > >> > >> Now, my question is, do you want to remove live_status check for VLV and > >> other platforms too ? or this is good enough ? > > > > No, I'm objecting to changing the entire code when you could just add > > two lines. Also my way has the extra benefit that we keep the live > > status check mostly working on these presumed "broken" platforms. > > > > So the only difference to the current situation is that we would still > > attempt the EDID read even if live_status came out as false, but thanks > > to the extra delay from the live status polling we would hopefully > > avoid spurious detection results since the EDID read gets delayed a bit. > > > Ok, thanks for this suggestion. I will do the change. > I was thinking of adding a warning, if we are going to read the EDID > without live status being set, sounds ok ? Yeah, adding a debug message might be prudent. > > >> > >>>> > >>>>>>>> intel_hdmi_unset_edid(connector); > >>>>>>>> > >>>>>>>> if (intel_hdmi_set_edid(connector, live_status)) { > >>>>>>>> -- > >>>>>>>> 1.9.1 > >>>>>>> > >>>>> > >>> > >
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index e2dab48..d24d18a 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1397,7 +1397,8 @@ 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); - bool live_status = false; + struct drm_device *dev = connector->dev; + bool live_status = true; unsigned int try; DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", @@ -1405,16 +1406,21 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); - for (try = 0; !live_status && try < 9; try++) { - if (try) - msleep(10); - live_status = intel_digital_port_connected(dev_priv, + /* + * Live status check for HDMI detection is not very + * reliable on older platforms. So insist the live + * status check for EDID read from VLV onwards. + */ + if (INTEL_INFO(dev)->gen >= 7 && !IS_IVYBRIDGE(dev)) { + for (try = 0; !live_status && try < 9; try++) { + if (try) + msleep(10); + live_status = intel_digital_port_connected(dev_priv, hdmi_to_dig_port(intel_hdmi)); + } + DRM_DEBUG_KMS("Live status %s\n", live_status ? "up" : "down"); } - if (!live_status) - DRM_DEBUG_KMS("Live status not up!"); - 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: 237ed86 "drm/i915: Check live status" Author: Sonika Jindal <sonika.jindal@intel.com> Date: Tue Sep 15 09:44:20 2015 +0530 Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> --- drivers/gpu/drm/i915/intel_hdmi.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)