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(-)
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;
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
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);
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; }
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
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; }
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;
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 auxtransaction\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
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)
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, - ®, 1) < 0) + if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, ®) < 0) return connector_status_unknown;
return DP_GET_SINK_COUNT(reg) ? connector_status_connected
dri-devel@lists.freedesktop.org
-
Rodrigo Vivi -
Rodrigo Vivi