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@xxxxxxxxx> On Fri, Jun 30, 2017 at 12:08 PM, Jim Bride <jim.bride@xxxxxxxxxxxxxxx> 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@xxxxxxxxx> > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > Cc: Jani Nikula <jani.nikula@xxxxxxxxx> > Signed-off-by: Jim Bride <jim.bride@xxxxxxxxxxxxxxx> > --- > 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@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Rodrigo Vivi Blog: http://blog.vivi.eng.br _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx