[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

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

[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

[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

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