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@xxxxxxxxxxxxxxx> > Cc: Imre Deak <imre.deak@xxxxxxxxx> > Signed-off-by: Vanshidhar Konda <vanshidhar.r.konda@xxxxxxxxx> > --- > 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@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
