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

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

On 3/31/2026 5:23 AM, 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.
>
> Cc: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_dp.c | 114 +++++++++++++-----------
>   1 file changed, 61 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 230b45acde29..86319bf09a19 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -1320,12 +1320,10 @@ intel_dp_tmds_clock_valid(struct intel_dp *intel_dp,
>   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;
>   
>   	/* If PCON supports FRL MODE, check FRL bandwidth constraints */
>   	if (intel_dp->dfp.pcon_max_frl_bw) {
> @@ -1350,25 +1348,9 @@ intel_dp_mode_valid_downstream(struct intel_connector *connector,
>   	    target_clock > intel_dp->dfp.max_dotclock)
>   		return MODE_CLOCK_HIGH;
>   
> -	sink_format = intel_dp_sink_format(connector, mode);
> -
>   	/* 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
> @@ -1464,15 +1446,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;
> @@ -1480,29 +1461,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);
> @@ -1600,7 +1558,57 @@ 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;
> +	}
> +
> +	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);
> +

Perhaps we can write  a comment or TODO here about the PCON special case 
which you mentioned:

that even though we are trying 420 sink output format, with PCON it is 
possible that RGB output format gets picked up (if Pcon supports color 
conversion).

In which case the rest of the mode validation will be wrt to RGB. Unless 
we handle fallback in intel_dp_output_format().


In any case, the patch LGTM.

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


> +		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)