Re: [Intel-gfx] [PATCH] drm/i915/display/dp: 128/132b LT requirement

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

On Mon, 17 Apr 2023, Jani Nikula <jani.nikula@intel.com> wrote:
> On Mon, 17 Apr 2023, Arun R Murthy <arun.r.murthy@intel.com> wrote:
>> For 128b/132b LT prior to LT DPTX should set power state, DP channel
>> coding and then link rate.
>>
>> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
>> ---
>>  .../drm/i915/display/intel_dp_link_training.c | 52 +++++++++++++------
>>  1 file changed, 35 insertions(+), 17 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 6aa4ae5e7ebe..83ea9ece0157 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
>> @@ -686,23 +686,41 @@ intel_dp_prepare_link_train(struct intel_dp *intel_dp,
>>  		drm_dbg_kms(&i915->drm,
>>  			    "[ENCODER:%d:%s] Using LINK_RATE_SET value %02x\n",
>>  			    encoder->base.base.id, encoder->base.name, rate_select);
>> -
>> -	/* Write the link configuration data */
>> -	link_config[0] = link_bw;
>> -	link_config[1] = crtc_state->lane_count;
>> -	if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
>> -		link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
>> -	drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2);
>> -
>> -	/* eDP 1.4 rate select method. */
>> -	if (!link_bw)
>> -		drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_RATE_SET,
>> -				  &rate_select, 1);
>> -
>> -	link_config[0] = crtc_state->vrr.flipline ? DP_MSA_TIMING_PAR_IGNORE_EN : 0;
>> -	link_config[1] = intel_dp_is_uhbr(crtc_state) ?
>> -		DP_SET_ANSI_128B132B : DP_SET_ANSI_8B10B;
>> -	drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
>> +	if (intel_dp_is_uhbr(crtc_state)) {
>> +		/*
>> +		 * Spec DP2.1 Section 3.5.2.16
>> +		 * Prior to LT DPTX should set 128/132 DP Channel coding and then set link rate

PS. I've taken great care to use "128b/132b" in comments
everywhere. There isn't a single instance of "128/132".

It'll be helpful when you git grep 128b/132b.

BR,
Jani.

>> +		 */
>> +		link_config[0] = crtc_state->vrr.enable ? DP_MSA_TIMING_PAR_IGNORE_EN : 0;
>> +		link_config[1] = intel_dp_is_uhbr(crtc_state) ?
>> +			DP_SET_ANSI_128B132B : DP_SET_ANSI_8B10B;
>> +		drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
>> +		/* Write the link configuration data */
>> +		link_config[0] = link_bw;
>> +		link_config[1] = crtc_state->lane_count;
>> +		if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
>> +			link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
>> +		drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2);
>> +		/* eDP 1.4 rate select method. */
>> +		if (!link_bw)
>> +			drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_RATE_SET,
>> +					  &rate_select, 1);
>> +	} else {
>> +		/* Write the link configuration data */
>> +		link_config[0] = link_bw;
>> +		link_config[1] = crtc_state->lane_count;
>> +		if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
>> +			link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
>> +		drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2);
>> +		/* eDP 1.4 rate select method. */
>> +		if (!link_bw)
>> +			drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_RATE_SET,
>> +					  &rate_select, 1);
>> +		link_config[0] = crtc_state->vrr.enable ? DP_MSA_TIMING_PAR_IGNORE_EN : 0;
>> +		link_config[1] = intel_dp_is_uhbr(crtc_state) ?
>> +			DP_SET_ANSI_128B132B : DP_SET_ANSI_8B10B;
>> +		drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
>> +	}
>
> I'd rather we change the order for 8b10b too.
>
> If we can't do that, you need to add two functions that do each step,
> and then call them in different order for different channel coding. We
> don't want all of the above duplicated.
>
> Also, in what looks like a rebase fail, you change vrr.flipline to
> vrr.enable.
>
>
> BR,
> Jani.
>
>
>>  
>>  	return true;
>>  }

-- 
Jani Nikula, Intel Open Source Graphics Center