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

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

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
> +		 */
> +		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