diff mbox series

[v2,04/21] drm/i915/dp: Sanitize intel_dp_get_link_train_fallback_values()

Message ID 20240520185822.3725844-5-imre.deak@intel.com (mailing list archive)
State New
Headers show
Series drm/i915/dp_mst: Enable link training fallback | expand

Commit Message

Imre Deak May 20, 2024, 6:58 p.m. UTC 
Reduce the indentation in intel_dp_get_link_train_fallback_values() by
adding separate helpers to reduce the link rate and lane count. Also
simplify things by passing crtc_state to the function.

This also prepares for later patches in the patchset adding a limitation
on how the link params are reduced.

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

Comments

Ville Syrjälä May 22, 2024, 6:23 p.m. UTC | #1 
On Mon, May 20, 2024 at 09:58:02PM +0300, Imre Deak wrote:
> Reduce the indentation in intel_dp_get_link_train_fallback_values() by
> adding separate helpers to reduce the link rate and lane count. Also
> simplify things by passing crtc_state to the function.
> 
> This also prepares for later patches in the patchset adding a limitation
> on how the link params are reduced.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  .../drm/i915/display/intel_dp_link_training.c | 82 ++++++++++++-------
>  1 file changed, 51 insertions(+), 31 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 4db293f256896..edc970036866a 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> @@ -1109,11 +1109,37 @@ static bool intel_dp_can_link_train_fallback_for_edp(struct intel_dp *intel_dp,
>  	return true;
>  }
>  
> +static int reduce_link_rate(struct intel_dp *intel_dp, int current_rate)
> +{
> +	int rate_index;
> +	int new_rate;
> +
> +	rate_index = intel_dp_rate_index(intel_dp->common_rates,
> +					 intel_dp->num_common_rates,
> +					 current_rate);
> +
> +	if (rate_index <= 0)
> +		return -1;
> +
> +	new_rate = intel_dp_common_rate(intel_dp, rate_index - 1);
> +
> +	return new_rate;

This is structured as

if (bad)
	fail;
success;

> +}
> +
> +static int reduce_lane_count(struct intel_dp *intel_dp, int current_lane_count)
> +{
> +	if (current_lane_count > 1)
> +		return current_lane_count >> 1;
> +
> +	return -1;

whereas this is

if (ok)
	success;
fail;

I'd rearrange one of them so the logic is the same way around in both.

Otherwise lgtm
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +}
> +
>  static int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
> -						   int link_rate, u8 lane_count)
> +						   const struct intel_crtc_state *crtc_state)
>  {
>  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> -	int index;
> +	int new_link_rate;
> +	int new_lane_count;
>  
>  	/*
>  	 * TODO: Enable fallback on MST links once MST link compute can handle
> @@ -1131,36 +1157,32 @@ static int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
>  		return 0;
>  	}
>  
> -	index = intel_dp_rate_index(intel_dp->common_rates,
> -				    intel_dp->num_common_rates,
> -				    link_rate);
> -	if (index > 0) {
> -		if (intel_dp_is_edp(intel_dp) &&
> -		    !intel_dp_can_link_train_fallback_for_edp(intel_dp,
> -							      intel_dp_common_rate(intel_dp, index - 1),
> -							      lane_count)) {
> -			drm_dbg_kms(&i915->drm,
> -				    "Retrying Link training for eDP with same parameters\n");
> -			return 0;
> -		}
> -		intel_dp->link.max_rate = intel_dp_common_rate(intel_dp, index - 1);
> -		intel_dp->link.max_lane_count = lane_count;
> -	} else if (lane_count > 1) {
> -		if (intel_dp_is_edp(intel_dp) &&
> -		    !intel_dp_can_link_train_fallback_for_edp(intel_dp,
> -							      intel_dp_max_common_rate(intel_dp),
> -							      lane_count >> 1)) {
> -			drm_dbg_kms(&i915->drm,
> -				    "Retrying Link training for eDP with same parameters\n");
> -			return 0;
> -		}
> -		intel_dp->link.max_rate = intel_dp_max_common_rate(intel_dp);
> -		intel_dp->link.max_lane_count = lane_count >> 1;
> -	} else {
> +	new_lane_count = crtc_state->lane_count;
> +	new_link_rate = reduce_link_rate(intel_dp, crtc_state->port_clock);
> +	if (new_link_rate < 0) {
> +		new_lane_count = reduce_lane_count(intel_dp, crtc_state->lane_count);
> +		new_link_rate = intel_dp_max_common_rate(intel_dp);
> +	}
> +
> +	if (new_lane_count < 0) {
>  		drm_err(&i915->drm, "Link Training Unsuccessful\n");
>  		return -1;
>  	}
>  
> +	if (intel_dp_is_edp(intel_dp) &&
> +	    !intel_dp_can_link_train_fallback_for_edp(intel_dp, new_link_rate, new_lane_count)) {
> +		drm_dbg_kms(&i915->drm,
> +			    "Retrying Link training for eDP with same parameters\n");
> +		return 0;
> +	}
> +
> +	drm_dbg_kms(&i915->drm, "Reducing link parameters from %dx%d to %dx%d\n",
> +		    crtc_state->lane_count, crtc_state->port_clock,
> +		    new_lane_count, new_link_rate);
> +
> +	intel_dp->link.max_rate = new_link_rate;
> +	intel_dp->link.max_lane_count = new_lane_count;
> +
>  	return 0;
>  }
>  
> @@ -1178,9 +1200,7 @@ static void intel_dp_schedule_fallback_link_training(struct intel_dp *intel_dp,
>  		lt_dbg(intel_dp, DP_PHY_DPRX,
>  		       "Link Training failed with HOBL active, not enabling it from now on\n");
>  		intel_dp->hobl_failed = true;
> -	} else if (intel_dp_get_link_train_fallback_values(intel_dp,
> -							   crtc_state->port_clock,
> -							   crtc_state->lane_count)) {
> +	} else if (intel_dp_get_link_train_fallback_values(intel_dp, crtc_state)) {
>  		return;
>  	}
>  
> -- 
> 2.43.3
Imre Deak May 22, 2024, 6:45 p.m. UTC | #2 
On Wed, May 22, 2024 at 09:23:20PM +0300, Ville Syrjälä wrote:
> On Mon, May 20, 2024 at 09:58:02PM +0300, Imre Deak wrote:
> > Reduce the indentation in intel_dp_get_link_train_fallback_values() by
> > adding separate helpers to reduce the link rate and lane count. Also
> > simplify things by passing crtc_state to the function.
> > 
> > This also prepares for later patches in the patchset adding a limitation
> > on how the link params are reduced.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  .../drm/i915/display/intel_dp_link_training.c | 82 ++++++++++++-------
> >  1 file changed, 51 insertions(+), 31 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 4db293f256896..edc970036866a 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > @@ -1109,11 +1109,37 @@ static bool intel_dp_can_link_train_fallback_for_edp(struct intel_dp *intel_dp,
> >  	return true;
> >  }
> >  
> > +static int reduce_link_rate(struct intel_dp *intel_dp, int current_rate)
> > +{
> > +	int rate_index;
> > +	int new_rate;
> > +
> > +	rate_index = intel_dp_rate_index(intel_dp->common_rates,
> > +					 intel_dp->num_common_rates,
> > +					 current_rate);
> > +
> > +	if (rate_index <= 0)
> > +		return -1;
> > +
> > +	new_rate = intel_dp_common_rate(intel_dp, rate_index - 1);
> > +
> > +	return new_rate;
> 
> This is structured as
> 
> if (bad)
> 	fail;
> success;
> 
> > +}
> > +
> > +static int reduce_lane_count(struct intel_dp *intel_dp, int current_lane_count)
> > +{
> > +	if (current_lane_count > 1)
> > +		return current_lane_count >> 1;
> > +
> > +	return -1;
> 
> whereas this is
> 
> if (ok)
> 	success;
> fail;
> 
> I'd rearrange one of them so the logic is the same way around in both.

Ok, will change that.

> Otherwise lgtm
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> > +}
> > +
> >  static int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
> > -						   int link_rate, u8 lane_count)
> > +						   const struct intel_crtc_state *crtc_state)
> >  {
> >  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > -	int index;
> > +	int new_link_rate;
> > +	int new_lane_count;
> >  
> >  	/*
> >  	 * TODO: Enable fallback on MST links once MST link compute can handle
> > @@ -1131,36 +1157,32 @@ static int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
> >  		return 0;
> >  	}
> >  
> > -	index = intel_dp_rate_index(intel_dp->common_rates,
> > -				    intel_dp->num_common_rates,
> > -				    link_rate);
> > -	if (index > 0) {
> > -		if (intel_dp_is_edp(intel_dp) &&
> > -		    !intel_dp_can_link_train_fallback_for_edp(intel_dp,
> > -							      intel_dp_common_rate(intel_dp, index - 1),
> > -							      lane_count)) {
> > -			drm_dbg_kms(&i915->drm,
> > -				    "Retrying Link training for eDP with same parameters\n");
> > -			return 0;
> > -		}
> > -		intel_dp->link.max_rate = intel_dp_common_rate(intel_dp, index - 1);
> > -		intel_dp->link.max_lane_count = lane_count;
> > -	} else if (lane_count > 1) {
> > -		if (intel_dp_is_edp(intel_dp) &&
> > -		    !intel_dp_can_link_train_fallback_for_edp(intel_dp,
> > -							      intel_dp_max_common_rate(intel_dp),
> > -							      lane_count >> 1)) {
> > -			drm_dbg_kms(&i915->drm,
> > -				    "Retrying Link training for eDP with same parameters\n");
> > -			return 0;
> > -		}
> > -		intel_dp->link.max_rate = intel_dp_max_common_rate(intel_dp);
> > -		intel_dp->link.max_lane_count = lane_count >> 1;
> > -	} else {
> > +	new_lane_count = crtc_state->lane_count;
> > +	new_link_rate = reduce_link_rate(intel_dp, crtc_state->port_clock);
> > +	if (new_link_rate < 0) {
> > +		new_lane_count = reduce_lane_count(intel_dp, crtc_state->lane_count);
> > +		new_link_rate = intel_dp_max_common_rate(intel_dp);
> > +	}
> > +
> > +	if (new_lane_count < 0) {
> >  		drm_err(&i915->drm, "Link Training Unsuccessful\n");
> >  		return -1;
> >  	}
> >  
> > +	if (intel_dp_is_edp(intel_dp) &&
> > +	    !intel_dp_can_link_train_fallback_for_edp(intel_dp, new_link_rate, new_lane_count)) {
> > +		drm_dbg_kms(&i915->drm,
> > +			    "Retrying Link training for eDP with same parameters\n");
> > +		return 0;
> > +	}
> > +
> > +	drm_dbg_kms(&i915->drm, "Reducing link parameters from %dx%d to %dx%d\n",
> > +		    crtc_state->lane_count, crtc_state->port_clock,
> > +		    new_lane_count, new_link_rate);
> > +
> > +	intel_dp->link.max_rate = new_link_rate;
> > +	intel_dp->link.max_lane_count = new_lane_count;
> > +
> >  	return 0;
> >  }
> >  
> > @@ -1178,9 +1200,7 @@ static void intel_dp_schedule_fallback_link_training(struct intel_dp *intel_dp,
> >  		lt_dbg(intel_dp, DP_PHY_DPRX,
> >  		       "Link Training failed with HOBL active, not enabling it from now on\n");
> >  		intel_dp->hobl_failed = true;
> > -	} else if (intel_dp_get_link_train_fallback_values(intel_dp,
> > -							   crtc_state->port_clock,
> > -							   crtc_state->lane_count)) {
> > +	} else if (intel_dp_get_link_train_fallback_values(intel_dp, crtc_state)) {
> >  		return;
> >  	}
> >  
> > -- 
> > 2.43.3
> 
> -- 
> Ville Syrjälä
> Intel
Jani Nikula May 23, 2024, 3:48 p.m. UTC | #3 
On Wed, 22 May 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Mon, May 20, 2024 at 09:58:02PM +0300, Imre Deak wrote:
>> Reduce the indentation in intel_dp_get_link_train_fallback_values() by
>> adding separate helpers to reduce the link rate and lane count. Also
>> simplify things by passing crtc_state to the function.
>> 
>> This also prepares for later patches in the patchset adding a limitation
>> on how the link params are reduced.
>> 
>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>> ---
>>  .../drm/i915/display/intel_dp_link_training.c | 82 ++++++++++++-------
>>  1 file changed, 51 insertions(+), 31 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 4db293f256896..edc970036866a 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
>> @@ -1109,11 +1109,37 @@ static bool intel_dp_can_link_train_fallback_for_edp(struct intel_dp *intel_dp,
>>  	return true;
>>  }
>>  
>> +static int reduce_link_rate(struct intel_dp *intel_dp, int current_rate)
>> +{
>> +	int rate_index;
>> +	int new_rate;
>> +
>> +	rate_index = intel_dp_rate_index(intel_dp->common_rates,
>> +					 intel_dp->num_common_rates,
>> +					 current_rate);
>> +
>> +	if (rate_index <= 0)
>> +		return -1;
>> +
>> +	new_rate = intel_dp_common_rate(intel_dp, rate_index - 1);
>> +
>> +	return new_rate;
>
> This is structured as
>
> if (bad)
> 	fail;
> success;
>
>> +}
>> +
>> +static int reduce_lane_count(struct intel_dp *intel_dp, int current_lane_count)
>> +{
>> +	if (current_lane_count > 1)
>> +		return current_lane_count >> 1;
>> +
>> +	return -1;
>
> whereas this is
>
> if (ok)
> 	success;
> fail;
>
> I'd rearrange one of them so the logic is the same way around in both.

Usually failures in if branches with early returns and happy day
scenario with the least indentation.

BR,
Jani.



>
> Otherwise lgtm
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>> +}
>> +
>>  static int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
>> -						   int link_rate, u8 lane_count)
>> +						   const struct intel_crtc_state *crtc_state)
>>  {
>>  	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>> -	int index;
>> +	int new_link_rate;
>> +	int new_lane_count;
>>  
>>  	/*
>>  	 * TODO: Enable fallback on MST links once MST link compute can handle
>> @@ -1131,36 +1157,32 @@ static int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
>>  		return 0;
>>  	}
>>  
>> -	index = intel_dp_rate_index(intel_dp->common_rates,
>> -				    intel_dp->num_common_rates,
>> -				    link_rate);
>> -	if (index > 0) {
>> -		if (intel_dp_is_edp(intel_dp) &&
>> -		    !intel_dp_can_link_train_fallback_for_edp(intel_dp,
>> -							      intel_dp_common_rate(intel_dp, index - 1),
>> -							      lane_count)) {
>> -			drm_dbg_kms(&i915->drm,
>> -				    "Retrying Link training for eDP with same parameters\n");
>> -			return 0;
>> -		}
>> -		intel_dp->link.max_rate = intel_dp_common_rate(intel_dp, index - 1);
>> -		intel_dp->link.max_lane_count = lane_count;
>> -	} else if (lane_count > 1) {
>> -		if (intel_dp_is_edp(intel_dp) &&
>> -		    !intel_dp_can_link_train_fallback_for_edp(intel_dp,
>> -							      intel_dp_max_common_rate(intel_dp),
>> -							      lane_count >> 1)) {
>> -			drm_dbg_kms(&i915->drm,
>> -				    "Retrying Link training for eDP with same parameters\n");
>> -			return 0;
>> -		}
>> -		intel_dp->link.max_rate = intel_dp_max_common_rate(intel_dp);
>> -		intel_dp->link.max_lane_count = lane_count >> 1;
>> -	} else {
>> +	new_lane_count = crtc_state->lane_count;
>> +	new_link_rate = reduce_link_rate(intel_dp, crtc_state->port_clock);
>> +	if (new_link_rate < 0) {
>> +		new_lane_count = reduce_lane_count(intel_dp, crtc_state->lane_count);
>> +		new_link_rate = intel_dp_max_common_rate(intel_dp);
>> +	}
>> +
>> +	if (new_lane_count < 0) {
>>  		drm_err(&i915->drm, "Link Training Unsuccessful\n");
>>  		return -1;
>>  	}
>>  
>> +	if (intel_dp_is_edp(intel_dp) &&
>> +	    !intel_dp_can_link_train_fallback_for_edp(intel_dp, new_link_rate, new_lane_count)) {
>> +		drm_dbg_kms(&i915->drm,
>> +			    "Retrying Link training for eDP with same parameters\n");
>> +		return 0;
>> +	}
>> +
>> +	drm_dbg_kms(&i915->drm, "Reducing link parameters from %dx%d to %dx%d\n",
>> +		    crtc_state->lane_count, crtc_state->port_clock,
>> +		    new_lane_count, new_link_rate);
>> +
>> +	intel_dp->link.max_rate = new_link_rate;
>> +	intel_dp->link.max_lane_count = new_lane_count;
>> +
>>  	return 0;
>>  }
>>  
>> @@ -1178,9 +1200,7 @@ static void intel_dp_schedule_fallback_link_training(struct intel_dp *intel_dp,
>>  		lt_dbg(intel_dp, DP_PHY_DPRX,
>>  		       "Link Training failed with HOBL active, not enabling it from now on\n");
>>  		intel_dp->hobl_failed = true;
>> -	} else if (intel_dp_get_link_train_fallback_values(intel_dp,
>> -							   crtc_state->port_clock,
>> -							   crtc_state->lane_count)) {
>> +	} else if (intel_dp_get_link_train_fallback_values(intel_dp, crtc_state)) {
>>  		return;
>>  	}
>>  
>> -- 
>> 2.43.3
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 4db293f256896..edc970036866a 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
@@ -1109,11 +1109,37 @@  static bool intel_dp_can_link_train_fallback_for_edp(struct intel_dp *intel_dp,
 	return true;
 }
 
+static int reduce_link_rate(struct intel_dp *intel_dp, int current_rate)
+{
+	int rate_index;
+	int new_rate;
+
+	rate_index = intel_dp_rate_index(intel_dp->common_rates,
+					 intel_dp->num_common_rates,
+					 current_rate);
+
+	if (rate_index <= 0)
+		return -1;
+
+	new_rate = intel_dp_common_rate(intel_dp, rate_index - 1);
+
+	return new_rate;
+}
+
+static int reduce_lane_count(struct intel_dp *intel_dp, int current_lane_count)
+{
+	if (current_lane_count > 1)
+		return current_lane_count >> 1;
+
+	return -1;
+}
+
 static int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
-						   int link_rate, u8 lane_count)
+						   const struct intel_crtc_state *crtc_state)
 {
 	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
-	int index;
+	int new_link_rate;
+	int new_lane_count;
 
 	/*
 	 * TODO: Enable fallback on MST links once MST link compute can handle
@@ -1131,36 +1157,32 @@  static int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
 		return 0;
 	}
 
-	index = intel_dp_rate_index(intel_dp->common_rates,
-				    intel_dp->num_common_rates,
-				    link_rate);
-	if (index > 0) {
-		if (intel_dp_is_edp(intel_dp) &&
-		    !intel_dp_can_link_train_fallback_for_edp(intel_dp,
-							      intel_dp_common_rate(intel_dp, index - 1),
-							      lane_count)) {
-			drm_dbg_kms(&i915->drm,
-				    "Retrying Link training for eDP with same parameters\n");
-			return 0;
-		}
-		intel_dp->link.max_rate = intel_dp_common_rate(intel_dp, index - 1);
-		intel_dp->link.max_lane_count = lane_count;
-	} else if (lane_count > 1) {
-		if (intel_dp_is_edp(intel_dp) &&
-		    !intel_dp_can_link_train_fallback_for_edp(intel_dp,
-							      intel_dp_max_common_rate(intel_dp),
-							      lane_count >> 1)) {
-			drm_dbg_kms(&i915->drm,
-				    "Retrying Link training for eDP with same parameters\n");
-			return 0;
-		}
-		intel_dp->link.max_rate = intel_dp_max_common_rate(intel_dp);
-		intel_dp->link.max_lane_count = lane_count >> 1;
-	} else {
+	new_lane_count = crtc_state->lane_count;
+	new_link_rate = reduce_link_rate(intel_dp, crtc_state->port_clock);
+	if (new_link_rate < 0) {
+		new_lane_count = reduce_lane_count(intel_dp, crtc_state->lane_count);
+		new_link_rate = intel_dp_max_common_rate(intel_dp);
+	}
+
+	if (new_lane_count < 0) {
 		drm_err(&i915->drm, "Link Training Unsuccessful\n");
 		return -1;
 	}
 
+	if (intel_dp_is_edp(intel_dp) &&
+	    !intel_dp_can_link_train_fallback_for_edp(intel_dp, new_link_rate, new_lane_count)) {
+		drm_dbg_kms(&i915->drm,
+			    "Retrying Link training for eDP with same parameters\n");
+		return 0;
+	}
+
+	drm_dbg_kms(&i915->drm, "Reducing link parameters from %dx%d to %dx%d\n",
+		    crtc_state->lane_count, crtc_state->port_clock,
+		    new_lane_count, new_link_rate);
+
+	intel_dp->link.max_rate = new_link_rate;
+	intel_dp->link.max_lane_count = new_lane_count;
+
 	return 0;
 }
 
@@ -1178,9 +1200,7 @@  static void intel_dp_schedule_fallback_link_training(struct intel_dp *intel_dp,
 		lt_dbg(intel_dp, DP_PHY_DPRX,
 		       "Link Training failed with HOBL active, not enabling it from now on\n");
 		intel_dp->hobl_failed = true;
-	} else if (intel_dp_get_link_train_fallback_values(intel_dp,
-							   crtc_state->port_clock,
-							   crtc_state->lane_count)) {
+	} else if (intel_dp_get_link_train_fallback_values(intel_dp, crtc_state)) {
 		return;
 	}