On Fri, Nov 20, 2015 at 04:46:25PM -0800, Rodrigo Vivi wrote:

Current EBUSY meaning is immediately retry, but this is about to change. DRM aux transfer is about to change and make EAGAIN the immediately retry and use EBUSY to wait a bit for aux channels to recover for any error or wake up.

This has no functional change if the EAGAIN support is in place already for drm aux transfer.

Signed-off-by: Rodrigo Vivi rodrigo.vivi@intel.com

Please document the EAGAIN and EBUSY return codes for the ->transfer member fo drm_dp_aux in the kerneldoc properly. While at it would be great if you could document any other error codes that are treated specially for this function.

Since this will be a bit more text please convert the kerneldoc to the new per-member comment layout that 4.4 supports, i.e.

/** * struct foo - foo structure * * top-level blabla */ struct foo { /** * @bar: * * Long text (with paragraphs) explaining bar. */ };

Otherwise this looks really tidy and I really like how this allows us to get rid of the intel_read_wake hack. -Daniel

---

drivers/gpu/drm/i915/intel_dp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index bec443a..ed07f0a 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -835,7 +835,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp, last_status = status; }

• ret = -EBUSY;
  

• ret = -EAGAIN;
  

  goto out; }

@@ -890,7 +890,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,

if ((status & DP_AUX_CH_CTL_DONE) == 0) { DRM_ERROR("dp_aux_ch not done status 0x%08x\n", status);

• ret = -EBUSY;
  

• ret = -EAGAIN;
  

  goto out; }

-- 2.4.3

---

dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel

On Sat, 21 Nov 2015, Rodrigo Vivi rodrigo.vivi@intel.com wrote:

drm level already takes care of the needed retries so instead of duplicate the effort here.

If the retry is possible immediately we return EAGAIN. Otherwise, if we have no idea what caused the error let's assume something was busy and let drm level handle the wait and retries.

Signed-off-by: Rodrigo Vivi rodrigo.vivi@intel.com

drivers/gpu/drm/i915/intel_dp.c | 64 ++++++++++++++++++++++------------------- 1 file changed, 35 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index a8ba243..35048d6 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -795,7 +795,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp, uint32_t aux_clock_divider; int i, ret, recv_bytes; uint32_t status;

• int try, clock = 0;

• int clock = 0; bool has_aux_irq = HAS_AUX_IRQ(dev); bool vdd;

@@ -835,41 +835,47 @@ intel_dp_aux_ch(struct intel_dp *intel_dp, send_bytes, aux_clock_divider);

• /* Must try at least 3 times according to DP spec */
  

• for (try = 0; try < 5; try++) {
  

This now inverses the retries wrt the aux clock divider retries, which probably breaks the non-ULT HSW workaround:

"WA: For the PCH Aux channels (Aux B/C/D) use an aux divider value of 63 decimal (03Fh). If there is a failure, retry at least three times with 63, then retry at least three times with 72 decimal (048h). See South Display Engine Registers, DP_AUX_CTL."

It is sad.

BR, Jani.

• 	/* Load the send data into the aux channel data registers */
  

• 	for (i = 0; i < send_bytes; i += 4)
  

• 		I915_WRITE(intel_dp->aux_ch_data_reg[i >> 2],
  

• 			   intel_dp_pack_aux(send + i,
  

• 					     send_bytes - i));
  

• /* Load the send data into the aux channel data registers */
  

• for (i = 0; i < send_bytes; i += 4)
  

• 	I915_WRITE(intel_dp->aux_ch_data_reg[i >> 2],
  

• 		   intel_dp_pack_aux(send + i,
  

• 				     send_bytes - i));
  
  

• 	/* Send the command and wait for it to complete */
  

• 	I915_WRITE(ch_ctl, send_ctl);
  

• /* Send the command and wait for it to complete */
  

• I915_WRITE(ch_ctl, send_ctl);
  
  

• 	status = intel_dp_aux_wait_done(intel_dp, has_aux_irq);
  

• status = intel_dp_aux_wait_done(intel_dp, has_aux_irq);
  
  

• 	/* Clear done status and any errors */
  

• 	I915_WRITE(ch_ctl,
  

• 		   status |
  

• 		   DP_AUX_CH_CTL_DONE |
  

• 		   DP_AUX_CH_CTL_TIME_OUT_ERROR |
  

• 		   DP_AUX_CH_CTL_RECEIVE_ERROR);
  

• /* Clear done status and any errors */
  

• I915_WRITE(ch_ctl,
  

• 	   status |
  

• 	   DP_AUX_CH_CTL_DONE |
  

• 	   DP_AUX_CH_CTL_TIME_OUT_ERROR |
  

• 	   DP_AUX_CH_CTL_RECEIVE_ERROR);
  
  

• 	if (status & DP_AUX_CH_CTL_TIME_OUT_ERROR)
  

• 		continue;
  

• if (status & DP_AUX_CH_CTL_TIME_OUT_ERROR) {
  

• 	/*
  

• 	 * DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
  

• 	 * 400us delay required for errors and timeouts
  

• 	 * Timeout errors from the HW already meet this
  

• 	 * requirement so skip to next iteration
  

• 	 */
  

• 	ret = -EAGAIN;
  

• 	goto out;
  

• }
  
  

• 	/* DP CTS 1.2 Core Rev 1.1, 4.2.1.1 & 4.2.1.2
  

• 	 *   400us delay required for errors and timeouts
  

• 	 *   Timeout errors from the HW already meet this
  

• 	 *   requirement so skip to next iteration
  

• if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
  

• 	/*
  

• 	 * We don't know what caused the error, so let's
  

• 	 * return -EBUSY so drm level takes care of
  

• 	 * the necessary wait for recover and retries
   */
  

• 	if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
  

• 		usleep_range(400, 500);
  

• 		continue;
  

• 	}
  

• 	if (status & DP_AUX_CH_CTL_DONE)
  

• 		goto done;
  

• 	ret = -EBUSY;
  

• 	goto out;
  

  }

• if (status & DP_AUX_CH_CTL_DONE)
  

• 	goto done;
  

  }

  if ((status & DP_AUX_CH_CTL_DONE) == 0) {

On Sat, 21 Nov 2015, Rodrigo Vivi rodrigo.vivi@intel.com wrote:

Mainly aux communications on sink_crc were failing a lot randomly on recent platforms. The first solution was to try to use intel_dp_dpcd_read_wake, but then it was suggested to move retries to drm level.

Since drm level was already taking care of retries and didn't want to through random retries on that level the second solution was to put the retries at aux_transfer layer what was nacked.

So I realized we had so many retries in different places and started to organize that a bit. During this organization I noticed that we weren't handing at all the case were the message size was zeroed. And this was exactly the case that was affecting sink_crc.

Also we weren't respect BSPec who says this size message = 0 or > 20 are forbidden.

It is a fact that we still have no clue why we are getting this forbidden value there. But anyway we need to handle that for now so we return -EBUSY and drm level takes care of the retries that are already in place.

Cc: Jani Nikula jani.nikula@intel.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Signed-off-by: Rodrigo Vivi rodrigo.vivi@intel.com

---

drivers/gpu/drm/i915/intel_dp.c | 11 +++++++++++ 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 35048d6..c87e937 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -905,6 +905,17 @@ done: /* Unload any bytes sent back from the other side */ recv_bytes = ((status & DP_AUX_CH_CTL_MESSAGE_SIZE_MASK) >> DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT);

• /*

• * By BSpec: "Message sizes of 0 or >20 are not allowed."
  

• * We have no idea of what happened so we return -EBUSY so
  

• * drm layer takes care for the necessary retries.
  

• */
  

• if (recv_bytes == 0 || recv_bytes > 20) {

• ret = -EBUSY;
  

This deserves debug logging at the very least.

BR, Jani.

• goto out;
  

• }
• if (recv_bytes > recv_size) recv_bytes = recv_size;

I don't see how the subject matches the commit.

On Sat, 21 Nov 2015, Rodrigo Vivi rodrigo.vivi@intel.com wrote:

This read wake with retries were initially added by 2 commits:

commit 61da5fab ("drm/i915/dp: retry link status read 3 times on failure") commit 899526d9 ("drm/i915/dp: try to read receiver capabilities 3 times when detecting")

Both mentioning section 9.1 of the 1.1a DisplayPort spec, that actually tell us to retry three times on certain case when "writing 01h to DPCD address 600h" and this code is already in place in our driver. Added by:

commit c7ad3810 ("drm/i915/dp: manage sink power state if possible")

I still think what we currently do for the sink power state management works by coincidence. We should still look into it.

However, I think this series overall (apart from patch 6/8 which really is a bummer, the comment inline below, and the minor other comments) looks like worthwhile changes. We can leave the power state management for later. Or rip it out for now...

At this point we have no visibility if those patches were added to workaround certain corner cases like lazy dongles or what, but also at that time there wasn't enough retries on the proper places.

So my proposal is to remove these retries for now that we have drm handling the retries and if we face any corner case back again we study it to return EAGAIN or EBUSY to force retries at drm instead of handling them here.

v2: Improve commit message trying to explain the origin of the retries.

Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Jani Nikula jani.nikula@intel.com Cc: Jesse Barnes jbarnes@virtuousgeek.org Signed-off-by: Rodrigo Vivi rodrigo.vivi@intel.com

---

drivers/gpu/drm/i915/intel_dp.c | 95 ++++++++++++++--------------------------- 1 file changed, 32 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index c87e937..2ce6527 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -985,7 +985,8 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg) if (WARN_ON(rxsize > 20)) return -E2BIG;

• ret = intel_dp_aux_ch(intel_dp, txbuf, txsize, rxbuf, rxsize);
  

• ret = intel_dp_aux_ch(intel_dp, txbuf, txsize,
  

• 		      rxbuf, rxsize);
  

  if (ret > 0) { msg->reply = rxbuf[0] >> 4; /*

@@ -3150,47 +3151,16 @@ 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);

This still needs to be addressed somehow. Maybe it's sufficient for Ville to test with his monitor?

commit f6a1906674005377b64ee5431c1418077c1b2425 Author: Ville Syrjälä ville.syrjala@linux.intel.com Date: Thu Oct 16 20:46:09 2014 +0300

drm/i915: Do a dummy DPCD read before the actual read

• 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. */ @@ -3825,8 +3795,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);

@@ -3837,9 +3807,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");

@@ -3850,9 +3820,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_readb(&intel_dp->aux,
  

• 			  DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
  

• 			  &frame_sync_cap);
  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;
  

@@ -3868,15 +3838,15 @@ 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_readb(&intel_dp->aux, DP_EDP_DPCD_REV, &rev) == 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]);

@@ -3899,9 +3869,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;

@@ -3915,11 +3885,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]);

} @@ -3935,7 +3905,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_readb(&intel_dp->aux, DP_MSTM_CAP, buf)) { if (buf[0] & DP_MST_CAP) { DRM_DEBUG_KMS("Sink is MST capable\n"); intel_dp->is_mst = true;

@@ -4072,9 +4042,9 @@ 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,

• 		       DP_DEVICE_SERVICE_IRQ_VECTOR,
  

• 		       sink_irq_vector, 1) == 1;
  

• return drm_dp_dpcd_readb(&intel_dp->aux,

• 		 DP_DEVICE_SERVICE_IRQ_VECTOR,
  

• 		 sink_irq_vector) == 1;
  

}

static bool @@ -4082,9 +4052,9 @@ 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,

• 			     DP_SINK_COUNT_ESI,
  

• 			     sink_irq_vector, 14);
  

• ret = drm_dp_dpcd_read(&intel_dp->aux,

• 	       DP_SINK_COUNT_ESI,
  

• 	       sink_irq_vector, 14);
  

  if (ret != 14) return false;

@@ -4343,8 +4313,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,
  

• 			    &reg, 1) < 0)
  

• if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &reg) < 0)
  return connector_status_unknown;
  
  

  return DP_GET_SINK_COUNT(reg) ? connector_status_connected