[PATCH 1/2] drm/i915: Read sink_count dpcd always for short hpd

[PATCH 1/2] drm/i915: Read sink_count dpcd always for short hpd

[Sivakumar Thulasimani]

From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>

Compliance test 4.2.2.8 requires driver to read the sink_count for
short pulse interrupt even when the panel is not enabled.
This patch performs the following
a) reading sink_count by reusing intel_dp_detect_dpcd
instead of using intel_dp_get_dpcd
b) moving crtc enabled checks post sink_count read call

Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |  117 ++++++++++++++++++++-------------------
 1 file changed, 59 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b905c19..e4de8e5 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4342,6 +4342,56 @@ go_again:
 	return -EINVAL;
 }
 
+/* XXX this is probably wrong for multiple downstream ports */
+static enum drm_connector_status
+intel_dp_detect_dpcd(struct intel_dp *intel_dp)
+{
+	uint8_t *dpcd = intel_dp->dpcd;
+	uint8_t type;
+
+	if (!intel_dp_get_dpcd(intel_dp))
+		return connector_status_disconnected;
+
+	/* if there's no downstream port, we're done */
+	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
+		return connector_status_connected;
+
+	/* 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)
+			return connector_status_unknown;
+
+		return DP_GET_SINK_COUNT(reg) ? connector_status_connected
+					      : connector_status_disconnected;
+	}
+
+	/* If no HPD, poke DDC gently */
+	if (drm_probe_ddc(&intel_dp->aux.ddc))
+		return connector_status_connected;
+
+	/* Well we tried, say unknown for unreliable port types */
+	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11) {
+		type = intel_dp->downstream_ports[0] & DP_DS_PORT_TYPE_MASK;
+		if (type == DP_DS_PORT_TYPE_VGA ||
+		    type == DP_DS_PORT_TYPE_NON_EDID)
+			return connector_status_unknown;
+	} else {
+		type = intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
+			DP_DWN_STRM_PORT_TYPE_MASK;
+		if (type == DP_DWN_STRM_PORT_TYPE_ANALOG ||
+		    type == DP_DWN_STRM_PORT_TYPE_OTHER)
+			return connector_status_unknown;
+	}
+
+	/* Anything else is out of spec, warn and ignore */
+	DRM_DEBUG_KMS("Broken DP branch device, ignoring\n");
+	return connector_status_disconnected;
+}
+
 /*
  * According to DP spec
  * 5.1.2:
@@ -4362,21 +4412,22 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 
 	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
-	if (!intel_encoder->base.crtc)
+	/* 4.2.2.8 requires source to read link_status, 0 - 12 DPCD &
+	 * sink_count even for short pulse irrespective of the sink is
+	 * in use or not
+	 */
+	if (!intel_dp_get_link_status(intel_dp, link_status))
 		return;
 
-	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
+	/* reuse to read both 0 - 12 DPCD & sink_count */
+	if (intel_dp_detect_dpcd(intel_dp) != connector_status_connected)
 		return;
 
-	/* Try to read receiver status if the link appears to be up */
-	if (!intel_dp_get_link_status(intel_dp, link_status)) {
+	if (!intel_encoder->base.crtc)
 		return;
-	}
 
-	/* Now read the DPCD to see if it's actually running */
-	if (!intel_dp_get_dpcd(intel_dp)) {
+	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
 		return;
-	}
 
 	/* Try to read the source of the interrupt */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
@@ -4401,56 +4452,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 	}
 }
 
-/* XXX this is probably wrong for multiple downstream ports */
-static enum drm_connector_status
-intel_dp_detect_dpcd(struct intel_dp *intel_dp)
-{
-	uint8_t *dpcd = intel_dp->dpcd;
-	uint8_t type;
-
-	if (!intel_dp_get_dpcd(intel_dp))
-		return connector_status_disconnected;
-
-	/* if there's no downstream port, we're done */
-	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
-		return connector_status_connected;
-
-	/* 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)
-			return connector_status_unknown;
-
-		return DP_GET_SINK_COUNT(reg) ? connector_status_connected
-					      : connector_status_disconnected;
-	}
-
-	/* If no HPD, poke DDC gently */
-	if (drm_probe_ddc(&intel_dp->aux.ddc))
-		return connector_status_connected;
-
-	/* Well we tried, say unknown for unreliable port types */
-	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11) {
-		type = intel_dp->downstream_ports[0] & DP_DS_PORT_TYPE_MASK;
-		if (type == DP_DS_PORT_TYPE_VGA ||
-		    type == DP_DS_PORT_TYPE_NON_EDID)
-			return connector_status_unknown;
-	} else {
-		type = intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
-			DP_DWN_STRM_PORT_TYPE_MASK;
-		if (type == DP_DWN_STRM_PORT_TYPE_ANALOG ||
-		    type == DP_DWN_STRM_PORT_TYPE_OTHER)
-			return connector_status_unknown;
-	}
-
-	/* Anything else is out of spec, warn and ignore */
-	DRM_DEBUG_KMS("Broken DP branch device, ignoring\n");
-	return connector_status_disconnected;
-}
-
 static enum drm_connector_status
 edp_detect(struct intel_dp *intel_dp)
 {
-- 
1.7.9.5

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

Re: [PATCH 1/2] drm/i915: Read sink_count dpcd always for short hpd

[Jani Nikula]

On Mon, 17 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
>
> Compliance test 4.2.2.8 requires driver to read the sink_count for
> short pulse interrupt even when the panel is not enabled.
> This patch performs the following
> a) reading sink_count by reusing intel_dp_detect_dpcd
> instead of using intel_dp_get_dpcd
> b) moving crtc enabled checks post sink_count read call
>
> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |  117 ++++++++++++++++++++-------------------
>  1 file changed, 59 insertions(+), 58 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b905c19..e4de8e5 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4342,6 +4342,56 @@ go_again:
>  	return -EINVAL;
>  }
>  
> +/* XXX this is probably wrong for multiple downstream ports */
> +static enum drm_connector_status
> +intel_dp_detect_dpcd(struct intel_dp *intel_dp)
> +{
> +	uint8_t *dpcd = intel_dp->dpcd;
> +	uint8_t type;
> +
> +	if (!intel_dp_get_dpcd(intel_dp))
> +		return connector_status_disconnected;
> +
> +	/* if there's no downstream port, we're done */
> +	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
> +		return connector_status_connected;
> +
> +	/* 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)
> +			return connector_status_unknown;
> +
> +		return DP_GET_SINK_COUNT(reg) ? connector_status_connected
> +					      : connector_status_disconnected;
> +	}
> +
> +	/* If no HPD, poke DDC gently */
> +	if (drm_probe_ddc(&intel_dp->aux.ddc))
> +		return connector_status_connected;
> +
> +	/* Well we tried, say unknown for unreliable port types */
> +	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11) {
> +		type = intel_dp->downstream_ports[0] & DP_DS_PORT_TYPE_MASK;
> +		if (type == DP_DS_PORT_TYPE_VGA ||
> +		    type == DP_DS_PORT_TYPE_NON_EDID)
> +			return connector_status_unknown;
> +	} else {
> +		type = intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> +			DP_DWN_STRM_PORT_TYPE_MASK;
> +		if (type == DP_DWN_STRM_PORT_TYPE_ANALOG ||
> +		    type == DP_DWN_STRM_PORT_TYPE_OTHER)
> +			return connector_status_unknown;
> +	}
> +
> +	/* Anything else is out of spec, warn and ignore */
> +	DRM_DEBUG_KMS("Broken DP branch device, ignoring\n");
> +	return connector_status_disconnected;
> +}

Please either a) just add a forward declaration for
intel_dp_detect_dpcd, or b) add a separate non-functional prep patch
that moves the function around. Please don't combine code movement with
functional changes.

BR,
Jani.


> +
>  /*
>   * According to DP spec
>   * 5.1.2:
> @@ -4362,21 +4412,22 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  
>  	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>  
> -	if (!intel_encoder->base.crtc)
> +	/* 4.2.2.8 requires source to read link_status, 0 - 12 DPCD &
> +	 * sink_count even for short pulse irrespective of the sink is
> +	 * in use or not
> +	 */
> +	if (!intel_dp_get_link_status(intel_dp, link_status))
>  		return;
>  
> -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> +	/* reuse to read both 0 - 12 DPCD & sink_count */
> +	if (intel_dp_detect_dpcd(intel_dp) != connector_status_connected)
>  		return;
>  
> -	/* Try to read receiver status if the link appears to be up */
> -	if (!intel_dp_get_link_status(intel_dp, link_status)) {
> +	if (!intel_encoder->base.crtc)
>  		return;
> -	}
>  
> -	/* Now read the DPCD to see if it's actually running */
> -	if (!intel_dp_get_dpcd(intel_dp)) {
> +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>  		return;
> -	}
>  
>  	/* Try to read the source of the interrupt */
>  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> @@ -4401,56 +4452,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  	}
>  }
>  
> -/* XXX this is probably wrong for multiple downstream ports */
> -static enum drm_connector_status
> -intel_dp_detect_dpcd(struct intel_dp *intel_dp)
> -{
> -	uint8_t *dpcd = intel_dp->dpcd;
> -	uint8_t type;
> -
> -	if (!intel_dp_get_dpcd(intel_dp))
> -		return connector_status_disconnected;
> -
> -	/* if there's no downstream port, we're done */
> -	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
> -		return connector_status_connected;
> -
> -	/* 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)
> -			return connector_status_unknown;
> -
> -		return DP_GET_SINK_COUNT(reg) ? connector_status_connected
> -					      : connector_status_disconnected;
> -	}
> -
> -	/* If no HPD, poke DDC gently */
> -	if (drm_probe_ddc(&intel_dp->aux.ddc))
> -		return connector_status_connected;
> -
> -	/* Well we tried, say unknown for unreliable port types */
> -	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11) {
> -		type = intel_dp->downstream_ports[0] & DP_DS_PORT_TYPE_MASK;
> -		if (type == DP_DS_PORT_TYPE_VGA ||
> -		    type == DP_DS_PORT_TYPE_NON_EDID)
> -			return connector_status_unknown;
> -	} else {
> -		type = intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
> -			DP_DWN_STRM_PORT_TYPE_MASK;
> -		if (type == DP_DWN_STRM_PORT_TYPE_ANALOG ||
> -		    type == DP_DWN_STRM_PORT_TYPE_OTHER)
> -			return connector_status_unknown;
> -	}
> -
> -	/* Anything else is out of spec, warn and ignore */
> -	DRM_DEBUG_KMS("Broken DP branch device, ignoring\n");
> -	return connector_status_disconnected;
> -}
> -
>  static enum drm_connector_status
>  edp_detect(struct intel_dp *intel_dp)
>  {
> -- 
> 1.7.9.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 0/2] Detect DP displays based on sink count change

[Sivakumar Thulasimani]

From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>

These two patches together help detect DP displays on short pulse HPD
and pass the respective compliance test case (4.2.2.8)

Thulasimani,Sivakumar (2):
  drm/i915: Read sink_count dpcd always for short hpd
  drm/i915: Perform full detect on sink_count change

 drivers/gpu/drm/i915/intel_dp.c  |   43 ++++++++++++++++++++++++++------------
 drivers/gpu/drm/i915/intel_drv.h |    1 +
 2 files changed, 31 insertions(+), 13 deletions(-)

-- 
1.7.9.5

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

[PATCH 1/2] drm/i915: Read sink_count dpcd always for short hpd

[Sivakumar Thulasimani]

From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>

Compliance test 4.2.2.8 requires driver to read the sink_count for
short pulse interrupt even when the panel is not enabled.
This patch performs the following
a) reading sink_count by reusing intel_dp_detect_dpcd
instead of using intel_dp_get_dpcd
b) moving crtc enabled checks post sink_count read call

v2: avoid code movement with functionality changes (Ville)

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b905c19..0b73e98 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -132,6 +132,8 @@ 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 enum drm_connector_status
+intel_dp_detect_dpcd(struct intel_dp *intel_dp);
 
 static int
 intel_dp_max_link_bw(struct intel_dp  *intel_dp)
@@ -4362,21 +4364,23 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 
 	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
-	if (!intel_encoder->base.crtc)
+	/* 4.2.2.8 requires source to read link_status, 0 - 12 DPCD &
+	 * sink_count even for short pulse irrespective of the sink is
+	 * in use or not
+	 */
+	if (!intel_dp_get_link_status(intel_dp, link_status)) {
 		return;
+	}
 
-	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
+	/* reuse to read both 0 - 12 DPCD & sink_count */
+	if (intel_dp_detect_dpcd(intel_dp) != connector_status_connected)
 		return;
 
-	/* Try to read receiver status if the link appears to be up */
-	if (!intel_dp_get_link_status(intel_dp, link_status)) {
+	if (!intel_encoder->base.crtc)
 		return;
-	}
 
-	/* Now read the DPCD to see if it's actually running */
-	if (!intel_dp_get_dpcd(intel_dp)) {
+	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
 		return;
-	}
 
 	/* Try to read the source of the interrupt */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
-- 
1.7.9.5

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

[PATCH 2/2] drm/i915: Perform full detect on sink_count change

[Sivakumar Thulasimani]

From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>

This patch checks for changes in sink_count during short pulse hpd
in check_link_status and forces full detect when sink_count
changes. Compliance test 4.2.2.8 expects this behavior in
compliant driver.

Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  |   25 +++++++++++++++++++------
 drivers/gpu/drm/i915/intel_drv.h |    1 +
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 0b73e98..067f9ee 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4353,17 +4353,20 @@ 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_check_link_status(struct intel_dp *intel_dp, bool *need_full_detect)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
 	struct intel_crtc *crtc =
 		to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc);
 	u8 sink_irq_vector;
+	u8 old_sink_count = intel_dp->sink_count;
 	u8 link_status[DP_LINK_STATUS_SIZE];
 
 	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
+	*need_full_detect = false;
+
 	/* 4.2.2.8 requires source to read link_status, 0 - 12 DPCD &
 	 * sink_count even for short pulse irrespective of the sink is
 	 * in use or not
@@ -4376,6 +4379,12 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 	if (intel_dp_detect_dpcd(intel_dp) != connector_status_connected)
 		return;
 
+	if (old_sink_count != intel_dp->sink_count) {
+		DRM_ERROR("forcing full detect\n");
+		*need_full_detect = true;
+		return;
+	}
+
 	if (!intel_encoder->base.crtc)
 		return;
 
@@ -4422,14 +4431,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 */
@@ -5029,13 +5037,18 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 		}
 
 		if (!intel_dp->is_mst) {
+			bool full_detect = false;
+
 			/*
 			 * we'll check the link status via the normal hot plug path later -
 			 * but for short hpds we should check it now
 			 */
 			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-			intel_dp_check_link_status(intel_dp);
+			intel_dp_check_link_status(intel_dp, &full_detect);
 			drm_modeset_unlock(&dev->mode_config.connection_mutex);
+
+			if (full_detect)
+				goto put_power;
 		}
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 81b7d77..8aca5bb 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -712,6 +712,7 @@ struct intel_dp {
 	enum hdmi_force_audio force_audio;
 	bool limited_color_range;
 	bool color_range_auto;
+	uint8_t sink_count;
 	uint8_t dpcd[DP_RECEIVER_CAP_SIZE];
 	uint8_t psr_dpcd[EDP_PSR_RECEIVER_CAP_SIZE];
 	uint8_t downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
-- 
1.7.9.5

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

Re: [PATCH 1/2] drm/i915: Read sink_count dpcd always for short hpd

[Sivakumar Thulasimani]

On 8/17/2015 5:39 PM, Jani Nikula wrote:
> On Mon, 17 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
>> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
>>
>> Compliance test 4.2.2.8 requires driver to read the sink_count for
>> short pulse interrupt even when the panel is not enabled.
>> This patch performs the following
>> a) reading sink_count by reusing intel_dp_detect_dpcd
>> instead of using intel_dp_get_dpcd
>> b) moving crtc enabled checks post sink_count read call
>>
>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c |  117 ++++++++++++++++++++-------------------
>>   1 file changed, 59 insertions(+), 58 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index b905c19..e4de8e5 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4342,6 +4342,56 @@ go_again:
>>   	return -EINVAL;
>>   }
>>   
>> +/* XXX this is probably wrong for multiple downstream ports */
>> +static enum drm_connector_status
>> +intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>> +{
>> +	uint8_t *dpcd = intel_dp->dpcd;
>> +	uint8_t type;
>> +
>> +	if (!intel_dp_get_dpcd(intel_dp))
>> +		return connector_status_disconnected;
>> +
>> +	/* if there's no downstream port, we're done */
>> +	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
>> +		return connector_status_connected;
>> +
>> +	/* 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)
>> +			return connector_status_unknown;
>> +
>> +		return DP_GET_SINK_COUNT(reg) ? connector_status_connected
>> +					      : connector_status_disconnected;
>> +	}
>> +
>> +	/* If no HPD, poke DDC gently */
>> +	if (drm_probe_ddc(&intel_dp->aux.ddc))
>> +		return connector_status_connected;
>> +
>> +	/* Well we tried, say unknown for unreliable port types */
>> +	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11) {
>> +		type = intel_dp->downstream_ports[0] & DP_DS_PORT_TYPE_MASK;
>> +		if (type == DP_DS_PORT_TYPE_VGA ||
>> +		    type == DP_DS_PORT_TYPE_NON_EDID)
>> +			return connector_status_unknown;
>> +	} else {
>> +		type = intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
>> +			DP_DWN_STRM_PORT_TYPE_MASK;
>> +		if (type == DP_DWN_STRM_PORT_TYPE_ANALOG ||
>> +		    type == DP_DWN_STRM_PORT_TYPE_OTHER)
>> +			return connector_status_unknown;
>> +	}
>> +
>> +	/* Anything else is out of spec, warn and ignore */
>> +	DRM_DEBUG_KMS("Broken DP branch device, ignoring\n");
>> +	return connector_status_disconnected;
>> +}
> Please either a) just add a forward declaration for
> intel_dp_detect_dpcd, or b) add a separate non-functional prep patch
> that moves the function around. Please don't combine code movement with
> functional changes.
>
> BR,
> Jani.
uploaded V2 of the patch with forward declaration.
>
>> +
>>   /*
>>    * According to DP spec
>>    * 5.1.2:
>> @@ -4362,21 +4412,22 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>   
>>   	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>>   
>> -	if (!intel_encoder->base.crtc)
>> +	/* 4.2.2.8 requires source to read link_status, 0 - 12 DPCD &
>> +	 * sink_count even for short pulse irrespective of the sink is
>> +	 * in use or not
>> +	 */
>> +	if (!intel_dp_get_link_status(intel_dp, link_status))
>>   		return;
>>   
>> -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>> +	/* reuse to read both 0 - 12 DPCD & sink_count */
>> +	if (intel_dp_detect_dpcd(intel_dp) != connector_status_connected)
>>   		return;
>>   
>> -	/* Try to read receiver status if the link appears to be up */
>> -	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>> +	if (!intel_encoder->base.crtc)
>>   		return;
>> -	}
>>   
>> -	/* Now read the DPCD to see if it's actually running */
>> -	if (!intel_dp_get_dpcd(intel_dp)) {
>> +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>>   		return;
>> -	}
>>   
>>   	/* Try to read the source of the interrupt */
>>   	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>> @@ -4401,56 +4452,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>   	}
>>   }
>>   
>> -/* XXX this is probably wrong for multiple downstream ports */
>> -static enum drm_connector_status
>> -intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>> -{
>> -	uint8_t *dpcd = intel_dp->dpcd;
>> -	uint8_t type;
>> -
>> -	if (!intel_dp_get_dpcd(intel_dp))
>> -		return connector_status_disconnected;
>> -
>> -	/* if there's no downstream port, we're done */
>> -	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
>> -		return connector_status_connected;
>> -
>> -	/* 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)
>> -			return connector_status_unknown;
>> -
>> -		return DP_GET_SINK_COUNT(reg) ? connector_status_connected
>> -					      : connector_status_disconnected;
>> -	}
>> -
>> -	/* If no HPD, poke DDC gently */
>> -	if (drm_probe_ddc(&intel_dp->aux.ddc))
>> -		return connector_status_connected;
>> -
>> -	/* Well we tried, say unknown for unreliable port types */
>> -	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11) {
>> -		type = intel_dp->downstream_ports[0] & DP_DS_PORT_TYPE_MASK;
>> -		if (type == DP_DS_PORT_TYPE_VGA ||
>> -		    type == DP_DS_PORT_TYPE_NON_EDID)
>> -			return connector_status_unknown;
>> -	} else {
>> -		type = intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] &
>> -			DP_DWN_STRM_PORT_TYPE_MASK;
>> -		if (type == DP_DWN_STRM_PORT_TYPE_ANALOG ||
>> -		    type == DP_DWN_STRM_PORT_TYPE_OTHER)
>> -			return connector_status_unknown;
>> -	}
>> -
>> -	/* Anything else is out of spec, warn and ignore */
>> -	DRM_DEBUG_KMS("Broken DP branch device, ignoring\n");
>> -	return connector_status_disconnected;
>> -}
>> -
>>   static enum drm_connector_status
>>   edp_detect(struct intel_dp *intel_dp)
>>   {
>> -- 
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
regards,
Sivakumar

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

Re: [PATCH 1/2] drm/i915: Read sink_count dpcd always for short hpd

[Sivakumar Thulasimani]

dropping this patch as i understood more about
SINK_COUNT dpcd and DOWNSTREAM_PORT_PRESENT dpcd.
will upload a new series with proper fix.

On 8/17/2015 6:21 PM, Sivakumar Thulasimani wrote:
> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
>
> Compliance test 4.2.2.8 requires driver to read the sink_count for
> short pulse interrupt even when the panel is not enabled.
> This patch performs the following
> a) reading sink_count by reusing intel_dp_detect_dpcd
> instead of using intel_dp_get_dpcd
> b) moving crtc enabled checks post sink_count read call
>
> v2: avoid code movement with functionality changes (Ville)
>
> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_dp.c |   20 ++++++++++++--------
>   1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b905c19..0b73e98 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -132,6 +132,8 @@ 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 enum drm_connector_status
> +intel_dp_detect_dpcd(struct intel_dp *intel_dp);
>   
>   static int
>   intel_dp_max_link_bw(struct intel_dp  *intel_dp)
> @@ -4362,21 +4364,23 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>   
>   	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>   
> -	if (!intel_encoder->base.crtc)
> +	/* 4.2.2.8 requires source to read link_status, 0 - 12 DPCD &
> +	 * sink_count even for short pulse irrespective of the sink is
> +	 * in use or not
> +	 */
> +	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>   		return;
> +	}
>   
> -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> +	/* reuse to read both 0 - 12 DPCD & sink_count */
> +	if (intel_dp_detect_dpcd(intel_dp) != connector_status_connected)
>   		return;
>   
> -	/* Try to read receiver status if the link appears to be up */
> -	if (!intel_dp_get_link_status(intel_dp, link_status)) {
> +	if (!intel_encoder->base.crtc)
>   		return;
> -	}
>   
> -	/* Now read the DPCD to see if it's actually running */
> -	if (!intel_dp_get_dpcd(intel_dp)) {
> +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>   		return;
> -	}
>   
>   	/* Try to read the source of the interrupt */
>   	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&

-- 
regards,
Sivakumar

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