Linux-ARM-Kernel Archive on lore.kernel.org

* Re: [PATCH RFC v2 2/4] drm/i915/display/dp: Adopt dp_connector helpers to expose link training state
From: Jani Nikula @ 2026-06-22  7:28 UTC (permalink / raw)
  To: Kory Maincent, Maarten Lankhorst, Maxime Ripard,
	Thomas Zimmermann, David Airlie, Simona Vetter, Rodrigo Vivi,
	Joonas Lahtinen, Tvrtko Ursulin, Andrzej Hajda, Neil Armstrong,
	Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
	Luca Ceresoli, Chun-Kuang Hu, Philipp Zabel, Matthias Brugger,
	AngeloGioacchino Del Regno, Dmitry Baryshkov, Daniel Stone
  Cc: Thomas Petazzoni, Mark Yacoub, Sean Paul, Manasi Navare,
	Drew Davenport, Louis Chauvet, Luca Ceresoli, dri-devel,
	linux-kernel, intel-gfx, intel-xe, linux-mediatek,
	linux-arm-kernel, Kory Maincent
In-Reply-To: <20260619-feat_link_cap-v2-2-a3dec4c02ad9@bootlin.com>

On Fri, 19 Jun 2026, Kory Maincent <kory.maincent@bootlin.com> wrote:
> Switch the i915 DP connector initialization from
> drm_connector_init_with_ddc() to drm_connector_dp_init_with_ddc(),
> providing the source link capabilities (supported lane counts, link rates
> and DSC support).
>
> Add intel_dp_report_link_train() to collect the negotiated link
> parameters (rate, lane count and DSC enable) and report them via
> drm_dp_set_max_link_params() and drm_dp_set_cur_link_params() once
> link training completes successfully.
>
> Reset the link properties via drm_dp_reset_link_params()
> when the connector is reported as disconnected or when the display device
> is disabled, so the exposed state always reflects the current link status.
>
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
>
> Changes in v2:
> - Remove voltage swing and pre emphasis properties.
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c            | 26 ++++++++++++++++++----
>  .../gpu/drm/i915/display/intel_dp_link_training.c  | 17 ++++++++++++++
>  2 files changed, 39 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index f01a6eed38395..46c06c76952e0 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -6414,8 +6414,10 @@ intel_dp_detect(struct drm_connector *_connector,
>  	drm_WARN_ON(display->drm,
>  		    !drm_modeset_is_locked(&display->drm->mode_config.connection_mutex));
>  
> -	if (!intel_display_device_enabled(display))
> +	if (!intel_display_device_enabled(display)) {
> +		drm_dp_sink_reset_link_caps(_connector);

Gut feeling is that the sprinkling of these around is error prone.

>  		return connector_status_disconnected;
> +	}
>  
>  	if (!intel_display_driver_check_access(display))
>  		return connector->base.status;
> @@ -6465,6 +6467,8 @@ intel_dp_detect(struct drm_connector *_connector,
>  
>  		intel_dp_tunnel_disconnect(intel_dp);
>  
> +		drm_dp_sink_reset_link_caps(_connector);
> +
>  		goto out_unset_edid;
>  	}
>  
> @@ -7240,10 +7244,12 @@ intel_dp_init_connector(struct intel_digital_port *dig_port,
>  			struct intel_connector *connector)
>  {
>  	struct intel_display *display = to_intel_display(dig_port);
> +	struct drm_connector_dp_link_caps link_caps;
>  	struct intel_dp *intel_dp = &dig_port->dp;
>  	struct intel_encoder *encoder = &dig_port->base;
>  	struct drm_device *dev = encoder->base.dev;
>  	enum port port = encoder->port;
> +	u32 *rates;
>  	int type;
>  
>  	if (drm_WARN(dev, dig_port->max_lanes < 1,
> @@ -7291,8 +7297,21 @@ intel_dp_init_connector(struct intel_digital_port *dig_port,
>  		    type == DRM_MODE_CONNECTOR_eDP ? "eDP" : "DP",
>  		    encoder->base.base.id, encoder->base.name);
>  
> -	drm_connector_init_with_ddc(dev, &connector->base, &intel_dp_connector_funcs,
> -				    type, &intel_dp->aux.ddc);
> +	intel_dp_set_source_rates(intel_dp);
> +	link_caps.nlanes = 4;
> +	link_caps.nlink_rates = intel_dp->num_source_rates;
> +	rates = kmemdup_array(intel_dp->source_rates, intel_dp->num_source_rates,
> +			      sizeof(*rates), GFP_KERNEL);
> +	if (!rates)
> +		goto fail;
> +
> +	link_caps.link_rates = rates;
> +	link_caps.dsc = HAS_DSC(display);
> +
> +	drm_connector_dp_init_with_ddc(dev, &connector->base, &intel_dp_connector_funcs,
> +				       &link_caps, type, &intel_dp->aux.ddc);
> +	kfree(rates);
> +

All of the above feels a bit clumsy in the middle of an already too long
function.

>  	drm_connector_helper_add(&connector->base, &intel_dp_connector_helper_funcs);
>  
>  	if (!HAS_GMCH(display) && DISPLAY_VER(display) < 12)
> @@ -7315,7 +7334,6 @@ intel_dp_init_connector(struct intel_digital_port *dig_port,
>  		goto fail;
>  	}
>  
> -	intel_dp_set_source_rates(intel_dp);
>  	intel_dp_set_common_rates(intel_dp);
>  	intel_dp_reset_link_params(intel_dp);
>  
> 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 a26094223f780..25e0e957fe36d 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> @@ -1231,6 +1231,18 @@ intel_dp_128b132b_intra_hop(struct intel_dp *intel_dp,
>  	return sink_status & DP_INTRA_HOP_AUX_REPLY_INDICATION ? 1 : 0;
>  }
>  
> +static void intel_dp_report_link_train(struct intel_dp *intel_dp)
> +{
> +	struct intel_connector *connector = intel_dp->attached_connector;
> +
> +	drm_dp_set_max_link_params(&connector->base, intel_dp->link_rate,
> +				   intel_dp->lane_count);
> +
> +	drm_dp_set_cur_link_params(&connector->base, intel_dp->link_rate,
> +				   intel_dp->lane_count,
> +				   connector->dp.dsc_decompression_enabled);
> +}
> +
>  /**
>   * intel_dp_stop_link_train - stop link training
>   * @intel_dp: DP struct
> @@ -1259,6 +1271,9 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp,
>  	intel_dp_program_link_training_pattern(intel_dp, crtc_state, DP_PHY_DPRX,
>  					       DP_TRAINING_PATTERN_DISABLE);
>  
> +	if (!intel_dp->is_mst)
> +		intel_dp_report_link_train(intel_dp);
> +
>  	if (intel_dp_is_uhbr(crtc_state)) {
>  		ret = poll_timeout_us(ret = intel_dp_128b132b_intra_hop(intel_dp, crtc_state),
>  				      ret == 0,
> @@ -1772,6 +1787,8 @@ void intel_dp_start_link_train(struct intel_atomic_state *state,
>  	 */
>  	int lttpr_count;
>  
> +	drm_dp_sink_set_link_caps(&intel_dp->attached_connector->base, &intel_dp->aux);

This is not okay.

We've already figured out all the information needed, and this call goes
ahead and reads the same information out again.

Moreover, some sinks are fragile when it comes to reading the info and
starting link training. The CTS might complain about redundant reads as
well.

drm_dp_sink_set_link_caps() as a name implies something about link,
i.e. the thing between the source and the sink. But this only sets sink
capabilities. Which also means it should not happen at link training
time at all. We figure the information out at detect, which means it's
available *before* modeset.

The more I think about the function, the more I question it. Like, if
the source doesn't support UHBR at all, or not on this connector, what's
the point of reading the sink UHBR rates?

> +
>  	intel_hpd_block(encoder);
>  
>  	lttpr_count = intel_dp_init_lttpr_and_dprx_caps(intel_dp);

-- 
Jani Nikula, Intel

^ permalink raw reply