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

[Rodrigo Vivi]

On Fri, Mar 15, 2019 at 01:27:22PM -0700, Vanshidhar Konda wrote:
> 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?

For now I believe the best alternative is to --subject-prefix="[CI]"

But also you need to be sure that the fail rate don't fool us...

I believe there will be better alternatives for using CI for cases like
this soon, so +Martin

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