Re: [PATCH v2 5/9] drm/i915/dp: Validate "4:2:0 also" modes twice

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 4/9/2026 3:45 PM, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Currently we only validate "4:2:0 also" modes as RGB. But
> if that fails we could perhaps still use the mode in with
> 4:2:0 output. All we have to do is retry the validation with
> the different sink format.
>
> So far we did the double validation only so far as it affects
> PCON TMDS clock limits. But validating everything twice seems
> a bit more sane.
>
> Note that intel_dp_output_format() might still end up picking
> RGB for the actual output format (and letting PCON deal with
> the YCbCr conversion). So I suppose we could still fail the
> validation due to that, and forcing even the output format
> to 4:2:0 might solve it on a third try. But we'd need the
> same fallback logic in intel_dp_compute_config(). For now
> this seems sufficient.
>
> v2: Pass output_format to intel_dp_mode_min_link_bpp_x16()
>      Add TODO about remaining issues
>      Nuke intel_dp_sink_format()

Thanks, this addresses all the comments on v1.

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

>
> Cc: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_dp.c | 151 ++++++++++++------------
>   1 file changed, 78 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index ed5841f224ee..99672341f43e 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1234,28 +1234,11 @@ int intel_dp_output_format_link_bpp_x16(enum intel_output_format output_format,
>   	return fxp_q4_from_int(pipe_bpp);
>   }
>   
> -static enum intel_output_format
> -intel_dp_sink_format(struct intel_connector *connector,
> -		     const struct drm_display_mode *mode)
> -{
> -	const struct drm_display_info *info = &connector->base.display_info;
> -
> -	if (drm_mode_is_420_only(info, mode))
> -		return INTEL_OUTPUT_FORMAT_YCBCR420;
> -
> -	return INTEL_OUTPUT_FORMAT_RGB;
> -}
> -
>   static int
>   intel_dp_mode_min_link_bpp_x16(struct intel_connector *connector,
> -			       const struct drm_display_mode *mode)
> +			       const struct drm_display_mode *mode,
> +			       enum intel_output_format output_format)
>   {
> -	enum intel_output_format output_format, sink_format;
> -
> -	sink_format = intel_dp_sink_format(connector, mode);
> -
> -	output_format = intel_dp_output_format(connector, sink_format);
> -
>   	return intel_dp_output_format_link_bpp_x16(output_format,
>   						   intel_dp_min_bpp(output_format));
>   }
> @@ -1329,14 +1312,10 @@ static int frl_required_bw(int clock, int bpc,
>   static enum drm_mode_status
>   intel_dp_mode_valid_downstream(struct intel_connector *connector,
>   			       const struct drm_display_mode *mode,
> -			       int target_clock)
> +			       int target_clock,
> +			       enum intel_output_format sink_format)
>   {
>   	struct intel_dp *intel_dp = intel_attached_dp(connector);
> -	const struct drm_display_info *info = &connector->base.display_info;
> -	enum drm_mode_status status;
> -	enum intel_output_format sink_format;
> -
> -	sink_format = intel_dp_sink_format(connector, mode);
>   
>   	/* If PCON supports FRL MODE, check FRL bandwidth constraints */
>   	if (intel_dp->dfp.pcon_max_frl_bw) {
> @@ -1361,22 +1340,8 @@ intel_dp_mode_valid_downstream(struct intel_connector *connector,
>   		return MODE_CLOCK_HIGH;
>   
>   	/* Assume 8bpc for the DP++/HDMI/DVI TMDS clock check */
> -	status = intel_dp_tmds_clock_valid(intel_dp, target_clock,
> -					   8, sink_format, true);
> -
> -	if (status != MODE_OK) {
> -		if (sink_format == INTEL_OUTPUT_FORMAT_YCBCR420 ||
> -		    !connector->base.ycbcr_420_allowed ||
> -		    !drm_mode_is_420_also(info, mode))
> -			return status;
> -		sink_format = INTEL_OUTPUT_FORMAT_YCBCR420;
> -		status = intel_dp_tmds_clock_valid(intel_dp, target_clock,
> -						   8, sink_format, true);
> -		if (status != MODE_OK)
> -			return status;
> -	}
> -
> -	return MODE_OK;
> +	return intel_dp_tmds_clock_valid(intel_dp, target_clock,
> +					 8, sink_format, true);
>   }
>   
>   static enum drm_mode_status
> @@ -1472,15 +1437,14 @@ bool intel_dp_dotclk_valid(struct intel_display *display,
>   }
>   
>   static enum drm_mode_status
> -intel_dp_mode_valid(struct drm_connector *_connector,
> -		    const struct drm_display_mode *mode)
> +intel_dp_mode_valid_format(struct intel_connector *connector,
> +			   const struct drm_display_mode *mode,
> +			   int target_clock,
> +			   enum intel_output_format sink_format)
>   {
> -	struct intel_display *display = to_intel_display(_connector->dev);
> -	struct intel_connector *connector = to_intel_connector(_connector);
> +	struct intel_display *display = to_intel_display(connector);
>   	struct intel_dp *intel_dp = intel_attached_dp(connector);
> -	enum intel_output_format sink_format, output_format;
> -	const struct drm_display_mode *fixed_mode;
> -	int target_clock = mode->clock;
> +	enum intel_output_format output_format;
>   	int max_rate, mode_rate, max_lanes, max_link_clock;
>   	u16 dsc_max_compressed_bpp = 0;
>   	enum drm_mode_status status;
> @@ -1488,29 +1452,6 @@ intel_dp_mode_valid(struct drm_connector *_connector,
>   	int num_joined_pipes;
>   	int link_bpp_x16;
>   
> -	status = intel_cpu_transcoder_mode_valid(display, mode);
> -	if (status != MODE_OK)
> -		return status;
> -
> -	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> -		return MODE_H_ILLEGAL;
> -
> -	if (mode->clock < 10000)
> -		return MODE_CLOCK_LOW;
> -
> -	if (intel_dp_hdisplay_bad(display, mode->hdisplay))
> -		return MODE_H_ILLEGAL;
> -
> -	fixed_mode = intel_panel_fixed_mode(connector, mode);
> -	if (intel_dp_is_edp(intel_dp) && fixed_mode) {
> -		status = intel_panel_mode_valid(connector, mode);
> -		if (status != MODE_OK)
> -			return status;
> -
> -		target_clock = fixed_mode->clock;
> -	}
> -
> -	sink_format = intel_dp_sink_format(connector, mode);
>   	output_format = intel_dp_output_format(connector, sink_format);
>   
>   	max_link_clock = intel_dp_max_link_rate(intel_dp);
> @@ -1518,7 +1459,8 @@ intel_dp_mode_valid(struct drm_connector *_connector,
>   
>   	max_rate = intel_dp_max_link_data_rate(intel_dp, max_link_clock, max_lanes);
>   
> -	link_bpp_x16 = intel_dp_mode_min_link_bpp_x16(connector, mode);
> +	link_bpp_x16 = intel_dp_mode_min_link_bpp_x16(connector, mode,
> +						      output_format);
>   	mode_rate = intel_dp_link_required(max_link_clock, max_lanes,
>   					   target_clock, mode->hdisplay,
>   					   link_bpp_x16, 0);
> @@ -1608,7 +1550,70 @@ intel_dp_mode_valid(struct drm_connector *_connector,
>   	if (status != MODE_OK)
>   		return status;
>   
> -	return intel_dp_mode_valid_downstream(connector, mode, target_clock);
> +	return intel_dp_mode_valid_downstream(connector, mode,
> +					      target_clock, sink_format);
> +}
> +
> +static enum drm_mode_status
> +intel_dp_mode_valid(struct drm_connector *_connector,
> +		    const struct drm_display_mode *mode)
> +{
> +	struct intel_display *display = to_intel_display(_connector->dev);
> +	struct intel_connector *connector = to_intel_connector(_connector);
> +	const struct drm_display_info *info = &connector->base.display_info;
> +	struct intel_dp *intel_dp = intel_attached_dp(connector);
> +	const struct drm_display_mode *fixed_mode;
> +	int target_clock = mode->clock;
> +	enum drm_mode_status status;
> +
> +	status = intel_cpu_transcoder_mode_valid(display, mode);
> +	if (status != MODE_OK)
> +		return status;
> +
> +	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> +		return MODE_H_ILLEGAL;
> +
> +	if (mode->clock < 10000)
> +		return MODE_CLOCK_LOW;
> +
> +	if (intel_dp_hdisplay_bad(display, mode->hdisplay))
> +		return MODE_H_ILLEGAL;
> +
> +	fixed_mode = intel_panel_fixed_mode(connector, mode);
> +	if (intel_dp_is_edp(intel_dp) && fixed_mode) {
> +		status = intel_panel_mode_valid(connector, mode);
> +		if (status != MODE_OK)
> +			return status;
> +
> +		target_clock = fixed_mode->clock;
> +	}
> +
> +	/*
> +	 * TODO: Even when using a 4:2:0 sink_format intel_dp_output_format()
> +	 * will always choose a 4:4:4 output_format if the DFP can do the
> +	 * 4:4:4->4:2:0 conversion for us. Thus a mode may still be rejected
> +	 * if we only have enough DP link bandwidth for 4:2:0 but not for
> +	 * 4:4:4. Another attempt with an explicit 4:2:0 output_format might
> +	 * be needed here. intel_dp_compute_config() would need the same
> +	 * logic, or else the actual modeset would still fail.
> +	 *
> +	 * Also a lot of the checks only depend on output_format but not
> +	 * sink_format, so we are potentially doing redundant work by
> +	 * testing the same output_format for two different sink_formats.
> +	 */
> +	if (drm_mode_is_420_only(info, mode)) {
> +		status = intel_dp_mode_valid_format(connector, mode, target_clock,
> +						    INTEL_OUTPUT_FORMAT_YCBCR420);
> +	} else {
> +		status = intel_dp_mode_valid_format(connector, mode, target_clock,
> +						    INTEL_OUTPUT_FORMAT_RGB);
> +
> +		if (status != MODE_OK && drm_mode_is_420_also(info, mode))
> +			status = intel_dp_mode_valid_format(connector, mode, target_clock,
> +							    INTEL_OUTPUT_FORMAT_YCBCR420);
> +	}
> +
> +	return status;
>   }
>   
>   bool intel_dp_source_supports_tps3(struct intel_display *display)