[Intel-gfx] [PATCH] drm/i915: Restrict usage of live status check

[Ville Syrjälä]

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 at intel.com>
> >>>>>> 	Date:   Tue Sep 15 09:44:20 2015 +0530
> >>>>>>
> >>>>>> Signed-off-by: Shashank Sharma <shashank.sharma at 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
> >>>>>
> >>>
> >

-- 
Ville Syrjälä
Intel OTC