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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

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...

> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Signed-off-by: Vanshidhar Konda <vanshidhar.r.konda@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.

> +
> 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?

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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx