| Message ID | 1432052010-13412-1-git-send-email-jim.bride@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jim, I checked the BSpec as well and there's nothing indicating that these two bits are mutually exclusive. They are both sticky though, and if the loop times out 5 times, both _DONE and _TIMEOUT may very well be set. In that case the current code would just exit and never bother to change clock dividers. So I think your code here is valid. The only thing you may want to do is capture some of the IRC discussion though. In particular, whether or not this is really HSW-specific, how this affects other platforms and and the additional delays incurred by running through the loop again after changing AUX channel clock dividers would be good information to put in there. That's probably more of a bikeshed though. Reviewed-by: Todd Previte <tprevite@gmail.com> -T On 5/19/2015 9:13 AM, jim.bride@linux.intel.com wrote: > From: Jim Bride <jim.bride@linux.intel.com> > > According to the HSW b-spec we need to try clock divisors of 63 > and 72, each 3 or more times, when attempting DP AUX channel > communication on a server chipset. This actually wasn't happening > due to a short-circuit that only checked the DP_AUX_CH_CTL_DONE bit > in status rather than checking that the operation was done and > that DP_AUX_CH_CTL_TIME_OUT_ERROR was not set. > > Signed-off-by: Jim Bride <jim.bride@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 0edc305..c01a3f9 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -895,7 +895,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp, > if (status & DP_AUX_CH_CTL_DONE) > break; > } > - if (status & DP_AUX_CH_CTL_DONE) > + if ((status & DP_AUX_CH_CTL_DONE) && > + !(status & DP_AUX_CH_CTL_TIME_OUT_ERROR)) > break; > } >
On Wed, May 20, 2015 at 04:07:37PM -0700, Todd Previte wrote: > Hi Jim, > > I checked the BSpec as well and there's nothing indicating that these > two bits are mutually exclusive. They are both sticky though, and if the > loop times out 5 times, both _DONE and _TIMEOUT may very well be set. In > that case the current code would just exit and never bother to change > clock dividers. So I think your code here is valid. Shouldn't we we checking receive error as well then? In fact looking at our code after the loop, it's expecting DONE to be set before it'll even report receive/timeout errors to the user as EIO/ETIMEDOUT, otherwise it'll just return EBUSY. > > The only thing you may want to do is capture some of the IRC discussion > though. In particular, whether or not this is really HSW-specific, how > this affects other platforms and and the additional delays incurred by > running through the loop again after changing AUX channel clock dividers > would be good information to put in there. That's probably more of a > bikeshed though. > > Reviewed-by: Todd Previte <tprevite@gmail.com> > > -T > > On 5/19/2015 9:13 AM, jim.bride@linux.intel.com wrote: > > From: Jim Bride <jim.bride@linux.intel.com> > > > > According to the HSW b-spec we need to try clock divisors of 63 > > and 72, each 3 or more times, when attempting DP AUX channel > > communication on a server chipset. This actually wasn't happening > > due to a short-circuit that only checked the DP_AUX_CH_CTL_DONE bit > > in status rather than checking that the operation was done and > > that DP_AUX_CH_CTL_TIME_OUT_ERROR was not set. > > > > Signed-off-by: Jim Bride <jim.bride@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_dp.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 0edc305..c01a3f9 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -895,7 +895,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp, > > if (status & DP_AUX_CH_CTL_DONE) > > break; > > } > > - if (status & DP_AUX_CH_CTL_DONE) > > + if ((status & DP_AUX_CH_CTL_DONE) && > > + !(status & DP_AUX_CH_CTL_TIME_OUT_ERROR)) > > break; > > } > > > > _______________________________________________ > 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 0edc305..c01a3f9 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -895,7 +895,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp, if (status & DP_AUX_CH_CTL_DONE) break; } - if (status & DP_AUX_CH_CTL_DONE) + if ((status & DP_AUX_CH_CTL_DONE) && + !(status & DP_AUX_CH_CTL_TIME_OUT_ERROR)) break; }