Re: [Intel-gfx] [PATCH 1/2] drm/i915: Do a dummy DPCD read before the actual read
On Thu, Oct 16, 2014 at 12:39:29PM -0700, Todd Previte wrote:
On 10/16/2014 10:46 AM, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
Sometimes we seem to get utter garbage from DPCD reads. The resulting
buffer is filled with the same byte, and the operation completed without
errors. My HP ZR24w monitor seems particularly susceptible to this
problem once it's gone into a sleep mode.
The issue seems to happen only for the first AUX message that wakes the
sink up. But as the first AUX read we often do is the DPCD receiver
cap it does wreak a bit of havoc with subsequent link training etc. when
the receiver cap bw/lane/etc. information is garbage.
A sufficient workaround seems to be to perform a single byte dummy read
before reading the actual data. I suppose that just wakes up the sink
sufficiently and we can just throw away the returned data in case it's
crap. DP_DPCD_REV seems like a sufficiently safe location to read here.
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/intel_dp.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c
b/drivers/gpu/drm/i915/intel_dp.c
index 64c8e04..f07f02c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2870,6 +2870,13 @@ intel_dp_dpcd_read_wake(struct drm_dp_aux *aux,
unsigned int offset,
ssize_t ret;
int i;
+ /*
+* Sometime we just get the same incorrect byte repeated
+* over the entire buffer. Doing just one throw away read
+* initially seems to solve it.
+*/
+ drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
+
for (i = 0; i 3; i++) {
ret = drm_dp_dpcd_read(aux, offset, buffer, size);
if (ret == size)
Seems like a reasonable workaround for this problem, though
investigating the actual root cause might be worthwhile.
Sure. If someone has an AUX analyzer and a HP ZR24w monitor it should
be trivial to look at the traffic and see if there's something bogus in
our AUX communication. Sadly I don't have an AUX analyzer.
Reviewed-by: Todd Previte tprev...@gmail.com
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Do a dummy DPCD read before the actual read
On Thu, Oct 16, 2014 at 12:39:29PM -0700, Todd Previte wrote:
On 10/16/2014 10:46 AM, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
Sometimes we seem to get utter garbage from DPCD reads. The resulting
buffer is filled with the same byte, and the operation completed without
errors. My HP ZR24w monitor seems particularly susceptible to this
problem once it's gone into a sleep mode.
The issue seems to happen only for the first AUX message that wakes the
sink up. But as the first AUX read we often do is the DPCD receiver
cap it does wreak a bit of havoc with subsequent link training etc. when
the receiver cap bw/lane/etc. information is garbage.
A sufficient workaround seems to be to perform a single byte dummy read
before reading the actual data. I suppose that just wakes up the sink
sufficiently and we can just throw away the returned data in case it's
crap. DP_DPCD_REV seems like a sufficiently safe location to read here.
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/intel_dp.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c
b/drivers/gpu/drm/i915/intel_dp.c
index 64c8e04..f07f02c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2870,6 +2870,13 @@ intel_dp_dpcd_read_wake(struct drm_dp_aux *aux,
unsigned int offset,
ssize_t ret;
int i;
+/*
+ * Sometime we just get the same incorrect byte repeated
+ * over the entire buffer. Doing just one throw away read
+ * initially seems to solve it.
+ */
+drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
+
for (i = 0; i 3; i++) {
ret = drm_dp_dpcd_read(aux, offset, buffer, size);
if (ret == size)
Seems like a reasonable workaround for this problem, though investigating
the actual root cause might be worthwhile.
Reviewed-by: Todd Previte tprev...@gmail.com
Cc: sta...@vger.kernel.org
... one for Jani I guess. But I'm suspicious here too, so maybe we should
extract this read_wake function to the core dp helpers and convince that
some other driver should use it? Adding more people and lists.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Do a dummy DPCD read before the actual read
On Thu, 16 Oct 2014, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
Sometimes we seem to get utter garbage from DPCD reads. The resulting
buffer is filled with the same byte, and the operation completed without
errors. My HP ZR24w monitor seems particularly susceptible to this
problem once it's gone into a sleep mode.
The issue seems to happen only for the first AUX message that wakes the
sink up. But as the first AUX read we often do is the DPCD receiver
cap it does wreak a bit of havoc with subsequent link training etc. when
the receiver cap bw/lane/etc. information is garbage.
This makes me suspect our sink dpms and wake handling even more than I
already did. Someone(tm) should dig into the DP and hw specs again with
fresh eyes...
A sufficient workaround seems to be to perform a single byte dummy read
before reading the actual data. I suppose that just wakes up the sink
sufficiently and we can just throw away the returned data in case it's
crap. DP_DPCD_REV seems like a sufficiently safe location to read here.
Seems like a pretty harmless thing to do, and we already do loads of
dpcd reads anyway. We should throw this at some related bugs.
So ack.
BR,
Jani.
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/intel_dp.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 64c8e04..f07f02c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2870,6 +2870,13 @@ intel_dp_dpcd_read_wake(struct drm_dp_aux *aux,
unsigned int offset,
ssize_t ret;
int i;
+ /*
+ * Sometime we just get the same incorrect byte repeated
+ * over the entire buffer. Doing just one throw away read
+ * initially seems to solve it.
+ */
+ drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
+
for (i = 0; i 3; i++) {
ret = drm_dp_dpcd_read(aux, offset, buffer, size);
if (ret == size)
--
2.0.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Do a dummy DPCD read before the actual read
On Fri, Oct 17, 2014 at 11:43:21AM +0300, Jani Nikula wrote:
On Thu, 16 Oct 2014, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
Sometimes we seem to get utter garbage from DPCD reads. The resulting
buffer is filled with the same byte, and the operation completed without
errors. My HP ZR24w monitor seems particularly susceptible to this
problem once it's gone into a sleep mode.
The issue seems to happen only for the first AUX message that wakes the
sink up. But as the first AUX read we often do is the DPCD receiver
cap it does wreak a bit of havoc with subsequent link training etc. when
the receiver cap bw/lane/etc. information is garbage.
This makes me suspect our sink dpms and wake handling even more than I
already did. Someone(tm) should dig into the DP and hw specs again with
fresh eyes...
Yeah when I last looked at the spec it was rather vague on the subject.
On the other hand it seems to suggest that you're supposed to wake up
the sink via DP_SET_POWER before any other AUX transfer, but on the
other hand it seems to say AUX is fine as long as you remember to do
the extended retry in case the AUX circuitry was asleep in the sink.
However DPCD 1.0 doesn't even support DP_SET_POWER so that does suggest
that it's not really mandatory to wake things up first. Well, assuming
DPCD 1.0 devices even exist.
I was a bit suspicious of our AUX code as well, but IIRC didn't spot
anything obviously incorrect there when I had a look. So might be this
is just the sink being a bit buggy.
A sufficient workaround seems to be to perform a single byte dummy read
before reading the actual data. I suppose that just wakes up the sink
sufficiently and we can just throw away the returned data in case it's
crap. DP_DPCD_REV seems like a sufficiently safe location to read here.
Seems like a pretty harmless thing to do, and we already do loads of
dpcd reads anyway. We should throw this at some related bugs.
So ack.
BR,
Jani.
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/intel_dp.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c
b/drivers/gpu/drm/i915/intel_dp.c
index 64c8e04..f07f02c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2870,6 +2870,13 @@ intel_dp_dpcd_read_wake(struct drm_dp_aux *aux,
unsigned int offset,
ssize_t ret;
int i;
+ /*
+* Sometime we just get the same incorrect byte repeated
+* over the entire buffer. Doing just one throw away read
+* initially seems to solve it.
+*/
+ drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
+
for (i = 0; i 3; i++) {
ret = drm_dp_dpcd_read(aux, offset, buffer, size);
if (ret == size)
--
2.0.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Do a dummy DPCD read before the actual read
On 10/17/2014 1:43 AM, Jani Nikula wrote:
On Thu, 16 Oct 2014, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
Sometimes we seem to get utter garbage from DPCD reads. The resulting
buffer is filled with the same byte, and the operation completed without
errors. My HP ZR24w monitor seems particularly susceptible to this
problem once it's gone into a sleep mode.
The issue seems to happen only for the first AUX message that wakes the
sink up. But as the first AUX read we often do is the DPCD receiver
cap it does wreak a bit of havoc with subsequent link training etc. when
the receiver cap bw/lane/etc. information is garbage.
This makes me suspect our sink dpms and wake handling even more than I
already did. Someone(tm) should dig into the DP and hw specs again with
fresh eyes...
I can go look into this today/next week. This problem sounds vaguely
similar to one I've run across before.
-T
A sufficient workaround seems to be to perform a single byte dummy read
before reading the actual data. I suppose that just wakes up the sink
sufficiently and we can just throw away the returned data in case it's
crap. DP_DPCD_REV seems like a sufficiently safe location to read here.
Seems like a pretty harmless thing to do, and we already do loads of
dpcd reads anyway. We should throw this at some related bugs.
So ack.
BR,
Jani.
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/intel_dp.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 64c8e04..f07f02c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2870,6 +2870,13 @@ intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned
int offset,
ssize_t ret;
int i;
+ /*
+* Sometime we just get the same incorrect byte repeated
+* over the entire buffer. Doing just one throw away read
+* initially seems to solve it.
+*/
+ drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
+
for (i = 0; i 3; i++) {
ret = drm_dp_dpcd_read(aux, offset, buffer, size);
if (ret == size)
--
2.0.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Do a dummy DPCD read before the actual read
On 10/17/2014 2:06 AM, Ville Syrjälä wrote:
On Thu, Oct 16, 2014 at 12:39:29PM -0700, Todd Previte wrote:
On 10/16/2014 10:46 AM, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
Sometimes we seem to get utter garbage from DPCD reads. The resulting
buffer is filled with the same byte, and the operation completed without
errors. My HP ZR24w monitor seems particularly susceptible to this
problem once it's gone into a sleep mode.
The issue seems to happen only for the first AUX message that wakes the
sink up. But as the first AUX read we often do is the DPCD receiver
cap it does wreak a bit of havoc with subsequent link training etc. when
the receiver cap bw/lane/etc. information is garbage.
A sufficient workaround seems to be to perform a single byte dummy read
before reading the actual data. I suppose that just wakes up the sink
sufficiently and we can just throw away the returned data in case it's
crap. DP_DPCD_REV seems like a sufficiently safe location to read here.
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/intel_dp.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 64c8e04..f07f02c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2870,6 +2870,13 @@ intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned
int offset,
ssize_t ret;
int i;
+ /*
+* Sometime we just get the same incorrect byte repeated
+* over the entire buffer. Doing just one throw away read
+* initially seems to solve it.
+*/
+ drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
+
for (i = 0; i 3; i++) {
ret = drm_dp_dpcd_read(aux, offset, buffer, size);
if (ret == size)
Seems like a reasonable workaround for this problem, though
investigating the actual root cause might be worthwhile.
Sure. If someone has an AUX analyzer and a HP ZR24w monitor it should
be trivial to look at the traffic and see if there's something bogus in
our AUX communication. Sadly I don't have an AUX analyzer.
I've got the monitor on my desk but no AUX analyzer to use. I'll see if
I can track one down.
-T
Reviewed-by: Todd Previte tprev...@gmail.com
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Do a dummy DPCD read before the actual read
On 10/17/2014 1:59 AM, Ville Syrjälä wrote:
On Fri, Oct 17, 2014 at 11:43:21AM +0300, Jani Nikula wrote:
On Thu, 16 Oct 2014, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
Sometimes we seem to get utter garbage from DPCD reads. The resulting
buffer is filled with the same byte, and the operation completed without
errors. My HP ZR24w monitor seems particularly susceptible to this
problem once it's gone into a sleep mode.
The issue seems to happen only for the first AUX message that wakes the
sink up. But as the first AUX read we often do is the DPCD receiver
cap it does wreak a bit of havoc with subsequent link training etc. when
the receiver cap bw/lane/etc. information is garbage.
This makes me suspect our sink dpms and wake handling even more than I
already did. Someone(tm) should dig into the DP and hw specs again with
fresh eyes...
Yeah when I last looked at the spec it was rather vague on the subject.
On the other hand it seems to suggest that you're supposed to wake up
the sink via DP_SET_POWER before any other AUX transfer, but on the
other hand it seems to say AUX is fine as long as you remember to do
the extended retry in case the AUX circuitry was asleep in the sink.
The sink is supposed to exit power-down mode within 1ms of detecting a
differential signal voltage on the AUX lines, according to the spec. So
in theory, any AUX transactions should be able to wake up the sink
device. As you pointed out, the AUX retries will catch the case where
the AUX channel is powered down. I know of one instance where the write
to SET_POWER is required, and that's in one of the compliance tests.
Outside of that though, I haven't seen anything where it's mandatory.
-T
However DPCD 1.0 doesn't even support DP_SET_POWER so that does suggest
that it's not really mandatory to wake things up first. Well, assuming
DPCD 1.0 devices even exist.
I was a bit suspicious of our AUX code as well, but IIRC didn't spot
anything obviously incorrect there when I had a look. So might be this
is just the sink being a bit buggy.
A sufficient workaround seems to be to perform a single byte dummy read
before reading the actual data. I suppose that just wakes up the sink
sufficiently and we can just throw away the returned data in case it's
crap. DP_DPCD_REV seems like a sufficiently safe location to read here.
Seems like a pretty harmless thing to do, and we already do loads of
dpcd reads anyway. We should throw this at some related bugs.
So ack.
BR,
Jani.
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/intel_dp.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 64c8e04..f07f02c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2870,6 +2870,13 @@ intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned
int offset,
ssize_t ret;
int i;
+ /*
+* Sometime we just get the same incorrect byte repeated
+* over the entire buffer. Doing just one throw away read
+* initially seems to solve it.
+*/
+ drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
+
for (i = 0; i 3; i++) {
ret = drm_dp_dpcd_read(aux, offset, buffer, size);
if (ret == size)
--
2.0.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Do a dummy DPCD read before the actual read
On 10/16/2014 10:46 AM, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
Sometimes we seem to get utter garbage from DPCD reads. The resulting
buffer is filled with the same byte, and the operation completed without
errors. My HP ZR24w monitor seems particularly susceptible to this
problem once it's gone into a sleep mode.
The issue seems to happen only for the first AUX message that wakes the
sink up. But as the first AUX read we often do is the DPCD receiver
cap it does wreak a bit of havoc with subsequent link training etc. when
the receiver cap bw/lane/etc. information is garbage.
A sufficient workaround seems to be to perform a single byte dummy read
before reading the actual data. I suppose that just wakes up the sink
sufficiently and we can just throw away the returned data in case it's
crap. DP_DPCD_REV seems like a sufficiently safe location to read here.
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/intel_dp.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 64c8e04..f07f02c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2870,6 +2870,13 @@ intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned
int offset,
ssize_t ret;
int i;
+ /*
+* Sometime we just get the same incorrect byte repeated
+* over the entire buffer. Doing just one throw away read
+* initially seems to solve it.
+*/
+ drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
+
for (i = 0; i 3; i++) {
ret = drm_dp_dpcd_read(aux, offset, buffer, size);
if (ret == size)
Seems like a reasonable workaround for this problem, though
investigating the actual root cause might be worthwhile.
Reviewed-by: Todd Previte tprev...@gmail.com
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
