Re: [PATCH] drm/i915: Restrict usage of live status check

["Sharma, Shashank" <shashank.sharma@intel.com>]

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
>>>>>>>
>>>>>
>>>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx