[PATCH 0/4] HDMI optimization series

[PATCH 0/4] 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.

v2:
   Some refactoring is with this series.

   Also, right now this is done for platforms gen7 and above because we
   couldn't test with older platforms. For newer platforms it works
   reliably.

   For HPD and live status to work on SKL, following patch is required:
   "drm/i915: Handle HPD when it has actually occurred"

v3:
   Added retrial for live_status.
   Relying on HPD fpr edid detection onlyfrom gen 8 onwards and VLV

Shashank Sharma (2):
  drm/i915: add attached connector to hdmi container
  drm/i915: Add HDMI probe function

Sonika Jindal (2):
  drm/i915: Check live status before reading edid
  drm/i915: Retry for live status

 drivers/gpu/drm/i915/intel_dp.c   |    4 +-
 drivers/gpu/drm/i915/intel_drv.h  |    3 +
 drivers/gpu/drm/i915/intel_hdmi.c |  130 +++++++++++++++++++++++++++++++++----
 3 files changed, 123 insertions(+), 14 deletions(-)

-- 
1.7.10.4

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

[PATCH 1/4] 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 81b7d77..232b814 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -666,6 +666,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 7185062..3825507 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -2035,6 +2035,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/4] 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 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. During init_connector his probe will be called to read the edid
3. Reuse the cached EDID in hdmi_detect() function.

The "< gen7" check is there because this was tested only for >=gen7
platforms. For older platforms the hotplug/reading edid path remains same.

v2: Calling set_edid instead of hdmi_probe during init.
Also, for platforms having DDI, intel_encoder for DP and HDMI is same
(taken from intel_dig_port), so for DP also, hot_plug function gets called
which is not intended here. So, check for HDMI in intel_hdmi_probe
Rely on HPD for updating edid only for platforms gen > 8 and also for VLV.

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 |   56 +++++++++++++++++++++++++++++++++----
 1 file changed, 50 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 3825507..68886c0 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1368,23 +1368,63 @@ 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;
+	/*
+	 * Sometimes DDI ports are enumerated as DP as well as HDMI and
+	 * detection is left for runtime. Since DP's detection method already
+	 * takes care of live status checks, do this only for HDMI.
+	 */
+	if (intel_connector->base.connector_type != DRM_MODE_CONNECTOR_HDMIA)
+		return;
+	/*
+	 * 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)
 {
 	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);
+	/*
+	 * There are many userspace calls which probe EDID from
+	 * detect path. In case of 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, reading edid in detect only for older platforms (< gen8)
+	 * Letting the newer platforms rely on hotplugs and init to read edid.
+	 * Check connector status based on availability of cached EDID.
+	 */
+	if (INTEL_INFO(dev)->gen < 8 && !IS_VALLEYVIEW(dev))
+		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;
 }
@@ -2032,11 +2072,15 @@ 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);
 	intel_hdmi->attached_connector = intel_connector;
 
+	/* Set edid during init */
+	intel_hdmi_set_edid(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
 	 * 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 3/4] 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)

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 |   62 ++++++++++++++++++++++++++++++++-----
 3 files changed, 58 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b905c19..8f2eba2 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4477,8 +4477,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 232b814..b07e141 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1192,6 +1192,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 68886c0..59518b4 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1328,23 +1328,63 @@ 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 *intel_dig_port)
+{
+	struct intel_encoder *intel_encoder = &intel_dig_port->base;
+	enum port port;
+	u32 temp = I915_READ(GEN8_DE_PORT_ISR);
+
+	intel_hpd_pin_to_port(intel_encoder->hpd_pin, &port);
+	switch (port) {
+	case PORT_A:
+		return temp & BXT_DE_PORT_HP_DDIA;
+
+	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) || IS_BROADWELL(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)
+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 +1414,8 @@ 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;
+	bool live_status = false;
+
 	/*
 	 * Sometimes DDI ports are enumerated as DP as well as HDMI and
 	 * detection is left for runtime. Since DP's detection method already
@@ -1381,13 +1423,15 @@ void intel_hdmi_probe(struct intel_encoder *intel_encoder)
 	 */
 	if (intel_connector->base.connector_type != DRM_MODE_CONNECTOR_HDMIA)
 		return;
+
+	live_status = intel_hdmi_live_status(hdmi_to_dig_port(intel_hdmi));
 	/*
 	 * 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");
@@ -1442,7 +1486,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;
 }
 
@@ -1996,6 +2040,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 	struct drm_device *dev = intel_encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum port port = intel_dig_port->port;
+	bool live_status = false;
 
 	drm_connector_init(dev, connector, &intel_hdmi_connector_funcs,
 			   DRM_MODE_CONNECTOR_HDMIA);
@@ -2079,7 +2124,8 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
 	intel_hdmi->attached_connector = intel_connector;
 
 	/* Set edid during init */
-	intel_hdmi_set_edid(connector);
+	live_status = intel_hdmi_live_status(intel_dig_port);
+	intel_hdmi_set_edid(connector, live_status);
 
 	/* 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 4/4] drm/i915: Retry for live status

[Sonika Jindal]

Some monitors take time in setting the live status.
So retry for few times if this is a connect HPD

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

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 59518b4..239d70d 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1415,6 +1415,7 @@ void intel_hdmi_probe(struct intel_encoder *intel_encoder)
 	struct intel_connector *intel_connector =
 				intel_hdmi->attached_connector;
 	bool live_status = false;
+	unsigned int retry = 3;
 
 	/*
 	 * Sometimes DDI ports are enumerated as DP as well as HDMI and
@@ -1425,6 +1426,20 @@ void intel_hdmi_probe(struct intel_encoder *intel_encoder)
 		return;
 
 	live_status = intel_hdmi_live_status(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_hdmi_live_status(hdmi_to_dig_port(intel_hdmi));
+			if (live_status)
+				break;
+		} while (retry--);
+	}
+
 	/*
 	 * We are here, means there is a hotplug or a force
 	 * detection. Clear the cached EDID and probe the
-- 
1.7.10.4

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

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

[Jani Nikula]

On Tue, 25 Aug 2015, Sonika Jindal <sonika.jindal@intel.com> 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)
> 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)

We may want to merge this cleanup first:
http://mid.gmane.org/cover.1440056461.git.jani.nikula@intel.com

BR,
Jani.



>
> 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 |   62 ++++++++++++++++++++++++++++++++-----
>  3 files changed, 58 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b905c19..8f2eba2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4477,8 +4477,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 232b814..b07e141 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1192,6 +1192,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 68886c0..59518b4 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1328,23 +1328,63 @@ 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 *intel_dig_port)
> +{
> +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> +	enum port port;
> +	u32 temp = I915_READ(GEN8_DE_PORT_ISR);
> +
> +	intel_hpd_pin_to_port(intel_encoder->hpd_pin, &port);
> +	switch (port) {
> +	case PORT_A:
> +		return temp & BXT_DE_PORT_HP_DDIA;
> +
> +	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) || IS_BROADWELL(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)
> +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 +1414,8 @@ 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;
> +	bool live_status = false;
> +
>  	/*
>  	 * Sometimes DDI ports are enumerated as DP as well as HDMI and
>  	 * detection is left for runtime. Since DP's detection method already
> @@ -1381,13 +1423,15 @@ void intel_hdmi_probe(struct intel_encoder *intel_encoder)
>  	 */
>  	if (intel_connector->base.connector_type != DRM_MODE_CONNECTOR_HDMIA)
>  		return;
> +
> +	live_status = intel_hdmi_live_status(hdmi_to_dig_port(intel_hdmi));
>  	/*
>  	 * 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");
> @@ -1442,7 +1486,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;
>  }
>  
> @@ -1996,6 +2040,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>  	struct drm_device *dev = intel_encoder->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	enum port port = intel_dig_port->port;
> +	bool live_status = false;
>  
>  	drm_connector_init(dev, connector, &intel_hdmi_connector_funcs,
>  			   DRM_MODE_CONNECTOR_HDMIA);
> @@ -2079,7 +2124,8 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>  	intel_hdmi->attached_connector = intel_connector;
>  
>  	/* Set edid during init */
> -	intel_hdmi_set_edid(connector);
> +	live_status = intel_hdmi_live_status(intel_dig_port);
> +	intel_hdmi_set_edid(connector, live_status);
>  
>  	/* 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

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

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

[Hindman, Gavin]

Jani - do you believe that this series will need to be reworked following merge of your cleanup series?   We've got some outstanding CHV hot-plug issues that Sonika's series should fix, so we need to get it in, but should definitely take the path towards longer-term quality.

Gavin Hindman
Senior Program Manager
SSG/OTC – Open Source Technology Center


-----Original Message-----
From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Jani Nikula
Sent: Tuesday, August 25, 2015 9:52 AM
To: Jindal, Sonika; intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 3/4] drm/i915: Check live status before reading edid

On Tue, 25 Aug 2015, Sonika Jindal <sonika.jindal@intel.com> 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)
> 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)

We may want to merge this cleanup first:
http://mid.gmane.org/cover.1440056461.git.jani.nikula@intel.com

BR,
Jani.



>
> 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 |   62 ++++++++++++++++++++++++++++++++-----
>  3 files changed, 58 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> b/drivers/gpu/drm/i915/intel_dp.c index b905c19..8f2eba2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4477,8 +4477,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 232b814..b07e141 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1192,6 +1192,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 68886c0..59518b4 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1328,23 +1328,63 @@ 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 *intel_dig_port) {
> +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> +	enum port port;
> +	u32 temp = I915_READ(GEN8_DE_PORT_ISR);
> +
> +	intel_hpd_pin_to_port(intel_encoder->hpd_pin, &port);
> +	switch (port) {
> +	case PORT_A:
> +		return temp & BXT_DE_PORT_HP_DDIA;
> +
> +	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) || IS_BROADWELL(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)
> +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 +1414,8 @@ 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;
> +	bool live_status = false;
> +
>  	/*
>  	 * Sometimes DDI ports are enumerated as DP as well as HDMI and
>  	 * detection is left for runtime. Since DP's detection method 
> already @@ -1381,13 +1423,15 @@ void intel_hdmi_probe(struct intel_encoder *intel_encoder)
>  	 */
>  	if (intel_connector->base.connector_type != DRM_MODE_CONNECTOR_HDMIA)
>  		return;
> +
> +	live_status = intel_hdmi_live_status(hdmi_to_dig_port(intel_hdmi));
>  	/*
>  	 * 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"); @@ -1442,7 +1486,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;  }
>  
> @@ -1996,6 +2040,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>  	struct drm_device *dev = intel_encoder->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	enum port port = intel_dig_port->port;
> +	bool live_status = false;
>  
>  	drm_connector_init(dev, connector, &intel_hdmi_connector_funcs,
>  			   DRM_MODE_CONNECTOR_HDMIA);
> @@ -2079,7 +2124,8 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>  	intel_hdmi->attached_connector = intel_connector;
>  
>  	/* Set edid during init */
> -	intel_hdmi_set_edid(connector);
> +	live_status = intel_hdmi_live_status(intel_dig_port);
> +	intel_hdmi_set_edid(connector, live_status);
>  
>  	/* 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

--
Jani Nikula, Intel Open Source Technology Center _______________________________________________
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 3/4] drm/i915: Check live status before reading edid

[Jani Nikula]

The cleanup series has already been reviewed, this one hasn't. The
cleanup series should actually make patch 3/4 simpler to
review. Otherwise there really isn't conflict.

BR,
Jani.


On Wed, 26 Aug 2015, "Hindman, Gavin" <gavin.hindman@intel.com> wrote:
> Jani - do you believe that this series will need to be reworked following merge of your cleanup series?   We've got some outstanding CHV hot-plug issues that Sonika's series should fix, so we need to get it in, but should definitely take the path towards longer-term quality.
>
> Gavin Hindman
> Senior Program Manager
> SSG/OTC – Open Source Technology Center
>
>
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Jani Nikula
> Sent: Tuesday, August 25, 2015 9:52 AM
> To: Jindal, Sonika; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 3/4] drm/i915: Check live status before reading edid
>
> On Tue, 25 Aug 2015, Sonika Jindal <sonika.jindal@intel.com> 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)
>> 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)
>
> We may want to merge this cleanup first:
> http://mid.gmane.org/cover.1440056461.git.jani.nikula@intel.com
>
> BR,
> Jani.
>
>
>
>>
>> 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 |   62 ++++++++++++++++++++++++++++++++-----
>>  3 files changed, 58 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c 
>> b/drivers/gpu/drm/i915/intel_dp.c index b905c19..8f2eba2 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4477,8 +4477,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 232b814..b07e141 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1192,6 +1192,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 68886c0..59518b4 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -1328,23 +1328,63 @@ 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 *intel_dig_port) {
>> +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
>> +	enum port port;
>> +	u32 temp = I915_READ(GEN8_DE_PORT_ISR);
>> +
>> +	intel_hpd_pin_to_port(intel_encoder->hpd_pin, &port);
>> +	switch (port) {
>> +	case PORT_A:
>> +		return temp & BXT_DE_PORT_HP_DDIA;
>> +
>> +	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) || IS_BROADWELL(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)
>> +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 +1414,8 @@ 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;
>> +	bool live_status = false;
>> +
>>  	/*
>>  	 * Sometimes DDI ports are enumerated as DP as well as HDMI and
>>  	 * detection is left for runtime. Since DP's detection method 
>> already @@ -1381,13 +1423,15 @@ void intel_hdmi_probe(struct intel_encoder *intel_encoder)
>>  	 */
>>  	if (intel_connector->base.connector_type != DRM_MODE_CONNECTOR_HDMIA)
>>  		return;
>> +
>> +	live_status = intel_hdmi_live_status(hdmi_to_dig_port(intel_hdmi));
>>  	/*
>>  	 * 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"); @@ -1442,7 +1486,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;  }
>>  
>> @@ -1996,6 +2040,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>>  	struct drm_device *dev = intel_encoder->base.dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	enum port port = intel_dig_port->port;
>> +	bool live_status = false;
>>  
>>  	drm_connector_init(dev, connector, &intel_hdmi_connector_funcs,
>>  			   DRM_MODE_CONNECTOR_HDMIA);
>> @@ -2079,7 +2124,8 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>>  	intel_hdmi->attached_connector = intel_connector;
>>  
>>  	/* Set edid during init */
>> -	intel_hdmi_set_edid(connector);
>> +	live_status = intel_hdmi_live_status(intel_dig_port);
>> +	intel_hdmi_set_edid(connector, live_status);
>>  
>>  	/* 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
>
> --
> Jani Nikula, Intel Open Source Technology Center _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 4/4] drm/i915: Retry for live status

[Daniel Vetter]

On Tue, Aug 25, 2015 at 05:31:33PM +0530, Sonika Jindal wrote:
> Some monitors take time in setting the live status.
> So retry for few times if this is a connect HPD
> 
> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>

Why was this bugfix not part of the original series? Now I have to retest
on my ivb to figure out whether maybe this one here is the issue ...

Also how exactly does this work? I thought the hpd bits control whether we
get an interrupt, not the other way round? Why exactly does this help?
Definitely needs a lot more explanation.

Also this seems to break bisect, since before the preceeding patch to
check hpd status we just retried edid reading for a while.

This kind of hacking doesn't really convince me that hpd status is
working, just that our own testing isn't good enough to catch all
real-world issues.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_hdmi.c |   15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 59518b4..239d70d 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1415,6 +1415,7 @@ void intel_hdmi_probe(struct intel_encoder *intel_encoder)
>  	struct intel_connector *intel_connector =
>  				intel_hdmi->attached_connector;
>  	bool live_status = false;
> +	unsigned int retry = 3;
>  
>  	/*
>  	 * Sometimes DDI ports are enumerated as DP as well as HDMI and
> @@ -1425,6 +1426,20 @@ void intel_hdmi_probe(struct intel_encoder *intel_encoder)
>  		return;
>  
>  	live_status = intel_hdmi_live_status(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_hdmi_live_status(hdmi_to_dig_port(intel_hdmi));
> +			if (live_status)
> +				break;
> +		} while (retry--);
> +	}
> +
>  	/*
>  	 * We are here, means there is a hotplug or a force
>  	 * detection. Clear the cached EDID and probe the
> -- 
> 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/4] drm/i915: Retry for live status

[Jindal, Sonika]

HPD bits control the interrupt but the live status (with some monitors) takes time to get set.
We had experienced this with VLV and CHV with few monitors.
So Android code always has this retry for live status.

Yes, this was not added in the previous series because we planned to add the next set of optimization a little while later.
But this seems to be an important one.

It will be great if you can try it with your ivb. But for that you would need to first change the gen check and add a call to check live status for ivb.

Regards,
Sonika
-----Original Message-----
From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
Sent: Wednesday, August 26, 2015 3:10 PM
To: Jindal, Sonika
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 4/4] drm/i915: Retry for live status

On Tue, Aug 25, 2015 at 05:31:33PM +0530, Sonika Jindal wrote:
> Some monitors take time in setting the live status.
> So retry for few times if this is a connect HPD
> 
> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>

Why was this bugfix not part of the original series? Now I have to retest on my ivb to figure out whether maybe this one here is the issue ...

Also how exactly does this work? I thought the hpd bits control whether we get an interrupt, not the other way round? Why exactly does this help?
Definitely needs a lot more explanation.

Also this seems to break bisect, since before the preceeding patch to check hpd status we just retried edid reading for a while.

This kind of hacking doesn't really convince me that hpd status is working, just that our own testing isn't good enough to catch all real-world issues.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_hdmi.c |   15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c 
> b/drivers/gpu/drm/i915/intel_hdmi.c
> index 59518b4..239d70d 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1415,6 +1415,7 @@ void intel_hdmi_probe(struct intel_encoder *intel_encoder)
>  	struct intel_connector *intel_connector =
>  				intel_hdmi->attached_connector;
>  	bool live_status = false;
> +	unsigned int retry = 3;
>  
>  	/*
>  	 * Sometimes DDI ports are enumerated as DP as well as HDMI and @@ 
> -1425,6 +1426,20 @@ void intel_hdmi_probe(struct intel_encoder *intel_encoder)
>  		return;
>  
>  	live_status = intel_hdmi_live_status(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_hdmi_live_status(hdmi_to_dig_port(intel_hdmi));
> +			if (live_status)
> +				break;
> +		} while (retry--);
> +	}
> +
>  	/*
>  	 * We are here, means there is a hotplug or a force
>  	 * detection. Clear the cached EDID and probe the
> --
> 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/4] drm/i915: Retry for live status

[Daniel Vetter]

On Wed, Aug 26, 2015 at 10:05:00AM +0000, Jindal, Sonika wrote:
> HPD bits control the interrupt but the live status (with some monitors) takes time to get set.
> We had experienced this with VLV and CHV with few monitors.
> So Android code always has this retry for live status.
> 
> Yes, this was not added in the previous series because we planned to add the next set of optimization a little while later.
> But this seems to be an important one.
> 
> It will be great if you can try it with your ivb. But for that you would need to first change the gen check and add a call to check live status for ivb.

Done (well I just quickly hacked up the same idea on top of your old
patches). Lessons to be learned from this:
- Make sure that you really include _all_ the bugfixes. This pach here
  isn't just tuning, it's crucial to make it work. And this isn't the
  first time vpg teams upstream something and later on we notice that
  important bugfixes have been forgotten.

  Because this wasn't done both you & me wasted a lot of time arguing
  about these patches and trying to test them.

- Please squash this patch in with patch 3 since otherwise we have a
  regression. Also please try to dig out why exactly this works like this
  since the hpd irq happening _before_ hpd status settles sounds to me
  like we have a little time machine in our silicon which can predict the
  future ...

- Please respin the patch series with the IS_VLV || gen >= 8 checks drop,
  I'm fairly confident that this bugfix here is the bit we've been looking
  for since years. At least it would be good to retest on all platforms
  for maximal test coverage.

Cheers, Daniel
> 
> Regards,
> Sonika
> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Wednesday, August 26, 2015 3:10 PM
> To: Jindal, Sonika
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 4/4] drm/i915: Retry for live status
> 
> On Tue, Aug 25, 2015 at 05:31:33PM +0530, Sonika Jindal wrote:
> > Some monitors take time in setting the live status.
> > So retry for few times if this is a connect HPD
> > 
> > Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
> 
> Why was this bugfix not part of the original series? Now I have to retest on my ivb to figure out whether maybe this one here is the issue ...
> 
> Also how exactly does this work? I thought the hpd bits control whether we get an interrupt, not the other way round? Why exactly does this help?
> Definitely needs a lot more explanation.
> 
> Also this seems to break bisect, since before the preceeding patch to check hpd status we just retried edid reading for a while.
> 
> This kind of hacking doesn't really convince me that hpd status is working, just that our own testing isn't good enough to catch all real-world issues.
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/intel_hdmi.c |   15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c 
> > b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 59518b4..239d70d 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -1415,6 +1415,7 @@ void intel_hdmi_probe(struct intel_encoder *intel_encoder)
> >  	struct intel_connector *intel_connector =
> >  				intel_hdmi->attached_connector;
> >  	bool live_status = false;
> > +	unsigned int retry = 3;
> >  
> >  	/*
> >  	 * Sometimes DDI ports are enumerated as DP as well as HDMI and @@ 
> > -1425,6 +1426,20 @@ void intel_hdmi_probe(struct intel_encoder *intel_encoder)
> >  		return;
> >  
> >  	live_status = intel_hdmi_live_status(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_hdmi_live_status(hdmi_to_dig_port(intel_hdmi));
> > +			if (live_status)
> > +				break;
> > +		} while (retry--);
> > +	}
> > +
> >  	/*
> >  	 * We are here, means there is a hotplug or a force
> >  	 * detection. Clear the cached EDID and probe the
> > --
> > 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

-- 
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/4] drm/i915: Retry for live status

[Sharma, Shashank]

Regards
Shashank

On 8/26/2015 8:47 PM, Daniel Vetter wrote:
> On Wed, Aug 26, 2015 at 10:05:00AM +0000, Jindal, Sonika wrote:
>> HPD bits control the interrupt but the live status (with some monitors) takes time to get set.
>> We had experienced this with VLV and CHV with few monitors.
>> So Android code always has this retry for live status.
>>
>> Yes, this was not added in the previous series because we planned to add the next set of optimization a little while later.
>> But this seems to be an important one.
>>
>> It will be great if you can try it with your ivb. But for that you would need to first change the gen check and add a call to check live status for ivb.
>
> Done (well I just quickly hacked up the same idea on top of your old
> patches). Lessons to be learned from this:
> - Make sure that you really include _all_ the bugfixes. This pach here
>    isn't just tuning, it's crucial to make it work. And this isn't the
>    first time vpg teams upstream something and later on we notice that
>    important bugfixes have been forgotten.
>
>    Because this wasn't done both you & me wasted a lot of time arguing
>    about these patches and trying to test them.
>
Agree. We thought once the basic optimization goes in, we will add this 
as fine tuning patch. We were afraid of you guys doubting this approach 
at first itself. It looks like a little hack, but the HW itself is 
screwed up like this, to deal with.
> - Please squash this patch in with patch 3 since otherwise we have a
>    regression. Also please try to dig out why exactly this works like this
>    since the hpd irq happening _before_ hpd status settles sounds to me
>    like we have a little time machine in our silicon which can predict the
>    future ...
>
Actually this depends on the monitor also. Few monitors are slow to 
assert the HPD line, or sometimes they don't provide the right voltage 
on that, causing live status to fluctuate for a while. While VLV/CHV 
beta testing we have done this experiment with a big range of monitors, 
and concluded that 30ms(retry of 10ms * 3) is the optimized time where 
most of the monitors respond well.

We saw that we cant delay further, because HDCP compliance expects us to 
respond to HPD (out) with in 100ms. So after careful testing with many 
monitors, we have concluded this range.
> - Please respin the patch series with the IS_VLV || gen >= 8 checks drop,
>    I'm fairly confident that this bugfix here is the bit we've been looking
>    for since years. At least it would be good to retest on all platforms
>    for maximal test coverage.
>
Sure. Will do that.
> Cheers, Daniel
>>
>> Regards,
>> Sonika
>> -----Original Message-----
>> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
>> Sent: Wednesday, August 26, 2015 3:10 PM
>> To: Jindal, Sonika
>> Cc: intel-gfx@lists.freedesktop.org
>> Subject: Re: [Intel-gfx] [PATCH 4/4] drm/i915: Retry for live status
>>
>> On Tue, Aug 25, 2015 at 05:31:33PM +0530, Sonika Jindal wrote:
>>> Some monitors take time in setting the live status.
>>> So retry for few times if this is a connect HPD
>>>
>>> Signed-off-by: Sonika Jindal <sonika.jindal@intel.com>
>>
>> Why was this bugfix not part of the original series? Now I have to retest on my ivb to figure out whether maybe this one here is the issue ...
>>
>> Also how exactly does this work? I thought the hpd bits control whether we get an interrupt, not the other way round? Why exactly does this help?
>> Definitely needs a lot more explanation.
>>
>> Also this seems to break bisect, since before the preceeding patch to check hpd status we just retried edid reading for a while.
>>
>> This kind of hacking doesn't really convince me that hpd status is working, just that our own testing isn't good enough to catch all real-world issues.
>> -Daniel
>>
>>> ---
>>>   drivers/gpu/drm/i915/intel_hdmi.c |   15 +++++++++++++++
>>>   1 file changed, 15 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
>>> b/drivers/gpu/drm/i915/intel_hdmi.c
>>> index 59518b4..239d70d 100644
>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>>> @@ -1415,6 +1415,7 @@ void intel_hdmi_probe(struct intel_encoder *intel_encoder)
>>>   	struct intel_connector *intel_connector =
>>>   				intel_hdmi->attached_connector;
>>>   	bool live_status = false;
>>> +	unsigned int retry = 3;
>>>
>>>   	/*
>>>   	 * Sometimes DDI ports are enumerated as DP as well as HDMI and @@
>>> -1425,6 +1426,20 @@ void intel_hdmi_probe(struct intel_encoder *intel_encoder)
>>>   		return;
>>>
>>>   	live_status = intel_hdmi_live_status(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_hdmi_live_status(hdmi_to_dig_port(intel_hdmi));
>>> +			if (live_status)
>>> +				break;
>>> +		} while (retry--);
>>> +	}
>>> +
>>>   	/*
>>>   	 * We are here, means there is a hotplug or a force
>>>   	 * detection. Clear the cached EDID and probe the
>>> --
>>> 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/4] drm/i915: Retry for live status

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 7251
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
ILK                 -2              302/302              300/302
SNB                                  315/315              315/315
IVB                                  336/336              336/336
BYT                 -7              283/283              276/283
HSW                                  378/378              378/378
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*ILK  igt@kms_flip@flip-vs-dpms-interruptible      PASS(1)      DMESG_WARN(1)
*ILK  igt@kms_flip@wf_vblank-vs-modeset-interruptible      PASS(1)      DMESG_WARN(1)
*BYT  igt@drm_read@empty-block      PASS(1)      NSPT(1)
*BYT  igt@drm_read@empty-nonblock      PASS(1)      NSPT(1)
*BYT  igt@drm_read@invalid-buffer      PASS(1)      NSPT(1)
*BYT  igt@drm_read@short-buffer-block      PASS(1)      NSPT(1)
*BYT  igt@drm_read@short-buffer-nonblock      PASS(1)      NSPT(1)
*BYT  igt@gem_partial_pwrite_pread@reads-uncached      PASS(1)      FAIL(1)
*BYT  igt@gem_tiled_partial_pwrite_pread@reads      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/4] drm/i915: Retry for live status

[Daniel Vetter]

On Thu, Aug 27, 2015 at 02:46:26PM +0530, Sharma, Shashank wrote:
> Regards
> Shashank
> 
> On 8/26/2015 8:47 PM, Daniel Vetter wrote:
> >On Wed, Aug 26, 2015 at 10:05:00AM +0000, Jindal, Sonika wrote:
> >>HPD bits control the interrupt but the live status (with some monitors) takes time to get set.
> >>We had experienced this with VLV and CHV with few monitors.
> >>So Android code always has this retry for live status.
> >>
> >>Yes, this was not added in the previous series because we planned to add the next set of optimization a little while later.
> >>But this seems to be an important one.
> >>
> >>It will be great if you can try it with your ivb. But for that you would need to first change the gen check and add a call to check live status for ivb.
> >
> >Done (well I just quickly hacked up the same idea on top of your old
> >patches). Lessons to be learned from this:
> >- Make sure that you really include _all_ the bugfixes. This pach here
> >   isn't just tuning, it's crucial to make it work. And this isn't the
> >   first time vpg teams upstream something and later on we notice that
> >   important bugfixes have been forgotten.
> >
> >   Because this wasn't done both you & me wasted a lot of time arguing
> >   about these patches and trying to test them.
> >
> Agree. We thought once the basic optimization goes in, we will add this as
> fine tuning patch. We were afraid of you guys doubting this approach at
> first itself. It looks like a little hack, but the HW itself is screwed up
> like this, to deal with.

Reality always wins, even if it looks really broken at first. The
important bit in those cases is to justify the change with a really good
commit message to make it clear what exactly is going on and why this
change is the correct fix/work-around.

> >- Please squash this patch in with patch 3 since otherwise we have a
> >   regression. Also please try to dig out why exactly this works like this
> >   since the hpd irq happening _before_ hpd status settles sounds to me
> >   like we have a little time machine in our silicon which can predict the
> >   future ...
> >
> Actually this depends on the monitor also. Few monitors are slow to assert
> the HPD line, or sometimes they don't provide the right voltage on that,
> causing live status to fluctuate for a while. While VLV/CHV beta testing we
> have done this experiment with a big range of monitors, and concluded that
> 30ms(retry of 10ms * 3) is the optimized time where most of the monitors
> respond well.
> 
> We saw that we cant delay further, because HDCP compliance expects us to
> respond to HPD (out) with in 100ms. So after careful testing with many
> monitors, we have concluded this range.

So what's happening on the wire is
                               
                              /------------
                ^      /\    /
               / \/\  /  \/\/              <- hpd threshold
----------/\--/     \/

                ^ first early hpd causing irq
                               ^ hpd status stable

If that's what's going on then yeah your patch makes a lot of sense, since
it perfectly explains why we've seen this on all platforms. Have you
confirmed this with oscilloscopes and all that? Please add all that
information (including pretty ascii graphs pls) to the commit message to
make sure we really have this documented correctly.
			  
> >- Please respin the patch series with the IS_VLV || gen >= 8 checks drop,
> >   I'm fairly confident that this bugfix here is the bit we've been looking
> >   for since years. At least it would be good to retest on all platforms
> >   for maximal test coverage.
> >
> Sure. Will do that.

Awesome, really looking forward to finally settling this after years!

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