Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()

From: Lyude Paul <cpaul@redhat.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>,
	"Daniel Vetter" <daniel.vetter@intel.com>,
	intel-gfx@lists.freedesktop.org, arthur.j.runyan@intel.com,
	"open list" <linux-kernel@vger.kernel.org>,
	dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915: Get rid of intel_dp_dpcd_read_wake()
Date: Mon, 21 Mar 2016 12:37:47 -0400	[thread overview]
Message-ID: <1458578267.3031.4.camel@redhat.com> (raw)
In-Reply-To: <20160318180553.GS4329@intel.com>

On Fri, 2016-03-18 at 20:05 +0200, Ville Syrjälä wrote:
> On Fri, Mar 18, 2016 at 07:00:29PM +0100, Daniel Vetter wrote:
> > 
> > On Fri, Mar 18, 2016 at 06:41:40PM +0200, Ville Syrjälä wrote:
> > > 
> > > On Fri, Mar 18, 2016 at 06:12:35PM +0200, Ville Syrjälä wrote:
> > > > 
> > > > On Fri, Mar 18, 2016 at 04:13:45PM +0200, Ville Syrjälä wrote:
> > > > > 
> > > > > On Thu, Mar 17, 2016 at 11:40:45AM -0400, Lyude wrote:
> > > > > > 
> > > > > > Since we've fixed up drm_dp_dpcd_read() to allow for retries when
> > > > > > things
> > > > > > timeout, there's no use for having this function anymore. Good
> > > > > > riddens.
> > > > > > 
> > > > > > Signed-off-by: Lyude <cpaul@redhat.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_dp.c | 79 ++++++++++++----------------
> > > > > > -------------
> > > > > >  1 file changed, 22 insertions(+), 57 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > index cdc2c15..fb4cbbe5 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > > @@ -3190,47 +3190,14 @@ static void chv_dp_post_pll_disable(struct
> > > > > > intel_encoder *encoder)
> > > > > >  }
> > > > > >  
> > > > > >  /*
> > > > > > - * Native read with retry for link status and receiver capability
> > > > > > reads for
> > > > > > - * cases where the sink may still be asleep.
> > > > > > - *
> > > > > > - * Sinks are *supposed* to come up within 1ms from an off state,
> > > > > > but we're also
> > > > > > - * supposed to retry 3 times per the spec.
> > > > > > - */
> > > > > > -static ssize_t
> > > > > > -intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int
> > > > > > offset,
> > > > > > -			void *buffer, size_t size)
> > > > > > -{
> > > > > > -	ssize_t ret;
> > > > > > -	int i;
> > > > > > -
> > > > > > -	/*
> > > > > > -	 * Sometime we just get the same incorrect byte repeated
> > > > > > -	 * over the entire buffer. Doing just one throw away read
> > > > > > -	 * initially seems to "solve" it.
> > > > > > -	 */
> > > > > > -	drm_dp_dpcd_read(aux, DP_DPCD_REV, buffer, 1);
> > > > > NAK
> > > > > 
> > > > > If people keep intentionally breaking my shit I'm going to become
> > > > > really grumpy soon.
> > > > Oh, and just in case someone wants to come up with a better kludge,
> > > > I just spent a few minutes analyzing the behavior of this crappy
> > > > monitor a.
> > > > 
> > > > What happens is that when the monitor is fully powered up (LED is blue)
> > > > things are fine. After the monitor goes to sleep (LED turns orange)
> > > > the first DPCD read will produce garbage. Further DPCD reads are fine,
> > > > even if I wait a significant amount of time between the reads, as long
> > > > as the monitor didn't do a power on->off cycle in between. So it looks
> > > > like it's always just the first read after power down that gets
> > > > corrupted.
> > > > 
> > > > Now I think I'll go and test how writes behave, assuming I can find a
> > > > decently sized chunk of DPCD address space I can write. And maybe I
> > > > should also try i2c-over-aux...
> > > The first DPCD write after powerdown also got corrupted. But i2c-over-aux
> > > seems unaffected for whatever reason.
> > Do you have an amd card nearby to test there?
> Nope.
I also work on radeon so I have plenty. The issue with that though is that this
is an issue that only really happens with the thinkpad docks, and as far as I'm
aware we don't have any ThinkPads that have AMD hardware in them…
> > 
> > Would be interesting to
> > confirm that this is indeed a sink bug, and hence that it really all
> > should be in the shared code.
> > -Daniel
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch