* [PATCH] drm/i915: preserve other DP_TEST_SINK bits. @ 2014-09-29 22:29 Rodrigo Vivi 2014-09-29 22:39 ` Todd Previte 2014-09-30 20:36 ` Daniel Vetter 0 siblings, 2 replies; 6+ messages in thread From: Rodrigo Vivi @ 2014-09-29 22:29 UTC (permalink / raw) To: intel-gfx; +Cc: Rodrigo Vivi Sink crc was implemented based on dp 1.1 spec that had all TEST_SINK bits reserved reading all 0s. But when reviewing my latest changes on sink crc Todd warned me that on new specs we have other valid bits on this reg that we might want to preserve. Cc: Todd Previte <tprevite@gmail.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index b8699b0..7d5fa2f 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3821,8 +3821,9 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) if (!(buf & DP_TEST_CRC_SUPPORTED)) return -ENOTTY; + drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf); if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, - DP_TEST_SINK_START) < 0) + buf | DP_TEST_SINK_START) < 0) return -EIO; drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf); @@ -3841,7 +3842,10 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) return -EIO; - drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, 0); + drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf); + drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, + buf & ~DP_TEST_SINK_START); + return 0; } -- 1.9.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: preserve other DP_TEST_SINK bits. 2014-09-29 22:29 [PATCH] drm/i915: preserve other DP_TEST_SINK bits Rodrigo Vivi @ 2014-09-29 22:39 ` Todd Previte 2014-09-30 7:42 ` Daniel Vetter 2014-09-30 20:36 ` Daniel Vetter 1 sibling, 1 reply; 6+ messages in thread From: Todd Previte @ 2014-09-29 22:39 UTC (permalink / raw) To: 'Rodrigo Vivi', intel-gfx Hi Rodrigo, Looks good. Only thing that needs to be removed is that extra blank line between the last part of the function and the return statement. Otherwise... Reviewed-by: Todd Previte <tprevite@gmail.com> -----Original Message----- From: Rodrigo Vivi [mailto:rodrigo.vivi@intel.com] Sent: Monday, September 29, 2014 3:30 PM To: intel-gfx@lists.freedesktop.org Cc: Rodrigo Vivi; Todd Previte Subject: [PATCH] drm/i915: preserve other DP_TEST_SINK bits. Sink crc was implemented based on dp 1.1 spec that had all TEST_SINK bits reserved reading all 0s. But when reviewing my latest changes on sink crc Todd warned me that on new specs we have other valid bits on this reg that we might want to preserve. Cc: Todd Previte <tprevite@gmail.com> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> --- drivers/gpu/drm/i915/intel_dp.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index b8699b0..7d5fa2f 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3821,8 +3821,9 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) if (!(buf & DP_TEST_CRC_SUPPORTED)) return -ENOTTY; + drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf); if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, - DP_TEST_SINK_START) < 0) + buf | DP_TEST_SINK_START) < 0) return -EIO; drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf); @@ -3841,7 +3842,10 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) return -EIO; - drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, 0); + drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf); + drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, + buf & ~DP_TEST_SINK_START); + return 0; } -- 1.9.3 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: preserve other DP_TEST_SINK bits. 2014-09-29 22:39 ` Todd Previte @ 2014-09-30 7:42 ` Daniel Vetter 0 siblings, 0 replies; 6+ messages in thread From: Daniel Vetter @ 2014-09-30 7:42 UTC (permalink / raw) To: Todd Previte; +Cc: intel-gfx, 'Rodrigo Vivi' On Mon, Sep 29, 2014 at 03:39:06PM -0700, Todd Previte wrote: > Hi Rodrigo, > > Looks good. Only thing that needs to be removed is that extra blank line > between the last part of the function and the return statement. Otherwise... Looking around in our driver the return statement at the end of the function is often on a separate line. > > Reviewed-by: Todd Previte <tprevite@gmail.com> Queued for -next, thanks for the patch. > > -----Original Message----- > From: Rodrigo Vivi [mailto:rodrigo.vivi@intel.com] > Sent: Monday, September 29, 2014 3:30 PM > To: intel-gfx@lists.freedesktop.org > Cc: Rodrigo Vivi; Todd Previte > Subject: [PATCH] drm/i915: preserve other DP_TEST_SINK bits. > > Sink crc was implemented based on dp 1.1 spec that had all TEST_SINK bits > reserved reading all 0s. But when reviewing my latest changes on sink crc > Todd warned me that on new specs we have other valid bits on this reg that > we might want to preserve. Why was this not mentioned in the review to that patch? I've double-checked Todd's reply and I've only found a "lgtm, r-b". If you spot something odd, or something that should be addressed in a follow-up patch, that should be part of your review reply - review is much more than rubber-stamping with r-b tags. And if that review happened offline or in Portland, pls always try to summarize the main points discussed in the patch or reply for the benefit of everyone else. We're a globally distributed team, so active and inclusive communication is absolutely key. Thanks, Daniel > > Cc: Todd Previte <tprevite@gmail.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > b/drivers/gpu/drm/i915/intel_dp.c index b8699b0..7d5fa2f 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3821,8 +3821,9 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 > *crc) > if (!(buf & DP_TEST_CRC_SUPPORTED)) > return -ENOTTY; > > + drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf); > if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, > - DP_TEST_SINK_START) < 0) > + buf | DP_TEST_SINK_START) < 0) > return -EIO; > > drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf); @@ > -3841,7 +3842,10 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 > *crc) > if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) > return -EIO; > > - drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, 0); > + drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf); > + drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, > + buf & ~DP_TEST_SINK_START); > + > return 0; > } > > -- > 1.9.3 > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: preserve other DP_TEST_SINK bits. 2014-09-29 22:29 [PATCH] drm/i915: preserve other DP_TEST_SINK bits Rodrigo Vivi 2014-09-29 22:39 ` Todd Previte @ 2014-09-30 20:36 ` Daniel Vetter 2014-09-30 20:41 ` Rodrigo Vivi 1 sibling, 1 reply; 6+ messages in thread From: Daniel Vetter @ 2014-09-30 20:36 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: intel-gfx On Mon, Sep 29, 2014 at 06:29:52PM -0400, Rodrigo Vivi wrote: > Sink crc was implemented based on dp 1.1 spec that had all TEST_SINK bits > reserved reading all 0s. But when reviewing my latest changes on sink crc > Todd warned me that on new specs we have other valid bits on this reg that we > might want to preserve. > > Cc: Todd Previte <tprevite@gmail.com> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index b8699b0..7d5fa2f 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3821,8 +3821,9 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) > if (!(buf & DP_TEST_CRC_SUPPORTED)) > return -ENOTTY; > > + drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf); > if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, With this we no longer check the first aux transaction for errors, and maybe we should even check them all? -Daniel > - DP_TEST_SINK_START) < 0) > + buf | DP_TEST_SINK_START) < 0) > return -EIO; > > drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf); > @@ -3841,7 +3842,10 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) > if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) > return -EIO; > > - drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, 0); > + drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf); > + drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, > + buf & ~DP_TEST_SINK_START); > + > return 0; > } > > -- > 1.9.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: preserve other DP_TEST_SINK bits. 2014-09-30 20:36 ` Daniel Vetter @ 2014-09-30 20:41 ` Rodrigo Vivi 2014-10-01 8:06 ` Daniel Vetter 0 siblings, 1 reply; 6+ messages in thread From: Rodrigo Vivi @ 2014-09-30 20:41 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx, Rodrigo Vivi Please don't forget the drm/i915: Fix Sink CRC That is the important one that fix crc calculation and will allow me to remove the extra wait for vblanks on tests besides making tests more stable. On Tue, Sep 30, 2014 at 1:36 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Sep 29, 2014 at 06:29:52PM -0400, Rodrigo Vivi wrote: >> Sink crc was implemented based on dp 1.1 spec that had all TEST_SINK bits >> reserved reading all 0s. But when reviewing my latest changes on sink crc >> Todd warned me that on new specs we have other valid bits on this reg that we >> might want to preserve. >> >> Cc: Todd Previte <tprevite@gmail.com> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> >> --- >> drivers/gpu/drm/i915/intel_dp.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c >> index b8699b0..7d5fa2f 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -3821,8 +3821,9 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) >> if (!(buf & DP_TEST_CRC_SUPPORTED)) >> return -ENOTTY; >> >> + drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf); >> if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, > > With this we no longer check the first aux transaction for errors, and > maybe we should even check them all? That is true. Do you prefer a v2 or top fix? And what do you prefer? avoid the check at all or check on the first aux transaction? > -Daniel > >> - DP_TEST_SINK_START) < 0) >> + buf | DP_TEST_SINK_START) < 0) >> return -EIO; >> >> drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf); >> @@ -3841,7 +3842,10 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) >> if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) >> return -EIO; >> >> - drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, 0); >> + drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf); >> + drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, >> + buf & ~DP_TEST_SINK_START); >> + >> return 0; >> } >> >> -- >> 1.9.3 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > 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 -- Rodrigo Vivi Blog: http://blog.vivi.eng.br ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/i915: preserve other DP_TEST_SINK bits. 2014-09-30 20:41 ` Rodrigo Vivi @ 2014-10-01 8:06 ` Daniel Vetter 0 siblings, 0 replies; 6+ messages in thread From: Daniel Vetter @ 2014-10-01 8:06 UTC (permalink / raw) To: Rodrigo Vivi; +Cc: intel-gfx, Rodrigo Vivi On Tue, Sep 30, 2014 at 01:41:20PM -0700, Rodrigo Vivi wrote: > Please don't forget the drm/i915: Fix Sink CRC > > That is the important one that fix crc calculation and will allow me > to remove the extra wait for vblanks on tests besides making tests > more stable. > > > On Tue, Sep 30, 2014 at 1:36 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > > On Mon, Sep 29, 2014 at 06:29:52PM -0400, Rodrigo Vivi wrote: > >> Sink crc was implemented based on dp 1.1 spec that had all TEST_SINK bits > >> reserved reading all 0s. But when reviewing my latest changes on sink crc > >> Todd warned me that on new specs we have other valid bits on this reg that we > >> might want to preserve. > >> > >> Cc: Todd Previte <tprevite@gmail.com> > >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_dp.c | 8 ++++++-- > >> 1 file changed, 6 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > >> index b8699b0..7d5fa2f 100644 > >> --- a/drivers/gpu/drm/i915/intel_dp.c > >> +++ b/drivers/gpu/drm/i915/intel_dp.c > >> @@ -3821,8 +3821,9 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) > >> if (!(buf & DP_TEST_CRC_SUPPORTED)) > >> return -ENOTTY; > >> > >> + drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf); > >> if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, > > > > With this we no longer check the first aux transaction for errors, and > > maybe we should even check them all? > > That is true. Do you prefer a v2 or top fix? > And what do you prefer? avoid the check at all or check on the first > aux transaction? Fixup patch is ok, I've kept your patch here. And I guess if you bother you should add checks with return -EIO to all of them, since they could all fail. Even if it's just for consistency. -Daniel > > > > -Daniel > > > >> - DP_TEST_SINK_START) < 0) > >> + buf | DP_TEST_SINK_START) < 0) > >> return -EIO; > >> > >> drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf); > >> @@ -3841,7 +3842,10 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc) > >> if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) > >> return -EIO; > >> > >> - drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, 0); > >> + drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf); > >> + drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK, > >> + buf & ~DP_TEST_SINK_START); > >> + > >> return 0; > >> } > >> > >> -- > >> 1.9.3 > >> > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@lists.freedesktop.org > >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > 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 > > > > -- > Rodrigo Vivi > Blog: http://blog.vivi.eng.br -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-10-01 8:06 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-09-29 22:29 [PATCH] drm/i915: preserve other DP_TEST_SINK bits Rodrigo Vivi 2014-09-29 22:39 ` Todd Previte 2014-09-30 7:42 ` Daniel Vetter 2014-09-30 20:36 ` Daniel Vetter 2014-09-30 20:41 ` Rodrigo Vivi 2014-10-01 8:06 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox