[Intel-gfx] [PATCH] drm/i915/display: Increase timeout for DP Aux channel ctl signal

[Vanshidhar Konda]

On Fri, Mar 15, 2019 at 12:38:41PM -0700, Rodrigo Vivi wrote:
>On Fri, Mar 15, 2019 at 11:39:54AM -0700, Vanshidhar Konda wrote:
>> Extend the timeout for the hardware to signal SEND_BUSY on the DP
>> Aux Channel Controller register.
>>
>> This is needed to address FDO #109982
>> https://bugzilla.freedesktop.org/show_bug.cgi?id=109982
>
>instead of mentioning like this, please use:
>Bugzilla: https://bugzilla.freedesktop.org/show_bug.cgi?id=109982
>
>Also instead of the "needed to address" it would be better
>to add some reasoning explaining that
>"empirically we got some bugs workarounded by increasing the timeout
> from 10ms to 15ms although spec was only requiring 4ms"
>or something like that...
>
Thanks! I'll keep these in mind for next patches.
>>
>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> Cc: Imre Deak <imre.deak at intel.com>
>> Signed-off-by: Vanshidhar Konda <vanshidhar.r.konda at intel.com>
>> ---
>> drivers/gpu/drm/i915/intel_dp.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 47857f96c3b1..fd6de33c5664 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1053,6 +1053,8 @@ intel_dp_check_edp(struct intel_dp *intel_dp)
>> 	}
>> }
>>
>> +#define DP_AUX_CH_CTL_SIGNAL_TIMEOUT_MS 15
>
>This define is spurious since it's in use in a single place.
>
>Also, giving the timeout a name, like this, makes it appear it came from
>the spec. Well, if it came from Spec it should be defined in the proper .h
>files.
>
>Since I don't believe this came from spec I believe we can just remove it
>and go for the timeout directly on the function below.
>
I'll make this change in the next patch.
>> +
>> static u32
>> intel_dp_aux_wait_done(struct intel_dp *intel_dp)
>> {
>> @@ -1063,7 +1065,7 @@ intel_dp_aux_wait_done(struct intel_dp *intel_dp)
>>
>> #define C (((status = I915_READ_NOTRACE(ch_ctl)) & DP_AUX_CH_CTL_SEND_BUSY) == 0)
>> 	done = wait_event_timeout(dev_priv->gmbus_wait_queue, C,
>> -				  msecs_to_jiffies_timeout(10));
>> +				  msecs_to_jiffies_timeout(DP_AUX_CH_CTL_SIGNAL_TIMEOUT_MS));
>
>Is this just a guess that you are trying to check?
>I'm asking because I didn't see any indication that the increase
>really fixed the issue.
>
>So, if you are trying to just validate your approach maybe the try-bot
>could be used?
>
I also would have preferred to use the trybot. The error in Bugzilla 109982 is only reproduced on only shard-iclb2 machine once
in approximately 2 days. So the trybot is not of much help for this.

Is there another way of testing experimental patches for bugs that are
not reproduced locally or on other machines? Or, if the issue is only
happening on one machine, should it be lowered in priority/whitelisted
on the specific CI machine?

>Thanks,
>Rodrigo.
>
>>
>> 	/* just trace the final value */
>> 	trace_i915_reg_rw(false, ch_ctl, status, sizeof(status), true);
>> --
>> 2.20.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx