[Intel-gfx] [PATCH] drm/i915/hsw: Fix workaround for server AUX channel clock divisor

[Jim Bride]

On Thu, May 21, 2015 at 03:04:40PM +0300, Jani Nikula wrote:
> On Thu, 21 May 2015, Ville Syrjälä <ville.syrjala at linux.intel.com> 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 at gmail.com>
> >> 
> >> -T
> >> 
> >> On 5/19/2015 9:13 AM, jim.bride at linux.intel.com wrote:
> >> > From: Jim Bride <jim.bride at 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 at 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 at lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > -- 
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx