On Thu, May 21, 2015 at 03:04:40PM +0300, Jani Nikula wrote:
> 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);
This looks good, except I think we might want something more like:
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 0edc305..89ab714 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) {
@@ -905,6 +903,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
goto out;
}
+done:
/* Check for timeout or receive error.
* Timeouts occur when the sink is not connected
*/
This way we actually set ret appropriately in the event of a receive error
or a timeout.
Thoughts?
Jim
>
>
>
> >
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
