[PATCH] drm/i915: Retry on every aux read.

Rodrigo Vivi
2015-10-19T23:06:14Z

We have an inconsistency on our code on using intel_dp_dpcd_read_wake with
retries and when using drm_dp_dpcd_read helper without retry.

Since the retries help in many cases let's be consistent and be on
the safe side retrying on every aux read, including i2c ones.

So we can kill the intel_dp_dpcd_read function and keep code consistent.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 09bdd94..46f3c2d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -944,7 +944,7 @@ 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];
size_t txsize, rxsize;
- int ret;
+ int ret, i;

txbuf[0] = (msg->request << 4) |
((msg->address >> 16) & 0xf);
@@ -986,17 +986,27 @@ 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);
- if (ret > 0) {
- msg->reply = rxbuf[0] >> 4;
- /*
- * Assume happy day, and copy the data. The caller is
- * expected to check msg->reply before touching it.
- *
- * Return payload size.
- */
- ret--;
- memcpy(msg->buffer, rxbuf + 1, ret);
+ /*
+ * 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.
+ */
+ for (i = 0, ret = 0; ret <= 0 && i < 3; i++) {
+ ret = intel_dp_aux_ch(intel_dp, txbuf, txsize,
+ rxbuf, rxsize);
+ if (ret > 0) {
+ msg->reply = rxbuf[0] >> 4;
+ /*
+ * Assume happy day, and copy the data. The caller is
+ * expected to check msg->reply before touching it.
+ *
+ * Return payload size.
+ */
+ ret--;
+ memcpy(msg->buffer, rxbuf + 1, ret);
+ } else
+ msleep(1);
}
break;

@@ -3012,47 +3022,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);
-
- 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
*/
static 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. */
@@ -3979,8 +3958,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);
@@ -3991,9 +3970,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");
@@ -4004,9 +3983,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;
@@ -4022,15 +4001,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]);
@@ -4053,9 +4032,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;
@@ -4069,11 +4048,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]);
}
@@ -4089,7 +4068,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;
@@ -4228,9 +4207,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
@@ -4238,9 +4217,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;

@@ -4496,8 +4475,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