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

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

[Shashank Sharma]

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!");
-
 	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

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

[Jani Nikula]

On Thu, 17 Mar 2016, Shashank Sharma <shashank.sharma@intel.com> wrote:
> [ text/plain ]
> 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
>

Fixes: 237ed86c693d ("drm/i915: Check live status before reading edid")
Cc: stable@vger.kernel.org # v4.4
Bugzilla: ?

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

This is unnecessary.

> +	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)) {

Replace dev with dev_priv. Both work, it's magic, but we prefer using
dev_priv where possible.

Move try variable declaration here.

> +		for (try = 0; !live_status && try < 9; try++) {

You'll now ignore live_status on all platforms. (But please do not turn
this into a do-while, keep it as a for loop.)

> +			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");

Consider using onoff(live_status) instead of 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)) {

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Sharma, Shashank]

Regards
Shashank

On 3/17/2016 2:55 PM, Jani Nikula wrote:
> On Thu, 17 Mar 2016, Shashank Sharma <shashank.sharma@intel.com> wrote:
>> [ text/plain ]
>> 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
>>
>
> Fixes: 237ed86c693d ("drm/i915: Check live status before reading edid")
> Cc: stable@vger.kernel.org # v4.4
> Bugzilla: ?
>
Let me dig for it ... Do you have a link ? If yes, please share.
>> 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;
>
> This is unnecessary.
>
>> +	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)) {
>
> Replace dev with dev_priv. Both work, it's magic, but we prefer using
> dev_priv where possible.
>
Interesting, will try that out.
> Move try variable declaration here.
>
Got it.
>> +		for (try = 0; !live_status && try < 9; try++) {
>
> You'll now ignore live_status on all platforms. (But please do not turn
> this into a do-while, keep it as a for loop.)
>
>> +			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");
>
> Consider using onoff(live_status) instead of live_status ? "up" :
> "down".
>
Sounds cool, I dint know about onoff yet, will check.
>>   	}
>>
>> -	if (!live_status)
>> -		DRM_DEBUG_KMS("Live status not up!");
>> -
>>   	intel_hdmi_unset_edid(connector);
>>
>>   	if (intel_hdmi_set_edid(connector, live_status)) {
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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;

>  	intel_hdmi_unset_edid(connector);
>  
>  	if (intel_hdmi_set_edid(connector, live_status)) {
> -- 
> 1.9.1

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915: Restrict usage of live status check (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Restrict usage of live status check (rev3)
URL   : https://patchwork.freedesktop.org/series/4297/
State : failure

== Summary ==

Series 4297v3 drm/i915: Restrict usage of live status check
http://patchwork.freedesktop.org/api/1.0/series/4297/revisions/3/mbox/

Test gem_ringfill:
        Subgroup basic-default-s3:
                dmesg-warn -> PASS       (skl-nuci5)
Test gem_storedw_loop:
        Subgroup basic-default:
                dmesg-warn -> PASS       (skl-nuci5)
Test gem_sync:
        Subgroup basic-vebox:
                dmesg-warn -> PASS       (skl-nuci5)
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-b:
                pass       -> DMESG-WARN (snb-x220t)
        Subgroup read-crc-pipe-b-frame-sequence:
                dmesg-warn -> PASS       (snb-x220t)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                fail       -> DMESG-FAIL (snb-x220t)

bdw-nuci7        total:194  pass:181  dwarn:1   dfail:0   fail:0   skip:12 
bdw-ultra        total:194  pass:172  dwarn:1   dfail:0   fail:0   skip:21 
byt-nuc          total:194  pass:155  dwarn:4   dfail:0   fail:0   skip:35 
ivb-t430s        total:194  pass:168  dwarn:1   dfail:0   fail:0   skip:25 
skl-i5k-2        total:194  pass:170  dwarn:1   dfail:0   fail:0   skip:23 
skl-i7k-2        total:194  pass:170  dwarn:1   dfail:0   fail:0   skip:23 
skl-nuci5        total:194  pass:182  dwarn:1   dfail:0   fail:0   skip:11 
snb-x220t        total:194  pass:158  dwarn:2   dfail:1   fail:0   skip:33 

Results at /archive/results/CI_IGT_test/Patchwork_1629/

10e913a48ca36790da9b58bed8729598ea79ebdb drm-intel-nightly: 2016y-03m-17d-13h-22m-41s UTC integration manifest
d4dd91856267b81c8ac5bbaab7a24be5ac54a282 drm/i915: Restrict usage of live status check

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Sharma, Shashank]

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

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

[Chris Wilson]

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.

The question is what is the safest approach for stable@ and for that
just doing as we historically did and ignore live_status makes the most
sense. Once the regressions have been rectified, then you can decide how
best to approach -next.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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.

> >>   	intel_hdmi_unset_edid(connector);
> >>
> >>   	if (intel_hdmi_set_edid(connector, live_status)) {
> >> --
> >> 1.9.1
> >

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Sharma, Shashank]

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;
	}

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

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

[Ville Syrjälä]

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)) {
	...
}


> 
> >>>>    	intel_hdmi_unset_edid(connector);
> >>>>
> >>>>    	if (intel_hdmi_set_edid(connector, live_status)) {
> >>>> --
> >>>> 1.9.1
> >>>
> >

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Sharma, Shashank]

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

Re: [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@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.

> 	
> >>
> >>>>>>     	intel_hdmi_unset_edid(connector);
> >>>>>>
> >>>>>>     	if (intel_hdmi_set_edid(connector, live_status)) {
> >>>>>> --
> >>>>>> 1.9.1
> >>>>>
> >>>
> >

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

On Thu, 17 Mar 2016, "Sharma, Shashank" <shashank.sharma@intel.com> wrote:
> [ text/plain ]
> 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,

Actually, not. You set live_status to true in the beginning, and your
loop condition has !live_status, i.e. you won't check live status even
once on any platform. Which is what I said in my review.

BR,
Jani.


> 	/* 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 ?
> 	
>>>
>>>>>>>     	intel_hdmi_unset_edid(connector);
>>>>>>>
>>>>>>>     	if (intel_hdmi_set_edid(connector, live_status)) {
>>>>>>> --
>>>>>>> 1.9.1
>>>>>>
>>>>
>>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Sharma, Shashank]

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

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

[Sharma, Shashank]

Regards
Shashank

On 3/17/2016 11:29 PM, Jani Nikula wrote:
> On Thu, 17 Mar 2016, "Sharma, Shashank" <shashank.sharma@intel.com> wrote:
>> [ text/plain ]
>> 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,
>
> Actually, not. You set live_status to true in the beginning, and your
> loop condition has !live_status, i.e. you won't check live status even
> once on any platform. Which is what I said in my review.
>
> BR,
> Jani.
Ah.. damn. While splitting the patch into two, I missed this part. 
Thanks for pointing this out.
>
>
>> 	/* 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 ?
>> 	
>>>>
>>>>>>>>      	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

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

[Ville Syrjälä]

On Fri, Mar 18, 2016 at 08:51:29AM +0530, Sharma, Shashank wrote:
> 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 ?

Yeah, adding a debug message might be prudent.

> 
> >> 	
> >>>>
> >>>>>>>>      	intel_hdmi_unset_edid(connector);
> >>>>>>>>
> >>>>>>>>      	if (intel_hdmi_set_edid(connector, live_status)) {
> >>>>>>>> --
> >>>>>>>> 1.9.1
> >>>>>>>
> >>>>>
> >>>
> >

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Shashank Sharma]

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;
+	}
 
 	intel_hdmi_unset_edid(connector);
 
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Restrict usage of live status check (rev4)

[Patchwork]

== Series Details ==

Series: drm/i915: Restrict usage of live status check (rev4)
URL   : https://patchwork.freedesktop.org/series/4297/
State : success

== Summary ==

Series 4297v4 drm/i915: Restrict usage of live status check
http://patchwork.freedesktop.org/api/1.0/series/4297/revisions/4/mbox/


bdw-nuci7        total:192  pass:180  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:192  pass:169  dwarn:0   dfail:0   fail:0   skip:23 
bsw-nuc-2        total:191  pass:152  dwarn:0   dfail:0   fail:0   skip:39 
byt-nuc          total:191  pass:153  dwarn:0   dfail:0   fail:0   skip:38 
hsw-brixbox      total:192  pass:168  dwarn:0   dfail:0   fail:0   skip:24 
hsw-gt2          total:192  pass:173  dwarn:0   dfail:0   fail:0   skip:19 
ilk-hp8440p      total:192  pass:135  dwarn:0   dfail:0   fail:0   skip:57 
ivb-t430s        total:192  pass:164  dwarn:0   dfail:0   fail:0   skip:28 
skl-i7k-2        total:192  pass:167  dwarn:0   dfail:0   fail:0   skip:25 
skl-nuci5        total:192  pass:181  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:192  pass:154  dwarn:0   dfail:0   fail:0   skip:38 
snb-x220t        total:192  pass:154  dwarn:0   dfail:0   fail:1   skip:37 

Results at /archive/results/CI_IGT_test/Patchwork_1957/

4dc4ce0da8c23182942ce0ecb96cdac57bb1bcd7 drm-intel-nightly: 2016y-04m-20d-16h-33m-39s UTC integration manifest
31430fd drm/i915: Restrict usage of live status check

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Sharma, Shashank]

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

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

[Sharma, Shashank]

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

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

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Sharma, Shashank]

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

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

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Shashank Sharma]

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(-)

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