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

[Jani Nikula <jani.nikula@linux.intel.com>]

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

-- 
Jani Nikula, Intel