Re: [Intel-gfx] [alsa-devel] USB Type-C monitor flashes once when play a video file after unplug and re-plug the monitor

[Nathan Ciobanu]

On Mon, Jan 06, 2020 at 08:08:04AM +, lucien_...@compal.com wrote:
> Hi Takashi
> 
> We verified on Ubuntu 19.10 with kernel 5.4.0.0-050400-generic (please refer 
> to attachment), the result is positive which symptom doesn't happen anymore 
> once I played music or video sound output through Dell S2718D Type-C monitor. 
> It seems had some fix in latest kernel.

Takashi, can you point to the patch series you suspect may have fixed this 
issue? 

Thanks,
Nathan
> 
> Thanks.
> 
> 
> -Original Message-
> From: Takashi Iwai  
> Sent: Friday, January 3, 2020 5:16 PM
> To: Cheng. AJ (TPE) 
> Cc: intel-gfx@lists.freedesktop.org; alsa-de...@alsa-project.org; 
> nathan.d.ciob...@linux.intel.com; Wang. CindyXT (TPE) 
> ; Ye. Nelson (TPE) ; Yap. 
> Shane (TPE) ; Kao. Lucien (TPE) 
> ; Tseng. Evan (TPE) 
> Subject: Re: [alsa-devel] USB Type-C monitor flashes once when play a video 
> file after unplug and re-plug the monitor
> 
> On Fri, 03 Jan 2020 02:57:03 +0100,
>  wrote:
> > 
> > Hi Sirs,
> > Here is chromebook SW team from Compal.
> > As the mail title, we hit issue that the external monitor will flash once 
> > when play video after hot pluging.
> > We can reproduce not only on chromebook but also ubuntu 16.04.
> > There has higher failure rate with Dell Solomon dock and Dell S2718D 
> > monitor.
> > 
> > We found adding the delay in "sound/pci/hda/patch_hdmi.c " can fix 
> > this issue.(as the attachment) May need your help to review and advice. 
> > Thanks.
> > 
> > Here is the issue number in gitlab for more detail.
> > https://gitlab.freedesktop.org/drm/intel/issues/318
> 
> Could you check whether it still happens with the latest upstream kernel, at 
> least 5.4.y, if it wasn't tested yet?
> 
> I don't want to put a long delay just because of random reason unless it's 
> really mandatory.  I'm wondering whether the recent write-sync change 
> improves the situation, so let's check the recent code.
> 
> 
> thanks,
> 
> Takashi
> 
> > 
> > 
> > 
> > AJ Cheng
> > NID/NID1
> > e-mail: aj_ch...@compal.com
> > Tel:  +886-2-8797-8599 ext. 17561
> > Mobile : +886-932827829
> > COMPAL Electronics, Inc.
> > 
> > [2 flash_once.diff ]
> > 
> > ___
> > Alsa-devel mailing list
> > alsa-de...@alsa-project.org
> > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v4 2/2] drm/i915: Enable hotplug retry

[Nathan Ciobanu]

On Wed, Jul 10, 2019 at 03:15:00PM -0700, José Roberto de Souza wrote:
> Right now we are aware of two cases that needs another hotplug retry:
> - Unpowered type-c dongles
> - HDMI slow unplug
> 
> Both have a complete explanation in the code to schedule another run
> of the hotplug handler.
> 
> It could have more checks to just trigger the retry in those two
> specific cases but why would sink signal a long pulse if there is
> no change? Also the drawback of running the hotplug handler again
> is really low and that could fix another cases that we are not
> aware.
> 
> Also retrying for old DP ports(non-DDI) to make it consistent and not
> cause CI failures if those systems are connected to chamelium boards
> that will be used to simulate the issues reported in here.
> 
> v2: Also retrying for old DP ports(non-DDI)(Imre)
> 
> v4: Renamed INTEL_HOTPLUG_NOCHANGE to INTEL_HOTPLUG_UNCHANGED to keep
> it consistent(Rodrigo)
> 
> Cc: Ville Syrjälä 
> Cc: Imre Deak 
> Cc: Jani Nikula 
> Reviewed-by: Imre Deak 
> Signed-off-by: José Roberto de Souza 
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c  | 21 +
>  drivers/gpu/drm/i915/display/intel_dp.c   |  7 ++
>  drivers/gpu/drm/i915/display/intel_hdmi.c | 28 ++-
>  3 files changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c 
> b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 734c004800f8..ea6d1873f6cb 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -4052,6 +4052,7 @@ intel_ddi_hotplug(struct intel_encoder *encoder,
> struct intel_connector *connector,
> bool irq_received)
>  {
> + struct intel_digital_port *dig_port = enc_to_dig_port(>base);
>   struct drm_modeset_acquire_ctx ctx;
>   enum intel_hotplug_state state;
>   int ret;
> @@ -4078,6 +4079,26 @@ intel_ddi_hotplug(struct intel_encoder *encoder,
>   drm_modeset_acquire_fini();
>   WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
>  
> + /*
> +  * Unpowered type-c dongles can take some time to boot and be
> +  * responsible, so here giving some time to those dongles to power up
> +  * and then retrying the probe.
> +  *
> +  * On many platforms the HDMI live state signal is known to be
> +  * unreliable, so we can't use it to detect if a sink is connected or
> +  * not. Instead we detect if it's connected based on whether we can
> +  * read the EDID or not. That in turn has a problem during disconnect,
> +  * since the HPD interrupt may be raised before the DDC lines get
> +  * disconnected (due to how the required length of DDC vs. HPD
> +  * connector pins are specified) and so we'll still be able to get a
> +  * valid EDID. To solve this schedule another detection cycle if this
> +  * time around we didn't detect any change in the sink's connection
> +  * status.
> +  */
> + if (state == INTEL_HOTPLUG_UNCHANGED && irq_received &&
> + !dig_port->dp.is_mst)
> + state = INTEL_HOTPLUG_RETRY;
> +
>   return state;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 4423abbc7907..7106a2d80f79 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4881,6 +4881,13 @@ intel_dp_hotplug(struct intel_encoder *encoder,
>   drm_modeset_acquire_fini();
>   WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
>  
> + /*
> +  * Keeping it consistent with intel_ddi_hotplug() and
> +  * intel_hdmi_hotplug().
> +  */
> + if (state == INTEL_HOTPLUG_UNCHANGED && irq_received)
> + state = INTEL_HOTPLUG_RETRY;
> +
>   return state;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c 
> b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index 0ebec69bbbfc..26c8556f6980 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -3143,6 +3143,32 @@ void intel_hdmi_init_connector(struct 
> intel_digital_port *intel_dig_port,
>   DRM_DEBUG_KMS("CEC notifier get failed\n");
>  }
>  
> +static enum intel_hotplug_state
> +intel_hdmi_hotplug(struct intel_encoder *encoder,
> +struct intel_connector *connector, bool irq_received)
> +{
> + enum intel_hotplug_state state;
> +
> + state = intel_encoder_hotplug(encoder, connector, irq_received);
> +
> + /*
> +  * On many platforms the HDMI live state signal is known to be
> +  * unreliable, so we can't use it to detect if a sink is connected or
> +  * not. Instead we detect if it's connected based on whether we can
> +  * read the EDID or not. That in turn has a problem during disconnect,
> +  * since the HPD interrupt may be raised before the DDC lines get
> +  * disconnected (due to how the 

Re: [Intel-gfx] [PATCH v4 1/2] drm/i915: Add support for retrying hotplug

[Nathan Ciobanu]

On Wed, Jul 10, 2019 at 03:14:59PM -0700, José Roberto de Souza wrote:
> From: Imre Deak 
> 
> There is some scenarios that we are aware that sink probe can fail,
> so lets add the infrastructure to let hotplug() hook to request
> another probe after some time.
> 
> v2: Handle shared HPD pins (Imre)
> v3: Rebased
> v4: Renamed INTEL_HOTPLUG_NOCHANGE to INTEL_HOTPLUG_UNCHANGED to keep
> it consistent(Rodrigo)
> 
> Cc: Ville Syrjälä 
> Reviewed-by: Rodrigo Vivi 
> Signed-off-by: José Roberto de Souza 
> Signed-off-by: Jani Nikula 
> Signed-off-by: Imre Deak 
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 12 ++--
>  drivers/gpu/drm/i915/display/intel_dp.c  | 12 ++--
>  drivers/gpu/drm/i915/display/intel_hotplug.c | 59 +++-
>  drivers/gpu/drm/i915/display/intel_hotplug.h |  5 +-
>  drivers/gpu/drm/i915/display/intel_sdvo.c|  8 ++-
>  drivers/gpu/drm/i915/i915_debugfs.c  |  2 +-
>  drivers/gpu/drm/i915/i915_drv.h  |  3 +-
>  drivers/gpu/drm/i915/intel_drv.h | 11 +++-
>  8 files changed, 80 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c 
> b/drivers/gpu/drm/i915/display/intel_ddi.c
> index ad638e7f27bb..734c004800f8 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -4047,14 +4047,16 @@ static int intel_hdmi_reset_link(struct intel_encoder 
> *encoder,
>   return modeset_pipe(>base, ctx);
>  }
>  
> -static bool intel_ddi_hotplug(struct intel_encoder *encoder,
> -   struct intel_connector *connector)
> +static enum intel_hotplug_state
> +intel_ddi_hotplug(struct intel_encoder *encoder,
> +   struct intel_connector *connector,
> +   bool irq_received)
>  {
>   struct drm_modeset_acquire_ctx ctx;
> - bool changed;
> + enum intel_hotplug_state state;
>   int ret;
>  
> - changed = intel_encoder_hotplug(encoder, connector);
> + state = intel_encoder_hotplug(encoder, connector, irq_received);
>  
>   drm_modeset_acquire_init(, 0);
>  
> @@ -4076,7 +4078,7 @@ static bool intel_ddi_hotplug(struct intel_encoder 
> *encoder,
>   drm_modeset_acquire_fini();
>   WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
>  
> - return changed;
> + return state;
>  }
>  
>  static struct intel_connector *
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
> b/drivers/gpu/drm/i915/display/intel_dp.c
> index 0bdb7ecc5a81..4423abbc7907 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -4853,14 +4853,16 @@ int intel_dp_retrain_link(struct intel_encoder 
> *encoder,
>   * retrain the link to get a picture. That's in case no
>   * userspace component reacted to intermittent HPD dip.
>   */
> -static bool intel_dp_hotplug(struct intel_encoder *encoder,
> -  struct intel_connector *connector)
> +static enum intel_hotplug_state
> +intel_dp_hotplug(struct intel_encoder *encoder,
> +  struct intel_connector *connector,
> +  bool irq_received)
>  {
>   struct drm_modeset_acquire_ctx ctx;
> - bool changed;
> + enum intel_hotplug_state state;
>   int ret;
>  
> - changed = intel_encoder_hotplug(encoder, connector);
> + state = intel_encoder_hotplug(encoder, connector, irq_received);
>  
>   drm_modeset_acquire_init(, 0);
>  
> @@ -4879,7 +4881,7 @@ static bool intel_dp_hotplug(struct intel_encoder 
> *encoder,
>   drm_modeset_acquire_fini();
>   WARN(ret, "Acquiring modeset locks failed with %i\n", ret);
>  
> - return changed;
> + return state;
>  }
>  
>  static void intel_dp_check_service_irq(struct intel_dp *intel_dp)
> diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c 
> b/drivers/gpu/drm/i915/display/intel_hotplug.c
> index ea3de4acc850..2ca92780c659 100644
> --- a/drivers/gpu/drm/i915/display/intel_hotplug.c
> +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c
> @@ -112,6 +112,7 @@ enum hpd_pin intel_hpd_pin_default(struct 
> drm_i915_private *dev_priv,
>  
>  #define HPD_STORM_DETECT_PERIOD  1000
>  #define HPD_STORM_REENABLE_DELAY (2 * 60 * 1000)
> +#define HPD_RETRY_DELAY  1000
>  
>  /**
>   * intel_hpd_irq_storm_detect - gather stats and detect HPD IRQ storm on a 
> pin
> @@ -266,8 +267,10 @@ static void intel_hpd_irq_storm_reenable_work(struct 
> work_struct *work)
>   intel_runtime_pm_put(_priv->runtime_pm, wakeref);
>  }
>  
> -bool intel_encoder_hotplug(struct intel_encoder *encoder,
> -struct intel_connector *connector)
> +enum intel_hotplug_state
> +intel_encoder_hotplug(struct intel_encoder *encoder,
> +   struct intel_connector *connector,
> +   bool irq_received)
>  {
>   struct drm_device *dev = connector->base.dev;
>   enum drm_connector_status old_status;
> @@ -279,7 +282,7 @@ bool 

Re: [Intel-gfx] [PATCH 3/3] drm/i915/psr: Enable PSR1 on gen-9+ HW

[Nathan Ciobanu]

On Thu, Sep 06, 2018 at 11:21:35PM -0700, Dhinakaran Pandiyan wrote:
> We have new tests and fixes in place since the feature was last
> disabled.
> 
> Try again for gen-9+ hardware and enable only PSR1 as a first step.
Since this patch explicitly disables PSR2 for all platforms maybe you 
can clarify that in the commit message or disable PSR2 in a separate
patch. It will help with productized kernels ;)
> 
> Cc: Jani Nikula 
> Cc: Jose Roberto de Souza 
> Cc: Paulo Zanoni 
> Cc: Rodrigo Vivi 
> Cc: Ville Syrjälä 
> References: 2ee7dc497e34 ("drm/i915: disable PSR by default on HSW/BDW")
> References: dcb2e993f3c0 ("Revert "drm/i915: Enable PSR by default on 
> Valleyview and Cherryview."")
> Signed-off-by: Dhinakaran Pandiyan 
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 17 -
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> b/drivers/gpu/drm/i915/intel_psr.c
> index b6838b525502..fc823f93a4dc 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -71,6 +71,10 @@ static bool psr_global_enabled(u32 debug)
>  static bool intel_psr2_enabled(struct drm_i915_private *dev_priv,
>  const struct intel_crtc_state *crtc_state)
>  {
> + /* Disable PSR2 by default for all platforms */
> + if (i915_modparams.enable_psr == -1)
> + return false;
> +
>   switch (dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK) {
>   case I915_PSR_DEBUG_FORCE_PSR1:
>   return false;
> @@ -1051,7 +1055,7 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
>   * intel_psr_init - Init basic PSR work and mutex.
>   * @dev_priv: i915 device private
>   *
> - * This function is  called only once at driver load to initialize basic
> + * This function is called only once at driver load to initialize basic
>   * PSR stuff.
>   */
>  void intel_psr_init(struct drm_i915_private *dev_priv)
> @@ -1065,19 +1069,14 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
>   if (!dev_priv->psr.sink_support)
>   return;
>  
> - if (i915_modparams.enable_psr == -1) {
> - i915_modparams.enable_psr = dev_priv->vbt.psr.enable;
> -
> - /* Per platform default: all disabled. */
> - i915_modparams.enable_psr = 0;
> - }
> + if (i915_modparams.enable_psr == -1)
> + if (INTEL_GEN(dev_priv) < 9 || !dev_priv->vbt.psr.enable)
> + i915_modparams.enable_psr = 0;
>  
> - /* Set link_standby x link_off defaults */
>   if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>   /* HSW and BDW require workarounds that we don't implement. */
>   dev_priv->psr.link_standby = false;
>   else
> - /* For new platforms let's respect VBT back again */
>   dev_priv->psr.link_standby = dev_priv->vbt.psr.full_link;
>  
>   INIT_WORK(_priv->psr.work, intel_psr_work);
> -- 
> 2.17.1
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915/dp: Improve clock recovery loop limit comment

[Nathan Ciobanu]

Clarifies the clock recovery loop limit comment that 80
max_cr_tries for pre-DP1.4 devices was chosen as a very
tolerant upper bound.
Assumptions made:
- DP1.4 syncs should be smarter so they won't need more
than 10 tries
- pre-DP1.4 syncs should be compliant enough to not need
that many tries (80) but we should tolerate any that may
trigger this corner case

Cc: Dhinakaran Pandiyan 
Cc: Rodrigo Vivi 
Cc: Marc Herbert 
Suggested-by: Marc Herbert 
Signed-off-by: Nathan Ciobanu 

diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c 
b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 07e128c7443c..a9f40985a621 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -172,10 +172,12 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
}
 
/*
-* DP 1.4 spec clock recovery retries defined but
-* for devices pre-DP 1.4 we set the retry limit
-* to 4 (voltage levels) x 4 (preemphasis levels) x
-* x 5 (same voltage retries) = 80 (max iterations)
+* The DP 1.4 spec defines the max clock recovery retries value
+* as 10 but for pre-DP 1.4 devices we set a very tolerant
+* retry limit of 80 (4 voltage levels x 4 preemphasis levels x
+* x 5 identical voltage retries). Since the previous specs didn't
+* define a limit and created the possibility of an infinite loop
+* we want to prevent any sync from triggering that corner case.
 */
if (intel_dp->dpcd[DP_DPCD_REV] >= DP_DPCD_REV_14)
max_cr_tries = 10;
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v6] drm/i915/dp: Limit link training clock recovery loop

[Nathan Ciobanu]

On Fri, Jul 20, 2018 at 01:28:44PM -0700, Rodrigo Vivi wrote:
> On Fri, Jul 20, 2018 at 01:15:59PM -0700, Nathan Ciobanu wrote:
> > Limit the link training clock recovery loop to 10 attempts at
> > LANEx_CR_DONE per DP 1.4 spec section 3.5.1.2.2 and 80 attempts for
> > pre-DP 1.4 (4 voltage levels x 4 preemphasis levels x
> > x 5 identical voltages tries). Some faulty USB-C MST hubs can
> > cause us to get stuck in this loop indefinitely requesting something
> > like:
> > 
> > voltage swing: 0, pre-emphasis level: 2
> > voltage swing: 1, pre-emphasis level: 2
> > voltage swing: 0, pre-emphasis level: 3
> > 
> > over and over so max_vswing would never be reached,
> > drm_dp_clock_recovery_ok() would never return true and voltage_tries
> > would always get reset to 1. The driver sends those values to the hub
> > but the hub keeps requesting new values every time.
> > 
> > Changes in v2:
> > - updated commit message (DK, Manasi)
> > - defined DP_DP14_MAX_CR_TRIES (Marc)
> > - made the loop iterate for max 10 times (Rodrigo, Marc)
> > 
> > Changes in v3:
> > - changed error message to use DP_DP14_MAX_CR_TRIES
> > 
> > Changes in v4:
> > - Updated the title to reflect the change
> > - Updated the commit message
> > - Added 80 attempts for pre-DP 1.4 devices
> > 
> > Changes in v5:
> > - Removed DP_DP14_MAX_CR_TRIES from drm
> > 
> > v6: Updated comment to match kernel style (Rodrigo)
> 
> thanks... you could've added my rv-b directly as well ;)
Why should it be simple when I can make it complicated, right? :D. 
Hopefully less noob mistakes next time.
> 
> > 
> > Cc: Dhinakaran Pandiyan 
> > Cc: Rodrigo Vivi 
> > Cc: Marc Herbert 
> > Cc: Manasi Navare 
> > Signed-off-by: Nathan Ciobanu 
> 
> anyway
> 
> Reviewed-by: Rodrigo Vivi 
> 
> > ---
> >  drivers/gpu/drm/i915/intel_dp_link_training.c | 17 +++--
> >  1 file changed, 15 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c 
> > b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > index 4da6e33c7fa1..299cad5632ed 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > @@ -129,7 +129,7 @@ static bool intel_dp_link_max_vswing_reached(struct 
> > intel_dp *intel_dp)
> >  intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> >  {
> > uint8_t voltage;
> > -   int voltage_tries, max_vswing_tries;
> > +   int voltage_tries, max_vswing_tries, cr_tries, max_cr_tries;
> > uint8_t link_config[2];
> > uint8_t link_bw, rate_select;
> >  
> > @@ -170,9 +170,20 @@ static bool intel_dp_link_max_vswing_reached(struct 
> > intel_dp *intel_dp)
> > return false;
> > }
> >  
> > +   /*
> > +* DP 1.4 spec clock recovery retries defined but
> > +* for devices pre-DP 1.4 we set the retry limit
> > +* to 4 (voltage levels) x 4 (preemphasis levels) x
> > +* x 5 (same voltage retries) = 80 (max iterations)
> > +*/
> > +   if (intel_dp->dpcd[DP_DPCD_REV] >= DP_DPCD_REV_14)
> > +   max_cr_tries = 10;
> > +   else
> > +   max_cr_tries = 80;
> > +
> > voltage_tries = 1;
> > max_vswing_tries = 0;
> > -   for (;;) {
> > +   for (cr_tries = 0; cr_tries < max_cr_tries; ++cr_tries) {
> > uint8_t link_status[DP_LINK_STATUS_SIZE];
> >  
> > drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
> > @@ -216,6 +227,8 @@ static bool intel_dp_link_max_vswing_reached(struct 
> > intel_dp *intel_dp)
> > ++max_vswing_tries;
> >  
> > }
> > +   DRM_ERROR("Failed clock recovery %d times, giving up!\n", max_cr_tries);
> > +   return false;
> >  }
> >  
> >  /*
> > -- 
> > 1.9.1
> > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 1/2] drm/i915/dp: Limit link training clock recovery loop

[Nathan Ciobanu]

On Fri, Jul 20, 2018 at 12:22:15PM -0700, Rodrigo Vivi wrote:
> On Thu, Jul 19, 2018 at 02:47:38PM -0700, Nathan Ciobanu wrote:
> > Limit the link training clock recovery loop to 10 attempts at
> > LANEx_CR_DONE per DP 1.4 spec section 3.5.1.2.2 and 80 attempts for
> > pre-DP 1.4 (4 voltage levels x 4 preemphasis levels x
> > x 5 identical voltages tries). Some faulty USB-C MST hubs can
> > cause us to get stuck in this loop indefinitely requesting something
> > like:
> > 
> > voltage swing: 0, pre-emphasis level: 2
> > voltage swing: 1, pre-emphasis level: 2
> > voltage swing: 0, pre-emphasis level: 3
> > 
> > over and over so max_vswing would never be reached,
> > drm_dp_clock_recovery_ok() would never return true and voltage_tries
> > would always get reset to 1. The driver sends those values to the hub
> > but the hub keeps requesting new values every time.
> > 
> > Changes in v2:
> > - updated commit message (DK, Manasi)
> > - defined DP_DP14_MAX_CR_TRIES (Marc)
> > - made the loop iterate for max 10 times (Rodrigo, Marc)
> > 
> > Changes in v3:
> > - changed error message to use DP_DP14_MAX_CR_TRIES
> > 
> > Changes in v4:
> > - Updated the title to reflect the change
> > - Updated the commit message
> > - Added 80 attempts for pre-DP 1.4 devices
> > 
> > Changes in v5:
> > - Removed DP_DP14_MAX_CR_TRIES from drm
> > 
> > Cc: Dhinakaran Pandiyan 
> > Cc: Rodrigo Vivi 
> > Cc: Marc Herbert 
> > Cc: Manasi Navare 
> > Signed-off-by: Nathan Ciobanu 
> > ---
> >  drivers/gpu/drm/i915/intel_dp_link_training.c | 16 ++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c 
> > b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > index 4da6e33c7fa1..7903de7a54c9 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > @@ -129,7 +129,7 @@ static bool intel_dp_link_max_vswing_reached(struct 
> > intel_dp *intel_dp)
> >  intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> >  {
> > uint8_t voltage;
> > -   int voltage_tries, max_vswing_tries;
> > +   int voltage_tries, max_vswing_tries, cr_tries, max_cr_tries;
> > uint8_t link_config[2];
> > uint8_t link_bw, rate_select;
> >  
> > @@ -170,9 +170,19 @@ static bool intel_dp_link_max_vswing_reached(struct 
> > intel_dp *intel_dp)
> > return false;
> > }
> >  
> > +   /* DP 1.4 spec clock recovery retries defined but
> 
> nip: Preferred kernel multi-line comment is:
> 
> /*
>  * Start comment..
>  * second line..
>  */
> 
> but yeah, I understand we have many precedent cases using the other style...
> 
> Reviewed-by: Rodrigo Vivi 
> (better with comment updated ;))
Absolutely. I made the change and sent the patch again. 
Thanks, Nathan.
> 
> > +* for devices pre-DP 1.4 we set the retry limit
> > +* to 4 (voltage levels) x 4 (preemphasis levels) x
> > +* x 5 (same voltage retries) = 80 (max iterations)
> > +*/
> > +   if (intel_dp->dpcd[DP_DPCD_REV] >= DP_DPCD_REV_14)
> > +   max_cr_tries = 10;
> > +   else
> > +   max_cr_tries = 80;
> > +
> > voltage_tries = 1;
> > max_vswing_tries = 0;
> > -   for (;;) {
> > +   for (cr_tries = 0; cr_tries < max_cr_tries; ++cr_tries) {
> > uint8_t link_status[DP_LINK_STATUS_SIZE];
> >  
> > drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
> > @@ -216,6 +226,8 @@ static bool intel_dp_link_max_vswing_reached(struct 
> > intel_dp *intel_dp)
> > ++max_vswing_tries;
> >  
> > }
> > +   DRM_ERROR("Failed clock recovery %d times, giving up!\n", max_cr_tries);
> > +   return false;
> >  }
> >  
> >  /*
> > -- 
> > 1.9.1
> > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v6] drm/i915/dp: Limit link training clock recovery loop

[Nathan Ciobanu]

Limit the link training clock recovery loop to 10 attempts at
LANEx_CR_DONE per DP 1.4 spec section 3.5.1.2.2 and 80 attempts for
pre-DP 1.4 (4 voltage levels x 4 preemphasis levels x
x 5 identical voltages tries). Some faulty USB-C MST hubs can
cause us to get stuck in this loop indefinitely requesting something
like:

voltage swing: 0, pre-emphasis level: 2
voltage swing: 1, pre-emphasis level: 2
voltage swing: 0, pre-emphasis level: 3

over and over so max_vswing would never be reached,
drm_dp_clock_recovery_ok() would never return true and voltage_tries
would always get reset to 1. The driver sends those values to the hub
but the hub keeps requesting new values every time.

Changes in v2:
- updated commit message (DK, Manasi)
- defined DP_DP14_MAX_CR_TRIES (Marc)
- made the loop iterate for max 10 times (Rodrigo, Marc)

Changes in v3:
- changed error message to use DP_DP14_MAX_CR_TRIES

Changes in v4:
- Updated the title to reflect the change
- Updated the commit message
- Added 80 attempts for pre-DP 1.4 devices

Changes in v5:
- Removed DP_DP14_MAX_CR_TRIES from drm

v6: Updated comment to match kernel style (Rodrigo)

Cc: Dhinakaran Pandiyan 
Cc: Rodrigo Vivi 
Cc: Marc Herbert 
Cc: Manasi Navare 
Signed-off-by: Nathan Ciobanu 
---
 drivers/gpu/drm/i915/intel_dp_link_training.c | 17 +++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c 
b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 4da6e33c7fa1..299cad5632ed 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -129,7 +129,7 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
 intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 {
uint8_t voltage;
-   int voltage_tries, max_vswing_tries;
+   int voltage_tries, max_vswing_tries, cr_tries, max_cr_tries;
uint8_t link_config[2];
uint8_t link_bw, rate_select;
 
@@ -170,9 +170,20 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
return false;
}
 
+   /*
+* DP 1.4 spec clock recovery retries defined but
+* for devices pre-DP 1.4 we set the retry limit
+* to 4 (voltage levels) x 4 (preemphasis levels) x
+* x 5 (same voltage retries) = 80 (max iterations)
+*/
+   if (intel_dp->dpcd[DP_DPCD_REV] >= DP_DPCD_REV_14)
+   max_cr_tries = 10;
+   else
+   max_cr_tries = 80;
+
voltage_tries = 1;
max_vswing_tries = 0;
-   for (;;) {
+   for (cr_tries = 0; cr_tries < max_cr_tries; ++cr_tries) {
uint8_t link_status[DP_LINK_STATUS_SIZE];
 
drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
@@ -216,6 +227,8 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
++max_vswing_tries;
 
}
+   DRM_ERROR("Failed clock recovery %d times, giving up!\n", max_cr_tries);
+   return false;
 }
 
 /*
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 2/2] drm/i915/dp: Refactor mav_vswing_tries variable

[Nathan Ciobanu]

On Fri, Jul 20, 2018 at 01:19:02PM +0300, Ville Syrjälä wrote:
> On Thu, Jul 19, 2018 at 02:48:07PM -0700, Nathan Ciobanu wrote:
> > Changes the type and renames the max_vswing_tries variable
> > which was declared as an integer but used as a boolean
> > making it easy to be confused with a counter.
> > 
> > Changes in v2:
> > - updated the title and commit message
> > - left the loop exit point in place
> > 
> > Cc: Dhinakaran Pandiyan 
> > Cc: Rodrigo Vivi 
> > Cc: Marc Herbert 
> > Signed-off-by: Nathan Ciobanu 
> > ---
> >  drivers/gpu/drm/i915/intel_dp_link_training.c | 8 
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c 
> > b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > index 7903de7a54c9..ec5f2bb55c9a 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > @@ -129,7 +129,8 @@ static bool intel_dp_link_max_vswing_reached(struct 
> > intel_dp *intel_dp)
> >  intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> >  {
> > uint8_t voltage;
> > -   int voltage_tries, max_vswing_tries, cr_tries, max_cr_tries;
> > +   int voltage_tries, cr_tries, max_cr_tries;
> > +   bool max_vswing = false;
> 
> max_vswing_reached?
Ok, I renamed it and resent the patch. Thanks,
Nathan
> 
> > uint8_t link_config[2];
> > uint8_t link_bw, rate_select;
> >  
> > @@ -181,7 +182,6 @@ static bool intel_dp_link_max_vswing_reached(struct 
> > intel_dp *intel_dp)
> > max_cr_tries = 80;
> >  
> > voltage_tries = 1;
> > -   max_vswing_tries = 0;
> > for (cr_tries = 0; cr_tries < max_cr_tries; ++cr_tries) {
> > uint8_t link_status[DP_LINK_STATUS_SIZE];
> >  
> > @@ -202,7 +202,7 @@ static bool intel_dp_link_max_vswing_reached(struct 
> > intel_dp *intel_dp)
> > return false;
> > }
> >  
> > -   if (max_vswing_tries == 1) {
> > +   if (max_vswing) {
> > DRM_DEBUG_KMS("Max Voltage Swing reached\n");
> > return false;
> > }
> > @@ -223,7 +223,7 @@ static bool intel_dp_link_max_vswing_reached(struct 
> > intel_dp *intel_dp)
> > voltage_tries = 1;
> >  
> > if (intel_dp_link_max_vswing_reached(intel_dp))
> > -   ++max_vswing_tries;
> > +   max_vswing = true;
> >  
> > }
> > DRM_ERROR("Failed clock recovery %d times, giving up!\n", max_cr_tries);
> > -- 
> > 1.9.1
> > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v5] drm/i915/dp: Refactor max_vswing_tries variable

[Nathan Ciobanu]

Changes the type and renames the max_vswing_tries variable
which was declared as an integer but used as a boolean
making it easy to be confused with a counter.

Changes in v2:
- updated the title and commit message
- left the loop exit point in place

v3: fix typo in title
v4: renamed max_vswing to max_vswing_reached (Ville)

Cc: Dhinakaran Pandiyan 
Cc: Rodrigo Vivi 
Cc: Marc Herbert 
Signed-off-by: Nathan Ciobanu 
---
 drivers/gpu/drm/i915/intel_dp_link_training.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c 
b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 7903de7a54c9..0d418f5a59b0 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -129,7 +129,8 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
 intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 {
uint8_t voltage;
-   int voltage_tries, max_vswing_tries, cr_tries, max_cr_tries;
+   int voltage_tries, cr_tries, max_cr_tries;
+   bool max_vswing_reached = false;
uint8_t link_config[2];
uint8_t link_bw, rate_select;
 
@@ -181,7 +182,6 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
max_cr_tries = 80;
 
voltage_tries = 1;
-   max_vswing_tries = 0;
for (cr_tries = 0; cr_tries < max_cr_tries; ++cr_tries) {
uint8_t link_status[DP_LINK_STATUS_SIZE];
 
@@ -202,7 +202,7 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
return false;
}
 
-   if (max_vswing_tries == 1) {
+   if (max_vswing_reached) {
DRM_DEBUG_KMS("Max Voltage Swing reached\n");
return false;
}
@@ -223,7 +223,7 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
voltage_tries = 1;
 
if (intel_dp_link_max_vswing_reached(intel_dp))
-   ++max_vswing_tries;
+   max_vswing_reached = true;
 
}
DRM_ERROR("Failed clock recovery %d times, giving up!\n", max_cr_tries);
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Remove unused "ret" variable.

[Nathan Ciobanu]

On Thu, Jul 19, 2018 at 05:16:03PM -0700, Nathan Ciobanu wrote:
> On Thu, Jul 19, 2018 at 04:42:17PM -0700, Rodrigo Vivi wrote:
> > Just a small clean-up with no functional change, only
> > removing a variable that is never actually used.
> > 
> > Cc: Dhinakaran Pandiyan 
> > Signed-off-by: Rodrigo Vivi 
> Nice one :)
> Reviewed-by: 
Reviewed-by: Nathan Ciobanu 
> > ---
> >  drivers/gpu/drm/i915/intel_dp_mst.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c 
> > b/drivers/gpu/drm/i915/intel_dp_mst.c
> > index 7e3e01607643..18c65f8e4fe8 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> > @@ -263,7 +263,6 @@ static void intel_mst_enable_dp(struct intel_encoder 
> > *encoder,
> > struct intel_dp *intel_dp = _dig_port->dp;
> > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > enum port port = intel_dig_port->base.port;
> > -   int ret;
> >  
> > DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
> >  
> > @@ -274,9 +273,9 @@ static void intel_mst_enable_dp(struct intel_encoder 
> > *encoder,
> > 1))
> > DRM_ERROR("Timed out waiting for ACT sent\n");
> >  
> > -   ret = drm_dp_check_act_status(_dp->mst_mgr);
> > +   drm_dp_check_act_status(_dp->mst_mgr);
> >  
> > -   ret = drm_dp_update_payload_part2(_dp->mst_mgr);
> > +   drm_dp_update_payload_part2(_dp->mst_mgr);
> > if (pipe_config->has_audio)
> > intel_audio_codec_enable(encoder, pipe_config, conn_state);
> >  }
> > -- 
> > 2.17.1
> > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Remove unused "ret" variable.

[Nathan Ciobanu]

On Thu, Jul 19, 2018 at 04:42:17PM -0700, Rodrigo Vivi wrote:
> Just a small clean-up with no functional change, only
> removing a variable that is never actually used.
> 
> Cc: Dhinakaran Pandiyan 
> Signed-off-by: Rodrigo Vivi 
Nice one :)
Reviewed-by: 
> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c 
> b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 7e3e01607643..18c65f8e4fe8 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -263,7 +263,6 @@ static void intel_mst_enable_dp(struct intel_encoder 
> *encoder,
>   struct intel_dp *intel_dp = _dig_port->dp;
>   struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>   enum port port = intel_dig_port->base.port;
> - int ret;
>  
>   DRM_DEBUG_KMS("active links %d\n", intel_dp->active_mst_links);
>  
> @@ -274,9 +273,9 @@ static void intel_mst_enable_dp(struct intel_encoder 
> *encoder,
>   1))
>   DRM_ERROR("Timed out waiting for ACT sent\n");
>  
> - ret = drm_dp_check_act_status(_dp->mst_mgr);
> + drm_dp_check_act_status(_dp->mst_mgr);
>  
> - ret = drm_dp_update_payload_part2(_dp->mst_mgr);
> + drm_dp_update_payload_part2(_dp->mst_mgr);
>   if (pipe_config->has_audio)
>   intel_audio_codec_enable(encoder, pipe_config, conn_state);
>  }
> -- 
> 2.17.1
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v5] drm/i915/dp: Refactor max_vswing_tries variable

[Nathan Ciobanu]

Changes the type and renames the max_vswing_tries variable
which was declared as an integer but used as a boolean
making it easy to be confused with a counter.

Changes in v2:
- updated the title and commit message
- left the loop exit point in place

v3: fix typo in title

Cc: Dhinakaran Pandiyan 
Cc: Rodrigo Vivi 
Cc: Marc Herbert 
Signed-off-by: Nathan Ciobanu 
---
 drivers/gpu/drm/i915/intel_dp_link_training.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c 
b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 7903de7a54c9..ec5f2bb55c9a 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -129,7 +129,8 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
 intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 {
uint8_t voltage;
-   int voltage_tries, max_vswing_tries, cr_tries, max_cr_tries;
+   int voltage_tries, cr_tries, max_cr_tries;
+   bool max_vswing = false;
uint8_t link_config[2];
uint8_t link_bw, rate_select;
 
@@ -181,7 +182,6 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
max_cr_tries = 80;
 
voltage_tries = 1;
-   max_vswing_tries = 0;
for (cr_tries = 0; cr_tries < max_cr_tries; ++cr_tries) {
uint8_t link_status[DP_LINK_STATUS_SIZE];
 
@@ -202,7 +202,7 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
return false;
}
 
-   if (max_vswing_tries == 1) {
+   if (max_vswing) {
DRM_DEBUG_KMS("Max Voltage Swing reached\n");
return false;
}
@@ -223,7 +223,7 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
voltage_tries = 1;
 
if (intel_dp_link_max_vswing_reached(intel_dp))
-   ++max_vswing_tries;
+   max_vswing = true;
 
}
DRM_ERROR("Failed clock recovery %d times, giving up!\n", max_cr_tries);
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v5 2/2] drm/i915/dp: Refactor mav_vswing_tries variable

[Nathan Ciobanu]

Changes the type and renames the max_vswing_tries variable
which was declared as an integer but used as a boolean
making it easy to be confused with a counter.

Changes in v2:
- updated the title and commit message
- left the loop exit point in place

Cc: Dhinakaran Pandiyan 
Cc: Rodrigo Vivi 
Cc: Marc Herbert 
Signed-off-by: Nathan Ciobanu 
---
 drivers/gpu/drm/i915/intel_dp_link_training.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c 
b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 7903de7a54c9..ec5f2bb55c9a 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -129,7 +129,8 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
 intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 {
uint8_t voltage;
-   int voltage_tries, max_vswing_tries, cr_tries, max_cr_tries;
+   int voltage_tries, cr_tries, max_cr_tries;
+   bool max_vswing = false;
uint8_t link_config[2];
uint8_t link_bw, rate_select;
 
@@ -181,7 +182,6 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
max_cr_tries = 80;
 
voltage_tries = 1;
-   max_vswing_tries = 0;
for (cr_tries = 0; cr_tries < max_cr_tries; ++cr_tries) {
uint8_t link_status[DP_LINK_STATUS_SIZE];
 
@@ -202,7 +202,7 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
return false;
}
 
-   if (max_vswing_tries == 1) {
+   if (max_vswing) {
DRM_DEBUG_KMS("Max Voltage Swing reached\n");
return false;
}
@@ -223,7 +223,7 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
voltage_tries = 1;
 
if (intel_dp_link_max_vswing_reached(intel_dp))
-   ++max_vswing_tries;
+   max_vswing = true;
 
}
DRM_ERROR("Failed clock recovery %d times, giving up!\n", max_cr_tries);
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v5 1/2] drm/i915/dp: Limit link training clock recovery loop

[Nathan Ciobanu]

Limit the link training clock recovery loop to 10 attempts at
LANEx_CR_DONE per DP 1.4 spec section 3.5.1.2.2 and 80 attempts for
pre-DP 1.4 (4 voltage levels x 4 preemphasis levels x
x 5 identical voltages tries). Some faulty USB-C MST hubs can
cause us to get stuck in this loop indefinitely requesting something
like:

voltage swing: 0, pre-emphasis level: 2
voltage swing: 1, pre-emphasis level: 2
voltage swing: 0, pre-emphasis level: 3

over and over so max_vswing would never be reached,
drm_dp_clock_recovery_ok() would never return true and voltage_tries
would always get reset to 1. The driver sends those values to the hub
but the hub keeps requesting new values every time.

Changes in v2:
- updated commit message (DK, Manasi)
- defined DP_DP14_MAX_CR_TRIES (Marc)
- made the loop iterate for max 10 times (Rodrigo, Marc)

Changes in v3:
- changed error message to use DP_DP14_MAX_CR_TRIES

Changes in v4:
- Updated the title to reflect the change
- Updated the commit message
- Added 80 attempts for pre-DP 1.4 devices

Changes in v5:
- Removed DP_DP14_MAX_CR_TRIES from drm

Cc: Dhinakaran Pandiyan 
Cc: Rodrigo Vivi 
Cc: Marc Herbert 
Cc: Manasi Navare 
Signed-off-by: Nathan Ciobanu 
---
 drivers/gpu/drm/i915/intel_dp_link_training.c | 16 ++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c 
b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 4da6e33c7fa1..7903de7a54c9 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -129,7 +129,7 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
 intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 {
uint8_t voltage;
-   int voltage_tries, max_vswing_tries;
+   int voltage_tries, max_vswing_tries, cr_tries, max_cr_tries;
uint8_t link_config[2];
uint8_t link_bw, rate_select;
 
@@ -170,9 +170,19 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
return false;
}
 
+   /* DP 1.4 spec clock recovery retries defined but
+* for devices pre-DP 1.4 we set the retry limit
+* to 4 (voltage levels) x 4 (preemphasis levels) x
+* x 5 (same voltage retries) = 80 (max iterations)
+*/
+   if (intel_dp->dpcd[DP_DPCD_REV] >= DP_DPCD_REV_14)
+   max_cr_tries = 10;
+   else
+   max_cr_tries = 80;
+
voltage_tries = 1;
max_vswing_tries = 0;
-   for (;;) {
+   for (cr_tries = 0; cr_tries < max_cr_tries; ++cr_tries) {
uint8_t link_status[DP_LINK_STATUS_SIZE];
 
drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
@@ -216,6 +226,8 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
++max_vswing_tries;
 
}
+   DRM_ERROR("Failed clock recovery %d times, giving up!\n", max_cr_tries);
+   return false;
 }
 
 /*
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3] drm/i915/dp: Give up link training clock recovery after 10 failed attempts

[Nathan Ciobanu]

On Thu, Jul 19, 2018 at 10:01:41AM -0700, Rodrigo Vivi wrote:
> On Tue, Jul 17, 2018 at 06:05:51PM -0700, Nathan Ciobanu wrote:
> > On Tue, Jul 17, 2018 at 03:21:17PM -0700, Dhinakaran Pandiyan wrote:
> > > On Mon, 2018-07-16 at 16:51 -0700, Marc Herbert wrote:
> > > > > 
> > > > > > 
> > > > > > I think the bug is with this infinite loop which is at the mercy
> > > > > > of an external device
> > > > > > and in my case I have this MST hub which appears to be DP 1.2
> > > > > > that triggers the
> > > > > > infinite loop case. We have to limit the number of iterations and
> > > > > > DP 1.4 spec happened to fix this issue. I'm a newbie in this area
> > > > > > but in this case
> > > > > > shouldn't we apply the same counter to all <= "DP 1.4" devices?
> > > > > ok, the infinite loop situation is really bad... but I don't
> > > > > believe
> > > > > we are going with the right fix...
> > > > > and a good indication is that your fix is for DP-1.4 while your bug
> > > > > is seeing on DP-1.2
> > > > I thought the only reason the infinite loop fix isn't in 1.2 is just
> > > > because there's
> > > > no 1.2.1 spec... (plus the naive assumption that buggy sinks don't
> > > > exist)
> > > > 
> > > Irrespective of whether the sink is DP1.2 or 1.4, if there are sinks
> > > out there that keep toggling between two values there should be an
> > > overall limit to how many times this loop gets executed. Even if this
> > > isn't right fix for the issue at hand, the loop has to break.
> > > 
> > > Then there's the question of what the limit should be. We could use the
> > > DP1.4 limit as a reference and apply it widely. But there's a problem
> > > here, we have 4 vswing values and 4 pre-emphasis values. If the sink
> > > progressively changes one variable at a time, we'll need at least 16
> > > iterations. Nathan's patch here will prematurely error out and that
> > > doesn't sound reasonable to impose on non DP1.4 sinks.
> > 
> > I think it would be a max of 13 iterations since one of the vswing
> > values will be max_vswing and the loop will terminate at that point
> > without trying the other 3 preemph values for that voltage, correct?
> 
> I was talking to DK yesterday and he suggested that we should go with
> a huge number for DP_1.2 and with the spec for DP_1.4. According to DK
> 80 was covering all combinations few times.
> 
> So you get your patch and create some max_cr_tries = dp_1.4 ? spec : 80;
> 
> or something like that.
> 
> I think I like that because infinite loop situation here is so bad
> and we should avoid even if it was something else that got really wrong.
I just sent v4 to do that. Thanks!
-Nathan
> 
> Thanks,
> Rodrigo.
> 
> > 
> > -Nathan
> > > 
> > > -DK
> > > 
> > > 
> > > > 
> > > > 
> > > > ___
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v4 2/2] drm/i915/dp: Remove unneeded mav_vswing_tries variable

[Nathan Ciobanu]

max_vswing_tries variable was declared as an int but
used as a bool and not even needed because we can
just check the return of intel_dp_link_max_vswing_reached.

Cc: Dhinakaran Pandiyan 
Cc: Rodrigo Vivi 
Cc: Marc Herbert 
Signed-off-by: Nathan Ciobanu 
---
 drivers/gpu/drm/i915/intel_dp_link_training.c | 14 +-
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c 
b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 05f209f6d73f..ddfdd5d05168 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -129,7 +129,7 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
 intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 {
uint8_t voltage;
-   int voltage_tries, max_vswing_tries, cr_tries, max_cr_tries;
+   int voltage_tries, cr_tries, max_cr_tries;
uint8_t link_config[2];
uint8_t link_bw, rate_select;
 
@@ -181,7 +181,6 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
max_cr_tries = 80;
 
voltage_tries = 1;
-   max_vswing_tries = 0;
for (cr_tries = 0; cr_tries < max_cr_tries; ++cr_tries) {
uint8_t link_status[DP_LINK_STATUS_SIZE];
 
@@ -202,11 +201,6 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
return false;
}
 
-   if (max_vswing_tries == 1) {
-   DRM_DEBUG_KMS("Max Voltage Swing reached\n");
-   return false;
-   }
-
voltage = intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK;
 
/* Update training set as requested by target */
@@ -222,8 +216,10 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
else
voltage_tries = 1;
 
-   if (intel_dp_link_max_vswing_reached(intel_dp))
-   ++max_vswing_tries;
+   if (intel_dp_link_max_vswing_reached(intel_dp)) {
+   DRM_DEBUG_KMS("Max Voltage Swing reached\n");
+   return false;
+   }
 
}
DRM_ERROR("Failed clock recovery %d times, giving up!\n", max_cr_tries);
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v4 1/2] drm/i915/dp: Limit link training clock recovery loop

[Nathan Ciobanu]

Limit the link training clock recovery loop to 10 attempts at
LANEx_CR_DONE per DP 1.4 spec section 3.5.1.2.2 and 80 attempts for
pre-DP 1.4 (4 voltage levels x 4 preemphasis levels x
x 5 identical voltages tries). Some faulty USB-C MST hubs can
cause us to get stuck in this loop indefinitely requesting something
like:

voltage swing: 0, pre-emphasis level: 2
voltage swing: 1, pre-emphasis level: 2
voltage swing: 0, pre-emphasis level: 3

over and over so max_vswing would never be reached,
drm_dp_clock_recovery_ok() would never return true and voltage_tries
would always get reset to 1. The driver sends those values to the hub
but the hub keeps requesting new values every time.

Changes in v2:
- updated commit message (DK, Manasi)
- defined DP_DP14_MAX_CR_TRIES (Marc)
- made the loop iterate for max 10 times (Rodrigo, Marc)

Changes in v3:
- changed error message to use DP_DP14_MAX_CR_TRIES

Changes in v4:
- Updated the title to reflect the change
- Updated the commit message
- Added 80 attempts for pre-DP 1.4 devices

Cc: Dhinakaran Pandiyan 
Cc: Rodrigo Vivi 
Cc: Marc Herbert 
Cc: Manasi Navare 
Signed-off-by: Nathan Ciobanu 
---
 drivers/gpu/drm/i915/intel_dp_link_training.c | 16 ++--
 include/drm/drm_dp_helper.h   |  1 +
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c 
b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 4da6e33c7fa1..05f209f6d73f 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -129,7 +129,7 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
 intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 {
uint8_t voltage;
-   int voltage_tries, max_vswing_tries;
+   int voltage_tries, max_vswing_tries, cr_tries, max_cr_tries;
uint8_t link_config[2];
uint8_t link_bw, rate_select;
 
@@ -170,9 +170,19 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
return false;
}
 
+   /* DP 1.4 spec clock recovery retries defined but
+* for devices pre-DP 1.4 we set the retry limit
+* to 4 (voltage levels) x 4 (preemphasis levels) x
+* x 5 (same voltage retries) = 80 (max iterations)
+*/
+   if (intel_dp->dpcd[DP_DPCD_REV] >= DP_DPCD_REV_14)
+   max_cr_tries = DP_DP14_MAX_CR_TRIES;
+   else
+   max_cr_tries = 80;
+
voltage_tries = 1;
max_vswing_tries = 0;
-   for (;;) {
+   for (cr_tries = 0; cr_tries < max_cr_tries; ++cr_tries) {
uint8_t link_status[DP_LINK_STATUS_SIZE];
 
drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
@@ -216,6 +226,8 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
++max_vswing_tries;
 
}
+   DRM_ERROR("Failed clock recovery %d times, giving up!\n", max_cr_tries);
+   return false;
 }
 
 /*
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 05cc31b5db16..2c9cee8731ce 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -820,6 +820,7 @@
 
 #define DP_DP13_DPCD_REV0x2200
 #define DP_DP13_MAX_LINK_RATE   0x2201
+#define DP_DP14_MAX_CR_TRIES10
 
 #define DP_DPRX_FEATURE_ENUMERATION_LIST0x2210  /* DP 1.3 */
 # define DP_GTC_CAP(1 << 0)  /* DP 1.3 */
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 1/2] drm/i915/mst: Do not retrain new links

[Nathan Ciobanu]

On Wed, Jul 18, 2018 at 10:19:42AM -0700, Dhinakaran Pandiyan wrote:
> The short pulse handler checks if channel equalization is okay and
> goes onto retrain a link if there are active MST links. This retraining
> path is not meant for new MST connections, but due to a bug elsewhere, if
> active_mst_links is < 0 the boolean check for active_mst_links passes and
> we proceed to retrain a new link. This results in a sequence of failed link
> training attempts, most likely due to the hardware not setup for link
> training at that point i.e., missing the DDI pre_enable sequence.
> 
> [   80.301272] [drm:intel_dp_check_mst_status] channel EQ not ok, retraining
> [   80.301312] [drm:intel_ddi_prepare_link_retrain] *ERROR* Timeout waiting 
> for DDI BUF C idle bit
> 
> The above error gives us a hint something went wrong before link
> training started.
> 
> Check for a positive value of active_mst_links and throw in a warning for
> invalid active_mst_links as debug aid.
> 
> Cc: Nathan Ciobanu 
> Cc: Rodrigo Vivi 
> Signed-off-by: Dhinakaran Pandiyan 
Tested-by: Nathan Ciobanu 
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b45b08420c1f..2d61ff01cf51 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4213,12 +4213,14 @@ intel_dp_check_mst_status(struct intel_dp *intel_dp)
>   int ret = 0;
>   int retry;
>   bool handled;
> +
> + WARN_ON_ONCE(intel_dp->active_mst_links < 0);
>   bret = intel_dp_get_sink_irq_esi(intel_dp, esi);
>  go_again:
>   if (bret == true) {
>  
>   /* check link status - esi[10] = 0x200c */
> - if (intel_dp->active_mst_links &&
> + if (intel_dp->active_mst_links > 0 &&
>   !drm_dp_channel_eq_ok([10], 
> intel_dp->lane_count)) {
>   DRM_DEBUG_KMS("channel EQ not ok, 
> retraining\n");
>   intel_dp_start_link_train(intel_dp);
> -- 
> 2.17.1
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 2/2] drm/i915/mst: Continue state updates even if AUX writes fail.

[Nathan Ciobanu]

On Wed, Jul 18, 2018 at 10:19:43AM -0700, Dhinakaran Pandiyan wrote:
> We are too late in the enabling sequence to back out cleanly, not updating
> state tracking variables, like intel_dp->active_mst_links in this
> instance, results in incorrect behaviour further along.
> 
> v2: Fixed int v/s bool comparison
> 
> Cc: Ville Syrjälä 
> Cc: Rodrigo Vivi 
> Cc: Nathan Ciobanu 
> Signed-off-by: Dhinakaran Pandiyan 
Reviewed-by: Nathan Ciobanu 
Tested-by: Nathan Ciobanu 
> ---
>  drivers/gpu/drm/i915/intel_dp_mst.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c 
> b/drivers/gpu/drm/i915/intel_dp_mst.c
> index 7e3e01607643..110e7ff22ef7 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -241,11 +241,8 @@ static void intel_mst_pre_enable_dp(struct intel_encoder 
> *encoder,
>  connector->port,
>  pipe_config->pbn,
>  pipe_config->dp_m_n.tu);
> - if (ret == false) {
> + if (!ret)
>   DRM_ERROR("failed to allocate vcpi\n");
> - return;
> - }
> -
>  
>   intel_dp->active_mst_links++;
>   temp = I915_READ(DP_TP_STATUS(port));
> -- 
> 2.17.1
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3] drm/i915/dp: Give up link training clock recovery after 10 failed attempts

[Nathan Ciobanu]

On Tue, Jul 17, 2018 at 03:21:17PM -0700, Dhinakaran Pandiyan wrote:
> On Mon, 2018-07-16 at 16:51 -0700, Marc Herbert wrote:
> > > 
> > > > 
> > > > I think the bug is with this infinite loop which is at the mercy
> > > > of an external device
> > > > and in my case I have this MST hub which appears to be DP 1.2
> > > > that triggers the
> > > > infinite loop case. We have to limit the number of iterations and
> > > > DP 1.4 spec happened to fix this issue. I'm a newbie in this area
> > > > but in this case
> > > > shouldn't we apply the same counter to all <= "DP 1.4" devices?
> > > ok, the infinite loop situation is really bad... but I don't
> > > believe
> > > we are going with the right fix...
> > > and a good indication is that your fix is for DP-1.4 while your bug
> > > is seeing on DP-1.2
> > I thought the only reason the infinite loop fix isn't in 1.2 is just
> > because there's
> > no 1.2.1 spec... (plus the naive assumption that buggy sinks don't
> > exist)
> > 
> Irrespective of whether the sink is DP1.2 or 1.4, if there are sinks
> out there that keep toggling between two values there should be an
> overall limit to how many times this loop gets executed. Even if this
> isn't right fix for the issue at hand, the loop has to break.
> 
> Then there's the question of what the limit should be. We could use the
> DP1.4 limit as a reference and apply it widely. But there's a problem
> here, we have 4 vswing values and 4 pre-emphasis values. If the sink
> progressively changes one variable at a time, we'll need at least 16
> iterations. Nathan's patch here will prematurely error out and that
> doesn't sound reasonable to impose on non DP1.4 sinks.

I think it would be a max of 13 iterations since one of the vswing
values will be max_vswing and the loop will terminate at that point
without trying the other 3 preemph values for that voltage, correct?

-Nathan
> 
> -DK
> 
> 
> > 
> > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3] drm/i915/dp: Give up link training clock recovery after 10 failed attempts

[Nathan Ciobanu]

On Mon, Jul 16, 2018 at 03:30:49PM -0700, Rodrigo Vivi wrote:
> On Mon, Jul 16, 2018 at 12:22:13PM -0700, Marc Herbert wrote:
> > 
> > While the shortness of v3 is really nice, I think it's rather a good
> > opportunity to make much clearer (as a separate, no functional change
> > patch?)  its existing terminating condition(s) and what the existing loop
> > iterates on. I mean it's painful and risky enough to _combine multiple
> > counters in the same loop_, so at least these different counters should be
> > _invidually_ crystal-clear with enough comments. For instance let's pretend
> > there are 4 possible voltage values and that each value is tried 4 times -
> > then the last value will never be tried! Can this ever happen? If not then
> > why not? Not even with a buggy sink?  If it can happen then is it fine to
> > give up before trying some values? Is it compliant with the spec? Etc.
> 
> hm.. it seems that that code has room for improvements to make it more
> clear and easier to map with the spec...
> but yeap, separated patches.
Do we like to add macros for the other counters? I can try to clarify this a 
bit.

> 
> > 
> > This should incidentally help clarify why and how the current logic allows
> > infinite loops.
> > 
> > BTW "max_vswing_tries" looks like a boolean, correct? If correct then please
> > remove this confusing "increment from false to true":
> > 
> > - if (max_vswing_tries == 1)
> > + if (max_vswing_tries)
> > 
> > - max_vswing_tries++;
> > + max_vwing_tries = true;
> 
> if we change to boolean it is better to really change the type
> and remove "_tries" from the name
Yup, will do!

> 
> > 
> > Marc
> > 
> > 
> > On 16/07/2018 11:14, Nathan Ciobanu wrote:
> > > Signed-off-by: Nathan Ciobanu 
> > > ---
> > >  drivers/gpu/drm/i915/intel_dp_link_training.c | 7 +--
> > >  include/drm/drm_dp_helper.h   | 1 +
> > >  2 files changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c 
> > > b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > index 4da6e33c7fa1..4bfba65c431c 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > @@ -129,7 +129,7 @@ static bool intel_dp_link_max_vswing_reached(struct 
> > > intel_dp *intel_dp)
> > >  intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> > >  {
> > >   uint8_t voltage;
> > > - int voltage_tries, max_vswing_tries;
> > > + int voltage_tries, max_vswing_tries, cr_tries;
> > >   uint8_t link_config[2];
> > >   uint8_t link_bw, rate_select;
> > >  
> > > @@ -172,7 +172,7 @@ static bool intel_dp_link_max_vswing_reached(struct 
> > > intel_dp *intel_dp)
> > >  
> > >   voltage_tries = 1;
> > >   max_vswing_tries = 0;
> > > - for (;;) {
> > > + for (cr_tries = 0; cr_tries < DP_DP14_MAX_CR_TRIES; ++cr_tries) {
> > >   uint8_t link_status[DP_LINK_STATUS_SIZE];
> > >  
> > >   drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
> > > @@ -216,6 +216,9 @@ static bool intel_dp_link_max_vswing_reached(struct 
> > > intel_dp *intel_dp)
> > >   ++max_vswing_tries;
> > >  
> > >   }
> > > + DRM_ERROR("Failed clock recovery %d times, giving up!\n",
> > > +   DP_DP14_MAX_CR_TRIES);
> > > + return false;
> > >  }
> > >  
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3] drm/i915/dp: Give up link training clock recovery after 10 failed attempts

[Nathan Ciobanu]

On Mon, Jul 16, 2018 at 03:34:58PM -0700, Rodrigo Vivi wrote:
> On Mon, Jul 16, 2018 at 11:14:45AM -0700, Nathan Ciobanu wrote:
> > Limit the link training clock recovery loop to 10 failed attempts at
> > LANEx_CR_DONE per DP 1.4 spec section 3.5.1.2.2.
> 
> Thanks... so this made me look to DP 1.3 and DP 1.4 differences here.
> Are we already addressing all new conditions?
> 
> I wonder if that should be at drm level.
> 
> I noticed that one difference, 100us wait difference is already
> addressed there at drm level...
> 
> > Some USB-C MST hubs
> > cause us to get stuck in this loop indefinitely requesting
> > 
> > voltage swing: 0, pre-emphasis level: 2
> > voltage swing: 1, pre-emphasis level: 2
> > voltage swing: 0, pre-emphasis level: 3
> > 
> > over and over: max_vswing is never reached, drm_dp_clock_recovery_ok()
> > never returns true and voltage_tries always gets reset to 1. The driver
> > sends those values to the hub but the hub keeps requesting new values
> > every time in the pattern shown above.
> > 
> > Changes in v2:
> > - updated commit message (DK, Manasi)
> > - defined DP_DP14_MAX_CR_TRIES (Marc)
> > - made the loop iterate for max 10 times (Rodrigo, Marc)
> > 
> > Changes in v3:
> > - changed error message to use DP_DP14_MAX_CR_TRIES
> > 
> > Signed-off-by: Nathan Ciobanu 
> > ---
> >  drivers/gpu/drm/i915/intel_dp_link_training.c | 7 +--
> >  include/drm/drm_dp_helper.h   | 1 +
> >  2 files changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c 
> > b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > index 4da6e33c7fa1..4bfba65c431c 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > @@ -129,7 +129,7 @@ static bool intel_dp_link_max_vswing_reached(struct 
> > intel_dp *intel_dp)
> >  intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> >  {
> > uint8_t voltage;
> > -   int voltage_tries, max_vswing_tries;
> > +   int voltage_tries, max_vswing_tries, cr_tries;
> > uint8_t link_config[2];
> > uint8_t link_bw, rate_select;
> >  
> > @@ -172,7 +172,7 @@ static bool intel_dp_link_max_vswing_reached(struct 
> > intel_dp *intel_dp)
> >  
> > voltage_tries = 1;
> > max_vswing_tries = 0;
> > -   for (;;) {
> > +   for (cr_tries = 0; cr_tries < DP_DP14_MAX_CR_TRIES; ++cr_tries) {
> 
> I know that I was the on that asked you to make this global here,
> but just now with better spec pointers and definition is that I noticed
> that this is only for DP 1.4 spec while this code here runs for all
> DP...  So we need to change in a way that we check the spec version somehow...
I think the bug is with this infinite loop which is at the mercy of an external 
device
and in my case I have this MST hub which appears to be DP 1.2 that triggers the
infinite loop case. We have to limit the number of iterations and
DP 1.4 spec happened to fix this issue. I'm a newbie in this area but in this 
case
shouldn't we apply the same counter to all <= "DP 1.4" devices?

-Nathan 
> 
> and possibly at drm level?!
> 
> > uint8_t link_status[DP_LINK_STATUS_SIZE];
> >  
> > drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
> > @@ -216,6 +216,9 @@ static bool intel_dp_link_max_vswing_reached(struct 
> > intel_dp *intel_dp)
> > ++max_vswing_tries;
> >  
> > }
> > +   DRM_ERROR("Failed clock recovery %d times, giving up!\n",
> > + DP_DP14_MAX_CR_TRIES);
> > +   return false;
> >  }
> >  
> >  /*
> > diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> > index c01564991a9f..2303ad8ed24e 100644
> > --- a/include/drm/drm_dp_helper.h
> > +++ b/include/drm/drm_dp_helper.h
> > @@ -820,6 +820,7 @@
> >  
> >  #define DP_DP13_DPCD_REV0x2200
> >  #define DP_DP13_MAX_LINK_RATE   0x2201
> > +#define DP_DP14_MAX_CR_TRIES10
> >  
> >  #define DP_DPRX_FEATURE_ENUMERATION_LIST0x2210  /* DP 1.3 */
> >  # define DP_GTC_CAP(1 << 0)  /* DP 
> > 1.3 */
> > -- 
> > 1.9.1
> > 
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v3] drm/i915/dp: Give up link training clock recovery after 10 failed attempts

[Nathan Ciobanu]

Limit the link training clock recovery loop to 10 failed attempts at
LANEx_CR_DONE per DP 1.4 spec section 3.5.1.2.2. Some USB-C MST hubs
cause us to get stuck in this loop indefinitely requesting

voltage swing: 0, pre-emphasis level: 2
voltage swing: 1, pre-emphasis level: 2
voltage swing: 0, pre-emphasis level: 3

over and over: max_vswing is never reached, drm_dp_clock_recovery_ok()
never returns true and voltage_tries always gets reset to 1. The driver
sends those values to the hub but the hub keeps requesting new values
every time in the pattern shown above.

Changes in v2:
- updated commit message (DK, Manasi)
- defined DP_DP14_MAX_CR_TRIES (Marc)
- made the loop iterate for max 10 times (Rodrigo, Marc)

Changes in v3:
- changed error message to use DP_DP14_MAX_CR_TRIES

Signed-off-by: Nathan Ciobanu 
---
 drivers/gpu/drm/i915/intel_dp_link_training.c | 7 +--
 include/drm/drm_dp_helper.h   | 1 +
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c 
b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 4da6e33c7fa1..4bfba65c431c 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -129,7 +129,7 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
 intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 {
uint8_t voltage;
-   int voltage_tries, max_vswing_tries;
+   int voltage_tries, max_vswing_tries, cr_tries;
uint8_t link_config[2];
uint8_t link_bw, rate_select;
 
@@ -172,7 +172,7 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
 
voltage_tries = 1;
max_vswing_tries = 0;
-   for (;;) {
+   for (cr_tries = 0; cr_tries < DP_DP14_MAX_CR_TRIES; ++cr_tries) {
uint8_t link_status[DP_LINK_STATUS_SIZE];
 
drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
@@ -216,6 +216,9 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
++max_vswing_tries;
 
}
+   DRM_ERROR("Failed clock recovery %d times, giving up!\n",
+ DP_DP14_MAX_CR_TRIES);
+   return false;
 }
 
 /*
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index c01564991a9f..2303ad8ed24e 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -820,6 +820,7 @@
 
 #define DP_DP13_DPCD_REV0x2200
 #define DP_DP13_MAX_LINK_RATE   0x2201
+#define DP_DP14_MAX_CR_TRIES10
 
 #define DP_DPRX_FEATURE_ENUMERATION_LIST0x2210  /* DP 1.3 */
 # define DP_GTC_CAP(1 << 0)  /* DP 1.3 */
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v2] drm/i915/dp: Give up link training clock recovery after 10 failed attempts

[Nathan Ciobanu]

Limit the link training clock recovery loop to 10 failed attempts at
LANEx_CR_DONE per DP 1.4 spec section 3.5.1.2.2. Some USB-C MST hubs
cause us to get stuck in this loop indefinitely requesting

voltage swing: 0, pre-emphasis level: 2
voltage swing: 1, pre-emphasis level: 2
voltage swing: 0, pre-emphasis level: 3

over and over: max_vswing is never reached, drm_dp_clock_recovery_ok()
never returns true and voltage_tries always gets reset to 1. The driver
sends those values to the hub but the hub keeps requesting new values
every time in the pattern shown above.

Changes in v2:
- updated commit message (DK, Manasi)
- defined DP_DP14_MAX_CR_TRIES (Marc)
- made the loop iterate for max 10 times (Rodrigo, Marc)

Signed-off-by: Nathan Ciobanu 
---
 drivers/gpu/drm/i915/intel_dp_link_training.c | 6 --
 include/drm/drm_dp_helper.h   | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c 
b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 4da6e33c7fa1..671de62aacc2 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -129,7 +129,7 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
 intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 {
uint8_t voltage;
-   int voltage_tries, max_vswing_tries;
+   int voltage_tries, max_vswing_tries, cr_tries;
uint8_t link_config[2];
uint8_t link_bw, rate_select;
 
@@ -172,7 +172,7 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
 
voltage_tries = 1;
max_vswing_tries = 0;
-   for (;;) {
+   for (cr_tries = 0; cr_tries < DP_DP14_MAX_CR_TRIES; ++cr_tries) {
uint8_t link_status[DP_LINK_STATUS_SIZE];
 
drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
@@ -216,6 +216,8 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
++max_vswing_tries;
 
}
+   DRM_ERROR("Failed clock recovery 10 times, giving up!\n");
+   return false;
 }
 
 /*
diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index c01564991a9f..2303ad8ed24e 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -820,6 +820,7 @@
 
 #define DP_DP13_DPCD_REV0x2200
 #define DP_DP13_MAX_LINK_RATE   0x2201
+#define DP_DP14_MAX_CR_TRIES10
 
 #define DP_DPRX_FEATURE_ENUMERATION_LIST0x2210  /* DP 1.3 */
 # define DP_GTC_CAP(1 << 0)  /* DP 1.3 */
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/dp: Give up link training clock recovery after 10 failed attempts

[Nathan Ciobanu]

On Fri, Jul 13, 2018 at 03:18:32PM -0700, Manasi Navare wrote:
> On Fri, Jul 13, 2018 at 03:23:41PM -0700, Dhinakaran Pandiyan wrote:
> > On Fri, 2018-07-13 at 14:22 -0700, Rodrigo Vivi wrote:
> > > On Fri, Jul 13, 2018 at 10:32:15AM -0700, Nathan Ciobanu wrote:
> > > > 
> > > > Limit the link training clock recovery loop to 10 failed attempts
> > > > at
> > > > LANEx_CR_DONE per DP 1.4 spec.
> > > Where exactly in the spec?
> 
> I see that this is specified in 3.5.1.2.2 Clock Recovery Sequence section
> in DP 1.4 spec. Might be a good idea to mention this expliitly in the commit 
> message.
Right, I'll do that.
> 
> > > 
> > > > 
> > > > Some USB-C MST hubs cause us to get
> > > > stuck in this loop on hot-plugging indefinitely as
> 
> This is likely to occur in case of USB Type C cases where only fewer lanes
> might be connected whereas the max lane count returned is still 4.
> If this is the case, explicitly mentioning this is also a good idea.
I'm pretty sure this device uses 4 lanes. I am getting voltage 
swing/pre-emphasis
values and status on all 4.
> 
> Doesnt this get recovered at all after the fallback when it will eventually
> try with the lower lane count?
Yes it does recover after the 10 attempts.
> 
> > 
> > Also include the information (the vswing toggling part) about why it is
> > stuck in the loop. 
> > 
> > > > drm_dp_clock_recovery_ok() never returns true and none of the
> > > > other conditions occur.
> > > Although it seems really bad situation that we need to avoid...
> > > 
> > > > 
> > > > 
> > > > Signed-off-by: Nathan Ciobanu 
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_dp_link_training.c | 8 +++-
> > > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > > b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > > index 4da6e33c7fa1..66c1a70343ba 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > > > @@ -129,7 +129,7 @@ static bool
> > > > intel_dp_link_max_vswing_reached(struct intel_dp *intel_dp)
> > > >  intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> > > >  {
> > > >     uint8_t voltage;
> > > > -   int voltage_tries, max_vswing_tries;
> > > > +   int voltage_tries, max_vswing_tries, cr_tries;
> > > >     uint8_t link_config[2];
> > > >     uint8_t link_bw, rate_select;
> > > >  
> > > > @@ -172,6 +172,7 @@ static bool
> > > > intel_dp_link_max_vswing_reached(struct intel_dp *intel_dp)
> > > >  
> > > >     voltage_tries = 1;
> > > >     max_vswing_tries = 0;
> > > > +   cr_tries = 0;
> > > >     for (;;) {
> > > >     uint8_t link_status[DP_LINK_STATUS_SIZE];
> > > >  
> > > > @@ -215,6 +216,11 @@ static bool
> > > > intel_dp_link_max_vswing_reached(struct intel_dp *intel_dp)
> > > >     if (intel_dp_link_max_vswing_reached(intel_dp))
> > > >     ++max_vswing_tries;
> > > >  
> > > > +   if (cr_tries == 9) {
> > > > +   DRM_ERROR("Failed clock recovery 10 times,
> > > > giving up!\n");
> > > > +   return false;
> > > > +   }
> > > > +   ++cr_tries;
> > > If I understood correctly this is a global thing for the for(;;)
> > > right?
> > > 
> > > Shouldn't we make then like a:
> > > 
> > > - for(;;)
> > > + for(cr_tries = 0; cr_tries < 10; cr_tries++)
> > >   {
> > >   }
> 
> I think incrementing it at the end of the loop makes sense because we
> are checking for the 10 total read requests for lane_cr_done
> and adjust_request_lane.
> But if the max voltage swing is reached before that it should exit earlier.
> 
> Manasi
> 
> > > 
> > > + DRM_ERROR("Failed clock recovery 10 times, giving up!\n"); 
> > > + return false;
> > > }
> > > 
> > > Thanks,
> > > Rodrigo.
> > > 
> > > > 
> > > >     }
> > > >  }
> > > >  
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/dp: Give up link training clock recovery after 10 failed attempts

[Nathan Ciobanu]

On Fri, Jul 13, 2018 at 02:22:03PM -0700, Rodrigo Vivi wrote:
> On Fri, Jul 13, 2018 at 10:32:15AM -0700, Nathan Ciobanu wrote:
> > Limit the link training clock recovery loop to 10 failed attempts at
> > LANEx_CR_DONE per DP 1.4 spec.
> 
> Where exactly in the spec?
I'll add the section number to the commit message.
> 
> > Some USB-C MST hubs cause us to get
> > stuck in this loop on hot-plugging indefinitely as
> > drm_dp_clock_recovery_ok() never returns true and none of the
> > other conditions occur.
> 
> Although it seems really bad situation that we need to avoid...
> 
> > 
> > Signed-off-by: Nathan Ciobanu 
> > ---
> >  drivers/gpu/drm/i915/intel_dp_link_training.c | 8 +++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c 
> > b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > index 4da6e33c7fa1..66c1a70343ba 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > @@ -129,7 +129,7 @@ static bool intel_dp_link_max_vswing_reached(struct 
> > intel_dp *intel_dp)
> >  intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> >  {
> > uint8_t voltage;
> > -   int voltage_tries, max_vswing_tries;
> > +   int voltage_tries, max_vswing_tries, cr_tries;
> > uint8_t link_config[2];
> > uint8_t link_bw, rate_select;
> >  
> > @@ -172,6 +172,7 @@ static bool intel_dp_link_max_vswing_reached(struct 
> > intel_dp *intel_dp)
> >  
> > voltage_tries = 1;
> > max_vswing_tries = 0;
> > +   cr_tries = 0;
> > for (;;) {
> > uint8_t link_status[DP_LINK_STATUS_SIZE];
> >  
> > @@ -215,6 +216,11 @@ static bool intel_dp_link_max_vswing_reached(struct 
> > intel_dp *intel_dp)
> > if (intel_dp_link_max_vswing_reached(intel_dp))
> > ++max_vswing_tries;
> >  
> > +   if (cr_tries == 9) {
> > +   DRM_ERROR("Failed clock recovery 10 times, giving 
> > up!\n");
> > +   return false;
> > +   }
> > +   ++cr_tries;
> 
> If I understood correctly this is a global thing for the for(;;) right?
> 
> Shouldn't we make then like a:
> 
> - for(;;)
> + for(cr_tries = 0; cr_tries < 10; cr_tries++)
>   {
>   }
> 
> + DRM_ERROR("Failed clock recovery 10 times, giving up!\n"); 
> + return false;
> }
That was my thought initially as well but I was worried that
it will not be immediately obvious why I'm returning false after
the loop - although the error message tells you why. I'll change
this. 

-Nathan
> 
> Thanks,
> Rodrigo.
> 
> > }
> >  }
> >  
> > -- 
> > 1.9.1
> > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915/dp: Give up link training clock recovery after 10 failed attempts

[Nathan Ciobanu]

Limit the link training clock recovery loop to 10 failed attempts at
LANEx_CR_DONE per DP 1.4 spec. Some USB-C MST hubs cause us to get
stuck in this loop on hot-plugging indefinitely as
drm_dp_clock_recovery_ok() never returns true and none of the
other conditions occur.

Signed-off-by: Nathan Ciobanu 
---
 drivers/gpu/drm/i915/intel_dp_link_training.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c 
b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 4da6e33c7fa1..66c1a70343ba 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -129,7 +129,7 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
 intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 {
uint8_t voltage;
-   int voltage_tries, max_vswing_tries;
+   int voltage_tries, max_vswing_tries, cr_tries;
uint8_t link_config[2];
uint8_t link_bw, rate_select;
 
@@ -172,6 +172,7 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
 
voltage_tries = 1;
max_vswing_tries = 0;
+   cr_tries = 0;
for (;;) {
uint8_t link_status[DP_LINK_STATUS_SIZE];
 
@@ -215,6 +216,11 @@ static bool intel_dp_link_max_vswing_reached(struct 
intel_dp *intel_dp)
if (intel_dp_link_max_vswing_reached(intel_dp))
++max_vswing_tries;
 
+   if (cr_tries == 9) {
+   DRM_ERROR("Failed clock recovery 10 times, giving 
up!\n");
+   return false;
+   }
+   ++cr_tries;
}
 }
 
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [CI 1/6] drm/i915/psr: New power domain for AUX IO.

[Nathan Ciobanu]

On Fri, Feb 23, 2018 at 02:15:15PM -0800, Dhinakaran Pandiyan wrote:
> From: "Dhinakaran Pandiyan" <dhinakaran.pandi...@intel.com>
> 
> PSR on CNL requires AUX IO wells to be kept on and the existing AUX domain
> for AUX-A enables DC_OFF well too. This is not required, so add a new
> AUX_IO_A domain for AUX-A to allow DC states to remain enabled. Other AUX
> channels re-use the existing AUX domains.
> 
> v4: Reword comment (Rodrigo and Ville)
> Rename _get and _put functions to include aux_io substring(Rodrigo)
> Remove unnecessary diff that got included.
> v3: Extract aux domain selection into a function (Ville)
> v2: Add AUX IO domain only for AUX-A
> Rebased on top of Ville's AUX series.
> 
> Cc: Imre Deak <imre.d...@intel.com>
> Cc: Rodrigo Vivi <rodrigo.v...@intel.com>
> Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> Suggested-by: Imre Deak <imre.d...@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.v...@intel.com>
 Tested-by: Nathan Ciobanu <nathan.d.ciob...@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.h|  1 +
>  drivers/gpu/drm/i915/intel_psr.c| 41 
> +
>  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +++
>  3 files changed, 45 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.h 
> b/drivers/gpu/drm/i915/intel_display.h
> index f5733a2576e7..4e7418b345bc 100644
> --- a/drivers/gpu/drm/i915/intel_display.h
> +++ b/drivers/gpu/drm/i915/intel_display.h
> @@ -186,6 +186,7 @@ enum intel_display_power_domain {
>   POWER_DOMAIN_AUX_C,
>   POWER_DOMAIN_AUX_D,
>   POWER_DOMAIN_AUX_F,
> + POWER_DOMAIN_AUX_IO_A,
>   POWER_DOMAIN_GMBUS,
>   POWER_DOMAIN_MODESET,
>   POWER_DOMAIN_GT_IRQ,
> diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> b/drivers/gpu/drm/i915/intel_psr.c
> index 2ef374f936b9..04430d4c99c9 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -56,6 +56,43 @@
>  #include "intel_drv.h"
>  #include "i915_drv.h"
>  
> +static inline enum intel_display_power_domain
> +psr_aux_domain(struct intel_dp *intel_dp)
> +{
> + /* CNL HW requires corresponding AUX IOs to be powered up for PSR.
> +  * However, for non-A AUX ports the corresponding non-EDP transcoders
> +  * would have already enabled power well 2 and DC_OFF. This means we can
> +  * acquire a wider POWER_DOMAIN_AUX_{B,C,D,F} reference instead of a
> +  * specific AUX_IO reference without powering up any extra wells.
> +  * Note that PSR is enabled only on Port A even though this function
> +  * returns the correct domain for other ports too.
> +  */
> + return intel_dp->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
> +   intel_dp->aux_power_domain;
> +}
> +
> +static void psr_aux_io_power_get(struct intel_dp *intel_dp)
> +{
> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> + struct drm_i915_private *dev_priv = 
> to_i915(intel_dig_port->base.base.dev);
> +
> + if (INTEL_GEN(dev_priv) < 10)
> + return;
> +
> + intel_display_power_get(dev_priv, psr_aux_domain(intel_dp));
> +}
> +
> +static void psr_aux_io_power_put(struct intel_dp *intel_dp)
> +{
> + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> + struct drm_i915_private *dev_priv = 
> to_i915(intel_dig_port->base.base.dev);
> +
> + if (INTEL_GEN(dev_priv) < 10)
> + return;
> +
> + intel_display_power_put(dev_priv, psr_aux_domain(intel_dp));
> +}
> +
>  static bool vlv_is_psr_active_on_pipe(struct drm_device *dev, int pipe)
>  {
>   struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -459,6 +496,8 @@ static void hsw_psr_enable_source(struct intel_dp 
> *intel_dp,
>   enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>   u32 chicken;
>  
> + psr_aux_io_power_get(intel_dp);
> +
>   if (dev_priv->psr.psr2_support) {
>   chicken = PSR2_VSC_ENABLE_PROG_HEADER;
>   if (dev_priv->psr.y_cord_support)
> @@ -617,6 +656,8 @@ static void hsw_psr_disable(struct intel_dp *intel_dp,
>   else
>   WARN_ON(I915_READ(EDP_PSR_CTL) & EDP_PSR_ENABLE);
>   }
> +
> + psr_aux_io_power_put(intel_dp);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index b7924feb9f27..53ea564f971e 100644
> --- a/drivers/gp

Re: [Intel-gfx] [PATCH] drm/i915/psr: Check for power state control capability.

[Nathan Ciobanu]

On Mon, Feb 26, 2018 at 07:27:23PM -0800, Dhinakaran Pandiyan wrote:
> eDP spec says - "If PSR/PSR2 is supported, the SET_POWER_CAPABLE bit in the
> EDP_GENERAL_CAPABILITY_1 register (DPCD Address 00701h, bit d7) must be set
> to 1."
> 
> Reject PSR on panels without this cap bit set as such panels cannot be
> controlled via SET_POWER & SET_DP_PWR_VOLTAGE register and the DP source
> needs to be able to do that for PSR.
> 
> Thanks to Nathan for debugging this.
> 
> Panel cap checks like this can be done just once, let's fix this
> when PSR dpcd init movement lands.
> 
> Cc: Nathan D Ciobanu <nathan.d.ciob...@intel.com>
> Cc: Rodrigo Vivi <rodrigo.v...@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com>
 Tested-by: Nathan Ciobanu <nathan.d.ciob...@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> b/drivers/gpu/drm/i915/intel_psr.c
> index 89f41d28c44a..e0701b7f87f7 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -405,6 +405,11 @@ void intel_psr_compute_config(struct intel_dp *intel_dp,
>   return;
>   }
>  
> + if (!(intel_dp->edp_dpcd[1] & DP_EDP_SET_POWER_CAP)) {
> + DRM_DEBUG_KMS("PSR condition failed: panel lacks power state 
> control\n");
> + return;
> + }
> +
>   /*
>* FIXME psr2_support is messed up. It's both computed
>* dynamically during PSR enable, and extracted from sink
> -- 
> 2.14.1
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/dp/mst: fix kernel oops when turning off secondary monitor

[Nathan Ciobanu]

I tested this patch in dinq on a KBL system and it fixed the bug. The 
system doesn't crash on disconnecting or powering off the second monitor 
in the DP-MST chain. I also replied to the Bugzilla issue.


Tested-by: Nathan D Ciobanu 

On 12/05/2016 01:49 PM, Pierre-Louis Bossart wrote:

100% reproducible issue found on SKL SkullCanyon NUC with two external
DP daisy-chained monitors in DP/MST mode. When turning off or changing
the input of the second monitor the machine stops with a kernel
oops. This issue happened with 4.8.8 as well as drm/drm-intel-nightly.

This issue is traced to an inconsistent control flow in
drm_dp_update_payload_part1(): the 'port' pointer is set to NULL at
the same time as 'req_payload.num_slots' is set to zero, but the pointer
is dereferenced even when req_payload.num_slot is zero.

The problematic dereference was introduced in commit dfda0df34
("drm/mst: rework payload table allocation to conform better")
and may impact all versions since v3.18

The fix suggested by Chris Wilson removes the kernel oops and was found to
work well after 10mn of monkey-testing with the second monitor power and
input buttons

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98990
Cc: Dave Airlie 
Signed-off-by: Pierre-Louis Bossart 
---
  drivers/gpu/drm/drm_dp_mst_topology.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
b/drivers/gpu/drm/drm_dp_mst_topology.c
index aa64448..f59771d 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -1817,7 +1817,7 @@ int drm_dp_update_payload_part1(struct 
drm_dp_mst_topology_mgr *mgr)
mgr->payloads[i].vcpi = req_payload.vcpi;
} else if (mgr->payloads[i].num_slots) {
mgr->payloads[i].num_slots = 0;
-   drm_dp_destroy_payload_step1(mgr, port, 
port->vcpi.vcpi, >payloads[i]);
+   drm_dp_destroy_payload_step1(mgr, port, 
mgr->payloads[i].vcpi, >payloads[i]);
req_payload.payload_state = 
mgr->payloads[i].payload_state;
mgr->payloads[i].start_slot = 0;
}


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2] drm/dp/mst: fix kernel oops when turning off secondary monitor

[Nathan Ciobanu]

I tested this on a KBL using 01-25-2017 dinq and it fixed the bug.


Tested-by: Nathan D Ciobanu 


On 12/05/2016 01:49 PM, Pierre-Louis Bossart wrote:

100% reproducible issue found on SKL SkullCanyon NUC with two external
DP daisy-chained monitors in DP/MST mode. When turning off or changing
the input of the second monitor the machine stops with a kernel
oops. This issue happened with 4.8.8 as well as drm/drm-intel-nightly.

This issue is traced to an inconsistent control flow in
drm_dp_update_payload_part1(): the 'port' pointer is set to NULL at
the same time as 'req_payload.num_slots' is set to zero, but the pointer
is dereferenced even when req_payload.num_slot is zero.

The problematic dereference was introduced in commit dfda0df34
("drm/mst: rework payload table allocation to conform better")
and may impact all versions since v3.18

The fix suggested by Chris Wilson removes the kernel oops and was found to
work well after 10mn of monkey-testing with the second monitor power and
input buttons

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98990
Cc: Dave Airlie 
Signed-off-by: Pierre-Louis Bossart 
---
  drivers/gpu/drm/drm_dp_mst_topology.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_dp_mst_topology.c 
b/drivers/gpu/drm/drm_dp_mst_topology.c
index aa64448..f59771d 100644
--- a/drivers/gpu/drm/drm_dp_mst_topology.c
+++ b/drivers/gpu/drm/drm_dp_mst_topology.c
@@ -1817,7 +1817,7 @@ int drm_dp_update_payload_part1(struct 
drm_dp_mst_topology_mgr *mgr)
mgr->payloads[i].vcpi = req_payload.vcpi;
} else if (mgr->payloads[i].num_slots) {
mgr->payloads[i].num_slots = 0;
-   drm_dp_destroy_payload_step1(mgr, port, 
port->vcpi.vcpi, >payloads[i]);
+   drm_dp_destroy_payload_step1(mgr, port, 
mgr->payloads[i].vcpi, >payloads[i]);
req_payload.payload_state = 
mgr->payloads[i].payload_state;
mgr->payloads[i].start_slot = 0;
}


___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx