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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 21 May 2015, Ville Syrjälä <ville.syrjala@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);



>
> 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
>
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx