On Mon, 17 Apr 2023, Jani Nikula <jani.nikula@xxxxxxxxx> wrote:
> On Mon, 17 Apr 2023, Arun R Murthy <arun.r.murthy@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> .../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
