On Thu, 21 May 2015, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote: > 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 other words, diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 7cf3fd43071a..179af62d803d 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -893,10 +893,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp, continue; } if (status & DP_AUX_CH_CTL_DONE) - break; + goto done; } - if (status & DP_AUX_CH_CTL_DONE) - break; } if ((status & DP_AUX_CH_CTL_DONE) == 0) { @@ -921,7 +919,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp, ret = -ETIMEDOUT; goto out; } - +done: /* Unload any bytes sent back from the other side */ recv_bytes = ((status & DP_AUX_CH_CTL_MESSAGE_SIZE_MASK) >> DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT); > > 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@xxxxxxxxx> >> >> -T >> >> On 5/19/2015 9:13 AM, jim.bride@xxxxxxxxxxxxxxx wrote: >> > From: Jim Bride <jim.bride@xxxxxxxxxxxxxxx> >> > >> > 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@xxxxxxxxxxxxxxx> >> > --- >> > 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@xxxxxxxxxxxxxxxxxxxxx >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx