[Intel-gfx] [PATCH 2/2] drm/i915/lspcon: Add workaround for resuming in PCON mode

[Jani Nikula]

On Thu, 20 Oct 2016, Imre Deak <imre.deak at intel.com> wrote:
> On my APL the LSPCON firmware resumes in PCON mode as opposed to the
> expected LS mode. It also appears to be in a state where AUX DPCD reads
> will succeed but return garbage recovering only after a few hundreds of
> milliseconds. After the recovery time DPCD reads will result in the
> correct values and things will continue to work. If I2C over AUX is
> attempted during this recovery time (implying an AUX write transaction)
> the firmware won't recover and will stay in this broken state.
>
> As a workaround check if the firmware is in PCON state after resume and
> if so wait until the correct DPCD values are returned. For this we
> compare the branch descriptor with the one we cached during init time.
> If the firmware was in the LS state, we skip the w/a and continue as
> before.
>
> Cc: Shashank Sharma <shashank.sharma at intel.com>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: Jani Nikula <jani.nikula at intel.com>
> Signed-off-by: Imre Deak <imre.deak at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c     |  2 +-
>  drivers/gpu/drm/i915/intel_drv.h    |  6 ++++-
>  drivers/gpu/drm/i915/intel_lspcon.c | 52 ++++++++++++++++++++++++++++++-------
>  3 files changed, 48 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e90211e..ec031db 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3487,7 +3487,7 @@ intel_dp_link_down(struct intel_dp *intel_dp)
>  	intel_dp->DP = DP;
>  }
>  
> -static bool
> +bool
>  intel_dp_read_dpcd(struct intel_dp *intel_dp)
>  {
>  	if (drm_dp_dpcd_read(&intel_dp->aux, 0x000, intel_dp->dpcd,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index a35e241..9a2366e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -972,7 +972,9 @@ struct intel_dp {
>  struct intel_lspcon {
>  	bool active;
>  	enum drm_lspcon_mode mode;
> -	struct drm_dp_aux *aux;
> +	struct intel_dp *intel_dp;
> +	bool desc_valid;
> +	struct intel_dp_desc desc;

I guess we could cache the desc in intel_dp directly. Independent of
this patch.

Also, I'm wondering if we could stick with just aux here, and read
something else from dpcd instead.

>  };
>  
>  struct intel_digital_port {
> @@ -1469,6 +1471,8 @@ static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
>  }
>  
>  bool
> +intel_dp_read_dpcd(struct intel_dp *intel_dp);
> +bool
>  intel_dp_read_desc(struct intel_dp *intel_dp, struct intel_dp_desc *desc);
>  void
>  intel_dp_print_desc(struct intel_dp *intel_dp, struct intel_dp_desc *desc);
> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> index d2c8cb2..54c6173 100644
> --- a/drivers/gpu/drm/i915/intel_lspcon.c
> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> @@ -30,7 +30,7 @@
>  static enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon *lspcon)
>  {
>  	enum drm_lspcon_mode current_mode = DRM_LSPCON_MODE_INVALID;
> -	struct i2c_adapter *adapter = &lspcon->aux->ddc;
> +	struct i2c_adapter *adapter = &lspcon->intel_dp->aux.ddc;
>  
>  	if (drm_lspcon_get_mode(adapter, &current_mode))
>  		DRM_ERROR("Error reading LSPCON mode\n");
> @@ -45,7 +45,7 @@ static int lspcon_change_mode(struct intel_lspcon *lspcon,
>  {
>  	int err;
>  	enum drm_lspcon_mode current_mode;
> -	struct i2c_adapter *adapter = &lspcon->aux->ddc;
> +	struct i2c_adapter *adapter = &lspcon->intel_dp->aux.ddc;
>  
>  	err = drm_lspcon_get_mode(adapter, &current_mode);
>  	if (err) {
> @@ -72,7 +72,7 @@ static int lspcon_change_mode(struct intel_lspcon *lspcon,
>  static bool lspcon_probe(struct intel_lspcon *lspcon)
>  {
>  	enum drm_dp_dual_mode_type adaptor_type;
> -	struct i2c_adapter *adapter = &lspcon->aux->ddc;
> +	struct i2c_adapter *adapter = &lspcon->intel_dp->aux.ddc;
>  
>  	/* Lets probe the adaptor and check its type */
>  	adaptor_type = drm_dp_dual_mode_detect(adapter);
> @@ -89,8 +89,42 @@ static bool lspcon_probe(struct intel_lspcon *lspcon)
>  	return true;
>  }
>  
> +static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon)
> +{
> +	unsigned long start = jiffies;
> +
> +	if (!lspcon->desc_valid)
> +		return;
> +
> +	while (1) {
> +		struct intel_dp_desc desc;
> +
> +		/*
> +		 * The w/a only applies in PCON mode and we don't expect any
> +		 * AUX errors.
> +		 */
> +		if (!intel_dp_read_desc(lspcon->intel_dp, &desc))
> +			return;
> +
> +		if (!memcmp(&lspcon->desc, &desc, sizeof(desc))) {
> +			DRM_DEBUG_KMS("LSPCON recovering in PCON mode after %u ms\n",
> +				      jiffies_to_msecs(jiffies - start));
> +			return;
> +		}
> +
> +		if (time_after(jiffies, start + msecs_to_jiffies(1000)))
> +			break;
> +
> +		msleep(10);
> +	}
> +
> +	DRM_DEBUG_KMS("LSPCON DP descriptor mismatch after resume\n");
> +}

I think we need to go through the LSPCON spec once more before we merge
this. Maybe we don't reset the LSPCON properly. Maybe we don't handle
the resume properly. Maybe this isn't a "workaround" after all.

BR,
Jani.

> +
>  void lspcon_resume(struct intel_lspcon *lspcon)
>  {
> +	lspcon_resume_in_pcon_wa(lspcon);
> +
>  	if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON, true))
>  		DRM_ERROR("LSPCON resume failed\n");
>  	else
> @@ -111,7 +145,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>  
>  	lspcon->active = false;
>  	lspcon->mode = DRM_LSPCON_MODE_INVALID;
> -	lspcon->aux = &dp->aux;
> +	lspcon->intel_dp = dp;
>  
>  	if (!lspcon_probe(lspcon)) {
>  		DRM_ERROR("Failed to probe lspcon\n");
> @@ -131,12 +165,10 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>  		}
>  	}
>  
> -	if (drm_debug & DRM_UT_KMS) {
> -		struct intel_dp_desc desc;
> -
> -		if (intel_dp_read_desc(dp, &desc))
> -			intel_dp_print_desc(dp, &desc);
> -	}
> +	lspcon->desc_valid = intel_dp_read_dpcd(dp) &&
> +			     intel_dp_read_desc(dp, &lspcon->desc);
> +	if ((drm_debug & DRM_UT_KMS) && lspcon->desc_valid)
> +		intel_dp_print_desc(dp, &lspcon->desc);
>  
>  	DRM_DEBUG_KMS("Success: LSPCON init\n");
>  	return true;

-- 
Jani Nikula, Intel Open Source Technology Center