Re: [PATCH 3/4] drm/i915/lspcon: Wait for expected LSPCON mode to settle

[Imre Deak <imre.deak@intel.com>]

On Tue, 2016-11-22 at 21:06 +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 11/22/2016 12:45 AM, Imre Deak wrote:
> > Some LSPCON adaptors may return an incorrect LSPCON mode right after
> > waking from DP Sleep state. This is the case at least for the ParadTech
> > PS175 adaptor, both when waking because of exiting the DP Sleep to
> > active state, or due to any other AUX CH transfer. We can determine the
> > current expected mode based on whether the DPCD area is accessible,
> > since according to the LSPCON spec this area is only accesible
> > in PCON mode.
> > 
> > This wait will avoid us trying to change the mode, while the current
> > expected mode hasn't settled yet and start link training before the
> > adaptor thinks it's in PCON mode after waking from DP Sleep state.
> > 
> > Cc: Shashank Sharma <shashank.sharma@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_dp.c     |  5 +++
> >   drivers/gpu/drm/i915/intel_drv.h    |  1 +
> >   drivers/gpu/drm/i915/intel_lspcon.c | 70 ++++++++++++++++++++++++++++++++-----
> >   3 files changed, 68 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 16c19d78..8026a83 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -2401,6 +2401,8 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
> >   		ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_SET_POWER,
> >   					 DP_SET_POWER_D3);
> >   	} else {
> > +		struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
> > +
> >   		/*
> >   		 * When turning on, we need to retry for 1ms to give the sink
> >   		 * time to wake up.
> > @@ -2412,6 +2414,9 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
> >   				break;
> >   			msleep(1);
> >   		}
> > +
> > +		if (ret == 1 && lspcon->active)
> > +			lspcon_wait_pcon_mode(lspcon);
> >   	}
> >   
> >   	if (ret != 1)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index cf47e8a..d165904 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1830,4 +1830,5 @@ void intel_color_load_luts(struct drm_crtc_state *crtc_state);
> >   /* intel_lspcon.c */
> >   bool lspcon_init(struct intel_digital_port *intel_dig_port);
> >   void lspcon_resume(struct intel_lspcon *lspcon);
> > +void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon);
> >   #endif /* __INTEL_DRV_H__ */
> > diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> > index 5013124..281127d 100644
> > --- a/drivers/gpu/drm/i915/intel_lspcon.c
> > +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> > @@ -35,16 +35,54 @@ static struct intel_dp *lspcon_to_intel_dp(struct intel_lspcon *lspcon)
> >   	return &dig_port->dp;
> >   }
> >   
> > +static const char *lspcon_mode_name(enum drm_lspcon_mode mode)
> > +{
> > +	switch (mode) {
> > +	case DRM_LSPCON_MODE_PCON:
> > +		return "PCON";
> > +	case DRM_LSPCON_MODE_LS:
> > +		return "LS";
> > +	case DRM_LSPCON_MODE_INVALID:
> > +		return "INVALID";
> > +	default:
> > +		MISSING_CASE(mode);
> > +		return "INVALID";
> > +	}
> > +}
> > +
> >   static enum drm_lspcon_mode lspcon_get_current_mode(struct intel_lspcon *lspcon)
> >   {
> > -	enum drm_lspcon_mode current_mode = DRM_LSPCON_MODE_INVALID;
> > +	enum drm_lspcon_mode current_mode;
> >   	struct i2c_adapter *adapter = &lspcon_to_intel_dp(lspcon)->aux.ddc;
> >   
> > -	if (drm_lspcon_get_mode(adapter, ¤t_mode))
> > +	if (drm_lspcon_get_mode(adapter, ¤t_mode)) {
> >   		DRM_ERROR("Error reading LSPCON mode\n");
> > -	else
> > -		DRM_DEBUG_KMS("Current LSPCON mode %s\n",
> > -			current_mode == DRM_LSPCON_MODE_PCON ? "PCON" : "LS");
> > +		return DRM_LSPCON_MODE_INVALID;
> > +	}
> > +	return current_mode;
> > +}
> > +
> > +static enum drm_lspcon_mode lspcon_wait_mode(struct intel_lspcon *lspcon,
> > +					     enum drm_lspcon_mode mode)
> > +{
> > +	enum drm_lspcon_mode current_mode;
> > +
> > +	current_mode = lspcon_get_current_mode(lspcon);
> > +	if (current_mode == mode || current_mode == DRM_LSPCON_MODE_INVALID)
> > +		goto out;
> > +
> > +	DRM_DEBUG_KMS("Waiting for LSPCON mode %s to settle\n",
> > +		      lspcon_mode_name(mode));
> > +
> Can we remove above lines, and start from below lines ? I guess wait_for 
> checks the condition first and then executes wait ?

Yes wait_for() works like that, but I wanted to have a debug message
about the fact that we had to wait.

> Also, a comment stating 100ms wait ?

Why? It's clear what's the timeout from the code itself.

> - Shashank
> > +	wait_for((current_mode = lspcon_get_current_mode(lspcon)) == mode ||
> > +		 current_mode == DRM_LSPCON_MODE_INVALID, 100);
> > +	if (current_mode != mode)
> > +		DRM_DEBUG_KMS("LSPCON mode hasn't settled\n");
> > +
> > +out:
> > +	DRM_DEBUG_KMS("Current LSPCON mode %s\n",
> > +		      lspcon_mode_name(current_mode));
> > +
> >   	return current_mode;
> >   }
> >   
> > @@ -97,8 +135,10 @@ static bool lspcon_probe(struct intel_lspcon *lspcon)
> >   {
> >   	enum drm_dp_dual_mode_type adaptor_type;
> >   	struct i2c_adapter *adapter = &lspcon_to_intel_dp(lspcon)->aux.ddc;
> > +	enum drm_lspcon_mode expected_mode;
> >   
> > -	lspcon_wake_native_aux_ch(lspcon);
> > +	expected_mode = lspcon_wake_native_aux_ch(lspcon) ?
> > +			DRM_LSPCON_MODE_PCON : DRM_LSPCON_MODE_LS;
> >   
> >   	/* Lets probe the adaptor and check its type */
> >   	adaptor_type = drm_dp_dual_mode_detect(adapter);
> > @@ -110,7 +150,7 @@ static bool lspcon_probe(struct intel_lspcon *lspcon)
> >   
> >   	/* Yay ... got a LSPCON device */
> >   	DRM_DEBUG_KMS("LSPCON detected\n");
> > -	lspcon->mode = lspcon_get_current_mode(lspcon);
> > +	lspcon->mode = lspcon_wait_mode(lspcon, expected_mode);
> >   	lspcon->active = true;
> >   	return true;
> >   }
> > @@ -150,8 +190,17 @@ static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon)
> >   
> >   void lspcon_resume(struct intel_lspcon *lspcon)
> >   {
> > -	if (lspcon_wake_native_aux_ch(lspcon))
> > +	enum drm_lspcon_mode expected_mode;
> > +
> > +	if (lspcon_wake_native_aux_ch(lspcon)) {
> > +		expected_mode = DRM_LSPCON_MODE_PCON;
> >   		lspcon_resume_in_pcon_wa(lspcon);
> > +	} else {
> > +		expected_mode = DRM_LSPCON_MODE_LS;
> > +	}
> > +
> > +	if (lspcon_wait_mode(lspcon, expected_mode) == DRM_LSPCON_MODE_PCON)
> > +		return;
> >   
> >   	if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON, true))
> >   		DRM_ERROR("LSPCON resume failed\n");
> > @@ -159,6 +208,11 @@ void lspcon_resume(struct intel_lspcon *lspcon)
> >   		DRM_DEBUG_KMS("LSPCON resume success\n");
> >   }
> >   
> > +void lspcon_wait_pcon_mode(struct intel_lspcon *lspcon)
> > +{
> > +	lspcon_wait_mode(lspcon, DRM_LSPCON_MODE_PCON);
> > +}
> > +
> >   bool lspcon_init(struct intel_digital_port *intel_dig_port)
> >   {
> >   	struct intel_dp *dp = &intel_dig_port->dp;
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx