Date: Fri, 18 Mar 2016 20:05:53 +0200
From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= <ville.syrjala@linux.intel.com>
To: Lyude <cpaul@redhat.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()
Message-ID: <20160318180553.GS4329@intel.com>
References: <1458229245-8634-1-git-send-email-cpaul@redhat.com>
 <1458229245-8634-2-git-send-email-cpaul@redhat.com>
 <20160318141345.GG4329@intel.com>
 <20160318161235.GN4329@intel.com>
 <20160318164140.GO4329@intel.com>
 <20160318180029.GL14170@phenom.ffwll.local>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20160318180029.GL14170@phenom.ffwll.local>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3312
Lines: 81

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.

> 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

-- 
Ville Syrj?l?
Intel OTC