| Message ID | 1440113007-22615-1-git-send-email-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Qui, 2015-08-20 às 16:23 -0700, Rodrigo Vivi escreveu: > 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. Does this commit actually fix any known problem with Sink CRC? Or is it just a try? It would be nice to have this clarified in the commit message. Anyway, it looks correct, so: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > v2: Fix my email domain on commit message. Thanks Rafael. > > Cc: Rafael Antognolli <rafael.antognolli@intel.com> > Cc: Sonika Jindal <sonika.jindal@intel.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > 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; > }
On Mon, 2015-08-24 at 19:54 +0000, Zanoni, Paulo R wrote: > Em Qui, 2015-08-20 às 16:23 -0700, Rodrigo Vivi escreveu: > > 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. > > Does this commit actually fix any known problem with Sink CRC? Or is > it > just a try? It would be nice to have this clarified in the commit > message. It was just a try but that made sink crc working on my SKL when PSR is enabled. nothing much to add... > > Anyway, it looks correct, so: > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > v2: Fix my email domain on commit message. Thanks Rafael. > > > > Cc: Rafael Antognolli <rafael.antognolli@intel.com> > > Cc: Sonika Jindal <sonika.jindal@intel.com> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > --- > > 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;
On 8/25/2015 2:50 AM, Vivi, Rodrigo wrote: > On Mon, 2015-08-24 at 19:54 +0000, Zanoni, Paulo R wrote: >> Em Qui, 2015-08-20 às 16:23 -0700, Rodrigo Vivi escreveu: >>> 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. >> Does this commit actually fix any known problem with Sink CRC? Or is >> it >> just a try? It would be nice to have this clarified in the commit >> message. > It was just a try but that made sink crc working on my SKL when PSR is > enabled. nothing much to add... SKL has new register AUX_MUTEX which should be used when accessing dpcd on edp. just searched the nightly code and could not find it. it might be the reason for random dpcd failures reported in the other thread. regards, Sivakumar >> Anyway, it looks correct, so: >> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >>> v2: Fix my email domain on commit message. Thanks Rafael. >>> >>> Cc: Rafael Antognolli <rafael.antognolli@intel.com> >>> Cc: Sonika Jindal <sonika.jindal@intel.com> >>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >>> --- >>> 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; > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, 2015-10-22 at 00:01 +0530, Thulasimani, Sivakumar wrote: > > On 8/25/2015 2:50 AM, Vivi, Rodrigo wrote: > > On Mon, 2015-08-24 at 19:54 +0000, Zanoni, Paulo R wrote: > > > Em Qui, 2015-08-20 às 16:23 -0700, Rodrigo Vivi escreveu: > > > > 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. > > > Does this commit actually fix any known problem with Sink CRC? Or > > > is > > > it > > > just a try? It would be nice to have this clarified in the commit > > > message. > > It was just a try but that made sink crc working on my SKL when PSR > > is > > enabled. nothing much to add... > SKL has new register AUX_MUTEX which should be used when accessing > dpcd > on edp. just searched the nightly code and could not find it. it > might > be the reason > for random dpcd failures reported in the other thread. Oh, this is interesting... I didn't know that. That would explain that forbidden value on patch 3/4... > > regards, > Sivakumar > > > Anyway, it looks correct, so: > > > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > > > v2: Fix my email domain on commit message. Thanks Rafael. > > > > > > > > Cc: Rafael Antognolli <rafael.antognolli@intel.com> > > > > Cc: Sonika Jindal <sonika.jindal@intel.com> > > > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > --- > > > > 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; > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Thu, Oct 22, 2015 at 12:01:21AM +0530, Thulasimani, Sivakumar wrote: > > > On 8/25/2015 2:50 AM, Vivi, Rodrigo wrote: > >On Mon, 2015-08-24 at 19:54 +0000, Zanoni, Paulo R wrote: > >>Em Qui, 2015-08-20 às 16:23 -0700, Rodrigo Vivi escreveu: > >>>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. > >>Does this commit actually fix any known problem with Sink CRC? Or is > >>it > >>just a try? It would be nice to have this clarified in the commit > >>message. > >It was just a try but that made sink crc working on my SKL when PSR is > >enabled. nothing much to add... > SKL has new register AUX_MUTEX which should be used when accessing dpcd > on edp. just searched the nightly code and could not find it. it might be > the reason > for random dpcd failures reported in the other thread. We had patches for that back in December 2013 :) The feedback from Art was: The non-software aux users are PSR/SRD and GTC. Better leave out the mutex for now. Hardware is going to try do the arbitration itself. I expect you will then need to increase any software timeout you may have. Do you know if anything has changed since then?
On 10/22/2015 1:44 AM, Damien Lespiau wrote: > On Thu, Oct 22, 2015 at 12:01:21AM +0530, Thulasimani, Sivakumar wrote: >> >> On 8/25/2015 2:50 AM, Vivi, Rodrigo wrote: >>> On Mon, 2015-08-24 at 19:54 +0000, Zanoni, Paulo R wrote: >>>> Em Qui, 2015-08-20 às 16:23 -0700, Rodrigo Vivi escreveu: >>>>> 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. >>>> Does this commit actually fix any known problem with Sink CRC? Or is >>>> it >>>> just a try? It would be nice to have this clarified in the commit >>>> message. >>> It was just a try but that made sink crc working on my SKL when PSR is >>> enabled. nothing much to add... >> SKL has new register AUX_MUTEX which should be used when accessing dpcd >> on edp. just searched the nightly code and could not find it. it might be >> the reason >> for random dpcd failures reported in the other thread. > We had patches for that back in December 2013 :) > > The feedback from Art was: > > The non-software aux users are PSR/SRD and GTC. > Better leave out the mutex for now. Hardware is going to try do the > arbitration itself. I expect you will then need to increase any software > timeout you may have. > > Do you know if anything has changed since then? > Not sure, it is in the bspec sequence to use AUX hence forwarded. Art might be the right person to contact :). it might be due to some minor DPCD access issues we observed in BDW when PSR was enabled. regards, Sivakumar
Ok, so after trying it we saw that we really cannot trust on aux mutex.At least not on all SKL/KBL It worked in a KBL but failed on a SKL that I have here... So without aux mutex option we still need to get sink_crc more reliable and I see only 2 quick ways here: - This read wake - Return -EBUSY to force the drm retries on message size = 0. Daniel, what do you believe? Please let me know witch way and if necessary I rebase the patch and re-send. Thanks, Rodrigo. On Wed, Oct 21, 2015 at 8:27 PM Thulasimani, Sivakumar < sivakumar.thulasimani@intel.com> wrote: > > > On 10/22/2015 1:44 AM, Damien Lespiau wrote: > > On Thu, Oct 22, 2015 at 12:01:21AM +0530, Thulasimani, Sivakumar wrote: > >> > >> On 8/25/2015 2:50 AM, Vivi, Rodrigo wrote: > >>> On Mon, 2015-08-24 at 19:54 +0000, Zanoni, Paulo R wrote: > >>>> Em Qui, 2015-08-20 às 16:23 -0700, Rodrigo Vivi escreveu: > >>>>> 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. > >>>> Does this commit actually fix any known problem with Sink CRC? Or is > >>>> it > >>>> just a try? It would be nice to have this clarified in the commit > >>>> message. > >>> It was just a try but that made sink crc working on my SKL when PSR is > >>> enabled. nothing much to add... > >> SKL has new register AUX_MUTEX which should be used when accessing dpcd > >> on edp. just searched the nightly code and could not find it. it might > be > >> the reason > >> for random dpcd failures reported in the other thread. > > We had patches for that back in December 2013 :) > > > > The feedback from Art was: > > > > The non-software aux users are PSR/SRD and GTC. > > Better leave out the mutex for now. Hardware is going to try do the > > arbitration itself. I expect you will then need to increase any > software > > timeout you may have. > > > > Do you know if anything has changed since then? > > > Not sure, it is in the bspec sequence to use AUX hence forwarded. Art > might be the > right person to contact :). it might be due to some minor DPCD access > issues we > observed in BDW when PSR was enabled. > > regards, > Sivakumar > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On Mon, Nov 16, 2015 at 04:05:42PM +0000, Rodrigo Vivi wrote: > Ok, so after trying it we saw that we really cannot trust on aux mutex.At > least not on all SKL/KBL > It worked in a KBL but failed on a SKL that I have here... > > So without aux mutex option we still need to get sink_crc more reliable and > I see only 2 quick ways here: > - This read wake > - Return -EBUSY to force the drm retries on message size = 0. > > Daniel, what do you believe? It's still a mess. My opinion is still that we should move the hacks from read_wake into a more suitable place: a) either into drm_dp_dpcd_read in drm_dp_helper.c b) or into intel_dp_aux_transfer in intel_dp.c Option a) is the right one if this is a generic sink issue (and it seems to be the case, at least for edp panels). Option b) if it's an issue with our hw. Either way I think intel_dp_dpcd_read_wake should die. On a personal gut level I'd go with option a). Cheers, Daniel > > Please let me know witch way and if necessary I rebase the patch and > re-send. > > Thanks, > Rodrigo. > > > > > On Wed, Oct 21, 2015 at 8:27 PM Thulasimani, Sivakumar < > sivakumar.thulasimani@intel.com> wrote: > > > > > > > On 10/22/2015 1:44 AM, Damien Lespiau wrote: > > > On Thu, Oct 22, 2015 at 12:01:21AM +0530, Thulasimani, Sivakumar wrote: > > >> > > >> On 8/25/2015 2:50 AM, Vivi, Rodrigo wrote: > > >>> On Mon, 2015-08-24 at 19:54 +0000, Zanoni, Paulo R wrote: > > >>>> Em Qui, 2015-08-20 às 16:23 -0700, Rodrigo Vivi escreveu: > > >>>>> 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. > > >>>> Does this commit actually fix any known problem with Sink CRC? Or is > > >>>> it > > >>>> just a try? It would be nice to have this clarified in the commit > > >>>> message. > > >>> It was just a try but that made sink crc working on my SKL when PSR is > > >>> enabled. nothing much to add... > > >> SKL has new register AUX_MUTEX which should be used when accessing dpcd > > >> on edp. just searched the nightly code and could not find it. it might > > be > > >> the reason > > >> for random dpcd failures reported in the other thread. > > > We had patches for that back in December 2013 :) > > > > > > The feedback from Art was: > > > > > > The non-software aux users are PSR/SRD and GTC. > > > Better leave out the mutex for now. Hardware is going to try do the > > > arbitration itself. I expect you will then need to increase any > > software > > > timeout you may have. > > > > > > Do you know if anything has changed since then? > > > > > Not sure, it is in the bspec sequence to use AUX hence forwarded. Art > > might be the > > right person to contact :). it might be due to some minor DPCD access > > issues we > > observed in BDW when PSR was enabled. > > > > regards, > > Sivakumar > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
On Tue, 2015-11-17 at 15:08 +0100, Daniel Vetter wrote: > On Mon, Nov 16, 2015 at 04:05:42PM +0000, Rodrigo Vivi wrote: > > Ok, so after trying it we saw that we really cannot trust on aux > > mutex.At > > least not on all SKL/KBL > > It worked in a KBL but failed on a SKL that I have here... > > > > So without aux mutex option we still need to get sink_crc more > > reliable and > > I see only 2 quick ways here: > > - This read wake > > - Return -EBUSY to force the drm retries on message size = 0. > > > > Daniel, what do you believe? > > It's still a mess. My opinion is still that we should move the hacks > from > read_wake into a more suitable place: > a) either into drm_dp_dpcd_read in drm_dp_helper.c Well, but drm_dp_helper already does many retries already (32 times) but only on EBUSY. My idea is that we should consider that and return EBUSY instead of adding another retry case at drm. > b) or into intel_dp_aux_transfer in intel_dp.c Oh, I thought you had nacked this option. > > Option a) is the right one if this is a generic sink issue (and it > seems > to be the case, at least for edp panels). Option b) if it's an issue > with > our hw. Either way I think intel_dp_dpcd_read_wake should die. Well, Jani and Ville kind of nacked this while we don't understand why this was ever introduced at first place. Although I believe with the proper EBUSY returns in place and 32 times retry I believe we could safely remove this as I tried on that series. > > On a personal gut level I'd go with option a). So, EBUSY is ok or should we create another case on drm level? > > Cheers, Daniel Thanks, Rodrigo. > > > > > Please let me know witch way and if necessary I rebase the patch > > and > > re-send. > > > > Thanks, > > Rodrigo. > > > > > > > > > > On Wed, Oct 21, 2015 at 8:27 PM Thulasimani, Sivakumar < > > sivakumar.thulasimani@intel.com> wrote: > > > > > > > > > > > On 10/22/2015 1:44 AM, Damien Lespiau wrote: > > > > On Thu, Oct 22, 2015 at 12:01:21AM +0530, Thulasimani, > > > > Sivakumar wrote: > > > > > > > > > > On 8/25/2015 2:50 AM, Vivi, Rodrigo wrote: > > > > > > On Mon, 2015-08-24 at 19:54 +0000, Zanoni, Paulo R wrote: > > > > > > > Em Qui, 2015-08-20 às 16:23 -0700, Rodrigo Vivi escreveu: > > > > > > > > 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. > > > > > > > Does this commit actually fix any known problem with Sink > > > > > > > CRC? Or is > > > > > > > it > > > > > > > just a try? It would be nice to have this clarified in > > > > > > > the commit > > > > > > > message. > > > > > > It was just a try but that made sink crc working on my SKL > > > > > > when PSR is > > > > > > enabled. nothing much to add... > > > > > SKL has new register AUX_MUTEX which should be used when > > > > > accessing dpcd > > > > > on edp. just searched the nightly code and could not find it. > > > > > it might > > > be > > > > > the reason > > > > > for random dpcd failures reported in the other thread. > > > > We had patches for that back in December 2013 :) > > > > > > > > The feedback from Art was: > > > > > > > > The non-software aux users are PSR/SRD and GTC. > > > > Better leave out the mutex for now. Hardware is going to > > > > try do the > > > > arbitration itself. I expect you will then need to > > > > increase any > > > software > > > > timeout you may have. > > > > > > > > Do you know if anything has changed since then? > > > > > > > Not sure, it is in the bspec sequence to use AUX hence forwarded. > > > Art > > > might be the > > > right person to contact :). it might be due to some minor DPCD > > > access > > > issues we > > > observed in BDW when PSR was enabled. > > > > > > regards, > > > Sivakumar > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > >
On Wed, Nov 18, 2015 at 06:31:05PM +0000, Vivi, Rodrigo wrote: > On Tue, 2015-11-17 at 15:08 +0100, Daniel Vetter wrote: > > On Mon, Nov 16, 2015 at 04:05:42PM +0000, Rodrigo Vivi wrote: > > > Ok, so after trying it we saw that we really cannot trust on aux > > > mutex.At > > > least not on all SKL/KBL > > > It worked in a KBL but failed on a SKL that I have here... > > > > > > So without aux mutex option we still need to get sink_crc more > > > reliable and > > > I see only 2 quick ways here: > > > - This read wake > > > - Return -EBUSY to force the drm retries on message size = 0. > > > > > > Daniel, what do you believe? > > > > It's still a mess. My opinion is still that we should move the hacks > > from > > read_wake into a more suitable place: > > a) either into drm_dp_dpcd_read in drm_dp_helper.c > > Well, but drm_dp_helper already does many retries already (32 times) > but only on EBUSY. My idea is that we should consider that and return > EBUSY instead of adding another retry case at drm. > > > > b) or into intel_dp_aux_transfer in intel_dp.c > > Oh, I thought you had nacked this option. > > > > > Option a) is the right one if this is a generic sink issue (and it > > seems > > to be the case, at least for edp panels). Option b) if it's an issue > > with > > our hw. Either way I think intel_dp_dpcd_read_wake should die. > > Well, Jani and Ville kind of nacked this while we don't understand why > this was ever introduced at first place. > Although I believe with the proper EBUSY returns in place and 32 times > retry I believe we could safely remove this as I tried on that series. > > > > > On a personal gut level I'd go with option a). > > So, EBUSY is ok or should we create another case on drm level? Well, what I'd really like is to get rid of our read_wake code, since pretty obviously it's a hack we seem to need everywhere. If the EBUSY trick will allow us to do that I'm all for it. -Daniel
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; }
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. v2: Fix my email domain on commit message. Thanks Rafael. Cc: Rafael Antognolli <rafael.antognolli@intel.com> Cc: Sonika Jindal <sonika.jindal@intel.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)