Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1359964 > unrolled thread

[PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()

Started byLyude <cpaul@redhat.com>
First post2016-03-17 16:50 +0100
Last post2016-03-21 14:40 +0100
Articles 11 — 5 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake() Lyude <cpaul@redhat.com> - 2016-03-17 16:50 +0100
    Re: [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake() Jani Nikula <jani.nikula@linux.intel.com> - 2016-03-17 19:00 +0100
      Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of  intel_dp_dpcd_read_wake() Daniel Vetter <daniel@ffwll.ch> - 2016-03-18 17:00 +0100
    Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of  intel_dp_dpcd_read_wake() Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-03-18 15:20 +0100
      Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of  intel_dp_dpcd_read_wake() Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-03-18 17:20 +0100
        Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of  intel_dp_dpcd_read_wake() Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-03-18 17:50 +0100
          Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of  intel_dp_dpcd_read_wake() Daniel Vetter <daniel@ffwll.ch> - 2016-03-18 19:00 +0100
            Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of  intel_dp_dpcd_read_wake() Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-03-18 19:10 +0100
              Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of  intel_dp_dpcd_read_wake() Lyude Paul <cpaul@redhat.com> - 2016-03-21 17:40 +0100
          Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake() Jani Nikula <jani.nikula@linux.intel.com> - 2016-03-21 11:40 +0100
            Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of  intel_dp_dpcd_read_wake() Ville Syrjälä <ville.syrjala@linux.intel.com> - 2016-03-21 14:40 +0100

#1359964 — [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()

FromLyude <cpaul@redhat.com>
Date2016-03-17 16:50 +0100
Subject[PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()
Message-ID<rdIlr-MU-5@gated-at.bofh.it>
Since we've fixed up drm_dp_dpcd_read() to allow for retries when things
timeout, there's no use for having this function anymore. Good riddens.

Signed-off-by: Lyude <cpaul@redhat.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 79 ++++++++++++-----------------------------
 1 file changed, 22 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index cdc2c15..fb4cbbe5 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3190,47 +3190,14 @@ static void chv_dp_post_pll_disable(struct intel_encoder *encoder)
 }
 
 /*
- * Native read with retry for link status and receiver capability reads for
- * cases where the sink may still be asleep.
- *
- * Sinks are *supposed* to come up within 1ms from an off state, but we're also
- * supposed to retry 3 times per the spec.
- */
-static ssize_t
-intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
-			void *buffer, size_t size)
-{
-	ssize_t ret;
-	int i;
-
-	/*
-	 * Sometime we just get the same incorrect byte repeated
-	 * over the entire buffer. Doing just one throw away read
-	 * initially seems to "solve" it.
-	 */
-	drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
-
-	for (i = 0; i < 3; i++) {
-		ret = drm_dp_dpcd_read(aux, offset, buffer, size);
-		if (ret == size)
-			return ret;
-		msleep(1);
-	}
-
-	return ret;
-}
-
-/*
  * Fetch AUX CH registers 0x202 - 0x207 which contain
  * link status information
  */
 bool
 intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE])
 {
-	return intel_dp_dpcd_read_wake(&intel_dp->aux,
-				       DP_LANE0_1_STATUS,
-				       link_status,
-				       DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
+	return drm_dp_dpcd_read(&intel_dp->aux, DP_LANE0_1_STATUS, link_status,
+				DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
 }
 
 /* These are source-specific values. */
@@ -3865,8 +3832,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint8_t rev;
 
-	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
-				    sizeof(intel_dp->dpcd)) < 0)
+	if (drm_dp_dpcd_read(&intel_dp->aux, 0x000, intel_dp->dpcd,
+			     sizeof(intel_dp->dpcd)) < 0)
 		return false; /* aux transfer failed */
 
 	DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp->dpcd), intel_dp->dpcd);
@@ -3877,9 +3844,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	/* Check if the panel supports PSR */
 	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
 	if (is_edp(intel_dp)) {
-		intel_dp_dpcd_read_wake(&intel_dp->aux, DP_PSR_SUPPORT,
-					intel_dp->psr_dpcd,
-					sizeof(intel_dp->psr_dpcd));
+		drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT,
+				 intel_dp->psr_dpcd,
+				 sizeof(intel_dp->psr_dpcd));
 		if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
 			dev_priv->psr.sink_support = true;
 			DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
@@ -3890,9 +3857,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 			uint8_t frame_sync_cap;
 
 			dev_priv->psr.sink_support = true;
-			intel_dp_dpcd_read_wake(&intel_dp->aux,
-					DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
-					&frame_sync_cap, 1);
+			drm_dp_dpcd_read(&intel_dp->aux,
+					 DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
+					 &frame_sync_cap, 1);
 			dev_priv->psr.aux_frame_sync = frame_sync_cap ? true : false;
 			/* PSR2 needs frame sync as well */
 			dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync;
@@ -3908,15 +3875,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	/* Intermediate frequency support */
 	if (is_edp(intel_dp) &&
 	    (intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] &	DP_DPCD_DISPLAY_CONTROL_CAPABLE) &&
-	    (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_EDP_DPCD_REV, &rev, 1) == 1) &&
+	    (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_DPCD_REV, &rev, 1) == 1) &&
 	    (rev >= 0x03)) { /* eDp v1.4 or higher */
 		__le16 sink_rates[DP_MAX_SUPPORTED_RATES];
 		int i;
 
-		intel_dp_dpcd_read_wake(&intel_dp->aux,
-				DP_SUPPORTED_LINK_RATES,
-				sink_rates,
-				sizeof(sink_rates));
+		drm_dp_dpcd_read(&intel_dp->aux, DP_SUPPORTED_LINK_RATES,
+				sink_rates, sizeof(sink_rates));
 
 		for (i = 0; i < ARRAY_SIZE(sink_rates); i++) {
 			int val = le16_to_cpu(sink_rates[i]);
@@ -3939,9 +3904,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	if (intel_dp->dpcd[DP_DPCD_REV] == 0x10)
 		return true; /* no per-port downstream info */
 
-	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_DOWNSTREAM_PORT_0,
-				    intel_dp->downstream_ports,
-				    DP_MAX_DOWNSTREAM_PORTS) < 0)
+	if (drm_dp_dpcd_read(&intel_dp->aux, DP_DOWNSTREAM_PORT_0,
+			     intel_dp->downstream_ports,
+			     DP_MAX_DOWNSTREAM_PORTS) < 0)
 		return false; /* downstream port status fetch failed */
 
 	return true;
@@ -3955,11 +3920,11 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
 	if (!(intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
 		return;
 
-	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
+	if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
 		DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
 			      buf[0], buf[1], buf[2]);
 
-	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
+	if (drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
 		DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
 			      buf[0], buf[1], buf[2]);
 }
@@ -3975,7 +3940,7 @@ intel_dp_probe_mst(struct intel_dp *intel_dp)
 	if (intel_dp->dpcd[DP_DPCD_REV] < 0x12)
 		return false;
 
-	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_MSTM_CAP, buf, 1)) {
+	if (drm_dp_dpcd_read(&intel_dp->aux, DP_MSTM_CAP, buf, 1)) {
 		if (buf[0] & DP_MST_CAP) {
 			DRM_DEBUG_KMS("Sink is MST capable\n");
 			intel_dp->is_mst = true;
@@ -4112,7 +4077,7 @@ stop:
 static bool
 intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
 {
-	return intel_dp_dpcd_read_wake(&intel_dp->aux,
+	return drm_dp_dpcd_read(&intel_dp->aux,
 				       DP_DEVICE_SERVICE_IRQ_VECTOR,
 				       sink_irq_vector, 1) == 1;
 }
@@ -4122,7 +4087,7 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
 {
 	int ret;
 
-	ret = intel_dp_dpcd_read_wake(&intel_dp->aux,
+	ret = drm_dp_dpcd_read(&intel_dp->aux,
 					     DP_SINK_COUNT_ESI,
 					     sink_irq_vector, 14);
 	if (ret != 14)
@@ -4383,7 +4348,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
 	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
 		uint8_t reg;
 
-		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
+		if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_COUNT,
 					    &reg, 1) < 0)
 			return connector_status_unknown;
 
-- 
2.5.0

[toc] | [next] | [standalone]

#1360098

FromJani Nikula <jani.nikula@linux.intel.com>
Date2016-03-17 19:00 +0100
Message-ID<rdKnh-22V-23@gated-at.bofh.it>
In reply to#1359964
On Thu, 17 Mar 2016, Lyude <cpaul@redhat.com> wrote:
> Since we've fixed up drm_dp_dpcd_read() to allow for retries when things
> timeout, there's no use for having this function anymore. Good riddens.
>
> Signed-off-by: Lyude <cpaul@redhat.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 79 ++++++++++++-----------------------------
>  1 file changed, 22 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index cdc2c15..fb4cbbe5 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3190,47 +3190,14 @@ static void chv_dp_post_pll_disable(struct intel_encoder *encoder)
>  }
>  
>  /*
> - * Native read with retry for link status and receiver capability reads for
> - * cases where the sink may still be asleep.
> - *
> - * Sinks are *supposed* to come up within 1ms from an off state, but we're also
> - * supposed to retry 3 times per the spec.
> - */
> -static ssize_t
> -intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
> -			void *buffer, size_t size)
> -{
> -	ssize_t ret;
> -	int i;
> -
> -	/*
> -	 * Sometime we just get the same incorrect byte repeated
> -	 * over the entire buffer. Doing just one throw away read
> -	 * initially seems to "solve" it.
> -	 */
> -	drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);

Last time around I dug out the commit adding the line above, which fixed
a bug, and said it should be accounted for somehow. But I've had a
change of heart. I want this function gone already. We've improved the
dp helpers considerably, and if this regresses, we need to look at
fixing this either at the drm level or in our ->transfer hook.

Acked-by: Jani Nikula <jani.nikula@intel.com>


> -
> -	for (i = 0; i < 3; i++) {
> -		ret = drm_dp_dpcd_read(aux, offset, buffer, size);
> -		if (ret == size)
> -			return ret;
> -		msleep(1);
> -	}
> -
> -	return ret;
> -}
> -
> -/*
>   * Fetch AUX CH registers 0x202 - 0x207 which contain
>   * link status information
>   */
>  bool
>  intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE])
>  {
> -	return intel_dp_dpcd_read_wake(&intel_dp->aux,
> -				       DP_LANE0_1_STATUS,
> -				       link_status,
> -				       DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
> +	return drm_dp_dpcd_read(&intel_dp->aux, DP_LANE0_1_STATUS, link_status,
> +				DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
>  }
>  
>  /* These are source-specific values. */
> @@ -3865,8 +3832,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint8_t rev;
>  
> -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
> -				    sizeof(intel_dp->dpcd)) < 0)
> +	if (drm_dp_dpcd_read(&intel_dp->aux, 0x000, intel_dp->dpcd,
> +			     sizeof(intel_dp->dpcd)) < 0)
>  		return false; /* aux transfer failed */
>  
>  	DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp->dpcd), intel_dp->dpcd);
> @@ -3877,9 +3844,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  	/* Check if the panel supports PSR */
>  	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>  	if (is_edp(intel_dp)) {
> -		intel_dp_dpcd_read_wake(&intel_dp->aux, DP_PSR_SUPPORT,
> -					intel_dp->psr_dpcd,
> -					sizeof(intel_dp->psr_dpcd));
> +		drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT,
> +				 intel_dp->psr_dpcd,
> +				 sizeof(intel_dp->psr_dpcd));
>  		if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
>  			dev_priv->psr.sink_support = true;
>  			DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> @@ -3890,9 +3857,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  			uint8_t frame_sync_cap;
>  
>  			dev_priv->psr.sink_support = true;
> -			intel_dp_dpcd_read_wake(&intel_dp->aux,
> -					DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> -					&frame_sync_cap, 1);
> +			drm_dp_dpcd_read(&intel_dp->aux,
> +					 DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> +					 &frame_sync_cap, 1);
>  			dev_priv->psr.aux_frame_sync = frame_sync_cap ? true : false;
>  			/* PSR2 needs frame sync as well */
>  			dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync;
> @@ -3908,15 +3875,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  	/* Intermediate frequency support */
>  	if (is_edp(intel_dp) &&
>  	    (intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] &	DP_DPCD_DISPLAY_CONTROL_CAPABLE) &&
> -	    (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_EDP_DPCD_REV, &rev, 1) == 1) &&
> +	    (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_DPCD_REV, &rev, 1) == 1) &&
>  	    (rev >= 0x03)) { /* eDp v1.4 or higher */
>  		__le16 sink_rates[DP_MAX_SUPPORTED_RATES];
>  		int i;
>  
> -		intel_dp_dpcd_read_wake(&intel_dp->aux,
> -				DP_SUPPORTED_LINK_RATES,
> -				sink_rates,
> -				sizeof(sink_rates));
> +		drm_dp_dpcd_read(&intel_dp->aux, DP_SUPPORTED_LINK_RATES,
> +				sink_rates, sizeof(sink_rates));
>  
>  		for (i = 0; i < ARRAY_SIZE(sink_rates); i++) {
>  			int val = le16_to_cpu(sink_rates[i]);
> @@ -3939,9 +3904,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  	if (intel_dp->dpcd[DP_DPCD_REV] == 0x10)
>  		return true; /* no per-port downstream info */
>  
> -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_DOWNSTREAM_PORT_0,
> -				    intel_dp->downstream_ports,
> -				    DP_MAX_DOWNSTREAM_PORTS) < 0)
> +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_DOWNSTREAM_PORT_0,
> +			     intel_dp->downstream_ports,
> +			     DP_MAX_DOWNSTREAM_PORTS) < 0)
>  		return false; /* downstream port status fetch failed */
>  
>  	return true;
> @@ -3955,11 +3920,11 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
>  	if (!(intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
>  		return;
>  
> -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
> +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
>  		DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
>  			      buf[0], buf[1], buf[2]);
>  
> -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
> +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
>  		DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
>  			      buf[0], buf[1], buf[2]);
>  }
> @@ -3975,7 +3940,7 @@ intel_dp_probe_mst(struct intel_dp *intel_dp)
>  	if (intel_dp->dpcd[DP_DPCD_REV] < 0x12)
>  		return false;
>  
> -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_MSTM_CAP, buf, 1)) {
> +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_MSTM_CAP, buf, 1)) {
>  		if (buf[0] & DP_MST_CAP) {
>  			DRM_DEBUG_KMS("Sink is MST capable\n");
>  			intel_dp->is_mst = true;
> @@ -4112,7 +4077,7 @@ stop:
>  static bool
>  intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>  {
> -	return intel_dp_dpcd_read_wake(&intel_dp->aux,
> +	return drm_dp_dpcd_read(&intel_dp->aux,
>  				       DP_DEVICE_SERVICE_IRQ_VECTOR,
>  				       sink_irq_vector, 1) == 1;
>  }
> @@ -4122,7 +4087,7 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>  {
>  	int ret;
>  
> -	ret = intel_dp_dpcd_read_wake(&intel_dp->aux,
> +	ret = drm_dp_dpcd_read(&intel_dp->aux,
>  					     DP_SINK_COUNT_ESI,
>  					     sink_irq_vector, 14);
>  	if (ret != 14)
> @@ -4383,7 +4348,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>  	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
>  		uint8_t reg;
>  
> -		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
> +		if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_COUNT,
>  					    &reg, 1) < 0)
>  			return connector_status_unknown;

-- 
Jani Nikula, Intel Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1360749 — Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()

FromDaniel Vetter <daniel@ffwll.ch>
Date2016-03-18 17:00 +0100
SubjectRe: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()
Message-ID<re4YF-Ni-7@gated-at.bofh.it>
In reply to#1360098
On Thu, Mar 17, 2016 at 07:56:33PM +0200, Jani Nikula wrote:
> On Thu, 17 Mar 2016, Lyude <cpaul@redhat.com> wrote:
> > Since we've fixed up drm_dp_dpcd_read() to allow for retries when things
> > timeout, there's no use for having this function anymore. Good riddens.
> >
> > Signed-off-by: Lyude <cpaul@redhat.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 79 ++++++++++++-----------------------------
> >  1 file changed, 22 insertions(+), 57 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index cdc2c15..fb4cbbe5 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3190,47 +3190,14 @@ static void chv_dp_post_pll_disable(struct intel_encoder *encoder)
> >  }
> >  
> >  /*
> > - * Native read with retry for link status and receiver capability reads for
> > - * cases where the sink may still be asleep.
> > - *
> > - * Sinks are *supposed* to come up within 1ms from an off state, but we're also
> > - * supposed to retry 3 times per the spec.
> > - */
> > -static ssize_t
> > -intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
> > -			void *buffer, size_t size)
> > -{
> > -	ssize_t ret;
> > -	int i;
> > -
> > -	/*
> > -	 * Sometime we just get the same incorrect byte repeated
> > -	 * over the entire buffer. Doing just one throw away read
> > -	 * initially seems to "solve" it.
> > -	 */
> > -	drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
> 
> Last time around I dug out the commit adding the line above, which fixed
> a bug, and said it should be accounted for somehow. But I've had a
> change of heart. I want this function gone already. We've improved the
> dp helpers considerably, and if this regresses, we need to look at
> fixing this either at the drm level or in our ->transfer hook.
> 
> Acked-by: Jani Nikula <jani.nikula@intel.com>

Hm, my idea was actually to 1:1 move intel_dp_dpcd_read_wake logic into
drm_dp_dpcd_read, without any functional change for i915.ko. And everyone
else shouldn't complain about a few more retries either.

That would also address Ville's grumpy nack.
-Daniel

> 
> 
> > -
> > -	for (i = 0; i < 3; i++) {
> > -		ret = drm_dp_dpcd_read(aux, offset, buffer, size);
> > -		if (ret == size)
> > -			return ret;
> > -		msleep(1);
> > -	}
> > -
> > -	return ret;
> > -}
> > -
> > -/*
> >   * Fetch AUX CH registers 0x202 - 0x207 which contain
> >   * link status information
> >   */
> >  bool
> >  intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE])
> >  {
> > -	return intel_dp_dpcd_read_wake(&intel_dp->aux,
> > -				       DP_LANE0_1_STATUS,
> > -				       link_status,
> > -				       DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
> > +	return drm_dp_dpcd_read(&intel_dp->aux, DP_LANE0_1_STATUS, link_status,
> > +				DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
> >  }
> >  
> >  /* These are source-specific values. */
> > @@ -3865,8 +3832,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	uint8_t rev;
> >  
> > -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
> > -				    sizeof(intel_dp->dpcd)) < 0)
> > +	if (drm_dp_dpcd_read(&intel_dp->aux, 0x000, intel_dp->dpcd,
> > +			     sizeof(intel_dp->dpcd)) < 0)
> >  		return false; /* aux transfer failed */
> >  
> >  	DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp->dpcd), intel_dp->dpcd);
> > @@ -3877,9 +3844,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> >  	/* Check if the panel supports PSR */
> >  	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
> >  	if (is_edp(intel_dp)) {
> > -		intel_dp_dpcd_read_wake(&intel_dp->aux, DP_PSR_SUPPORT,
> > -					intel_dp->psr_dpcd,
> > -					sizeof(intel_dp->psr_dpcd));
> > +		drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT,
> > +				 intel_dp->psr_dpcd,
> > +				 sizeof(intel_dp->psr_dpcd));
> >  		if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
> >  			dev_priv->psr.sink_support = true;
> >  			DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> > @@ -3890,9 +3857,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> >  			uint8_t frame_sync_cap;
> >  
> >  			dev_priv->psr.sink_support = true;
> > -			intel_dp_dpcd_read_wake(&intel_dp->aux,
> > -					DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> > -					&frame_sync_cap, 1);
> > +			drm_dp_dpcd_read(&intel_dp->aux,
> > +					 DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> > +					 &frame_sync_cap, 1);
> >  			dev_priv->psr.aux_frame_sync = frame_sync_cap ? true : false;
> >  			/* PSR2 needs frame sync as well */
> >  			dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync;
> > @@ -3908,15 +3875,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> >  	/* Intermediate frequency support */
> >  	if (is_edp(intel_dp) &&
> >  	    (intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] &	DP_DPCD_DISPLAY_CONTROL_CAPABLE) &&
> > -	    (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_EDP_DPCD_REV, &rev, 1) == 1) &&
> > +	    (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_DPCD_REV, &rev, 1) == 1) &&
> >  	    (rev >= 0x03)) { /* eDp v1.4 or higher */
> >  		__le16 sink_rates[DP_MAX_SUPPORTED_RATES];
> >  		int i;
> >  
> > -		intel_dp_dpcd_read_wake(&intel_dp->aux,
> > -				DP_SUPPORTED_LINK_RATES,
> > -				sink_rates,
> > -				sizeof(sink_rates));
> > +		drm_dp_dpcd_read(&intel_dp->aux, DP_SUPPORTED_LINK_RATES,
> > +				sink_rates, sizeof(sink_rates));
> >  
> >  		for (i = 0; i < ARRAY_SIZE(sink_rates); i++) {
> >  			int val = le16_to_cpu(sink_rates[i]);
> > @@ -3939,9 +3904,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> >  	if (intel_dp->dpcd[DP_DPCD_REV] == 0x10)
> >  		return true; /* no per-port downstream info */
> >  
> > -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_DOWNSTREAM_PORT_0,
> > -				    intel_dp->downstream_ports,
> > -				    DP_MAX_DOWNSTREAM_PORTS) < 0)
> > +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_DOWNSTREAM_PORT_0,
> > +			     intel_dp->downstream_ports,
> > +			     DP_MAX_DOWNSTREAM_PORTS) < 0)
> >  		return false; /* downstream port status fetch failed */
> >  
> >  	return true;
> > @@ -3955,11 +3920,11 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
> >  	if (!(intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
> >  		return;
> >  
> > -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
> > +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
> >  		DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
> >  			      buf[0], buf[1], buf[2]);
> >  
> > -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
> > +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
> >  		DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
> >  			      buf[0], buf[1], buf[2]);
> >  }
> > @@ -3975,7 +3940,7 @@ intel_dp_probe_mst(struct intel_dp *intel_dp)
> >  	if (intel_dp->dpcd[DP_DPCD_REV] < 0x12)
> >  		return false;
> >  
> > -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_MSTM_CAP, buf, 1)) {
> > +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_MSTM_CAP, buf, 1)) {
> >  		if (buf[0] & DP_MST_CAP) {
> >  			DRM_DEBUG_KMS("Sink is MST capable\n");
> >  			intel_dp->is_mst = true;
> > @@ -4112,7 +4077,7 @@ stop:
> >  static bool
> >  intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
> >  {
> > -	return intel_dp_dpcd_read_wake(&intel_dp->aux,
> > +	return drm_dp_dpcd_read(&intel_dp->aux,
> >  				       DP_DEVICE_SERVICE_IRQ_VECTOR,
> >  				       sink_irq_vector, 1) == 1;
> >  }
> > @@ -4122,7 +4087,7 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
> >  {
> >  	int ret;
> >  
> > -	ret = intel_dp_dpcd_read_wake(&intel_dp->aux,
> > +	ret = drm_dp_dpcd_read(&intel_dp->aux,
> >  					     DP_SINK_COUNT_ESI,
> >  					     sink_irq_vector, 14);
> >  	if (ret != 14)
> > @@ -4383,7 +4348,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
> >  	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
> >  		uint8_t reg;
> >  
> > -		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
> > +		if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_COUNT,
> >  					    &reg, 1) < 0)
> >  			return connector_status_unknown;
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1360658 — Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()

FromVille Syrjälä <ville.syrjala@linux.intel.com>
Date2016-03-18 15:20 +0100
SubjectRe: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()
Message-ID<re3pU-6Yy-11@gated-at.bofh.it>
In reply to#1359964
On Thu, Mar 17, 2016 at 11:40:45AM -0400, Lyude wrote:
> Since we've fixed up drm_dp_dpcd_read() to allow for retries when things
> timeout, there's no use for having this function anymore. Good riddens.
> 
> Signed-off-by: Lyude <cpaul@redhat.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 79 ++++++++++++-----------------------------
>  1 file changed, 22 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index cdc2c15..fb4cbbe5 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3190,47 +3190,14 @@ static void chv_dp_post_pll_disable(struct intel_encoder *encoder)
>  }
>  
>  /*
> - * Native read with retry for link status and receiver capability reads for
> - * cases where the sink may still be asleep.
> - *
> - * Sinks are *supposed* to come up within 1ms from an off state, but we're also
> - * supposed to retry 3 times per the spec.
> - */
> -static ssize_t
> -intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
> -			void *buffer, size_t size)
> -{
> -	ssize_t ret;
> -	int i;
> -
> -	/*
> -	 * Sometime we just get the same incorrect byte repeated
> -	 * over the entire buffer. Doing just one throw away read
> -	 * initially seems to "solve" it.
> -	 */
> -	drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);

NAK

If people keep intentionally breaking my shit I'm going to become
really grumpy soon.


> -
> -	for (i = 0; i < 3; i++) {
> -		ret = drm_dp_dpcd_read(aux, offset, buffer, size);
> -		if (ret == size)
> -			return ret;
> -		msleep(1);
> -	}
> -
> -	return ret;
> -}
> -
> -/*
>   * Fetch AUX CH registers 0x202 - 0x207 which contain
>   * link status information
>   */
>  bool
>  intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE])
>  {
> -	return intel_dp_dpcd_read_wake(&intel_dp->aux,
> -				       DP_LANE0_1_STATUS,
> -				       link_status,
> -				       DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
> +	return drm_dp_dpcd_read(&intel_dp->aux, DP_LANE0_1_STATUS, link_status,
> +				DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
>  }
>  
>  /* These are source-specific values. */
> @@ -3865,8 +3832,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint8_t rev;
>  
> -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
> -				    sizeof(intel_dp->dpcd)) < 0)
> +	if (drm_dp_dpcd_read(&intel_dp->aux, 0x000, intel_dp->dpcd,
> +			     sizeof(intel_dp->dpcd)) < 0)
>  		return false; /* aux transfer failed */
>  
>  	DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp->dpcd), intel_dp->dpcd);
> @@ -3877,9 +3844,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  	/* Check if the panel supports PSR */
>  	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>  	if (is_edp(intel_dp)) {
> -		intel_dp_dpcd_read_wake(&intel_dp->aux, DP_PSR_SUPPORT,
> -					intel_dp->psr_dpcd,
> -					sizeof(intel_dp->psr_dpcd));
> +		drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT,
> +				 intel_dp->psr_dpcd,
> +				 sizeof(intel_dp->psr_dpcd));
>  		if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
>  			dev_priv->psr.sink_support = true;
>  			DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> @@ -3890,9 +3857,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  			uint8_t frame_sync_cap;
>  
>  			dev_priv->psr.sink_support = true;
> -			intel_dp_dpcd_read_wake(&intel_dp->aux,
> -					DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> -					&frame_sync_cap, 1);
> +			drm_dp_dpcd_read(&intel_dp->aux,
> +					 DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> +					 &frame_sync_cap, 1);
>  			dev_priv->psr.aux_frame_sync = frame_sync_cap ? true : false;
>  			/* PSR2 needs frame sync as well */
>  			dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync;
> @@ -3908,15 +3875,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  	/* Intermediate frequency support */
>  	if (is_edp(intel_dp) &&
>  	    (intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] &	DP_DPCD_DISPLAY_CONTROL_CAPABLE) &&
> -	    (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_EDP_DPCD_REV, &rev, 1) == 1) &&
> +	    (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_DPCD_REV, &rev, 1) == 1) &&
>  	    (rev >= 0x03)) { /* eDp v1.4 or higher */
>  		__le16 sink_rates[DP_MAX_SUPPORTED_RATES];
>  		int i;
>  
> -		intel_dp_dpcd_read_wake(&intel_dp->aux,
> -				DP_SUPPORTED_LINK_RATES,
> -				sink_rates,
> -				sizeof(sink_rates));
> +		drm_dp_dpcd_read(&intel_dp->aux, DP_SUPPORTED_LINK_RATES,
> +				sink_rates, sizeof(sink_rates));
>  
>  		for (i = 0; i < ARRAY_SIZE(sink_rates); i++) {
>  			int val = le16_to_cpu(sink_rates[i]);
> @@ -3939,9 +3904,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  	if (intel_dp->dpcd[DP_DPCD_REV] == 0x10)
>  		return true; /* no per-port downstream info */
>  
> -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_DOWNSTREAM_PORT_0,
> -				    intel_dp->downstream_ports,
> -				    DP_MAX_DOWNSTREAM_PORTS) < 0)
> +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_DOWNSTREAM_PORT_0,
> +			     intel_dp->downstream_ports,
> +			     DP_MAX_DOWNSTREAM_PORTS) < 0)
>  		return false; /* downstream port status fetch failed */
>  
>  	return true;
> @@ -3955,11 +3920,11 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
>  	if (!(intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
>  		return;
>  
> -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
> +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
>  		DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
>  			      buf[0], buf[1], buf[2]);
>  
> -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
> +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
>  		DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
>  			      buf[0], buf[1], buf[2]);
>  }
> @@ -3975,7 +3940,7 @@ intel_dp_probe_mst(struct intel_dp *intel_dp)
>  	if (intel_dp->dpcd[DP_DPCD_REV] < 0x12)
>  		return false;
>  
> -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_MSTM_CAP, buf, 1)) {
> +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_MSTM_CAP, buf, 1)) {
>  		if (buf[0] & DP_MST_CAP) {
>  			DRM_DEBUG_KMS("Sink is MST capable\n");
>  			intel_dp->is_mst = true;
> @@ -4112,7 +4077,7 @@ stop:
>  static bool
>  intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>  {
> -	return intel_dp_dpcd_read_wake(&intel_dp->aux,
> +	return drm_dp_dpcd_read(&intel_dp->aux,
>  				       DP_DEVICE_SERVICE_IRQ_VECTOR,
>  				       sink_irq_vector, 1) == 1;
>  }
> @@ -4122,7 +4087,7 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>  {
>  	int ret;
>  
> -	ret = intel_dp_dpcd_read_wake(&intel_dp->aux,
> +	ret = drm_dp_dpcd_read(&intel_dp->aux,
>  					     DP_SINK_COUNT_ESI,
>  					     sink_irq_vector, 14);
>  	if (ret != 14)
> @@ -4383,7 +4348,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>  	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
>  		uint8_t reg;
>  
> -		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
> +		if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_COUNT,
>  					    &reg, 1) < 0)
>  			return connector_status_unknown;
>  
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1360768 — Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()

FromVille Syrjälä <ville.syrjala@linux.intel.com>
Date2016-03-18 17:20 +0100
SubjectRe: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()
Message-ID<re5i3-1rG-25@gated-at.bofh.it>
In reply to#1360658
On Fri, Mar 18, 2016 at 04:13:45PM +0200, Ville Syrjälä wrote:
> On Thu, Mar 17, 2016 at 11:40:45AM -0400, Lyude wrote:
> > Since we've fixed up drm_dp_dpcd_read() to allow for retries when things
> > timeout, there's no use for having this function anymore. Good riddens.
> > 
> > Signed-off-by: Lyude <cpaul@redhat.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 79 ++++++++++++-----------------------------
> >  1 file changed, 22 insertions(+), 57 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index cdc2c15..fb4cbbe5 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3190,47 +3190,14 @@ static void chv_dp_post_pll_disable(struct intel_encoder *encoder)
> >  }
> >  
> >  /*
> > - * Native read with retry for link status and receiver capability reads for
> > - * cases where the sink may still be asleep.
> > - *
> > - * Sinks are *supposed* to come up within 1ms from an off state, but we're also
> > - * supposed to retry 3 times per the spec.
> > - */
> > -static ssize_t
> > -intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
> > -			void *buffer, size_t size)
> > -{
> > -	ssize_t ret;
> > -	int i;
> > -
> > -	/*
> > -	 * Sometime we just get the same incorrect byte repeated
> > -	 * over the entire buffer. Doing just one throw away read
> > -	 * initially seems to "solve" it.
> > -	 */
> > -	drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
> 
> NAK
> 
> If people keep intentionally breaking my shit I'm going to become
> really grumpy soon.

Oh, and just in case someone wants to come up with a better kludge,
I just spent a few minutes analyzing the behavior of this crappy
monitor a.

What happens is that when the monitor is fully powered up (LED is blue)
things are fine. After the monitor goes to sleep (LED turns orange)
the first DPCD read will produce garbage. Further DPCD reads are fine,
even if I wait a significant amount of time between the reads, as long
as the monitor didn't do a power on->off cycle in between. So it looks
like it's always just the first read after power down that gets
corrupted.

Now I think I'll go and test how writes behave, assuming I can find a
decently sized chunk of DPCD address space I can write. And maybe I
should also try i2c-over-aux...
 
> 
> > -
> > -	for (i = 0; i < 3; i++) {
> > -		ret = drm_dp_dpcd_read(aux, offset, buffer, size);
> > -		if (ret == size)
> > -			return ret;
> > -		msleep(1);
> > -	}
> > -
> > -	return ret;
> > -}
> > -
> > -/*
> >   * Fetch AUX CH registers 0x202 - 0x207 which contain
> >   * link status information
> >   */
> >  bool
> >  intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE])
> >  {
> > -	return intel_dp_dpcd_read_wake(&intel_dp->aux,
> > -				       DP_LANE0_1_STATUS,
> > -				       link_status,
> > -				       DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
> > +	return drm_dp_dpcd_read(&intel_dp->aux, DP_LANE0_1_STATUS, link_status,
> > +				DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
> >  }
> >  
> >  /* These are source-specific values. */
> > @@ -3865,8 +3832,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	uint8_t rev;
> >  
> > -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
> > -				    sizeof(intel_dp->dpcd)) < 0)
> > +	if (drm_dp_dpcd_read(&intel_dp->aux, 0x000, intel_dp->dpcd,
> > +			     sizeof(intel_dp->dpcd)) < 0)
> >  		return false; /* aux transfer failed */
> >  
> >  	DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp->dpcd), intel_dp->dpcd);
> > @@ -3877,9 +3844,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> >  	/* Check if the panel supports PSR */
> >  	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
> >  	if (is_edp(intel_dp)) {
> > -		intel_dp_dpcd_read_wake(&intel_dp->aux, DP_PSR_SUPPORT,
> > -					intel_dp->psr_dpcd,
> > -					sizeof(intel_dp->psr_dpcd));
> > +		drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT,
> > +				 intel_dp->psr_dpcd,
> > +				 sizeof(intel_dp->psr_dpcd));
> >  		if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
> >  			dev_priv->psr.sink_support = true;
> >  			DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> > @@ -3890,9 +3857,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> >  			uint8_t frame_sync_cap;
> >  
> >  			dev_priv->psr.sink_support = true;
> > -			intel_dp_dpcd_read_wake(&intel_dp->aux,
> > -					DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> > -					&frame_sync_cap, 1);
> > +			drm_dp_dpcd_read(&intel_dp->aux,
> > +					 DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> > +					 &frame_sync_cap, 1);
> >  			dev_priv->psr.aux_frame_sync = frame_sync_cap ? true : false;
> >  			/* PSR2 needs frame sync as well */
> >  			dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync;
> > @@ -3908,15 +3875,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> >  	/* Intermediate frequency support */
> >  	if (is_edp(intel_dp) &&
> >  	    (intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] &	DP_DPCD_DISPLAY_CONTROL_CAPABLE) &&
> > -	    (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_EDP_DPCD_REV, &rev, 1) == 1) &&
> > +	    (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_DPCD_REV, &rev, 1) == 1) &&
> >  	    (rev >= 0x03)) { /* eDp v1.4 or higher */
> >  		__le16 sink_rates[DP_MAX_SUPPORTED_RATES];
> >  		int i;
> >  
> > -		intel_dp_dpcd_read_wake(&intel_dp->aux,
> > -				DP_SUPPORTED_LINK_RATES,
> > -				sink_rates,
> > -				sizeof(sink_rates));
> > +		drm_dp_dpcd_read(&intel_dp->aux, DP_SUPPORTED_LINK_RATES,
> > +				sink_rates, sizeof(sink_rates));
> >  
> >  		for (i = 0; i < ARRAY_SIZE(sink_rates); i++) {
> >  			int val = le16_to_cpu(sink_rates[i]);
> > @@ -3939,9 +3904,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> >  	if (intel_dp->dpcd[DP_DPCD_REV] == 0x10)
> >  		return true; /* no per-port downstream info */
> >  
> > -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_DOWNSTREAM_PORT_0,
> > -				    intel_dp->downstream_ports,
> > -				    DP_MAX_DOWNSTREAM_PORTS) < 0)
> > +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_DOWNSTREAM_PORT_0,
> > +			     intel_dp->downstream_ports,
> > +			     DP_MAX_DOWNSTREAM_PORTS) < 0)
> >  		return false; /* downstream port status fetch failed */
> >  
> >  	return true;
> > @@ -3955,11 +3920,11 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
> >  	if (!(intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
> >  		return;
> >  
> > -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
> > +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
> >  		DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
> >  			      buf[0], buf[1], buf[2]);
> >  
> > -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
> > +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
> >  		DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
> >  			      buf[0], buf[1], buf[2]);
> >  }
> > @@ -3975,7 +3940,7 @@ intel_dp_probe_mst(struct intel_dp *intel_dp)
> >  	if (intel_dp->dpcd[DP_DPCD_REV] < 0x12)
> >  		return false;
> >  
> > -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_MSTM_CAP, buf, 1)) {
> > +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_MSTM_CAP, buf, 1)) {
> >  		if (buf[0] & DP_MST_CAP) {
> >  			DRM_DEBUG_KMS("Sink is MST capable\n");
> >  			intel_dp->is_mst = true;
> > @@ -4112,7 +4077,7 @@ stop:
> >  static bool
> >  intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
> >  {
> > -	return intel_dp_dpcd_read_wake(&intel_dp->aux,
> > +	return drm_dp_dpcd_read(&intel_dp->aux,
> >  				       DP_DEVICE_SERVICE_IRQ_VECTOR,
> >  				       sink_irq_vector, 1) == 1;
> >  }
> > @@ -4122,7 +4087,7 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
> >  {
> >  	int ret;
> >  
> > -	ret = intel_dp_dpcd_read_wake(&intel_dp->aux,
> > +	ret = drm_dp_dpcd_read(&intel_dp->aux,
> >  					     DP_SINK_COUNT_ESI,
> >  					     sink_irq_vector, 14);
> >  	if (ret != 14)
> > @@ -4383,7 +4348,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
> >  	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
> >  		uint8_t reg;
> >  
> > -		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
> > +		if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_COUNT,
> >  					    &reg, 1) < 0)
> >  			return connector_status_unknown;
> >  
> > -- 
> > 2.5.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1360782 — Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()

FromVille Syrjälä <ville.syrjala@linux.intel.com>
Date2016-03-18 17:50 +0100
SubjectRe: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()
Message-ID<re5L3-23u-1@gated-at.bofh.it>
In reply to#1360768
On Fri, Mar 18, 2016 at 06:12:35PM +0200, Ville Syrjälä wrote:
> On Fri, Mar 18, 2016 at 04:13:45PM +0200, Ville Syrjälä wrote:
> > On Thu, Mar 17, 2016 at 11:40:45AM -0400, Lyude wrote:
> > > Since we've fixed up drm_dp_dpcd_read() to allow for retries when things
> > > timeout, there's no use for having this function anymore. Good riddens.
> > > 
> > > Signed-off-by: Lyude <cpaul@redhat.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp.c | 79 ++++++++++++-----------------------------
> > >  1 file changed, 22 insertions(+), 57 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index cdc2c15..fb4cbbe5 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -3190,47 +3190,14 @@ static void chv_dp_post_pll_disable(struct intel_encoder *encoder)
> > >  }
> > >  
> > >  /*
> > > - * Native read with retry for link status and receiver capability reads for
> > > - * cases where the sink may still be asleep.
> > > - *
> > > - * Sinks are *supposed* to come up within 1ms from an off state, but we're also
> > > - * supposed to retry 3 times per the spec.
> > > - */
> > > -static ssize_t
> > > -intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
> > > -			void *buffer, size_t size)
> > > -{
> > > -	ssize_t ret;
> > > -	int i;
> > > -
> > > -	/*
> > > -	 * Sometime we just get the same incorrect byte repeated
> > > -	 * over the entire buffer. Doing just one throw away read
> > > -	 * initially seems to "solve" it.
> > > -	 */
> > > -	drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
> > 
> > NAK
> > 
> > If people keep intentionally breaking my shit I'm going to become
> > really grumpy soon.
> 
> Oh, and just in case someone wants to come up with a better kludge,
> I just spent a few minutes analyzing the behavior of this crappy
> monitor a.
> 
> What happens is that when the monitor is fully powered up (LED is blue)
> things are fine. After the monitor goes to sleep (LED turns orange)
> the first DPCD read will produce garbage. Further DPCD reads are fine,
> even if I wait a significant amount of time between the reads, as long
> as the monitor didn't do a power on->off cycle in between. So it looks
> like it's always just the first read after power down that gets
> corrupted.
> 
> Now I think I'll go and test how writes behave, assuming I can find a
> decently sized chunk of DPCD address space I can write. And maybe I
> should also try i2c-over-aux...

The first DPCD write after powerdown also got corrupted. But i2c-over-aux
seems unaffected for whatever reason.

>  
> > 
> > > -
> > > -	for (i = 0; i < 3; i++) {
> > > -		ret = drm_dp_dpcd_read(aux, offset, buffer, size);
> > > -		if (ret == size)
> > > -			return ret;
> > > -		msleep(1);
> > > -	}
> > > -
> > > -	return ret;
> > > -}
> > > -
> > > -/*
> > >   * Fetch AUX CH registers 0x202 - 0x207 which contain
> > >   * link status information
> > >   */
> > >  bool
> > >  intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE])
> > >  {
> > > -	return intel_dp_dpcd_read_wake(&intel_dp->aux,
> > > -				       DP_LANE0_1_STATUS,
> > > -				       link_status,
> > > -				       DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
> > > +	return drm_dp_dpcd_read(&intel_dp->aux, DP_LANE0_1_STATUS, link_status,
> > > +				DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
> > >  }
> > >  
> > >  /* These are source-specific values. */
> > > @@ -3865,8 +3832,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > >  	uint8_t rev;
> > >  
> > > -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
> > > -				    sizeof(intel_dp->dpcd)) < 0)
> > > +	if (drm_dp_dpcd_read(&intel_dp->aux, 0x000, intel_dp->dpcd,
> > > +			     sizeof(intel_dp->dpcd)) < 0)
> > >  		return false; /* aux transfer failed */
> > >  
> > >  	DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp->dpcd), intel_dp->dpcd);
> > > @@ -3877,9 +3844,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> > >  	/* Check if the panel supports PSR */
> > >  	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
> > >  	if (is_edp(intel_dp)) {
> > > -		intel_dp_dpcd_read_wake(&intel_dp->aux, DP_PSR_SUPPORT,
> > > -					intel_dp->psr_dpcd,
> > > -					sizeof(intel_dp->psr_dpcd));
> > > +		drm_dp_dpcd_read(&intel_dp->aux, DP_PSR_SUPPORT,
> > > +				 intel_dp->psr_dpcd,
> > > +				 sizeof(intel_dp->psr_dpcd));
> > >  		if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
> > >  			dev_priv->psr.sink_support = true;
> > >  			DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> > > @@ -3890,9 +3857,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> > >  			uint8_t frame_sync_cap;
> > >  
> > >  			dev_priv->psr.sink_support = true;
> > > -			intel_dp_dpcd_read_wake(&intel_dp->aux,
> > > -					DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> > > -					&frame_sync_cap, 1);
> > > +			drm_dp_dpcd_read(&intel_dp->aux,
> > > +					 DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> > > +					 &frame_sync_cap, 1);
> > >  			dev_priv->psr.aux_frame_sync = frame_sync_cap ? true : false;
> > >  			/* PSR2 needs frame sync as well */
> > >  			dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync;
> > > @@ -3908,15 +3875,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> > >  	/* Intermediate frequency support */
> > >  	if (is_edp(intel_dp) &&
> > >  	    (intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] &	DP_DPCD_DISPLAY_CONTROL_CAPABLE) &&
> > > -	    (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_EDP_DPCD_REV, &rev, 1) == 1) &&
> > > +	    (drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_DPCD_REV, &rev, 1) == 1) &&
> > >  	    (rev >= 0x03)) { /* eDp v1.4 or higher */
> > >  		__le16 sink_rates[DP_MAX_SUPPORTED_RATES];
> > >  		int i;
> > >  
> > > -		intel_dp_dpcd_read_wake(&intel_dp->aux,
> > > -				DP_SUPPORTED_LINK_RATES,
> > > -				sink_rates,
> > > -				sizeof(sink_rates));
> > > +		drm_dp_dpcd_read(&intel_dp->aux, DP_SUPPORTED_LINK_RATES,
> > > +				sink_rates, sizeof(sink_rates));
> > >  
> > >  		for (i = 0; i < ARRAY_SIZE(sink_rates); i++) {
> > >  			int val = le16_to_cpu(sink_rates[i]);
> > > @@ -3939,9 +3904,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> > >  	if (intel_dp->dpcd[DP_DPCD_REV] == 0x10)
> > >  		return true; /* no per-port downstream info */
> > >  
> > > -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_DOWNSTREAM_PORT_0,
> > > -				    intel_dp->downstream_ports,
> > > -				    DP_MAX_DOWNSTREAM_PORTS) < 0)
> > > +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_DOWNSTREAM_PORT_0,
> > > +			     intel_dp->downstream_ports,
> > > +			     DP_MAX_DOWNSTREAM_PORTS) < 0)
> > >  		return false; /* downstream port status fetch failed */
> > >  
> > >  	return true;
> > > @@ -3955,11 +3920,11 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
> > >  	if (!(intel_dp->dpcd[DP_DOWN_STREAM_PORT_COUNT] & DP_OUI_SUPPORT))
> > >  		return;
> > >  
> > > -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
> > > +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_OUI, buf, 3) == 3)
> > >  		DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
> > >  			      buf[0], buf[1], buf[2]);
> > >  
> > > -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
> > > +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_BRANCH_OUI, buf, 3) == 3)
> > >  		DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
> > >  			      buf[0], buf[1], buf[2]);
> > >  }
> > > @@ -3975,7 +3940,7 @@ intel_dp_probe_mst(struct intel_dp *intel_dp)
> > >  	if (intel_dp->dpcd[DP_DPCD_REV] < 0x12)
> > >  		return false;
> > >  
> > > -	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_MSTM_CAP, buf, 1)) {
> > > +	if (drm_dp_dpcd_read(&intel_dp->aux, DP_MSTM_CAP, buf, 1)) {
> > >  		if (buf[0] & DP_MST_CAP) {
> > >  			DRM_DEBUG_KMS("Sink is MST capable\n");
> > >  			intel_dp->is_mst = true;
> > > @@ -4112,7 +4077,7 @@ stop:
> > >  static bool
> > >  intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
> > >  {
> > > -	return intel_dp_dpcd_read_wake(&intel_dp->aux,
> > > +	return drm_dp_dpcd_read(&intel_dp->aux,
> > >  				       DP_DEVICE_SERVICE_IRQ_VECTOR,
> > >  				       sink_irq_vector, 1) == 1;
> > >  }
> > > @@ -4122,7 +4087,7 @@ intel_dp_get_sink_irq_esi(struct intel_dp *intel_dp, u8 *sink_irq_vector)
> > >  {
> > >  	int ret;
> > >  
> > > -	ret = intel_dp_dpcd_read_wake(&intel_dp->aux,
> > > +	ret = drm_dp_dpcd_read(&intel_dp->aux,
> > >  					     DP_SINK_COUNT_ESI,
> > >  					     sink_irq_vector, 14);
> > >  	if (ret != 14)
> > > @@ -4383,7 +4348,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
> > >  	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
> > >  		uint8_t reg;
> > >  
> > > -		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
> > > +		if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_COUNT,
> > >  					    &reg, 1) < 0)
> > >  			return connector_status_unknown;
> > >  
> > > -- 
> > > 2.5.0
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1360843 — Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()

FromDaniel Vetter <daniel@ffwll.ch>
Date2016-03-18 19:00 +0100
SubjectRe: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()
Message-ID<re6QP-3H6-9@gated-at.bofh.it>
In reply to#1360782
On Fri, Mar 18, 2016 at 06:41:40PM +0200, Ville Syrjälä wrote:
> On Fri, Mar 18, 2016 at 06:12:35PM +0200, Ville Syrjälä wrote:
> > On Fri, Mar 18, 2016 at 04:13:45PM +0200, Ville Syrjälä wrote:
> > > On Thu, Mar 17, 2016 at 11:40:45AM -0400, Lyude wrote:
> > > > Since we've fixed up drm_dp_dpcd_read() to allow for retries when things
> > > > timeout, there's no use for having this function anymore. Good riddens.
> > > > 
> > > > Signed-off-by: Lyude <cpaul@redhat.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp.c | 79 ++++++++++++-----------------------------
> > > >  1 file changed, 22 insertions(+), 57 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > index cdc2c15..fb4cbbe5 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -3190,47 +3190,14 @@ static void chv_dp_post_pll_disable(struct intel_encoder *encoder)
> > > >  }
> > > >  
> > > >  /*
> > > > - * Native read with retry for link status and receiver capability reads for
> > > > - * cases where the sink may still be asleep.
> > > > - *
> > > > - * Sinks are *supposed* to come up within 1ms from an off state, but we're also
> > > > - * supposed to retry 3 times per the spec.
> > > > - */
> > > > -static ssize_t
> > > > -intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
> > > > -			void *buffer, size_t size)
> > > > -{
> > > > -	ssize_t ret;
> > > > -	int i;
> > > > -
> > > > -	/*
> > > > -	 * Sometime we just get the same incorrect byte repeated
> > > > -	 * over the entire buffer. Doing just one throw away read
> > > > -	 * initially seems to "solve" it.
> > > > -	 */
> > > > -	drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
> > > 
> > > NAK
> > > 
> > > If people keep intentionally breaking my shit I'm going to become
> > > really grumpy soon.
> > 
> > Oh, and just in case someone wants to come up with a better kludge,
> > I just spent a few minutes analyzing the behavior of this crappy
> > monitor a.
> > 
> > What happens is that when the monitor is fully powered up (LED is blue)
> > things are fine. After the monitor goes to sleep (LED turns orange)
> > the first DPCD read will produce garbage. Further DPCD reads are fine,
> > even if I wait a significant amount of time between the reads, as long
> > as the monitor didn't do a power on->off cycle in between. So it looks
> > like it's always just the first read after power down that gets
> > corrupted.
> > 
> > Now I think I'll go and test how writes behave, assuming I can find a
> > decently sized chunk of DPCD address space I can write. And maybe I
> > should also try i2c-over-aux...
> 
> The first DPCD write after powerdown also got corrupted. But i2c-over-aux
> seems unaffected for whatever reason.

Do you have an amd card nearby to test there? Would be interesting to
confirm that this is indeed a sink bug, and hence that it really all
should be in the shared code.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1360848 — Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()

FromVille Syrjälä <ville.syrjala@linux.intel.com>
Date2016-03-18 19:10 +0100
SubjectRe: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()
Message-ID<re70u-49j-7@gated-at.bofh.it>
In reply to#1360843
On Fri, Mar 18, 2016 at 07:00:29PM +0100, Daniel Vetter wrote:
> On Fri, Mar 18, 2016 at 06:41:40PM +0200, Ville Syrjälä wrote:
> > On Fri, Mar 18, 2016 at 06:12:35PM +0200, Ville Syrjälä wrote:
> > > On Fri, Mar 18, 2016 at 04:13:45PM +0200, Ville Syrjälä wrote:
> > > > On Thu, Mar 17, 2016 at 11:40:45AM -0400, Lyude wrote:
> > > > > Since we've fixed up drm_dp_dpcd_read() to allow for retries when things
> > > > > timeout, there's no use for having this function anymore. Good riddens.
> > > > > 
> > > > > Signed-off-by: Lyude <cpaul@redhat.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_dp.c | 79 ++++++++++++-----------------------------
> > > > >  1 file changed, 22 insertions(+), 57 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index cdc2c15..fb4cbbe5 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -3190,47 +3190,14 @@ static void chv_dp_post_pll_disable(struct intel_encoder *encoder)
> > > > >  }
> > > > >  
> > > > >  /*
> > > > > - * Native read with retry for link status and receiver capability reads for
> > > > > - * cases where the sink may still be asleep.
> > > > > - *
> > > > > - * Sinks are *supposed* to come up within 1ms from an off state, but we're also
> > > > > - * supposed to retry 3 times per the spec.
> > > > > - */
> > > > > -static ssize_t
> > > > > -intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
> > > > > -			void *buffer, size_t size)
> > > > > -{
> > > > > -	ssize_t ret;
> > > > > -	int i;
> > > > > -
> > > > > -	/*
> > > > > -	 * Sometime we just get the same incorrect byte repeated
> > > > > -	 * over the entire buffer. Doing just one throw away read
> > > > > -	 * initially seems to "solve" it.
> > > > > -	 */
> > > > > -	drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
> > > > 
> > > > NAK
> > > > 
> > > > If people keep intentionally breaking my shit I'm going to become
> > > > really grumpy soon.
> > > 
> > > Oh, and just in case someone wants to come up with a better kludge,
> > > I just spent a few minutes analyzing the behavior of this crappy
> > > monitor a.
> > > 
> > > What happens is that when the monitor is fully powered up (LED is blue)
> > > things are fine. After the monitor goes to sleep (LED turns orange)
> > > the first DPCD read will produce garbage. Further DPCD reads are fine,
> > > even if I wait a significant amount of time between the reads, as long
> > > as the monitor didn't do a power on->off cycle in between. So it looks
> > > like it's always just the first read after power down that gets
> > > corrupted.
> > > 
> > > Now I think I'll go and test how writes behave, assuming I can find a
> > > decently sized chunk of DPCD address space I can write. And maybe I
> > > should also try i2c-over-aux...
> > 
> > The first DPCD write after powerdown also got corrupted. But i2c-over-aux
> > seems unaffected for whatever reason.
> 
> Do you have an amd card nearby to test there?

Nope.

> Would be interesting to
> confirm that this is indeed a sink bug, and hence that it really all
> should be in the shared code.
> -Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [next] | [standalone]

#1361985 — Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()

FromLyude Paul <cpaul@redhat.com>
Date2016-03-21 17:40 +0100
SubjectRe: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()
Message-ID<rfb21-8iP-19@gated-at.bofh.it>
In reply to#1360848
On Fri, 2016-03-18 at 20:05 +0200, Ville Syrjälä wrote:
> On Fri, Mar 18, 2016 at 07:00:29PM +0100, Daniel Vetter wrote:
> > 
> > On Fri, Mar 18, 2016 at 06:41:40PM +0200, Ville Syrjälä wrote:
> > > 
> > > On Fri, Mar 18, 2016 at 06:12:35PM +0200, Ville Syrjälä wrote:
> > > > 
> > > > On Fri, Mar 18, 2016 at 04:13:45PM +0200, Ville Syrjälä wrote:
> > > > > 
> > > > > On Thu, Mar 17, 2016 at 11:40:45AM -0400, Lyude wrote:
> > > > > > 
> > > > > > Since we've fixed up drm_dp_dpcd_read() to allow for retries when
> > > > > > things
> > > > > > timeout, there's no use for having this function anymore. Good
> > > > > > riddens.
> > > > > > 
> > > > > > Signed-off-by: Lyude <cpaul@redhat.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_dp.c | 79 ++++++++++++----------------
> > > > > > -------------
> > > > > >  1 file changed, 22 insertions(+), 57 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > index cdc2c15..fb4cbbe5 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > @@ -3190,47 +3190,14 @@ static void chv_dp_post_pll_disable(struct
> > > > > > intel_encoder *encoder)
> > > > > >  }
> > > > > >  
> > > > > >  /*
> > > > > > - * Native read with retry for link status and receiver capability
> > > > > > reads for
> > > > > > - * cases where the sink may still be asleep.
> > > > > > - *
> > > > > > - * Sinks are *supposed* to come up within 1ms from an off state,
> > > > > > but we're also
> > > > > > - * supposed to retry 3 times per the spec.
> > > > > > - */
> > > > > > -static ssize_t
> > > > > > -intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int
> > > > > > offset,
> > > > > > -			void *buffer, size_t size)
> > > > > > -{
> > > > > > -	ssize_t ret;
> > > > > > -	int i;
> > > > > > -
> > > > > > -	/*
> > > > > > -	 * Sometime we just get the same incorrect byte repeated
> > > > > > -	 * over the entire buffer. Doing just one throw away read
> > > > > > -	 * initially seems to "solve" it.
> > > > > > -	 */
> > > > > > -	drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
> > > > > NAK
> > > > > 
> > > > > If people keep intentionally breaking my shit I'm going to become
> > > > > really grumpy soon.
> > > > Oh, and just in case someone wants to come up with a better kludge,
> > > > I just spent a few minutes analyzing the behavior of this crappy
> > > > monitor a.
> > > > 
> > > > What happens is that when the monitor is fully powered up (LED is blue)
> > > > things are fine. After the monitor goes to sleep (LED turns orange)
> > > > the first DPCD read will produce garbage. Further DPCD reads are fine,
> > > > even if I wait a significant amount of time between the reads, as long
> > > > as the monitor didn't do a power on->off cycle in between. So it looks
> > > > like it's always just the first read after power down that gets
> > > > corrupted.
> > > > 
> > > > Now I think I'll go and test how writes behave, assuming I can find a
> > > > decently sized chunk of DPCD address space I can write. And maybe I
> > > > should also try i2c-over-aux...
> > > The first DPCD write after powerdown also got corrupted. But i2c-over-aux
> > > seems unaffected for whatever reason.
> > Do you have an amd card nearby to test there?
> Nope.
I also work on radeon so I have plenty. The issue with that though is that this
is an issue that only really happens with the thinkpad docks, and as far as I'm
aware we don't have any ThinkPads that have AMD hardware in them…
> > 
> > Would be interesting to
> > confirm that this is indeed a sink bug, and hence that it really all
> > should be in the shared code.
> > -Daniel
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1361687 — Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()

FromJani Nikula <jani.nikula@linux.intel.com>
Date2016-03-21 11:40 +0100
SubjectRe: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()
Message-ID<rf5pE-4hd-15@gated-at.bofh.it>
In reply to#1360782
On Fri, 18 Mar 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Mar 18, 2016 at 06:12:35PM +0200, Ville Syrjälä wrote:
>> On Fri, Mar 18, 2016 at 04:13:45PM +0200, Ville Syrjälä wrote:
>> > On Thu, Mar 17, 2016 at 11:40:45AM -0400, Lyude wrote:
>> > > -	drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
>> > 
>> > NAK
>> > 
>> > If people keep intentionally breaking my shit I'm going to become
>> > really grumpy soon.
>> 
>> Oh, and just in case someone wants to come up with a better kludge,
>> I just spent a few minutes analyzing the behavior of this crappy
>> monitor a.
>> 
>> What happens is that when the monitor is fully powered up (LED is blue)
>> things are fine. After the monitor goes to sleep (LED turns orange)
>> the first DPCD read will produce garbage. Further DPCD reads are fine,
>> even if I wait a significant amount of time between the reads, as long
>> as the monitor didn't do a power on->off cycle in between. So it looks
>> like it's always just the first read after power down that gets
>> corrupted.
>> 
>> Now I think I'll go and test how writes behave, assuming I can find a
>> decently sized chunk of DPCD address space I can write. And maybe I
>> should also try i2c-over-aux...
>
> The first DPCD write after powerdown also got corrupted. But i2c-over-aux
> seems unaffected for whatever reason.

Did the display go to sleep on its own, or did we do something? In
particular, does DPCD DP_SET_POWER register play a role? What if we skip
writing D3 to it? What if we do that write as the first thing (every
time)?


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center

[toc] | [prev] | [next] | [standalone]

#1361840 — Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()

FromVille Syrjälä <ville.syrjala@linux.intel.com>
Date2016-03-21 14:40 +0100
SubjectRe: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()
Message-ID<rf8dQ-6jL-7@gated-at.bofh.it>
In reply to#1361687
On Mon, Mar 21, 2016 at 12:30:54PM +0200, Jani Nikula wrote:
> On Fri, 18 Mar 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Mar 18, 2016 at 06:12:35PM +0200, Ville Syrjälä wrote:
> >> On Fri, Mar 18, 2016 at 04:13:45PM +0200, Ville Syrjälä wrote:
> >> > On Thu, Mar 17, 2016 at 11:40:45AM -0400, Lyude wrote:
> >> > > -	drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
> >> > 
> >> > NAK
> >> > 
> >> > If people keep intentionally breaking my shit I'm going to become
> >> > really grumpy soon.
> >> 
> >> Oh, and just in case someone wants to come up with a better kludge,
> >> I just spent a few minutes analyzing the behavior of this crappy
> >> monitor a.
> >> 
> >> What happens is that when the monitor is fully powered up (LED is blue)
> >> things are fine. After the monitor goes to sleep (LED turns orange)
> >> the first DPCD read will produce garbage. Further DPCD reads are fine,
> >> even if I wait a significant amount of time between the reads, as long
> >> as the monitor didn't do a power on->off cycle in between. So it looks
> >> like it's always just the first read after power down that gets
> >> corrupted.
> >> 
> >> Now I think I'll go and test how writes behave, assuming I can find a
> >> decently sized chunk of DPCD address space I can write. And maybe I
> >> should also try i2c-over-aux...
> >
> > The first DPCD write after powerdown also got corrupted. But i2c-over-aux
> > seems unaffected for whatever reason.
> 
> Did the display go to sleep on its own, or did we do something? In
> particular, does DPCD DP_SET_POWER register play a role? What if we skip
> writing D3 to it? What if we do that write as the first thing (every
> time)?

User pressing any of the buttons on the monitor is enough to wake it,
and after a short timeout it will power down on its own, leading to
the corrupted access.

Keeping DP_SET_POWER at D0 doesn't change anything.

-- 
Ville Syrjälä
Intel OTC

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web