| Message ID | 1448066790-19591-9-git-send-email-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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, > - ®, 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
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); - - 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, - ®, 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
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. 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(-)