[PATCH 0/6] Fixing sink count related detection over

[PATCH 0/6] Fixing sink count related detection over

[Shubhangi Shrivastava]

This patch set cleans up DP detection logic to bring all DPCD
operations at one place and to create a clear demarcation
between handling of long and short pulses. This simplifies
fixing of sink count related detection for DP panels.

Patches:
1. First two patches clean up intel_dp_detect and form a new
function which will include all DPCD related operations.
2. Second patch splits up intel_dp_check_link_status to form
a new function which will handle short pulse requests.
3. Last three patches fixes the detection logic related to
sink count i.e detect changes in sink count and handle them
appropriately. 

Note: this is tested on BXT with non-mst panels,
will get back ASAP with results for MST panels too. 

Shubhangi Shrivastava (6):
  drm/i915: Splitting intel_dp_detect
  drm/i915: Cleaning up intel_dp_hpd_pulse
  drm/i915: Splitting intel_dp_check_link_status
  drm/i915: Save sink_count for tracking changes to it
  drm/i915: read sink_count dpcd always
  drm/i915: force full detect on sink count change

 drivers/gpu/drm/i915/intel_dp.c  | 170 +++++++++++++++++++++++++--------------
 drivers/gpu/drm/i915/intel_drv.h |   1 +
 2 files changed, 110 insertions(+), 61 deletions(-)

-- 
2.6.1

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

[PATCH 1/6] drm/i915: Splitting intel_dp_detect

[Shubhangi Shrivastava]

This patch moves probing for panel, DPCD read etc to another
function intel_dp_long_pulse, while intel_dp_detect returns
the status as connected or disconnected depending on
whether the edid is available or not.
This change will be required by further patches in the series
to avoid performing multiple DPCD operations and removing
their duplication.

Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 66 ++++++++++++++++++++++++++++++-----------
 1 file changed, 49 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1cb1f3f..a0fe827 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4785,9 +4785,10 @@ intel_dp_power_put(struct intel_dp *dp,
 	intel_display_power_put(to_i915(encoder->base.dev), power_domain);
 }
 
-static enum drm_connector_status
-intel_dp_detect(struct drm_connector *connector, bool force)
+static void
+intel_dp_long_pulse(struct intel_connector *intel_connector)
 {
+	struct drm_connector *connector = &intel_connector->base;
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
@@ -4797,17 +4798,6 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	bool ret;
 	u8 sink_irq_vector;
 
-	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
-		      connector->base.id, connector->name);
-	intel_dp_unset_edid(intel_dp);
-
-	if (intel_dp->is_mst) {
-		/* MST devices are disconnected from a monitor POV */
-		if (intel_encoder->type != INTEL_OUTPUT_EDP)
-			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
-		return connector_status_disconnected;
-	}
-
 	power_domain = intel_dp_power_get(intel_dp);
 
 	/* Can't disconnect eDP, but you can close the lid... */
@@ -4817,19 +4807,35 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 		status = ironlake_dp_detect(intel_dp);
 	else
 		status = g4x_dp_detect(intel_dp);
-	if (status != connector_status_connected)
+	if (status != connector_status_connected) {
+		intel_dp_unset_edid(intel_dp);
 		goto out;
+	}
 
 	intel_dp_probe_oui(intel_dp);
 
 	ret = intel_dp_probe_mst(intel_dp);
 	if (ret) {
-		/* if we are in MST mode then this connector
-		   won't appear connected or have anything with EDID on it */
+		/*
+		 * if we are in MST mode then this connector
+		 * won't appear connected or have anything with
+		 * EDID on it
+		 */
 		if (intel_encoder->type != INTEL_OUTPUT_EDP)
 			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
 		status = connector_status_disconnected;
 		goto out;
+	} else if (connector->status == connector_status_connected) {
+
+		/*
+		 * If display was connected already and is still connected
+		 * check links status, there has been known issues of
+		 * link loss triggerring long pulse!!!!
+		 */
+		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+		intel_dp_check_link_status(intel_dp);
+		drm_modeset_unlock(&dev->mode_config.connection_mutex);
+		goto out;
 	}
 
 	intel_dp_set_edid(intel_dp);
@@ -4854,7 +4860,33 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 
 out:
 	intel_dp_power_put(intel_dp, power_domain);
-	return status;
+	return;
+}
+
+static enum drm_connector_status
+intel_dp_detect(struct drm_connector *connector, bool force)
+{
+	struct intel_dp *intel_dp = intel_attached_dp(connector);
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct intel_encoder *intel_encoder = &intel_dig_port->base;
+	struct intel_connector *intel_connector = to_intel_connector(connector);
+
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
+		      connector->base.id, connector->name);
+
+	if (intel_dp->is_mst) {
+		/* MST devices are disconnected from a monitor POV */
+		if (intel_encoder->type != INTEL_OUTPUT_EDP)
+			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
+		return connector_status_disconnected;
+	}
+
+	intel_dp_long_pulse(intel_dp->attached_connector);
+
+	if (intel_connector->detect_edid)
+		return connector_status_connected;
+	else
+		return connector_status_disconnected;
 }
 
 static void
-- 
2.6.1

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

[PATCH 2/6] drm/i915: Cleaning up intel_dp_hpd_pulse

[Shubhangi Shrivastava]

Current DP detection has DPCD operations split across
intel_dp_hpd_pulse and intel_dp_detect which contains
duplicates as well. Also intel_dp_detect is called
during modes enumeration as well which will result
in multiple dpcd operations. So this patch tries
to solve both these by bringing all DPCD operations
in one single function and make intel_dp_detect
use existing values instead of repeating same steps.

Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a0fe827..4e74cd6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4881,7 +4881,8 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 		return connector_status_disconnected;
 	}
 
-	intel_dp_long_pulse(intel_dp->attached_connector);
+	if (force)
+		intel_dp_long_pulse(intel_dp->attached_connector);
 
 	if (intel_connector->detect_edid)
 		return connector_status_connected;
@@ -5211,21 +5212,9 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 		/* indicate that we need to restart link training */
 		intel_dp->train_set_valid = false;
 
-		if (!intel_digital_port_connected(dev_priv, intel_dig_port))
-			goto mst_fail;
+		intel_dp_long_pulse(intel_dp->attached_connector);
+		goto put_power;
 
-		if (!intel_dp_get_dpcd(intel_dp)) {
-			goto mst_fail;
-		}
-
-		intel_dp_probe_oui(intel_dp);
-
-		if (!intel_dp_probe_mst(intel_dp)) {
-			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-			intel_dp_check_link_status(intel_dp);
-			drm_modeset_unlock(&dev->mode_config.connection_mutex);
-			goto mst_fail;
-		}
 	} else {
 		if (intel_dp->is_mst) {
 			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
-- 
2.6.1

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

[PATCH 3/6] drm/i915: Splitting intel_dp_check_link_status

[Shubhangi Shrivastava]

The link retraining part when EQ is not correct is
retained to intel_dp_check_link_status whereas other
operations are handled as part of intel_dp_short_pulse.
This change is required to avoid performing all DPCD
related operations on performing link retraining.

Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 49 +++++++++++++++++++++++++----------------
 1 file changed, 30 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 4e74cd6..92378b4 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4421,6 +4421,31 @@ go_again:
 	return -EINVAL;
 }
 
+static void
+intel_dp_check_link_status(struct intel_dp *intel_dp)
+{
+	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
+	u8 link_status[DP_LINK_STATUS_SIZE];
+
+	if (!intel_dp_get_link_status(intel_dp, link_status)) {
+		DRM_ERROR("Failed to get link status\n");
+		return;
+	}
+
+	if (!intel_encoder->base.crtc)
+		return;
+
+	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
+		return;
+
+	if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
+		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
+				intel_encoder->base.name);
+		intel_dp_start_link_train(intel_dp);
+		intel_dp_stop_link_train(intel_dp);
+	}
+}
+
 /*
  * According to DP spec
  * 5.1.2:
@@ -4430,21 +4455,12 @@ go_again:
  *  4. Check link status on receipt of hot-plug interrupt
  */
 static void
-intel_dp_check_link_status(struct intel_dp *intel_dp)
+intel_dp_short_pulse(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
-	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
 	u8 sink_irq_vector;
 	u8 link_status[DP_LINK_STATUS_SIZE];
 
-	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
-
-	if (!intel_encoder->base.crtc)
-		return;
-
-	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
-		return;
-
 	/* Try to read receiver status if the link appears to be up */
 	if (!intel_dp_get_link_status(intel_dp, link_status)) {
 		return;
@@ -4469,12 +4485,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
 	}
 
-	if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
-		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
-			      intel_encoder->base.name);
-		intel_dp_start_link_train(intel_dp);
-		intel_dp_stop_link_train(intel_dp);
-	}
+	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+	intel_dp_check_link_status(intel_dp);
+	drm_modeset_unlock(&dev->mode_config.connection_mutex);
 }
 
 /* XXX this is probably wrong for multiple downstream ports */
@@ -5222,9 +5235,7 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 		}
 
 		if (!intel_dp->is_mst) {
-			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-			intel_dp_check_link_status(intel_dp);
-			drm_modeset_unlock(&dev->mode_config.connection_mutex);
+			intel_dp_short_pulse(intel_dp);
 		}
 	}
 
-- 
2.6.1

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

[PATCH 4/6] drm/i915: Save sink_count for tracking changes to it

[Shubhangi Shrivastava]

Sink count can change between short pulse hpd hence this patch
adds a member variable to intel_dp so we can track any changes
between short pulse interrupts.

Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 7 +++----
 drivers/gpu/drm/i915/intel_drv.h | 1 +
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 92378b4..8014322 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4507,14 +4507,13 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
 	/* If we're HPD-aware, SINK_COUNT changes dynamically */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
 	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
-		uint8_t reg;
 
 		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
-					    &reg, 1) < 0)
+					    &intel_dp->sink_count, 1) < 0)
 			return connector_status_unknown;
 
-		return DP_GET_SINK_COUNT(reg) ? connector_status_connected
-					      : connector_status_disconnected;
+		return DP_GET_SINK_COUNT(intel_dp->sink_count) ?
+		connector_status_connected : connector_status_disconnected;
 	}
 
 	/* If no HPD, poke DDC gently */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1a3bbdc..abcedc5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -748,6 +748,7 @@ struct intel_dp {
 	uint32_t DP;
 	int link_rate;
 	uint8_t lane_count;
+	uint8_t sink_count;
 	bool has_audio;
 	enum hdmi_force_audio force_audio;
 	bool limited_color_range;
-- 
2.6.1

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

[PATCH 5/6] drm/i915: read sink_count dpcd always

[Shubhangi Shrivastava]

This patch reads sink_count dpcd always and removes its
read operation based on values in downstream port dpcd.

SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT dpcd.
SINK_COUNT denotes if a display is attached, while
DOWNSTREAM_PORT_PRESET indicates how many ports are available
in the dongle where display can be attached. so it is possible
for sink count to change irrespective of value in downstream
port dpcd.

Here is a table of possible values and scenarios

sink_count      downstream_port
                present
0               0               no display is attached
0               1               dongle is connected without display
1               0               display connected directly
1               1               display connected through dongle

v2: moved out crtc enabled checks to prior patch(Jani)

Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8014322..2acff47 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3988,6 +3988,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	if (intel_dp->dpcd[DP_DPCD_REV] == 0)
 		return false; /* DPCD not present */
 
+	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
+				    &intel_dp->sink_count, 1) < 0)
+		return false;
+
+	if (!DP_GET_SINK_COUNT(intel_dp->sink_count))
+		return false;
+
 	/* Check if the panel supports PSR */
 	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
 	if (is_edp(intel_dp)) {
@@ -4508,10 +4515,6 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
 	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
 
-		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
-					    &intel_dp->sink_count, 1) < 0)
-			return connector_status_unknown;
-
 		return DP_GET_SINK_COUNT(intel_dp->sink_count) ?
 		connector_status_connected : connector_status_disconnected;
 	}
-- 
2.6.1

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

[PATCH 6/6] drm/i915: force full detect on sink count change

[Shubhangi Shrivastava]

This patch checks for changes in sink count between short pulse
hpds and forces full detect when there is a change.

This will allow both detection of hotplug and unplug of panels
through dongles that give only short pulse for such events.

v2: changed variable type from u8 to bool (Jani)
    return immediately if perform_full_detect is set(Siva)

v3: changed method of determining full detection from using
    pointer to return code (Siva)

Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 2acff47..4ce41e7 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4461,21 +4461,30 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
  *  3. Use Link Training from 2.5.3.3 and 3.5.1.3
  *  4. Check link status on receipt of hot-plug interrupt
  */
-static void
+static bool
 intel_dp_short_pulse(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	u8 sink_irq_vector;
 	u8 link_status[DP_LINK_STATUS_SIZE];
+	u8 old_sink_count = intel_dp->sink_count;
+	bool ret;
 
 	/* Try to read receiver status if the link appears to be up */
 	if (!intel_dp_get_link_status(intel_dp, link_status)) {
-		return;
+		return false;
 	}
 
-	/* Now read the DPCD to see if it's actually running */
-	if (!intel_dp_get_dpcd(intel_dp)) {
-		return;
+	/* Now read the DPCD to see if it's actually running
+	 * Don't return immediately if dpcd read failed,
+	 * if sink count was 1 and dpcd read failed we need
+	 * to do full detection
+	 */
+	ret = intel_dp_get_dpcd(intel_dp);
+
+	if ((old_sink_count != intel_dp->sink_count) || !ret) {
+		/* No need to proceed if we are going to do full detect */
+		return false;
 	}
 
 	/* Try to read the source of the interrupt */
@@ -4495,6 +4504,8 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
 	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
 	intel_dp_check_link_status(intel_dp);
 	drm_modeset_unlock(&dev->mode_config.connection_mutex);
+
+	return true;
 }
 
 /* XXX this is probably wrong for multiple downstream ports */
@@ -5237,7 +5248,10 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 		}
 
 		if (!intel_dp->is_mst) {
-			intel_dp_short_pulse(intel_dp);
+			if (!intel_dp_short_pulse(intel_dp)) {
+				intel_dp_long_pulse(intel_dp->attached_connector);
+				goto put_power;
+			}
 		}
 	}
 
-- 
2.6.1

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

Re: [PATCH 1/6] drm/i915: Splitting intel_dp_detect

[Ander Conselvan De Oliveira]

On Fri, 2015-11-06 at 17:58 +0530, Shubhangi Shrivastava wrote:
> This patch moves probing for panel, DPCD read etc to another
> function intel_dp_long_pulse, while intel_dp_detect returns
> the status as connected or disconnected depending on
> whether the edid is available or not.

Why is the change in connector status necessary?

> This change will be required by further patches in the series
> to avoid performing multiple DPCD operations and removing
> their duplication.
> 
> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 66 ++++++++++++++++++++++++++++++----------
> -
>  1 file changed, 49 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 1cb1f3f..a0fe827 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4785,9 +4785,10 @@ intel_dp_power_put(struct intel_dp *dp,
>  	intel_display_power_put(to_i915(encoder->base.dev), power_domain);
>  }
>  
> -static enum drm_connector_status
> -intel_dp_detect(struct drm_connector *connector, bool force)
> +static void
> +intel_dp_long_pulse(struct intel_connector *intel_connector)
>  {
> +	struct drm_connector *connector = &intel_connector->base;
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>  	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> @@ -4797,17 +4798,6 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  	bool ret;
>  	u8 sink_irq_vector;
>  
> -	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> -		      connector->base.id, connector->name);
> -	intel_dp_unset_edid(intel_dp);
> -
> -	if (intel_dp->is_mst) {
> -		/* MST devices are disconnected from a monitor POV */
> -		if (intel_encoder->type != INTEL_OUTPUT_EDP)
> -			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> -		return connector_status_disconnected;
> -	}
> -
>  	power_domain = intel_dp_power_get(intel_dp);
>  
>  	/* Can't disconnect eDP, but you can close the lid... */
> @@ -4817,19 +4807,35 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  		status = ironlake_dp_detect(intel_dp);
>  	else
>  		status = g4x_dp_detect(intel_dp);
> -	if (status != connector_status_connected)
> +	if (status != connector_status_connected) {
> +		intel_dp_unset_edid(intel_dp);
>  		goto out;
> +	}
>  
>  	intel_dp_probe_oui(intel_dp);
>  
>  	ret = intel_dp_probe_mst(intel_dp);
>  	if (ret) {
> -		/* if we are in MST mode then this connector
> -		   won't appear connected or have anything with EDID on it */
> +		/*
> +		 * if we are in MST mode then this connector
> +		 * won't appear connected or have anything with
> +		 * EDID on it
> +		 */
>  		if (intel_encoder->type != INTEL_OUTPUT_EDP)
>  			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
>  		status = connector_status_disconnected;
>  		goto out;

Here the function will exit without a call to intel_dp_unset_edid(), which is
different from the behavior prior to this patch.

> +	} else if (connector->status == connector_status_connected) {
> +
> +		/*
> +		 * If display was connected already and is still connected
> +		 * check links status, there has been known issues of
> +		 * link loss triggerring long pulse!!!!
> +		 */
> +		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> +		intel_dp_check_link_status(intel_dp);
> +		drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +		goto out;
>  	}

The hunk above should be in a separate patch. It also doesn't call
intel_dp_unset_edid(). Might be easier to just add

out:
	if (status == connector_status_disconnected)
		intel_dp_unset_edid(intel_dp);

to the end of the function.

Ander

>  	intel_dp_set_edid(intel_dp);
> @@ -4854,7 +4860,33 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  
>  out:
>  	intel_dp_power_put(intel_dp, power_domain);
> -	return status;
> +	return;
> +}
> +
> +static enum drm_connector_status
> +intel_dp_detect(struct drm_connector *connector, bool force)
> +{
> +	struct intel_dp *intel_dp = intel_attached_dp(connector);
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> +	struct intel_connector *intel_connector =
> to_intel_connector(connector);
> +
> +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> +		      connector->base.id, connector->name);
> +
> +	if (intel_dp->is_mst) {
> +		/* MST devices are disconnected from a monitor POV */
> +		if (intel_encoder->type != INTEL_OUTPUT_EDP)
> +			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> +		return connector_status_disconnected;
> +	}
> +
> +	intel_dp_long_pulse(intel_dp->attached_connector);
> +
> +	if (intel_connector->detect_edid)
> +		return connector_status_connected;
> +	else
> +		return connector_status_disconnected;
>  }
>  
>  static void
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/6] drm/i915: Cleaning up intel_dp_hpd_pulse

[Ander Conselvan De Oliveira]

On Fri, 2015-11-06 at 17:58 +0530, Shubhangi Shrivastava wrote:
> Current DP detection has DPCD operations split across
> intel_dp_hpd_pulse and intel_dp_detect which contains
> duplicates as well. Also intel_dp_detect is called
> during modes enumeration as well which will result
> in multiple dpcd operations. So this patch tries
> to solve both these by bringing all DPCD operations
> in one single function and make intel_dp_detect
> use existing values instead of repeating same steps.
> 
> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 19 ++++---------------
>  1 file changed, 4 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a0fe827..4e74cd6 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4881,7 +4881,8 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  		return connector_status_disconnected;
>  	}
>  
> -	intel_dp_long_pulse(intel_dp->attached_connector);
> +	if (force)
> +		intel_dp_long_pulse(intel_dp->attached_connector);
>  
>  	if (intel_connector->detect_edid)
>  		return connector_status_connected;
> @@ -5211,21 +5212,9 @@ intel_dp_hpd_pulse(struct intel_digital_port
> *intel_dig_port, bool long_hpd)
>  		/* indicate that we need to restart link training */
>  		intel_dp->train_set_valid = false;
>  
> -		if (!intel_digital_port_connected(dev_priv, intel_dig_port))
> -			goto mst_fail;
> +		intel_dp_long_pulse(intel_dp->attached_connector);
> +		goto put_power;
>  
> -		if (!intel_dp_get_dpcd(intel_dp)) {
> -			goto mst_fail;
> -		}

So we don't call this for eDP anymore on long pulse, which I assume is harmless
since the bits we are reading from DPCD shouldn't change?

> -
> -		intel_dp_probe_oui(intel_dp);
> -
> -		if (!intel_dp_probe_mst(intel_dp)) {
> -			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> -			intel_dp_check_link_status(intel_dp);
> -			drm_modeset_unlock(&dev->mode_config.connection_mutex);
> -			goto mst_fail;
> -		}

Hmm, so this is where that hunk from patch 1 I said should be a separate patch
comes from. Looks like in belongs to this patch.

>  	} else {
>  		if (intel_dp->is_mst) {
>  			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/6] drm/i915: Cleaning up intel_dp_hpd_pulse

[Ander Conselvan De Oliveira]

On Mon, 2015-11-16 at 16:33 +0200, Ander Conselvan De Oliveira wrote:
> On Fri, 2015-11-06 at 17:58 +0530, Shubhangi Shrivastava wrote:
> > Current DP detection has DPCD operations split across
> > intel_dp_hpd_pulse and intel_dp_detect which contains
> > duplicates as well. Also intel_dp_detect is called
> > during modes enumeration as well which will result
> > in multiple dpcd operations. So this patch tries
> > to solve both these by bringing all DPCD operations
> > in one single function and make intel_dp_detect
> > use existing values instead of repeating same steps.
> > 
> > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> > Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 19 ++++---------------
> >  1 file changed, 4 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index a0fe827..4e74cd6 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4881,7 +4881,8 @@ intel_dp_detect(struct drm_connector *connector, bool
> > force)
> >  		return connector_status_disconnected;
> >  	}
> >  
> > -	intel_dp_long_pulse(intel_dp->attached_connector);
> > +	if (force)
> > +		intel_dp_long_pulse(intel_dp->attached_connector);
> >  
> >  	if (intel_connector->detect_edid)
> >  		return connector_status_connected;
> > @@ -5211,21 +5212,9 @@ intel_dp_hpd_pulse(struct intel_digital_port
> > *intel_dig_port, bool long_hpd)
> >  		/* indicate that we need to restart link training */
> >  		intel_dp->train_set_valid = false;
> >  
> > -		if (!intel_digital_port_connected(dev_priv, intel_dig_port))
> > -			goto mst_fail;
> > +		intel_dp_long_pulse(intel_dp->attached_connector);
> > +		goto put_power;

This skips the line that sets ret to IRQ_HANDLED, which will cause the hotplug
code to "fall back to old school hpd".

Ander

> >  
> > -		if (!intel_dp_get_dpcd(intel_dp)) {
> > -			goto mst_fail;
> > -		}
> 
> So we don't call this for eDP anymore on long pulse, which I assume is
> harmless
> since the bits we are reading from DPCD shouldn't change?
> 
> > -
> > -		intel_dp_probe_oui(intel_dp);
> > -
> > -		if (!intel_dp_probe_mst(intel_dp)) {
> > -			drm_modeset_lock(&dev
> > ->mode_config.connection_mutex, NULL);
> > -			intel_dp_check_link_status(intel_dp);
> > -			drm_modeset_unlock(&dev
> > ->mode_config.connection_mutex);
> > -			goto mst_fail;
> > -		}
> 
> Hmm, so this is where that hunk from patch 1 I said should be a separate patch
> comes from. Looks like in belongs to this patch.
> 
> >  	} else {
> >  		if (intel_dp->is_mst) {
> >  			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/6] drm/i915: Splitting intel_dp_detect

[Shubhangi Shrivastava]

On Monday 16 November 2015 07:10 PM, Ander Conselvan De Oliveira wrote:
> On Fri, 2015-11-06 at 17:58 +0530, Shubhangi Shrivastava wrote:
>> This patch moves probing for panel, DPCD read etc to another
>> function intel_dp_long_pulse, while intel_dp_detect returns
>> the status as connected or disconnected depending on
>> whether the edid is available or not.
> Why is the change in connector status necessary?
Since intel_dp_detect is also called in mode enumeration, so all detect 
related operations are moved to intel_dp_long_pulse to avoid any such 
operations to be performed during mode enumeration. intel_dp_long_pulse 
will perform all the panel probing, DPCD read operations etc. which will 
end with EDID being read. In intel_dp_detect, it is checked if EDID has 
been read or not. If EDID has been read then the status is returned as 
connected to allow further operations otherwise the status is returned 
as disconnected.

>
>> This change will be required by further patches in the series
>> to avoid performing multiple DPCD operations and removing
>> their duplication.
>>
>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 66 ++++++++++++++++++++++++++++++----------
>> -
>>   1 file changed, 49 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 1cb1f3f..a0fe827 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4785,9 +4785,10 @@ intel_dp_power_put(struct intel_dp *dp,
>>   	intel_display_power_put(to_i915(encoder->base.dev), power_domain);
>>   }
>>   
>> -static enum drm_connector_status
>> -intel_dp_detect(struct drm_connector *connector, bool force)
>> +static void
>> +intel_dp_long_pulse(struct intel_connector *intel_connector)
>>   {
>> +	struct drm_connector *connector = &intel_connector->base;
>>   	struct intel_dp *intel_dp = intel_attached_dp(connector);
>>   	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>>   	struct intel_encoder *intel_encoder = &intel_dig_port->base;
>> @@ -4797,17 +4798,6 @@ intel_dp_detect(struct drm_connector *connector, bool
>> force)
>>   	bool ret;
>>   	u8 sink_irq_vector;
>>   
>> -	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>> -		      connector->base.id, connector->name);
>> -	intel_dp_unset_edid(intel_dp);
>> -
>> -	if (intel_dp->is_mst) {
>> -		/* MST devices are disconnected from a monitor POV */
>> -		if (intel_encoder->type != INTEL_OUTPUT_EDP)
>> -			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
>> -		return connector_status_disconnected;
>> -	}
>> -
>>   	power_domain = intel_dp_power_get(intel_dp);
>>   
>>   	/* Can't disconnect eDP, but you can close the lid... */
>> @@ -4817,19 +4807,35 @@ intel_dp_detect(struct drm_connector *connector, bool
>> force)
>>   		status = ironlake_dp_detect(intel_dp);
>>   	else
>>   		status = g4x_dp_detect(intel_dp);
>> -	if (status != connector_status_connected)
>> +	if (status != connector_status_connected) {
>> +		intel_dp_unset_edid(intel_dp);
>>   		goto out;
>> +	}
>>   
>>   	intel_dp_probe_oui(intel_dp);
>>   
>>   	ret = intel_dp_probe_mst(intel_dp);
>>   	if (ret) {
>> -		/* if we are in MST mode then this connector
>> -		   won't appear connected or have anything with EDID on it */
>> +		/*
>> +		 * if we are in MST mode then this connector
>> +		 * won't appear connected or have anything with
>> +		 * EDID on it
>> +		 */
>>   		if (intel_encoder->type != INTEL_OUTPUT_EDP)
>>   			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
>>   		status = connector_status_disconnected;
>>   		goto out;
> Here the function will exit without a call to intel_dp_unset_edid(), which is
> different from the behavior prior to this patch.
Moved call to intel_dp_unset_edid() to out, as per next comment.
>
>> +	} else if (connector->status == connector_status_connected) {
>> +
>> +		/*
>> +		 * If display was connected already and is still connected
>> +		 * check links status, there has been known issues of
>> +		 * link loss triggerring long pulse!!!!
>> +		 */
>> +		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> +		intel_dp_check_link_status(intel_dp);
>> +		drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> +		goto out;
>>   	}
> The hunk above should be in a separate patch. It also doesn't call
> intel_dp_unset_edid(). Might be easier to just add
>
> out:
> 	if (status == connector_status_disconnected)
> 		intel_dp_unset_edid(intel_dp);
>
> to the end of the function.
>
> Ander
Done. Moved the hunk to next patch and call to intel_dp_unset_edid to out.
>
>>   	intel_dp_set_edid(intel_dp);
>> @@ -4854,7 +4860,33 @@ intel_dp_detect(struct drm_connector *connector, bool
>> force)
>>   
>>   out:
>>   	intel_dp_power_put(intel_dp, power_domain);
>> -	return status;
>> +	return;
>> +}
>> +
>> +static enum drm_connector_status
>> +intel_dp_detect(struct drm_connector *connector, bool force)
>> +{
>> +	struct intel_dp *intel_dp = intel_attached_dp(connector);
>> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>> +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
>> +	struct intel_connector *intel_connector =
>> to_intel_connector(connector);
>> +
>> +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>> +		      connector->base.id, connector->name);
>> +
>> +	if (intel_dp->is_mst) {
>> +		/* MST devices are disconnected from a monitor POV */
>> +		if (intel_encoder->type != INTEL_OUTPUT_EDP)
>> +			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
>> +		return connector_status_disconnected;
>> +	}
>> +
>> +	intel_dp_long_pulse(intel_dp->attached_connector);
>> +
>> +	if (intel_connector->detect_edid)
>> +		return connector_status_connected;
>> +	else
>> +		return connector_status_disconnected;
>>   }
>>   
>>   static void

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

[PATCH] drm/i915: Splitting intel_dp_detect

[Shubhangi Shrivastava]

This patch moves probing for panel, DPCD read etc to another
function intel_dp_long_pulse, while intel_dp_detect returns
the status as connected or disconnected depending on
whether the edid is available or not.
This change will be required by further patches in the series
to avoid performing multiple DPCD operations and removing
their duplication.

v2: Moved a hunk to next patch of the series.
    Moved intel_dp_unset_edid to out. (Ander)

Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 52 ++++++++++++++++++++++++++++-------------
 1 file changed, 36 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8071247..dd2b9da 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4631,9 +4631,10 @@ intel_dp_power_put(struct intel_dp *dp,
 	intel_display_power_put(to_i915(encoder->base.dev), power_domain);
 }
 
-static enum drm_connector_status
-intel_dp_detect(struct drm_connector *connector, bool force)
+static void
+intel_dp_long_pulse(struct intel_connector *intel_connector)
 {
+	struct drm_connector *connector = &intel_connector->base;
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
@@ -4643,17 +4644,6 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	bool ret;
 	u8 sink_irq_vector;
 
-	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
-		      connector->base.id, connector->name);
-	intel_dp_unset_edid(intel_dp);
-
-	if (intel_dp->is_mst) {
-		/* MST devices are disconnected from a monitor POV */
-		if (intel_encoder->type != INTEL_OUTPUT_EDP)
-			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
-		return connector_status_disconnected;
-	}
-
 	power_domain = intel_dp_power_get(intel_dp);
 
 	/* Can't disconnect eDP, but you can close the lid... */
@@ -4670,8 +4660,10 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 
 	ret = intel_dp_probe_mst(intel_dp);
 	if (ret) {
-		/* if we are in MST mode then this connector
-		   won't appear connected or have anything with EDID on it */
+		/*
+		 * if we are in MST mode then this connector
+		 * won't appear connected or have anything with EDID on it
+		 */
 		if (intel_encoder->type != INTEL_OUTPUT_EDP)
 			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
 		status = connector_status_disconnected;
@@ -4699,8 +4691,36 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	}
 
 out:
+	if (status != connector_status_connected)
+		intel_dp_unset_edid(intel_dp);
 	intel_dp_power_put(intel_dp, power_domain);
-	return status;
+	return;
+}
+
+static enum drm_connector_status
+intel_dp_detect(struct drm_connector *connector, bool force)
+{
+	struct intel_dp *intel_dp = intel_attached_dp(connector);
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct intel_encoder *intel_encoder = &intel_dig_port->base;
+	struct intel_connector *intel_connector = to_intel_connector(connector);
+
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
+		      connector->base.id, connector->name);
+
+	if (intel_dp->is_mst) {
+		/* MST devices are disconnected from a monitor POV */
+		if (intel_encoder->type != INTEL_OUTPUT_EDP)
+			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
+		return connector_status_disconnected;
+	}
+
+	intel_dp_long_pulse(intel_dp->attached_connector);
+
+	if (intel_connector->detect_edid)
+		return connector_status_connected;
+	else
+		return connector_status_disconnected;
 }
 
 static void
-- 
2.6.1

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

Re: [PATCH 2/6] drm/i915: Cleaning up intel_dp_hpd_pulse

[Shubhangi Shrivastava]

On Monday 16 November 2015 08:03 PM, Ander Conselvan De Oliveira wrote:
> On Fri, 2015-11-06 at 17:58 +0530, Shubhangi Shrivastava wrote:
>> Current DP detection has DPCD operations split across
>> intel_dp_hpd_pulse and intel_dp_detect which contains
>> duplicates as well. Also intel_dp_detect is called
>> during modes enumeration as well which will result
>> in multiple dpcd operations. So this patch tries
>> to solve both these by bringing all DPCD operations
>> in one single function and make intel_dp_detect
>> use existing values instead of repeating same steps.
>>
>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c | 19 ++++---------------
>>   1 file changed, 4 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index a0fe827..4e74cd6 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4881,7 +4881,8 @@ intel_dp_detect(struct drm_connector *connector, bool
>> force)
>>   		return connector_status_disconnected;
>>   	}
>>   
>> -	intel_dp_long_pulse(intel_dp->attached_connector);
>> +	if (force)
>> +		intel_dp_long_pulse(intel_dp->attached_connector);
>>   
>>   	if (intel_connector->detect_edid)
>>   		return connector_status_connected;
>> @@ -5211,21 +5212,9 @@ intel_dp_hpd_pulse(struct intel_digital_port
>> *intel_dig_port, bool long_hpd)
>>   		/* indicate that we need to restart link training */
>>   		intel_dp->train_set_valid = false;
>>   
>> -		if (!intel_digital_port_connected(dev_priv, intel_dig_port))
>> -			goto mst_fail;
>> +		intel_dp_long_pulse(intel_dp->attached_connector);
>> +		goto put_power;
>>   
>> -		if (!intel_dp_get_dpcd(intel_dp)) {
>> -			goto mst_fail;
>> -		}
> So we don't call this for eDP anymore on long pulse, which I assume is harmless
> since the bits we are reading from DPCD shouldn't change?
>
>> -
>> -		intel_dp_probe_oui(intel_dp);
>> -
>> -		if (!intel_dp_probe_mst(intel_dp)) {
>> -			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> -			intel_dp_check_link_status(intel_dp);
>> -			drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> -			goto mst_fail;
>> -		}
> Hmm, so this is where that hunk from patch 1 I said should be a separate patch
> comes from. Looks like in belongs to this patch.
Pulled in that hunk from patch 1 to this patch.
>
>>   	} else {
>>   		if (intel_dp->is_mst) {
>>   			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)

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

[PATCH] drm/i915: Cleaning up intel_dp_hpd_pulse

[Shubhangi Shrivastava]

Current DP detection has DPCD operations split across
intel_dp_hpd_pulse and intel_dp_detect which contains
duplicates as well. Also intel_dp_detect is called
during modes enumeration as well which will result
in multiple dpcd operations. So this patch tries
to solve both these by bringing all DPCD operations
in one single function and make intel_dp_detect
use existing values instead of repeating same steps.

v2: Pulled in a hunk from last patch of the series to
    this patch. (Ander)

Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index dd2b9da..e4d6d33 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4668,6 +4668,16 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
 		status = connector_status_disconnected;
 		goto out;
+	} else if (connector->status == connector_status_connected) {
+		/*
+		 * If display was connected already and is still connected
+		 * check links status, there has been known issues of
+		 * link loss triggerring long pulse!!!!
+		 */
+		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+		intel_dp_check_link_status(intel_dp);
+		drm_modeset_unlock(&dev->mode_config.connection_mutex);
+		goto out;
 	}
 
 	intel_dp_set_edid(intel_dp);
@@ -4715,7 +4725,8 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 		return connector_status_disconnected;
 	}
 
-	intel_dp_long_pulse(intel_dp->attached_connector);
+	if (force)
+		intel_dp_long_pulse(intel_dp->attached_connector);
 
 	if (intel_connector->detect_edid)
 		return connector_status_connected;
@@ -5045,21 +5056,9 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 		/* indicate that we need to restart link training */
 		intel_dp->train_set_valid = false;
 
-		if (!intel_digital_port_connected(dev_priv, intel_dig_port))
-			goto mst_fail;
-
-		if (!intel_dp_get_dpcd(intel_dp)) {
-			goto mst_fail;
-		}
-
-		intel_dp_probe_oui(intel_dp);
+		intel_dp_long_pulse(intel_dp->attached_connector);
+		goto put_power;
 
-		if (!intel_dp_probe_mst(intel_dp)) {
-			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-			intel_dp_check_link_status(intel_dp);
-			drm_modeset_unlock(&dev->mode_config.connection_mutex);
-			goto mst_fail;
-		}
 	} else {
 		if (intel_dp->is_mst) {
 			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
-- 
2.6.1

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

Re: [PATCH 2/6] drm/i915: Cleaning up intel_dp_hpd_pulse

[Shubhangi Shrivastava]

On Monday 16 November 2015 08:16 PM, Ander Conselvan De Oliveira wrote:
> On Mon, 2015-11-16 at 16:33 +0200, Ander Conselvan De Oliveira wrote:
>> On Fri, 2015-11-06 at 17:58 +0530, Shubhangi Shrivastava wrote:
>>> Current DP detection has DPCD operations split across
>>> intel_dp_hpd_pulse and intel_dp_detect which contains
>>> duplicates as well. Also intel_dp_detect is called
>>> during modes enumeration as well which will result
>>> in multiple dpcd operations. So this patch tries
>>> to solve both these by bringing all DPCD operations
>>> in one single function and make intel_dp_detect
>>> use existing values instead of repeating same steps.
>>>
>>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>>> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_dp.c | 19 ++++---------------
>>>   1 file changed, 4 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>> b/drivers/gpu/drm/i915/intel_dp.c
>>> index a0fe827..4e74cd6 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -4881,7 +4881,8 @@ intel_dp_detect(struct drm_connector *connector, bool
>>> force)
>>>   		return connector_status_disconnected;
>>>   	}
>>>   
>>> -	intel_dp_long_pulse(intel_dp->attached_connector);
>>> +	if (force)
>>> +		intel_dp_long_pulse(intel_dp->attached_connector);
>>>   
>>>   	if (intel_connector->detect_edid)
>>>   		return connector_status_connected;
>>> @@ -5211,21 +5212,9 @@ intel_dp_hpd_pulse(struct intel_digital_port
>>> *intel_dig_port, bool long_hpd)
>>>   		/* indicate that we need to restart link training */
>>>   		intel_dp->train_set_valid = false;
>>>   
>>> -		if (!intel_digital_port_connected(dev_priv, intel_dig_port))
>>> -			goto mst_fail;
>>> +		intel_dp_long_pulse(intel_dp->attached_connector);
>>> +		goto put_power;
> This skips the line that sets ret to IRQ_HANDLED, which will cause the hotplug
> code to "fall back to old school hpd".
>
> Ander
Notification of hotplug to user mode goes from i915_hotplug_work_func. 
Returning IRQ_NONE from here will notify user mode about hotplug.
>
>>>   
>>> -		if (!intel_dp_get_dpcd(intel_dp)) {
>>> -			goto mst_fail;
>>> -		}
>> So we don't call this for eDP anymore on long pulse, which I assume is
>> harmless
>> since the bits we are reading from DPCD shouldn't change?
>>
>>> -
>>> -		intel_dp_probe_oui(intel_dp);
>>> -
>>> -		if (!intel_dp_probe_mst(intel_dp)) {
>>> -			drm_modeset_lock(&dev
>>> ->mode_config.connection_mutex, NULL);
>>> -			intel_dp_check_link_status(intel_dp);
>>> -			drm_modeset_unlock(&dev
>>> ->mode_config.connection_mutex);
>>> -			goto mst_fail;
>>> -		}
>> Hmm, so this is where that hunk from patch 1 I said should be a separate patch
>> comes from. Looks like in belongs to this patch.
>>
>>>   	} else {
>>>   		if (intel_dp->is_mst) {
>>>   			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)

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

Re: [PATCH 1/6] drm/i915: Splitting intel_dp_detect

[Ander Conselvan De Oliveira]

On Tue, 2015-11-17 at 11:47 +0530, Shubhangi Shrivastava wrote:
> 
> On Monday 16 November 2015 07:10 PM, Ander Conselvan De Oliveira wrote:
> > On Fri, 2015-11-06 at 17:58 +0530, Shubhangi Shrivastava wrote:
> > > This patch moves probing for panel, DPCD read etc to another
> > > function intel_dp_long_pulse, while intel_dp_detect returns
> > > the status as connected or disconnected depending on
> > > whether the edid is available or not.
> > Why is the change in connector status necessary?
> Since intel_dp_detect is also called in mode enumeration, so all detect 
> related operations are moved to intel_dp_long_pulse to avoid any such 
> operations to be performed during mode enumeration. intel_dp_long_pulse 
> will perform all the panel probing, DPCD read operations etc. which will 
> end with EDID being read. In intel_dp_detect, it is checked if EDID has 
> been read or not. If EDID has been read then the status is returned as 
> connected to allow further operations otherwise the status is returned 
> as disconnected.

So if I understood correctly, the EDID change is only a about doing a better
split of the code. My concern is that drm_get_edid() might fail. Before this
patch that wouldn't affect the connector status, but now a failure to read EDID 
implies that the connector is disconnected. I'm not sure in which scenario this
would happen, so I wanted to know if this change in behavior is intentional.

Ander

> 
> > 
> > > This change will be required by further patches in the series
> > > to avoid performing multiple DPCD operations and removing
> > > their duplication.
> > > 
> > > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> > > Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/intel_dp.c | 66 ++++++++++++++++++++++++++++++-----
> > > -----
> > > -
> > >   1 file changed, 49 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index 1cb1f3f..a0fe827 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -4785,9 +4785,10 @@ intel_dp_power_put(struct intel_dp *dp,
> > >   	intel_display_power_put(to_i915(encoder->base.dev),
> > > power_domain);
> > >   }
> > >   
> > > -static enum drm_connector_status
> > > -intel_dp_detect(struct drm_connector *connector, bool force)
> > > +static void
> > > +intel_dp_long_pulse(struct intel_connector *intel_connector)
> > >   {
> > > +	struct drm_connector *connector = &intel_connector->base;
> > >   	struct intel_dp *intel_dp = intel_attached_dp(connector);
> > >   	struct intel_digital_port *intel_dig_port =
> > > dp_to_dig_port(intel_dp);
> > >   	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> > > @@ -4797,17 +4798,6 @@ intel_dp_detect(struct drm_connector *connector,
> > > bool
> > > force)
> > >   	bool ret;
> > >   	u8 sink_irq_vector;
> > >   
> > > -	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > > -		      connector->base.id, connector->name);
> > > -	intel_dp_unset_edid(intel_dp);
> > > -
> > > -	if (intel_dp->is_mst) {
> > > -		/* MST devices are disconnected from a monitor POV */
> > > -		if (intel_encoder->type != INTEL_OUTPUT_EDP)
> > > -			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> > > -		return connector_status_disconnected;
> > > -	}
> > > -
> > >   	power_domain = intel_dp_power_get(intel_dp);
> > >   
> > >   	/* Can't disconnect eDP, but you can close the lid... */
> > > @@ -4817,19 +4807,35 @@ intel_dp_detect(struct drm_connector *connector,
> > > bool
> > > force)
> > >   		status = ironlake_dp_detect(intel_dp);
> > >   	else
> > >   		status = g4x_dp_detect(intel_dp);
> > > -	if (status != connector_status_connected)
> > > +	if (status != connector_status_connected) {
> > > +		intel_dp_unset_edid(intel_dp);
> > >   		goto out;
> > > +	}
> > >   
> > >   	intel_dp_probe_oui(intel_dp);
> > >   
> > >   	ret = intel_dp_probe_mst(intel_dp);
> > >   	if (ret) {
> > > -		/* if we are in MST mode then this connector
> > > -		   won't appear connected or have anything with EDID on
> > > it */
> > > +		/*
> > > +		 * if we are in MST mode then this connector
> > > +		 * won't appear connected or have anything with
> > > +		 * EDID on it
> > > +		 */
> > >   		if (intel_encoder->type != INTEL_OUTPUT_EDP)
> > >   			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> > >   		status = connector_status_disconnected;
> > >   		goto out;
> > Here the function will exit without a call to intel_dp_unset_edid(), which
> > is
> > different from the behavior prior to this patch.
> Moved call to intel_dp_unset_edid() to out, as per next comment.
> > 
> > > +	} else if (connector->status == connector_status_connected) {
> > > +
> > > +		/*
> > > +		 * If display was connected already and is still
> > > connected
> > > +		 * check links status, there has been known issues of
> > > +		 * link loss triggerring long pulse!!!!
> > > +		 */
> > > +		drm_modeset_lock(&dev->mode_config.connection_mutex,
> > > NULL);
> > > +		intel_dp_check_link_status(intel_dp);
> > > +		drm_modeset_unlock(&dev->mode_config.connection_mutex);
> > > +		goto out;
> > >   	}
> > The hunk above should be in a separate patch. It also doesn't call
> > intel_dp_unset_edid(). Might be easier to just add
> > 
> > out:
> > 	if (status == connector_status_disconnected)
> > 		intel_dp_unset_edid(intel_dp);
> > 
> > to the end of the function.
> > 
> > Ander
> Done. Moved the hunk to next patch and call to intel_dp_unset_edid to out.
> > 
> > >   	intel_dp_set_edid(intel_dp);
> > > @@ -4854,7 +4860,33 @@ intel_dp_detect(struct drm_connector *connector,
> > > bool
> > > force)
> > >   
> > >   out:
> > >   	intel_dp_power_put(intel_dp, power_domain);
> > > -	return status;
> > > +	return;
> > > +}
> > > +
> > > +static enum drm_connector_status
> > > +intel_dp_detect(struct drm_connector *connector, bool force)
> > > +{
> > > +	struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > +	struct intel_digital_port *intel_dig_port =
> > > dp_to_dig_port(intel_dp);
> > > +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> > > +	struct intel_connector *intel_connector =
> > > to_intel_connector(connector);
> > > +
> > > +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > > +		      connector->base.id, connector->name);
> > > +
> > > +	if (intel_dp->is_mst) {
> > > +		/* MST devices are disconnected from a monitor POV */
> > > +		if (intel_encoder->type != INTEL_OUTPUT_EDP)
> > > +			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> > > +		return connector_status_disconnected;
> > > +	}
> > > +
> > > +	intel_dp_long_pulse(intel_dp->attached_connector);
> > > +
> > > +	if (intel_connector->detect_edid)
> > > +		return connector_status_connected;
> > > +	else
> > > +		return connector_status_disconnected;
> > >   }
> > >   
> > >   static void
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/6] drm/i915: Splitting intel_dp_detect

[Shubhangi Shrivastava]

On Tuesday 17 November 2015 02:52 PM, Ander Conselvan De Oliveira wrote:
> On Tue, 2015-11-17 at 11:47 +0530, Shubhangi Shrivastava wrote:
>> On Monday 16 November 2015 07:10 PM, Ander Conselvan De Oliveira wrote:
>>> On Fri, 2015-11-06 at 17:58 +0530, Shubhangi Shrivastava wrote:
>>>> This patch moves probing for panel, DPCD read etc to another
>>>> function intel_dp_long_pulse, while intel_dp_detect returns
>>>> the status as connected or disconnected depending on
>>>> whether the edid is available or not.
>>> Why is the change in connector status necessary?
>> Since intel_dp_detect is also called in mode enumeration, so all detect
>> related operations are moved to intel_dp_long_pulse to avoid any such
>> operations to be performed during mode enumeration. intel_dp_long_pulse
>> will perform all the panel probing, DPCD read operations etc. which will
>> end with EDID being read. In intel_dp_detect, it is checked if EDID has
>> been read or not. If EDID has been read then the status is returned as
>> connected to allow further operations otherwise the status is returned
>> as disconnected.
> So if I understood correctly, the EDID change is only a about doing a better
> split of the code. My concern is that drm_get_edid() might fail. Before this
> patch that wouldn't affect the connector status, but now a failure to read EDID
> implies that the connector is disconnected. I'm not sure in which scenario this
> would happen, so I wanted to know if this change in behavior is intentional.
>
> Ander
Yes, this is intentional. Intel_dp_detect is supposed to be called after 
the long pulse has been handled. If EDID is not present, that means 
either long pulse was never received or there is no panel connected.

>
>>>> This change will be required by further patches in the series
>>>> to avoid performing multiple DPCD operations and removing
>>>> their duplication.
>>>>
>>>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>>>> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_dp.c | 66 ++++++++++++++++++++++++++++++-----
>>>> -----
>>>> -
>>>>    1 file changed, 49 insertions(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>>> b/drivers/gpu/drm/i915/intel_dp.c
>>>> index 1cb1f3f..a0fe827 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -4785,9 +4785,10 @@ intel_dp_power_put(struct intel_dp *dp,
>>>>    	intel_display_power_put(to_i915(encoder->base.dev),
>>>> power_domain);
>>>>    }
>>>>    
>>>> -static enum drm_connector_status
>>>> -intel_dp_detect(struct drm_connector *connector, bool force)
>>>> +static void
>>>> +intel_dp_long_pulse(struct intel_connector *intel_connector)
>>>>    {
>>>> +	struct drm_connector *connector = &intel_connector->base;
>>>>    	struct intel_dp *intel_dp = intel_attached_dp(connector);
>>>>    	struct intel_digital_port *intel_dig_port =
>>>> dp_to_dig_port(intel_dp);
>>>>    	struct intel_encoder *intel_encoder = &intel_dig_port->base;
>>>> @@ -4797,17 +4798,6 @@ intel_dp_detect(struct drm_connector *connector,
>>>> bool
>>>> force)
>>>>    	bool ret;
>>>>    	u8 sink_irq_vector;
>>>>    
>>>> -	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>>>> -		      connector->base.id, connector->name);
>>>> -	intel_dp_unset_edid(intel_dp);
>>>> -
>>>> -	if (intel_dp->is_mst) {
>>>> -		/* MST devices are disconnected from a monitor POV */
>>>> -		if (intel_encoder->type != INTEL_OUTPUT_EDP)
>>>> -			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
>>>> -		return connector_status_disconnected;
>>>> -	}
>>>> -
>>>>    	power_domain = intel_dp_power_get(intel_dp);
>>>>    
>>>>    	/* Can't disconnect eDP, but you can close the lid... */
>>>> @@ -4817,19 +4807,35 @@ intel_dp_detect(struct drm_connector *connector,
>>>> bool
>>>> force)
>>>>    		status = ironlake_dp_detect(intel_dp);
>>>>    	else
>>>>    		status = g4x_dp_detect(intel_dp);
>>>> -	if (status != connector_status_connected)
>>>> +	if (status != connector_status_connected) {
>>>> +		intel_dp_unset_edid(intel_dp);
>>>>    		goto out;
>>>> +	}
>>>>    
>>>>    	intel_dp_probe_oui(intel_dp);
>>>>    
>>>>    	ret = intel_dp_probe_mst(intel_dp);
>>>>    	if (ret) {
>>>> -		/* if we are in MST mode then this connector
>>>> -		   won't appear connected or have anything with EDID on
>>>> it */
>>>> +		/*
>>>> +		 * if we are in MST mode then this connector
>>>> +		 * won't appear connected or have anything with
>>>> +		 * EDID on it
>>>> +		 */
>>>>    		if (intel_encoder->type != INTEL_OUTPUT_EDP)
>>>>    			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
>>>>    		status = connector_status_disconnected;
>>>>    		goto out;
>>> Here the function will exit without a call to intel_dp_unset_edid(), which
>>> is
>>> different from the behavior prior to this patch.
>> Moved call to intel_dp_unset_edid() to out, as per next comment.
>>>> +	} else if (connector->status == connector_status_connected) {
>>>> +
>>>> +		/*
>>>> +		 * If display was connected already and is still
>>>> connected
>>>> +		 * check links status, there has been known issues of
>>>> +		 * link loss triggerring long pulse!!!!
>>>> +		 */
>>>> +		drm_modeset_lock(&dev->mode_config.connection_mutex,
>>>> NULL);
>>>> +		intel_dp_check_link_status(intel_dp);
>>>> +		drm_modeset_unlock(&dev->mode_config.connection_mutex);
>>>> +		goto out;
>>>>    	}
>>> The hunk above should be in a separate patch. It also doesn't call
>>> intel_dp_unset_edid(). Might be easier to just add
>>>
>>> out:
>>> 	if (status == connector_status_disconnected)
>>> 		intel_dp_unset_edid(intel_dp);
>>>
>>> to the end of the function.
>>>
>>> Ander
>> Done. Moved the hunk to next patch and call to intel_dp_unset_edid to out.
>>>>    	intel_dp_set_edid(intel_dp);
>>>> @@ -4854,7 +4860,33 @@ intel_dp_detect(struct drm_connector *connector,
>>>> bool
>>>> force)
>>>>    
>>>>    out:
>>>>    	intel_dp_power_put(intel_dp, power_domain);
>>>> -	return status;
>>>> +	return;
>>>> +}
>>>> +
>>>> +static enum drm_connector_status
>>>> +intel_dp_detect(struct drm_connector *connector, bool force)
>>>> +{
>>>> +	struct intel_dp *intel_dp = intel_attached_dp(connector);
>>>> +	struct intel_digital_port *intel_dig_port =
>>>> dp_to_dig_port(intel_dp);
>>>> +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
>>>> +	struct intel_connector *intel_connector =
>>>> to_intel_connector(connector);
>>>> +
>>>> +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>>>> +		      connector->base.id, connector->name);
>>>> +
>>>> +	if (intel_dp->is_mst) {
>>>> +		/* MST devices are disconnected from a monitor POV */
>>>> +		if (intel_encoder->type != INTEL_OUTPUT_EDP)
>>>> +			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
>>>> +		return connector_status_disconnected;
>>>> +	}
>>>> +
>>>> +	intel_dp_long_pulse(intel_dp->attached_connector);
>>>> +
>>>> +	if (intel_connector->detect_edid)
>>>> +		return connector_status_connected;
>>>> +	else
>>>> +		return connector_status_disconnected;
>>>>    }
>>>>    
>>>>    static void

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

Re: [PATCH 2/6] drm/i915: Cleaning up intel_dp_hpd_pulse

[Ander Conselvan De Oliveira]

On Tue, 2015-11-17 at 12:23 +0530, Shubhangi Shrivastava wrote:
> 
> On Monday 16 November 2015 08:16 PM, Ander Conselvan De Oliveira wrote:
> > On Mon, 2015-11-16 at 16:33 +0200, Ander Conselvan De Oliveira wrote:
> > > On Fri, 2015-11-06 at 17:58 +0530, Shubhangi Shrivastava wrote:
> > > > Current DP detection has DPCD operations split across
> > > > intel_dp_hpd_pulse and intel_dp_detect which contains
> > > > duplicates as well. Also intel_dp_detect is called
> > > > during modes enumeration as well which will result
> > > > in multiple dpcd operations. So this patch tries
> > > > to solve both these by bringing all DPCD operations
> > > > in one single function and make intel_dp_detect
> > > > use existing values instead of repeating same steps.
> > > > 
> > > > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> > > > Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/i915/intel_dp.c | 19 ++++---------------
> > > >   1 file changed, 4 insertions(+), 15 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index a0fe827..4e74cd6 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -4881,7 +4881,8 @@ intel_dp_detect(struct drm_connector *connector,
> > > > bool
> > > > force)
> > > >   		return connector_status_disconnected;
> > > >   	}
> > > >   
> > > > -	intel_dp_long_pulse(intel_dp->attached_connector);
> > > > +	if (force)
> > > > +		intel_dp_long_pulse(intel_dp->attached_connector);
> > > >   
> > > >   	if (intel_connector->detect_edid)
> > > >   		return connector_status_connected;
> > > > @@ -5211,21 +5212,9 @@ intel_dp_hpd_pulse(struct intel_digital_port
> > > > *intel_dig_port, bool long_hpd)
> > > >   		/* indicate that we need to restart link training */
> > > >   		intel_dp->train_set_valid = false;
> > > >   
> > > > -		if (!intel_digital_port_connected(dev_priv,
> > > > intel_dig_port))
> > > > -			goto mst_fail;
> > > > +		intel_dp_long_pulse(intel_dp->attached_connector);
> > > > +		goto put_power;
> > This skips the line that sets ret to IRQ_HANDLED, which will cause the
> > hotplug
> > code to "fall back to old school hpd".
> > 
> > Ander
> Notification of hotplug to user mode goes from i915_hotplug_work_func. 
> Returning IRQ_NONE from here will notify user mode about hotplug.

So if I understand correctly, previously if the device was connected, we
succeeded in getting the dpcd and the device was mst, there was no hotplug event
being sent. With the change above the hotplug event is generated
unconditionally, and we never do the error handling in mst_fail for the long pul
se.

Ander


> > 
> > > >   
> > > > -		if (!intel_dp_get_dpcd(intel_dp)) {
> > > > -			goto mst_fail;
> > > > -		}
> > > So we don't call this for eDP anymore on long pulse, which I assume is
> > > harmless
> > > since the bits we are reading from DPCD shouldn't change?
> > > 
> > > > -
> > > > -		intel_dp_probe_oui(intel_dp);
> > > > -
> > > > -		if (!intel_dp_probe_mst(intel_dp)) {
> > > > -			drm_modeset_lock(&dev
> > > > ->mode_config.connection_mutex, NULL);
> > > > -			intel_dp_check_link_status(intel_dp);
> > > > -			drm_modeset_unlock(&dev
> > > > ->mode_config.connection_mutex);
> > > > -			goto mst_fail;
> > > > -		}
> > > Hmm, so this is where that hunk from patch 1 I said should be a separate
> > > patch
> > > comes from. Looks like in belongs to this patch.
> > > 
> > > >   	} else {
> > > >   		if (intel_dp->is_mst) {
> > > >   			if (intel_dp_check_mst_status(intel_dp) == 
> > > > -EINVAL)
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Cleaning up intel_dp_hpd_pulse

[Ander Conselvan De Oliveira]

On Tue, 2015-11-17 at 12:17 +0530, Shubhangi Shrivastava wrote:
> Current DP detection has DPCD operations split across
> intel_dp_hpd_pulse and intel_dp_detect which contains
> duplicates as well. Also intel_dp_detect is called
> during modes enumeration as well which will result
> in multiple dpcd operations. So this patch tries
> to solve both these by bringing all DPCD operations
> in one single function and make intel_dp_detect
> use existing values instead of repeating same steps.
> 
> v2: Pulled in a hunk from last patch of the series to
>     this patch. (Ander)
> 
> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index dd2b9da..e4d6d33 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4668,6 +4668,16 @@ intel_dp_long_pulse(struct intel_connector
> *intel_connector)
>  			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
>  		status = connector_status_disconnected;
>  		goto out;
> +	} else if (connector->status == connector_status_connected) {
> +		/*
> +		 * If display was connected already and is still connected
> +		 * check links status, there has been known issues of
> +		 * link loss triggerring long pulse!!!!
> +		 */
> +		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> +		intel_dp_check_link_status(intel_dp);
> +		drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +		goto out;
>  	}
>  
>  	intel_dp_set_edid(intel_dp);
> @@ -4715,7 +4725,8 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  		return connector_status_disconnected;
>  	}
>  
> -	intel_dp_long_pulse(intel_dp->attached_connector);
> +	if (force)
> +		intel_dp_long_pulse(intel_dp->attached_connector);
>  
>  	if (intel_connector->detect_edid)
>  		return connector_status_connected;
> @@ -5045,21 +5056,9 @@ intel_dp_hpd_pulse(struct intel_digital_port
> *intel_dig_port, bool long_hpd)
>  		/* indicate that we need to restart link training */
>  		intel_dp->train_set_valid = false;
>  
> -		if (!intel_digital_port_connected(dev_priv, intel_dig_port))
> -			goto mst_fail;
> -
> -		if (!intel_dp_get_dpcd(intel_dp)) {
> -			goto mst_fail;
> -		}
> -
> -		intel_dp_probe_oui(intel_dp);
> +		intel_dp_long_pulse(intel_dp->attached_connector);
> +		goto put_power;

This has the problem with handling an mst device that is gone which Sivakumar
and I discussed on IRC.

I'm thinking whether it makes sense to move the code in the mst_fail label to 
intel_dp_detect(), since it gets called every time we reach that label. We would
just need a way to tell it about the failure, since we can't rely on connector
status being disconnected (because of MST). That could help us clean up
intel_dp_hpd_pulse even more.
Thoughts?

Ander

>  
> -		if (!intel_dp_probe_mst(intel_dp)) {
> -			drm_modeset_lock(&dev->mode_config.connection_mutex,
> NULL);
> -			intel_dp_check_link_status(intel_dp);
> -			drm_modeset_unlock(&dev
> ->mode_config.connection_mutex);
> -			goto mst_fail;
> -		}
>  	} else {
>  		if (intel_dp->is_mst) {
>  			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Splitting intel_dp_detect

[Ander Conselvan De Oliveira]

On Tue, 2015-11-17 at 11:56 +0530, Shubhangi Shrivastava wrote:
> This patch moves probing for panel, DPCD read etc to another
> function intel_dp_long_pulse, while intel_dp_detect returns
> the status as connected or disconnected depending on
> whether the edid is available or not.

May i suggest a slight rephrase?

"This patch moves probing for panel, DPCD read, etc done in intel_dp_detect() to
a new function: intel_dp_long_pulse(). Note that the behavior of
intel_dp_detect() is changed to report connected or disconnected depending on
whether the edid is available or not."

I think we also need a note why the EDID change is necessary and at the same
doesn't break anything.


> This change will be required by further patches in the series
> to avoid performing multiple DPCD operations and removing
> their duplication.

I found this last part confusing. Maybe replace "avoid multiple DPCD operations
and removing their duplication" with "avoid duplicated DPCD operations on
hotplug", since we are not going into details here.

> 
> v2: Moved a hunk to next patch of the series.
>     Moved intel_dp_unset_edid to out. (Ander)
> 
> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 52 ++++++++++++++++++++++++++++------------
> -
>  1 file changed, 36 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 8071247..dd2b9da 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4631,9 +4631,10 @@ intel_dp_power_put(struct intel_dp *dp,
>  	intel_display_power_put(to_i915(encoder->base.dev), power_domain);
>  }
>  
> -static enum drm_connector_status
> -intel_dp_detect(struct drm_connector *connector, bool force)
> +static void
> +intel_dp_long_pulse(struct intel_connector *intel_connector)
>  {
> +	struct drm_connector *connector = &intel_connector->base;
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>  	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> @@ -4643,17 +4644,6 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  	bool ret;
>  	u8 sink_irq_vector;
>  
> -	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> -		      connector->base.id, connector->name);
> -	intel_dp_unset_edid(intel_dp);
> -
> -	if (intel_dp->is_mst) {
> -		/* MST devices are disconnected from a monitor POV */
> -		if (intel_encoder->type != INTEL_OUTPUT_EDP)
> -			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> -		return connector_status_disconnected;
> -	}
> -
>  	power_domain = intel_dp_power_get(intel_dp);
>  
>  	/* Can't disconnect eDP, but you can close the lid... */
> @@ -4670,8 +4660,10 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  
>  	ret = intel_dp_probe_mst(intel_dp);
>  	if (ret) {
> -		/* if we are in MST mode then this connector
> -		   won't appear connected or have anything with EDID on it */
> +		/*
> +		 * if we are in MST mode then this connector
> +		 * won't appear connected or have anything with EDID on it
> +		 */
>  		if (intel_encoder->type != INTEL_OUTPUT_EDP)
>  			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
>  		status = connector_status_disconnected;
> @@ -4699,8 +4691,36 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  	}
>  
>  out:
> +	if (status != connector_status_connected)
> +		intel_dp_unset_edid(intel_dp);

I didn't notice this at first, but intel_dp_set_edid() doesn't free the previous
edid, so I think we also need to call it just before intel_dp_set_edid(). Patch
looks good otherwise.

Ander

>  	intel_dp_power_put(intel_dp, power_domain);
> -	return status;
> +	return;
> +}
> +
> +static enum drm_connector_status
> +intel_dp_detect(struct drm_connector *connector, bool force)
> +{
> +	struct intel_dp *intel_dp = intel_attached_dp(connector);
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> +	struct intel_connector *intel_connector =
> to_intel_connector(connector);
> +
> +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> +		      connector->base.id, connector->name);
> +
> +	if (intel_dp->is_mst) {
> +		/* MST devices are disconnected from a monitor POV */
> +		if (intel_encoder->type != INTEL_OUTPUT_EDP)
> +			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> +		return connector_status_disconnected;
> +	}
> +
> +	intel_dp_long_pulse(intel_dp->attached_connector);
> +
> +	if (intel_connector->detect_edid)
> +		return connector_status_connected;
> +	else
> +		return connector_status_disconnected;
>  }
>  
>  static void
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/6] drm/i915: Splitting intel_dp_detect

[Shubhangi Shrivastava]

On Friday 15 January 2016 03:37 PM, Ander Conselvan De Oliveira wrote:
> On Thu, 2016-01-14 at 19:20 +0530, Shubhangi Shrivastava wrote:
>> On Wednesday 13 January 2016 07:03 PM, Ander Conselvan De Oliveira wrote:
>>> On Wed, 2016-01-13 at 13:20 +0200, Ander Conselvan De Oliveira wrote:
>>>> On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote:
>>>>> intel_dp_detect() is called for not just detection but
>>>>> during modes enumeration as well. Repeating the whole
>>>>> sequence during each of these calls is wasteful and
>>>>> time consuming.
>>>>> This patch moves probing for panel, DPCD read etc done in
>>>>> intel_dp_detect() to a new function intel_dp_long_pulse().
>>>>> Note that the behavior of intel_dp_detect() is changed to
>>>>> report connected or disconnected depending on whether the
>>>>> EDID is available or not.
>>>>> This change will be required by further patches in the series
>>>>> to avoid performing duplicated DPCD operations on hotplug.
>>>>>
>>>>> v2: Moved a hunk to next patch of the series.
>>>>>       Moved intel_dp_unset_edid to out. (Ander)
>>>>> v3: Rephrased commit message and intel_dp_unset_dp() is called
>>>>>       within intel_dp_set_dp() to free the previous EDID. (Ander)
>>>>>
>>>>> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
>>>>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>>>>> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
>>>>> ---
>>>>>    drivers/gpu/drm/i915/intel_dp.c | 56 +++++++++++++++++++++++++--------
>>>>> -----
>>>>> --
>>>>> -
>>>>>    1 file changed, 35 insertions(+), 21 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>>>> b/drivers/gpu/drm/i915/intel_dp.c
>>>>> index 796e3d3..e3b4208 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>> @@ -129,6 +129,7 @@ static void edp_panel_vdd_off(struct intel_dp
>>>>> *intel_dp,
>>>>> bool sync);
>>>>>    static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp);
>>>>>    static void vlv_steal_power_sequencer(struct drm_device *dev,
>>>>>    				      enum pipe pipe);
>>>>> +static void intel_dp_unset_edid(struct intel_dp *intel_dp);
>>>>>    
>>>>>    static unsigned int intel_dp_unused_lane_mask(int lane_count)
>>>>>    {
>>>>> @@ -4587,6 +4588,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
>>>>>    	struct intel_connector *intel_connector = intel_dp
>>>>> ->attached_connector;
>>>>>    	struct edid *edid;
>>>>>    
>>>>> +	intel_dp_unset_edid(intel_dp);
>>>>>    	edid = intel_dp_get_edid(intel_dp);
>>>>>    	intel_connector->detect_edid = edid;
>>>>>    
>>>>> @@ -4607,9 +4609,10 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
>>>>>    	intel_dp->has_audio = false;
>>>>>    }
>>>>>    
>>>>> -static enum drm_connector_status
>>>>> -intel_dp_detect(struct drm_connector *connector, bool force)
>>>>> +static void
>>>>> +intel_dp_long_pulse(struct intel_connector *intel_connector)
>>>>>    {
>>>>> +	struct drm_connector *connector = &intel_connector->base;
>>>>>    	struct intel_dp *intel_dp = intel_attached_dp(connector);
>>>>>    	struct intel_digital_port *intel_dig_port =
>>>>> dp_to_dig_port(intel_dp);
>>>>>    	struct intel_encoder *intel_encoder = &intel_dig_port->base;
>>>>> @@ -4619,17 +4622,6 @@ intel_dp_detect(struct drm_connector *connector,
>>>>> bool
>>>>> force)
>>>>>    	bool ret;
>>>>>    	u8 sink_irq_vector;
>>>>>    
>>>>> -	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>>>>> -		      connector->base.id, connector->name);
>>>>> -	intel_dp_unset_edid(intel_dp);
>>>>> -
>>>>> -	if (intel_dp->is_mst) {
>>>>> -		/* MST devices are disconnected from a monitor POV */
>>>>> -		if (intel_encoder->type != INTEL_OUTPUT_EDP)
>>>>> -			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
>>>>> -		return connector_status_disconnected;
>>>>> -	}
>>>>> -
>>>>>    	power_domain =
>>>>> intel_display_port_aux_power_domain(intel_encoder);
>>>>>    	intel_display_power_get(to_i915(dev), power_domain);
>>>>>    
>>>>> @@ -4653,14 +4645,8 @@ intel_dp_detect(struct drm_connector *connector,
>>>>> bool
>>>>> force)
>>>>>    	intel_dp_probe_oui(intel_dp);
>>>>>    
>>>>>    	ret = intel_dp_probe_mst(intel_dp);
>>>>> -	if (ret) {
>>>>> -		/* if we are in MST mode then this connector
>>>>> -		   won't appear connected or have anything with EDID on
>>>>> it
>>>>> */
>>>>> -		if (intel_encoder->type != INTEL_OUTPUT_EDP)
>>>>> -			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
>>>> This deletion is new in this version of the patch. I think we still need
>>>> the
>>>> hunk above, otherwise we might not properly update the encoder type when
>>>> we
>>>> switch from an HDMI sink connected through a level shifter to an MST sink.
>>>>
>>>> Ander
>>>>
>> Encoder type setting for MST is being done in intel_dp_detect(). So,
>> don't find a need to add it here.
> Yes, but that one only covers the case where the device was already previously
> identified as MST. For a device identified as MST by the call to
> intel_dp_probe_mst() in intel_dp_long_pulse(), the encoder type override will
> not be done. Hopefully, Ville's patch that splits the encoder types and makes
> this unnecessary will land soon, but for now just leave the override there.
>
> Ander

Alright.. Moved the overriding of encoder type to be done before probing..

>>>>> -		status = connector_status_disconnected;
>>>>> +	if (ret)
>>>>>    		goto out;
>>> Also, there is no call to intel_dp_unset_edid() for this case, since the
>>> code
>>> will reach the label 'out' with status being connected. So in this case the
>>> return value of intel_dp_detect() will depend on the stale value of
>>> intel_dp->detect_edid.
>>>
>>> Ander
>> Yes.. Thats right.. Will add a call to intel_dp_unset_edid() in is_mst()
>> check of intel_dp_detect().

Added call to intel_dp_unset_edid() in is_mst() check of intel_dp_detect().

>>>>> -	}
>>>>>    
>>>>>    	/*
>>>>>    	 * Clearing NACK and defer counts to get their exact values
>>>>> @@ -4691,8 +4677,36 @@ intel_dp_detect(struct drm_connector *connector,
>>>>> bool
>>>>> force)
>>>>>    	}
>>>>>    
>>>>>    out:
>>>>> +	if (status != connector_status_connected)
>>>>> +		intel_dp_unset_edid(intel_dp);
>>>>>    	intel_display_power_put(to_i915(dev), power_domain);
>>>>> -	return status;
>>>>> +	return;
>>>>> +}
>>>>> +
>>>>> +static enum drm_connector_status
>>>>> +intel_dp_detect(struct drm_connector *connector, bool force)
>>>>> +{
>>>>> +	struct intel_dp *intel_dp = intel_attached_dp(connector);
>>>>> +	struct intel_digital_port *intel_dig_port =
>>>>> dp_to_dig_port(intel_dp);
>>>>> +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
>>>>> +	struct intel_connector *intel_connector =
>>>>> to_intel_connector(connector);
>>>>> +
>>>>> +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>>>>> +		     connector->base.id, connector->name);
>>>>> +
>>>>> +	if (intel_dp->is_mst) {
>>>>> +		/* MST devices are disconnected from a monitor POV */
>>>>> +		if (intel_encoder->type != INTEL_OUTPUT_EDP)
>>>>> +			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
>>>>> +		return connector_status_disconnected;
>>>>> +	}
>>>>> +
>>>>> +	intel_dp_long_pulse(intel_dp->attached_connector);
>>>>> +
>>>>> +	if (intel_connector->detect_edid)
>>>>> +		return connector_status_connected;
>>>>> +	else
>>>>> +		return connector_status_disconnected;
>>>>>    }
>>>>>    
>>>>>    static void

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

Re: [PATCH 1/6] drm/i915: Splitting intel_dp_detect

[Ander Conselvan De Oliveira]

On Thu, 2016-01-14 at 19:20 +0530, Shubhangi Shrivastava wrote:
> 
> On Wednesday 13 January 2016 07:03 PM, Ander Conselvan De Oliveira wrote:
> > On Wed, 2016-01-13 at 13:20 +0200, Ander Conselvan De Oliveira wrote:
> > > On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote:
> > > > intel_dp_detect() is called for not just detection but
> > > > during modes enumeration as well. Repeating the whole
> > > > sequence during each of these calls is wasteful and
> > > > time consuming.
> > > > This patch moves probing for panel, DPCD read etc done in
> > > > intel_dp_detect() to a new function intel_dp_long_pulse().
> > > > Note that the behavior of intel_dp_detect() is changed to
> > > > report connected or disconnected depending on whether the
> > > > EDID is available or not.
> > > > This change will be required by further patches in the series
> > > > to avoid performing duplicated DPCD operations on hotplug.
> > > > 
> > > > v2: Moved a hunk to next patch of the series.
> > > >      Moved intel_dp_unset_edid to out. (Ander)
> > > > v3: Rephrased commit message and intel_dp_unset_dp() is called
> > > >      within intel_dp_set_dp() to free the previous EDID. (Ander)
> > > > 
> > > > Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> > > > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> > > > Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/i915/intel_dp.c | 56 +++++++++++++++++++++++++--------
> > > > -----
> > > > --
> > > > -
> > > >   1 file changed, 35 insertions(+), 21 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 796e3d3..e3b4208 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -129,6 +129,7 @@ static void edp_panel_vdd_off(struct intel_dp
> > > > *intel_dp,
> > > > bool sync);
> > > >   static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp);
> > > >   static void vlv_steal_power_sequencer(struct drm_device *dev,
> > > >   				      enum pipe pipe);
> > > > +static void intel_dp_unset_edid(struct intel_dp *intel_dp);
> > > >   
> > > >   static unsigned int intel_dp_unused_lane_mask(int lane_count)
> > > >   {
> > > > @@ -4587,6 +4588,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
> > > >   	struct intel_connector *intel_connector = intel_dp
> > > > ->attached_connector;
> > > >   	struct edid *edid;
> > > >   
> > > > +	intel_dp_unset_edid(intel_dp);
> > > >   	edid = intel_dp_get_edid(intel_dp);
> > > >   	intel_connector->detect_edid = edid;
> > > >   
> > > > @@ -4607,9 +4609,10 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
> > > >   	intel_dp->has_audio = false;
> > > >   }
> > > >   
> > > > -static enum drm_connector_status
> > > > -intel_dp_detect(struct drm_connector *connector, bool force)
> > > > +static void
> > > > +intel_dp_long_pulse(struct intel_connector *intel_connector)
> > > >   {
> > > > +	struct drm_connector *connector = &intel_connector->base;
> > > >   	struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > >   	struct intel_digital_port *intel_dig_port =
> > > > dp_to_dig_port(intel_dp);
> > > >   	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> > > > @@ -4619,17 +4622,6 @@ intel_dp_detect(struct drm_connector *connector,
> > > > bool
> > > > force)
> > > >   	bool ret;
> > > >   	u8 sink_irq_vector;
> > > >   
> > > > -	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > > > -		      connector->base.id, connector->name);
> > > > -	intel_dp_unset_edid(intel_dp);
> > > > -
> > > > -	if (intel_dp->is_mst) {
> > > > -		/* MST devices are disconnected from a monitor POV */
> > > > -		if (intel_encoder->type != INTEL_OUTPUT_EDP)
> > > > -			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> > > > -		return connector_status_disconnected;
> > > > -	}
> > > > -
> > > >   	power_domain =
> > > > intel_display_port_aux_power_domain(intel_encoder);
> > > >   	intel_display_power_get(to_i915(dev), power_domain);
> > > >   
> > > > @@ -4653,14 +4645,8 @@ intel_dp_detect(struct drm_connector *connector,
> > > > bool
> > > > force)
> > > >   	intel_dp_probe_oui(intel_dp);
> > > >   
> > > >   	ret = intel_dp_probe_mst(intel_dp);
> > > > -	if (ret) {
> > > > -		/* if we are in MST mode then this connector
> > > > -		   won't appear connected or have anything with EDID on
> > > > it
> > > > */
> > > > -		if (intel_encoder->type != INTEL_OUTPUT_EDP)
> > > > -			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> > > This deletion is new in this version of the patch. I think we still need
> > > the
> > > hunk above, otherwise we might not properly update the encoder type when
> > > we
> > > switch from an HDMI sink connected through a level shifter to an MST sink.
> > > 
> > > Ander
> > > 
> Encoder type setting for MST is being done in intel_dp_detect(). So, 
> don't find a need to add it here.

Yes, but that one only covers the case where the device was already previously
identified as MST. For a device identified as MST by the call to
intel_dp_probe_mst() in intel_dp_long_pulse(), the encoder type override will
not be done. Hopefully, Ville's patch that splits the encoder types and makes
this unnecessary will land soon, but for now just leave the override there.

Ander

> > > > -		status = connector_status_disconnected;
> > > > +	if (ret)
> > > >   		goto out;
> > Also, there is no call to intel_dp_unset_edid() for this case, since the
> > code
> > will reach the label 'out' with status being connected. So in this case the
> > return value of intel_dp_detect() will depend on the stale value of
> > intel_dp->detect_edid.
> > 
> > Ander
> Yes.. Thats right.. Will add a call to intel_dp_unset_edid() in is_mst() 
> check of intel_dp_detect().
> > 
> > > > -	}
> > > >   
> > > >   	/*
> > > >   	 * Clearing NACK and defer counts to get their exact values
> > > > @@ -4691,8 +4677,36 @@ intel_dp_detect(struct drm_connector *connector,
> > > > bool
> > > > force)
> > > >   	}
> > > >   
> > > >   out:
> > > > +	if (status != connector_status_connected)
> > > > +		intel_dp_unset_edid(intel_dp);
> > > >   	intel_display_power_put(to_i915(dev), power_domain);
> > > > -	return status;
> > > > +	return;
> > > > +}
> > > > +
> > > > +static enum drm_connector_status
> > > > +intel_dp_detect(struct drm_connector *connector, bool force)
> > > > +{
> > > > +	struct intel_dp *intel_dp = intel_attached_dp(connector);
> > > > +	struct intel_digital_port *intel_dig_port =
> > > > dp_to_dig_port(intel_dp);
> > > > +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> > > > +	struct intel_connector *intel_connector =
> > > > to_intel_connector(connector);
> > > > +
> > > > +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > > > +		     connector->base.id, connector->name);
> > > > +
> > > > +	if (intel_dp->is_mst) {
> > > > +		/* MST devices are disconnected from a monitor POV */
> > > > +		if (intel_encoder->type != INTEL_OUTPUT_EDP)
> > > > +			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> > > > +		return connector_status_disconnected;
> > > > +	}
> > > > +
> > > > +	intel_dp_long_pulse(intel_dp->attached_connector);
> > > > +
> > > > +	if (intel_connector->detect_edid)
> > > > +		return connector_status_connected;
> > > > +	else
> > > > +		return connector_status_disconnected;
> > > >   }
> > > >   
> > > >   static void
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/6] drm/i915: Splitting intel_dp_detect

[Shubhangi Shrivastava]

On Wednesday 13 January 2016 07:03 PM, Ander Conselvan De Oliveira wrote:
> On Wed, 2016-01-13 at 13:20 +0200, Ander Conselvan De Oliveira wrote:
>> On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote:
>>> intel_dp_detect() is called for not just detection but
>>> during modes enumeration as well. Repeating the whole
>>> sequence during each of these calls is wasteful and
>>> time consuming.
>>> This patch moves probing for panel, DPCD read etc done in
>>> intel_dp_detect() to a new function intel_dp_long_pulse().
>>> Note that the behavior of intel_dp_detect() is changed to
>>> report connected or disconnected depending on whether the
>>> EDID is available or not.
>>> This change will be required by further patches in the series
>>> to avoid performing duplicated DPCD operations on hotplug.
>>>
>>> v2: Moved a hunk to next patch of the series.
>>>      Moved intel_dp_unset_edid to out. (Ander)
>>> v3: Rephrased commit message and intel_dp_unset_dp() is called
>>>      within intel_dp_set_dp() to free the previous EDID. (Ander)
>>>
>>> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
>>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>>> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_dp.c | 56 +++++++++++++++++++++++++-------------
>>> --
>>> -
>>>   1 file changed, 35 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>> b/drivers/gpu/drm/i915/intel_dp.c
>>> index 796e3d3..e3b4208 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -129,6 +129,7 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp,
>>> bool sync);
>>>   static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp);
>>>   static void vlv_steal_power_sequencer(struct drm_device *dev,
>>>   				      enum pipe pipe);
>>> +static void intel_dp_unset_edid(struct intel_dp *intel_dp);
>>>   
>>>   static unsigned int intel_dp_unused_lane_mask(int lane_count)
>>>   {
>>> @@ -4587,6 +4588,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
>>>   	struct intel_connector *intel_connector = intel_dp
>>> ->attached_connector;
>>>   	struct edid *edid;
>>>   
>>> +	intel_dp_unset_edid(intel_dp);
>>>   	edid = intel_dp_get_edid(intel_dp);
>>>   	intel_connector->detect_edid = edid;
>>>   
>>> @@ -4607,9 +4609,10 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
>>>   	intel_dp->has_audio = false;
>>>   }
>>>   
>>> -static enum drm_connector_status
>>> -intel_dp_detect(struct drm_connector *connector, bool force)
>>> +static void
>>> +intel_dp_long_pulse(struct intel_connector *intel_connector)
>>>   {
>>> +	struct drm_connector *connector = &intel_connector->base;
>>>   	struct intel_dp *intel_dp = intel_attached_dp(connector);
>>>   	struct intel_digital_port *intel_dig_port =
>>> dp_to_dig_port(intel_dp);
>>>   	struct intel_encoder *intel_encoder = &intel_dig_port->base;
>>> @@ -4619,17 +4622,6 @@ intel_dp_detect(struct drm_connector *connector, bool
>>> force)
>>>   	bool ret;
>>>   	u8 sink_irq_vector;
>>>   
>>> -	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>>> -		      connector->base.id, connector->name);
>>> -	intel_dp_unset_edid(intel_dp);
>>> -
>>> -	if (intel_dp->is_mst) {
>>> -		/* MST devices are disconnected from a monitor POV */
>>> -		if (intel_encoder->type != INTEL_OUTPUT_EDP)
>>> -			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
>>> -		return connector_status_disconnected;
>>> -	}
>>> -
>>>   	power_domain = intel_display_port_aux_power_domain(intel_encoder);
>>>   	intel_display_power_get(to_i915(dev), power_domain);
>>>   
>>> @@ -4653,14 +4645,8 @@ intel_dp_detect(struct drm_connector *connector, bool
>>> force)
>>>   	intel_dp_probe_oui(intel_dp);
>>>   
>>>   	ret = intel_dp_probe_mst(intel_dp);
>>> -	if (ret) {
>>> -		/* if we are in MST mode then this connector
>>> -		   won't appear connected or have anything with EDID on it
>>> */
>>> -		if (intel_encoder->type != INTEL_OUTPUT_EDP)
>>> -			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
>> This deletion is new in this version of the patch. I think we still need the
>> hunk above, otherwise we might not properly update the encoder type when we
>> switch from an HDMI sink connected through a level shifter to an MST sink.
>>
>> Ander
>>
Encoder type setting for MST is being done in intel_dp_detect(). So, 
don't find a need to add it here.
>>> -		status = connector_status_disconnected;
>>> +	if (ret)
>>>   		goto out;
> Also, there is no call to intel_dp_unset_edid() for this case, since the code
> will reach the label 'out' with status being connected. So in this case the
> return value of intel_dp_detect() will depend on the stale value of
> intel_dp->detect_edid.
>
> Ander
Yes.. Thats right.. Will add a call to intel_dp_unset_edid() in is_mst() 
check of intel_dp_detect().
>
>>> -	}
>>>   
>>>   	/*
>>>   	 * Clearing NACK and defer counts to get their exact values
>>> @@ -4691,8 +4677,36 @@ intel_dp_detect(struct drm_connector *connector, bool
>>> force)
>>>   	}
>>>   
>>>   out:
>>> +	if (status != connector_status_connected)
>>> +		intel_dp_unset_edid(intel_dp);
>>>   	intel_display_power_put(to_i915(dev), power_domain);
>>> -	return status;
>>> +	return;
>>> +}
>>> +
>>> +static enum drm_connector_status
>>> +intel_dp_detect(struct drm_connector *connector, bool force)
>>> +{
>>> +	struct intel_dp *intel_dp = intel_attached_dp(connector);
>>> +	struct intel_digital_port *intel_dig_port =
>>> dp_to_dig_port(intel_dp);
>>> +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
>>> +	struct intel_connector *intel_connector =
>>> to_intel_connector(connector);
>>> +
>>> +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
>>> +		     connector->base.id, connector->name);
>>> +
>>> +	if (intel_dp->is_mst) {
>>> +		/* MST devices are disconnected from a monitor POV */
>>> +		if (intel_encoder->type != INTEL_OUTPUT_EDP)
>>> +			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
>>> +		return connector_status_disconnected;
>>> +	}
>>> +
>>> +	intel_dp_long_pulse(intel_dp->attached_connector);
>>> +
>>> +	if (intel_connector->detect_edid)
>>> +		return connector_status_connected;
>>> +	else
>>> +		return connector_status_disconnected;
>>>   }
>>>   
>>>   static void

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

Re: [PATCH 1/6] drm/i915: Splitting intel_dp_detect

[Ander Conselvan De Oliveira]

On Wed, 2016-01-13 at 13:20 +0200, Ander Conselvan De Oliveira wrote:
> On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote:
> > intel_dp_detect() is called for not just detection but
> > during modes enumeration as well. Repeating the whole
> > sequence during each of these calls is wasteful and
> > time consuming.
> > This patch moves probing for panel, DPCD read etc done in
> > intel_dp_detect() to a new function intel_dp_long_pulse().
> > Note that the behavior of intel_dp_detect() is changed to
> > report connected or disconnected depending on whether the
> > EDID is available or not.
> > This change will be required by further patches in the series
> > to avoid performing duplicated DPCD operations on hotplug.
> > 
> > v2: Moved a hunk to next patch of the series.
> >     Moved intel_dp_unset_edid to out. (Ander)
> > v3: Rephrased commit message and intel_dp_unset_dp() is called
> >     within intel_dp_set_dp() to free the previous EDID. (Ander)
> > 
> > Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> > Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 56 +++++++++++++++++++++++++-------------
> > --
> > -
> >  1 file changed, 35 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 796e3d3..e3b4208 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -129,6 +129,7 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp,
> > bool sync);
> >  static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp);
> >  static void vlv_steal_power_sequencer(struct drm_device *dev,
> >  				      enum pipe pipe);
> > +static void intel_dp_unset_edid(struct intel_dp *intel_dp);
> >  
> >  static unsigned int intel_dp_unused_lane_mask(int lane_count)
> >  {
> > @@ -4587,6 +4588,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
> >  	struct intel_connector *intel_connector = intel_dp
> > ->attached_connector;
> >  	struct edid *edid;
> >  
> > +	intel_dp_unset_edid(intel_dp);
> >  	edid = intel_dp_get_edid(intel_dp);
> >  	intel_connector->detect_edid = edid;
> >  
> > @@ -4607,9 +4609,10 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
> >  	intel_dp->has_audio = false;
> >  }
> >  
> > -static enum drm_connector_status
> > -intel_dp_detect(struct drm_connector *connector, bool force)
> > +static void
> > +intel_dp_long_pulse(struct intel_connector *intel_connector)
> >  {
> > +	struct drm_connector *connector = &intel_connector->base;
> >  	struct intel_dp *intel_dp = intel_attached_dp(connector);
> >  	struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> >  	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> > @@ -4619,17 +4622,6 @@ intel_dp_detect(struct drm_connector *connector, bool
> > force)
> >  	bool ret;
> >  	u8 sink_irq_vector;
> >  
> > -	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > -		      connector->base.id, connector->name);
> > -	intel_dp_unset_edid(intel_dp);
> > -
> > -	if (intel_dp->is_mst) {
> > -		/* MST devices are disconnected from a monitor POV */
> > -		if (intel_encoder->type != INTEL_OUTPUT_EDP)
> > -			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> > -		return connector_status_disconnected;
> > -	}
> > -
> >  	power_domain = intel_display_port_aux_power_domain(intel_encoder);
> >  	intel_display_power_get(to_i915(dev), power_domain);
> >  
> > @@ -4653,14 +4645,8 @@ intel_dp_detect(struct drm_connector *connector, bool
> > force)
> >  	intel_dp_probe_oui(intel_dp);
> >  
> >  	ret = intel_dp_probe_mst(intel_dp);
> > -	if (ret) {
> > -		/* if we are in MST mode then this connector
> > -		   won't appear connected or have anything with EDID on it
> > */
> > -		if (intel_encoder->type != INTEL_OUTPUT_EDP)
> > -			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> 
> This deletion is new in this version of the patch. I think we still need the
> hunk above, otherwise we might not properly update the encoder type when we
> switch from an HDMI sink connected through a level shifter to an MST sink.
> 
> Ander
> 
> 
> > -		status = connector_status_disconnected;
> > +	if (ret)
> >  		goto out;

Also, there is no call to intel_dp_unset_edid() for this case, since the code
will reach the label 'out' with status being connected. So in this case the
return value of intel_dp_detect() will depend on the stale value of
intel_dp->detect_edid.

Ander

> > -	}
> >  
> >  	/*
> >  	 * Clearing NACK and defer counts to get their exact values
> > @@ -4691,8 +4677,36 @@ intel_dp_detect(struct drm_connector *connector, bool
> > force)
> >  	}
> >  
> >  out:
> > +	if (status != connector_status_connected)
> > +		intel_dp_unset_edid(intel_dp);
> >  	intel_display_power_put(to_i915(dev), power_domain);
> > -	return status;
> > +	return;
> > +}
> > +
> > +static enum drm_connector_status
> > +intel_dp_detect(struct drm_connector *connector, bool force)
> > +{
> > +	struct intel_dp *intel_dp = intel_attached_dp(connector);
> > +	struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> > +	struct intel_connector *intel_connector =
> > to_intel_connector(connector);
> > +
> > +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> > +		     connector->base.id, connector->name);
> > +
> > +	if (intel_dp->is_mst) {
> > +		/* MST devices are disconnected from a monitor POV */
> > +		if (intel_encoder->type != INTEL_OUTPUT_EDP)
> > +			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> > +		return connector_status_disconnected;
> > +	}
> > +
> > +	intel_dp_long_pulse(intel_dp->attached_connector);
> > +
> > +	if (intel_connector->detect_edid)
> > +		return connector_status_connected;
> > +	else
> > +		return connector_status_disconnected;
> >  }
> >  
> >  static void
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/6] drm/i915: Splitting intel_dp_detect

[Ander Conselvan De Oliveira]

On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote:
> intel_dp_detect() is called for not just detection but
> during modes enumeration as well. Repeating the whole
> sequence during each of these calls is wasteful and
> time consuming.
> This patch moves probing for panel, DPCD read etc done in
> intel_dp_detect() to a new function intel_dp_long_pulse().
> Note that the behavior of intel_dp_detect() is changed to
> report connected or disconnected depending on whether the
> EDID is available or not.
> This change will be required by further patches in the series
> to avoid performing duplicated DPCD operations on hotplug.
> 
> v2: Moved a hunk to next patch of the series.
>     Moved intel_dp_unset_edid to out. (Ander)
> v3: Rephrased commit message and intel_dp_unset_dp() is called
>     within intel_dp_set_dp() to free the previous EDID. (Ander)
> 
> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 56 +++++++++++++++++++++++++---------------
> -
>  1 file changed, 35 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 796e3d3..e3b4208 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -129,6 +129,7 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp,
> bool sync);
>  static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp);
>  static void vlv_steal_power_sequencer(struct drm_device *dev,
>  				      enum pipe pipe);
> +static void intel_dp_unset_edid(struct intel_dp *intel_dp);
>  
>  static unsigned int intel_dp_unused_lane_mask(int lane_count)
>  {
> @@ -4587,6 +4588,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
>  	struct intel_connector *intel_connector = intel_dp
> ->attached_connector;
>  	struct edid *edid;
>  
> +	intel_dp_unset_edid(intel_dp);
>  	edid = intel_dp_get_edid(intel_dp);
>  	intel_connector->detect_edid = edid;
>  
> @@ -4607,9 +4609,10 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
>  	intel_dp->has_audio = false;
>  }
>  
> -static enum drm_connector_status
> -intel_dp_detect(struct drm_connector *connector, bool force)
> +static void
> +intel_dp_long_pulse(struct intel_connector *intel_connector)
>  {
> +	struct drm_connector *connector = &intel_connector->base;
>  	struct intel_dp *intel_dp = intel_attached_dp(connector);
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>  	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> @@ -4619,17 +4622,6 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  	bool ret;
>  	u8 sink_irq_vector;
>  
> -	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> -		      connector->base.id, connector->name);
> -	intel_dp_unset_edid(intel_dp);
> -
> -	if (intel_dp->is_mst) {
> -		/* MST devices are disconnected from a monitor POV */
> -		if (intel_encoder->type != INTEL_OUTPUT_EDP)
> -			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> -		return connector_status_disconnected;
> -	}
> -
>  	power_domain = intel_display_port_aux_power_domain(intel_encoder);
>  	intel_display_power_get(to_i915(dev), power_domain);
>  
> @@ -4653,14 +4645,8 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  	intel_dp_probe_oui(intel_dp);
>  
>  	ret = intel_dp_probe_mst(intel_dp);
> -	if (ret) {
> -		/* if we are in MST mode then this connector
> -		   won't appear connected or have anything with EDID on it */
> -		if (intel_encoder->type != INTEL_OUTPUT_EDP)
> -			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;

This deletion is new in this version of the patch. I think we still need the
hunk above, otherwise we might not properly update the encoder type when we
switch from an HDMI sink connected through a level shifter to an MST sink.

Ander


> -		status = connector_status_disconnected;
> +	if (ret)
>  		goto out;
> -	}
>  
>  	/*
>  	 * Clearing NACK and defer counts to get their exact values
> @@ -4691,8 +4677,36 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  	}
>  
>  out:
> +	if (status != connector_status_connected)
> +		intel_dp_unset_edid(intel_dp);
>  	intel_display_power_put(to_i915(dev), power_domain);
> -	return status;
> +	return;
> +}
> +
> +static enum drm_connector_status
> +intel_dp_detect(struct drm_connector *connector, bool force)
> +{
> +	struct intel_dp *intel_dp = intel_attached_dp(connector);
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct intel_encoder *intel_encoder = &intel_dig_port->base;
> +	struct intel_connector *intel_connector =
> to_intel_connector(connector);
> +
> +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> +		     connector->base.id, connector->name);
> +
> +	if (intel_dp->is_mst) {
> +		/* MST devices are disconnected from a monitor POV */
> +		if (intel_encoder->type != INTEL_OUTPUT_EDP)
> +			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> +		return connector_status_disconnected;
> +	}
> +
> +	intel_dp_long_pulse(intel_dp->attached_connector);
> +
> +	if (intel_connector->detect_edid)
> +		return connector_status_connected;
> +	else
> +		return connector_status_disconnected;
>  }
>  
>  static void
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/6] drm/i915: Splitting intel_dp_detect

[Shubhangi Shrivastava]

intel_dp_detect() is called for not just detection but
during modes enumeration as well. Repeating the whole
sequence during each of these calls is wasteful and
time consuming.
This patch moves probing for panel, DPCD read etc done in
intel_dp_detect() to a new function intel_dp_long_pulse().
Note that the behavior of intel_dp_detect() is changed to
report connected or disconnected depending on whether the
EDID is available or not.
This change will be required by further patches in the series
to avoid performing duplicated DPCD operations on hotplug.

v2: Moved a hunk to next patch of the series.
    Moved intel_dp_unset_edid to out. (Ander)
v3: Rephrased commit message and intel_dp_unset_dp() is called
    within intel_dp_set_dp() to free the previous EDID. (Ander)

Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 56 +++++++++++++++++++++++++----------------
 1 file changed, 35 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 796e3d3..e3b4208 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -129,6 +129,7 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
 static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp);
 static void vlv_steal_power_sequencer(struct drm_device *dev,
 				      enum pipe pipe);
+static void intel_dp_unset_edid(struct intel_dp *intel_dp);
 
 static unsigned int intel_dp_unused_lane_mask(int lane_count)
 {
@@ -4587,6 +4588,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
 	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	struct edid *edid;
 
+	intel_dp_unset_edid(intel_dp);
 	edid = intel_dp_get_edid(intel_dp);
 	intel_connector->detect_edid = edid;
 
@@ -4607,9 +4609,10 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
 	intel_dp->has_audio = false;
 }
 
-static enum drm_connector_status
-intel_dp_detect(struct drm_connector *connector, bool force)
+static void
+intel_dp_long_pulse(struct intel_connector *intel_connector)
 {
+	struct drm_connector *connector = &intel_connector->base;
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
@@ -4619,17 +4622,6 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	bool ret;
 	u8 sink_irq_vector;
 
-	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
-		      connector->base.id, connector->name);
-	intel_dp_unset_edid(intel_dp);
-
-	if (intel_dp->is_mst) {
-		/* MST devices are disconnected from a monitor POV */
-		if (intel_encoder->type != INTEL_OUTPUT_EDP)
-			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
-		return connector_status_disconnected;
-	}
-
 	power_domain = intel_display_port_aux_power_domain(intel_encoder);
 	intel_display_power_get(to_i915(dev), power_domain);
 
@@ -4653,14 +4645,8 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	intel_dp_probe_oui(intel_dp);
 
 	ret = intel_dp_probe_mst(intel_dp);
-	if (ret) {
-		/* if we are in MST mode then this connector
-		   won't appear connected or have anything with EDID on it */
-		if (intel_encoder->type != INTEL_OUTPUT_EDP)
-			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
-		status = connector_status_disconnected;
+	if (ret)
 		goto out;
-	}
 
 	/*
 	 * Clearing NACK and defer counts to get their exact values
@@ -4691,8 +4677,36 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	}
 
 out:
+	if (status != connector_status_connected)
+		intel_dp_unset_edid(intel_dp);
 	intel_display_power_put(to_i915(dev), power_domain);
-	return status;
+	return;
+}
+
+static enum drm_connector_status
+intel_dp_detect(struct drm_connector *connector, bool force)
+{
+	struct intel_dp *intel_dp = intel_attached_dp(connector);
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct intel_encoder *intel_encoder = &intel_dig_port->base;
+	struct intel_connector *intel_connector = to_intel_connector(connector);
+
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
+		     connector->base.id, connector->name);
+
+	if (intel_dp->is_mst) {
+		/* MST devices are disconnected from a monitor POV */
+		if (intel_encoder->type != INTEL_OUTPUT_EDP)
+			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
+		return connector_status_disconnected;
+	}
+
+	intel_dp_long_pulse(intel_dp->attached_connector);
+
+	if (intel_connector->detect_edid)
+		return connector_status_connected;
+	else
+		return connector_status_disconnected;
 }
 
 static void
-- 
2.6.1

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

Re: [PATCH 1/6] drm/i915: Splitting intel_dp_detect

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2151 bytes --]

Hi Shubhangi,

[auto build test WARNING on drm-intel/for-linux-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]

url:    https://github.com/0day-ci/linux/commits/Shubhangi-Shrivastava/Fixing-sink-count-related-detection-over/20151102-205435
config: i386-randconfig-x003-11010709 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/i915/intel_dp.c: In function 'intel_dp_detect':
>> drivers/gpu/drm/i915/intel_dp.c:4883:22: warning: passing argument 1 of 'intel_dp_long_pulse' from incompatible pointer type [-Wincompatible-pointer-types]
     intel_dp_long_pulse(intel_dp->attached_connector);
                         ^
   drivers/gpu/drm/i915/intel_dp.c:4789:1: note: expected 'struct drm_connector *' but argument is of type 'struct intel_connector *'
    intel_dp_long_pulse(struct drm_connector *connector)
    ^

vim +/intel_dp_long_pulse +4883 drivers/gpu/drm/i915/intel_dp.c

  4867	{
  4868		struct intel_dp *intel_dp = intel_attached_dp(connector);
  4869		struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
  4870		struct intel_encoder *intel_encoder = &intel_dig_port->base;
  4871		struct intel_connector *intel_connector = to_intel_connector(connector);
  4872	
  4873		DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
  4874			      connector->base.id, connector->name);
  4875	
  4876		if (intel_dp->is_mst) {
  4877			/* MST devices are disconnected from a monitor POV */
  4878			if (intel_encoder->type != INTEL_OUTPUT_EDP)
  4879				intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
  4880			return connector_status_disconnected;
  4881		}
  4882	
> 4883		intel_dp_long_pulse(intel_dp->attached_connector);
  4884	
  4885		if (intel_connector->detect_edid)
  4886			return connector_status_connected;
  4887		else
  4888			return connector_status_disconnected;
  4889	}
  4890	
  4891	static void

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 28234 bytes --]

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

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

[PATCH 1/6] drm/i915: Splitting intel_dp_detect

[Shubhangi Shrivastava]

This patch moves probing for panel, DPCD read etc to another
function intel_dp_long_pulse, while intel_dp_detect returns
the status as connected or disconnected depending on
whether the edid is available or not.
This change will be required by further patches in the series
to avoid performing multiple DPCD operations and removing
their duplication.

Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 65 ++++++++++++++++++++++++++++++-----------
 1 file changed, 48 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1cb1f3f..1677f7c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4785,8 +4785,8 @@ intel_dp_power_put(struct intel_dp *dp,
 	intel_display_power_put(to_i915(encoder->base.dev), power_domain);
 }
 
-static enum drm_connector_status
-intel_dp_detect(struct drm_connector *connector, bool force)
+static void
+intel_dp_long_pulse(struct drm_connector *connector)
 {
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
@@ -4797,17 +4797,6 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	bool ret;
 	u8 sink_irq_vector;
 
-	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
-		      connector->base.id, connector->name);
-	intel_dp_unset_edid(intel_dp);
-
-	if (intel_dp->is_mst) {
-		/* MST devices are disconnected from a monitor POV */
-		if (intel_encoder->type != INTEL_OUTPUT_EDP)
-			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
-		return connector_status_disconnected;
-	}
-
 	power_domain = intel_dp_power_get(intel_dp);
 
 	/* Can't disconnect eDP, but you can close the lid... */
@@ -4817,19 +4806,35 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 		status = ironlake_dp_detect(intel_dp);
 	else
 		status = g4x_dp_detect(intel_dp);
-	if (status != connector_status_connected)
+	if (status != connector_status_connected) {
+		intel_dp_unset_edid(intel_dp);
 		goto out;
+	}
 
 	intel_dp_probe_oui(intel_dp);
 
 	ret = intel_dp_probe_mst(intel_dp);
 	if (ret) {
-		/* if we are in MST mode then this connector
-		   won't appear connected or have anything with EDID on it */
+		/*
+		 * if we are in MST mode then this connector
+		 * won't appear connected or have anything with
+		 * EDID on it
+		 */
 		if (intel_encoder->type != INTEL_OUTPUT_EDP)
 			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
 		status = connector_status_disconnected;
 		goto out;
+	} else if (connector->status == connector_status_connected) {
+
+		/*
+		 * If display was connected already and is still connected
+		 * check links status, there has been known issues of
+		 * link loss triggerring long pulse!!!!
+		 */
+		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+		intel_dp_check_link_status(intel_dp);
+		drm_modeset_unlock(&dev->mode_config.connection_mutex);
+		goto out;
 	}
 
 	intel_dp_set_edid(intel_dp);
@@ -4854,7 +4859,33 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 
 out:
 	intel_dp_power_put(intel_dp, power_domain);
-	return status;
+	return;
+}
+
+static enum drm_connector_status
+intel_dp_detect(struct drm_connector *connector, bool force)
+{
+	struct intel_dp *intel_dp = intel_attached_dp(connector);
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct intel_encoder *intel_encoder = &intel_dig_port->base;
+	struct intel_connector *intel_connector = to_intel_connector(connector);
+
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
+		      connector->base.id, connector->name);
+
+	if (intel_dp->is_mst) {
+		/* MST devices are disconnected from a monitor POV */
+		if (intel_encoder->type != INTEL_OUTPUT_EDP)
+			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
+		return connector_status_disconnected;
+	}
+
+	intel_dp_long_pulse(intel_dp->attached_connector);
+
+	if (intel_connector->detect_edid)
+		return connector_status_connected;
+	else
+		return connector_status_disconnected;
 }
 
 static void
-- 
2.6.1

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