| Message ID | 1457609747-21577-1-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 10, 2016 at 05:05:47PM +0530, Shashank Sharma wrote: > This patch does the following: > - 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 > doesn't work reliably on some older devices. So limit the > live_status check for HDMI detection, only for platforms > from gen7 onwards. Not good enough. We have regressions reported on IVB at least. Also I'm not convinced more recent platforms are any necessarily better here. > - Cleans up the retry logic. > There is an extra 'if (try)' check, which can be avoided by > changing the logic slightly, keeping the delay as same(80ms) I wouldn't call a goto loop clean. > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > --- > drivers/gpu/drm/i915/intel_hdmi.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index e2dab48..3f6db91 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -1397,24 +1397,31 @@ 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; > - unsigned int try; > + bool live_status = true; > + unsigned int try = 9; > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > connector->base.id, connector->name); > > intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); > > - for (try = 0; !live_status && try < 9; try++) { > - if (try) > - msleep(10); > + /* > + * When tested with specific set of monitors + cables, > + * live status behavior is not very consistent for older > + * platforms. So add live status check from GEN7 > + * onwards. > + */ > + if (INTEL_INFO(dev_priv->dev)->gen >= 7) { > +retry_ls: > live_status = intel_digital_port_connected(dev_priv, > hdmi_to_dig_port(intel_hdmi)); > + if (!live_status && --try) { > + msleep(10); > + goto retry_ls; > + } > + 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)) { > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Thanks Ville, Regards Shashank On 3/10/2016 5:10 PM, Ville Syrjälä wrote: > On Thu, Mar 10, 2016 at 05:05:47PM +0530, Shashank Sharma wrote: >> This patch does the following: >> - 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 >> doesn't work reliably on some older devices. So limit the >> live_status check for HDMI detection, only for platforms >> from gen7 onwards. > > Not good enough. We have regressions reported on IVB at least. > Also I'm not convinced more recent platforms are any necessarily > better here. Thanks for pointing out the miss for IVB. I will add skip for that. We have tested VLV, CHT SKL and BXT with this code, so we should be good with those. Anything particular in your mind ? > >> - Cleans up the retry logic. >> There is an extra 'if (try)' check, which can be avoided by >> changing the logic slightly, keeping the delay as same(80ms) > > I wouldn't call a goto loop clean. > I know, its flushing my pipeline, and its not the best thing to do. But makes code more readable, and removes one (if) condition, which was getting executed for every run of the loop. So I guess still some CPU cycles saved in the end :) >> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >> --- >> drivers/gpu/drm/i915/intel_hdmi.c | 23 +++++++++++++++-------- >> 1 file changed, 15 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c >> index e2dab48..3f6db91 100644 >> --- a/drivers/gpu/drm/i915/intel_hdmi.c >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >> @@ -1397,24 +1397,31 @@ 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; >> - unsigned int try; >> + bool live_status = true; >> + unsigned int try = 9; >> >> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", >> connector->base.id, connector->name); >> >> intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); >> >> - for (try = 0; !live_status && try < 9; try++) { >> - if (try) >> - msleep(10); >> + /* >> + * When tested with specific set of monitors + cables, >> + * live status behavior is not very consistent for older >> + * platforms. So add live status check from GEN7 >> + * onwards. >> + */ >> + if (INTEL_INFO(dev_priv->dev)->gen >= 7) { >> +retry_ls: >> live_status = intel_digital_port_connected(dev_priv, >> hdmi_to_dig_port(intel_hdmi)); >> + if (!live_status && --try) { >> + msleep(10); >> + goto retry_ls; >> + } >> + 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)) { >> -- >> 1.9.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Thu, Mar 10, 2016 at 06:12:32PM +0530, Sharma, Shashank wrote: > Thanks Ville, > > Regards > Shashank > > On 3/10/2016 5:10 PM, Ville Syrjälä wrote: > > On Thu, Mar 10, 2016 at 05:05:47PM +0530, Shashank Sharma wrote: > >> This patch does the following: > >> - 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 > >> doesn't work reliably on some older devices. So limit the > >> live_status check for HDMI detection, only for platforms > >> from gen7 onwards. > > > > Not good enough. We have regressions reported on IVB at least. > > Also I'm not convinced more recent platforms are any necessarily > > better here. > Thanks for pointing out the miss for IVB. I will add skip for that. > We have tested VLV, CHT SKL and BXT with this code, so we should be good > with those. Anything particular in your mind ? I'm concerned it's more about the displays/cables than about the source. I don't think we had any reports so far where people would have been able to test a bad sink/cable with multiple platforms, and found some working and some not. > > > >> - Cleans up the retry logic. > >> There is an extra 'if (try)' check, which can be avoided by > >> changing the logic slightly, keeping the delay as same(80ms) > > > > I wouldn't call a goto loop clean. > > > I know, its flushing my pipeline, and its not the best thing to do. But > makes code more readable, and removes one (if) condition, which was > getting executed for every run of the loop. So I guess still some CPU > cycles saved in the end :) Any kind of real loop would be more clear than a goto. Though we did use some while/do while thing originally I think, and that was messy in other ways. Pn option would of course be to unroll the first iteration out from the loop so that the 'if' would disappear from the loop. In any case, this sort of stuff should be a separate patch. > >> > >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_hdmi.c | 23 +++++++++++++++-------- > >> 1 file changed, 15 insertions(+), 8 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > >> index e2dab48..3f6db91 100644 > >> --- a/drivers/gpu/drm/i915/intel_hdmi.c > >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c > >> @@ -1397,24 +1397,31 @@ 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; > >> - unsigned int try; > >> + bool live_status = true; > >> + unsigned int try = 9; > >> > >> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", > >> connector->base.id, connector->name); > >> > >> intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); > >> > >> - for (try = 0; !live_status && try < 9; try++) { > >> - if (try) > >> - msleep(10); > >> + /* > >> + * When tested with specific set of monitors + cables, > >> + * live status behavior is not very consistent for older > >> + * platforms. So add live status check from GEN7 > >> + * onwards. > >> + */ > >> + if (INTEL_INFO(dev_priv->dev)->gen >= 7) { > >> +retry_ls: > >> live_status = intel_digital_port_connected(dev_priv, > >> hdmi_to_dig_port(intel_hdmi)); > >> + if (!live_status && --try) { > >> + msleep(10); > >> + goto retry_ls; > >> + } > >> + 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)) { > >> -- > >> 1.9.1 > >> > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
Regards Shashank On 3/10/2016 6:37 PM, Ville Syrjälä wrote: > On Thu, Mar 10, 2016 at 06:12:32PM +0530, Sharma, Shashank wrote: >> Thanks Ville, >> >> Regards >> Shashank >> >> On 3/10/2016 5:10 PM, Ville Syrjälä wrote: >>> On Thu, Mar 10, 2016 at 05:05:47PM +0530, Shashank Sharma wrote: >>>> This patch does the following: >>>> - 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 >>>> doesn't work reliably on some older devices. So limit the >>>> live_status check for HDMI detection, only for platforms >>>> from gen7 onwards. >>> >>> Not good enough. We have regressions reported on IVB at least. >>> Also I'm not convinced more recent platforms are any necessarily >>> better here. >> Thanks for pointing out the miss for IVB. I will add skip for that. >> We have tested VLV, CHT SKL and BXT with this code, so we should be good >> with those. Anything particular in your mind ? > > I'm concerned it's more about the displays/cables than about the source. > I don't think we had any reports so far where people would have been > able to test a bad sink/cable with multiple platforms, and found some > working and some not. > I agree, that it did seem to work fine till now. But the reason behind that was, we were probing EDID on every instance of detect(with out without interrupt), and it was hitting one or other time. So these issues were always there, but were hidden behind bugs in our driver. Also, for newer platforms, live status check is a *must* as per bspec. If we dont do this, we will see improper hdmi_detect() replies for consecutive connector_probe, just like chrome and android is facing now. Passing compliance is again other concern. IMHO, Now, when our driver is used between commercial/production devices and open source community, we have to let a cable/monitor vendor take the credit of the bug. Carrying this forward is gonna hit our performance and HDMI compliance certification. >>> >>>> - Cleans up the retry logic. >>>> There is an extra 'if (try)' check, which can be avoided by >>>> changing the logic slightly, keeping the delay as same(80ms) >>> >>> I wouldn't call a goto loop clean. >>> >> I know, its flushing my pipeline, and its not the best thing to do. But >> makes code more readable, and removes one (if) condition, which was >> getting executed for every run of the loop. So I guess still some CPU >> cycles saved in the end :) > > Any kind of real loop would be more clear than a goto. Though we did use > some while/do while thing originally I think, and that was messy in > other ways. Pn option would of course be to unroll the first iteration > out from the loop so that the 'if' would disappear from the loop. Yes, This will add code duplication, as I will be adding the same line after and before loop. Typical embedded system dilemma, space vs time :) > > In any case, this sort of stuff should be a separate patch. Agree, I can add another patch for this. > >>>> >>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_hdmi.c | 23 +++++++++++++++-------- >>>> 1 file changed, 15 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c >>>> index e2dab48..3f6db91 100644 >>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c >>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >>>> @@ -1397,24 +1397,31 @@ 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; >>>> - unsigned int try; >>>> + bool live_status = true; >>>> + unsigned int try = 9; >>>> >>>> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", >>>> connector->base.id, connector->name); >>>> >>>> intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); >>>> >>>> - for (try = 0; !live_status && try < 9; try++) { >>>> - if (try) >>>> - msleep(10); >>>> + /* >>>> + * When tested with specific set of monitors + cables, >>>> + * live status behavior is not very consistent for older >>>> + * platforms. So add live status check from GEN7 >>>> + * onwards. >>>> + */ >>>> + if (INTEL_INFO(dev_priv->dev)->gen >= 7) { >>>> +retry_ls: >>>> live_status = intel_digital_port_connected(dev_priv, >>>> hdmi_to_dig_port(intel_hdmi)); >>>> + if (!live_status && --try) { >>>> + msleep(10); >>>> + goto retry_ls; >>>> + } >>>> + 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)) { >>>> -- >>>> 1.9.1 >>>> >>>> _______________________________________________ >>>> Intel-gfx mailing list >>>> Intel-gfx@lists.freedesktop.org >>>> https://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 e2dab48..3f6db91 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1397,24 +1397,31 @@ 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; - unsigned int try; + bool live_status = true; + unsigned int try = 9; DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id, connector->name); intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS); - for (try = 0; !live_status && try < 9; try++) { - if (try) - msleep(10); + /* + * When tested with specific set of monitors + cables, + * live status behavior is not very consistent for older + * platforms. So add live status check from GEN7 + * onwards. + */ + if (INTEL_INFO(dev_priv->dev)->gen >= 7) { +retry_ls: live_status = intel_digital_port_connected(dev_priv, hdmi_to_dig_port(intel_hdmi)); + if (!live_status && --try) { + msleep(10); + goto retry_ls; + } + 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 does the following: - 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 doesn't work reliably on some older devices. So limit the live_status check for HDMI detection, only for platforms from gen7 onwards. - Cleans up the retry logic. There is an extra 'if (try)' check, which can be avoided by changing the logic slightly, keeping the delay as same(80ms) Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> --- drivers/gpu/drm/i915/intel_hdmi.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)