| Message ID | 1498849693-26240-5-git-send-email-jim.bride@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
my only bikeshed here is the subject since this is not the psr feature itself... anyways, I don't mind much since sink crc is only used to test psr... Also yes, based on what I saw on sink crc behaviour I know that this doesn't fix things for real because I remember reading hundreds or thousand incorrect values with on increment on count... But anyways I believe that this improve the situation So, Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> On Fri, Jun 30, 2017 at 12:08 PM, 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> > 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 b46fa03..1fe0975 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3927,6 +3927,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) > @@ -3950,11 +3958,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; > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index b46fa03..1fe0975 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3927,6 +3927,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) @@ -3950,11 +3958,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> 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(-)