diff mbox series

[4/7] drm/i915: Factor out a helper to disable the DPCD training pattern

Message ID 20200922125106.30540-5-imre.deak@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Add support for LTTPR non-transparent link training mode | expand

Commit Message

Imre Deak Sept. 22, 2020, 12:51 p.m. UTC 
To prepare for a follow-up LTTPR change factor out a helper to disable
the training pattern in DPCD. We'll need to do this for each LTTPR
(without programming the port to output the idle pattern) when training
in LTTPR non-transparent mode.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 .../drm/i915/display/intel_dp_link_training.c | 28 +++++++++++--------
 1 file changed, 16 insertions(+), 12 deletions(-)

Comments

Ville Syrjälä Sept. 22, 2020, 4:54 p.m. UTC | #1 
On Tue, Sep 22, 2020 at 03:51:03PM +0300, Imre Deak wrote:
> To prepare for a follow-up LTTPR change factor out a helper to disable
> the training pattern in DPCD. We'll need to do this for each LTTPR
> (without programming the port to output the idle pattern) when training
> in LTTPR non-transparent mode.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  .../drm/i915/display/intel_dp_link_training.c | 28 +++++++++++--------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> index 0c3809891bd2..6994a32244dc 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> @@ -102,30 +102,34 @@ void intel_dp_get_adjust_train(struct intel_dp *intel_dp,
>  		intel_dp->train_set[lane] = v | p;
>  }
>  
> +static bool intel_dp_disable_dpcd_training_pattern(struct intel_dp *intel_dp)
> +{
> +	u8 val = DP_TRAINING_PATTERN_DISABLE;
> +
> +	return drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_PATTERN_SET, &val, 1) == 1;
> +}


> +
>  static bool
>  intel_dp_set_link_train(struct intel_dp *intel_dp,
>  			u8 dp_train_pat)
>  {
>  	u8 buf[sizeof(intel_dp->train_set) + 1];
> -	int ret, len;
> +	int len;
>  
>  	intel_dp_program_link_training_pattern(intel_dp, dp_train_pat);
>  
> -	buf[0] = dp_train_pat;
>  	if ((dp_train_pat & ~DP_LINK_SCRAMBLING_DISABLE) ==
> -	    DP_TRAINING_PATTERN_DISABLE) {
> +	    DP_TRAINING_PATTERN_DISABLE)
>  		/* don't write DP_TRAINING_LANEx_SET on disable */

As mentioned in the other patch I think we're doing things in the wrong
order here. I suspect it'll be cleaner to just stop doing
intel_dp_set_link_train(DISABLE) entirely and just have a dedicated
function for disabling link training. We can then trivially do a
followup to swap the order of operations to match the spec.

> -		len = 1;
> -	} else {
> -		/* DP_TRAINING_LANEx_SET follow DP_TRAINING_PATTERN_SET */
> -		memcpy(buf + 1, intel_dp->train_set, intel_dp->lane_count);
> -		len = intel_dp->lane_count + 1;
> -	}
> +		return intel_dp_disable_dpcd_training_pattern(intel_dp);
>  
> -	ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_PATTERN_SET,
> -				buf, len);
> +	buf[0] = dp_train_pat;
> +	/* DP_TRAINING_LANEx_SET follow DP_TRAINING_PATTERN_SET */
> +	memcpy(buf + 1, intel_dp->train_set, intel_dp->lane_count);
> +	len = intel_dp->lane_count + 1;
>  
> -	return ret == len;
> +	return drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_PATTERN_SET,
> +				 buf, len) == len;
>  }
>  
>  static bool
> -- 
> 2.17.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Imre Deak Sept. 22, 2020, 5:41 p.m. UTC | #2 
On Tue, Sep 22, 2020 at 07:54:20PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 22, 2020 at 03:51:03PM +0300, Imre Deak wrote:
> > To prepare for a follow-up LTTPR change factor out a helper to disable
> > the training pattern in DPCD. We'll need to do this for each LTTPR
> > (without programming the port to output the idle pattern) when training
> > in LTTPR non-transparent mode.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  .../drm/i915/display/intel_dp_link_training.c | 28 +++++++++++--------
> >  1 file changed, 16 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > index 0c3809891bd2..6994a32244dc 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > @@ -102,30 +102,34 @@ void intel_dp_get_adjust_train(struct intel_dp *intel_dp,
> >  		intel_dp->train_set[lane] = v | p;
> >  }
> >  
> > +static bool intel_dp_disable_dpcd_training_pattern(struct intel_dp *intel_dp)
> > +{
> > +	u8 val = DP_TRAINING_PATTERN_DISABLE;
> > +
> > +	return drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_PATTERN_SET, &val, 1) == 1;
> > +}
> 
> 
> > +
> >  static bool
> >  intel_dp_set_link_train(struct intel_dp *intel_dp,
> >  			u8 dp_train_pat)
> >  {
> >  	u8 buf[sizeof(intel_dp->train_set) + 1];
> > -	int ret, len;
> > +	int len;
> >  
> >  	intel_dp_program_link_training_pattern(intel_dp, dp_train_pat);
> >  
> > -	buf[0] = dp_train_pat;
> >  	if ((dp_train_pat & ~DP_LINK_SCRAMBLING_DISABLE) ==
> > -	    DP_TRAINING_PATTERN_DISABLE) {
> > +	    DP_TRAINING_PATTERN_DISABLE)
> >  		/* don't write DP_TRAINING_LANEx_SET on disable */
> 
> As mentioned in the other patch I think we're doing things in the wrong
> order here. I suspect it'll be cleaner to just stop doing
> intel_dp_set_link_train(DISABLE) entirely and just have a dedicated
> function for disabling link training. We can then trivially do a
> followup to swap the order of operations to match the spec.

intel_dp_disable_dpcd_training_pattern() would be needed after each
LTTPR link training phase, where the port should not output idle
patterns, that's the only reason for this change.

Do you mean to remove intel_dp_stop_link_train() then and do the idle
pattern programming + corresponding DPCD training pattern disable
programming at the end of the link training sequence (and remove the
DP_TRAINING_PATTERN_DISABLE case handling from above)? I agree with
that, but I see that too as a follow-up material (along with changing
the order as you suggested).


> 
> > -		len = 1;
> > -	} else {
> > -		/* DP_TRAINING_LANEx_SET follow DP_TRAINING_PATTERN_SET */
> > -		memcpy(buf + 1, intel_dp->train_set, intel_dp->lane_count);
> > -		len = intel_dp->lane_count + 1;
> > -	}
> > +		return intel_dp_disable_dpcd_training_pattern(intel_dp);
> >  
> > -	ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_PATTERN_SET,
> > -				buf, len);
> > +	buf[0] = dp_train_pat;
> > +	/* DP_TRAINING_LANEx_SET follow DP_TRAINING_PATTERN_SET */
> > +	memcpy(buf + 1, intel_dp->train_set, intel_dp->lane_count);
> > +	len = intel_dp->lane_count + 1;
> >  
> > -	return ret == len;
> > +	return drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_PATTERN_SET,
> > +				 buf, len) == len;
> >  }
> >  
> >  static bool
> > -- 
> > 2.17.1
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel
Ville Syrjälä Sept. 22, 2020, 5:47 p.m. UTC | #3 
On Tue, Sep 22, 2020 at 08:41:28PM +0300, Imre Deak wrote:
> On Tue, Sep 22, 2020 at 07:54:20PM +0300, Ville Syrjälä wrote:
> > On Tue, Sep 22, 2020 at 03:51:03PM +0300, Imre Deak wrote:
> > > To prepare for a follow-up LTTPR change factor out a helper to disable
> > > the training pattern in DPCD. We'll need to do this for each LTTPR
> > > (without programming the port to output the idle pattern) when training
> > > in LTTPR non-transparent mode.
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  .../drm/i915/display/intel_dp_link_training.c | 28 +++++++++++--------
> > >  1 file changed, 16 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > > index 0c3809891bd2..6994a32244dc 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > > @@ -102,30 +102,34 @@ void intel_dp_get_adjust_train(struct intel_dp *intel_dp,
> > >  		intel_dp->train_set[lane] = v | p;
> > >  }
> > >  
> > > +static bool intel_dp_disable_dpcd_training_pattern(struct intel_dp *intel_dp)
> > > +{
> > > +	u8 val = DP_TRAINING_PATTERN_DISABLE;
> > > +
> > > +	return drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_PATTERN_SET, &val, 1) == 1;
> > > +}
> > 
> > 
> > > +
> > >  static bool
> > >  intel_dp_set_link_train(struct intel_dp *intel_dp,
> > >  			u8 dp_train_pat)
> > >  {
> > >  	u8 buf[sizeof(intel_dp->train_set) + 1];
> > > -	int ret, len;
> > > +	int len;
> > >  
> > >  	intel_dp_program_link_training_pattern(intel_dp, dp_train_pat);
> > >  
> > > -	buf[0] = dp_train_pat;
> > >  	if ((dp_train_pat & ~DP_LINK_SCRAMBLING_DISABLE) ==
> > > -	    DP_TRAINING_PATTERN_DISABLE) {
> > > +	    DP_TRAINING_PATTERN_DISABLE)
> > >  		/* don't write DP_TRAINING_LANEx_SET on disable */
> > 
> > As mentioned in the other patch I think we're doing things in the wrong
> > order here. I suspect it'll be cleaner to just stop doing
> > intel_dp_set_link_train(DISABLE) entirely and just have a dedicated
> > function for disabling link training. We can then trivially do a
> > followup to swap the order of operations to match the spec.
> 
> intel_dp_disable_dpcd_training_pattern() would be needed after each
> LTTPR link training phase, where the port should not output idle
> patterns, that's the only reason for this change.
> 
> Do you mean to remove intel_dp_stop_link_train() then and do the idle
> pattern programming + corresponding DPCD training pattern disable
> programming at the end of the link training sequence (and remove the
> DP_TRAINING_PATTERN_DISABLE case handling from above)? I agree with
> that, but I see that too as a follow-up material (along with changing
> the order as you suggested).

Yeah, followup shuld be fine. I was just thinking of doing
s/intel_dp_set_link_train(DP_TRAINING_PATTERN_DISABLE)/intel_dp_set_normal_link_train()/
or
s/intel_dp_set_link_train(DP_TRAINING_PATTERN_DISABLE)/intel_dp_disable_link_train()/
or something along those lines.
Imre Deak Sept. 22, 2020, 5:59 p.m. UTC | #4 
On Tue, Sep 22, 2020 at 08:47:56PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 22, 2020 at 08:41:28PM +0300, Imre Deak wrote:
> > On Tue, Sep 22, 2020 at 07:54:20PM +0300, Ville Syrjälä wrote:
> > > On Tue, Sep 22, 2020 at 03:51:03PM +0300, Imre Deak wrote:
> > > > To prepare for a follow-up LTTPR change factor out a helper to disable
> > > > the training pattern in DPCD. We'll need to do this for each LTTPR
> > > > (without programming the port to output the idle pattern) when training
> > > > in LTTPR non-transparent mode.
> > > > 
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > ---
> > > >  .../drm/i915/display/intel_dp_link_training.c | 28 +++++++++++--------
> > > >  1 file changed, 16 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > > > index 0c3809891bd2..6994a32244dc 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > > > @@ -102,30 +102,34 @@ void intel_dp_get_adjust_train(struct intel_dp *intel_dp,
> > > >  		intel_dp->train_set[lane] = v | p;
> > > >  }
> > > >  
> > > > +static bool intel_dp_disable_dpcd_training_pattern(struct intel_dp *intel_dp)
> > > > +{
> > > > +	u8 val = DP_TRAINING_PATTERN_DISABLE;
> > > > +
> > > > +	return drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_PATTERN_SET, &val, 1) == 1;
> > > > +}
> > > 
> > > 
> > > > +
> > > >  static bool
> > > >  intel_dp_set_link_train(struct intel_dp *intel_dp,
> > > >  			u8 dp_train_pat)
> > > >  {
> > > >  	u8 buf[sizeof(intel_dp->train_set) + 1];
> > > > -	int ret, len;
> > > > +	int len;
> > > >  
> > > >  	intel_dp_program_link_training_pattern(intel_dp, dp_train_pat);
> > > >  
> > > > -	buf[0] = dp_train_pat;
> > > >  	if ((dp_train_pat & ~DP_LINK_SCRAMBLING_DISABLE) ==
> > > > -	    DP_TRAINING_PATTERN_DISABLE) {
> > > > +	    DP_TRAINING_PATTERN_DISABLE)
> > > >  		/* don't write DP_TRAINING_LANEx_SET on disable */
> > > 
> > > As mentioned in the other patch I think we're doing things in the wrong
> > > order here. I suspect it'll be cleaner to just stop doing
> > > intel_dp_set_link_train(DISABLE) entirely and just have a dedicated
> > > function for disabling link training. We can then trivially do a
> > > followup to swap the order of operations to match the spec.
> > 
> > intel_dp_disable_dpcd_training_pattern() would be needed after each
> > LTTPR link training phase, where the port should not output idle
> > patterns, that's the only reason for this change.
> > 
> > Do you mean to remove intel_dp_stop_link_train() then and do the idle
> > pattern programming + corresponding DPCD training pattern disable
> > programming at the end of the link training sequence (and remove the
> > DP_TRAINING_PATTERN_DISABLE case handling from above)? I agree with
> > that, but I see that too as a follow-up material (along with changing
> > the order as you suggested).
> 
> Yeah, followup shuld be fine. I was just thinking of doing
> s/intel_dp_set_link_train(DP_TRAINING_PATTERN_DISABLE)/intel_dp_set_normal_link_train()/
> or
> s/intel_dp_set_link_train(DP_TRAINING_PATTERN_DISABLE)/intel_dp_disable_link_train()/
> or something along those lines.

Ok, that's simple enough, will do that instead in this patch.

> 
> -- 
> Ville Syrjälä
> Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
index 0c3809891bd2..6994a32244dc 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
@@ -102,30 +102,34 @@  void intel_dp_get_adjust_train(struct intel_dp *intel_dp,
 		intel_dp->train_set[lane] = v | p;
 }
 
+static bool intel_dp_disable_dpcd_training_pattern(struct intel_dp *intel_dp)
+{
+	u8 val = DP_TRAINING_PATTERN_DISABLE;
+
+	return drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_PATTERN_SET, &val, 1) == 1;
+}
+
 static bool
 intel_dp_set_link_train(struct intel_dp *intel_dp,
 			u8 dp_train_pat)
 {
 	u8 buf[sizeof(intel_dp->train_set) + 1];
-	int ret, len;
+	int len;
 
 	intel_dp_program_link_training_pattern(intel_dp, dp_train_pat);
 
-	buf[0] = dp_train_pat;
 	if ((dp_train_pat & ~DP_LINK_SCRAMBLING_DISABLE) ==
-	    DP_TRAINING_PATTERN_DISABLE) {
+	    DP_TRAINING_PATTERN_DISABLE)
 		/* don't write DP_TRAINING_LANEx_SET on disable */
-		len = 1;
-	} else {
-		/* DP_TRAINING_LANEx_SET follow DP_TRAINING_PATTERN_SET */
-		memcpy(buf + 1, intel_dp->train_set, intel_dp->lane_count);
-		len = intel_dp->lane_count + 1;
-	}
+		return intel_dp_disable_dpcd_training_pattern(intel_dp);
 
-	ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_PATTERN_SET,
-				buf, len);
+	buf[0] = dp_train_pat;
+	/* DP_TRAINING_LANEx_SET follow DP_TRAINING_PATTERN_SET */
+	memcpy(buf + 1, intel_dp->train_set, intel_dp->lane_count);
+	len = intel_dp->lane_count + 1;
 
-	return ret == len;
+	return drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_PATTERN_SET,
+				 buf, len) == len;
 }
 
 static bool