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

From: Daniel Vetter <daniel@ffwll.ch>
To: "Vivi, Rodrigo" <rodrigo.vivi@intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/i915: Retry on every aux read.
Date: Tue, 20 Oct 2015 20:31:46 +0200	[thread overview]
Message-ID: <20151020183146.GZ13786@phenom.ffwll.local> (raw)
In-Reply-To: <1445363249.24860.54.camel@intel.com>

On Tue, Oct 20, 2015 at 05:45:29PM +0000, Vivi, Rodrigo wrote:
> 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...

I thought we only return -EBUSY when the dp aux hw controlled died, otoh
maybe it should be -EIO in that case. It's a bit unclear, which is why I
think trying to clarify this would be useful.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx