[PATCH 0/9] Organize aux retries v3

[PATCH 0/9] Organize aux retries v3

[Rodrigo Vivi]

This new version first convert the drm_dp_aux doc to new per-member comment layout
and introduce a propper documentation for EBUSY/EAGAIN retry cases, as requested by
Daniel.

Also accept many suggestions from Jani, but mostly removes the Nacked patch and
introduce a new one to handle the missed case when killing intel_dp_dpcd_read_wake.
Ideally I believe we could and should kill that retry, but it needs to be done
in a separated patch and ideally by someone that can reproduce the old issue.

But with this series at least we have some consistency in the code and almost
all retries stay organized at drm level.

But by far the most important patch in this series for myself is
commit ("drm/i915: Fix random aux transactions failures.") who solves
the issues that I face on PSR validation on Skylake platforms.
So if the rest of the series still needs to be discussed, reworked or anything
else, please consider at least merging this patch at least that by itself
already solves my main issue.

Thank you all for all reviews, comments and test so far.

Rodrigo Vivi (9):
  drm: Improve drm_dp_aux DocBook.
  drm: Introduce EAGAIN handling for immediatelly aux retries
  drm/nouveau: Use EAGAIN instead EBUSY for aux retry.
  drm/i915: Use EAGAIN instead EBUSY for aux retry.
  drm: Wait 1ms before retrying aux transactions on EBUSY.
  drm/i915: Avoid EBUSY retry on intel_dp_aux_ch.
  drm/i915: Fix random aux transactions failures.
  drm/i915: Move dummy aux read from read_wake to aux_transfer.
  drm/i915: Kill intel_dp_dpcd_read_wake

 drivers/gpu/drm/drm_dp_helper.c                    |  18 ++-
 drivers/gpu/drm/i915/intel_dp.c                    | 141 +++++++++------------
 drivers/gpu/drm/nouveau/nvkm/subdev/i2c/auxg94.c   |   4 +-
 drivers/gpu/drm/nouveau/nvkm/subdev/i2c/auxgm204.c |   4 +-
 include/drm/drm_dp_helper.h                        |  69 ++++++----
 5 files changed, 126 insertions(+), 110 deletions(-)

-- 
2.4.3

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

[PATCH 1/9] drm: Improve drm_dp_aux DocBook.

[Rodrigo Vivi]

This patch converts drm_dp_aux doc to new per-member comment layout
that 4.4 supports as suggested by Daniel.

But also:

1. to let the split text with sense this patch also
introduce a brief general AUX channel definition.

2. Remove .name and .dev duplications from the original text.

3. Improve .transfer() error code handler making more clear
   what is going on with each error code generated or handled

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 include/drm/drm_dp_helper.h | 64 +++++++++++++++++++++++++++++----------------
 1 file changed, 42 insertions(+), 22 deletions(-)

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 1252108..ed7dbdc 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -703,26 +703,9 @@ struct drm_dp_aux_msg {
 
 /**
  * struct drm_dp_aux - DisplayPort AUX channel
- * @name: user-visible name of this AUX channel and the I2C-over-AUX adapter
- * @ddc: I2C adapter that can be used for I2C-over-AUX communication
- * @dev: pointer to struct device that is the parent for this AUX channel
- * @hw_mutex: internal mutex used for locking transfers
- * @transfer: transfers a message representing a single AUX transaction
  *
- * The .dev field should be set to a pointer to the device that implements
- * the AUX channel.
- *
- * The .name field may be used to specify the name of the I2C adapter. If set to
- * NULL, dev_name() of .dev will be used.
- *
- * Drivers provide a hardware-specific implementation of how transactions
- * are executed via the .transfer() function. A pointer to a drm_dp_aux_msg
- * structure describing the transaction is passed into this function. Upon
- * success, the implementation should return the number of payload bytes
- * that were transferred, or a negative error-code on failure. Helpers
- * propagate errors from the .transfer() function, with the exception of
- * the -EBUSY error, which causes a transaction to be retried. On a short,
- * helpers will return -EPROTO to make it simpler to check for failure.
+ * AUX channel is a half duplex channel used to transfer messages between
+ * source an sink. Mainly used for write and read sink DPCD registers.
  *
  * An AUX channel can also be used to transport I2C messages to a sink. A
  * typical application of that is to access an EDID that's present in the
@@ -734,15 +717,52 @@ struct drm_dp_aux_msg {
  * received, the adapter will drop down to the size given by the partial
  * response for this transaction only.
  *
- * Note that the aux helper code assumes that the .transfer() function
- * only modifies the reply field of the drm_dp_aux_msg structure.  The
- * retry logic and i2c helpers assume this is the case.
  */
 struct drm_dp_aux {
+	/**
+	 * @name: user-visible name of this AUX channel and the I2C-over-AUX
+	 *        adapter. If set to NULL, dev_name() of .dev will be used.
+	 */
 	const char *name;
+
+	/**
+	 * @ddc: I2C adapter that can be used for I2C-over-AUX communication
+	 */
 	struct i2c_adapter ddc;
+
+	/**
+	 * @dev: pointer to struct device that is the parent and implements
+	 *       this AUX channel
+	 */
 	struct device *dev;
+
+	/**
+	 * @hw_mutex: internal mutex used for locking transfers
+	 */
 	struct mutex hw_mutex;
+
+	/**
+	 * @transfer: transfers a message representing a single AUX transaction
+	 *
+	 * Drivers provide a hardware-specific implementation of how
+	 * transactions are executed via this function. A pointer to a
+	 * drm_dp_aux_msg structure describing the transaction is passed into
+	 * this function.
+	 *
+	 * Returns:
+	 * Upon success, the implementation should return the number of
+	 * payload bytes that were transferred.
+	 * Otherwise a negative error-code on failure. Helpers propagate errors
+	 * from the .transfer() function, with the exception of:
+	 * - -EPROTO: On a short, helpers will return -EPROTO to make it simpler
+	 *   to check for failure.
+	 * - -EBUSY: When BUSY helpers will attempt retries before propagating
+	 *   this error.
+	 *
+	 * Note that the aux helper code assumes that the .transfer() function
+	 * only modifies the reply field of the drm_dp_aux_msg structure.  The
+	 * retry logic and i2c helpers assume this is the case.
+	 */
 	ssize_t (*transfer)(struct drm_dp_aux *aux,
 			    struct drm_dp_aux_msg *msg);
 	unsigned i2c_nack_count, i2c_defer_count;
-- 
2.4.3

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

[PATCH 2/9] drm: Introduce EAGAIN handling for immediatelly aux retries

[Rodrigo Vivi]

The goal here is to offload aux retries handling from the
drivers to drm.

However there are 2 differents cases for retry:
1. Immediately retry - Hardware already took care of needed timeouts
   	       	       before answering that a retry is possible.
2. Busy - Wait some time and retry.

This driver introduce the support for EAGAIN that can handle the
first case and a following patch will introduce EBUSY waits,
after all drivers counting on immediately retries are changed.

v2: Rebased on top of drm_dp_helper docbook changes.
    Also this version add EAGAIN documentation and as suggested by
    Jani it inserts a comment at drm_dp_i2c_do_msg explaining EAGAIN
    timeout is already handled at i2c level.

Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Dave Airlie <airlied@redhat.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Tested-by: Daniel Stone <daniels@collabora.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/drm_dp_helper.c | 10 ++++++++++
 include/drm/drm_dp_helper.h     |  4 ++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 9535c5b..2e26097 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -198,6 +198,11 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
 		err = aux->transfer(aux, &msg);
 		mutex_unlock(&aux->hw_mutex);
 		if (err < 0) {
+			/* Immediately retry */
+			if (err == -EAGAIN)
+				continue;
+
+			/* FIXME: On BUSY we could wait before retrying */
 			if (err == -EBUSY)
 				continue;
 
@@ -547,6 +552,11 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 		ret = aux->transfer(aux, msg);
 		mutex_unlock(&aux->hw_mutex);
 		if (ret < 0) {
+
+			/*
+			 * -EAGAIN retries are handled by i2c layer with
+			 * timeouts for repeated -EAGAINs
+			 */
 			if (ret == -EBUSY)
 				continue;
 
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index ed7dbdc..518fc1b 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -758,6 +758,10 @@ struct drm_dp_aux {
 	 *   to check for failure.
 	 * - -EBUSY: When BUSY helpers will attempt retries before propagating
 	 *   this error.
+	 * - -EAGAIN: DPCD helper will attempt immediatelly retries before
+	 *   propagating this error. i2c helper will only propagate directly
+	 *   since i2c layer already perform retries with timeouts for repeated
+	 *   -EAGAINs.
 	 *
 	 * Note that the aux helper code assumes that the .transfer() function
 	 * only modifies the reply field of the drm_dp_aux_msg structure.  The
-- 
2.4.3

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

[PATCH 3/9] drm/nouveau: Use EAGAIN instead EBUSY for aux retry.

[Rodrigo Vivi]

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>
---
 drivers/gpu/drm/nouveau/nvkm/subdev/i2c/auxg94.c   | 4 ++--
 drivers/gpu/drm/nouveau/nvkm/subdev/i2c/auxgm204.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/auxg94.c b/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/auxg94.c
index 954f5b7..a6cd729 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/auxg94.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/auxg94.c
@@ -52,7 +52,7 @@ g94_i2c_aux_init(struct g94_i2c_aux *aux)
 		udelay(1);
 		if (!timeout--) {
 			AUX_ERR(&aux->base, "begin idle timeout %08x", ctrl);
-			return -EBUSY;
+			return -EAGAIN;
 		}
 	} while (ctrl & 0x03010000);
 
@@ -65,7 +65,7 @@ g94_i2c_aux_init(struct g94_i2c_aux *aux)
 		if (!timeout--) {
 			AUX_ERR(&aux->base, "magic wait %08x", ctrl);
 			g94_i2c_aux_fini(aux);
-			return -EBUSY;
+			return -EAGAIN;
 		}
 	} while ((ctrl & 0x03000000) != urep);
 
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/auxgm204.c b/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/auxgm204.c
index bed231b..6814e5b 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/auxgm204.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/i2c/auxgm204.c
@@ -52,7 +52,7 @@ gm204_i2c_aux_init(struct gm204_i2c_aux *aux)
 		udelay(1);
 		if (!timeout--) {
 			AUX_ERR(&aux->base, "begin idle timeout %08x", ctrl);
-			return -EBUSY;
+			return -EAGAIN;
 		}
 	} while (ctrl & 0x03010000);
 
@@ -65,7 +65,7 @@ gm204_i2c_aux_init(struct gm204_i2c_aux *aux)
 		if (!timeout--) {
 			AUX_ERR(&aux->base, "magic wait %08x", ctrl);
 			gm204_i2c_aux_fini(aux);
-			return -EBUSY;
+			return -EAGAIN;
 		}
 	} while ((ctrl & 0x03000000) != urep);
 
-- 
2.4.3

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

[PATCH 4/9] drm/i915: Use EAGAIN instead EBUSY for aux retry.

[Rodrigo Vivi]

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>
---
 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 2805f0d..1261d26 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

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

[PATCH 5/9] drm: Wait 1ms before retrying aux transactions on EBUSY.

[Rodrigo Vivi]

DP Specs isn't really clear about the amount of retries,
but for cases it mentions retries it also mention times that
vary from 300us to 1ms.

For many cases hardware can handled the timeouts before retry
is possible and allowed, but for many other cases it is better
to wait and give time so the aux channels can recover.

For instance one general case there is when downstream device
is waking up from sleep states generating HPD so it might take
up to 1ms before getting responsive.

I believe with this msleep we could minimize the 32 times retries
and still let Dell monitors happy, but I don't have this monitor
to test here so let's just add the sleep for now and still retry
32 times.

v2: Include doc comment.
    Also apply the 1ms to i2c helper.
    Use usleep_range for better precision as suggested by Jani.

Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Dave Airlie <airlied@redhat.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Tested-by: Daniel Stone <daniels@collabora.com> # v1 on SKL
---
 drivers/gpu/drm/drm_dp_helper.c | 10 +++++++---
 include/drm/drm_dp_helper.h     |  3 ++-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 2e26097..e328715 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -202,9 +202,11 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
 			if (err == -EAGAIN)
 				continue;
 
-			/* FIXME: On BUSY we could wait before retrying */
-			if (err == -EBUSY)
+			/* Give a time for aux channels to recover */
+			if (err == -EBUSY) {
+				usleep_range(1000, 1000);
 				continue;
+			}
 
 			return err;
 		}
@@ -557,8 +559,10 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 			 * -EAGAIN retries are handled by i2c layer with
 			 * timeouts for repeated -EAGAINs
 			 */
-			if (ret == -EBUSY)
+			if (ret == -EBUSY) {
+				usleep_range(1000, 1000);
 				continue;
+			}
 
 			DRM_DEBUG_KMS("transaction failed: %d\n", ret);
 			return ret;
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 518fc1b..ac9de5e 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -757,7 +757,8 @@ struct drm_dp_aux {
 	 * - -EPROTO: On a short, helpers will return -EPROTO to make it simpler
 	 *   to check for failure.
 	 * - -EBUSY: When BUSY helpers will attempt retries before propagating
-	 *   this error.
+	 *   this error, with 1ms between attempts to give a time for downstream
+	 *   devices to recover.
 	 * - -EAGAIN: DPCD helper will attempt immediatelly retries before
 	 *   propagating this error. i2c helper will only propagate directly
 	 *   since i2c layer already perform retries with timeouts for repeated
-- 
2.4.3

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

[PATCH 6/9] drm/i915: Avoid EBUSY retry on intel_dp_aux_ch.

[Rodrigo Vivi]

EBUSY retries are already in place at drm level.
We don't need to replicate the job here.

v2: rebase on top of EAGAIN x EBUSY retries change at drm.
    EBUSY retry at DRM is now handling the msleep(1).

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1261d26..54e85f5 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -817,25 +817,9 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 
 	intel_dp_check_edp(intel_dp);
 
-	/* Try to wait for any previous AUX channel activity */
-	for (try = 0; try < 3; try++) {
-		status = I915_READ_NOTRACE(ch_ctl);
-		if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0)
-			break;
-		msleep(1);
-	}
-
-	if (try == 3) {
-		static u32 last_status = -1;
-		const u32 status = I915_READ(ch_ctl);
-
-		if (status != last_status) {
-			WARN(1, "dp_aux_ch not started status 0x%08x\n",
-			     status);
-			last_status = status;
-		}
-
-		ret = -EAGAIN;
+	status = I915_READ_NOTRACE(ch_ctl);
+	if ((status & DP_AUX_CH_CTL_SEND_BUSY) != 0) {
+		ret = -EBUSY;
 		goto out;
 	}
 
-- 
2.4.3

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

[PATCH 7/9] drm/i915: Fix random aux transactions failures.

[Rodrigo Vivi]

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.

v2: Print debug messsage when this case is reached as suggested
    by Jani.

Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Tested-by: Daniel Stone <daniels@collabora.com> # SKL
---
 drivers/gpu/drm/i915/intel_dp.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 54e85f5..a02bfa1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -899,6 +899,19 @@ 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) {
+		DRM_DEBUG_KMS("Forbidden recv_bytes = %d on aux transaction\n",
+			      recv_bytes);
+		ret = -EBUSY;
+		goto out;
+	}
+
 	if (recv_bytes > recv_size)
 		recv_bytes = recv_size;
 
-- 
2.4.3

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

[PATCH 8/9] drm/i915: Move dummy aux read from read_wake to aux_transfer.

[Rodrigo Vivi]

The goal is to kill the intel_dp_dpcd_read_wake for all, however
Jani noticed we cannot ignore the case introduced by:
'commit f6a1906 ("drm/i915: Do a dummy DPCD read before the actual read")'

So, instead for removing this for now let's apply that dummy read
to all DP_AUX_NATIVE_READ cases. Pratically no functional changes with
this change, but ideally we want to remove this code for here.

Unfortunately I don't have the monitor/hardware that made us
to include this extra call so for now let's move this here and
add a FIXME tag so this case can be properly fixed/verified later.

An alternative plan is to remove completely this piece of code and when
we start getting the corner cases again we investigate it properly to
see if instead of this extra read we can simply handle properly or
return a -EBUSY or -EAGAIN so drm can retry instead.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a02bfa1..0febf8d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -937,7 +937,7 @@ static ssize_t
 intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 {
 	struct intel_dp *intel_dp = container_of(aux, struct intel_dp, aux);
-	uint8_t txbuf[20], rxbuf[20];
+	uint8_t txbuf[20], rxbuf[20], dumbuf[20];
 	size_t txsize, rxsize;
 	int ret;
 
@@ -974,6 +974,18 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
 		break;
 
 	case DP_AUX_NATIVE_READ:
+		/*
+		 * FIXME: Sometime we just get the same incorrect byte repeated
+		 * over the entire buffer. Doing just one throw away read
+		 * initially seems to "solve" it.
+		 */
+		dumbuf[0] = (DP_AUX_NATIVE_READ << 4) |
+			((DP_DPCD_REV >> 16) & 0xf);
+		dumbuf[1] = (DP_DPCD_REV >> 8) & 0xff;
+		dumbuf[2] = DP_DPCD_REV & 0xff;
+		dumbuf[3] = 0;
+		intel_dp_aux_ch(intel_dp, dumbuf, HEADER_SIZE, rxbuf, 2);
+
 	case DP_AUX_I2C_READ:
 		txsize = msg->size ? HEADER_SIZE : BARE_ADDRESS_SIZE;
 		rxsize = msg->size + 1;
@@ -3171,13 +3183,6 @@ intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
 	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)
-- 
2.4.3

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

[PATCH 9/9] drm/i915: Kill intel_dp_dpcd_read_wake

[Rodrigo Vivi]

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")

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.

v3: Rebase on top of new commit ("drm/i915: Move dummy aux read from
read_wake to aux_transfer."). Jani noticed that previous version was
ignoring a case fixed by 'commit f6a1906 ("drm/i915: Do a dummy DPCD
 read before the actual read")'.

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 | 85 +++++++++++++++--------------------------
 1 file changed, 30 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 0febf8d..f305ecb 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3170,40 +3170,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;
-
-	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. */
@@ -3838,8 +3814,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);
@@ -3850,9 +3826,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");
@@ -3863,9 +3839,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;
@@ -3881,15 +3857,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]);
@@ -3912,9 +3888,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;
@@ -3928,11 +3904,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]);
 }
@@ -3948,7 +3924,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;
@@ -4085,9 +4061,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
@@ -4095,9 +4071,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;
 
@@ -4356,8 +4332,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
-- 
2.4.3

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

Re: [PATCH 7/9] drm/i915: Fix random aux transactions failures.

[Rodrigo Vivi]

[-- Attachment #1.1: Type: text/plain, Size: 2791 bytes --]

Hi Jani,

Is this version ok now?
Do you believe we could go only with this patch for now and continue the
retry and kill read_wake function later?

Thanks,
Rodrigo.

On Wed, Nov 25, 2015 at 4:04 PM 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.
>
> v2: Print debug messsage when this case is reached as suggested
>     by Jani.
>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Tested-by: Daniel Stone <daniels@collabora.com> # SKL
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> index 54e85f5..a02bfa1 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -899,6 +899,19 @@ 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) {
> +               DRM_DEBUG_KMS("Forbidden recv_bytes = %d on aux
> transaction\n",
> +                             recv_bytes);
> +               ret = -EBUSY;
> +               goto out;
> +       }
> +
>         if (recv_bytes > recv_size)
>                 recv_bytes = recv_size;
>
> --
> 2.4.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

[-- Attachment #1.2: Type: text/html, Size: 3910 bytes --]

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

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