| Message ID | 1499811596-14634-3-git-send-email-jim.bride@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
If you had sent these 2 in a separated series I believe it would had passed CI so I could merge today. Also it would be better if you want to speed up things with a bit of sense of progress since the other 2 patches in this series will probably require some rework. On Tue, 2017-07-11 at 15:19 -0700, Jim Bride wrote: > According to the eDP spec, when the count field in TEST_SINK_MISC > increments then the six bytes of sink CRC information in the DPCD > should be valid. Unfortunately, this doesn't seem to be the case > on some panels, and as a result we get some incorrect and inconsistent > values from the sink CRC DPCD locations at times. This problem exhibits > itself more on faster processors (relative failure rates HSW < SKL < KBL.) > In order to try and account for this, we try a lot harder to read the sink > CRC until we get consistent values twice in a row before returning what we > read and delay for a time before trying to read. We still see some > occasional failures, but reading the sink CRC is much more reliable, > particularly on SKL and KBL, with these changes than without. > > v2: * Reduce number of retries when reading the sink CRC (Jani) > * Refactor to minimize changes to the code (Jani) > * Rebase > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Jim Bride <jim.bride@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 40 ++++++++++++++++++++++++++++++++++++---- > 1 file changed, 36 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 2d42d09..69c8130c 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3906,6 +3906,14 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) > u8 buf; > int count, ret; > int attempts = 6; > + u8 old_crc[6]; > + > + if (crc != NULL) { > + memset(crc, 0, 6); > + memset(old_crc, 0xff, 6); > + } else { > + return -ENOMEM; > + } > > ret = intel_dp_sink_crc_start(intel_dp); > if (ret) > @@ -3929,11 +3937,35 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) > goto stop; > } > > - if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) { > - ret = -EIO; > - goto stop; > - } > + attempts = 6; > + > + /* > + * Sometimes it takes a while for the "real" CRC values to land in > + * the DPCD, so try several times until we get two reads in a row > + * that are the same. If we're an eDP panel, delay between reads > + * for a while since the values take a bit longer to propagate. > + */ > + do { > + intel_wait_for_vblank(dev_priv, intel_crtc->pipe); > + if (is_edp(intel_dp)) > + usleep_range(20000, 25000); > + > + if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, > + crc, 6) < 0) { > + ret = -EIO; > + goto stop; > + } > + > + if (memcmp(old_crc, crc, 6) == 0) { > + ret = 0; > + goto stop; > + } else { > + memcpy(old_crc, crc, 6); > + } > + } while (--attempts); > > + DRM_DEBUG_KMS("Failed to get CRC after 6 attempts.\n"); > + ret = -ETIMEDOUT; > stop: > intel_dp_sink_crc_stop(intel_dp); > return ret;
On Tuesday, July 11, 2017 3:19:54 PM PDT Jim Bride wrote: > According to the eDP spec, when the count field in TEST_SINK_MISC > increments then the six bytes of sink CRC information in the DPCD > should be valid. Unfortunately, this doesn't seem to be the case > on some panels, and as a result we get some incorrect and inconsistent > values from the sink CRC DPCD locations at times. This problem exhibits > itself more on faster processors (relative failure rates HSW < SKL < KBL.) > In order to try and account for this, we try a lot harder to read the sink > CRC until we get consistent values twice in a row before returning what we > read and delay for a time before trying to read. We still see some > occasional failures, but reading the sink CRC is much more reliable, > particularly on SKL and KBL, with these changes than without. What's the goal here? Is this retry loop being added to deal with an IGT failure? A bit of context as to how this is related to PSR will be helpful. > > v2: * Reduce number of retries when reading the sink CRC (Jani) > * Refactor to minimize changes to the code (Jani) > * Rebase > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Jim Bride <jim.bride@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 40 > ++++++++++++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), > 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > b/drivers/gpu/drm/i915/intel_dp.c index 2d42d09..69c8130c 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3906,6 +3906,14 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 > *crc) u8 buf; > int count, ret; > int attempts = 6; > + u8 old_crc[6]; > + > + if (crc != NULL) { Why is this needed? > + memset(crc, 0, 6); > + memset(old_crc, 0xff, 6); I don't know much about crc values, is 0xff a known invalid value? > + } else { > + return -ENOMEM; > + } > > ret = intel_dp_sink_crc_start(intel_dp); > if (ret) > @@ -3929,11 +3937,35 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 > *crc) goto stop; > } > > - if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) { > - ret = -EIO; > - goto stop; > - } > + attempts = 6; > + > + /* > + * Sometimes it takes a while for the "real" CRC values to land in > + * the DPCD, so try several times until we get two reads in a row > + * that are the same. If we're an eDP panel, delay between reads > + * for a while since the values take a bit longer to propagate. > + */ > + do { > + intel_wait_for_vblank(dev_priv, intel_crtc->pipe); Why wait for a vblank if the idea is to check for consistent crc values for the same frame (I'm assuming that is the idea) ? > + if (is_edp(intel_dp)) > + usleep_range(20000, 25000); > + > + if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, > + crc, 6) < 0) { > + ret = -EIO; > + goto stop; > + } > + > + if (memcmp(old_crc, crc, 6) == 0) { > + ret = 0; > + goto stop; > + } else { > + memcpy(old_crc, crc, 6); > + } > + } while (--attempts); > > + DRM_DEBUG_KMS("Failed to get CRC after 6 attempts.\n"); > + ret = -ETIMEDOUT; > stop: > intel_dp_sink_crc_stop(intel_dp); > return ret;
On Tue, 11 Jul 2017, Jim Bride <jim.bride@linux.intel.com> wrote: > According to the eDP spec, when the count field in TEST_SINK_MISC > increments then the six bytes of sink CRC information in the DPCD > should be valid. Unfortunately, this doesn't seem to be the case > on some panels, and as a result we get some incorrect and inconsistent > values from the sink CRC DPCD locations at times. This problem exhibits > itself more on faster processors (relative failure rates HSW < SKL < KBL.) > In order to try and account for this, we try a lot harder to read the sink > CRC until we get consistent values twice in a row before returning what we > read and delay for a time before trying to read. We still see some > occasional failures, but reading the sink CRC is much more reliable, > particularly on SKL and KBL, with these changes than without. > > v2: * Reduce number of retries when reading the sink CRC (Jani) > * Refactor to minimize changes to the code (Jani) > * Rebase > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Jim Bride <jim.bride@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 40 ++++++++++++++++++++++++++++++++++++---- > 1 file changed, 36 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 2d42d09..69c8130c 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3906,6 +3906,14 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) > u8 buf; > int count, ret; > int attempts = 6; > + u8 old_crc[6]; > + > + if (crc != NULL) { As DK said, please drop the check. > + memset(crc, 0, 6); > + memset(old_crc, 0xff, 6); Both unnecessary, see below. > + } else { > + return -ENOMEM; > + } > > ret = intel_dp_sink_crc_start(intel_dp); > if (ret) > @@ -3929,11 +3937,35 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) > goto stop; > } > > - if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) { > - ret = -EIO; > - goto stop; > - } > + attempts = 6; > + > + /* > + * Sometimes it takes a while for the "real" CRC values to land in > + * the DPCD, so try several times until we get two reads in a row > + * that are the same. If we're an eDP panel, delay between reads > + * for a while since the values take a bit longer to propagate. > + */ > + do { Never use a do-while when a for loop will do. for (i = 0; i < 6; i++) gets interpreted in the spine, no need for further processing. > + intel_wait_for_vblank(dev_priv, intel_crtc->pipe); > + if (is_edp(intel_dp)) > + usleep_range(20000, 25000); Is the intention to do these *between* reads? If yes, then move this *after* the memcmp to only do this between reads. > + > + if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, > + crc, 6) < 0) { > + ret = -EIO; > + goto stop; break; > + } > + > + if (memcmp(old_crc, crc, 6) == 0) { > + ret = 0; > + goto stop; > + } else { > + memcpy(old_crc, crc, 6); > + } After you've switched this to the for loop, you can do: if (i && memcmp(old_crc, crc, sizeof(old_crc)) == 0) break; memcpy(old_crc, crc, sizeof(old_crc)); > + } while (--attempts); > if (i == 6) { > + DRM_DEBUG_KMS("Failed to get CRC after 6 attempts.\n"); > + ret = -ETIMEDOUT; } > stop: > intel_dp_sink_crc_stop(intel_dp); > return ret;
On Fri, Jul 14, 2017 at 12:46:08PM +0300, Jani Nikula wrote: > On Tue, 11 Jul 2017, Jim Bride <jim.bride@linux.intel.com> wrote: > > According to the eDP spec, when the count field in TEST_SINK_MISC > > increments then the six bytes of sink CRC information in the DPCD > > should be valid. Unfortunately, this doesn't seem to be the case > > on some panels, and as a result we get some incorrect and inconsistent > > values from the sink CRC DPCD locations at times. This problem exhibits > > itself more on faster processors (relative failure rates HSW < SKL < KBL.) > > In order to try and account for this, we try a lot harder to read the sink > > CRC until we get consistent values twice in a row before returning what we > > read and delay for a time before trying to read. We still see some > > occasional failures, but reading the sink CRC is much more reliable, > > particularly on SKL and KBL, with these changes than without. > > > > v2: * Reduce number of retries when reading the sink CRC (Jani) > > * Refactor to minimize changes to the code (Jani) > > * Rebase > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: Jim Bride <jim.bride@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 40 ++++++++++++++++++++++++++++++++++++---- > > 1 file changed, 36 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 2d42d09..69c8130c 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -3906,6 +3906,14 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) > > u8 buf; > > int count, ret; > > int attempts = 6; > > + u8 old_crc[6]; > > + > > + if (crc != NULL) { > > As DK said, please drop the check. > > > + memset(crc, 0, 6); > > + memset(old_crc, 0xff, 6); > > Both unnecessary, see below. > > > + } else { > > + return -ENOMEM; > > + } > > > > ret = intel_dp_sink_crc_start(intel_dp); > > if (ret) > > @@ -3929,11 +3937,35 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) > > goto stop; > > } > > > > - if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) { > > - ret = -EIO; > > - goto stop; > > - } > > + attempts = 6; > > + > > + /* > > + * Sometimes it takes a while for the "real" CRC values to land in > > + * the DPCD, so try several times until we get two reads in a row > > + * that are the same. If we're an eDP panel, delay between reads > > + * for a while since the values take a bit longer to propagate. > > + */ > > + do { > > Never use a do-while when a for loop will do. for (i = 0; i < 6; i++) > gets interpreted in the spine, no need for further processing. I had used do-while because it was used earlier to read the test count (code I didn't touch.) In any event, I can convert to a for loop easy enough. > > + intel_wait_for_vblank(dev_priv, intel_crtc->pipe); > > + if (is_edp(intel_dp)) > > + usleep_range(20000, 25000); > > Is the intention to do these *between* reads? If yes, then move this > *after* the memcmp to only do this between reads. Moved. > > + > > + if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, > > + crc, 6) < 0) { > > + ret = -EIO; > > + goto stop; > > break; Changed. > > + } > > + > > + if (memcmp(old_crc, crc, 6) == 0) { > > + ret = 0; > > + goto stop; > > + } else { > > + memcpy(old_crc, crc, 6); > > + } > > After you've switched this to the for loop, you can do: > > if (i && memcmp(old_crc, crc, sizeof(old_crc)) == 0) > break; > memcpy(old_crc, crc, sizeof(old_crc)); Changed. I'll submit a new version of the patch after I retest. Jim > > + } while (--attempts); > > > > if (i == 6) { > > > + DRM_DEBUG_KMS("Failed to get CRC after 6 attempts.\n"); > > + ret = -ETIMEDOUT; > > } > > > stop: > > intel_dp_sink_crc_stop(intel_dp); > > return ret; > > -- > Jani Nikula, Intel Open Source Technology Center
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 2d42d09..69c8130c 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3906,6 +3906,14 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) u8 buf; int count, ret; int attempts = 6; + u8 old_crc[6]; + + if (crc != NULL) { + memset(crc, 0, 6); + memset(old_crc, 0xff, 6); + } else { + return -ENOMEM; + } ret = intel_dp_sink_crc_start(intel_dp); if (ret) @@ -3929,11 +3937,35 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) goto stop; } - if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) { - ret = -EIO; - goto stop; - } + attempts = 6; + + /* + * Sometimes it takes a while for the "real" CRC values to land in + * the DPCD, so try several times until we get two reads in a row + * that are the same. If we're an eDP panel, delay between reads + * for a while since the values take a bit longer to propagate. + */ + do { + intel_wait_for_vblank(dev_priv, intel_crtc->pipe); + if (is_edp(intel_dp)) + usleep_range(20000, 25000); + + if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, + crc, 6) < 0) { + ret = -EIO; + goto stop; + } + + if (memcmp(old_crc, crc, 6) == 0) { + ret = 0; + goto stop; + } else { + memcpy(old_crc, crc, 6); + } + } while (--attempts); + DRM_DEBUG_KMS("Failed to get CRC after 6 attempts.\n"); + ret = -ETIMEDOUT; stop: intel_dp_sink_crc_stop(intel_dp); return ret;
According to the eDP spec, when the count field in TEST_SINK_MISC increments then the six bytes of sink CRC information in the DPCD should be valid. Unfortunately, this doesn't seem to be the case on some panels, and as a result we get some incorrect and inconsistent values from the sink CRC DPCD locations at times. This problem exhibits itself more on faster processors (relative failure rates HSW < SKL < KBL.) In order to try and account for this, we try a lot harder to read the sink CRC until we get consistent values twice in a row before returning what we read and delay for a time before trying to read. We still see some occasional failures, but reading the sink CRC is much more reliable, particularly on SKL and KBL, with these changes than without. v2: * Reduce number of retries when reading the sink CRC (Jani) * Refactor to minimize changes to the code (Jani) * Rebase Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Cc: Jani Nikula <jani.nikula@intel.com> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: Jim Bride <jim.bride@linux.intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 40 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 36 insertions(+), 4 deletions(-)