| Message ID | 1461154371-1292-1-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Apr 20, 2016 at 05:42:51PM +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 regression added by patch: > 'commit 237ed86c693d ("drm/i915: Check live status before reading edid")' > > === > V2 > === > Ville: > - Add live_status = true in the end of delay loop. > > Jani: > - Initialize bool live_status with false; > - Change regression reference in the commit message. > - Use dev_priv instead of dev > - Move declaration of try at the loop > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > --- > drivers/gpu/drm/i915/intel_hdmi.c | 31 ++++++++++++++++++++++++------- > 1 file changed, 24 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index b199ede..98f5ae0 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -1398,22 +1398,39 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) > 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; > > 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); > - 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_priv)->gen >= 7 && !IS_IVYBRIDGE(dev_priv)) { > + unsigned int try; > + > + 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)); > + } > } > > - if (!live_status) > - DRM_DEBUG_KMS("Live status not up!"); > + if (!live_status) { > + /* > + * TODO: Replace this with some magic code. > + * Even after giving enough delay, live status is not up. > + * Lets read EDID and decide if HDMI is available. We > + * have to do this not to break old platforms, but ideally > + * should read EDID only when live_status reg allows us. > + */ > + DRM_DEBUG_KMS("Live status down, making it up\n"); > + live_status = true; > + } Hmm. This is still not what I suggested earlier. Was there some problem with my idea? > > intel_hdmi_unset_edid(connector); > > -- > 1.9.1
On 4/20/2016 8:18 PM, Ville Syrjälä wrote: > On Wed, Apr 20, 2016 at 05:42:51PM +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 regression added by patch: >> 'commit 237ed86c693d ("drm/i915: Check live status before reading edid")' >> >> === >> V2 >> === >> Ville: >> - Add live_status = true in the end of delay loop. >> >> Jani: >> - Initialize bool live_status with false; >> - Change regression reference in the commit message. >> - Use dev_priv instead of dev >> - Move declaration of try at the loop >> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >> --- >> drivers/gpu/drm/i915/intel_hdmi.c | 31 ++++++++++++++++++++++++------- >> 1 file changed, 24 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c >> index b199ede..98f5ae0 100644 >> --- a/drivers/gpu/drm/i915/intel_hdmi.c >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >> @@ -1398,22 +1398,39 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) >> 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; >> >> 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); >> - 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_priv)->gen >= 7 && !IS_IVYBRIDGE(dev_priv)) { >> + unsigned int try; >> + >> + 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)); >> + } >> } >> >> - if (!live_status) >> - DRM_DEBUG_KMS("Live status not up!"); >> + if (!live_status) { >> + /* >> + * TODO: Replace this with some magic code. >> + * Even after giving enough delay, live status is not up. >> + * Lets read EDID and decide if HDMI is available. We >> + * have to do this not to break old platforms, but ideally >> + * should read EDID only when live_status reg allows us. >> + */ >> + DRM_DEBUG_KMS("Live status down, making it up\n"); >> + live_status = true; >> + } > > Hmm. This is still not what I suggested earlier. Was there some problem > with my idea? > Not at all, in fact I thought I was doing just as you told. I guess I still missed something, please help me to understand what was that. our last discussion was: Ville: Sample code: if (don't trust live status) live_status = true; Shashank: Now, my question is, do you want to remove live_status check for VLV and other platforms too ? or this is good enough ? Ville: "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" Did you mean to add a kernel command-line flag type stuff for 'don't trust live status' or just gen_check was enough ? Regards Shashank >> >> intel_hdmi_unset_edid(connector); >> >> -- >> 1.9.1 >
Hi Ville, Please ignore my last email. I think I finally got it what you exactly suggested. Please check another patch sent by me: drm/i915: Fake HDMI live status There are two versions of this patch: v1: fake live_status = true for all platforms. v2: fake live_status = true only for (< gen7 and IVB platforms). please suggest which one will you prefer to go as a fix. Regards Shashank On 4/21/2016 9:25 AM, Sharma, Shashank wrote: > On 4/20/2016 8:18 PM, Ville Syrjälä wrote: >> On Wed, Apr 20, 2016 at 05:42:51PM +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 regression added by patch: >>> 'commit 237ed86c693d ("drm/i915: Check live status before reading >>> edid")' >>> >>> === >>> V2 >>> === >>> Ville: >>> - Add live_status = true in the end of delay loop. >>> >>> Jani: >>> - Initialize bool live_status with false; >>> - Change regression reference in the commit message. >>> - Use dev_priv instead of dev >>> - Move declaration of try at the loop >>> >>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_hdmi.c | 31 ++++++++++++++++++++++++------- >>> 1 file changed, 24 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c >>> b/drivers/gpu/drm/i915/intel_hdmi.c >>> index b199ede..98f5ae0 100644 >>> --- a/drivers/gpu/drm/i915/intel_hdmi.c >>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >>> @@ -1398,22 +1398,39 @@ intel_hdmi_detect(struct drm_connector >>> *connector, bool force) >>> 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; >>> >>> 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); >>> - 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_priv)->gen >= 7 && !IS_IVYBRIDGE(dev_priv)) { >>> + unsigned int try; >>> + >>> + 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)); >>> + } >>> } >>> >>> - if (!live_status) >>> - DRM_DEBUG_KMS("Live status not up!"); >>> + if (!live_status) { >>> + /* >>> + * TODO: Replace this with some magic code. >>> + * Even after giving enough delay, live status is not up. >>> + * Lets read EDID and decide if HDMI is available. We >>> + * have to do this not to break old platforms, but ideally >>> + * should read EDID only when live_status reg allows us. >>> + */ >>> + DRM_DEBUG_KMS("Live status down, making it up\n"); >>> + live_status = true; >>> + } >> >> Hmm. This is still not what I suggested earlier. Was there some problem >> with my idea? >> > Not at all, in fact I thought I was doing just as you told. I guess I > still missed something, please help me to understand what was that. > our last discussion was: > > Ville: Sample code: > if (don't trust live status) > live_status = true; > > Shashank: Now, my question is, do you want to remove live_status check for > VLV and other platforms too ? or this is good enough ? > > Ville: > "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" > > Did you mean to add a kernel command-line flag type stuff for 'don't > trust live status' or just gen_check was enough ? > > Regards > Shashank >>> >>> intel_hdmi_unset_edid(connector); >>> >>> -- >>> 1.9.1 >>
=== V2 === Ville: - Add live_status = true in the end of delay loop. Jani: - Initialize bool live_status with false; - Change regression reference in the commit message. - Use dev_priv instead of dev - Move declaration of try at the loop Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> --- drivers/gpu/drm/i915/intel_hdmi.c | 31 ++++++++++++++++++++++++------- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index b199ede..98f5ae0 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1398,22 +1398,39 @@ intel_hdmi_detect(struct drm_connector *connector, bool force) 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; 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); - 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_priv)->gen >= 7 && !IS_IVYBRIDGE(dev_priv)) { + unsigned int try; + + 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)); + } } - if (!live_status) - DRM_DEBUG_KMS("Live status not up!"); + if (!live_status) { + /* + * TODO: Replace this with some magic code. + * Even after giving enough delay, live status is not up. + * Lets read EDID and decide if HDMI is available. We + * have to do this not to break old platforms, but ideally + * should read EDID only when live_status reg allows us. + */ + DRM_DEBUG_KMS("Live status down, making it up\n"); + live_status = true; + } intel_hdmi_unset_edid(connector);
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 regression added by patch: 'commit 237ed86c693d ("drm/i915: Check live status before reading edid")'