Re: [PATCH] drm/i915: Retry on every aux read.

Vivi, Rodrigo
2015-10-20T17:45:32Z

On Tue, 2015-10-20 at 08:38 -0700, Rodrigo Vivi wrote:
> On Tue, 2015-10-20 at 09:39 +0200, Daniel Vetter wrote:
> > On Tue, Oct 20, 2015 at 10:02:21AM +0300, Jani Nikula wrote:
> > > On Tue, 20 Oct 2015, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> > > > We have an inconsistency on our code on using
> > > > intel_dp_dpcd_read_wake with
> > > > retries and when using drm_dp_dpcd_read helper without retry.
> > >
> > > We're supposed to do the retries when the sink may be in a power
> > > down
> > > state. We're not very good at tracking that, and we've cargo
> > > culted
> > > the
> > > retry variants all over the place without thinking. This is why
> > > we're
> > > inconsistent.
> > >
> > > > Since the retries help in many cases let's be consistent and be
> > > > on
> > > > the safe side retrying on every aux read, including i2c ones.
> > >
> > > Please let's not add superfluous retries at different levels of
> > > the
> > > stack just to be safe. It's like a variant of the C programmer's
> > > disease.
>
> I'm not the one who is adding, but just being consistent on our side.
>
> > >
> > > If the retries really help [citation needed] we need to figure
> > > out
> > > what
> > > we're doing wrong and where, and preferrably fix this at the DP
> > > helper
> > > level for all drivers, if possible.
>
> Yes, I'm sorry, I forgot to copy the citation from the first patch in
> this thread that mentions this fix sink crc for Skylake... and also
> for
> some Broadwells what made me to remind about this patch....
>
> >
> > Yeah, same comment as I've done last time around. If we need this,
> > we
> > need
> > to do this in core dp helpers.
>
> If it is "superfluous" and "disease" to make the consistence in our
> driver I can't imagine the words that I'm going to hear if I try to
> touch other drivers...
>
> To see if we should actually do this in our level or on the drm level
> I
> was trying to understand where does:
> " Sinks are *supposed* to come up within 1ms from an off state, but
> we're also- * supposed to retry 3 times per the spec."
>
> But I couldn't find anything in Vesa spec that tells that neither on
> BSPec... at BSpec for some platforms I found the 3 times retry for
> aux
> ctl programming what we do already...

Besides, I noticed on drm level we have 32 times retry already but when
aux return -EBUSY...

So I wonder if it would be acceptable to retry for all errors not only
for -EBUSY or if we should find out when to return EBUSY propperly on
our aux transfer...

>
> So, yeah, I'm not convinced that we should through this to another
> level... So it is either this or the first one that fix the sink crc
> case individually and let sink crc reliable...
>
> > -Daniel