diff mbox series

[1/4] drm/i915/display/hsw+: Do not program the same vswing entry twice

Message ID 20200528200356.36756-1-jose.souza@intel.com (mailing list archive)
State New, archived
Headers show
Series [1/4] drm/i915/display/hsw+: Do not program the same vswing entry twice | expand

Commit Message

Souza, Jose May 28, 2020, 8:03 p.m. UTC 
It will be programed right before the link training, so no need to do
it twice.
It will not strictly follow BSpec sequences but most of this sequences
are not matching anyways.

Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

Comments

Ville Syrjala May 29, 2020, 6:51 a.m. UTC | #1 
On Thu, May 28, 2020 at 01:03:53PM -0700, José Roberto de Souza wrote:
> It will be programed right before the link training, so no need to do
> it twice.
> It will not strictly follow BSpec sequences but most of this sequences
> are not matching anyways.
> 
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 19 ++++---------------
>  1 file changed, 4 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index aa22465bb56e..c100efc6a2c4 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -3115,7 +3115,6 @@ static void tgl_ddi_pre_enable_dp(struct intel_atomic_state *state,
>  	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
>  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>  	bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
> -	int level = intel_ddi_dp_level(intel_dp);
>  	enum transcoder transcoder = crtc_state->cpu_transcoder;
>  
>  	intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
> @@ -3190,9 +3189,10 @@ static void tgl_ddi_pre_enable_dp(struct intel_atomic_state *state,
>  	 * down this function.
>  	 */
>  
> -	/* 7.e Configure voltage swing and related IO settings */
> -	tgl_ddi_vswing_sequence(encoder, crtc_state->port_clock, level,
> -				encoder->type);
> +	/*
> +	 * 7.e Configure voltage swing and related IO settings
> +	 * It will be done in intel_dp_start_link_train(), no need to do twice
> +	 */

Hmm. Do we still set it up before turning on the port?

>  
>  	/*
>  	 * 7.f Combo PHY: Configure PORT_CL_DW10 Static Power Down to power up
> @@ -3257,7 +3257,6 @@ static void hsw_ddi_pre_enable_dp(struct intel_atomic_state *state,
>  	enum phy phy = intel_port_to_phy(dev_priv, port);
>  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
>  	bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
> -	int level = intel_ddi_dp_level(intel_dp);
>  
>  	if (INTEL_GEN(dev_priv) < 11)
>  		drm_WARN_ON(&dev_priv->drm,
> @@ -3279,16 +3278,6 @@ static void hsw_ddi_pre_enable_dp(struct intel_atomic_state *state,
>  
>  	icl_program_mg_dp_mode(dig_port, crtc_state);
>  
> -	if (INTEL_GEN(dev_priv) >= 11)
> -		icl_ddi_vswing_sequence(encoder, crtc_state->port_clock,
> -					level, encoder->type);
> -	else if (IS_CANNONLAKE(dev_priv))
> -		cnl_ddi_vswing_sequence(encoder, level, encoder->type);
> -	else if (IS_GEN9_LP(dev_priv))
> -		bxt_ddi_vswing_sequence(encoder, level, encoder->type);
> -	else
> -		intel_prepare_dp_ddi_buffers(encoder, crtc_state);

This last one definitely has to stay IIRC. HSW/BDW/SKL buf trans
stuff works quite bit differently than the BXT+ style more manual
programming.

> -
>  	if (intel_phy_is_combo(dev_priv, phy)) {
>  		bool lane_reversal =
>  			dig_port->saved_port_bits & DDI_BUF_PORT_REVERSAL;
> -- 
> 2.26.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Souza, Jose May 29, 2020, 8:52 p.m. UTC | #2 
On Fri, 2020-05-29 at 09:51 +0300, Ville Syrjälä wrote:
> On Thu, May 28, 2020 at 01:03:53PM -0700, José Roberto de Souza wrote:
> > It will be programed right before the link training, so no need to do
> > it twice.
> > It will not strictly follow BSpec sequences but most of this sequences
> > are not matching anyways.
> > 
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_ddi.c | 19 ++++---------------
> >  1 file changed, 4 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > index aa22465bb56e..c100efc6a2c4 100644
> > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > @@ -3115,7 +3115,6 @@ static void tgl_ddi_pre_enable_dp(struct intel_atomic_state *state,
> >  	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
> >  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> >  	bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
> > -	int level = intel_ddi_dp_level(intel_dp);
> >  	enum transcoder transcoder = crtc_state->cpu_transcoder;
> >  
> >  	intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
> > @@ -3190,9 +3189,10 @@ static void tgl_ddi_pre_enable_dp(struct intel_atomic_state *state,
> >  	 * down this function.
> >  	 */
> >  
> > -	/* 7.e Configure voltage swing and related IO settings */
> > -	tgl_ddi_vswing_sequence(encoder, crtc_state->port_clock, level,
> > -				encoder->type);
> > +	/*
> > +	 * 7.e Configure voltage swing and related IO settings
> > +	 * It will be done in intel_dp_start_link_train(), no need to do twice
> > +	 */
> 
> Hmm. Do we still set it up before turning on the port?

No.

intel_dp_start_link_train()
	intel_dp_link_training_clock_recovery()
		intel_dp->prepare_link_retrain(intel_dp)/intel_ddi_prepare_link_retrain();/* Port is enabled here in training mode */

		....

		intel_dp_reset_link_train()
			intel_dp_set_signal_levels() /* Vswing table is set here */
Guess is safer keep programming it twice then?


> 
> >  
> >  	/*
> >  	 * 7.f Combo PHY: Configure PORT_CL_DW10 Static Power Down to power up
> > @@ -3257,7 +3257,6 @@ static void hsw_ddi_pre_enable_dp(struct intel_atomic_state *state,
> >  	enum phy phy = intel_port_to_phy(dev_priv, port);
> >  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> >  	bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
> > -	int level = intel_ddi_dp_level(intel_dp);
> >  
> >  	if (INTEL_GEN(dev_priv) < 11)
> >  		drm_WARN_ON(&dev_priv->drm,
> > @@ -3279,16 +3278,6 @@ static void hsw_ddi_pre_enable_dp(struct intel_atomic_state *state,
> >  
> >  	icl_program_mg_dp_mode(dig_port, crtc_state);
> >  
> > -	if (INTEL_GEN(dev_priv) >= 11)
> > -		icl_ddi_vswing_sequence(encoder, crtc_state->port_clock,
> > -					level, encoder->type);
> > -	else if (IS_CANNONLAKE(dev_priv))
> > -		cnl_ddi_vswing_sequence(encoder, level, encoder->type);
> > -	else if (IS_GEN9_LP(dev_priv))
> > -		bxt_ddi_vswing_sequence(encoder, level, encoder->type);
> > -	else
> > -		intel_prepare_dp_ddi_buffers(encoder, crtc_state);
> 
> This last one definitely has to stay IIRC. HSW/BDW/SKL buf trans
> stuff works quite bit differently than the BXT+ style more manual
> programming.
> 
> > -
> >  	if (intel_phy_is_combo(dev_priv, phy)) {
> >  		bool lane_reversal =
> >  			dig_port->saved_port_bits & DDI_BUF_PORT_REVERSAL;
> > -- 
> > 2.26.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjala June 3, 2020, 12:42 p.m. UTC | #3 
On Fri, May 29, 2020 at 08:52:38PM +0000, Souza, Jose wrote:
> On Fri, 2020-05-29 at 09:51 +0300, Ville Syrjälä wrote:
> > On Thu, May 28, 2020 at 01:03:53PM -0700, José Roberto de Souza wrote:
> > > It will be programed right before the link training, so no need to do
> > > it twice.
> > > It will not strictly follow BSpec sequences but most of this sequences
> > > are not matching anyways.
> > > 
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_ddi.c | 19 ++++---------------
> > >  1 file changed, 4 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > index aa22465bb56e..c100efc6a2c4 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> > > @@ -3115,7 +3115,6 @@ static void tgl_ddi_pre_enable_dp(struct intel_atomic_state *state,
> > >  	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
> > >  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> > >  	bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
> > > -	int level = intel_ddi_dp_level(intel_dp);
> > >  	enum transcoder transcoder = crtc_state->cpu_transcoder;
> > >  
> > >  	intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
> > > @@ -3190,9 +3189,10 @@ static void tgl_ddi_pre_enable_dp(struct intel_atomic_state *state,
> > >  	 * down this function.
> > >  	 */
> > >  
> > > -	/* 7.e Configure voltage swing and related IO settings */
> > > -	tgl_ddi_vswing_sequence(encoder, crtc_state->port_clock, level,
> > > -				encoder->type);
> > > +	/*
> > > +	 * 7.e Configure voltage swing and related IO settings
> > > +	 * It will be done in intel_dp_start_link_train(), no need to do twice
> > > +	 */
> > 
> > Hmm. Do we still set it up before turning on the port?
> 
> No.
> 
> intel_dp_start_link_train()
> 	intel_dp_link_training_clock_recovery()
> 		intel_dp->prepare_link_retrain(intel_dp)/intel_ddi_prepare_link_retrain();/* Port is enabled here in training mode */
> 
> 		....
> 
> 		intel_dp_reset_link_train()
> 			intel_dp_set_signal_levels() /* Vswing table is set here */
> Guess is safer keep programming it twice then?

Probably.

> 
> 
> > 
> > >  
> > >  	/*
> > >  	 * 7.f Combo PHY: Configure PORT_CL_DW10 Static Power Down to power up
> > > @@ -3257,7 +3257,6 @@ static void hsw_ddi_pre_enable_dp(struct intel_atomic_state *state,
> > >  	enum phy phy = intel_port_to_phy(dev_priv, port);
> > >  	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
> > >  	bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
> > > -	int level = intel_ddi_dp_level(intel_dp);
> > >  
> > >  	if (INTEL_GEN(dev_priv) < 11)
> > >  		drm_WARN_ON(&dev_priv->drm,
> > > @@ -3279,16 +3278,6 @@ static void hsw_ddi_pre_enable_dp(struct intel_atomic_state *state,
> > >  
> > >  	icl_program_mg_dp_mode(dig_port, crtc_state);
> > >  
> > > -	if (INTEL_GEN(dev_priv) >= 11)
> > > -		icl_ddi_vswing_sequence(encoder, crtc_state->port_clock,
> > > -					level, encoder->type);
> > > -	else if (IS_CANNONLAKE(dev_priv))
> > > -		cnl_ddi_vswing_sequence(encoder, level, encoder->type);
> > > -	else if (IS_GEN9_LP(dev_priv))
> > > -		bxt_ddi_vswing_sequence(encoder, level, encoder->type);
> > > -	else
> > > -		intel_prepare_dp_ddi_buffers(encoder, crtc_state);
> > 
> > This last one definitely has to stay IIRC. HSW/BDW/SKL buf trans
> > stuff works quite bit differently than the BXT+ style more manual
> > programming.
> > 
> > > -
> > >  	if (intel_phy_is_combo(dev_priv, phy)) {
> > >  		bool lane_reversal =
> > >  			dig_port->saved_port_bits & DDI_BUF_PORT_REVERSAL;
> > > -- 
> > > 2.26.2
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index aa22465bb56e..c100efc6a2c4 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -3115,7 +3115,6 @@  static void tgl_ddi_pre_enable_dp(struct intel_atomic_state *state,
 	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
 	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
 	bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
-	int level = intel_ddi_dp_level(intel_dp);
 	enum transcoder transcoder = crtc_state->cpu_transcoder;
 
 	intel_dp_set_link_params(intel_dp, crtc_state->port_clock,
@@ -3190,9 +3189,10 @@  static void tgl_ddi_pre_enable_dp(struct intel_atomic_state *state,
 	 * down this function.
 	 */
 
-	/* 7.e Configure voltage swing and related IO settings */
-	tgl_ddi_vswing_sequence(encoder, crtc_state->port_clock, level,
-				encoder->type);
+	/*
+	 * 7.e Configure voltage swing and related IO settings
+	 * It will be done in intel_dp_start_link_train(), no need to do twice
+	 */
 
 	/*
 	 * 7.f Combo PHY: Configure PORT_CL_DW10 Static Power Down to power up
@@ -3257,7 +3257,6 @@  static void hsw_ddi_pre_enable_dp(struct intel_atomic_state *state,
 	enum phy phy = intel_port_to_phy(dev_priv, port);
 	struct intel_digital_port *dig_port = enc_to_dig_port(encoder);
 	bool is_mst = intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST);
-	int level = intel_ddi_dp_level(intel_dp);
 
 	if (INTEL_GEN(dev_priv) < 11)
 		drm_WARN_ON(&dev_priv->drm,
@@ -3279,16 +3278,6 @@  static void hsw_ddi_pre_enable_dp(struct intel_atomic_state *state,
 
 	icl_program_mg_dp_mode(dig_port, crtc_state);
 
-	if (INTEL_GEN(dev_priv) >= 11)
-		icl_ddi_vswing_sequence(encoder, crtc_state->port_clock,
-					level, encoder->type);
-	else if (IS_CANNONLAKE(dev_priv))
-		cnl_ddi_vswing_sequence(encoder, level, encoder->type);
-	else if (IS_GEN9_LP(dev_priv))
-		bxt_ddi_vswing_sequence(encoder, level, encoder->type);
-	else
-		intel_prepare_dp_ddi_buffers(encoder, crtc_state);
-
 	if (intel_phy_is_combo(dev_priv, phy)) {
 		bool lane_reversal =
 			dig_port->saved_port_bits & DDI_BUF_PORT_REVERSAL;