2016-03-17 15:41:01

by Lyude Paul

Subject: [PATCH 1/2] drm/dp_helper: retry on -ETIMEDOUT in drm_dp_dpcd_access()

Some sinks need some time during the process of resuming the system from
sleep before they're ready to handle transactions. While it would be
nice if they responded with NACKs in these scenarios, this isn't always
the case as a few sinks will just timeout on all of the transactions
they receive.

This patch was originally intended to be a workaround for a very
mysterious bug on the T560, where any monitors connected to the dock
would fail to turn back on after resume. When resuming the laptop, it
appears that there's a short period of time where we're unable to
complete any aux transactions, as they all immediately timeout. The only
machine I'm able to reproduce this on is the T560 as other production
Skylake models seem to be fine. The period during which AUX transactions
fail appears to be around 22ms long. AFAIK, the dock for the T560 never
actually turns off, the only difference is that it's in SST mode at the
start of the resume process, so it's unclear as to why it would need so
much time to come back up.

There's been a discussion on this issue going on for a while on the
intel-gfx mailing list about this that has, in addition to including
developers from Intel, also had the correspondence of one of the
hardware engineers for Intel:

http://www.spinics.net/lists/intel-gfx/msg88831.html
http://www.spinics.net/lists/intel-gfx/msg88410.html

We've already looked into a couple of possible explanations for the
problem:

- Calling intel_dp_mst_resume() before right fix.
intel_runtime_pm_enable_interrupts(). This was the first fix I tried,
and while it worked it definitely wasn't the right fix. This worked
because DP aux transactions don't actually require interrupts to work:

static uint32_t
intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
{
struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
struct drm_device *dev = intel_dig_port->base.base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg;
uint32_t status;
bool done;

#define C (((status = I915_READ_NOTRACE(ch_ctl)) & DP_AUX_CH_CTL_SEND_BUSY) == 0)
if (has_aux_irq)
done = wait_event_timeout(dev_priv->gmbus_wait_queue, C,
msecs_to_jiffies_timeout(10));
else
done = wait_for_atomic(C, 10) == 0;
if (!done)
DRM_ERROR("dp aux hw did not signal timeout (has irq: %i)!\n",
has_aux_irq);
#undef C

return status;
}

When there's no interrupts enabled, we end up timing out on the
wait_event_timeout() call, which causes us to check the DP status
register once to see if the transaction was successful or not. Since
this adds a 10ms delay to each aux transaction, it ends up adding a
long enough delay to the resume process for aux transactions to become
functional again. This gave us the illusion that enabling interrupts
had something to do with making things work again, and put me on the
wrong track for a while.

- Interrupts occurring when we try to perform the aux transactions
required to put the dock back into MST mode. This isn't the problem,
as the only interrupts I've observed that come during this timeout
period are from the snd_hda_intel driver, and disabling that driver
doesn't appear to change the behavior at all.

- Skylake's PSR block causing issues by performing aux transactions
while we try to bring the dock out of MST mode. Disabling PSR through
i915's command line options doesn't seem to change the behavior
either, nor does preventing the DMC firmware from being loaded.

Since this investigation went on for about 2 weeks, we decided it would
be better for the time being to just workaround this issue until we find
a better fix. This patch adds some behavior we want in
drm_dp_dpcd_access() anyway, since DP aux transactions aren't exactly
robust and this will probably fix quite a few other issues with DP MST
hardware not responding in time. Plus, this is something we already do
in the i915 driver with intel_dp_dpcd_read_wake(), except that that
function is somewhat of a hack and DRM helpers can't make use of it.

Changes since v1

- Patch has been reworked to take the retry logic out of
intel_dp_mst_resume() and into drm_dp_dpcd_access(), based off a
suggestion from Daniel Vetter

- Commit message is much longer and gives a better description of the
issue this was originally intended to workaround.

Signed-off-by: Lyude <[email protected]>
---
drivers/gpu/drm/drm_dp_helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 9535c5b..38dd969 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -198,7 +198,7 @@ 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) {
- if (err == -EBUSY)
+ if (err == -EBUSY || err == -ETIMEDOUT)
continue;

return err;
--
2.5.0

2016-03-17 15:41:19

by Lyude Paul

Subject: [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()

Since we've fixed up drm_dp_dpcd_read() to allow for retries when things
timeout, there's no use for having this function anymore. Good riddens.

Signed-off-by: Lyude <[email protected]>
---
drivers/gpu/drm/i915/intel_dp.c | 79 ++++++++++++-----------------------------
1 file changed, 22 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index cdc2c15..fb4cbbe5 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3190,47 +3190,14 @@ 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. */
@@ -3865,8 +3832,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);
@@ -3877,9 +3844,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");
@@ -3890,9 +3857,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_read(&intel_dp->aux,
+ DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
+ &frame_sync_cap, 1);
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;
@@ -3908,15 +3875,13 @@ 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_read(&intel_dp->aux, DP_EDP_DPCD_REV, &rev, 1) == 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]);
@@ -3939,9 +3904,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;
@@ -3955,11 +3920,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]);
}
@@ -3975,7 +3940,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_read(&intel_dp->aux, DP_MSTM_CAP, buf, 1)) {
if (buf[0] & DP_MST_CAP) {
DRM_DEBUG_KMS("Sink is MST capable\n");
intel_dp->is_mst = true;
@@ -4112,7 +4077,7 @@ 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,
+ return drm_dp_dpcd_read(&intel_dp->aux,
DP_DEVICE_SERVICE_IRQ_VECTOR,
sink_irq_vector, 1) == 1;
}
@@ -4122,7 +4087,7 @@ 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,
+ ret = drm_dp_dpcd_read(&intel_dp->aux,
DP_SINK_COUNT_ESI,
sink_irq_vector, 14);
if (ret != 14)
@@ -4383,7 +4348,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,
+ if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_COUNT,
&reg, 1) < 0)
return connector_status_unknown;

--
2.5.0

2016-03-17 17:57:01

by Jani Nikula

Subject: Re: [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()

On Thu, 17 Mar 2016, Lyude <[email protected]> wrote:
> Since we've fixed up drm_dp_dpcd_read() to allow for retries when things
> timeout, there's no use for having this function anymore. Good riddens.
>
> Signed-off-by: Lyude <[email protected]>
> ---
> drivers/gpu/drm/i915/intel_dp.c | 79 ++++++++++++-----------------------------
> 1 file changed, 22 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index cdc2c15..fb4cbbe5 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3190,47 +3190,14 @@ 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);

Last time around I dug out the commit adding the line above, which fixed
a bug, and said it should be accounted for somehow. But I've had a
change of heart. I want this function gone already. We've improved the
dp helpers considerably, and if this regresses, we need to look at
fixing this either at the drm level or in our ->transfer hook.

Acked-by: Jani Nikula <[email protected]>


> -
> - 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. */
> @@ -3865,8 +3832,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);
> @@ -3877,9 +3844,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");
> @@ -3890,9 +3857,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_read(&intel_dp->aux,
> + DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> + &frame_sync_cap, 1);
> 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;
> @@ -3908,15 +3875,13 @@ 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_read(&intel_dp->aux, DP_EDP_DPCD_REV, &rev, 1) == 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]);
> @@ -3939,9 +3904,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;
> @@ -3955,11 +3920,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]);
> }
> @@ -3975,7 +3940,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_read(&intel_dp->aux, DP_MSTM_CAP, buf, 1)) {
> if (buf[0] & DP_MST_CAP) {
> DRM_DEBUG_KMS("Sink is MST capable\n");
> intel_dp->is_mst = true;
> @@ -4112,7 +4077,7 @@ 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,
> + return drm_dp_dpcd_read(&intel_dp->aux,
> DP_DEVICE_SERVICE_IRQ_VECTOR,
> sink_irq_vector, 1) == 1;
> }
> @@ -4122,7 +4087,7 @@ 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,
> + ret = drm_dp_dpcd_read(&intel_dp->aux,
> DP_SINK_COUNT_ESI,
> sink_irq_vector, 14);
> if (ret != 14)
> @@ -4383,7 +4348,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,
> + if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_COUNT,
> &reg, 1) < 0)
> return connector_status_unknown;

--
Jani Nikula, Intel Open Source Technology Center

2016-03-18 14:13:58

by Ville Syrjälä

Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()

On Thu, Mar 17, 2016 at 11:40:45AM -0400, Lyude wrote:
> Since we've fixed up drm_dp_dpcd_read() to allow for retries when things
> timeout, there's no use for having this function anymore. Good riddens.
>
> Signed-off-by: Lyude <[email protected]>
> ---
> drivers/gpu/drm/i915/intel_dp.c | 79 ++++++++++++-----------------------------
> 1 file changed, 22 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index cdc2c15..fb4cbbe5 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3190,47 +3190,14 @@ 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);

NAK

If people keep intentionally breaking my shit I'm going to become
really grumpy soon.


> -
> - 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. */
> @@ -3865,8 +3832,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);
> @@ -3877,9 +3844,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");
> @@ -3890,9 +3857,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_read(&intel_dp->aux,
> + DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> + &frame_sync_cap, 1);
> 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;
> @@ -3908,15 +3875,13 @@ 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_read(&intel_dp->aux, DP_EDP_DPCD_REV, &rev, 1) == 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]);
> @@ -3939,9 +3904,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;
> @@ -3955,11 +3920,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]);
> }
> @@ -3975,7 +3940,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_read(&intel_dp->aux, DP_MSTM_CAP, buf, 1)) {
> if (buf[0] & DP_MST_CAP) {
> DRM_DEBUG_KMS("Sink is MST capable\n");
> intel_dp->is_mst = true;
> @@ -4112,7 +4077,7 @@ 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,
> + return drm_dp_dpcd_read(&intel_dp->aux,
> DP_DEVICE_SERVICE_IRQ_VECTOR,
> sink_irq_vector, 1) == 1;
> }
> @@ -4122,7 +4087,7 @@ 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,
> + ret = drm_dp_dpcd_read(&intel_dp->aux,
> DP_SINK_COUNT_ESI,
> sink_irq_vector, 14);
> if (ret != 14)
> @@ -4383,7 +4348,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,
> + if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_COUNT,
> &reg, 1) < 0)
> return connector_status_unknown;
>
> --
> 2.5.0
>
> _______________________________________________
> Intel-gfx mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Ville Syrj?l?
Intel OTC

2016-03-18 15:56:38

by Daniel Vetter

Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()

On Thu, Mar 17, 2016 at 07:56:33PM +0200, Jani Nikula wrote:
> On Thu, 17 Mar 2016, Lyude <[email protected]> wrote:
> > Since we've fixed up drm_dp_dpcd_read() to allow for retries when things
> > timeout, there's no use for having this function anymore. Good riddens.
> >
> > Signed-off-by: Lyude <[email protected]>
> > ---
> > drivers/gpu/drm/i915/intel_dp.c | 79 ++++++++++++-----------------------------
> > 1 file changed, 22 insertions(+), 57 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index cdc2c15..fb4cbbe5 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3190,47 +3190,14 @@ 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);
>
> Last time around I dug out the commit adding the line above, which fixed
> a bug, and said it should be accounted for somehow. But I've had a
> change of heart. I want this function gone already. We've improved the
> dp helpers considerably, and if this regresses, we need to look at
> fixing this either at the drm level or in our ->transfer hook.
>
> Acked-by: Jani Nikula <[email protected]>

Hm, my idea was actually to 1:1 move intel_dp_dpcd_read_wake logic into
drm_dp_dpcd_read, without any functional change for i915.ko. And everyone
else shouldn't complain about a few more retries either.

That would also address Ville's grumpy nack.
-Daniel

>
>
> > -
> > - 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. */
> > @@ -3865,8 +3832,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);
> > @@ -3877,9 +3844,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");
> > @@ -3890,9 +3857,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_read(&intel_dp->aux,
> > + DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> > + &frame_sync_cap, 1);
> > 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;
> > @@ -3908,15 +3875,13 @@ 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_read(&intel_dp->aux, DP_EDP_DPCD_REV, &rev, 1) == 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]);
> > @@ -3939,9 +3904,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;
> > @@ -3955,11 +3920,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]);
> > }
> > @@ -3975,7 +3940,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_read(&intel_dp->aux, DP_MSTM_CAP, buf, 1)) {
> > if (buf[0] & DP_MST_CAP) {
> > DRM_DEBUG_KMS("Sink is MST capable\n");
> > intel_dp->is_mst = true;
> > @@ -4112,7 +4077,7 @@ 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,
> > + return drm_dp_dpcd_read(&intel_dp->aux,
> > DP_DEVICE_SERVICE_IRQ_VECTOR,
> > sink_irq_vector, 1) == 1;
> > }
> > @@ -4122,7 +4087,7 @@ 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,
> > + ret = drm_dp_dpcd_read(&intel_dp->aux,
> > DP_SINK_COUNT_ESI,
> > sink_irq_vector, 14);
> > if (ret != 14)
> > @@ -4383,7 +4348,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,
> > + if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_COUNT,
> > &reg, 1) < 0)
> > return connector_status_unknown;
>
> --
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

2016-03-18 16:12:41

by Ville Syrjälä

Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()

On Fri, Mar 18, 2016 at 04:13:45PM +0200, Ville Syrj?l? wrote:
> On Thu, Mar 17, 2016 at 11:40:45AM -0400, Lyude wrote:
> > Since we've fixed up drm_dp_dpcd_read() to allow for retries when things
> > timeout, there's no use for having this function anymore. Good riddens.
> >
> > Signed-off-by: Lyude <[email protected]>
> > ---
> > drivers/gpu/drm/i915/intel_dp.c | 79 ++++++++++++-----------------------------
> > 1 file changed, 22 insertions(+), 57 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index cdc2c15..fb4cbbe5 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3190,47 +3190,14 @@ 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);
>
> NAK
>
> If people keep intentionally breaking my shit I'm going to become
> really grumpy soon.

Oh, and just in case someone wants to come up with a better kludge,
I just spent a few minutes analyzing the behavior of this crappy
monitor a.

What happens is that when the monitor is fully powered up (LED is blue)
things are fine. After the monitor goes to sleep (LED turns orange)
the first DPCD read will produce garbage. Further DPCD reads are fine,
even if I wait a significant amount of time between the reads, as long
as the monitor didn't do a power on->off cycle in between. So it looks
like it's always just the first read after power down that gets
corrupted.

Now I think I'll go and test how writes behave, assuming I can find a
decently sized chunk of DPCD address space I can write. And maybe I
should also try i2c-over-aux...

>
> > -
> > - 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. */
> > @@ -3865,8 +3832,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);
> > @@ -3877,9 +3844,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");
> > @@ -3890,9 +3857,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_read(&intel_dp->aux,
> > + DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> > + &frame_sync_cap, 1);
> > 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;
> > @@ -3908,15 +3875,13 @@ 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_read(&intel_dp->aux, DP_EDP_DPCD_REV, &rev, 1) == 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]);
> > @@ -3939,9 +3904,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;
> > @@ -3955,11 +3920,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]);
> > }
> > @@ -3975,7 +3940,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_read(&intel_dp->aux, DP_MSTM_CAP, buf, 1)) {
> > if (buf[0] & DP_MST_CAP) {
> > DRM_DEBUG_KMS("Sink is MST capable\n");
> > intel_dp->is_mst = true;
> > @@ -4112,7 +4077,7 @@ 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,
> > + return drm_dp_dpcd_read(&intel_dp->aux,
> > DP_DEVICE_SERVICE_IRQ_VECTOR,
> > sink_irq_vector, 1) == 1;
> > }
> > @@ -4122,7 +4087,7 @@ 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,
> > + ret = drm_dp_dpcd_read(&intel_dp->aux,
> > DP_SINK_COUNT_ESI,
> > sink_irq_vector, 14);
> > if (ret != 14)
> > @@ -4383,7 +4348,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,
> > + if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_COUNT,
> > &reg, 1) < 0)
> > return connector_status_unknown;
> >
> > --
> > 2.5.0
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > [email protected]
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrj?l?
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Ville Syrj?l?
Intel OTC

2016-03-18 16:41:47

by Ville Syrjälä

Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()

On Fri, Mar 18, 2016 at 06:12:35PM +0200, Ville Syrj?l? wrote:
> On Fri, Mar 18, 2016 at 04:13:45PM +0200, Ville Syrj?l? wrote:
> > On Thu, Mar 17, 2016 at 11:40:45AM -0400, Lyude wrote:
> > > Since we've fixed up drm_dp_dpcd_read() to allow for retries when things
> > > timeout, there's no use for having this function anymore. Good riddens.
> > >
> > > Signed-off-by: Lyude <[email protected]>
> > > ---
> > > drivers/gpu/drm/i915/intel_dp.c | 79 ++++++++++++-----------------------------
> > > 1 file changed, 22 insertions(+), 57 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index cdc2c15..fb4cbbe5 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -3190,47 +3190,14 @@ 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);
> >
> > NAK
> >
> > If people keep intentionally breaking my shit I'm going to become
> > really grumpy soon.
>
> Oh, and just in case someone wants to come up with a better kludge,
> I just spent a few minutes analyzing the behavior of this crappy
> monitor a.
>
> What happens is that when the monitor is fully powered up (LED is blue)
> things are fine. After the monitor goes to sleep (LED turns orange)
> the first DPCD read will produce garbage. Further DPCD reads are fine,
> even if I wait a significant amount of time between the reads, as long
> as the monitor didn't do a power on->off cycle in between. So it looks
> like it's always just the first read after power down that gets
> corrupted.
>
> Now I think I'll go and test how writes behave, assuming I can find a
> decently sized chunk of DPCD address space I can write. And maybe I
> should also try i2c-over-aux...

The first DPCD write after powerdown also got corrupted. But i2c-over-aux
seems unaffected for whatever reason.

>
> >
> > > -
> > > - 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. */
> > > @@ -3865,8 +3832,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);
> > > @@ -3877,9 +3844,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");
> > > @@ -3890,9 +3857,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_read(&intel_dp->aux,
> > > + DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> > > + &frame_sync_cap, 1);
> > > 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;
> > > @@ -3908,15 +3875,13 @@ 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_read(&intel_dp->aux, DP_EDP_DPCD_REV, &rev, 1) == 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]);
> > > @@ -3939,9 +3904,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;
> > > @@ -3955,11 +3920,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]);
> > > }
> > > @@ -3975,7 +3940,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_read(&intel_dp->aux, DP_MSTM_CAP, buf, 1)) {
> > > if (buf[0] & DP_MST_CAP) {
> > > DRM_DEBUG_KMS("Sink is MST capable\n");
> > > intel_dp->is_mst = true;
> > > @@ -4112,7 +4077,7 @@ 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,
> > > + return drm_dp_dpcd_read(&intel_dp->aux,
> > > DP_DEVICE_SERVICE_IRQ_VECTOR,
> > > sink_irq_vector, 1) == 1;
> > > }
> > > @@ -4122,7 +4087,7 @@ 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,
> > > + ret = drm_dp_dpcd_read(&intel_dp->aux,
> > > DP_SINK_COUNT_ESI,
> > > sink_irq_vector, 14);
> > > if (ret != 14)
> > > @@ -4383,7 +4348,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,
> > > + if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_COUNT,
> > > &reg, 1) < 0)
> > > return connector_status_unknown;
> > >
> > > --
> > > 2.5.0
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > [email protected]
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > --
> > Ville Syrj?l?
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > [email protected]
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrj?l?
> Intel OTC
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

--
Ville Syrj?l?
Intel OTC

2016-03-18 17:59:44

by Daniel Vetter

Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()

On Fri, Mar 18, 2016 at 06:41:40PM +0200, Ville Syrj?l? wrote:
> On Fri, Mar 18, 2016 at 06:12:35PM +0200, Ville Syrj?l? wrote:
> > On Fri, Mar 18, 2016 at 04:13:45PM +0200, Ville Syrj?l? wrote:
> > > On Thu, Mar 17, 2016 at 11:40:45AM -0400, Lyude wrote:
> > > > Since we've fixed up drm_dp_dpcd_read() to allow for retries when things
> > > > timeout, there's no use for having this function anymore. Good riddens.
> > > >
> > > > Signed-off-by: Lyude <[email protected]>
> > > > ---
> > > > drivers/gpu/drm/i915/intel_dp.c | 79 ++++++++++++-----------------------------
> > > > 1 file changed, 22 insertions(+), 57 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > index cdc2c15..fb4cbbe5 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -3190,47 +3190,14 @@ 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);
> > >
> > > NAK
> > >
> > > If people keep intentionally breaking my shit I'm going to become
> > > really grumpy soon.
> >
> > Oh, and just in case someone wants to come up with a better kludge,
> > I just spent a few minutes analyzing the behavior of this crappy
> > monitor a.
> >
> > What happens is that when the monitor is fully powered up (LED is blue)
> > things are fine. After the monitor goes to sleep (LED turns orange)
> > the first DPCD read will produce garbage. Further DPCD reads are fine,
> > even if I wait a significant amount of time between the reads, as long
> > as the monitor didn't do a power on->off cycle in between. So it looks
> > like it's always just the first read after power down that gets
> > corrupted.
> >
> > Now I think I'll go and test how writes behave, assuming I can find a
> > decently sized chunk of DPCD address space I can write. And maybe I
> > should also try i2c-over-aux...
>
> The first DPCD write after powerdown also got corrupted. But i2c-over-aux
> seems unaffected for whatever reason.

Do you have an amd card nearby to test there? Would be interesting to
confirm that this is indeed a sink bug, and hence that it really all
should be in the shared code.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

2016-03-18 18:06:04

by Ville Syrjälä

Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()

On Fri, Mar 18, 2016 at 07:00:29PM +0100, Daniel Vetter wrote:
> On Fri, Mar 18, 2016 at 06:41:40PM +0200, Ville Syrj?l? wrote:
> > On Fri, Mar 18, 2016 at 06:12:35PM +0200, Ville Syrj?l? wrote:
> > > On Fri, Mar 18, 2016 at 04:13:45PM +0200, Ville Syrj?l? wrote:
> > > > On Thu, Mar 17, 2016 at 11:40:45AM -0400, Lyude wrote:
> > > > > Since we've fixed up drm_dp_dpcd_read() to allow for retries when things
> > > > > timeout, there's no use for having this function anymore. Good riddens.
> > > > >
> > > > > Signed-off-by: Lyude <[email protected]>
> > > > > ---
> > > > > drivers/gpu/drm/i915/intel_dp.c | 79 ++++++++++++-----------------------------
> > > > > 1 file changed, 22 insertions(+), 57 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index cdc2c15..fb4cbbe5 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -3190,47 +3190,14 @@ 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);
> > > >
> > > > NAK
> > > >
> > > > If people keep intentionally breaking my shit I'm going to become
> > > > really grumpy soon.
> > >
> > > Oh, and just in case someone wants to come up with a better kludge,
> > > I just spent a few minutes analyzing the behavior of this crappy
> > > monitor a.
> > >
> > > What happens is that when the monitor is fully powered up (LED is blue)
> > > things are fine. After the monitor goes to sleep (LED turns orange)
> > > the first DPCD read will produce garbage. Further DPCD reads are fine,
> > > even if I wait a significant amount of time between the reads, as long
> > > as the monitor didn't do a power on->off cycle in between. So it looks
> > > like it's always just the first read after power down that gets
> > > corrupted.
> > >
> > > Now I think I'll go and test how writes behave, assuming I can find a
> > > decently sized chunk of DPCD address space I can write. And maybe I
> > > should also try i2c-over-aux...
> >
> > The first DPCD write after powerdown also got corrupted. But i2c-over-aux
> > seems unaffected for whatever reason.
>
> Do you have an amd card nearby to test there?

Nope.

> Would be interesting to
> confirm that this is indeed a sink bug, and hence that it really all
> should be in the shared code.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

--
Ville Syrj?l?
Intel OTC

2016-03-21 10:31:12

by Jani Nikula

Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()

On Fri, 18 Mar 2016, Ville Syrjälä <[email protected]> wrote:
> On Fri, Mar 18, 2016 at 06:12:35PM +0200, Ville Syrjälä wrote:
>> On Fri, Mar 18, 2016 at 04:13:45PM +0200, Ville Syrjälä wrote:
>> > On Thu, Mar 17, 2016 at 11:40:45AM -0400, Lyude wrote:
>> > > - drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
>> >
>> > NAK
>> >
>> > If people keep intentionally breaking my shit I'm going to become
>> > really grumpy soon.
>>
>> Oh, and just in case someone wants to come up with a better kludge,
>> I just spent a few minutes analyzing the behavior of this crappy
>> monitor a.
>>
>> What happens is that when the monitor is fully powered up (LED is blue)
>> things are fine. After the monitor goes to sleep (LED turns orange)
>> the first DPCD read will produce garbage. Further DPCD reads are fine,
>> even if I wait a significant amount of time between the reads, as long
>> as the monitor didn't do a power on->off cycle in between. So it looks
>> like it's always just the first read after power down that gets
>> corrupted.
>>
>> Now I think I'll go and test how writes behave, assuming I can find a
>> decently sized chunk of DPCD address space I can write. And maybe I
>> should also try i2c-over-aux...
>
> The first DPCD write after powerdown also got corrupted. But i2c-over-aux
> seems unaffected for whatever reason.

Did the display go to sleep on its own, or did we do something? In
particular, does DPCD DP_SET_POWER register play a role? What if we skip
writing D3 to it? What if we do that write as the first thing (every
time)?


BR,
Jani.


--
Jani Nikula, Intel Open Source Technology Center

2016-03-21 13:38:57

by Ville Syrjälä

Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()

On Mon, Mar 21, 2016 at 12:30:54PM +0200, Jani Nikula wrote:
> On Fri, 18 Mar 2016, Ville Syrj?l? <[email protected]> wrote:
> > On Fri, Mar 18, 2016 at 06:12:35PM +0200, Ville Syrj?l? wrote:
> >> On Fri, Mar 18, 2016 at 04:13:45PM +0200, Ville Syrj?l? wrote:
> >> > On Thu, Mar 17, 2016 at 11:40:45AM -0400, Lyude wrote:
> >> > > - drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
> >> >
> >> > NAK
> >> >
> >> > If people keep intentionally breaking my shit I'm going to become
> >> > really grumpy soon.
> >>
> >> Oh, and just in case someone wants to come up with a better kludge,
> >> I just spent a few minutes analyzing the behavior of this crappy
> >> monitor a.
> >>
> >> What happens is that when the monitor is fully powered up (LED is blue)
> >> things are fine. After the monitor goes to sleep (LED turns orange)
> >> the first DPCD read will produce garbage. Further DPCD reads are fine,
> >> even if I wait a significant amount of time between the reads, as long
> >> as the monitor didn't do a power on->off cycle in between. So it looks
> >> like it's always just the first read after power down that gets
> >> corrupted.
> >>
> >> Now I think I'll go and test how writes behave, assuming I can find a
> >> decently sized chunk of DPCD address space I can write. And maybe I
> >> should also try i2c-over-aux...
> >
> > The first DPCD write after powerdown also got corrupted. But i2c-over-aux
> > seems unaffected for whatever reason.
>
> Did the display go to sleep on its own, or did we do something? In
> particular, does DPCD DP_SET_POWER register play a role? What if we skip
> writing D3 to it? What if we do that write as the first thing (every
> time)?

User pressing any of the buttons on the monitor is enough to wake it,
and after a short timeout it will power down on its own, leading to
the corrupted access.

Keeping DP_SET_POWER at D0 doesn't change anything.

--
Ville Syrj?l?
Intel OTC

2016-03-21 16:37:50

by Lyude Paul

Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()

On Fri, 2016-03-18 at 20:05 +0200, Ville Syrjälä wrote:
> On Fri, Mar 18, 2016 at 07:00:29PM +0100, Daniel Vetter wrote:
> >
> > On Fri, Mar 18, 2016 at 06:41:40PM +0200, Ville Syrjälä wrote:
> > >
> > > On Fri, Mar 18, 2016 at 06:12:35PM +0200, Ville Syrjälä wrote:
> > > >
> > > > On Fri, Mar 18, 2016 at 04:13:45PM +0200, Ville Syrjälä wrote:
> > > > >
> > > > > On Thu, Mar 17, 2016 at 11:40:45AM -0400, Lyude wrote:
> > > > > >
> > > > > > Since we've fixed up drm_dp_dpcd_read() to allow for retries when
> > > > > > things
> > > > > > timeout, there's no use for having this function anymore. Good
> > > > > > riddens.
> > > > > >
> > > > > > Signed-off-by: Lyude <[email protected]>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_dp.c | 79 ++++++++++++----------------
> > > > > > -------------
> > > > > >  1 file changed, 22 insertions(+), 57 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > index cdc2c15..fb4cbbe5 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > @@ -3190,47 +3190,14 @@ 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);
> > > > > NAK
> > > > >
> > > > > If people keep intentionally breaking my shit I'm going to become
> > > > > really grumpy soon.
> > > > Oh, and just in case someone wants to come up with a better kludge,
> > > > I just spent a few minutes analyzing the behavior of this crappy
> > > > monitor a.
> > > >
> > > > What happens is that when the monitor is fully powered up (LED is blue)
> > > > things are fine. After the monitor goes to sleep (LED turns orange)
> > > > the first DPCD read will produce garbage. Further DPCD reads are fine,
> > > > even if I wait a significant amount of time between the reads, as long
> > > > as the monitor didn't do a power on->off cycle in between. So it looks
> > > > like it's always just the first read after power down that gets
> > > > corrupted.
> > > >
> > > > Now I think I'll go and test how writes behave, assuming I can find a
> > > > decently sized chunk of DPCD address space I can write. And maybe I
> > > > should also try i2c-over-aux...
> > > The first DPCD write after powerdown also got corrupted. But i2c-over-aux
> > > seems unaffected for whatever reason.
> > Do you have an amd card nearby to test there?
> Nope.
I also work on radeon so I have plenty. The issue with that though is that this
is an issue that only really happens with the thinkpad docks, and as far as I'm
aware we don't have any ThinkPads that have AMD hardware in them…
> >
> > Would be interesting to
> > confirm that this is indeed a sink bug, and hence that it really all
> > should be in the shared code.
> > -Daniel
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

2016-03-23 19:30:23

by Lyude Paul

Subject: [PATCH v3 1/2] drm/dp_helper: add workarounds from intel_dp_dpcd_read_wake()

Some sinks need some time during the process of resuming the system from
sleep before they're ready to handle transactions. While it would be
nice if they responded with NACKs in these scenarios, this isn't always
the case as a few sinks will just timeout on all of the transactions
they receive.

This patch was originally intended to be a workaround for a very
mysterious bug on the T560, where any monitors connected to the dock
would fail to turn back on after resume. When resuming the laptop, it
appears that there's a short period of time where we're unable to
complete any aux transactions, as they all immediately timeout. The only
machine I'm able to reproduce this on is the T560 as other production
Skylake models seem to be fine. The period during which AUX transactions
fail appears to be around 22ms long. AFAIK, the dock for the T560 never
actually turns off, the only difference is that it's in SST mode at the
start of the resume process, so it's unclear as to why it would need so
much time to come back up.

There's been a discussion on this issue going on for a while on the
intel-gfx mailing list about this that has, in addition to including
developers from Intel, also had the correspondence of one of the
hardware engineers for Intel:

http://www.spinics.net/lists/intel-gfx/msg88831.html
http://www.spinics.net/lists/intel-gfx/msg88410.html

We've already looked into a couple of possible explanations for the
problem:

- Calling intel_dp_mst_resume() before right fix.
intel_runtime_pm_enable_interrupts(). This was the first fix I tried,
and while it worked it definitely wasn't the right fix. This worked
because DP aux transactions don't actually require interrupts to work:

static uint32_t
intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
{
struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
struct drm_device *dev = intel_dig_port->base.base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg;
uint32_t status;
bool done;

#define C (((status = I915_READ_NOTRACE(ch_ctl)) & DP_AUX_CH_CTL_SEND_BUSY) == 0)
if (has_aux_irq)
done = wait_event_timeout(dev_priv->gmbus_wait_queue, C,
msecs_to_jiffies_timeout(10));
else
done = wait_for_atomic(C, 10) == 0;
if (!done)
DRM_ERROR("dp aux hw did not signal timeout (has irq: %i)!\n",
has_aux_irq);
#undef C

return status;
}

When there's no interrupts enabled, we end up timing out on the
wait_event_timeout() call, which causes us to check the DP status
register once to see if the transaction was successful or not. Since
this adds a 10ms delay to each aux transaction, it ends up adding a
long enough delay to the resume process for aux transactions to become
functional again. This gave us the illusion that enabling interrupts
had something to do with making things work again, and put me on the
wrong track for a while.

- Interrupts occurring when we try to perform the aux transactions
required to put the dock back into MST mode. This isn't the problem,
as the only interrupts I've observed that come during this timeout
period are from the snd_hda_intel driver, and disabling that driver
doesn't appear to change the behavior at all.

- Skylake's PSR block causing issues by performing aux transactions
while we try to bring the dock out of MST mode. Disabling PSR through
i915's command line options doesn't seem to change the behavior
either, nor does preventing the DMC firmware from being loaded.

Since this investigation went on for about 2 weeks, we decided it would
be better for the time being to just workaround this issue until we find
a better fix. This patch adds some behavior we want in
drm_dp_dpcd_access() anyway, since DP aux transactions aren't exactly
robust and this will probably fix quite a few other issues with DP MST
hardware not responding in time. Plus, this is something we already do
in the i915 driver with intel_dp_dpcd_read_wake(), except that that
function is somewhat of a hack and DRM helpers can't make use of it.

Changes since v2
- Reworked the patch again to incorporate all of the behavior of
intel_dp_dpcd_read_wake() into drm_dp_dpcd_read() and
drm_dp_dpcd_access()

Changes since v1

- Patch has been reworked to take the retry logic out of
intel_dp_mst_resume() and into drm_dp_dpcd_access(), based off a
suggestion from Daniel Vetter

- Commit message is much longer and gives a better description of the
issue this was originally intended to workaround.

Signed-off-by: Lyude <[email protected]>

squash! drm/dp_helper: retry on -ETIMEDOUT in drm_dp_dpcd_access()

squash! drm/dp_helper: retry on -ETIMEDOUT in drm_dp_dpcd_access()
---
drivers/gpu/drm/drm_dp_helper.c | 47 +++++++++++++++++++++--------------------
1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 9535c5b..9b3426c 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -159,7 +159,7 @@ int drm_dp_bw_code_to_link_rate(u8 link_bw)
}
EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate);

-#define AUX_RETRY_INTERVAL 500 /* us */
+#define AUX_RETRY_INTERVAL 1000 /* us */

/**
* DOC: dp helpers
@@ -177,8 +177,8 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
unsigned int offset, void *buffer, size_t size)
{
struct drm_dp_aux_msg msg;
- unsigned int retry;
- int err;
+ unsigned int retry, native_reply;
+ int ret = 0;

memset(&msg, 0, sizeof(msg));
msg.address = offset;
@@ -193,35 +193,29 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
* sufficient, bump to 32 which makes Dell 4k monitors happier.
*/
for (retry = 0; retry < 32; retry++) {
+ if (ret != 0 && ret != -ETIMEDOUT) {
+ usleep_range(AUX_RETRY_INTERVAL,
+ AUX_RETRY_INTERVAL + 100);
+ }

mutex_lock(&aux->hw_mutex);
- err = aux->transfer(aux, &msg);
+ ret = aux->transfer(aux, &msg);
mutex_unlock(&aux->hw_mutex);
- if (err < 0) {
- if (err == -EBUSY)
- continue;
-
- return err;
- }
-
-
- switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {
- case DP_AUX_NATIVE_REPLY_ACK:
- if (err < size)
- return -EPROTO;
- return err;

- case DP_AUX_NATIVE_REPLY_NACK:
- return -EIO;
+ if (ret > 0) {
+ native_reply = msg.reply & DP_AUX_NATIVE_REPLY_MASK;
+ if (native_reply == DP_AUX_NATIVE_REPLY_ACK) {
+ if (ret == size)
+ return ret;

- case DP_AUX_NATIVE_REPLY_DEFER:
- usleep_range(AUX_RETRY_INTERVAL, AUX_RETRY_INTERVAL + 100);
- break;
+ ret = -EPROTO;
+ } else
+ ret = -EIO;
}
}

DRM_DEBUG_KMS("too many retries, giving up\n");
- return -EIO;
+ return ret;
}

/**
@@ -241,6 +235,13 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
void *buffer, size_t size)
{
+ /*
+ * Sometimes we just get the same incorrect byte repeated over the
+ * entire buffer. Doing one throw away read initially seems to "solve"
+ * it.
+ */
+ drm_dp_dpcd_access(aux, DP_AUX_NATIVE_READ, DP_DPCD_REV, buffer, 1);
+
return drm_dp_dpcd_access(aux, DP_AUX_NATIVE_READ, offset, buffer,
size);
}
--
2.5.5

2016-03-23 19:33:28

by Lyude Paul

Subject: [PATCH v3 1/2 RESEND] drm/dp_helper: add workarounds from intel_dp_dpcd_read_wake()

Some sinks need some time during the process of resuming the system from
sleep before they're ready to handle transactions. While it would be
nice if they responded with NACKs in these scenarios, this isn't always
the case as a few sinks will just timeout on all of the transactions
they receive.

This patch was originally intended to be a workaround for a very
mysterious bug on the T560, where any monitors connected to the dock
would fail to turn back on after resume. When resuming the laptop, it
appears that there's a short period of time where we're unable to
complete any aux transactions, as they all immediately timeout. The only
machine I'm able to reproduce this on is the T560 as other production
Skylake models seem to be fine. The period during which AUX transactions
fail appears to be around 22ms long. AFAIK, the dock for the T560 never
actually turns off, the only difference is that it's in SST mode at the
start of the resume process, so it's unclear as to why it would need so
much time to come back up.

There's been a discussion on this issue going on for a while on the
intel-gfx mailing list about this that has, in addition to including
developers from Intel, also had the correspondence of one of the
hardware engineers for Intel:

http://www.spinics.net/lists/intel-gfx/msg88831.html
http://www.spinics.net/lists/intel-gfx/msg88410.html

We've already looked into a couple of possible explanations for the
problem:

- Calling intel_dp_mst_resume() before right fix.
intel_runtime_pm_enable_interrupts(). This was the first fix I tried,
and while it worked it definitely wasn't the right fix. This worked
because DP aux transactions don't actually require interrupts to work:

static uint32_t
intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
{
struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
struct drm_device *dev = intel_dig_port->base.base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg;
uint32_t status;
bool done;

#define C (((status = I915_READ_NOTRACE(ch_ctl)) & DP_AUX_CH_CTL_SEND_BUSY) == 0)
if (has_aux_irq)
done = wait_event_timeout(dev_priv->gmbus_wait_queue, C,
msecs_to_jiffies_timeout(10));
else
done = wait_for_atomic(C, 10) == 0;
if (!done)
DRM_ERROR("dp aux hw did not signal timeout (has irq: %i)!\n",
has_aux_irq);
#undef C

return status;
}

When there's no interrupts enabled, we end up timing out on the
wait_event_timeout() call, which causes us to check the DP status
register once to see if the transaction was successful or not. Since
this adds a 10ms delay to each aux transaction, it ends up adding a
long enough delay to the resume process for aux transactions to become
functional again. This gave us the illusion that enabling interrupts
had something to do with making things work again, and put me on the
wrong track for a while.

- Interrupts occurring when we try to perform the aux transactions
required to put the dock back into MST mode. This isn't the problem,
as the only interrupts I've observed that come during this timeout
period are from the snd_hda_intel driver, and disabling that driver
doesn't appear to change the behavior at all.

- Skylake's PSR block causing issues by performing aux transactions
while we try to bring the dock out of MST mode. Disabling PSR through
i915's command line options doesn't seem to change the behavior
either, nor does preventing the DMC firmware from being loaded.

Since this investigation went on for about 2 weeks, we decided it would
be better for the time being to just workaround this issue until we find
a better fix. This patch adds some behavior we want in
drm_dp_dpcd_access() anyway, since DP aux transactions aren't exactly
robust and this will probably fix quite a few other issues with DP MST
hardware not responding in time. Plus, this is something we already do
in the i915 driver with intel_dp_dpcd_read_wake(), except that that
function is somewhat of a hack and DRM helpers can't make use of it.

Changes since v2
- Reworked the patch again to incorporate all of the behavior of
intel_dp_dpcd_read_wake() into drm_dp_dpcd_read() and
drm_dp_dpcd_access()

Changes since v1

- Patch has been reworked to take the retry logic out of
intel_dp_mst_resume() and into drm_dp_dpcd_access(), based off a
suggestion from Daniel Vetter

- Commit message is much longer and gives a better description of the
issue this was originally intended to workaround.

Signed-off-by: Lyude <[email protected]>
---
Left some rebase ditritus in the patch after sending it by accident, whoops

drivers/gpu/drm/drm_dp_helper.c | 47 +++++++++++++++++++++--------------------
1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 9535c5b..9b3426c 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -159,7 +159,7 @@ int drm_dp_bw_code_to_link_rate(u8 link_bw)
}
EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate);

-#define AUX_RETRY_INTERVAL 500 /* us */
+#define AUX_RETRY_INTERVAL 1000 /* us */

/**
* DOC: dp helpers
@@ -177,8 +177,8 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
unsigned int offset, void *buffer, size_t size)
{
struct drm_dp_aux_msg msg;
- unsigned int retry;
- int err;
+ unsigned int retry, native_reply;
+ int ret = 0;

memset(&msg, 0, sizeof(msg));
msg.address = offset;
@@ -193,35 +193,29 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
* sufficient, bump to 32 which makes Dell 4k monitors happier.
*/
for (retry = 0; retry < 32; retry++) {
+ if (ret != 0 && ret != -ETIMEDOUT) {
+ usleep_range(AUX_RETRY_INTERVAL,
+ AUX_RETRY_INTERVAL + 100);
+ }

mutex_lock(&aux->hw_mutex);
- err = aux->transfer(aux, &msg);
+ ret = aux->transfer(aux, &msg);
mutex_unlock(&aux->hw_mutex);
- if (err < 0) {
- if (err == -EBUSY)
- continue;
-
- return err;
- }
-
-
- switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {
- case DP_AUX_NATIVE_REPLY_ACK:
- if (err < size)
- return -EPROTO;
- return err;

- case DP_AUX_NATIVE_REPLY_NACK:
- return -EIO;
+ if (ret > 0) {
+ native_reply = msg.reply & DP_AUX_NATIVE_REPLY_MASK;
+ if (native_reply == DP_AUX_NATIVE_REPLY_ACK) {
+ if (ret == size)
+ return ret;

- case DP_AUX_NATIVE_REPLY_DEFER:
- usleep_range(AUX_RETRY_INTERVAL, AUX_RETRY_INTERVAL + 100);
- break;
+ ret = -EPROTO;
+ } else
+ ret = -EIO;
}
}

DRM_DEBUG_KMS("too many retries, giving up\n");
- return -EIO;
+ return ret;
}

/**
@@ -241,6 +235,13 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
void *buffer, size_t size)
{
+ /*
+ * Sometimes we just get the same incorrect byte repeated over the
+ * entire buffer. Doing one throw away read initially seems to "solve"
+ * it.
+ */
+ drm_dp_dpcd_access(aux, DP_AUX_NATIVE_READ, DP_DPCD_REV, buffer, 1);
+
return drm_dp_dpcd_access(aux, DP_AUX_NATIVE_READ, offset, buffer,
size);
}
--
2.5.5

2016-03-24 09:38:04

by Jani Nikula

Subject: Re: [PATCH v3 1/2 RESEND] drm/dp_helper: add workarounds from intel_dp_dpcd_read_wake()

On Wed, 23 Mar 2016, Lyude <[email protected]> wrote:
> Some sinks need some time during the process of resuming the system from
> sleep before they're ready to handle transactions. While it would be
> nice if they responded with NACKs in these scenarios, this isn't always
> the case as a few sinks will just timeout on all of the transactions
> they receive.
>
> This patch was originally intended to be a workaround for a very
> mysterious bug on the T560, where any monitors connected to the dock
> would fail to turn back on after resume. When resuming the laptop, it
> appears that there's a short period of time where we're unable to
> complete any aux transactions, as they all immediately timeout. The only
> machine I'm able to reproduce this on is the T560 as other production
> Skylake models seem to be fine. The period during which AUX transactions
> fail appears to be around 22ms long. AFAIK, the dock for the T560 never
> actually turns off, the only difference is that it's in SST mode at the
> start of the resume process, so it's unclear as to why it would need so
> much time to come back up.
>
> There's been a discussion on this issue going on for a while on the
> intel-gfx mailing list about this that has, in addition to including
> developers from Intel, also had the correspondence of one of the
> hardware engineers for Intel:
>
> http://www.spinics.net/lists/intel-gfx/msg88831.html
> http://www.spinics.net/lists/intel-gfx/msg88410.html
>
> We've already looked into a couple of possible explanations for the
> problem:
>
> - Calling intel_dp_mst_resume() before right fix.
> intel_runtime_pm_enable_interrupts(). This was the first fix I tried,
> and while it worked it definitely wasn't the right fix. This worked
> because DP aux transactions don't actually require interrupts to work:
>
> static uint32_t
> intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
> {
> struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> struct drm_device *dev = intel_dig_port->base.base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg;
> uint32_t status;
> bool done;
>
> #define C (((status = I915_READ_NOTRACE(ch_ctl)) & DP_AUX_CH_CTL_SEND_BUSY) == 0)
> if (has_aux_irq)
> done = wait_event_timeout(dev_priv->gmbus_wait_queue, C,
> msecs_to_jiffies_timeout(10));
> else
> done = wait_for_atomic(C, 10) == 0;
> if (!done)
> DRM_ERROR("dp aux hw did not signal timeout (has irq: %i)!\n",
> has_aux_irq);
> #undef C
>
> return status;
> }
>
> When there's no interrupts enabled, we end up timing out on the
> wait_event_timeout() call, which causes us to check the DP status
> register once to see if the transaction was successful or not. Since
> this adds a 10ms delay to each aux transaction, it ends up adding a
> long enough delay to the resume process for aux transactions to become
> functional again. This gave us the illusion that enabling interrupts
> had something to do with making things work again, and put me on the
> wrong track for a while.
>
> - Interrupts occurring when we try to perform the aux transactions
> required to put the dock back into MST mode. This isn't the problem,
> as the only interrupts I've observed that come during this timeout
> period are from the snd_hda_intel driver, and disabling that driver
> doesn't appear to change the behavior at all.
>
> - Skylake's PSR block causing issues by performing aux transactions
> while we try to bring the dock out of MST mode. Disabling PSR through
> i915's command line options doesn't seem to change the behavior
> either, nor does preventing the DMC firmware from being loaded.
>
> Since this investigation went on for about 2 weeks, we decided it would
> be better for the time being to just workaround this issue until we find
> a better fix. This patch adds some behavior we want in
> drm_dp_dpcd_access() anyway, since DP aux transactions aren't exactly
> robust and this will probably fix quite a few other issues with DP MST
> hardware not responding in time. Plus, this is something we already do
> in the i915 driver with intel_dp_dpcd_read_wake(), except that that
> function is somewhat of a hack and DRM helpers can't make use of it.
>
> Changes since v2
> - Reworked the patch again to incorporate all of the behavior of
> intel_dp_dpcd_read_wake() into drm_dp_dpcd_read() and
> drm_dp_dpcd_access()
>
> Changes since v1
>
> - Patch has been reworked to take the retry logic out of
> intel_dp_mst_resume() and into drm_dp_dpcd_access(), based off a
> suggestion from Daniel Vetter
>
> - Commit message is much longer and gives a better description of the
> issue this was originally intended to workaround.
>
> Signed-off-by: Lyude <[email protected]>

We've had a lot of issues with dp aux in the past, and we've slowly and
iteratively improved over time. In that light, I am strongly opposed to
doing as many changes in one go as this patch does.

If we got a bisect result on this, it would be hard to decide what it
really was. We might have to revert it all even if some parts of it were
good.

I'll identify some of the changes below that I think must be separated
to distinct changes. (The numbers don't necessarily reflect the order in
which the changes should be done.)

> ---
> Left some rebase ditritus in the patch after sending it by accident, whoops
>
> drivers/gpu/drm/drm_dp_helper.c | 47 +++++++++++++++++++++--------------------
> 1 file changed, 24 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 9535c5b..9b3426c 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -159,7 +159,7 @@ int drm_dp_bw_code_to_link_rate(u8 link_bw)
> }
> EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate);
>
> -#define AUX_RETRY_INTERVAL 500 /* us */
> +#define AUX_RETRY_INTERVAL 1000 /* us */

Change #1. Increasing the retry interval.

>
> /**
> * DOC: dp helpers
> @@ -177,8 +177,8 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
> unsigned int offset, void *buffer, size_t size)
> {
> struct drm_dp_aux_msg msg;
> - unsigned int retry;
> - int err;
> + unsigned int retry, native_reply;
> + int ret = 0;
>
> memset(&msg, 0, sizeof(msg));
> msg.address = offset;
> @@ -193,35 +193,29 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
> * sufficient, bump to 32 which makes Dell 4k monitors happier.
> */
> for (retry = 0; retry < 32; retry++) {
> + if (ret != 0 && ret != -ETIMEDOUT) {
> + usleep_range(AUX_RETRY_INTERVAL,
> + AUX_RETRY_INTERVAL + 100);

Change #2. Changing the conditions on when to sleep between retries.

> + }
>
> mutex_lock(&aux->hw_mutex);
> - err = aux->transfer(aux, &msg);
> + ret = aux->transfer(aux, &msg);
> mutex_unlock(&aux->hw_mutex);
> - if (err < 0) {
> - if (err == -EBUSY)
> - continue;

Change #3a. Changing which errors lead to retries.

> -
> - return err;
> - }
> -
> -
> - switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {
> - case DP_AUX_NATIVE_REPLY_ACK:
> - if (err < size)
> - return -EPROTO;
> - return err;
>
> - case DP_AUX_NATIVE_REPLY_NACK:
> - return -EIO;

Change #3b. Changing which errors lead to retries.

> + if (ret > 0) {
> + native_reply = msg.reply & DP_AUX_NATIVE_REPLY_MASK;
> + if (native_reply == DP_AUX_NATIVE_REPLY_ACK) {
> + if (ret == size)
> + return ret;
>
> - case DP_AUX_NATIVE_REPLY_DEFER:
> - usleep_range(AUX_RETRY_INTERVAL, AUX_RETRY_INTERVAL + 100);
> - break;
> + ret = -EPROTO;
> + } else
> + ret = -EIO;
> }
> }
>
> DRM_DEBUG_KMS("too many retries, giving up\n");
> - return -EIO;
> + return ret;

Change #4. Propagating errors from the last retry. (The error from the
last retry might not even be representative of what really went down.)

> }
>
> /**
> @@ -241,6 +235,13 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
> ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
> void *buffer, size_t size)
> {
> + /*
> + * Sometimes we just get the same incorrect byte repeated over the
> + * entire buffer. Doing one throw away read initially seems to "solve"
> + * it.
> + */
> + drm_dp_dpcd_access(aux, DP_AUX_NATIVE_READ, DP_DPCD_REV, buffer, 1);
> +

Change #5. Doing a dummy read to ensure the sink is awake.


BR,
Jani.

> return drm_dp_dpcd_access(aux, DP_AUX_NATIVE_READ, offset, buffer,
> size);
> }

--
Jani Nikula, Intel Open Source Technology Center

2016-03-24 18:27:33

by Lyude Paul

Subject: [PATCH v4 0/5] Move workarounds from intel_dp_dpcd_read_wake() into drm's DP helpers

This series of patches takes all of the workarounds we used in
intel_dp_dpcd_read_wake() for working around misbehaving sinks into drm's DP
aux transaction helpers, so that they can be applied to all aux transactions
across each driver. While this patch series was intended to fix issues with the
ThinkPad T560 not bringing displays connected to it's dock back up properly
after suspend, this should fix a lot of other various DP issues by ensuring
that we retry transactions appropriately in every possible failure scenario.

Changes since v3
- Split the patch that moves the logic out of intel_dp_dpcd_read_wake() into
multiple patches for each workaround that we apply, so that bisecting this
change isn't difficult in the event that this breaks something

- Made sure that drm_dp_dpcd_read() only returns the error it encountered during
the first attempted aux transaction, since the error that following retries
encounter might be different from the original

Changes since v2
- Reworked the patch again to incorporate all of the behavior of
intel_dp_dpcd_read_wake() into drm_dp_dpcd_read() and drm_dp_dpcd_access()

Changes since v1
- Patch has been reworked to take the retry logic out of intel_dp_mst_resume()
and into drm_dp_dpcd_access(), based off a suggestion from Daniel Vetter

- Commit message is much longer and gives a better description of the
issue this was originally intended to workaround.

Lyude (5):
drm/dp_helper: Increase retry interval to 1000us
drm/dp_helper: Always wait before retrying native aux transactions
drm/dp_helper: Retry aux transactions on all errors
drm/dp_helper: Perform throw-away read before actual read in
drm_dp_dpcd_read()
drm/i915: Get rid of intel_dp_dpcd_read_wake()

drivers/gpu/drm/drm_dp_helper.c | 55 ++++++++++++++++------------
drivers/gpu/drm/i915/intel_dp.c | 79 ++++++++++++-----------------------------
2 files changed, 54 insertions(+), 80 deletions(-)

--
2.5.5

2016-03-24 18:27:41

by Lyude Paul

Subject: [PATCH v4 1/5] drm/dp_helper: Increase retry interval to 1000us

This is part of a patch series to migrate all of the workarounds for
commonly seen behavior from bad sinks in intel_dp_dpcd_read_wake() to
drm's DP helper.

Signed-off-by: Lyude <[email protected]>
---
drivers/gpu/drm/drm_dp_helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 7d58f59..d1128fb 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -160,7 +160,7 @@ int drm_dp_bw_code_to_link_rate(u8 link_bw)
}
EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate);

-#define AUX_RETRY_INTERVAL 500 /* us */
+#define AUX_RETRY_INTERVAL 1000 /* us */

/**
* DOC: dp helpers
--
2.5.5

2016-03-24 18:27:48

by Lyude Paul

Subject: [PATCH v4 2/5] drm/dp_helper: Always wait before retrying native aux transactions

This is part of a patch series to migrate all of the workarounds for
commonly seen behavior from bad sinks in intel_dp_dpcd_read_wake() to
drm's DP helper.

Some sinks need some time during the process of resuming the system from
sleep before they're ready to handle transactions. While it would be
nice if they responded with NACKs in these scenarios, this isn't always
the case as a few sinks will just timeout on all of the transactions
they receive until they're ready.

Signed-off-by: Lyude <[email protected]>
---
drivers/gpu/drm/drm_dp_helper.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index d1128fb..3b915e2 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -179,7 +179,7 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
{
struct drm_dp_aux_msg msg;
unsigned int retry;
- int err;
+ int err = 0;

memset(&msg, 0, sizeof(msg));
msg.address = offset;
@@ -194,6 +194,10 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
* sufficient, bump to 32 which makes Dell 4k monitors happier.
*/
for (retry = 0; retry < 32; retry++) {
+ if (err != 0 && err != -ETIMEDOUT) {
+ usleep_range(AUX_RETRY_INTERVAL,
+ AUX_RETRY_INTERVAL + 100);
+ }

mutex_lock(&aux->hw_mutex);
err = aux->transfer(aux, &msg);
@@ -205,7 +209,6 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
return err;
}

-
switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {
case DP_AUX_NATIVE_REPLY_ACK:
if (err < size)
@@ -214,10 +217,6 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,

case DP_AUX_NATIVE_REPLY_NACK:
return -EIO;
-
- case DP_AUX_NATIVE_REPLY_DEFER:
- usleep_range(AUX_RETRY_INTERVAL, AUX_RETRY_INTERVAL + 100);
- break;
}
}

--
2.5.5

2016-03-24 18:27:57

by Lyude Paul

Subject: [PATCH v4 4/5] drm/dp_helper: Perform throw-away read before actual read in drm_dp_dpcd_read()

This is part of a patch series to migrate all of the workarounds for
commonly seen behavior from bad sinks in intel_dp_dpcd_read_wake() to drm's
DP helper.

Some sinks will just return garbage for the first aux tranaction they
receive when coming out of sleep mode, so we need to perform an additional
read before the actual read to workaround this.

Signed-off-by: Lyude <[email protected]>
---
drivers/gpu/drm/drm_dp_helper.c | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 86656ca..702c78a 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -244,6 +244,13 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset,
void *buffer, size_t size)
{
+ /*
+ * Sometimes we just get the same incorrect byte repeated over the
+ * entire buffer. Doing one throw away read initially seems to "solve"
+ * it.
+ */
+ drm_dp_dpcd_access(aux, DP_AUX_NATIVE_READ, DP_DPCD_REV, buffer, 1);
+
return drm_dp_dpcd_access(aux, DP_AUX_NATIVE_READ, offset, buffer,
size);
}
--
2.5.5

2016-03-24 18:28:27

by Lyude Paul

Subject: [PATCH v4 5/5] drm/i915: Get rid of intel_dp_dpcd_read_wake()

Since we've fixed up drm_dp_dpcd_read() to allow for retries when things
timeout, there's no use for having this function anymore. Good riddens.

Signed-off-by: Lyude <[email protected]>
---
drivers/gpu/drm/i915/intel_dp.c | 79 ++++++++++++-----------------------------
1 file changed, 22 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f069a82..43c2933 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3184,47 +3184,14 @@ 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. */
@@ -3859,8 +3826,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);
@@ -3871,9 +3838,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");
@@ -3884,9 +3851,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_read(&intel_dp->aux,
+ DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
+ &frame_sync_cap, 1);
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;
@@ -3902,15 +3869,13 @@ 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_read(&intel_dp->aux, DP_EDP_DPCD_REV, &rev, 1) == 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]);
@@ -3933,9 +3898,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;
@@ -3949,11 +3914,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]);
}
@@ -3969,7 +3934,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_read(&intel_dp->aux, DP_MSTM_CAP, buf, 1)) {
if (buf[0] & DP_MST_CAP) {
DRM_DEBUG_KMS("Sink is MST capable\n");
intel_dp->is_mst = true;
@@ -4106,7 +4071,7 @@ 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,
+ return drm_dp_dpcd_read(&intel_dp->aux,
DP_DEVICE_SERVICE_IRQ_VECTOR,
sink_irq_vector, 1) == 1;
}
@@ -4116,7 +4081,7 @@ 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,
+ ret = drm_dp_dpcd_read(&intel_dp->aux,
DP_SINK_COUNT_ESI,
sink_irq_vector, 14);
if (ret != 14)
@@ -4377,7 +4342,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,
+ if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_COUNT,
&reg, 1) < 0)
return connector_status_unknown;

--
2.5.5

2016-03-24 18:28:44

by Lyude Paul

Subject: [PATCH v4 3/5] drm/dp_helper: Retry aux transactions on all errors

This is part of a patch series to migrate all of the workarounds for
commonly seen behavior from bad sinks in intel_dp_dpcd_read_wake() to
drm's DP helper.

We cannot rely on sinks NACKing or deferring when they can't receive
transactions, nor can we rely on any other sort of consistent error to
know when we should stop retrying. As such, we need to just retry
unconditionally on errors. We also make sure here to return the error we
encountered during the first transaction, since it's possible that
retrying the transaction might return a different error then we had
originally.

This, along with the previous patch, work around a weird bug with the
ThinkPad T560's and it's dock. When resuming the laptop, it appears that
there's a short period of time where we're unable to complete any aux
transactions, as they all immediately timeout. The only machine I'm able
to reproduce this on is the T560 as other production Skylake models seem
to be fine. The period during which AUX transactions fail appears to be
around 22ms long. AFAIK, the dock for the T560 never actually turns off,
the only difference is that it's in SST mode at the start of the resume
process, so it's unclear as to why it would need so much time to come
back up.

There's been a discussion on this issue going on for a while on the
intel-gfx mailing list about this that has, in addition to including
developers from Intel, also had the correspondence of one of the
hardware engineers for Intel:

http://www.spinics.net/lists/intel-gfx/msg88831.html
http://www.spinics.net/lists/intel-gfx/msg88410.html

We've already looked into a couple of possible explanations for the
problem:

- Calling intel_dp_mst_resume() before right fix.
intel_runtime_pm_enable_interrupts(). This was the first fix I tried,
and while it worked it definitely wasn't the right fix. This worked
because DP aux transactions don't actually require interrupts to work:

static uint32_t
intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
{
struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
struct drm_device *dev = intel_dig_port->base.base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
i915_reg_t ch_ctl = intel_dp->aux_ch_ctl_reg;
uint32_t status;
bool done;

#define C (((status = I915_READ_NOTRACE(ch_ctl)) & DP_AUX_CH_CTL_SEND_BUSY) == 0)
if (has_aux_irq)
done = wait_event_timeout(dev_priv->gmbus_wait_queue, C,
msecs_to_jiffies_timeout(10));
else
done = wait_for_atomic(C, 10) == 0;
if (!done)
DRM_ERROR("dp aux hw did not signal timeout (has irq: %i)!\n",
has_aux_irq);
#undef C

return status;
}

When there's no interrupts enabled, we end up timing out on the
wait_event_timeout() call, which causes us to check the DP status
register once to see if the transaction was successful or not. Since
this adds a 10ms delay to each aux transaction, it ends up adding a
long enough delay to the resume process for aux transactions to become
functional again. This gave us the illusion that enabling interrupts
had something to do with making things work again, and put me on the
wrong track for a while.

- Interrupts occurring when we try to perform the aux transactions
required to put the dock back into MST mode. This isn't the problem,
as the only interrupts I've observed that come during this timeout
period are from the snd_hda_intel driver, and disabling that driver
doesn't appear to change the behavior at all.

- Skylake's PSR block causing issues by performing aux transactions
while we try to bring the dock out of MST mode. Disabling PSR through
i915's command line options doesn't seem to change the behavior
either, nor does preventing the DMC firmware from being loaded.

Since this investigation went on for about 2 weeks, we decided it would
be better for the time being to just workaround this issue by making
sure AUX transactions wait a short period of time before retrying.

Signed-off-by: Lyude <[email protected]>
---
drivers/gpu/drm/drm_dp_helper.c | 39 +++++++++++++++++++++------------------
1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index 3b915e2..86656ca 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -178,8 +178,8 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
unsigned int offset, void *buffer, size_t size)
{
struct drm_dp_aux_msg msg;
- unsigned int retry;
- int err = 0;
+ unsigned int retry, native_reply;
+ int err = 0, ret = 0;

memset(&msg, 0, sizeof(msg));
msg.address = offset;
@@ -194,34 +194,37 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
* sufficient, bump to 32 which makes Dell 4k monitors happier.
*/
for (retry = 0; retry < 32; retry++) {
- if (err != 0 && err != -ETIMEDOUT) {
+ if (ret != 0 && ret != -ETIMEDOUT) {
usleep_range(AUX_RETRY_INTERVAL,
AUX_RETRY_INTERVAL + 100);
}

mutex_lock(&aux->hw_mutex);
- err = aux->transfer(aux, &msg);
+ ret = aux->transfer(aux, &msg);
mutex_unlock(&aux->hw_mutex);
- if (err < 0) {
- if (err == -EBUSY)
- continue;

- return err;
- }
-
- switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {
- case DP_AUX_NATIVE_REPLY_ACK:
- if (err < size)
- return -EPROTO;
- return err;
+ if (ret > 0) {
+ native_reply = msg.reply & DP_AUX_NATIVE_REPLY_MASK;
+ if (native_reply == DP_AUX_NATIVE_REPLY_ACK) {
+ if (ret == size)
+ return ret;

- case DP_AUX_NATIVE_REPLY_NACK:
- return -EIO;
+ ret = -EPROTO;
+ } else
+ ret = -EIO;
}
+
+ /*
+ * We want the error we return to be the error we received on
+ * the first transaction, since we may get a different error the
+ * next time we retry
+ */
+ if (!err)
+ err = ret;
}

DRM_DEBUG_KMS("too many retries, giving up\n");
- return -EIO;
+ return err;
}

/**
--
2.5.5