| Message ID | 1440112320-21884-1-git-send-email-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Aug 20, 2015 at 04:12:00PM -0700, Rodrigo Vivi wrote: > From: Rodrigo Vivi <vivijim@rdvivi-budapest.jf.intel.com> > > Let's use a native read with retry as suggested per spec to > fix Sink CRC on SKL when PSR is enabled. > > With PSR enabled panel is probably taking more time to wake > and dpcd read is faling. > > Cc: Sonika Jindal <sonika.jindal@intel.com> > Signed-off-by: Rodrigo Vivi <vivijim@rdvivi-budapest.jf.intel.com> Seems like we should just move the trickery we do in our own version into the dp helpers in the core if this is needed all over the place? At least in i915 we use it everywhere and it doesn't seem actively harmful really ... Maybe the only exception would be the i2c-over-dp_aux code. -Daniel > --- > drivers/gpu/drm/i915/intel_dp.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index d32ce48..34f5e33 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4037,7 +4037,8 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp) > u8 buf; > int ret = 0; > > - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) { > + if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK, > + &buf, 1) < 0) { > DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n"); > ret = -EIO; > goto out; > @@ -4069,7 +4070,8 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp) > return ret; > } > > - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0) > + if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK_MISC, > + &buf, 1) < 0) > return -EIO; > > if (!(buf & DP_TEST_CRC_SUPPORTED)) > @@ -4077,7 +4079,7 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp) > > intel_dp->sink_crc.last_count = buf & DP_TEST_COUNT_MASK; > > - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) > + if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK, &buf, 1) < 0) > return -EIO; > > hsw_disable_ips(intel_crtc); > @@ -4109,8 +4111,8 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) > do { > intel_wait_for_vblank(dev, intel_crtc->pipe); > > - if (drm_dp_dpcd_readb(&intel_dp->aux, > - DP_TEST_SINK_MISC, &buf) < 0) { > + if (intel_dp_dpcd_read_wake(&intel_dp->aux, > + DP_TEST_SINK_MISC, &buf, 1) < 0) { > ret = -EIO; > goto stop; > } > @@ -4123,7 +4125,8 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) > if (count == 0) > intel_dp->sink_crc.last_count = 0; > > - if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) { > + if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_CRC_R_CR, > + crc, 6) < 0) { > ret = -EIO; > goto stop; > } > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, 2015-08-26 at 11:06 +0200, Daniel Vetter wrote: > On Thu, Aug 20, 2015 at 04:12:00PM -0700, Rodrigo Vivi wrote: > > From: Rodrigo Vivi <vivijim@rdvivi-budapest.jf.intel.com> > > > > Let's use a native read with retry as suggested per spec to > > fix Sink CRC on SKL when PSR is enabled. > > > > With PSR enabled panel is probably taking more time to wake > > and dpcd read is faling. > > > > Cc: Sonika Jindal <sonika.jindal@intel.com> > > Signed-off-by: Rodrigo Vivi <vivijim@rdvivi-budapest.jf.intel.com> > > Seems like we should just move the trickery we do in our own version > into > the dp helpers in the core if this is needed all over the place? I've wondered this, but I thought there was a good reason to let this trick separated. > At least in i915 we use it everywhere and it doesn't seem actively > harmful > really ... Maybe the only exception would be the i2c-over-dp_aux > code. Why this would be the exception? Maybe this was the good reason? > -Daniel > > --- > > drivers/gpu/drm/i915/intel_dp.c | 15 +++++++++------ > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > > b/drivers/gpu/drm/i915/intel_dp.c > > index d32ce48..34f5e33 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -4037,7 +4037,8 @@ static int intel_dp_sink_crc_stop(struct > > intel_dp *intel_dp) > > u8 buf; > > int ret = 0; > > > > - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) > > < 0) { > > + if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK, > > + &buf, 1) < 0) { > > DRM_DEBUG_KMS("Sink CRC couldn't be stopped > > properly\n"); > > ret = -EIO; > > goto out; > > @@ -4069,7 +4070,8 @@ static int intel_dp_sink_crc_start(struct > > intel_dp *intel_dp) > > return ret; > > } > > > > - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, > > &buf) < 0) > > + if (intel_dp_dpcd_read_wake(&intel_dp->aux, > > DP_TEST_SINK_MISC, > > + &buf, 1) < 0) > > return -EIO; > > > > if (!(buf & DP_TEST_CRC_SUPPORTED)) > > @@ -4077,7 +4079,7 @@ static int intel_dp_sink_crc_start(struct > > intel_dp *intel_dp) > > > > intel_dp->sink_crc.last_count = buf & DP_TEST_COUNT_MASK; > > > > - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) > > < 0) > > + if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK, > > &buf, 1) < 0) > > return -EIO; > > > > hsw_disable_ips(intel_crtc); > > @@ -4109,8 +4111,8 @@ int intel_dp_sink_crc(struct intel_dp > > *intel_dp, u8 *crc) > > do { > > intel_wait_for_vblank(dev, intel_crtc->pipe); > > > > - if (drm_dp_dpcd_readb(&intel_dp->aux, > > - DP_TEST_SINK_MISC, &buf) < > > 0) { > > + if (intel_dp_dpcd_read_wake(&intel_dp->aux, > > + DP_TEST_SINK_MISC, > > &buf, 1) < 0) { > > ret = -EIO; > > goto stop; > > } > > @@ -4123,7 +4125,8 @@ int intel_dp_sink_crc(struct intel_dp > > *intel_dp, u8 *crc) > > if (count == 0) > > intel_dp->sink_crc.last_count = 0; > > > > - if (drm_dp_dpcd_read(&intel_dp->aux, > > DP_TEST_CRC_R_CR, crc, 6) < 0) { > > + if (intel_dp_dpcd_read_wake(&intel_dp->aux, > > DP_TEST_CRC_R_CR, > > + crc, 6) < 0) { > > ret = -EIO; > > goto stop; > > } > > -- > > 2.1.0 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Wed, Aug 26, 2015 at 6:41 PM, Vivi, Rodrigo <rodrigo.vivi@intel.com> wrote: > On Wed, 2015-08-26 at 11:06 +0200, Daniel Vetter wrote: >> On Thu, Aug 20, 2015 at 04:12:00PM -0700, Rodrigo Vivi wrote: >> > From: Rodrigo Vivi <vivijim@rdvivi-budapest.jf.intel.com> >> > >> > Let's use a native read with retry as suggested per spec to >> > fix Sink CRC on SKL when PSR is enabled. >> > >> > With PSR enabled panel is probably taking more time to wake >> > and dpcd read is faling. >> > >> > Cc: Sonika Jindal <sonika.jindal@intel.com> >> > Signed-off-by: Rodrigo Vivi <vivijim@rdvivi-budapest.jf.intel.com> >> >> Seems like we should just move the trickery we do in our own version >> into >> the dp helpers in the core if this is needed all over the place? > > I've wondered this, but I thought there was a good reason to let this > trick separated. I think in general you can assume that if i915 dp sink handling is special it's because we have more testing on various broken hw out there. >> At least in i915 we use it everywhere and it doesn't seem actively >> harmful >> really ... Maybe the only exception would be the i2c-over-dp_aux >> code. > > Why this would be the exception? Maybe this was the good reason? I'd be fairly easy to keep an internal __drm_dp_aux_read (need it anyway to implement this trick) and use that in i2c. At least that's what I'd do without any evidence that we need to make this wake dance also for i2c transactions. i2c uses a special dp-aux mode on the wire, so makes some sense if it's different. See also the recent work from Ville to tune the i2c dp-aux timeouts and retries, it really seems to be a world of its own a bit. -Daniel
On Thu, 27 Aug 2015, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, Aug 26, 2015 at 6:41 PM, Vivi, Rodrigo <rodrigo.vivi@intel.com> wrote: >> On Wed, 2015-08-26 at 11:06 +0200, Daniel Vetter wrote: >>> On Thu, Aug 20, 2015 at 04:12:00PM -0700, Rodrigo Vivi wrote: >>> > From: Rodrigo Vivi <vivijim@rdvivi-budapest.jf.intel.com> >>> > >>> > Let's use a native read with retry as suggested per spec to >>> > fix Sink CRC on SKL when PSR is enabled. >>> > >>> > With PSR enabled panel is probably taking more time to wake >>> > and dpcd read is faling. >>> > >>> > Cc: Sonika Jindal <sonika.jindal@intel.com> >>> > Signed-off-by: Rodrigo Vivi <vivijim@rdvivi-budapest.jf.intel.com> >>> >>> Seems like we should just move the trickery we do in our own version >>> into >>> the dp helpers in the core if this is needed all over the place? >> >> I've wondered this, but I thought there was a good reason to let this >> trick separated. > > I think in general you can assume that if i915 dp sink handling is > special it's because we have more testing on various broken hw out > there. In truth our inconsistent use of wake vs. non-wake can be mostly attributed to the fact that we're clueless about sink sleep states, and we've just added more wakes here and there to paper over it. BR, Jani. > >>> At least in i915 we use it everywhere and it doesn't seem actively >>> harmful >>> really ... Maybe the only exception would be the i2c-over-dp_aux >>> code. >> >> Why this would be the exception? Maybe this was the good reason? > > I'd be fairly easy to keep an internal __drm_dp_aux_read (need it > anyway to implement this trick) and use that in i2c. At least that's > what I'd do without any evidence that we need to make this wake dance > also for i2c transactions. i2c uses a special dp-aux mode on the wire, > so makes some sense if it's different. See also the recent work from > Ville to tune the i2c dp-aux timeouts and retries, it really seems to > be a world of its own a bit. > -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
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index d32ce48..34f5e33 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4037,7 +4037,8 @@ static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp) u8 buf; int ret = 0; - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) { + if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK, + &buf, 1) < 0) { DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n"); ret = -EIO; goto out; @@ -4069,7 +4070,8 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp) return ret; } - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0) + if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK_MISC, + &buf, 1) < 0) return -EIO; if (!(buf & DP_TEST_CRC_SUPPORTED)) @@ -4077,7 +4079,7 @@ static int intel_dp_sink_crc_start(struct intel_dp *intel_dp) intel_dp->sink_crc.last_count = buf & DP_TEST_COUNT_MASK; - if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) + if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_SINK, &buf, 1) < 0) return -EIO; hsw_disable_ips(intel_crtc); @@ -4109,8 +4111,8 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) do { intel_wait_for_vblank(dev, intel_crtc->pipe); - if (drm_dp_dpcd_readb(&intel_dp->aux, - DP_TEST_SINK_MISC, &buf) < 0) { + if (intel_dp_dpcd_read_wake(&intel_dp->aux, + DP_TEST_SINK_MISC, &buf, 1) < 0) { ret = -EIO; goto stop; } @@ -4123,7 +4125,8 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) if (count == 0) intel_dp->sink_crc.last_count = 0; - if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) { + if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_TEST_CRC_R_CR, + crc, 6) < 0) { ret = -EIO; goto stop; }