[PATCH 0/4] drm/i915/lspcon: Fix ParadTech PS175 adaptor detection

[PATCH 0/4] drm/i915/lspcon: Fix ParadTech PS175 adaptor detection

[Imre Deak]

Patch 1 should fix the occasional detection problem seen on one
SKL-6770HQ machine in CI with a ParadTech PS175 LSPCON adaptor on board.
Patch 3 fixes a potential problem while changing the LSPCON mode or
starting link training while the mode is still settling as reported by
the adaptor.

Cc: Shashank Sharma <shashank.sharma@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>

Imre Deak (4):
  drm/i915/lspcon: Ensure AUX CH is awake while in DP Sleep state
  drm/i915/lspcon: Add dp_to_lspcon helper()
  drm/i915/lspcon: Wait for expected LSPCON mode to settle
  drm/i915/lspcon: Remove unused force change mode parameter

 drivers/gpu/drm/i915/intel_dp.c     | 12 +++--
 drivers/gpu/drm/i915/intel_drv.h    |  7 +++
 drivers/gpu/drm/i915/intel_lspcon.c | 94 ++++++++++++++++++++++++++++++++-----
 3 files changed, 98 insertions(+), 15 deletions(-)

-- 
2.5.0

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

[PATCH 1/4] drm/i915/lspcon: Ensure AUX CH is awake while in DP Sleep state

[Imre Deak]

Some LSPCON adaptors won't properly wake up in response to an AUX
request after the adaptor was placed to a DP Sink Sleep state (via
writing 0x2 to DP_SET_POWER). Based on the DP 1.4 specification 5.2.5,
the sink may place the AUX CH into a low-power state while in Sleep
state, but should wake it up in response to an AUX request within 1-20ms
(answering with AUX defers while waking it up). As opposed to this at
least the ParadTech PS175 adaptor won't fully wake in response to the
first I2C-over-AUX access and will occasionally ignore the offset in I2C
messages. This can result in accessing the DDC register at offset 0
regardless of the specified offset and the LSPCON detection failing.

To fix this do an initial dummy read from the DPCD area. The PS175 will
defer this access until it's fully woken (taking ~150ms) making sure the
following I2C-over-AUX accesses will work correctly.

Cc: Shashank Sharma <shashank.sharma@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Reference: https://bugs.freedesktop.org/show_bug.cgi?id=98353
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_lspcon.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
index daa5234..5013124 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -77,11 +77,29 @@ static int lspcon_change_mode(struct intel_lspcon *lspcon,
 	return 0;
 }
 
+static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon)
+{
+	uint8_t rev;
+
+	if (drm_dp_dpcd_readb(&lspcon_to_intel_dp(lspcon)->aux, DP_DPCD_REV,
+			      &rev) != 1) {
+		DRM_DEBUG_KMS("Native AUX CH down\n");
+		return false;
+	}
+
+	DRM_DEBUG_KMS("Native AUX CH up, DPCD version: %d.%d\n",
+		      rev >> 4, rev & 0xf);
+
+	return true;
+}
+
 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;
 
+	lspcon_wake_native_aux_ch(lspcon);
+
 	/* Lets probe the adaptor and check its type */
 	adaptor_type = drm_dp_dual_mode_detect(adapter);
 	if (adaptor_type != DRM_DP_DUAL_MODE_LSPCON) {
@@ -132,7 +150,8 @@ static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon)
 
 void lspcon_resume(struct intel_lspcon *lspcon)
 {
-	lspcon_resume_in_pcon_wa(lspcon);
+	if (lspcon_wake_native_aux_ch(lspcon))
+		lspcon_resume_in_pcon_wa(lspcon);
 
 	if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON, true))
 		DRM_ERROR("LSPCON resume failed\n");
-- 
2.5.0

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

Re: [PATCH 1/4] drm/i915/lspcon: Ensure AUX CH is awake while in DP Sleep state

[Sharma, Shashank]

Reviewed-by: Shashank Sharma

Regards
Shashank
On 11/22/2016 12:45 AM, Imre Deak wrote:
> Some LSPCON adaptors won't properly wake up in response to an AUX
> request after the adaptor was placed to a DP Sink Sleep state (via
> writing 0x2 to DP_SET_POWER). Based on the DP 1.4 specification 5.2.5,
> the sink may place the AUX CH into a low-power state while in Sleep
> state, but should wake it up in response to an AUX request within 1-20ms
> (answering with AUX defers while waking it up). As opposed to this at
> least the ParadTech PS175 adaptor won't fully wake in response to the
> first I2C-over-AUX access and will occasionally ignore the offset in I2C
> messages. This can result in accessing the DDC register at offset 0
> regardless of the specified offset and the LSPCON detection failing.
>
> To fix this do an initial dummy read from the DPCD area. The PS175 will
> defer this access until it's fully woken (taking ~150ms) making sure the
> following I2C-over-AUX accesses will work correctly.
>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=98353
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_lspcon.c | 21 ++++++++++++++++++++-
>   1 file changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> index daa5234..5013124 100644
> --- a/drivers/gpu/drm/i915/intel_lspcon.c
> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> @@ -77,11 +77,29 @@ static int lspcon_change_mode(struct intel_lspcon *lspcon,
>   	return 0;
>   }
>   
> +static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon)
> +{
> +	uint8_t rev;
> +
> +	if (drm_dp_dpcd_readb(&lspcon_to_intel_dp(lspcon)->aux, DP_DPCD_REV,
> +			      &rev) != 1) {
> +		DRM_DEBUG_KMS("Native AUX CH down\n");
> +		return false;
> +	}
> +
> +	DRM_DEBUG_KMS("Native AUX CH up, DPCD version: %d.%d\n",
> +		      rev >> 4, rev & 0xf);
> +
> +	return true;
> +}
> +
>   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;
>   
> +	lspcon_wake_native_aux_ch(lspcon);
> +
>   	/* Lets probe the adaptor and check its type */
>   	adaptor_type = drm_dp_dual_mode_detect(adapter);
>   	if (adaptor_type != DRM_DP_DUAL_MODE_LSPCON) {
> @@ -132,7 +150,8 @@ static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon)
>   
>   void lspcon_resume(struct intel_lspcon *lspcon)
>   {
> -	lspcon_resume_in_pcon_wa(lspcon);
> +	if (lspcon_wake_native_aux_ch(lspcon))
> +		lspcon_resume_in_pcon_wa(lspcon);
>   
>   	if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON, true))
>   		DRM_ERROR("LSPCON resume failed\n");

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

[PATCH 2/4] drm/i915/lspcon: Add dp_to_lspcon helper()

[Imre Deak]

We need to get to LSPCON in the next patch, so factor out the helper for
it. While at it also remove the redundant GEN9 check.

Cc: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 7 +++----
 drivers/gpu/drm/i915/intel_drv.h | 6 ++++++
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 90283ed..16c19d78 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4753,14 +4753,13 @@ static void intel_edp_panel_vdd_sanitize(struct intel_dp *intel_dp)
 void intel_dp_encoder_reset(struct drm_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->dev);
-	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
-	struct intel_lspcon *lspcon = &intel_dig_port->lspcon;
-	struct intel_dp *intel_dp = &intel_dig_port->dp;
+	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
+	struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
 
 	if (!HAS_DDI(dev_priv))
 		intel_dp->DP = I915_READ(intel_dp->output_reg);
 
-	if (IS_GEN9(dev_priv) && lspcon->active)
+	if (lspcon->active)
 		lspcon_resume(lspcon);
 
 	if (to_intel_encoder(encoder)->type != INTEL_OUTPUT_EDP)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index cd132c2..cf47e8a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1089,6 +1089,12 @@ dp_to_dig_port(struct intel_dp *intel_dp)
 	return container_of(intel_dp, struct intel_digital_port, dp);
 }
 
+static inline struct intel_lspcon *
+dp_to_lspcon(struct intel_dp *intel_dp)
+{
+	return &dp_to_dig_port(intel_dp)->lspcon;
+}
+
 static inline struct intel_digital_port *
 hdmi_to_dig_port(struct intel_hdmi *intel_hdmi)
 {
-- 
2.5.0

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

Re: [PATCH 2/4] drm/i915/lspcon: Add dp_to_lspcon helper()

[Sharma, Shashank]

Regards

Shashank


On 11/22/2016 12:45 AM, Imre Deak wrote:
> We need to get to LSPCON in the next patch, so factor out the helper for
> it. While at it also remove the redundant GEN9 check.
>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_dp.c  | 7 +++----
>   drivers/gpu/drm/i915/intel_drv.h | 6 ++++++
>   2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 90283ed..16c19d78 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4753,14 +4753,13 @@ static void intel_edp_panel_vdd_sanitize(struct intel_dp *intel_dp)
>   void intel_dp_encoder_reset(struct drm_encoder *encoder)
>   {
>   	struct drm_i915_private *dev_priv = to_i915(encoder->dev);
> -	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
> -	struct intel_lspcon *lspcon = &intel_dig_port->lspcon;
> -	struct intel_dp *intel_dp = &intel_dig_port->dp;
> +	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> +	struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
>   
>   	if (!HAS_DDI(dev_priv))
>   		intel_dp->DP = I915_READ(intel_dp->output_reg);
>   
> -	if (IS_GEN9(dev_priv) && lspcon->active)
> +	if (lspcon->active)
Shouldn't this be part of next patch itself ? Any reason why we have a 
separate patch for this ?

Apart from this,
Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>

Regards
Shashank
>   		lspcon_resume(lspcon);
>   
>   	if (to_intel_encoder(encoder)->type != INTEL_OUTPUT_EDP)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index cd132c2..cf47e8a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1089,6 +1089,12 @@ dp_to_dig_port(struct intel_dp *intel_dp)
>   	return container_of(intel_dp, struct intel_digital_port, dp);
>   }
>   
> +static inline struct intel_lspcon *
> +dp_to_lspcon(struct intel_dp *intel_dp)
> +{
> +	return &dp_to_dig_port(intel_dp)->lspcon;
> +}
> +
>   static inline struct intel_digital_port *
>   hdmi_to_dig_port(struct intel_hdmi *intel_hdmi)
>   {

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

Re: [PATCH 2/4] drm/i915/lspcon: Add dp_to_lspcon helper()

[Imre Deak]

On ti, 2016-11-22 at 15:49 +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 11/22/2016 12:45 AM, Imre Deak wrote:
> > We need to get to LSPCON in the next patch, so factor out the helper for
> > it. While at it also remove the redundant GEN9 check.
> > 
> > Cc: Shashank Sharma <shashank.sharma@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_dp.c  | 7 +++----
> >   drivers/gpu/drm/i915/intel_drv.h | 6 ++++++
> >   2 files changed, 9 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 90283ed..16c19d78 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4753,14 +4753,13 @@ static void intel_edp_panel_vdd_sanitize(struct intel_dp *intel_dp)
> >   void intel_dp_encoder_reset(struct drm_encoder *encoder)
> >   {
> >   	struct drm_i915_private *dev_priv = to_i915(encoder->dev);
> > -	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
> > -	struct intel_lspcon *lspcon = &intel_dig_port->lspcon;
> > -	struct intel_dp *intel_dp = &intel_dig_port->dp;
> > +	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > +	struct intel_lspcon *lspcon = dp_to_lspcon(intel_dp);
> >   
> >   	if (!HAS_DDI(dev_priv))
> >   		intel_dp->DP = I915_READ(intel_dp->output_reg);
> >   
> > -	if (IS_GEN9(dev_priv) && lspcon->active)
> > +	if (lspcon->active)
> Shouldn't this be part of next patch itself ? Any reason why we have a 
> separate patch for this ?

Having separate patches for cleanups without side effects makes the
review and bisection easier.

> Apart from this,
> Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>
> 
> Regards
> Shashank
> >   		lspcon_resume(lspcon);
> >   
> >   	if (to_intel_encoder(encoder)->type != INTEL_OUTPUT_EDP)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index cd132c2..cf47e8a 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1089,6 +1089,12 @@ dp_to_dig_port(struct intel_dp *intel_dp)
> >   	return container_of(intel_dp, struct intel_digital_port, dp);
> >   }
> >   
> > +static inline struct intel_lspcon *
> > +dp_to_lspcon(struct intel_dp *intel_dp)
> > +{
> > +	return &dp_to_dig_port(intel_dp)->lspcon;
> > +}
> > +
> >   static inline struct intel_digital_port *
> >   hdmi_to_dig_port(struct intel_hdmi *intel_hdmi)
> >   {
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Imre Deak]

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, &current_mode))
+	if (drm_lspcon_get_mode(adapter, &current_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));
+
+	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;
-- 
2.5.0

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

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

[Sharma, Shashank]

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, &current_mode))
> +	if (drm_lspcon_get_mode(adapter, &current_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 ? Also, a comment stating 100ms wait ?

- 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

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

[Imre Deak]

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

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

[Sharma, Shashank]

R-B: Shashank Sharma <shashank.sharma@intel.com>

Regards
Shashank
-----Original Message-----
From: Deak, Imre 
Sent: Tuesday, November 22, 2016 9:47 PM
To: Sharma, Shashank <shashank.sharma@intel.com>; intel-gfx@lists.freedesktop.org
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>; Nikula, Jani <jani.nikula@intel.com>
Subject: Re: [PATCH 3/4] drm/i915/lspcon: Wait for expected LSPCON mode to settle

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

[PATCH 4/4] drm/i915/lspcon: Remove unused force change mode parameter

[Imre Deak]

All callers asked for a forced change but the function ignored this
parameter. It doesn't seem to be necessary to force the change in any
case so let's just remove the parameter.

Cc: Shashank Sharma <shashank.sharma@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_lspcon.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
index 281127d..f6d4e69 100644
--- a/drivers/gpu/drm/i915/intel_lspcon.c
+++ b/drivers/gpu/drm/i915/intel_lspcon.c
@@ -87,7 +87,7 @@ static enum drm_lspcon_mode lspcon_wait_mode(struct intel_lspcon *lspcon,
 }
 
 static int lspcon_change_mode(struct intel_lspcon *lspcon,
-	enum drm_lspcon_mode mode, bool force)
+			      enum drm_lspcon_mode mode)
 {
 	int err;
 	enum drm_lspcon_mode current_mode;
@@ -202,7 +202,7 @@ void lspcon_resume(struct intel_lspcon *lspcon)
 	if (lspcon_wait_mode(lspcon, expected_mode) == DRM_LSPCON_MODE_PCON)
 		return;
 
-	if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON, true))
+	if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON))
 		DRM_ERROR("LSPCON resume failed\n");
 	else
 		DRM_DEBUG_KMS("LSPCON resume success\n");
@@ -239,8 +239,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
 	* 2.0 sinks.
 	*/
 	if (lspcon->active && lspcon->mode != DRM_LSPCON_MODE_PCON) {
-		if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON,
-			true) < 0) {
+		if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON) < 0) {
 			DRM_ERROR("LSPCON mode change to PCON failed\n");
 			return false;
 		}
-- 
2.5.0

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

Re: [PATCH 4/4] drm/i915/lspcon: Remove unused force change mode parameter

[Sharma, Shashank]

The reason why I kept the force function was, that during debug phase, I 
saw a need to change LSPCON mode.

Also, in future, if we need to provide an IOCTL/control to userspace, to 
change lspcon mode, force may be required

to differentiate the path from user/kernel space.

But in any case: Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>


Regards

Shashank

On 11/22/2016 12:45 AM, Imre Deak wrote:
> All callers asked for a forced change but the function ignored this
> parameter. It doesn't seem to be necessary to force the change in any
> case so let's just remove the parameter.
>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_lspcon.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> index 281127d..f6d4e69 100644
> --- a/drivers/gpu/drm/i915/intel_lspcon.c
> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> @@ -87,7 +87,7 @@ static enum drm_lspcon_mode lspcon_wait_mode(struct intel_lspcon *lspcon,
>   }
>   
>   static int lspcon_change_mode(struct intel_lspcon *lspcon,
> -	enum drm_lspcon_mode mode, bool force)
> +			      enum drm_lspcon_mode mode)
>   {
>   	int err;
>   	enum drm_lspcon_mode current_mode;
> @@ -202,7 +202,7 @@ void lspcon_resume(struct intel_lspcon *lspcon)
>   	if (lspcon_wait_mode(lspcon, expected_mode) == DRM_LSPCON_MODE_PCON)
>   		return;
>   
> -	if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON, true))
> +	if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON))
>   		DRM_ERROR("LSPCON resume failed\n");
>   	else
>   		DRM_DEBUG_KMS("LSPCON resume success\n");
> @@ -239,8 +239,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>   	* 2.0 sinks.
>   	*/
>   	if (lspcon->active && lspcon->mode != DRM_LSPCON_MODE_PCON) {
> -		if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON,
> -			true) < 0) {
> +		if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON) < 0) {
>   			DRM_ERROR("LSPCON mode change to PCON failed\n");
>   			return false;
>   		}

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

Re: [PATCH 4/4] drm/i915/lspcon: Remove unused force change mode parameter

[Imre Deak]

On Tue, 2016-11-22 at 21:09 +0530, Sharma, Shashank wrote:
> The reason why I kept the force function was, that during debug phase, I 
> saw a need to change LSPCON mode.
> 
> Also, in future, if we need to provide an IOCTL/control to userspace, to 
> change lspcon mode, force may be required to differentiate the path from
> user/kernel space.

Not sure how that would work, but it's easy to add the parameter when
it's actually implemented. Until then it only makes reading the code
more difficult.

> 
> But in any case: Reviewed-by: Shashank Sharma 
> 
> 
> Regards
> 
> Shashank
> 
> On 11/22/2016 12:45 AM, Imre Deak wrote:
> > All callers asked for a forced change but the function ignored this
> > parameter. It doesn't seem to be necessary to force the change in any
> > case so let's just remove the parameter.
> > 
> > Cc: Shashank Sharma <shashank.sharma@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_lspcon.c | 7 +++----
> >   1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> > index 281127d..f6d4e69 100644
> > --- a/drivers/gpu/drm/i915/intel_lspcon.c
> > +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> > @@ -87,7 +87,7 @@ static enum drm_lspcon_mode lspcon_wait_mode(struct intel_lspcon *lspcon,
> >   }
> >   
> >   static int lspcon_change_mode(struct intel_lspcon *lspcon,
> > -	enum drm_lspcon_mode mode, bool force)
> > +			      enum drm_lspcon_mode mode)
> >   {
> >   	int err;
> >   	enum drm_lspcon_mode current_mode;
> > @@ -202,7 +202,7 @@ void lspcon_resume(struct intel_lspcon *lspcon)
> >   	if (lspcon_wait_mode(lspcon, expected_mode) == DRM_LSPCON_MODE_PCON)
> >   		return;
> >   
> > -	if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON, true))
> > +	if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON))
> >   		DRM_ERROR("LSPCON resume failed\n");
> >   	else
> >   		DRM_DEBUG_KMS("LSPCON resume success\n");
> > @@ -239,8 +239,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
> >   	* 2.0 sinks.
> >   	*/
> >   	if (lspcon->active && lspcon->mode != DRM_LSPCON_MODE_PCON) {
> > -		if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON,
> > -			true) < 0) {
> > +		if (lspcon_change_mode(lspcon, DRM_LSPCON_MODE_PCON) < 0) {
> >   			DRM_ERROR("LSPCON mode change to PCON failed\n");
> >   			return false;
> >   		}
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915/lspcon: Fix ParadTech PS175 adaptor detection

[Patchwork]

== Series Details ==

Series: drm/i915/lspcon: Fix ParadTech PS175 adaptor detection
URL   : https://patchwork.freedesktop.org/series/15682/
State : success

== Summary ==

Series 15682v1 drm/i915/lspcon: Fix ParadTech PS175 adaptor detection
https://patchwork.freedesktop.org/api/1.0/series/15682/revisions/1/mbox/

Test drv_module_reload_basic:
                dmesg-warn -> PASS       (fi-skl-6770hq)

fi-bdw-5557u     total:244  pass:229  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:244  pass:204  dwarn:0   dfail:0   fail:0   skip:40 
fi-bxt-t5700     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-ilk-650       total:244  pass:191  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-7200u     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:244  pass:223  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:244  pass:222  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-snb-2600      total:244  pass:211  dwarn:0   dfail:0   fail:0   skip:33 

eeec5e7742b23082dd20523c8baa08fe495175e4 drm-intel-nightly: 2016y-11m-21d-18h-22m-22s UTC integration manifest
e951aca3 drm/i915/lspcon: Remove unused force change mode parameter
e4f336b drm/i915/lspcon: Wait for expected LSPCON mode to settle
c4e2bb7 drm/i915/lspcon: Add dp_to_lspcon helper()
baa8989 drm/i915/lspcon: Ensure AUX CH is awake while in DP Sleep state

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3073/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✓ Fi.CI.BAT: success for drm/i915/lspcon: Fix ParadTech PS175 adaptor detection

[Imre Deak]

On ma, 2016-11-21 at 19:45 +0000, Patchwork wrote:
> == Series Details ==
> 
> Series: drm/i915/lspcon: Fix ParadTech PS175 adaptor detection
> URL   : https://patchwork.freedesktop.org/series/15682/
> State : success
> 
> == Summary ==
> 
> Series 15682v1 drm/i915/lspcon: Fix ParadTech PS175 adaptor detection
> https://patchwork.freedesktop.org/api/1.0/series/15682/revisions/1/mb
> ox/
> 
> Test drv_module_reload_basic:
>                 dmesg-warn -> PASS       (fi-skl-6770hq)

I pushed the patchset to -dinq, thanks for the review.

--Imre

> fi-bdw-
> 5557u     total:244  pass:229  dwarn:0   dfail:0   fail:0   skip:15 
> fi-bsw-
> n3050     total:244  pass:204  dwarn:0   dfail:0   fail:0   skip:40 
> fi-bxt-
> t5700     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
> fi-byt-
> j1900     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
> fi-byt-
> n2820     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
> fi-hsw-
> 4770      total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
> fi-hsw-
> 4770r     total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
> fi-ilk-
> 650       total:244  pass:191  dwarn:0   dfail:0   fail:0   skip:53 
> fi-ivb-
> 3520m     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
> fi-ivb-
> 3770      total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
> fi-kbl-
> 7200u     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
> fi-skl-
> 6260u     total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
> fi-skl-
> 6700hq    total:244  pass:223  dwarn:0   dfail:0   fail:0   skip:21 
> fi-skl-
> 6700k     total:244  pass:222  dwarn:1   dfail:0   fail:0   skip:21 
> fi-skl-
> 6770hq    total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
> fi-snb-
> 2520m     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
> fi-snb-
> 2600      total:244  pass:211  dwarn:0   dfail:0   fail:0   skip:33 
> 
> eeec5e7742b23082dd20523c8baa08fe495175e4 drm-intel-nightly: 2016y-
> 11m-21d-18h-22m-22s UTC integration manifest
> e951aca3 drm/i915/lspcon: Remove unused force change mode parameter
> e4f336b drm/i915/lspcon: Wait for expected LSPCON mode to settle
> c4e2bb7 drm/i915/lspcon: Add dp_to_lspcon helper()
> baa8989 drm/i915/lspcon: Ensure AUX CH is awake while in DP Sleep
> state
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3073/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx