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

[Lyude Paul]

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 
> > > > > > ---
> > > > > >  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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Ville Syrjälä]

On Mon, Mar 21, 2016 at 12:30:54PM +0200, Jani Nikula wrote:
> On Fri, 18 Mar 2016, 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:
> >> > > -  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.
> 
> Did the display go to sleep on its own, or did we do something? In
> particular, does DPCD DP_SET_POWER register play a role? What if we skip
> writing D3 to it? What if we do that write as the first thing (every
> time)?

User pressing any of the buttons on the monitor is enough to wake it,
and after a short timeout it will power down on its own, leading to
the corrupted access.

Keeping DP_SET_POWER at D0 doesn't change anything.

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

On Fri, 18 Mar 2016, 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:
>> > > -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.

Did the display go to sleep on its own, or did we do something? In
particular, does DPCD DP_SET_POWER register play a role? What if we skip
writing D3 to it? What if we do that write as the first thing (every
time)?


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

On Thu, 17 Mar 2016, 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 
> ---
>  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);

Last time around I dug out the commit adding the line above, which fixed
a bug, and said it should be accounted for somehow. But I've had a
change of heart. I want this function gone already. We've improved the
dp helpers considerably, and if this regresses, we need to look at
fixing this either at the drm level or in our ->transfer hook.

Acked-by: Jani Nikula 


> -
> - for (i = 0; i < 3; i++) {
> - ret = drm_dp_dpcd_read(aux, offset, buffer, size);
> - if (ret == size)
> - return ret;
> - msleep(1);
> - }
> -
> - return ret;
> -}
> -
> -/*
>   * Fetch AUX CH registers 0x202 - 0x207 which contain
>   * link status information
>   */
>  bool
>  intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t 
> link_status[DP_LINK_STATUS_SIZE])
>  {
> - return intel_dp_dpcd_read_wake(_dp->aux,
> -DP_LANE0_1_STATUS,
> -link_status,
> -DP_LINK_STATUS_SIZE) == 
> DP_LINK_STATUS_SIZE;
> + return drm_dp_dpcd_read(_dp->aux, DP_LANE0_1_STATUS, link_status,
> + DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
>  }
>  
>  /* These are source-specific values. */
> @@ -3865,8 +3832,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>   struct drm_i915_private *dev_priv = dev->dev_private;
>   uint8_t rev;
>  
> - if (intel_dp_dpcd_read_wake(_dp->aux, 0x000, intel_dp->dpcd,
> - sizeof(intel_dp->dpcd)) < 0)
> + if (drm_dp_dpcd_read(_dp->aux, 0x000, intel_dp->dpcd,
> +  sizeof(intel_dp->dpcd)) < 0)
>   return false; /* aux transfer failed */
>  
>   DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp->dpcd), 
> intel_dp->dpcd);
> @@ -3877,9 +3844,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>   /* Check if the panel supports PSR */
>   memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>   if (is_edp(intel_dp)) {
> - intel_dp_dpcd_read_wake(_dp->aux, DP_PSR_SUPPORT,
> - intel_dp->psr_dpcd,
> - sizeof(intel_dp->psr_dpcd));
> + drm_dp_dpcd_read(_dp->aux, DP_PSR_SUPPORT,
> +  intel_dp->psr_dpcd,
> +  sizeof(intel_dp->psr_dpcd));
>   if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
>   dev_priv->psr.sink_support = true;
>   DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> @@ -3890,9 +3857,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>   uint8_t frame_sync_cap;
>  
>   dev_priv->psr.sink_support = true;
> - intel_dp_dpcd_read_wake(_dp->aux,
> - DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> - _sync_cap, 1);
> + drm_dp_dpcd_read(_dp->aux,
> +  DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> +  _sync_cap, 1);
>   dev_priv->psr.aux_frame_sync = frame_sync_cap ? true : 
> false;
>   /* PSR2 needs frame sync as well */
>   dev_priv->psr.psr2_support = 
> dev_priv->psr.aux_frame_sync;
> @@ -3908,15 +3875,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>   /* Intermediate frequency support */
>   if (is_edp(intel_dp) &&
>   

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

[Ville Syrjälä]

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 
> > ---
> >  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...
 
> 
> > -
> > -   for (i = 0; i < 3; i++) {
> > -   ret = drm_dp_dpcd_read(aux, offset, buffer, size);
> > -   if (ret == size)
> > -   return ret;
> > -   msleep(1);
> > -   }
> > -
> > -   return ret;
> > -}
> > -
> > -/*
> >   * Fetch AUX CH registers 0x202 - 0x207 which contain
> >   * link status information
> >   */
> >  bool
> >  intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t 
> > link_status[DP_LINK_STATUS_SIZE])
> >  {
> > -   return intel_dp_dpcd_read_wake(_dp->aux,
> > -  DP_LANE0_1_STATUS,
> > -  link_status,
> > -  DP_LINK_STATUS_SIZE) == 
> > DP_LINK_STATUS_SIZE;
> > +   return drm_dp_dpcd_read(_dp->aux, DP_LANE0_1_STATUS, link_status,
> > +   DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
> >  }
> >  
> >  /* These are source-specific values. */
> > @@ -3865,8 +3832,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > uint8_t rev;
> >  
> > -   if (intel_dp_dpcd_read_wake(_dp->aux, 0x000, intel_dp->dpcd,
> > -   sizeof(intel_dp->dpcd)) < 0)
> > +   if (drm_dp_dpcd_read(_dp->aux, 0x000, intel_dp->dpcd,
> > +sizeof(intel_dp->dpcd)) < 0)
> > return false; /* aux transfer failed */
> >  
> > DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp->dpcd), 
> > intel_dp->dpcd);
> > @@ -3877,9 +3844,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> > /* Check if the panel supports PSR */
> > memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
> > if (is_edp(intel_dp)) {
> > -   intel_dp_dpcd_read_wake(_dp->aux, DP_PSR_SUPPORT,
> > -   intel_dp->psr_dpcd,
> > -   sizeof(intel_dp->psr_dpcd));
> > +   drm_dp_dpcd_read(_dp->aux, DP_PSR_SUPPORT,
> > +intel_dp->psr_dpcd,
> > +sizeof(intel_dp->psr_dpcd));
> > if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
> > dev_priv->psr.sink_support = true;
> > DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> > @@ -3890,9 +3857,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> > uint8_t frame_sync_cap;
> >  
> > dev_priv->psr.sink_support = true;
> > -   intel_dp_dpcd_read_wake(_dp->aux,
> > -   DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> > -

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

[Ville Syrjälä]

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

>  
> > 
> > > -
> > > - for (i = 0; i < 3; i++) {
> > > - ret = drm_dp_dpcd_read(aux, offset, buffer, size);
> > > - if (ret == size)
> > > - return ret;
> > > - msleep(1);
> > > - }
> > > -
> > > - return ret;
> > > -}
> > > -
> > > -/*
> > >   * Fetch AUX CH registers 0x202 - 0x207 which contain
> > >   * link status information
> > >   */
> > >  bool
> > >  intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t 
> > > link_status[DP_LINK_STATUS_SIZE])
> > >  {
> > > - return intel_dp_dpcd_read_wake(_dp->aux,
> > > -DP_LANE0_1_STATUS,
> > > -link_status,
> > > -DP_LINK_STATUS_SIZE) == 
> > > DP_LINK_STATUS_SIZE;
> > > + return drm_dp_dpcd_read(_dp->aux, DP_LANE0_1_STATUS, link_status,
> > > + DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
> > >  }
> > >  
> > >  /* These are source-specific values. */
> > > @@ -3865,8 +3832,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> > >   struct drm_i915_private *dev_priv = dev->dev_private;
> > >   uint8_t rev;
> > >  
> > > - if (intel_dp_dpcd_read_wake(_dp->aux, 0x000, intel_dp->dpcd,
> > > - sizeof(intel_dp->dpcd)) < 0)
> > > + if (drm_dp_dpcd_read(_dp->aux, 0x000, intel_dp->dpcd,
> > > +  sizeof(intel_dp->dpcd)) < 0)
> > >   return false; /* aux transfer failed */
> > >  
> > >   DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp->dpcd), 
> > > intel_dp->dpcd);
> > > @@ -3877,9 +3844,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> > >   /* Check if the panel supports PSR */
> > >   memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
> > >   if (is_edp(intel_dp)) {
> > > - intel_dp_dpcd_read_wake(_dp->aux, DP_PSR_SUPPORT,
> > > - intel_dp->psr_dpcd,
> > > - sizeof(intel_dp->psr_dpcd));
> > > + drm_dp_dpcd_read(_dp->aux, DP_PSR_SUPPORT,
> > > +  intel_dp->psr_dpcd,
> > > +  sizeof(intel_dp->psr_dpcd));
> > >   if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
> > >   dev_priv->psr.sink_support = true;
> > >   DRM_DEBUG_KMS("Detected EDP PSR 

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

[Lyude]

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 
---
 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);
-
-   for (i = 0; i < 3; i++) {
-   ret = drm_dp_dpcd_read(aux, offset, buffer, size);
-   if (ret == size)
-   return ret;
-   msleep(1);
-   }
-
-   return ret;
-}
-
-/*
  * Fetch AUX CH registers 0x202 - 0x207 which contain
  * link status information
  */
 bool
 intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t 
link_status[DP_LINK_STATUS_SIZE])
 {
-   return intel_dp_dpcd_read_wake(_dp->aux,
-  DP_LANE0_1_STATUS,
-  link_status,
-  DP_LINK_STATUS_SIZE) == 
DP_LINK_STATUS_SIZE;
+   return drm_dp_dpcd_read(_dp->aux, DP_LANE0_1_STATUS, link_status,
+   DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
 }
 
 /* These are source-specific values. */
@@ -3865,8 +3832,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
struct drm_i915_private *dev_priv = dev->dev_private;
uint8_t rev;
 
-   if (intel_dp_dpcd_read_wake(_dp->aux, 0x000, intel_dp->dpcd,
-   sizeof(intel_dp->dpcd)) < 0)
+   if (drm_dp_dpcd_read(_dp->aux, 0x000, intel_dp->dpcd,
+sizeof(intel_dp->dpcd)) < 0)
return false; /* aux transfer failed */
 
DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp->dpcd), 
intel_dp->dpcd);
@@ -3877,9 +3844,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
/* Check if the panel supports PSR */
memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
if (is_edp(intel_dp)) {
-   intel_dp_dpcd_read_wake(_dp->aux, DP_PSR_SUPPORT,
-   intel_dp->psr_dpcd,
-   sizeof(intel_dp->psr_dpcd));
+   drm_dp_dpcd_read(_dp->aux, DP_PSR_SUPPORT,
+intel_dp->psr_dpcd,
+sizeof(intel_dp->psr_dpcd));
if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
dev_priv->psr.sink_support = true;
DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
@@ -3890,9 +3857,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
uint8_t frame_sync_cap;
 
dev_priv->psr.sink_support = true;
-   intel_dp_dpcd_read_wake(_dp->aux,
-   DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
-   _sync_cap, 1);
+   drm_dp_dpcd_read(_dp->aux,
+DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
+_sync_cap, 1);
dev_priv->psr.aux_frame_sync = frame_sync_cap ? true : 
false;
/* PSR2 needs frame sync as well */
dev_priv->psr.psr2_support = 
dev_priv->psr.aux_frame_sync;
@@ -3908,15 +3875,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
/* Intermediate frequency support */
if (is_edp(intel_dp) &&
(intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] & 
DP_DPCD_DISPLAY_CONTROL_CAPABLE) &&
-   (intel_dp_dpcd_read_wake(_dp->aux, DP_EDP_DPCD_REV, , 1) 
== 1) &&
+   (drm_dp_dpcd_read(_dp->aux, DP_EDP_DPCD_REV, , 1) == 1) &&
(rev >= 0x03)) { /* eDp v1.4 or higher */
__le16 sink_rates[DP_MAX_SUPPORTED_RATES];
int i;
 
-   intel_dp_dpcd_read_wake(_dp->aux,
-   DP_SUPPORTED_LINK_RATES,
-   sink_rates,
-   

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

[Daniel Vetter]

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 
> > > > ---
> > > >  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? 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Thu, Mar 17, 2016 at 07:56:33PM +0200, Jani Nikula wrote:
> On Thu, 17 Mar 2016, 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 
> > ---
> >  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);
> 
> Last time around I dug out the commit adding the line above, which fixed
> a bug, and said it should be accounted for somehow. But I've had a
> change of heart. I want this function gone already. We've improved the
> dp helpers considerably, and if this regresses, we need to look at
> fixing this either at the drm level or in our ->transfer hook.
> 
> Acked-by: Jani Nikula 

Hm, my idea was actually to 1:1 move intel_dp_dpcd_read_wake logic into
drm_dp_dpcd_read, without any functional change for i915.ko. And everyone
else shouldn't complain about a few more retries either.

That would also address Ville's grumpy nack.
-Daniel

> 
> 
> > -
> > -   for (i = 0; i < 3; i++) {
> > -   ret = drm_dp_dpcd_read(aux, offset, buffer, size);
> > -   if (ret == size)
> > -   return ret;
> > -   msleep(1);
> > -   }
> > -
> > -   return ret;
> > -}
> > -
> > -/*
> >   * Fetch AUX CH registers 0x202 - 0x207 which contain
> >   * link status information
> >   */
> >  bool
> >  intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t 
> > link_status[DP_LINK_STATUS_SIZE])
> >  {
> > -   return intel_dp_dpcd_read_wake(_dp->aux,
> > -  DP_LANE0_1_STATUS,
> > -  link_status,
> > -  DP_LINK_STATUS_SIZE) == 
> > DP_LINK_STATUS_SIZE;
> > +   return drm_dp_dpcd_read(_dp->aux, DP_LANE0_1_STATUS, link_status,
> > +   DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
> >  }
> >  
> >  /* These are source-specific values. */
> > @@ -3865,8 +3832,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > uint8_t rev;
> >  
> > -   if (intel_dp_dpcd_read_wake(_dp->aux, 0x000, intel_dp->dpcd,
> > -   sizeof(intel_dp->dpcd)) < 0)
> > +   if (drm_dp_dpcd_read(_dp->aux, 0x000, intel_dp->dpcd,
> > +sizeof(intel_dp->dpcd)) < 0)
> > return false; /* aux transfer failed */
> >  
> > DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp->dpcd), 
> > intel_dp->dpcd);
> > @@ -3877,9 +3844,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> > /* Check if the panel supports PSR */
> > memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
> > if (is_edp(intel_dp)) {
> > -   intel_dp_dpcd_read_wake(_dp->aux, DP_PSR_SUPPORT,
> > -   intel_dp->psr_dpcd,
> > -   sizeof(intel_dp->psr_dpcd));
> > +   drm_dp_dpcd_read(_dp->aux, DP_PSR_SUPPORT,
> > +intel_dp->psr_dpcd,
> > +sizeof(intel_dp->psr_dpcd));
> > if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
> > dev_priv->psr.sink_support = true;
> > DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> > @@ -3890,9 +3857,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> > uint8_t frame_sync_cap;
> >  
> > dev_priv->psr.sink_support = true;
> > -   intel_dp_dpcd_read_wake(_dp->aux,
> > -   DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> > -   _sync_cap, 1);
> > +   drm_dp_dpcd_read(_dp->aux,
> > +DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> > +  

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

[Ville Syrjälä]

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


> -
> - for (i = 0; i < 3; i++) {
> - ret = drm_dp_dpcd_read(aux, offset, buffer, size);
> - if (ret == size)
> - return ret;
> - msleep(1);
> - }
> -
> - return ret;
> -}
> -
> -/*
>   * Fetch AUX CH registers 0x202 - 0x207 which contain
>   * link status information
>   */
>  bool
>  intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t 
> link_status[DP_LINK_STATUS_SIZE])
>  {
> - return intel_dp_dpcd_read_wake(_dp->aux,
> -DP_LANE0_1_STATUS,
> -link_status,
> -DP_LINK_STATUS_SIZE) == 
> DP_LINK_STATUS_SIZE;
> + return drm_dp_dpcd_read(_dp->aux, DP_LANE0_1_STATUS, link_status,
> + DP_LINK_STATUS_SIZE) == DP_LINK_STATUS_SIZE;
>  }
>  
>  /* These are source-specific values. */
> @@ -3865,8 +3832,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>   struct drm_i915_private *dev_priv = dev->dev_private;
>   uint8_t rev;
>  
> - if (intel_dp_dpcd_read_wake(_dp->aux, 0x000, intel_dp->dpcd,
> - sizeof(intel_dp->dpcd)) < 0)
> + if (drm_dp_dpcd_read(_dp->aux, 0x000, intel_dp->dpcd,
> +  sizeof(intel_dp->dpcd)) < 0)
>   return false; /* aux transfer failed */
>  
>   DRM_DEBUG_KMS("DPCD: %*ph\n", (int) sizeof(intel_dp->dpcd), 
> intel_dp->dpcd);
> @@ -3877,9 +3844,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>   /* Check if the panel supports PSR */
>   memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>   if (is_edp(intel_dp)) {
> - intel_dp_dpcd_read_wake(_dp->aux, DP_PSR_SUPPORT,
> - intel_dp->psr_dpcd,
> - sizeof(intel_dp->psr_dpcd));
> + drm_dp_dpcd_read(_dp->aux, DP_PSR_SUPPORT,
> +  intel_dp->psr_dpcd,
> +  sizeof(intel_dp->psr_dpcd));
>   if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
>   dev_priv->psr.sink_support = true;
>   DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> @@ -3890,9 +3857,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>   uint8_t frame_sync_cap;
>  
>   dev_priv->psr.sink_support = true;
> - intel_dp_dpcd_read_wake(_dp->aux,
> - DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> - _sync_cap, 1);
> + drm_dp_dpcd_read(_dp->aux,
> +  DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
> +  _sync_cap, 1);
>   dev_priv->psr.aux_frame_sync = frame_sync_cap ? true : 
> false;
>   /* PSR2 needs frame sync as well */
>   dev_priv->psr.psr2_support = 
> dev_priv->psr.aux_frame_sync;
> @@ -3908,15 +3875,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>   /* Intermediate frequency support */
>   if (is_edp(intel_dp) &&
>   (intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] & 
> DP_DPCD_DISPLAY_CONTROL_CAPABLE) &&
> - (intel_dp_dpcd_read_wake(_dp->aux, DP_EDP_DPCD_REV, , 1) 
> == 1) &&
> + (drm_dp_dpcd_read(_dp->aux, DP_EDP_DPCD_REV, , 1) == 1) &&
>   (rev >= 0x03)) { /* eDp v1.4 or higher */
>   

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

[Ville Syrjälä]

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 
> > > > > ---
> > > > >  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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx