[PATCH 0/5] HDMI optimization series

[PATCH 0/5] HDMI optimization series

[Sonika Jindal]

This series adds some optimization related to reading of edid only when
required and when live status says so.
The comments in the patches explain more.

First 3 patches were posted earlier(last week) too.
Sending them again along with the live status patch as part of the series.

Shashank Sharma (3):
  drm/i915: add attached connector to hdmi container
  drm/i915: Add HDMI probe function
  drm/i915: Read HDMI EDID only when required

Sonika Jindal (2):
  drm/i915: Check live status before reading edid
  drm/i915: Set edid from detect only if forced

 drivers/gpu/drm/i915/intel_drv.h  |    1 +
 drivers/gpu/drm/i915/intel_hdmi.c |   99 +++++++++++++++++++++++++++++++++----
 2 files changed, 90 insertions(+), 10 deletions(-)

-- 
1.7.10.4

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

[PATCH 1/5] drm/i915: add attached connector to hdmi container

[Sonika Jindal]

From: Shashank Sharma <shashank.sharma@intel.com>

This patch adds the intel_connector initialized to intel_hdmi
display, during the init phase, just like the other encoders do.
This attachment is very useful when we need to extract the connector
pointer during the hotplug handler function

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h  |    1 +
 drivers/gpu/drm/i915/intel_hdmi.c |    1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e90c743..a47fbc3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -659,6 +659,7 @@ struct intel_hdmi {
 	enum hdmi_force_audio force_audio;
 	bool rgb_quant_range_selectable;
 	enum hdmi_picture_aspect aspect_ratio;
+	struct intel_connector *attached_connector;
 	void (*write_infoframe)(struct drm_encoder *encoder,
 				enum hdmi_infoframe_type type,
 				const void *frame, ssize_t len);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 00c4b40..af4d1e6 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -2000,6 +2000,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 
 	intel_connector_attach_encoder(intel_connector, intel_encoder);
 	drm_connector_register(connector);
+	intel_hdmi->attached_connector = intel_connector;
 
 	/* For G4X desktop chip, PEG_BAND_GAP_DATA 3:0 must first be written
 	 * 0xd.  Failure to do so will result in spurious interrupts being
-- 
1.7.10.4

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

[PATCH 2/5] drm/i915: Add HDMI probe function

[Sonika Jindal]

From: Shashank Sharma <shashank.sharma@intel.com>

This patch adds a separate probe function for HDMI
EDID read over DDC channel. This function has been
registered as a .hot_plug handler for HDMI encoder.

The reason behind addition of this separate function needs
brief explaination. The current implementation of hdmi_detect()
function re-sets the cached HDMI edid (in connector->detect_edid) in
every detect call.This function gets called many times, sometimes
directly from userspace probes, forcing drivers to read EDID every
detect function call.This causes several problems like:

1. Race conditions in multiple hot_plug / unplug cases, between
   interrupts bottom halves and userspace detections.
2. Many Un-necessary EDID reads for single hotplug/unplug
3. HDMI complaince failures which expects only one EDID read per hotplug

This function will be serving the purpose of really reading the EDID
by really probing the DDC channel, and updating the cached EDID.

The plan is to:
1. i915 IRQ handler bottom half function already calls
   intel_encoder->hotplug() function. Adding This probe function which
   will read the EDID only in case of a hotplug / unplug.
2. Reuse the cached EDID in hdmi_detect() function UNLESS we are forced
   to probe the EDID by the force=1 variable.The next patch in the
   series does this.
3. Make sure that we are using this force variable properly.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index af4d1e6..064ddd8 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1340,6 +1340,24 @@ intel_hdmi_set_edid(struct drm_connector *connector)
 	return connected;
 }
 
+void intel_hdmi_probe(struct intel_encoder *intel_encoder)
+{
+	struct intel_hdmi *intel_hdmi =
+			enc_to_intel_hdmi(&intel_encoder->base);
+	struct intel_connector *intel_connector =
+				intel_hdmi->attached_connector;
+	/*
+	 * We are here, means there is a hotplug or a force
+	 * detection. Clear the cached EDID and probe the
+	 * DDC bus to check the current status of HDMI.
+	 */
+	intel_hdmi_unset_edid(&intel_connector->base);
+	if (intel_hdmi_set_edid(&intel_connector->base))
+		DRM_DEBUG_DRIVER("DDC probe: got EDID\n");
+	else
+		DRM_DEBUG_DRIVER("DDC probe: no EDID\n");
+}
+
 static enum drm_connector_status
 intel_hdmi_detect(struct drm_connector *connector, bool force)
 {
@@ -1997,6 +2015,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 	intel_connector->unregister = intel_connector_unregister;
 
 	intel_hdmi_add_properties(intel_hdmi, connector);
+	intel_encoder->hot_plug = intel_hdmi_probe;
 
 	intel_connector_attach_encoder(intel_connector, intel_encoder);
 	drm_connector_register(connector);
-- 
1.7.10.4

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

[PATCH 3/5] drm/i915: Read HDMI EDID only when required

[Sonika Jindal]

From: Shashank Sharma <shashank.sharma@intel.com>

This patch makes sure that the HDMI detect function
reads EDID only when its forced to do it. All the other
times, it uses the connector->detect_edid which was cached
during hotplug handling in the hdmi_probe() function. As the
probe function gets called before detect in the interrupt handler
and handles the EDID cacheing part, its absolutely safe to assume
that presence of EDID reflects monitor connected and viceversa.

This will save us from many race conditions between hotplug/unplug
detect call handler threads and userspace calls for the same.
The previous patch in this patch series explains this in detail.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c |   26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 064ddd8..1fb6919 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1362,19 +1362,33 @@ static enum drm_connector_status
 intel_hdmi_detect(struct drm_connector *connector, bool force)
 {
 	enum drm_connector_status status;
+	struct intel_connector *intel_connector =
+				to_intel_connector(connector);
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
 		      connector->base.id, connector->name);
+	/*
+	 * There are many userspace calls which probe EDID from
+	 * detect path. In case on multiple hotplug/unplug, these
+	 * can cause race conditions while probing EDID. Also its
+	 * waste of CPU cycles to read the EDID again and again
+	 * unless there is a real hotplug.
+	 * So until we are forced, check connector status
+	 * based on availability of cached EDID. This will avoid many of
+	 * these race conditions and timing problems.
+	 */
+	if (force)
+		intel_hdmi_probe(intel_connector->encoder);
 
-	intel_hdmi_unset_edid(connector);
-
-	if (intel_hdmi_set_edid(connector)) {
+	if (intel_connector->detect_edid) {
 		struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
-
-		hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
 		status = connector_status_connected;
-	} else
+		hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
+		DRM_DEBUG_DRIVER("hdmi status = connected\n");
+	} else {
 		status = connector_status_disconnected;
+		DRM_DEBUG_DRIVER("hdmi status = disconnected\n");
+	}
 
 	return status;
 }
-- 
1.7.10.4

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

[PATCH 4/5] drm/i915: Check live status before reading edid

[Sonika Jindal]

Adding this for SKL onwards.

Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c |   49 ++++++++++++++++++++++++++++++++++---
 1 file changed, 45 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 1fb6919..769cf4f 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1300,6 +1300,46 @@ intel_hdmi_unset_edid(struct drm_connector *connector)
 	to_intel_connector(connector)->detect_edid = NULL;
 }
 
+static bool intel_hdmi_live_status(struct intel_digital_port *intel_dig_port)
+{
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	enum port port = intel_dig_port->port;
+
+	if (IS_SKYLAKE(dev)) {
+		u32 temp = I915_READ(SDEISR);
+
+		switch (port) {
+		case PORT_B:
+			return temp & SDE_PORTB_HOTPLUG_CPT;
+
+		case PORT_C:
+			return temp & SDE_PORTC_HOTPLUG_CPT;
+
+		case PORT_D:
+			return temp & SDE_PORTD_HOTPLUG_CPT;
+
+		default:
+			return false;
+		}
+	} else if (IS_BROXTON(dev)) {
+		u32 temp = I915_READ(GEN8_DE_PORT_ISR);
+
+		switch (port) {
+		case PORT_B:
+			return temp & BXT_DE_PORT_HP_DDIB;
+
+		case PORT_C:
+			return temp & BXT_DE_PORT_HP_DDIC;
+
+		default:
+			return false;
+
+		}
+	}
+	return true;
+}
+
 static bool
 intel_hdmi_set_edid(struct drm_connector *connector)
 {
@@ -1308,15 +1348,16 @@ intel_hdmi_set_edid(struct drm_connector *connector)
 	struct intel_encoder *intel_encoder =
 		&hdmi_to_dig_port(intel_hdmi)->base;
 	enum intel_display_power_domain power_domain;
-	struct edid *edid;
+	struct edid *edid = NULL;
 	bool connected = false;
 
 	power_domain = intel_display_port_power_domain(intel_encoder);
 	intel_display_power_get(dev_priv, power_domain);
 
-	edid = drm_get_edid(connector,
-			    intel_gmbus_get_adapter(dev_priv,
-						    intel_hdmi->ddc_bus));
+	if (intel_hdmi_live_status(hdmi_to_dig_port(intel_hdmi)))
+		edid = drm_get_edid(connector,
+				intel_gmbus_get_adapter(dev_priv,
+					intel_hdmi->ddc_bus));
 
 	intel_display_power_put(dev_priv, power_domain);
 
-- 
1.7.10.4

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

[PATCH 5/5] drm/i915: Set edid from detect only if forced

[Sonika Jindal]

During init_connector set the edid, then edid will be set/unset only during
hotplug. For the sake of older platforms where HPD is not stable, let edid
read happen from detect as well only if it is forced to do so.

Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 769cf4f..0111ac0a 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1405,6 +1405,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 	enum drm_connector_status status;
 	struct intel_connector *intel_connector =
 				to_intel_connector(connector);
+	struct drm_device *dev = connector->dev;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
 		      connector->base.id, connector->name);
@@ -1418,7 +1419,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 	 * based on availability of cached EDID. This will avoid many of
 	 * these race conditions and timing problems.
 	 */
-	if (force)
+	if (force && INTEL_INFO(dev)->gen < 9)
 		intel_hdmi_probe(intel_connector->encoder);
 
 	if (intel_connector->detect_edid) {
@@ -2076,6 +2077,9 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 	drm_connector_register(connector);
 	intel_hdmi->attached_connector = intel_connector;
 
+	/* Set edid during init */
+	intel_hdmi_probe(intel_encoder);
+
 	/* For G4X desktop chip, PEG_BAND_GAP_DATA 3:0 must first be written
 	 * 0xd.  Failure to do so will result in spurious interrupts being
 	 * generated on the port when a cable is not attached.
-- 
1.7.10.4

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

Re: [PATCH 5/5] drm/i915: Set edid from detect only if forced

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6762
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                                  302/302              302/302
SNB                                  312/316              312/316
IVB                                  343/343              343/343
BYT                 -1              287/287              286/287
HSW                                  380/380              380/380
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*BYT  igt@gem_partial_pwrite_pread@reads-display      PASS(1)      FAIL(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915: Check live status before reading edid

[Daniel Vetter]

On Thu, Jul 09, 2015 at 05:34:29PM +0530, Sonika Jindal wrote:
> Adding this for SKL onwards.
> 
> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>

I think this is the critical piece really, and I'd like to roll it out for
all platforms. git has the code for all the old ones, so no big deal to
digg that out. Also we need your fix for the interrupt handling first ofc,
otherwise this won't work.

Then we can apply this and give it some good testing before we start
relying on it with caching hdmi probe status. I know this means that
things will be slower, but I've been burned a bit too much the last few
times we've tried this. And I really think we need the most amount of
testing we can get (hence rolling this out for all platforms). If your
hpd irq handling bugfix is indeed the bug that will be confirmed quickly,
otherwise it means back to debugging.

> ---
>  drivers/gpu/drm/i915/intel_hdmi.c |   49 ++++++++++++++++++++++++++++++++++---
>  1 file changed, 45 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 1fb6919..769cf4f 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1300,6 +1300,46 @@ intel_hdmi_unset_edid(struct drm_connector *connector)
>  	to_intel_connector(connector)->detect_edid = NULL;
>  }
>  
> +static bool intel_hdmi_live_status(struct intel_digital_port *intel_dig_port)
> +{
> +	struct drm_device *dev = intel_dig_port->base.base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +	enum port port = intel_dig_port->port;
> +
> +	if (IS_SKYLAKE(dev)) {
> +		u32 temp = I915_READ(SDEISR);
> +
> +		switch (port) {
> +		case PORT_B:
> +			return temp & SDE_PORTB_HOTPLUG_CPT;
> +
> +		case PORT_C:
> +			return temp & SDE_PORTC_HOTPLUG_CPT;
> +
> +		case PORT_D:
> +			return temp & SDE_PORTD_HOTPLUG_CPT;
> +
> +		default:
> +			return false;
> +		}

The old code had per-platform helper functions for this instead of a big
if-ladder. I think that would look better.

Also when you digg out these old versions please also cite the commit sha1
of the patches where we had to last revert this (and explain why it's now
save).
-Daniel

> +	} else if (IS_BROXTON(dev)) {
> +		u32 temp = I915_READ(GEN8_DE_PORT_ISR);
> +
> +		switch (port) {
> +		case PORT_B:
> +			return temp & BXT_DE_PORT_HP_DDIB;
> +
> +		case PORT_C:
> +			return temp & BXT_DE_PORT_HP_DDIC;
> +
> +		default:
> +			return false;
> +
> +		}
> +	}
> +	return true;
> +}
> +
>  static bool
>  intel_hdmi_set_edid(struct drm_connector *connector)
>  {
> @@ -1308,15 +1348,16 @@ intel_hdmi_set_edid(struct drm_connector *connector)
>  	struct intel_encoder *intel_encoder =
>  		&hdmi_to_dig_port(intel_hdmi)->base;
>  	enum intel_display_power_domain power_domain;
> -	struct edid *edid;
> +	struct edid *edid = NULL;
>  	bool connected = false;
>  
>  	power_domain = intel_display_port_power_domain(intel_encoder);
>  	intel_display_power_get(dev_priv, power_domain);
>  
> -	edid = drm_get_edid(connector,
> -			    intel_gmbus_get_adapter(dev_priv,
> -						    intel_hdmi->ddc_bus));
> +	if (intel_hdmi_live_status(hdmi_to_dig_port(intel_hdmi)))
> +		edid = drm_get_edid(connector,
> +				intel_gmbus_get_adapter(dev_priv,
> +					intel_hdmi->ddc_bus));
>  
>  	intel_display_power_put(dev_priv, power_domain);
>  
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915: Check live status before reading edid

[Jindal, Sonika]

On 7/9/2015 10:57 PM, Daniel Vetter wrote:
> On Thu, Jul 09, 2015 at 05:34:29PM +0530, Sonika Jindal wrote:
>> Adding this for SKL onwards.
>>
>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>
> I think this is the critical piece really, and I'd like to roll it out for
> all platforms. git has the code for all the old ones, so no big deal to
> digg that out. Also we need your fix for the interrupt handling first ofc,
> otherwise this won't work.
>
We have tested this with VLV/CHV too, and it works fine. I'l add those 
platforms also in the next version of this patch and change the gen 
check in the next patch as well.

> Then we can apply this and give it some good testing before we start
> relying on it with caching hdmi probe status. I know this means that
> things will be slower, but I've been burned a bit too much the last few
> times we've tried this. And I really think we need the most amount of
> testing we can get (hence rolling this out for all platforms). If your
> hpd irq handling bugfix is indeed the bug that will be confirmed quickly,
> otherwise it means back to debugging.
>
>> ---
>>   drivers/gpu/drm/i915/intel_hdmi.c |   49 ++++++++++++++++++++++++++++++++++---
>>   1 file changed, 45 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>> index 1fb6919..769cf4f 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -1300,6 +1300,46 @@ intel_hdmi_unset_edid(struct drm_connector *connector)
>>   	to_intel_connector(connector)->detect_edid = NULL;
>>   }
>>
>> +static bool intel_hdmi_live_status(struct intel_digital_port *intel_dig_port)
>> +{
>> +	struct drm_device *dev = intel_dig_port->base.base.dev;
>> +	struct drm_i915_private *dev_priv = to_i915(dev);
>> +	enum port port = intel_dig_port->port;
>> +
>> +	if (IS_SKYLAKE(dev)) {
>> +		u32 temp = I915_READ(SDEISR);
>> +
>> +		switch (port) {
>> +		case PORT_B:
>> +			return temp & SDE_PORTB_HOTPLUG_CPT;
>> +
>> +		case PORT_C:
>> +			return temp & SDE_PORTC_HOTPLUG_CPT;
>> +
>> +		case PORT_D:
>> +			return temp & SDE_PORTD_HOTPLUG_CPT;
>> +
>> +		default:
>> +			return false;
>> +		}
>
> The old code had per-platform helper functions for this instead of a big
> if-ladder. I think that would look better.
>
Hmm, I see g4x_digital_port_connected and ibx_digital_port_connected 
which already checks for these bits. We can reuse that here instead.
I'l send the next version.

Thanks,
Sonika

> Also when you digg out these old versions please also cite the commit sha1
> of the patches where we had to last revert this (and explain why it's now
> save).
> -Daniel
>
>> +	} else if (IS_BROXTON(dev)) {
>> +		u32 temp = I915_READ(GEN8_DE_PORT_ISR);
>> +
>> +		switch (port) {
>> +		case PORT_B:
>> +			return temp & BXT_DE_PORT_HP_DDIB;
>> +
>> +		case PORT_C:
>> +			return temp & BXT_DE_PORT_HP_DDIC;
>> +
>> +		default:
>> +			return false;
>> +
>> +		}
>> +	}
>> +	return true;
>> +}
>> +
>>   static bool
>>   intel_hdmi_set_edid(struct drm_connector *connector)
>>   {
>> @@ -1308,15 +1348,16 @@ intel_hdmi_set_edid(struct drm_connector *connector)
>>   	struct intel_encoder *intel_encoder =
>>   		&hdmi_to_dig_port(intel_hdmi)->base;
>>   	enum intel_display_power_domain power_domain;
>> -	struct edid *edid;
>> +	struct edid *edid = NULL;
>>   	bool connected = false;
>>
>>   	power_domain = intel_display_port_power_domain(intel_encoder);
>>   	intel_display_power_get(dev_priv, power_domain);
>>
>> -	edid = drm_get_edid(connector,
>> -			    intel_gmbus_get_adapter(dev_priv,
>> -						    intel_hdmi->ddc_bus));
>> +	if (intel_hdmi_live_status(hdmi_to_dig_port(intel_hdmi)))
>> +		edid = drm_get_edid(connector,
>> +				intel_gmbus_get_adapter(dev_priv,
>> +					intel_hdmi->ddc_bus));
>>
>>   	intel_display_power_put(dev_priv, power_domain);
>>
>> --
>> 1.7.10.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Set edid from init and not from detect

[Sonika Jindal]

During init_connector set the edid, then edid will be set/unset only during
hotplug. For the sake of older platforms where HPD is not stable, let edid
read happen from detect as well only if it is forced to do so.

v2: Removing the 'force' check, instead let detect call read the edid for
platforms older than gen 7 (Shashank)

Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 66af388..44e7160 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1399,6 +1399,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 	enum drm_connector_status status;
 	struct intel_connector *intel_connector =
 				to_intel_connector(connector);
+	struct drm_device *dev = connector->dev;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
 		      connector->base.id, connector->name);
@@ -1412,7 +1413,7 @@ intel_hdmi_detect(struct drm_connector *connector, bool force)
 	 * based on availability of cached EDID. This will avoid many of
 	 * these race conditions and timing problems.
 	 */
-	if (force)
+	if (INTEL_INFO(dev)->gen < 7)
 		intel_hdmi_probe(intel_connector->encoder);
 
 	if (intel_connector->detect_edid) {
@@ -2070,6 +2071,9 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 	drm_connector_register(connector);
 	intel_hdmi->attached_connector = intel_connector;
 
+	/* Set edid during init */
+	intel_hdmi_probe(intel_encoder);
+
 	/* For G4X desktop chip, PEG_BAND_GAP_DATA 3:0 must first be written
 	 * 0xd.  Failure to do so will result in spurious interrupts being
 	 * generated on the port when a cable is not attached.
-- 
1.7.10.4

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

[PATCH] drm/i915: Check live status before reading edid

[Sonika Jindal]

Adding this for SKL onwards.

v2: Adding checks for VLV/CHV as well. Reusing old ibx and g4x functions
to check digital port status. Adding a separate function to get bxt live
status (Daniel)

Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c   |    4 ++--
 drivers/gpu/drm/i915/intel_drv.h  |    2 ++
 drivers/gpu/drm/i915/intel_hdmi.c |   42 +++++++++++++++++++++++++++++++++----
 3 files changed, 42 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 4ebfc3a..7b54b9d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4443,8 +4443,8 @@ ironlake_dp_detect(struct intel_dp *intel_dp)
 	return intel_dp_detect_dpcd(intel_dp);
 }
 
-static int g4x_digital_port_connected(struct drm_device *dev,
-				       struct intel_digital_port *intel_dig_port)
+int g4x_digital_port_connected(struct drm_device *dev,
+			       struct intel_digital_port *intel_dig_port)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t bit;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index a47fbc3..30c8dd6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1187,6 +1187,8 @@ void intel_edp_drrs_disable(struct intel_dp *intel_dp);
 void intel_edp_drrs_invalidate(struct drm_device *dev,
 		unsigned frontbuffer_bits);
 void intel_edp_drrs_flush(struct drm_device *dev, unsigned frontbuffer_bits);
+int g4x_digital_port_connected(struct drm_device *dev,
+			       struct intel_digital_port *intel_dig_port);
 
 /* intel_dp_mst.c */
 int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 1fb6919..7eb0d0e 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1300,6 +1300,39 @@ intel_hdmi_unset_edid(struct drm_connector *connector)
 	to_intel_connector(connector)->detect_edid = NULL;
 }
 
+static bool bxt_port_connected(struct drm_i915_private *dev_priv,
+			       struct intel_digital_port *port)
+{
+	u32 temp = I915_READ(GEN8_DE_PORT_ISR);
+
+	switch (port->port) {
+	case PORT_B:
+		return temp & BXT_DE_PORT_HP_DDIB;
+
+	case PORT_C:
+		return temp & BXT_DE_PORT_HP_DDIC;
+
+	default:
+		return false;
+
+	}
+}
+
+static bool intel_hdmi_live_status(struct intel_digital_port *intel_dig_port)
+{
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct drm_i915_private *dev_priv = to_i915(dev);
+
+	if (IS_VALLEYVIEW(dev))
+		return g4x_digital_port_connected(dev, intel_dig_port);
+	else if (IS_SKYLAKE(dev))
+		return ibx_digital_port_connected(dev_priv, intel_dig_port);
+	else if (IS_BROXTON(dev))
+		return bxt_port_connected(dev_priv, intel_dig_port);
+
+	return true;
+}
+
 static bool
 intel_hdmi_set_edid(struct drm_connector *connector)
 {
@@ -1308,15 +1341,16 @@ intel_hdmi_set_edid(struct drm_connector *connector)
 	struct intel_encoder *intel_encoder =
 		&hdmi_to_dig_port(intel_hdmi)->base;
 	enum intel_display_power_domain power_domain;
-	struct edid *edid;
+	struct edid *edid = NULL;
 	bool connected = false;
 
 	power_domain = intel_display_port_power_domain(intel_encoder);
 	intel_display_power_get(dev_priv, power_domain);
 
-	edid = drm_get_edid(connector,
-			    intel_gmbus_get_adapter(dev_priv,
-						    intel_hdmi->ddc_bus));
+	if (intel_hdmi_live_status(hdmi_to_dig_port(intel_hdmi)))
+		edid = drm_get_edid(connector,
+				intel_gmbus_get_adapter(dev_priv,
+					intel_hdmi->ddc_bus));
 
 	intel_display_power_put(dev_priv, power_domain);
 
-- 
1.7.10.4

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

Re: [PATCH] drm/i915: Set edid from init and not from detect

[Chris Wilson]

On Mon, Jul 13, 2015 at 04:19:15PM +0530, Sonika Jindal wrote:
> During init_connector set the edid, then edid will be set/unset only during
> hotplug. For the sake of older platforms where HPD is not stable, let edid
> read happen from detect as well only if it is forced to do so.
> 
> v2: Removing the 'force' check, instead let detect call read the edid for
> platforms older than gen 7 (Shashank)

That's enough worse. We now have a random gen check with no rationale
for why you suddenly believe all manufacturers have fixed their wiring.
There is no reason to believe that gen7 suddenly works is there? If
there is, why don't you mention it?
-Chris

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

Re: [PATCH] drm/i915: Set edid from init and not from detect

[Jindal, Sonika]

On 7/13/2015 5:10 PM, Chris Wilson wrote:
> On Mon, Jul 13, 2015 at 04:19:15PM +0530, Sonika Jindal wrote:
>> During init_connector set the edid, then edid will be set/unset only during
>> hotplug. For the sake of older platforms where HPD is not stable, let edid
>> read happen from detect as well only if it is forced to do so.
>>
>> v2: Removing the 'force' check, instead let detect call read the edid for
>> platforms older than gen 7 (Shashank)
>
> That's enough worse. We now have a random gen check with no rationale
> for why you suddenly believe all manufacturers have fixed their wiring.
> There is no reason to believe that gen7 suddenly works is there? If
> there is, why don't you mention it?
> -Chris
>
This gen7 check is to be on the safer side not to affect older paltforms.
For CHV/VLV, already the live status check is stable enough to determine 
if we can read edid or not. In VPG production branches we have this 
patch already available and it was tested with variety of monitors 
extensively. So we now read the edid only during init and during hotplug.
For SKL, the "[PATCH] drm/i915: Handle HPD when it has actually 
occurred" patch makes HPD stable enough.
So, we can rely on the edid read from init_connector instead of reading 
edid on every detect call.

Regards,
Sonika

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

Re: [PATCH] drm/i915: Check live status before reading edid

[Daniel Vetter]

On Mon, Jul 13, 2015 at 04:35:00PM +0530, Sonika Jindal wrote:
> Adding this for SKL onwards.
> 
> v2: Adding checks for VLV/CHV as well. Reusing old ibx and g4x functions
> to check digital port status. Adding a separate function to get bxt live
> status (Daniel)
> 
> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c   |    4 ++--
>  drivers/gpu/drm/i915/intel_drv.h  |    2 ++
>  drivers/gpu/drm/i915/intel_hdmi.c |   42 +++++++++++++++++++++++++++++++++----
>  3 files changed, 42 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 4ebfc3a..7b54b9d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4443,8 +4443,8 @@ ironlake_dp_detect(struct intel_dp *intel_dp)
>  	return intel_dp_detect_dpcd(intel_dp);
>  }
>  
> -static int g4x_digital_port_connected(struct drm_device *dev,
> -				       struct intel_digital_port *intel_dig_port)
> +int g4x_digital_port_connected(struct drm_device *dev,
> +			       struct intel_digital_port *intel_dig_port)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint32_t bit;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a47fbc3..30c8dd6 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1187,6 +1187,8 @@ void intel_edp_drrs_disable(struct intel_dp *intel_dp);
>  void intel_edp_drrs_invalidate(struct drm_device *dev,
>  		unsigned frontbuffer_bits);
>  void intel_edp_drrs_flush(struct drm_device *dev, unsigned frontbuffer_bits);
> +int g4x_digital_port_connected(struct drm_device *dev,
> +			       struct intel_digital_port *intel_dig_port);
>  
>  /* intel_dp_mst.c */
>  int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 1fb6919..7eb0d0e 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1300,6 +1300,39 @@ intel_hdmi_unset_edid(struct drm_connector *connector)
>  	to_intel_connector(connector)->detect_edid = NULL;
>  }
>  
> +static bool bxt_port_connected(struct drm_i915_private *dev_priv,
> +			       struct intel_digital_port *port)
> +{
> +	u32 temp = I915_READ(GEN8_DE_PORT_ISR);
> +
> +	switch (port->port) {
> +	case PORT_B:
> +		return temp & BXT_DE_PORT_HP_DDIB;
> +
> +	case PORT_C:
> +		return temp & BXT_DE_PORT_HP_DDIC;
> +
> +	default:
> +		return false;
> +
> +	}
> +}
> +
> +static bool intel_hdmi_live_status(struct intel_digital_port *intel_dig_port)
> +{
> +	struct drm_device *dev = intel_dig_port->base.base.dev;
> +	struct drm_i915_private *dev_priv = to_i915(dev);
> +
> +	if (IS_VALLEYVIEW(dev))
> +		return g4x_digital_port_connected(dev, intel_dig_port);
> +	else if (IS_SKYLAKE(dev))
> +		return ibx_digital_port_connected(dev_priv, intel_dig_port);
> +	else if (IS_BROXTON(dev))
> +		return bxt_port_connected(dev_priv, intel_dig_port);

Really I want this to be rolled out for all platforms, together with the
fix for handling hpd interrupts. Together with a reference to the old
commits we had to revert because it didn't work.

I want to test this on my ivb (since that's the machine where I can
readily reproduce this bug), and if it still doesn't work there I won't
take this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Set edid from init and not from detect

[Daniel Vetter]

On Mon, Jul 13, 2015 at 05:29:12PM +0530, Jindal, Sonika wrote:
> 
> 
> On 7/13/2015 5:10 PM, Chris Wilson wrote:
> >On Mon, Jul 13, 2015 at 04:19:15PM +0530, Sonika Jindal wrote:
> >>During init_connector set the edid, then edid will be set/unset only during
> >>hotplug. For the sake of older platforms where HPD is not stable, let edid
> >>read happen from detect as well only if it is forced to do so.
> >>
> >>v2: Removing the 'force' check, instead let detect call read the edid for
> >>platforms older than gen 7 (Shashank)
> >
> >That's enough worse. We now have a random gen check with no rationale
> >for why you suddenly believe all manufacturers have fixed their wiring.
> >There is no reason to believe that gen7 suddenly works is there? If
> >there is, why don't you mention it?
> >-Chris
> >
> This gen7 check is to be on the safer side not to affect older paltforms.
> For CHV/VLV, already the live status check is stable enough to determine if
> we can read edid or not. In VPG production branches we have this patch
> already available and it was tested with variety of monitors extensively. So
> we now read the edid only during init and during hotplug.
> For SKL, the "[PATCH] drm/i915: Handle HPD when it has actually occurred"
> patch makes HPD stable enough.
> So, we can rely on the edid read from init_connector instead of reading edid
> on every detect call.

Again, not going to take this with random gen checks. I want your fix for
handling hpd on other platforms, then roll this out everywhere.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Set edid from init and not from detect

[Sharma, Shashank]

Hi Daniel, Chris  

Gen7 and Gen8 platforms have a different live status register (0x61114) and we need not to rely on ISR on that. 
As Sonika mentioned, We have been using this register for our commercial releases, and found them reliable across a wide range of monitors. 

On the other hand, Bsepc clearly mentions, to check the live status before even try to read EDID. 
The current DRM nightly code doesn't do that, and we have received few errors from Gen7 Chromebooks where you can still read valid EDID on HDMI hot-unplug. 

So I think this patch and solution is ready, and it should go in. 

Regards
Shashank
-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Monday, July 13, 2015 8:27 PM
To: Jindal, Sonika
Cc: Chris Wilson; intel-gfx@lists.freedesktop.org; Sharma, Shashank
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Set edid from init and not from detect

On Mon, Jul 13, 2015 at 05:29:12PM +0530, Jindal, Sonika wrote:
> 
> 
> On 7/13/2015 5:10 PM, Chris Wilson wrote:
> >On Mon, Jul 13, 2015 at 04:19:15PM +0530, Sonika Jindal wrote:
> >>During init_connector set the edid, then edid will be set/unset only 
> >>during hotplug. For the sake of older platforms where HPD is not 
> >>stable, let edid read happen from detect as well only if it is forced to do so.
> >>
> >>v2: Removing the 'force' check, instead let detect call read the 
> >>edid for platforms older than gen 7 (Shashank)
> >
> >That's enough worse. We now have a random gen check with no rationale 
> >for why you suddenly believe all manufacturers have fixed their wiring.
> >There is no reason to believe that gen7 suddenly works is there? If 
> >there is, why don't you mention it?
> >-Chris
> >
> This gen7 check is to be on the safer side not to affect older paltforms.
> For CHV/VLV, already the live status check is stable enough to 
> determine if we can read edid or not. In VPG production branches we 
> have this patch already available and it was tested with variety of 
> monitors extensively. So we now read the edid only during init and during hotplug.
> For SKL, the "[PATCH] drm/i915: Handle HPD when it has actually occurred"
> patch makes HPD stable enough.
> So, we can rely on the edid read from init_connector instead of 
> reading edid on every detect call.

Again, not going to take this with random gen checks. I want your fix for handling hpd on other platforms, then roll this out everywhere.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Check live status before reading edid

[Jindal, Sonika]

On 7/13/2015 8:25 PM, Daniel Vetter wrote:
> On Mon, Jul 13, 2015 at 04:35:00PM +0530, Sonika Jindal wrote:
>> Adding this for SKL onwards.
>>
>> v2: Adding checks for VLV/CHV as well. Reusing old ibx and g4x functions
>> to check digital port status. Adding a separate function to get bxt live
>> status (Daniel)
>>
>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c   |    4 ++--
>>   drivers/gpu/drm/i915/intel_drv.h  |    2 ++
>>   drivers/gpu/drm/i915/intel_hdmi.c |   42 +++++++++++++++++++++++++++++++++----
>>   3 files changed, 42 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 4ebfc3a..7b54b9d 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4443,8 +4443,8 @@ ironlake_dp_detect(struct intel_dp *intel_dp)
>>   	return intel_dp_detect_dpcd(intel_dp);
>>   }
>>
>> -static int g4x_digital_port_connected(struct drm_device *dev,
>> -				       struct intel_digital_port *intel_dig_port)
>> +int g4x_digital_port_connected(struct drm_device *dev,
>> +			       struct intel_digital_port *intel_dig_port)
>>   {
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>>   	uint32_t bit;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index a47fbc3..30c8dd6 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1187,6 +1187,8 @@ void intel_edp_drrs_disable(struct intel_dp *intel_dp);
>>   void intel_edp_drrs_invalidate(struct drm_device *dev,
>>   		unsigned frontbuffer_bits);
>>   void intel_edp_drrs_flush(struct drm_device *dev, unsigned frontbuffer_bits);
>> +int g4x_digital_port_connected(struct drm_device *dev,
>> +			       struct intel_digital_port *intel_dig_port);
>>
>>   /* intel_dp_mst.c */
>>   int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>> index 1fb6919..7eb0d0e 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -1300,6 +1300,39 @@ intel_hdmi_unset_edid(struct drm_connector *connector)
>>   	to_intel_connector(connector)->detect_edid = NULL;
>>   }
>>
>> +static bool bxt_port_connected(struct drm_i915_private *dev_priv,
>> +			       struct intel_digital_port *port)
>> +{
>> +	u32 temp = I915_READ(GEN8_DE_PORT_ISR);
>> +
>> +	switch (port->port) {
>> +	case PORT_B:
>> +		return temp & BXT_DE_PORT_HP_DDIB;
>> +
>> +	case PORT_C:
>> +		return temp & BXT_DE_PORT_HP_DDIC;
>> +
>> +	default:
>> +		return false;
>> +
>> +	}
>> +}
>> +
>> +static bool intel_hdmi_live_status(struct intel_digital_port *intel_dig_port)
>> +{
>> +	struct drm_device *dev = intel_dig_port->base.base.dev;
>> +	struct drm_i915_private *dev_priv = to_i915(dev);
>> +
>> +	if (IS_VALLEYVIEW(dev))
>> +		return g4x_digital_port_connected(dev, intel_dig_port);
>> +	else if (IS_SKYLAKE(dev))
>> +		return ibx_digital_port_connected(dev_priv, intel_dig_port);
>> +	else if (IS_BROXTON(dev))
>> +		return bxt_port_connected(dev_priv, intel_dig_port);
>
> Really I want this to be rolled out for all platforms, together with the
> fix for handling hpd interrupts. Together with a reference to the old
> commits we had to revert because it didn't work.
>
> I want to test this on my ivb (since that's the machine where I can
> readily reproduce this bug), and if it still doesn't work there I won't
> take this.
> -Daniel
Is there a formal process to raise a test for hpd on all platforms which 
might be affected by this?

Regards,
Sonika
>

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

Re: [PATCH] drm/i915: Check live status before reading edid

[Daniel Vetter]

On Tue, Jul 14, 2015 at 10:16:17AM +0530, Jindal, Sonika wrote:
> 
> 
> On 7/13/2015 8:25 PM, Daniel Vetter wrote:
> >On Mon, Jul 13, 2015 at 04:35:00PM +0530, Sonika Jindal wrote:
> >>Adding this for SKL onwards.
> >>
> >>v2: Adding checks for VLV/CHV as well. Reusing old ibx and g4x functions
> >>to check digital port status. Adding a separate function to get bxt live
> >>status (Daniel)
> >>
> >>Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> >>---
> >>  drivers/gpu/drm/i915/intel_dp.c   |    4 ++--
> >>  drivers/gpu/drm/i915/intel_drv.h  |    2 ++
> >>  drivers/gpu/drm/i915/intel_hdmi.c |   42 +++++++++++++++++++++++++++++++++----
> >>  3 files changed, 42 insertions(+), 6 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >>index 4ebfc3a..7b54b9d 100644
> >>--- a/drivers/gpu/drm/i915/intel_dp.c
> >>+++ b/drivers/gpu/drm/i915/intel_dp.c
> >>@@ -4443,8 +4443,8 @@ ironlake_dp_detect(struct intel_dp *intel_dp)
> >>  	return intel_dp_detect_dpcd(intel_dp);
> >>  }
> >>
> >>-static int g4x_digital_port_connected(struct drm_device *dev,
> >>-				       struct intel_digital_port *intel_dig_port)
> >>+int g4x_digital_port_connected(struct drm_device *dev,
> >>+			       struct intel_digital_port *intel_dig_port)
> >>  {
> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >>  	uint32_t bit;
> >>diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >>index a47fbc3..30c8dd6 100644
> >>--- a/drivers/gpu/drm/i915/intel_drv.h
> >>+++ b/drivers/gpu/drm/i915/intel_drv.h
> >>@@ -1187,6 +1187,8 @@ void intel_edp_drrs_disable(struct intel_dp *intel_dp);
> >>  void intel_edp_drrs_invalidate(struct drm_device *dev,
> >>  		unsigned frontbuffer_bits);
> >>  void intel_edp_drrs_flush(struct drm_device *dev, unsigned frontbuffer_bits);
> >>+int g4x_digital_port_connected(struct drm_device *dev,
> >>+			       struct intel_digital_port *intel_dig_port);
> >>
> >>  /* intel_dp_mst.c */
> >>  int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
> >>diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> >>index 1fb6919..7eb0d0e 100644
> >>--- a/drivers/gpu/drm/i915/intel_hdmi.c
> >>+++ b/drivers/gpu/drm/i915/intel_hdmi.c
> >>@@ -1300,6 +1300,39 @@ intel_hdmi_unset_edid(struct drm_connector *connector)
> >>  	to_intel_connector(connector)->detect_edid = NULL;
> >>  }
> >>
> >>+static bool bxt_port_connected(struct drm_i915_private *dev_priv,
> >>+			       struct intel_digital_port *port)
> >>+{
> >>+	u32 temp = I915_READ(GEN8_DE_PORT_ISR);
> >>+
> >>+	switch (port->port) {
> >>+	case PORT_B:
> >>+		return temp & BXT_DE_PORT_HP_DDIB;
> >>+
> >>+	case PORT_C:
> >>+		return temp & BXT_DE_PORT_HP_DDIC;
> >>+
> >>+	default:
> >>+		return false;
> >>+
> >>+	}
> >>+}
> >>+
> >>+static bool intel_hdmi_live_status(struct intel_digital_port *intel_dig_port)
> >>+{
> >>+	struct drm_device *dev = intel_dig_port->base.base.dev;
> >>+	struct drm_i915_private *dev_priv = to_i915(dev);
> >>+
> >>+	if (IS_VALLEYVIEW(dev))
> >>+		return g4x_digital_port_connected(dev, intel_dig_port);
> >>+	else if (IS_SKYLAKE(dev))
> >>+		return ibx_digital_port_connected(dev_priv, intel_dig_port);
> >>+	else if (IS_BROXTON(dev))
> >>+		return bxt_port_connected(dev_priv, intel_dig_port);
> >
> >Really I want this to be rolled out for all platforms, together with the
> >fix for handling hpd interrupts. Together with a reference to the old
> >commits we had to revert because it didn't work.
> >
> >I want to test this on my ivb (since that's the machine where I can
> >readily reproduce this bug), and if it still doesn't work there I won't
> >take this.
> >-Daniel
> Is there a formal process to raise a test for hpd on all platforms which
> might be affected by this?

Get it merged and wait for the regression reports to come in or not. The
entire problem I'm trying to explain here is that these hpd problems where
_not_ detected internally here at Intel, but only reported by external
people. Only later on did I come across a machine (by accident) which
seems to exhibit the same problem.

That's also why I want to enable it everywhere, increases the chances
we'll get a negative report if it doesn't work.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Set edid from init and not from detect

[Daniel Vetter]

On Tue, Jul 14, 2015 at 03:48:36AM +0000, Sharma, Shashank wrote:
> Hi Daniel, Chris  
> 
> Gen7 and Gen8 platforms have a different live status register (0x61114) and we need not to rely on ISR on that. 
> As Sonika mentioned, We have been using this register for our commercial releases, and found them reliable across a wide range of monitors. 
> 
> On the other hand, Bsepc clearly mentions, to check the live status before even try to read EDID. 
> The current DRM nightly code doesn't do that, and we have received few errors from Gen7 Chromebooks where you can still read valid EDID on HDMI hot-unplug. 
> 
> So I think this patch and solution is ready, and it should go in. 

I have a gen7 machine here which is currently (and it's somewhat random)
broken wrt hpd and hdmi. And afaics this patch series doesn't have the
bugfix for the hpd handling - or did I miss it?
-Daniel

> 
> Regards
> Shashank
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Monday, July 13, 2015 8:27 PM
> To: Jindal, Sonika
> Cc: Chris Wilson; intel-gfx@lists.freedesktop.org; Sharma, Shashank
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Set edid from init and not from detect
> 
> On Mon, Jul 13, 2015 at 05:29:12PM +0530, Jindal, Sonika wrote:
> > 
> > 
> > On 7/13/2015 5:10 PM, Chris Wilson wrote:
> > >On Mon, Jul 13, 2015 at 04:19:15PM +0530, Sonika Jindal wrote:
> > >>During init_connector set the edid, then edid will be set/unset only 
> > >>during hotplug. For the sake of older platforms where HPD is not 
> > >>stable, let edid read happen from detect as well only if it is forced to do so.
> > >>
> > >>v2: Removing the 'force' check, instead let detect call read the 
> > >>edid for platforms older than gen 7 (Shashank)
> > >
> > >That's enough worse. We now have a random gen check with no rationale 
> > >for why you suddenly believe all manufacturers have fixed their wiring.
> > >There is no reason to believe that gen7 suddenly works is there? If 
> > >there is, why don't you mention it?
> > >-Chris
> > >
> > This gen7 check is to be on the safer side not to affect older paltforms.
> > For CHV/VLV, already the live status check is stable enough to 
> > determine if we can read edid or not. In VPG production branches we 
> > have this patch already available and it was tested with variety of 
> > monitors extensively. So we now read the edid only during init and during hotplug.
> > For SKL, the "[PATCH] drm/i915: Handle HPD when it has actually occurred"
> > patch makes HPD stable enough.
> > So, we can rely on the edid read from init_connector instead of 
> > reading edid on every detect call.
> 
> Again, not going to take this with random gen checks. I want your fix for handling hpd on other platforms, then roll this out everywhere.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Set edid from init and not from detect

[Sharma, Shashank]

Please apply this patch series along with the Interrupt handling fix patch Sonika shared. 
With these 4 patches applied, we should not see any problems with HPDs (Until the HW is broken). 

On a similar note, the corresponding chrome team applied the live status check patch, along with others, and they
are not seeing the problems with HPD, hence they are also interested in this patch series. 

Please let us know if you observe something else, we would love to dig further. 
But as we previously mentioned, this patch series is available and running across various MCG Gen7 devices, and available with
Gen8 PV production branches also, and there the hotplug stability is pretty good.

Regards
Shashank 
-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Tuesday, July 14, 2015 1:29 PM
To: Sharma, Shashank
Cc: Daniel Vetter; Jindal, Sonika; Chris Wilson; intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Set edid from init and not from detect

On Tue, Jul 14, 2015 at 03:48:36AM +0000, Sharma, Shashank wrote:
> Hi Daniel, Chris
> 
> Gen7 and Gen8 platforms have a different live status register (0x61114) and we need not to rely on ISR on that. 
> As Sonika mentioned, We have been using this register for our commercial releases, and found them reliable across a wide range of monitors. 
> 
> On the other hand, Bsepc clearly mentions, to check the live status before even try to read EDID. 
> The current DRM nightly code doesn't do that, and we have received few errors from Gen7 Chromebooks where you can still read valid EDID on HDMI hot-unplug. 
> 
> So I think this patch and solution is ready, and it should go in. 

I have a gen7 machine here which is currently (and it's somewhat random) broken wrt hpd and hdmi. And afaics this patch series doesn't have the bugfix for the hpd handling - or did I miss it?
-Daniel

> 
> Regards
> Shashank
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of 
> Daniel Vetter
> Sent: Monday, July 13, 2015 8:27 PM
> To: Jindal, Sonika
> Cc: Chris Wilson; intel-gfx@lists.freedesktop.org; Sharma, Shashank
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Set edid from init and not 
> from detect
> 
> On Mon, Jul 13, 2015 at 05:29:12PM +0530, Jindal, Sonika wrote:
> > 
> > 
> > On 7/13/2015 5:10 PM, Chris Wilson wrote:
> > >On Mon, Jul 13, 2015 at 04:19:15PM +0530, Sonika Jindal wrote:
> > >>During init_connector set the edid, then edid will be set/unset 
> > >>only during hotplug. For the sake of older platforms where HPD is 
> > >>not stable, let edid read happen from detect as well only if it is forced to do so.
> > >>
> > >>v2: Removing the 'force' check, instead let detect call read the 
> > >>edid for platforms older than gen 7 (Shashank)
> > >
> > >That's enough worse. We now have a random gen check with no 
> > >rationale for why you suddenly believe all manufacturers have fixed their wiring.
> > >There is no reason to believe that gen7 suddenly works is there? If 
> > >there is, why don't you mention it?
> > >-Chris
> > >
> > This gen7 check is to be on the safer side not to affect older paltforms.
> > For CHV/VLV, already the live status check is stable enough to 
> > determine if we can read edid or not. In VPG production branches we 
> > have this patch already available and it was tested with variety of 
> > monitors extensively. So we now read the edid only during init and during hotplug.
> > For SKL, the "[PATCH] drm/i915: Handle HPD when it has actually occurred"
> > patch makes HPD stable enough.
> > So, we can rely on the edid read from init_connector instead of 
> > reading edid on every detect call.
> 
> Again, not going to take this with random gen checks. I want your fix for handling hpd on other platforms, then roll this out everywhere.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Set edid from init and not from detect

[Daniel Vetter]

On Tue, Jul 14, 2015 at 08:09:21AM +0000, Sharma, Shashank wrote:
> Please apply this patch series along with the Interrupt handling fix patch Sonika shared. 
> With these 4 patches applied, we should not see any problems with HPDs (Until the HW is broken). 

Ok, that explains things. If you have depencies please include them all in
your patch series and don't put it all over the mailing lists. I won't be
able to keep track of everything. I was simply confused about why it
should suddenly work without that fix.

Can you please resend the entire series with all ingredients required? And
enabled on all platforms for maximum testing?

Thanks, Daniel

> On a similar note, the corresponding chrome team applied the live status check patch, along with others, and they
> are not seeing the problems with HPD, hence they are also interested in this patch series. 
> 
> Please let us know if you observe something else, we would love to dig further. 
> But as we previously mentioned, this patch series is available and running across various MCG Gen7 devices, and available with
> Gen8 PV production branches also, and there the hotplug stability is pretty good.
> 
> Regards
> Shashank 
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Tuesday, July 14, 2015 1:29 PM
> To: Sharma, Shashank
> Cc: Daniel Vetter; Jindal, Sonika; Chris Wilson; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Set edid from init and not from detect
> 
> On Tue, Jul 14, 2015 at 03:48:36AM +0000, Sharma, Shashank wrote:
> > Hi Daniel, Chris
> > 
> > Gen7 and Gen8 platforms have a different live status register (0x61114) and we need not to rely on ISR on that. 
> > As Sonika mentioned, We have been using this register for our commercial releases, and found them reliable across a wide range of monitors. 
> > 
> > On the other hand, Bsepc clearly mentions, to check the live status before even try to read EDID. 
> > The current DRM nightly code doesn't do that, and we have received few errors from Gen7 Chromebooks where you can still read valid EDID on HDMI hot-unplug. 
> > 
> > So I think this patch and solution is ready, and it should go in. 
> 
> I have a gen7 machine here which is currently (and it's somewhat random) broken wrt hpd and hdmi. And afaics this patch series doesn't have the bugfix for the hpd handling - or did I miss it?
> -Daniel
> 
> > 
> > Regards
> > Shashank
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of 
> > Daniel Vetter
> > Sent: Monday, July 13, 2015 8:27 PM
> > To: Jindal, Sonika
> > Cc: Chris Wilson; intel-gfx@lists.freedesktop.org; Sharma, Shashank
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Set edid from init and not 
> > from detect
> > 
> > On Mon, Jul 13, 2015 at 05:29:12PM +0530, Jindal, Sonika wrote:
> > > 
> > > 
> > > On 7/13/2015 5:10 PM, Chris Wilson wrote:
> > > >On Mon, Jul 13, 2015 at 04:19:15PM +0530, Sonika Jindal wrote:
> > > >>During init_connector set the edid, then edid will be set/unset 
> > > >>only during hotplug. For the sake of older platforms where HPD is 
> > > >>not stable, let edid read happen from detect as well only if it is forced to do so.
> > > >>
> > > >>v2: Removing the 'force' check, instead let detect call read the 
> > > >>edid for platforms older than gen 7 (Shashank)
> > > >
> > > >That's enough worse. We now have a random gen check with no 
> > > >rationale for why you suddenly believe all manufacturers have fixed their wiring.
> > > >There is no reason to believe that gen7 suddenly works is there? If 
> > > >there is, why don't you mention it?
> > > >-Chris
> > > >
> > > This gen7 check is to be on the safer side not to affect older paltforms.
> > > For CHV/VLV, already the live status check is stable enough to 
> > > determine if we can read edid or not. In VPG production branches we 
> > > have this patch already available and it was tested with variety of 
> > > monitors extensively. So we now read the edid only during init and during hotplug.
> > > For SKL, the "[PATCH] drm/i915: Handle HPD when it has actually occurred"
> > > patch makes HPD stable enough.
> > > So, we can rely on the edid read from init_connector instead of 
> > > reading edid on every detect call.
> > 
> > Again, not going to take this with random gen checks. I want your fix for handling hpd on other platforms, then roll this out everywhere.
> > -Daniel
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Set edid from init and not from detect

[Sharma, Shashank]

Sure, Daniel, Thanks. 
Sonika will send the patches in some time.

Regards
Shashank

-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Tuesday, July 14, 2015 2:33 PM
To: Sharma, Shashank
Cc: Daniel Vetter; Jindal, Sonika; Chris Wilson; intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Set edid from init and not from detect

On Tue, Jul 14, 2015 at 08:09:21AM +0000, Sharma, Shashank wrote:
> Please apply this patch series along with the Interrupt handling fix patch Sonika shared. 
> With these 4 patches applied, we should not see any problems with HPDs (Until the HW is broken). 

Ok, that explains things. If you have depencies please include them all in your patch series and don't put it all over the mailing lists. I won't be able to keep track of everything. I was simply confused about why it should suddenly work without that fix.

Can you please resend the entire series with all ingredients required? And enabled on all platforms for maximum testing?

Thanks, Daniel

> On a similar note, the corresponding chrome team applied the live 
> status check patch, along with others, and they are not seeing the problems with HPD, hence they are also interested in this patch series.
> 
> Please let us know if you observe something else, we would love to dig further. 
> But as we previously mentioned, this patch series is available and 
> running across various MCG Gen7 devices, and available with
> Gen8 PV production branches also, and there the hotplug stability is pretty good.
> 
> Regards
> Shashank
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of 
> Daniel Vetter
> Sent: Tuesday, July 14, 2015 1:29 PM
> To: Sharma, Shashank
> Cc: Daniel Vetter; Jindal, Sonika; Chris Wilson; 
> intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Set edid from init and not 
> from detect
> 
> On Tue, Jul 14, 2015 at 03:48:36AM +0000, Sharma, Shashank wrote:
> > Hi Daniel, Chris
> > 
> > Gen7 and Gen8 platforms have a different live status register (0x61114) and we need not to rely on ISR on that. 
> > As Sonika mentioned, We have been using this register for our commercial releases, and found them reliable across a wide range of monitors. 
> > 
> > On the other hand, Bsepc clearly mentions, to check the live status before even try to read EDID. 
> > The current DRM nightly code doesn't do that, and we have received few errors from Gen7 Chromebooks where you can still read valid EDID on HDMI hot-unplug. 
> > 
> > So I think this patch and solution is ready, and it should go in. 
> 
> I have a gen7 machine here which is currently (and it's somewhat random) broken wrt hpd and hdmi. And afaics this patch series doesn't have the bugfix for the hpd handling - or did I miss it?
> -Daniel
> 
> > 
> > Regards
> > Shashank
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of 
> > Daniel Vetter
> > Sent: Monday, July 13, 2015 8:27 PM
> > To: Jindal, Sonika
> > Cc: Chris Wilson; intel-gfx@lists.freedesktop.org; Sharma, Shashank
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Set edid from init and 
> > not from detect
> > 
> > On Mon, Jul 13, 2015 at 05:29:12PM +0530, Jindal, Sonika wrote:
> > > 
> > > 
> > > On 7/13/2015 5:10 PM, Chris Wilson wrote:
> > > >On Mon, Jul 13, 2015 at 04:19:15PM +0530, Sonika Jindal wrote:
> > > >>During init_connector set the edid, then edid will be set/unset 
> > > >>only during hotplug. For the sake of older platforms where HPD 
> > > >>is not stable, let edid read happen from detect as well only if it is forced to do so.
> > > >>
> > > >>v2: Removing the 'force' check, instead let detect call read the 
> > > >>edid for platforms older than gen 7 (Shashank)
> > > >
> > > >That's enough worse. We now have a random gen check with no 
> > > >rationale for why you suddenly believe all manufacturers have fixed their wiring.
> > > >There is no reason to believe that gen7 suddenly works is there? 
> > > >If there is, why don't you mention it?
> > > >-Chris
> > > >
> > > This gen7 check is to be on the safer side not to affect older paltforms.
> > > For CHV/VLV, already the live status check is stable enough to 
> > > determine if we can read edid or not. In VPG production branches 
> > > we have this patch already available and it was tested with 
> > > variety of monitors extensively. So we now read the edid only during init and during hotplug.
> > > For SKL, the "[PATCH] drm/i915: Handle HPD when it has actually occurred"
> > > patch makes HPD stable enough.
> > > So, we can rely on the edid read from init_connector instead of 
> > > reading edid on every detect call.
> > 
> > Again, not going to take this with random gen checks. I want your fix for handling hpd on other platforms, then roll this out everywhere.
> > -Daniel
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation http://blog.ffwll.ch
> 
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915: Check live status before reading edid

[David Weinehall]

On Thu, Jul 09, 2015 at 07:27:57PM +0200, Daniel Vetter wrote:
> On Thu, Jul 09, 2015 at 05:34:29PM +0530, Sonika Jindal wrote:
> > Adding this for SKL onwards.
> > 
> > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> 
> I think this is the critical piece really, and I'd like to roll it out for
> all platforms. git has the code for all the old ones, so no big deal to
> digg that out. Also we need your fix for the interrupt handling first ofc,
> otherwise this won't work.
> 
> Then we can apply this and give it some good testing before we start
> relying on it with caching hdmi probe status. I know this means that
> things will be slower, but I've been burned a bit too much the last few
> times we've tried this. And I really think we need the most amount of
> testing we can get (hence rolling this out for all platforms). If your
> hpd irq handling bugfix is indeed the bug that will be confirmed quickly,
> otherwise it means back to debugging.

"things will be slower" is a bit of an understatement, sadly.
On machines with no external display connected (the typical usecase for
any device with an eDP, such as a laptop, tablet, etc.), the approach to
repeat live status reads until buggy displays can be bothered to reply
makes us spend twice as long as needed on resume.

While it's nice that we can support buggy hardware, I think that we
also, at the very least, should allow for skipping said those
workarounds when not needed. Either by allow for disabling the
lvie status reads (proven to work on older platforms since that was
the default behaviour for a long, long time, and tested to work
on at least Broadwell & Skylake ThinkPads) or by making the number of
LSR reads configurable.

The former would be the simplest solution, the latter would meet the
letter of the spec, and allow for more precise tuning of behaviour;
people who have a display that needs -- say -- for LSR reads to work
reliably shouldn't have to wait for 9.

While allowing for unusual use-cases / buggy hardware is great,
we shouldn't miss out on the benefits that  working hardware can
give to the common use-cases.

Just my 2¢.

I'm feeling sorely tempted to treat this as a proper regression,
and simply submit a revert patch, seeing as it slows down resume by
something like 200ms, but seeing as there was mention of a case where
incorrect EDID-information might end up being read after resume on some
Chromebooks, I'll just try to open a discussion instead.


Kind regards, David Weinehall
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915: Check live status before reading edid

[Daniel Vetter]

On Tue, Jul 12, 2016 at 02:39:47PM +0300, David Weinehall wrote:
> On Thu, Jul 09, 2015 at 07:27:57PM +0200, Daniel Vetter wrote:
> > On Thu, Jul 09, 2015 at 05:34:29PM +0530, Sonika Jindal wrote:
> > > Adding this for SKL onwards.
> > > 
> > > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> > 
> > I think this is the critical piece really, and I'd like to roll it out for
> > all platforms. git has the code for all the old ones, so no big deal to
> > digg that out. Also we need your fix for the interrupt handling first ofc,
> > otherwise this won't work.
> > 
> > Then we can apply this and give it some good testing before we start
> > relying on it with caching hdmi probe status. I know this means that
> > things will be slower, but I've been burned a bit too much the last few
> > times we've tried this. And I really think we need the most amount of
> > testing we can get (hence rolling this out for all platforms). If your
> > hpd irq handling bugfix is indeed the bug that will be confirmed quickly,
> > otherwise it means back to debugging.
> 
> "things will be slower" is a bit of an understatement, sadly.
> On machines with no external display connected (the typical usecase for
> any device with an eDP, such as a laptop, tablet, etc.), the approach to
> repeat live status reads until buggy displays can be bothered to reply
> makes us spend twice as long as needed on resume.
> 
> While it's nice that we can support buggy hardware, I think that we
> also, at the very least, should allow for skipping said those
> workarounds when not needed. Either by allow for disabling the
> lvie status reads (proven to work on older platforms since that was
> the default behaviour for a long, long time, and tested to work
> on at least Broadwell & Skylake ThinkPads) or by making the number of
> LSR reads configurable.

Nack on making probing configurable, that just plain doesn't work. It's
the same hole war I've been fighting against adding the live status check
until vpg realized that they have this hack to make broken sinks work.

> The former would be the simplest solution, the latter would meet the
> letter of the spec, and allow for more precise tuning of behaviour;
> people who have a display that needs -- say -- for LSR reads to work
> reliably shouldn't have to wait for 9.
> 
> While allowing for unusual use-cases / buggy hardware is great,
> we shouldn't miss out on the benefits that  working hardware can
> give to the common use-cases.
> 
> Just my 2¢.
> 
> I'm feeling sorely tempted to treat this as a proper regression,
> and simply submit a revert patch, seeing as it slows down resume by
> something like 200ms, but seeing as there was mention of a case where
> incorrect EDID-information might end up being read after resume on some
> Chromebooks, I'll just try to open a discussion instead.

+1 on reverts from me on principle. No opinion on this case specifically,
but given that we fail the suspend/resume limits since forever I'm
inclined to say "meh" here.

Probably we should add this as a basic/BAT performance metric, but atm
igt/BAT/Jenkins aren't set up to track performance measurements and
automatically report/bisect regressions.

I think the proper solution should be to make all the probing and fbdev
restoring async. As soon as we have working async atomic commit for
modeset we should be able to do all that.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915: Check live status before reading edid

[Chris Wilson]

On Wed, Jul 13, 2016 at 01:59:52PM +0200, Daniel Vetter wrote:
> I think the proper solution should be to make all the probing and fbdev
> restoring async. As soon as we have working async atomic commit for
> modeset we should be able to do all that.

We're not just blocked on the mode change here (indeed, we shouldn't be
changing modes at resume at all, right?) but we appear to be doing a
full detection cycle whereas the intent was just to tell userspace
everything had changed, and for it to go figure out what to do about it.

Also note that we can simply move this all out of the blocking resume
path and still run this in parallel to userspace resuming (and of course
serialised with whatever userspace wants to do).
-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 4/5] drm/i915: Check live status before reading edid

[Chris Wilson]

On Wed, Jul 13, 2016 at 01:09:28PM +0100, Chris Wilson wrote:
> On Wed, Jul 13, 2016 at 01:59:52PM +0200, Daniel Vetter wrote:
> > I think the proper solution should be to make all the probing and fbdev
> > restoring async. As soon as we have working async atomic commit for
> > modeset we should be able to do all that.
> 
> We're not just blocked on the mode change here (indeed, we shouldn't be
> changing modes at resume at all, right?) but we appear to be doing a
> full detection cycle whereas the intent was just to tell userspace
> everything had changed, and for it to go figure out what to do about it.
> 
> Also note that we can simply move this all out of the blocking resume
> path and still run this in parallel to userspace resuming (and of course
> serialised with whatever userspace wants to do).

And to remind myself of conversations going on elsewhere, the more async
we make resume the more inaccurate the current method of measuing resume
time becomes (which more or less just looks at the initcall graph). We
need a metric that measures the time from resume to the time of first
pixel (first flip to a lit display preferably). I've shown how we can
get our "resume time" down to about 10ms - all because the metric is
subject to abuse.
-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 4/5] drm/i915: Check live status before reading edid

[David Weinehall]

On Wed, Jul 13, 2016 at 01:17:40PM +0100, Chris Wilson wrote:
> On Wed, Jul 13, 2016 at 01:09:28PM +0100, Chris Wilson wrote:
> > On Wed, Jul 13, 2016 at 01:59:52PM +0200, Daniel Vetter wrote:
> > > I think the proper solution should be to make all the probing and fbdev
> > > restoring async. As soon as we have working async atomic commit for
> > > modeset we should be able to do all that.
> > 
> > We're not just blocked on the mode change here (indeed, we shouldn't be
> > changing modes at resume at all, right?) but we appear to be doing a
> > full detection cycle whereas the intent was just to tell userspace
> > everything had changed, and for it to go figure out what to do about it.
> > 
> > Also note that we can simply move this all out of the blocking resume
> > path and still run this in parallel to userspace resuming (and of course
> > serialised with whatever userspace wants to do).
> 
> And to remind myself of conversations going on elsewhere, the more async
> we make resume the more inaccurate the current method of measuing resume
> time becomes (which more or less just looks at the initcall graph). We
> need a metric that measures the time from resume to the time of first
> pixel (first flip to a lit display preferably). I've shown how we can
> get our "resume time" down to about 10ms - all because the metric is
> subject to abuse.

Good news on this front -- it seems that the SuspendResume tool can be
adapted to measure our resume-time even if we "cheat", and the author
offered to help with this.  So we "just" need to decide what actually
constitutes being done with resume.


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

Re: [PATCH 4/5] drm/i915: Check live status before reading edid

[Chris Wilson]

On Tue, Jul 12, 2016 at 02:39:47PM +0300, David Weinehall wrote:
> On Thu, Jul 09, 2015 at 07:27:57PM +0200, Daniel Vetter wrote:
> > On Thu, Jul 09, 2015 at 05:34:29PM +0530, Sonika Jindal wrote:
> > > Adding this for SKL onwards.
> > > 
> > > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> > 
> > I think this is the critical piece really, and I'd like to roll it out for
> > all platforms. git has the code for all the old ones, so no big deal to
> > digg that out. Also we need your fix for the interrupt handling first ofc,
> > otherwise this won't work.
> > 
> > Then we can apply this and give it some good testing before we start
> > relying on it with caching hdmi probe status. I know this means that
> > things will be slower, but I've been burned a bit too much the last few
> > times we've tried this. And I really think we need the most amount of
> > testing we can get (hence rolling this out for all platforms). If your
> > hpd irq handling bugfix is indeed the bug that will be confirmed quickly,
> > otherwise it means back to debugging.
> 
> "things will be slower" is a bit of an understatement, sadly.
> On machines with no external display connected (the typical usecase for
> any device with an eDP, such as a laptop, tablet, etc.), the approach to
> repeat live status reads until buggy displays can be bothered to reply
> makes us spend twice as long as needed on resume.

Also this is causing stall during runtime hotplug. ~90ms stall causes
3-4 frames to be dropped in 24Hz movie. Even a single frame dropped
during movie playback is enough to be noticed.

This coupled with 

commit f8d03ea0053b23de42c828d559016eabe0b91523
Author: Gary Wang <gary.c.wang@intel.com>
Date:   Wed Dec 23 16:11:35 2015 +0800

    drm/i915: increase the tries for HDMI hotplug live status checking

which purports to "fix" live status for bdw+.

Afaict using live status is as broken as we told you it would be from
earlier experience.
-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 4/5] drm/i915: Check live status before reading edid

[Chris Wilson]

On Tue, Jul 12, 2016 at 02:39:47PM +0300, David Weinehall wrote:
> I'm feeling sorely tempted to treat this as a proper regression,
> and simply submit a revert patch, seeing as it slows down resume by
> something like 200ms, but seeing as there was mention of a case where
> incorrect EDID-information might end up being read after resume on some
> Chromebooks, I'll just try to open a discussion instead.

Wrt https://bugs.freedesktop.org/show_bug.cgi?id=97139 this is a
regression and I'll ack the revert. Yes, we need to resolve exactly how
we get a stall between intel_hdmi_detect() and pageflip/cursor, but the
onus is on the submitter of the patch to fix existing issues first to
prevent such regressions (if possible, or else mitigate them).
-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 4/5] drm/i915: Check live status before reading edid

[Jindal, Sonika]

Yes we had the issue of incorrect edid read.
But as of now you can go ahead with the revert.
I have moved to a different group, so I am not working on this anymore.

Regards,
Sonika

-----Original Message-----
From: Chris Wilson [mailto:chris@chris-wilson.co.uk] 
Sent: Tuesday, August 2, 2016 8:02 PM
To: Daniel Vetter <daniel@ffwll.ch>; Jindal, Sonika <sonika.jindal@intel.com>; intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 4/5] drm/i915: Check live status before reading edid

On Tue, Jul 12, 2016 at 02:39:47PM +0300, David Weinehall wrote:
> I'm feeling sorely tempted to treat this as a proper regression, and 
> simply submit a revert patch, seeing as it slows down resume by 
> something like 200ms, but seeing as there was mention of a case where 
> incorrect EDID-information might end up being read after resume on 
> some Chromebooks, I'll just try to open a discussion instead.

Wrt https://bugs.freedesktop.org/show_bug.cgi?id=97139 this is a regression and I'll ack the revert. Yes, we need to resolve exactly how we get a stall between intel_hdmi_detect() and pageflip/cursor, but the onus is on the submitter of the patch to fix existing issues first to prevent such regressions (if possible, or else mitigate them).
-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: Check live status before reading edid

[Sharma, Shashank]

Hi all, 

I have sent one patch with live_status check restricted to gen7 and +, for this regression.
drm/i915: Restrict usage of live status check
Please help in review. 

Regards
Shashank
-----Original Message-----
From: Sharma, Shashank 
Sent: Wednesday, March 09, 2016 11:25 AM
To: Jindal, Sonika; Chris Wilson
Cc: intel-gfx@lists.freedesktop.org; Mukherjee, Indranil
Subject: RE: [Intel-gfx] [PATCH] drm/i915: Check live status before reading edid

We are procuring the DVI-single-link cable, as we don't have one with us. 
Sonika tested the Dual link cable, and that was working well. 

We can do two things here: 
- Add the gen check, which will allow the live_status check only for VLV and + platforms, others will go as it is. 
- Wait for some more time, get the cable and try to rep and fix the issue. 

Regards
Shashank
-----Original Message-----
From: Jindal, Sonika
Sent: Wednesday, March 09, 2016 11:23 AM
To: Chris Wilson; Sharma, Shashank
Cc: intel-gfx@lists.freedesktop.org
Subject: RE: [Intel-gfx] [PATCH] drm/i915: Check live status before reading edid

+Shashank

Shashank was planning to give a patch to bypass live status checks for older platforms.

Regards,
Sonika

-----Original Message-----
From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
Sent: Wednesday, March 9, 2016 2:34 AM
To: Jindal, Sonika <sonika.jindal@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Check live status before reading edid

On Tue, Sep 15, 2015 at 09:44:20AM +0530, Sonika Jindal wrote:
> The Bspec is very clear that Live status must be checked about before 
> trying to read EDID over DDC channel. This patch makes sure that HDMI 
> EDID is read only when live status is up.
> 
> The live status doesn't seem to perform very consistent across various 
> platforms when tested with different monitors. The reason behind that 
> is some monitors are late to provide right voltage to set live_status up.
> So, after getting the interrupt, for a small duration, live status reg 
> fluctuates, and then settles down showing the correct staus.
> 
> This is explained here in, in a rough way:
> HPD line  ________________
> 			 |\ T1 = Monitor Hotplug causing IRQ
> 			 | \______________________________________
> 			 | |
>                          | |
> 			 | |   T2 = Live status is stable
> 			 | |  _____________________________________
> 			 | | /|
> Live status _____________|_|/ |
> 			 | |  |
> 			 | |  |
> 			 | |  |
> 			T0 T1  T2
> 
> (Between T1 and T2 Live status fluctuates or can be even low, 
> depending on  the monitor)
> 
> After several experiments, we have concluded that a max delay of 30ms 
> is enough to allow the live status to settle down with most of the 
> monitors. This total delay of 30ms has been split into a resolution of
> 3 retries of 10ms each, for the better cases.
> 
> This delay is kept at 30ms, keeping in consideration that, HDCP 
> compliance expect the HPD handler to respond a plug out in 100ms, by disabling port.

This is a regression-fest. Revert with stable@?
-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: Check live status before reading edid

[Sharma, Shashank]

We are procuring the DVI-single-link cable, as we don't have one with us. 
Sonika tested the Dual link cable, and that was working well. 

We can do two things here: 
- Add the gen check, which will allow the live_status check only for VLV and + platforms, others will go as it is. 
- Wait for some more time, get the cable and try to rep and fix the issue. 

Regards
Shashank
-----Original Message-----
From: Jindal, Sonika 
Sent: Wednesday, March 09, 2016 11:23 AM
To: Chris Wilson; Sharma, Shashank
Cc: intel-gfx@lists.freedesktop.org
Subject: RE: [Intel-gfx] [PATCH] drm/i915: Check live status before reading edid

+Shashank

Shashank was planning to give a patch to bypass live status checks for older platforms.

Regards,
Sonika

-----Original Message-----
From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
Sent: Wednesday, March 9, 2016 2:34 AM
To: Jindal, Sonika <sonika.jindal@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Check live status before reading edid

On Tue, Sep 15, 2015 at 09:44:20AM +0530, Sonika Jindal wrote:
> The Bspec is very clear that Live status must be checked about before 
> trying to read EDID over DDC channel. This patch makes sure that HDMI 
> EDID is read only when live status is up.
> 
> The live status doesn't seem to perform very consistent across various 
> platforms when tested with different monitors. The reason behind that 
> is some monitors are late to provide right voltage to set live_status up.
> So, after getting the interrupt, for a small duration, live status reg 
> fluctuates, and then settles down showing the correct staus.
> 
> This is explained here in, in a rough way:
> HPD line  ________________
> 			 |\ T1 = Monitor Hotplug causing IRQ
> 			 | \______________________________________
> 			 | |
>                          | |
> 			 | |   T2 = Live status is stable
> 			 | |  _____________________________________
> 			 | | /|
> Live status _____________|_|/ |
> 			 | |  |
> 			 | |  |
> 			 | |  |
> 			T0 T1  T2
> 
> (Between T1 and T2 Live status fluctuates or can be even low, 
> depending on  the monitor)
> 
> After several experiments, we have concluded that a max delay of 30ms 
> is enough to allow the live status to settle down with most of the 
> monitors. This total delay of 30ms has been split into a resolution of
> 3 retries of 10ms each, for the better cases.
> 
> This delay is kept at 30ms, keeping in consideration that, HDCP 
> compliance expect the HPD handler to respond a plug out in 100ms, by disabling port.

This is a regression-fest. Revert with stable@?
-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: Check live status before reading edid

[Jindal, Sonika]

+Shashank

Shashank was planning to give a patch to bypass live status checks for older platforms.

Regards,
Sonika

-----Original Message-----
From: Chris Wilson [mailto:chris@chris-wilson.co.uk] 
Sent: Wednesday, March 9, 2016 2:34 AM
To: Jindal, Sonika <sonika.jindal@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Check live status before reading edid

On Tue, Sep 15, 2015 at 09:44:20AM +0530, Sonika Jindal wrote:
> The Bspec is very clear that Live status must be checked about before 
> trying to read EDID over DDC channel. This patch makes sure that HDMI 
> EDID is read only when live status is up.
> 
> The live status doesn't seem to perform very consistent across various 
> platforms when tested with different monitors. The reason behind that 
> is some monitors are late to provide right voltage to set live_status up.
> So, after getting the interrupt, for a small duration, live status reg 
> fluctuates, and then settles down showing the correct staus.
> 
> This is explained here in, in a rough way:
> HPD line  ________________
> 			 |\ T1 = Monitor Hotplug causing IRQ
> 			 | \______________________________________
> 			 | |
>                          | |
> 			 | |   T2 = Live status is stable
> 			 | |  _____________________________________
> 			 | | /|
> Live status _____________|_|/ |
> 			 | |  |
> 			 | |  |
> 			 | |  |
> 			T0 T1  T2
> 
> (Between T1 and T2 Live status fluctuates or can be even low, 
> depending on  the monitor)
> 
> After several experiments, we have concluded that a max delay of 30ms 
> is enough to allow the live status to settle down with most of the 
> monitors. This total delay of 30ms has been split into a resolution of 
> 3 retries of 10ms each, for the better cases.
> 
> This delay is kept at 30ms, keeping in consideration that, HDCP 
> compliance expect the HPD handler to respond a plug out in 100ms, by disabling port.

This is a regression-fest. Revert with stable@?
-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: Check live status before reading edid

[Chris Wilson]

On Tue, Sep 15, 2015 at 09:44:20AM +0530, Sonika Jindal wrote:
> The Bspec is very clear that Live status must be checked about before
> trying to read EDID over DDC channel. This patch makes sure that HDMI
> EDID is read only when live status is up.
> 
> The live status doesn't seem to perform very consistent across various
> platforms when tested with different monitors. The reason behind that is
> some monitors are late to provide right voltage to set live_status up.
> So, after getting the interrupt, for a small duration, live status reg
> fluctuates, and then settles down showing the correct staus.
> 
> This is explained here in, in a rough way:
> HPD line  ________________
> 			 |\ T1 = Monitor Hotplug causing IRQ
> 			 | \______________________________________
> 			 | |
>                          | |
> 			 | |   T2 = Live status is stable
> 			 | |  _____________________________________
> 			 | | /|
> Live status _____________|_|/ |
> 			 | |  |
> 			 | |  |
> 			 | |  |
> 			T0 T1  T2
> 
> (Between T1 and T2 Live status fluctuates or can be even low, depending on
>  the monitor)
> 
> After several experiments, we have concluded that a max delay
> of 30ms is enough to allow the live status to settle down with
> most of the monitors. This total delay of 30ms has been split into
> a resolution of 3 retries of 10ms each, for the better cases.
> 
> This delay is kept at 30ms, keeping in consideration that, HDCP compliance
> expect the HPD handler to respond a plug out in 100ms, by disabling port.

This is a regression-fest. Revert with stable@?
-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: Check live status before reading edid

[Daniel Vetter]

On Tue, Sep 15, 2015 at 09:44:20AM +0530, Sonika Jindal wrote:
> The Bspec is very clear that Live status must be checked about before
> trying to read EDID over DDC channel. This patch makes sure that HDMI
> EDID is read only when live status is up.
> 
> The live status doesn't seem to perform very consistent across various
> platforms when tested with different monitors. The reason behind that is
> some monitors are late to provide right voltage to set live_status up.
> So, after getting the interrupt, for a small duration, live status reg
> fluctuates, and then settles down showing the correct staus.
> 
> This is explained here in, in a rough way:
> HPD line  ________________
> 			 |\ T1 = Monitor Hotplug causing IRQ
> 			 | \______________________________________
> 			 | |
>                          | |
> 			 | |   T2 = Live status is stable
> 			 | |  _____________________________________
> 			 | | /|
> Live status _____________|_|/ |
> 			 | |  |
> 			 | |  |
> 			 | |  |
> 			T0 T1  T2
> 
> (Between T1 and T2 Live status fluctuates or can be even low, depending on
>  the monitor)
> 
> After several experiments, we have concluded that a max delay
> of 30ms is enough to allow the live status to settle down with
> most of the monitors. This total delay of 30ms has been split into
> a resolution of 3 retries of 10ms each, for the better cases.
> 
> This delay is kept at 30ms, keeping in consideration that, HDCP compliance
> expect the HPD handler to respond a plug out in 100ms, by disabling port.
> 
> v2: Adding checks for VLV/CHV as well. Reusing old ibx and g4x functions
> to check digital port status. Adding a separate function to get bxt live
> status (Daniel)
> v3: Using intel_encoder->hpd_pin to check the live status (Siva)
> Moving the live status read to intel_hdmi_probe and passing parameter
> to read/not to read the edid. (me)
> v4:
> * Added live status check for all platforms using
> intel_digital_port_connected.
> * Rebased on top of Jani's DP cleanup series
> * Some monitors take time in setting the live status. So retry for few
> times if this is a connect HPD
> v5: Removed extra "drm/i915" from commit message. Adding Shashank's sob
>     which was missed.
> v6: Drop the (!detect_edid && !live_status check) check because for DDI
> ports which are enumerated as hdmi as well as DP, we don't have a
> mechanism to differentiate between DP and hdmi inside the encoder's
> hot_plug. This leads to call to the hdmi's hot_plug hook for DP as well
> as hdmi which leads to issues during unplug because of the above check.
> v7: Make intel_digital_port_connected global in this patch, some
> reformatting of while loop, adding a print when live status is not
> up. (Rodrigo)
> v8: Rebase it on nightly which involved skipping the hot_plug hook for now
> and letting the live_status check happen in detect until the hpd handling
> part is finalized (Daniel)
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Queued for -next, thanks for the patch.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_dp.c   |    2 +-
>  drivers/gpu/drm/i915/intel_drv.h  |    2 ++
>  drivers/gpu/drm/i915/intel_hdmi.c |   28 +++++++++++++++++++++-------
>  3 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a687250..a6e22dd 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4670,7 +4670,7 @@ static bool bxt_digital_port_connected(struct drm_i915_private *dev_priv,
>   *
>   * Return %true if @port is connected, %false otherwise.
>   */
> -static bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
> +bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
>  					 struct intel_digital_port *port)
>  {
>  	if (HAS_PCH_IBX(dev_priv))
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 08ba362..1e01350 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1210,6 +1210,8 @@ void intel_edp_drrs_disable(struct intel_dp *intel_dp);
>  void intel_edp_drrs_invalidate(struct drm_device *dev,
>  		unsigned frontbuffer_bits);
>  void intel_edp_drrs_flush(struct drm_device *dev, unsigned frontbuffer_bits);
> +bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
> +					 struct intel_digital_port *port);
>  void hsw_dp_set_ddi_pll_sel(struct intel_crtc_state *pipe_config);
>  
>  /* intel_dp_mst.c */
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index e978c59..bb33c66 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1329,22 +1329,23 @@ intel_hdmi_unset_edid(struct drm_connector *connector)
>  }
>  
>  static bool
> -intel_hdmi_set_edid(struct drm_connector *connector)
> +intel_hdmi_set_edid(struct drm_connector *connector, bool force)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(connector->dev);
>  	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
>  	struct intel_encoder *intel_encoder =
>  		&hdmi_to_dig_port(intel_hdmi)->base;
>  	enum intel_display_power_domain power_domain;
> -	struct edid *edid;
> +	struct edid *edid = NULL;
>  	bool connected = false;
>  
>  	power_domain = intel_display_port_power_domain(intel_encoder);
>  	intel_display_power_get(dev_priv, power_domain);
>  
> -	edid = drm_get_edid(connector,
> -			    intel_gmbus_get_adapter(dev_priv,
> -						    intel_hdmi->ddc_bus));
> +	if (force)
> +		edid = drm_get_edid(connector,
> +				    intel_gmbus_get_adapter(dev_priv,
> +				    intel_hdmi->ddc_bus));
>  
>  	intel_display_power_put(dev_priv, power_domain);
>  
> @@ -1372,13 +1373,26 @@ static enum drm_connector_status
>  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 retry = 3;
>  
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>  		      connector->base.id, connector->name);
>  
> +	while (!live_status && --retry) {
> +		live_status = intel_digital_port_connected(dev_priv,
> +				hdmi_to_dig_port(intel_hdmi));
> +		mdelay(10);
> +	}
> +
> +	if (!live_status)
> +		DRM_DEBUG_KMS("Live status not up!");
> +
>  	intel_hdmi_unset_edid(connector);
>  
> -	if (intel_hdmi_set_edid(connector)) {
> +	if (intel_hdmi_set_edid(connector, live_status)) {
>  		struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
>  
>  		hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
> @@ -1402,7 +1416,7 @@ intel_hdmi_force(struct drm_connector *connector)
>  	if (connector->status != connector_status_connected)
>  		return;
>  
> -	intel_hdmi_set_edid(connector);
> +	intel_hdmi_set_edid(connector, true);
>  	hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
>  }
>  
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Check live status before reading edid

[Sonika Jindal]

The Bspec is very clear that Live status must be checked about before
trying to read EDID over DDC channel. This patch makes sure that HDMI
EDID is read only when live status is up.

The live status doesn't seem to perform very consistent across various
platforms when tested with different monitors. The reason behind that is
some monitors are late to provide right voltage to set live_status up.
So, after getting the interrupt, for a small duration, live status reg
fluctuates, and then settles down showing the correct staus.

This is explained here in, in a rough way:
HPD line  ________________
			 |\ T1 = Monitor Hotplug causing IRQ
			 | \______________________________________
			 | |
                         | |
			 | |   T2 = Live status is stable
			 | |  _____________________________________
			 | | /|
Live status _____________|_|/ |
			 | |  |
			 | |  |
			 | |  |
			T0 T1  T2

(Between T1 and T2 Live status fluctuates or can be even low, depending on
 the monitor)

After several experiments, we have concluded that a max delay
of 30ms is enough to allow the live status to settle down with
most of the monitors. This total delay of 30ms has been split into
a resolution of 3 retries of 10ms each, for the better cases.

This delay is kept at 30ms, keeping in consideration that, HDCP compliance
expect the HPD handler to respond a plug out in 100ms, by disabling port.

v2: Adding checks for VLV/CHV as well. Reusing old ibx and g4x functions
to check digital port status. Adding a separate function to get bxt live
status (Daniel)
v3: Using intel_encoder->hpd_pin to check the live status (Siva)
Moving the live status read to intel_hdmi_probe and passing parameter
to read/not to read the edid. (me)
v4:
* Added live status check for all platforms using
intel_digital_port_connected.
* Rebased on top of Jani's DP cleanup series
* Some monitors take time in setting the live status. So retry for few
times if this is a connect HPD
v5: Removed extra "drm/i915" from commit message. Adding Shashank's sob
    which was missed.
v6: Drop the (!detect_edid && !live_status check) check because for DDI
ports which are enumerated as hdmi as well as DP, we don't have a
mechanism to differentiate between DP and hdmi inside the encoder's
hot_plug. This leads to call to the hdmi's hot_plug hook for DP as well
as hdmi which leads to issues during unplug because of the above check.
v7: Make intel_digital_port_connected global in this patch, some
reformatting of while loop, adding a print when live status is not
up. (Rodrigo)
v8: Rebase it on nightly which involved skipping the hot_plug hook for now
and letting the live_status check happen in detect until the hpd handling
part is finalized (Daniel)

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c   |    2 +-
 drivers/gpu/drm/i915/intel_drv.h  |    2 ++
 drivers/gpu/drm/i915/intel_hdmi.c |   28 +++++++++++++++++++++-------
 3 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a687250..a6e22dd 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4670,7 +4670,7 @@ static bool bxt_digital_port_connected(struct drm_i915_private *dev_priv,
  *
  * Return %true if @port is connected, %false otherwise.
  */
-static bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
+bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
 					 struct intel_digital_port *port)
 {
 	if (HAS_PCH_IBX(dev_priv))
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 08ba362..1e01350 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1210,6 +1210,8 @@ void intel_edp_drrs_disable(struct intel_dp *intel_dp);
 void intel_edp_drrs_invalidate(struct drm_device *dev,
 		unsigned frontbuffer_bits);
 void intel_edp_drrs_flush(struct drm_device *dev, unsigned frontbuffer_bits);
+bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
+					 struct intel_digital_port *port);
 void hsw_dp_set_ddi_pll_sel(struct intel_crtc_state *pipe_config);
 
 /* intel_dp_mst.c */
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index e978c59..bb33c66 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1329,22 +1329,23 @@ intel_hdmi_unset_edid(struct drm_connector *connector)
 }
 
 static bool
-intel_hdmi_set_edid(struct drm_connector *connector)
+intel_hdmi_set_edid(struct drm_connector *connector, bool force)
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->dev);
 	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
 	struct intel_encoder *intel_encoder =
 		&hdmi_to_dig_port(intel_hdmi)->base;
 	enum intel_display_power_domain power_domain;
-	struct edid *edid;
+	struct edid *edid = NULL;
 	bool connected = false;
 
 	power_domain = intel_display_port_power_domain(intel_encoder);
 	intel_display_power_get(dev_priv, power_domain);
 
-	edid = drm_get_edid(connector,
-			    intel_gmbus_get_adapter(dev_priv,
-						    intel_hdmi->ddc_bus));
+	if (force)
+		edid = drm_get_edid(connector,
+				    intel_gmbus_get_adapter(dev_priv,
+				    intel_hdmi->ddc_bus));
 
 	intel_display_power_put(dev_priv, power_domain);
 
@@ -1372,13 +1373,26 @@ static enum drm_connector_status
 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 retry = 3;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
 		      connector->base.id, connector->name);
 
+	while (!live_status && --retry) {
+		live_status = intel_digital_port_connected(dev_priv,
+				hdmi_to_dig_port(intel_hdmi));
+		mdelay(10);
+	}
+
+	if (!live_status)
+		DRM_DEBUG_KMS("Live status not up!");
+
 	intel_hdmi_unset_edid(connector);
 
-	if (intel_hdmi_set_edid(connector)) {
+	if (intel_hdmi_set_edid(connector, live_status)) {
 		struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
 
 		hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
@@ -1402,7 +1416,7 @@ intel_hdmi_force(struct drm_connector *connector)
 	if (connector->status != connector_status_connected)
 		return;
 
-	intel_hdmi_set_edid(connector);
+	intel_hdmi_set_edid(connector, true);
 	hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
 }
 
-- 
1.7.10.4

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

Re: [PATCH] drm/i915: Check live status before reading edid

[Daniel Vetter]

On Mon, Sep 14, 2015 at 02:44:01PM +0530, Jindal, Sonika wrote:
> 
> 
> On 9/14/2015 2:12 PM, Daniel Vetter wrote:
> >On Fri, Sep 11, 2015 at 05:56:47PM +0000, Rodrigo Vivi wrote:
> >>Thanks
> >>
> >>Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >>
> >>
> >>On Fri, Sep 11, 2015 at 4:38 AM Sonika Jindal <sonika.jindal@intel.com>
> >>wrote:
> >>
> >>>The Bspec is very clear that Live status must be checked about before
> >>>trying to read EDID over DDC channel. This patch makes sure that HDMI
> >>>EDID is read only when live status is up.
> >>>
> >>>The live status doesn't seem to perform very consistent across various
> >>>platforms when tested with different monitors. The reason behind that is
> >>>some monitors are late to provide right voltage to set live_status up.
> >>>So, after getting the interrupt, for a small duration, live status reg
> >>>fluctuates, and then settles down showing the correct staus.
> >>>
> >>>This is explained here in, in a rough way:
> >>>HPD line  ________________
> >>>                          |\ T1 = Monitor Hotplug causing IRQ
> >>>                          | \______________________________________
> >>>                          | |
> >>>                          | |
> >>>                          | |   T2 = Live status is stable
> >>>                          | |  _____________________________________
> >>>                          | | /|
> >>>Live status _____________|_|/ |
> >>>                          | |  |
> >>>                          | |  |
> >>>                          | |  |
> >>>                         T0 T1  T2
> >>>
> >>>(Between T1 and T2 Live status fluctuates or can be even low, depending on
> >>>  the monitor)
> >>>
> >>>After several experiments, we have concluded that a max delay
> >>>of 30ms is enough to allow the live status to settle down with
> >>>most of the monitors. This total delay of 30ms has been split into
> >>>a resolution of 3 retries of 10ms each, for the better cases.
> >>>
> >>>This delay is kept at 30ms, keeping in consideration that, HDCP compliance
> >>>expect the HPD handler to respond a plug out in 100ms, by disabling port.
> >>>
> >>>v2: Adding checks for VLV/CHV as well. Reusing old ibx and g4x functions
> >>>to check digital port status. Adding a separate function to get bxt live
> >>>status (Daniel)
> >>>v3: Using intel_encoder->hpd_pin to check the live status (Siva)
> >>>Moving the live status read to intel_hdmi_probe and passing parameter
> >>>to read/not to read the edid. (me)
> >>>v4:
> >>>* Added live status check for all platforms using
> >>>intel_digital_port_connected.
> >>>* Rebased on top of Jani's DP cleanup series
> >>>* Some monitors take time in setting the live status. So retry for few
> >>>times if this is a connect HPD
> >>>v5: Removed extra "drm/i915" from commit message. Adding Shashank's sob
> >>>     which was missed.
> >>>v6: Drop the (!detect_edid && !live_status check) check because for DDI
> >>>ports which are enumerated as hdmi as well as DP, we don't have a
> >>>mechanism to differentiate between DP and hdmi inside the encoder's
> >>>hot_plug. This leads to call to the hdmi's hot_plug hook for DP as well
> >>>as hdmi which leads to issues during unplug because of the above check.
> >>>v7: Make intel_digital_port_connected global in this patch, some
> >>>reformatting of while loop, adding a print when live status is not
> >>>up. (Rodrigo)
> >>>
> >>>Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >>>Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> >
> >Since this is tricky stuff and other patches in this series are blocked
> >until we have a clearer picture can you please rebase this to be the first
> >patch on top of -nightly so that I can pull it in directly?
> >
> >I tried to do that but proved a bit too messy.
> >-Daniel
> >
> Hmm, since rebasing this will require these changes (check for live status)
> to go in detect and then later when we reach a conclusion on
> hot_plug hook, we would need to move it to that function.
> I think lets wait for the conclusion on the placement of that function.
> What do you suggest?

I think given how tricky this patch here is with testing it's preferably
to get it int as soon as possible. Yes that means shuffling things around
twice. But since the structure of how we'll handle hpd isn't clear yet at
all there's a really high chance that you need to rework this patch
anyway. So I don't expect doing this rebase right now will be more work.
And it will give us better testing coverage, which is always good to have.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Check live status before reading edid

[Jindal, Sonika]

On 9/14/2015 2:12 PM, Daniel Vetter wrote:
> On Fri, Sep 11, 2015 at 05:56:47PM +0000, Rodrigo Vivi wrote:
>> Thanks
>>
>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>
>>
>> On Fri, Sep 11, 2015 at 4:38 AM Sonika Jindal <sonika.jindal@intel.com>
>> wrote:
>>
>>> The Bspec is very clear that Live status must be checked about before
>>> trying to read EDID over DDC channel. This patch makes sure that HDMI
>>> EDID is read only when live status is up.
>>>
>>> The live status doesn't seem to perform very consistent across various
>>> platforms when tested with different monitors. The reason behind that is
>>> some monitors are late to provide right voltage to set live_status up.
>>> So, after getting the interrupt, for a small duration, live status reg
>>> fluctuates, and then settles down showing the correct staus.
>>>
>>> This is explained here in, in a rough way:
>>> HPD line  ________________
>>>                           |\ T1 = Monitor Hotplug causing IRQ
>>>                           | \______________________________________
>>>                           | |
>>>                           | |
>>>                           | |   T2 = Live status is stable
>>>                           | |  _____________________________________
>>>                           | | /|
>>> Live status _____________|_|/ |
>>>                           | |  |
>>>                           | |  |
>>>                           | |  |
>>>                          T0 T1  T2
>>>
>>> (Between T1 and T2 Live status fluctuates or can be even low, depending on
>>>   the monitor)
>>>
>>> After several experiments, we have concluded that a max delay
>>> of 30ms is enough to allow the live status to settle down with
>>> most of the monitors. This total delay of 30ms has been split into
>>> a resolution of 3 retries of 10ms each, for the better cases.
>>>
>>> This delay is kept at 30ms, keeping in consideration that, HDCP compliance
>>> expect the HPD handler to respond a plug out in 100ms, by disabling port.
>>>
>>> v2: Adding checks for VLV/CHV as well. Reusing old ibx and g4x functions
>>> to check digital port status. Adding a separate function to get bxt live
>>> status (Daniel)
>>> v3: Using intel_encoder->hpd_pin to check the live status (Siva)
>>> Moving the live status read to intel_hdmi_probe and passing parameter
>>> to read/not to read the edid. (me)
>>> v4:
>>> * Added live status check for all platforms using
>>> intel_digital_port_connected.
>>> * Rebased on top of Jani's DP cleanup series
>>> * Some monitors take time in setting the live status. So retry for few
>>> times if this is a connect HPD
>>> v5: Removed extra "drm/i915" from commit message. Adding Shashank's sob
>>>      which was missed.
>>> v6: Drop the (!detect_edid && !live_status check) check because for DDI
>>> ports which are enumerated as hdmi as well as DP, we don't have a
>>> mechanism to differentiate between DP and hdmi inside the encoder's
>>> hot_plug. This leads to call to the hdmi's hot_plug hook for DP as well
>>> as hdmi which leads to issues during unplug because of the above check.
>>> v7: Make intel_digital_port_connected global in this patch, some
>>> reformatting of while loop, adding a print when live status is not
>>> up. (Rodrigo)
>>>
>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>
> Since this is tricky stuff and other patches in this series are blocked
> until we have a clearer picture can you please rebase this to be the first
> patch on top of -nightly so that I can pull it in directly?
>
> I tried to do that but proved a bit too messy.
> -Daniel
>
Hmm, since rebasing this will require these changes (check for live 
status) to go in detect and then later when we reach a conclusion on
hot_plug hook, we would need to move it to that function.
I think lets wait for the conclusion on the placement of that function.
What do you suggest?

Regards,
Sonika
>>> ---
>>>   drivers/gpu/drm/i915/intel_dp.c   |    2 +-
>>>   drivers/gpu/drm/i915/intel_drv.h  |    2 ++
>>>   drivers/gpu/drm/i915/intel_hdmi.c |   26 +++++++++++++++++++-------
>>>   3 files changed, 22 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>> b/drivers/gpu/drm/i915/intel_dp.c
>>> index bf17030..fedf6d1 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -4695,7 +4695,7 @@ static bool bxt_digital_port_connected(struct
>>> drm_i915_private *dev_priv,
>>>    *
>>>    * Return %true if @port is connected, %false otherwise.
>>>    */
>>> -static bool intel_digital_port_connected(struct drm_i915_private
>>> *dev_priv,
>>> +bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
>>>                                           struct intel_digital_port *port)
>>>   {
>>>          if (HAS_PCH_IBX(dev_priv))
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>>> b/drivers/gpu/drm/i915/intel_drv.h
>>> index b6c2c20..ac6d748 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -1210,6 +1210,8 @@ void intel_edp_drrs_disable(struct intel_dp
>>> *intel_dp);
>>>   void intel_edp_drrs_invalidate(struct drm_device *dev,
>>>                  unsigned frontbuffer_bits);
>>>   void intel_edp_drrs_flush(struct drm_device *dev, unsigned
>>> frontbuffer_bits);
>>> +bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
>>> +                                        struct intel_digital_port *port);
>> g>
>>>   /* intel_dp_mst.c */
>>>   int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port,
>>> int conn_id);
>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
>>> b/drivers/gpu/drm/i915/intel_hdmi.c
>>> index 1eda71a..d366ca5 100644
>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>>> @@ -1329,22 +1329,23 @@ intel_hdmi_unset_edid(struct drm_connector
>>> *connector)
>>>   }
>>>
>>>   static bool
>>> -intel_hdmi_set_edid(struct drm_connector *connector)
>>> +intel_hdmi_set_edid(struct drm_connector *connector, bool force)
>>>   {
>>>          struct drm_i915_private *dev_priv = to_i915(connector->dev);
>>>          struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
>>>          struct intel_encoder *intel_encoder =
>>>                  &hdmi_to_dig_port(intel_hdmi)->base;
>>>          enum intel_display_power_domain power_domain;
>>> -       struct edid *edid;
>>> +       struct edid *edid = NULL;
>>>          bool connected = false;
>>>
>>>          power_domain = intel_display_port_power_domain(intel_encoder);
>>>          intel_display_power_get(dev_priv, power_domain);
>>>
>>> -       edid = drm_get_edid(connector,
>>> -                           intel_gmbus_get_adapter(dev_priv,
>>> -                                                   intel_hdmi->ddc_bus));
>>> +       if (force)
>>> +               edid = drm_get_edid(connector,
>>> +                                   intel_gmbus_get_adapter(dev_priv,
>>> +                                   intel_hdmi->ddc_bus));
>>>
>>>          intel_display_power_put(dev_priv, power_domain);
>>>
>>> @@ -1374,14 +1375,25 @@ void intel_hdmi_probe(struct intel_encoder
>>> *intel_encoder)
>>>                          enc_to_intel_hdmi(&intel_encoder->base);
>>>          struct intel_connector *intel_connector =
>>>                                  intel_hdmi->attached_connector;
>>> +       struct drm_i915_private *dev_priv =
>>> to_i915(intel_encoder->base.dev);
>>> +       bool live_status = false;
>>> +       unsigned int retry = 3;
>>> +
>>> +       while (!live_status && --retry) {
>>> +               live_status = intel_digital_port_connected(dev_priv,
>>> +                               hdmi_to_dig_port(intel_hdmi));
>>> +               mdelay(10);
>>> +       }
>>>
>>> +       if (!live_status)
>>> +               DRM_DEBUG_KMS("Live status not up!");
>>>          /*
>>>           * We are here, means there is a hotplug or a force
>>>           * detection. Clear the cached EDID and probe the
>>>           * DDC bus to check the current status of HDMI.
>>>           */
>>>          intel_hdmi_unset_edid(&intel_connector->base);
>>> -       if (intel_hdmi_set_edid(&intel_connector->base))
>>> +       if (intel_hdmi_set_edid(&intel_connector->base, live_status))
>>>                  DRM_DEBUG_DRIVER("DDC probe: got EDID\n");
>>>          else
>>>                  DRM_DEBUG_DRIVER("DDC probe: no EDID\n");
>>> @@ -1432,7 +1444,7 @@ intel_hdmi_force(struct drm_connector *connector)
>>>          if (connector->status != connector_status_connected)
>>>                  return;
>>>
>>> -       intel_hdmi_set_edid(connector);
>>> +       intel_hdmi_set_edid(connector, true);
>>>          hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
>>>   }
>>>
>>> --
>>> 1.7.10.4
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>>
>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Check live status before reading edid

[Daniel Vetter]

On Fri, Sep 11, 2015 at 05:56:47PM +0000, Rodrigo Vivi wrote:
> Thanks
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> 
> On Fri, Sep 11, 2015 at 4:38 AM Sonika Jindal <sonika.jindal@intel.com>
> wrote:
> 
> > The Bspec is very clear that Live status must be checked about before
> > trying to read EDID over DDC channel. This patch makes sure that HDMI
> > EDID is read only when live status is up.
> >
> > The live status doesn't seem to perform very consistent across various
> > platforms when tested with different monitors. The reason behind that is
> > some monitors are late to provide right voltage to set live_status up.
> > So, after getting the interrupt, for a small duration, live status reg
> > fluctuates, and then settles down showing the correct staus.
> >
> > This is explained here in, in a rough way:
> > HPD line  ________________
> >                          |\ T1 = Monitor Hotplug causing IRQ
> >                          | \______________________________________
> >                          | |
> >                          | |
> >                          | |   T2 = Live status is stable
> >                          | |  _____________________________________
> >                          | | /|
> > Live status _____________|_|/ |
> >                          | |  |
> >                          | |  |
> >                          | |  |
> >                         T0 T1  T2
> >
> > (Between T1 and T2 Live status fluctuates or can be even low, depending on
> >  the monitor)
> >
> > After several experiments, we have concluded that a max delay
> > of 30ms is enough to allow the live status to settle down with
> > most of the monitors. This total delay of 30ms has been split into
> > a resolution of 3 retries of 10ms each, for the better cases.
> >
> > This delay is kept at 30ms, keeping in consideration that, HDCP compliance
> > expect the HPD handler to respond a plug out in 100ms, by disabling port.
> >
> > v2: Adding checks for VLV/CHV as well. Reusing old ibx and g4x functions
> > to check digital port status. Adding a separate function to get bxt live
> > status (Daniel)
> > v3: Using intel_encoder->hpd_pin to check the live status (Siva)
> > Moving the live status read to intel_hdmi_probe and passing parameter
> > to read/not to read the edid. (me)
> > v4:
> > * Added live status check for all platforms using
> > intel_digital_port_connected.
> > * Rebased on top of Jani's DP cleanup series
> > * Some monitors take time in setting the live status. So retry for few
> > times if this is a connect HPD
> > v5: Removed extra "drm/i915" from commit message. Adding Shashank's sob
> >     which was missed.
> > v6: Drop the (!detect_edid && !live_status check) check because for DDI
> > ports which are enumerated as hdmi as well as DP, we don't have a
> > mechanism to differentiate between DP and hdmi inside the encoder's
> > hot_plug. This leads to call to the hdmi's hot_plug hook for DP as well
> > as hdmi which leads to issues during unplug because of the above check.
> > v7: Make intel_digital_port_connected global in this patch, some
> > reformatting of while loop, adding a print when live status is not
> > up. (Rodrigo)
> >
> > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>

Since this is tricky stuff and other patches in this series are blocked
until we have a clearer picture can you please rebase this to be the first
patch on top of -nightly so that I can pull it in directly?

I tried to do that but proved a bit too messy.
-Daniel

> > ---
> >  drivers/gpu/drm/i915/intel_dp.c   |    2 +-
> >  drivers/gpu/drm/i915/intel_drv.h  |    2 ++
> >  drivers/gpu/drm/i915/intel_hdmi.c |   26 +++++++++++++++++++-------
> >  3 files changed, 22 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index bf17030..fedf6d1 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4695,7 +4695,7 @@ static bool bxt_digital_port_connected(struct
> > drm_i915_private *dev_priv,
> >   *
> >   * Return %true if @port is connected, %false otherwise.
> >   */
> > -static bool intel_digital_port_connected(struct drm_i915_private
> > *dev_priv,
> > +bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
> >                                          struct intel_digital_port *port)
> >  {
> >         if (HAS_PCH_IBX(dev_priv))
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index b6c2c20..ac6d748 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1210,6 +1210,8 @@ void intel_edp_drrs_disable(struct intel_dp
> > *intel_dp);
> >  void intel_edp_drrs_invalidate(struct drm_device *dev,
> >                 unsigned frontbuffer_bits);
> >  void intel_edp_drrs_flush(struct drm_device *dev, unsigned
> > frontbuffer_bits);
> > +bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
> > +                                        struct intel_digital_port *port);
>g>
> >  /* intel_dp_mst.c */
> >  int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port,
> > int conn_id);
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 1eda71a..d366ca5 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -1329,22 +1329,23 @@ intel_hdmi_unset_edid(struct drm_connector
> > *connector)
> >  }
> >
> >  static bool
> > -intel_hdmi_set_edid(struct drm_connector *connector)
> > +intel_hdmi_set_edid(struct drm_connector *connector, bool force)
> >  {
> >         struct drm_i915_private *dev_priv = to_i915(connector->dev);
> >         struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
> >         struct intel_encoder *intel_encoder =
> >                 &hdmi_to_dig_port(intel_hdmi)->base;
> >         enum intel_display_power_domain power_domain;
> > -       struct edid *edid;
> > +       struct edid *edid = NULL;
> >         bool connected = false;
> >
> >         power_domain = intel_display_port_power_domain(intel_encoder);
> >         intel_display_power_get(dev_priv, power_domain);
> >
> > -       edid = drm_get_edid(connector,
> > -                           intel_gmbus_get_adapter(dev_priv,
> > -                                                   intel_hdmi->ddc_bus));
> > +       if (force)
> > +               edid = drm_get_edid(connector,
> > +                                   intel_gmbus_get_adapter(dev_priv,
> > +                                   intel_hdmi->ddc_bus));
> >
> >         intel_display_power_put(dev_priv, power_domain);
> >
> > @@ -1374,14 +1375,25 @@ void intel_hdmi_probe(struct intel_encoder
> > *intel_encoder)
> >                         enc_to_intel_hdmi(&intel_encoder->base);
> >         struct intel_connector *intel_connector =
> >                                 intel_hdmi->attached_connector;
> > +       struct drm_i915_private *dev_priv =
> > to_i915(intel_encoder->base.dev);
> > +       bool live_status = false;
> > +       unsigned int retry = 3;
> > +
> > +       while (!live_status && --retry) {
> > +               live_status = intel_digital_port_connected(dev_priv,
> > +                               hdmi_to_dig_port(intel_hdmi));
> > +               mdelay(10);
> > +       }
> >
> > +       if (!live_status)
> > +               DRM_DEBUG_KMS("Live status not up!");
> >         /*
> >          * We are here, means there is a hotplug or a force
> >          * detection. Clear the cached EDID and probe the
> >          * DDC bus to check the current status of HDMI.
> >          */
> >         intel_hdmi_unset_edid(&intel_connector->base);
> > -       if (intel_hdmi_set_edid(&intel_connector->base))
> > +       if (intel_hdmi_set_edid(&intel_connector->base, live_status))
> >                 DRM_DEBUG_DRIVER("DDC probe: got EDID\n");
> >         else
> >                 DRM_DEBUG_DRIVER("DDC probe: no EDID\n");
> > @@ -1432,7 +1444,7 @@ intel_hdmi_force(struct drm_connector *connector)
> >         if (connector->status != connector_status_connected)
> >                 return;
> >
> > -       intel_hdmi_set_edid(connector);
> > +       intel_hdmi_set_edid(connector, true);
> >         hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
> >  }
> >
> > --
> > 1.7.10.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >

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


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Check live status before reading edid

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 7938 bytes --]

Thanks

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>


On Fri, Sep 11, 2015 at 4:38 AM Sonika Jindal <sonika.jindal@intel.com>
wrote:

> The Bspec is very clear that Live status must be checked about before
> trying to read EDID over DDC channel. This patch makes sure that HDMI
> EDID is read only when live status is up.
>
> The live status doesn't seem to perform very consistent across various
> platforms when tested with different monitors. The reason behind that is
> some monitors are late to provide right voltage to set live_status up.
> So, after getting the interrupt, for a small duration, live status reg
> fluctuates, and then settles down showing the correct staus.
>
> This is explained here in, in a rough way:
> HPD line  ________________
>                          |\ T1 = Monitor Hotplug causing IRQ
>                          | \______________________________________
>                          | |
>                          | |
>                          | |   T2 = Live status is stable
>                          | |  _____________________________________
>                          | | /|
> Live status _____________|_|/ |
>                          | |  |
>                          | |  |
>                          | |  |
>                         T0 T1  T2
>
> (Between T1 and T2 Live status fluctuates or can be even low, depending on
>  the monitor)
>
> After several experiments, we have concluded that a max delay
> of 30ms is enough to allow the live status to settle down with
> most of the monitors. This total delay of 30ms has been split into
> a resolution of 3 retries of 10ms each, for the better cases.
>
> This delay is kept at 30ms, keeping in consideration that, HDCP compliance
> expect the HPD handler to respond a plug out in 100ms, by disabling port.
>
> v2: Adding checks for VLV/CHV as well. Reusing old ibx and g4x functions
> to check digital port status. Adding a separate function to get bxt live
> status (Daniel)
> v3: Using intel_encoder->hpd_pin to check the live status (Siva)
> Moving the live status read to intel_hdmi_probe and passing parameter
> to read/not to read the edid. (me)
> v4:
> * Added live status check for all platforms using
> intel_digital_port_connected.
> * Rebased on top of Jani's DP cleanup series
> * Some monitors take time in setting the live status. So retry for few
> times if this is a connect HPD
> v5: Removed extra "drm/i915" from commit message. Adding Shashank's sob
>     which was missed.
> v6: Drop the (!detect_edid && !live_status check) check because for DDI
> ports which are enumerated as hdmi as well as DP, we don't have a
> mechanism to differentiate between DP and hdmi inside the encoder's
> hot_plug. This leads to call to the hdmi's hot_plug hook for DP as well
> as hdmi which leads to issues during unplug because of the above check.
> v7: Make intel_digital_port_connected global in this patch, some
> reformatting of while loop, adding a print when live status is not
> up. (Rodrigo)
>
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c   |    2 +-
>  drivers/gpu/drm/i915/intel_drv.h  |    2 ++
>  drivers/gpu/drm/i915/intel_hdmi.c |   26 +++++++++++++++++++-------
>  3 files changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> index bf17030..fedf6d1 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4695,7 +4695,7 @@ static bool bxt_digital_port_connected(struct
> drm_i915_private *dev_priv,
>   *
>   * Return %true if @port is connected, %false otherwise.
>   */
> -static bool intel_digital_port_connected(struct drm_i915_private
> *dev_priv,
> +bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
>                                          struct intel_digital_port *port)
>  {
>         if (HAS_PCH_IBX(dev_priv))
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index b6c2c20..ac6d748 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1210,6 +1210,8 @@ void intel_edp_drrs_disable(struct intel_dp
> *intel_dp);
>  void intel_edp_drrs_invalidate(struct drm_device *dev,
>                 unsigned frontbuffer_bits);
>  void intel_edp_drrs_flush(struct drm_device *dev, unsigned
> frontbuffer_bits);
> +bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
> +                                        struct intel_digital_port *port);
>
>  /* intel_dp_mst.c */
>  int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port,
> int conn_id);
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> b/drivers/gpu/drm/i915/intel_hdmi.c
> index 1eda71a..d366ca5 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1329,22 +1329,23 @@ intel_hdmi_unset_edid(struct drm_connector
> *connector)
>  }
>
>  static bool
> -intel_hdmi_set_edid(struct drm_connector *connector)
> +intel_hdmi_set_edid(struct drm_connector *connector, bool force)
>  {
>         struct drm_i915_private *dev_priv = to_i915(connector->dev);
>         struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
>         struct intel_encoder *intel_encoder =
>                 &hdmi_to_dig_port(intel_hdmi)->base;
>         enum intel_display_power_domain power_domain;
> -       struct edid *edid;
> +       struct edid *edid = NULL;
>         bool connected = false;
>
>         power_domain = intel_display_port_power_domain(intel_encoder);
>         intel_display_power_get(dev_priv, power_domain);
>
> -       edid = drm_get_edid(connector,
> -                           intel_gmbus_get_adapter(dev_priv,
> -                                                   intel_hdmi->ddc_bus));
> +       if (force)
> +               edid = drm_get_edid(connector,
> +                                   intel_gmbus_get_adapter(dev_priv,
> +                                   intel_hdmi->ddc_bus));
>
>         intel_display_power_put(dev_priv, power_domain);
>
> @@ -1374,14 +1375,25 @@ void intel_hdmi_probe(struct intel_encoder
> *intel_encoder)
>                         enc_to_intel_hdmi(&intel_encoder->base);
>         struct intel_connector *intel_connector =
>                                 intel_hdmi->attached_connector;
> +       struct drm_i915_private *dev_priv =
> to_i915(intel_encoder->base.dev);
> +       bool live_status = false;
> +       unsigned int retry = 3;
> +
> +       while (!live_status && --retry) {
> +               live_status = intel_digital_port_connected(dev_priv,
> +                               hdmi_to_dig_port(intel_hdmi));
> +               mdelay(10);
> +       }
>
> +       if (!live_status)
> +               DRM_DEBUG_KMS("Live status not up!");
>         /*
>          * We are here, means there is a hotplug or a force
>          * detection. Clear the cached EDID and probe the
>          * DDC bus to check the current status of HDMI.
>          */
>         intel_hdmi_unset_edid(&intel_connector->base);
> -       if (intel_hdmi_set_edid(&intel_connector->base))
> +       if (intel_hdmi_set_edid(&intel_connector->base, live_status))
>                 DRM_DEBUG_DRIVER("DDC probe: got EDID\n");
>         else
>                 DRM_DEBUG_DRIVER("DDC probe: no EDID\n");
> @@ -1432,7 +1444,7 @@ intel_hdmi_force(struct drm_connector *connector)
>         if (connector->status != connector_status_connected)
>                 return;
>
> -       intel_hdmi_set_edid(connector);
> +       intel_hdmi_set_edid(connector, true);
>         hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
>  }
>
> --
> 1.7.10.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 9766 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH] drm/i915: Check live status before reading edid

[Sonika Jindal]

The Bspec is very clear that Live status must be checked about before
trying to read EDID over DDC channel. This patch makes sure that HDMI
EDID is read only when live status is up.

The live status doesn't seem to perform very consistent across various
platforms when tested with different monitors. The reason behind that is
some monitors are late to provide right voltage to set live_status up.
So, after getting the interrupt, for a small duration, live status reg
fluctuates, and then settles down showing the correct staus.

This is explained here in, in a rough way:
HPD line  ________________
			 |\ T1 = Monitor Hotplug causing IRQ
			 | \______________________________________
			 | |
                         | |
			 | |   T2 = Live status is stable
			 | |  _____________________________________
			 | | /|
Live status _____________|_|/ |
			 | |  |
			 | |  |
			 | |  |
			T0 T1  T2

(Between T1 and T2 Live status fluctuates or can be even low, depending on
 the monitor)

After several experiments, we have concluded that a max delay
of 30ms is enough to allow the live status to settle down with
most of the monitors. This total delay of 30ms has been split into
a resolution of 3 retries of 10ms each, for the better cases.

This delay is kept at 30ms, keeping in consideration that, HDCP compliance
expect the HPD handler to respond a plug out in 100ms, by disabling port.

v2: Adding checks for VLV/CHV as well. Reusing old ibx and g4x functions
to check digital port status. Adding a separate function to get bxt live
status (Daniel)
v3: Using intel_encoder->hpd_pin to check the live status (Siva)
Moving the live status read to intel_hdmi_probe and passing parameter
to read/not to read the edid. (me)
v4:
* Added live status check for all platforms using
intel_digital_port_connected.
* Rebased on top of Jani's DP cleanup series
* Some monitors take time in setting the live status. So retry for few
times if this is a connect HPD
v5: Removed extra "drm/i915" from commit message. Adding Shashank's sob
    which was missed.
v6: Drop the (!detect_edid && !live_status check) check because for DDI
ports which are enumerated as hdmi as well as DP, we don't have a
mechanism to differentiate between DP and hdmi inside the encoder's
hot_plug. This leads to call to the hdmi's hot_plug hook for DP as well
as hdmi which leads to issues during unplug because of the above check.
v7: Make intel_digital_port_connected global in this patch, some
reformatting of while loop, adding a print when live status is not
up. (Rodrigo)

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c   |    2 +-
 drivers/gpu/drm/i915/intel_drv.h  |    2 ++
 drivers/gpu/drm/i915/intel_hdmi.c |   26 +++++++++++++++++++-------
 3 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index bf17030..fedf6d1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4695,7 +4695,7 @@ static bool bxt_digital_port_connected(struct drm_i915_private *dev_priv,
  *
  * Return %true if @port is connected, %false otherwise.
  */
-static bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
+bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
 					 struct intel_digital_port *port)
 {
 	if (HAS_PCH_IBX(dev_priv))
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index b6c2c20..ac6d748 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1210,6 +1210,8 @@ void intel_edp_drrs_disable(struct intel_dp *intel_dp);
 void intel_edp_drrs_invalidate(struct drm_device *dev,
 		unsigned frontbuffer_bits);
 void intel_edp_drrs_flush(struct drm_device *dev, unsigned frontbuffer_bits);
+bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
+					 struct intel_digital_port *port);
 
 /* intel_dp_mst.c */
 int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 1eda71a..d366ca5 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1329,22 +1329,23 @@ intel_hdmi_unset_edid(struct drm_connector *connector)
 }
 
 static bool
-intel_hdmi_set_edid(struct drm_connector *connector)
+intel_hdmi_set_edid(struct drm_connector *connector, bool force)
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->dev);
 	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
 	struct intel_encoder *intel_encoder =
 		&hdmi_to_dig_port(intel_hdmi)->base;
 	enum intel_display_power_domain power_domain;
-	struct edid *edid;
+	struct edid *edid = NULL;
 	bool connected = false;
 
 	power_domain = intel_display_port_power_domain(intel_encoder);
 	intel_display_power_get(dev_priv, power_domain);
 
-	edid = drm_get_edid(connector,
-			    intel_gmbus_get_adapter(dev_priv,
-						    intel_hdmi->ddc_bus));
+	if (force)
+		edid = drm_get_edid(connector,
+				    intel_gmbus_get_adapter(dev_priv,
+				    intel_hdmi->ddc_bus));
 
 	intel_display_power_put(dev_priv, power_domain);
 
@@ -1374,14 +1375,25 @@ void intel_hdmi_probe(struct intel_encoder *intel_encoder)
 			enc_to_intel_hdmi(&intel_encoder->base);
 	struct intel_connector *intel_connector =
 				intel_hdmi->attached_connector;
+	struct drm_i915_private *dev_priv = to_i915(intel_encoder->base.dev);
+	bool live_status = false;
+	unsigned int retry = 3;
+
+	while (!live_status && --retry) {
+		live_status = intel_digital_port_connected(dev_priv,
+				hdmi_to_dig_port(intel_hdmi));
+		mdelay(10);
+	}
 
+	if (!live_status)
+		DRM_DEBUG_KMS("Live status not up!");
 	/*
 	 * We are here, means there is a hotplug or a force
 	 * detection. Clear the cached EDID and probe the
 	 * DDC bus to check the current status of HDMI.
 	 */
 	intel_hdmi_unset_edid(&intel_connector->base);
-	if (intel_hdmi_set_edid(&intel_connector->base))
+	if (intel_hdmi_set_edid(&intel_connector->base, live_status))
 		DRM_DEBUG_DRIVER("DDC probe: got EDID\n");
 	else
 		DRM_DEBUG_DRIVER("DDC probe: no EDID\n");
@@ -1432,7 +1444,7 @@ intel_hdmi_force(struct drm_connector *connector)
 	if (connector->status != connector_status_connected)
 		return;
 
-	intel_hdmi_set_edid(connector);
+	intel_hdmi_set_edid(connector, true);
 	hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
 }
 
-- 
1.7.10.4

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

[PATCH] drm/i915: Check live status before reading edid

[Sonika Jindal]

The Bspec is very clear that Live status must be checked about before
trying to read EDID over DDC channel. This patch makes sure that HDMI
EDID is read only when live status us up.

The live status doesn't seem to perform very consistent across various
platforms when tested with different monitors. The reason behind that is
some monitors are late to provide right voltage to set live_status up.
So, after getting the interrupt, for a small duration, live status reg
fluctuates, and then settles down showing the correct staus.

This is explained here in, in a rough way:
HPD line  ________________
			 |\ T1 = Monitor Hotplug causing IRQ
			 | \______________________________________
			 | |
                         | |
			 | |   T2 = Live status is stable
			 | |  _____________________________________
			 | | /|
Live status _____________|_|/ |
			 | |  |
			 | |  |
			 | |  |
			T0 T1  T2

(Between T1 and T2 Live status fluctuates or can be even low, depending on
 the monitor)

After several experiments, we have concluded that a max delay
of 30ms is enough to allow the live status to settle down with
most of the monitors. This total delay of 30ms has been split into
a resolution of 3 retries of 10ms each, for the better cases.

This delay is kept at 30ms, keeping in consideration that, HDCP compliance
expect the HPD handler to respond a plug out in 100ms, by disabling port.

v2: Adding checks for VLV/CHV as well. Reusing old ibx and g4x functions
to check digital port status. Adding a separate function to get bxt live
status (Daniel)
v3: Using intel_encoder->hpd_pin to check the live status (Siva)
Moving the live status read to intel_hdmi_probe and passing parameter
to read/not to read the edid. (me)
v4:
* Added live status check for all platforms using
intel_digital_port_connected.
* Rebased on top of Jani's DP cleanup series
* Some monitors take time in setting the live status. So retry for few
times if this is a connect HPD
v5: Removed extra "drm/i915" from commit message. Adding Shashank's sob
    which was missed.
v6: Drop the (!detect_edid && !live_status check) check because for DDI
ports which are enumerated as hdmi as well as DP, we don't have a
mechanism to differentiate between DP and hdmi inside the encoder's
hot_plug. This leads to call to the hdmi's hot_plug hook for DP as well
as hdmi which leads to issues during unplug because of the above check.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c |   26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 1eda71a..c6e9156 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1329,22 +1329,23 @@ intel_hdmi_unset_edid(struct drm_connector *connector)
 }
 
 static bool
-intel_hdmi_set_edid(struct drm_connector *connector)
+intel_hdmi_set_edid(struct drm_connector *connector, bool force)
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->dev);
 	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
 	struct intel_encoder *intel_encoder =
 		&hdmi_to_dig_port(intel_hdmi)->base;
 	enum intel_display_power_domain power_domain;
-	struct edid *edid;
+	struct edid *edid = NULL;
 	bool connected = false;
 
 	power_domain = intel_display_port_power_domain(intel_encoder);
 	intel_display_power_get(dev_priv, power_domain);
 
-	edid = drm_get_edid(connector,
-			    intel_gmbus_get_adapter(dev_priv,
-						    intel_hdmi->ddc_bus));
+	if (force)
+		edid = drm_get_edid(connector,
+				    intel_gmbus_get_adapter(dev_priv,
+				    intel_hdmi->ddc_bus));
 
 	intel_display_power_put(dev_priv, power_domain);
 
@@ -1374,6 +1375,17 @@ void intel_hdmi_probe(struct intel_encoder *intel_encoder)
 			enc_to_intel_hdmi(&intel_encoder->base);
 	struct intel_connector *intel_connector =
 				intel_hdmi->attached_connector;
+	struct drm_i915_private *dev_priv = to_i915(intel_encoder->base.dev);
+	bool live_status = false;
+	unsigned int retry = 3;
+
+	do {
+		live_status = intel_digital_port_connected(dev_priv,
+				hdmi_to_dig_port(intel_hdmi));
+		if (live_status)
+			break;
+		mdelay(10);
+	} while (retry--);
 
 	/*
 	 * We are here, means there is a hotplug or a force
@@ -1381,7 +1393,7 @@ void intel_hdmi_probe(struct intel_encoder *intel_encoder)
 	 * DDC bus to check the current status of HDMI.
 	 */
 	intel_hdmi_unset_edid(&intel_connector->base);
-	if (intel_hdmi_set_edid(&intel_connector->base))
+	if (intel_hdmi_set_edid(&intel_connector->base, live_status))
 		DRM_DEBUG_DRIVER("DDC probe: got EDID\n");
 	else
 		DRM_DEBUG_DRIVER("DDC probe: no EDID\n");
@@ -1432,7 +1444,7 @@ intel_hdmi_force(struct drm_connector *connector)
 	if (connector->status != connector_status_connected)
 		return;
 
-	intel_hdmi_set_edid(connector);
+	intel_hdmi_set_edid(connector, true);
 	hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
 }
 
-- 
1.7.10.4

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

[PATCH] drm/i915: Check live status before reading edid

[Sonika Jindal]

Adding this for SKL onwards.

v2: Adding checks for VLV/CHV as well. Reusing old ibx and g4x functions
to check digital port status. Adding a separate function to get bxt live
status (Daniel)
v3: Using intel_encoder->hpd_pin to check the live status (Siva)
Moving the live status read to intel_hdmi_probe and passing parameter
to read/not to read the edid. (me)
v4:
* Added live status check for all platforms using
intel_digital_port_connected.
* Rebased on top of Jani's DP cleanup series
* Some monitors take time in setting the live status. So retry for few
times if this is a connect HPD
v5: Removed extra "drm/i915" from commit message. Adding Shashank's sob
which was missed.

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c |   35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 1eda71a..d82887b 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1329,22 +1329,23 @@ intel_hdmi_unset_edid(struct drm_connector *connector)
 }
 
 static bool
-intel_hdmi_set_edid(struct drm_connector *connector)
+intel_hdmi_set_edid(struct drm_connector *connector, bool force)
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->dev);
 	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
 	struct intel_encoder *intel_encoder =
 		&hdmi_to_dig_port(intel_hdmi)->base;
 	enum intel_display_power_domain power_domain;
-	struct edid *edid;
+	struct edid *edid = NULL;
 	bool connected = false;
 
 	power_domain = intel_display_port_power_domain(intel_encoder);
 	intel_display_power_get(dev_priv, power_domain);
 
-	edid = drm_get_edid(connector,
-			    intel_gmbus_get_adapter(dev_priv,
-						    intel_hdmi->ddc_bus));
+	if (force)
+		edid = drm_get_edid(connector,
+				    intel_gmbus_get_adapter(dev_priv,
+				    intel_hdmi->ddc_bus));
 
 	intel_display_power_put(dev_priv, power_domain);
 
@@ -1374,6 +1375,26 @@ void intel_hdmi_probe(struct intel_encoder *intel_encoder)
 			enc_to_intel_hdmi(&intel_encoder->base);
 	struct intel_connector *intel_connector =
 				intel_hdmi->attached_connector;
+	struct drm_i915_private *dev_priv = to_i915(intel_encoder->base.dev);
+	bool live_status = false;
+	unsigned int retry = 3;
+
+	live_status = intel_digital_port_connected(dev_priv,
+						   hdmi_to_dig_port(intel_hdmi));
+	if (!intel_connector->detect_edid && live_status == false) {
+		/*
+		 * Hotplug had occurred and old status was disconnected,
+		 * so it might be possible that live status is not set,
+		 * so retry for few times
+		 */
+		do {
+			mdelay(10);
+			live_status = intel_digital_port_connected(dev_priv,
+						hdmi_to_dig_port(intel_hdmi));
+			if (live_status)
+				break;
+		} while (retry--);
+	}
 
 	/*
 	 * We are here, means there is a hotplug or a force
@@ -1381,7 +1402,7 @@ void intel_hdmi_probe(struct intel_encoder *intel_encoder)
 	 * DDC bus to check the current status of HDMI.
 	 */
 	intel_hdmi_unset_edid(&intel_connector->base);
-	if (intel_hdmi_set_edid(&intel_connector->base))
+	if (intel_hdmi_set_edid(&intel_connector->base, live_status))
 		DRM_DEBUG_DRIVER("DDC probe: got EDID\n");
 	else
 		DRM_DEBUG_DRIVER("DDC probe: no EDID\n");
@@ -1432,7 +1453,7 @@ intel_hdmi_force(struct drm_connector *connector)
 	if (connector->status != connector_status_connected)
 		return;
 
-	intel_hdmi_set_edid(connector);
+	intel_hdmi_set_edid(connector, true);
 	hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
 }
 
-- 
1.7.10.4

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