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

[Sharma, Shashank]

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 at 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
>