Re: [PATCH 2/9] drm/i915/hdmi: Restructure the sink/output format selection

[Nicolas Frattaroli <nicolas.frattaroli@collabora.com>]

On Tuesday, 31 March 2026 01:53:32 Central European Summer Time Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> intel_hdmi_compute_output_format() is a bit of a mess. Try to
> restructure it into a more readable form.
> 
> Right now we basically have two main code paths:
> - YCbCr 4:2:0 only modes
> - everything else including YCbCr 4:2:0 also modes
> 
> Those two basically do the same two steps (try 4:2:0 and try 4:4:4)
> but in opposite orders. Let's write that out in a more explicit
> if-else form. And since I'm running out of function names I'll
> rename the function with that high level logic into
> intel_hdmi_compute_formats() and it will call (the new) with
> intel_hdmi_compute_output_format() with an explicit sink_format
> as needed.
> 
> Cc: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_hdmi.c | 112 ++++++++++++++--------
>  1 file changed, 70 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index 072b0554cc24..16873fc7bcb9 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2021,6 +2021,30 @@ intel_hdmi_mode_clock_valid(struct drm_connector *_connector, int clock,
>  	return status;
>  }
>  
> +static enum drm_mode_status
> +intel_hdmi_sink_format_valid(struct intel_connector *connector,
> +			     const struct drm_display_mode *mode,
> +			     bool has_hdmi_sink,
> +			     enum intel_output_format sink_format)
> +{
> +	const struct drm_display_info *info = &connector->base.display_info;
> +
> +	switch (sink_format) {
> +	case INTEL_OUTPUT_FORMAT_YCBCR420:
> +		if (!has_hdmi_sink ||
> +		    !connector->base.ycbcr_420_allowed ||
> +		    !drm_mode_is_420(info, mode))
> +			return MODE_NO_420;
> +
> +		return MODE_OK;
> +	case INTEL_OUTPUT_FORMAT_RGB:
> +		return MODE_OK;
> +	default:
> +		MISSING_CASE(sink_format);
> +		return MODE_BAD;
> +	}
> +}

I think this here is missing INTEL_OUTPUT_FORMAT_YCBCR444 as a case.
The following diff adding it for both intel_hdmi_sink_format_valid
and intel_hdmi_sink_bpc_possible that I quickly whipped up may be the
way to go, but I'm unsure about the interaction between the two:

---
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index 6bc1689cbf93..38b9924ebef0 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -1966,6 +1966,8 @@ static bool intel_hdmi_sink_bpc_possible(struct drm_connector *_connector,
 
                if (sink_format == INTEL_OUTPUT_FORMAT_YCBCR420)
                        return hdmi->y420_dc_modes & DRM_EDID_YCBCR420_DC_36;
+               else if (sink_format == INTEL_OUTPUT_FORMAT_YCBCR444)
+                       return info->edid_hdmi_ycbcr444_dc_modes & DRM_EDID_HDMI_DC_36;
                else
                        return info->edid_hdmi_rgb444_dc_modes & DRM_EDID_HDMI_DC_36;
        case 10:
@@ -1974,6 +1976,8 @@ static bool intel_hdmi_sink_bpc_possible(struct drm_connector *_connector,
 
                if (sink_format == INTEL_OUTPUT_FORMAT_YCBCR420)
                        return hdmi->y420_dc_modes & DRM_EDID_YCBCR420_DC_30;
+               else if (sink_format == INTEL_OUTPUT_FORMAT_YCBCR444)
+                       return info->edid_hdmi_ycbcr444_dc_modes & DRM_EDID_HDMI_DC_30;
                else
                        return info->edid_hdmi_rgb444_dc_modes & DRM_EDID_HDMI_DC_30;
        case 8:
@@ -2038,6 +2042,11 @@ intel_hdmi_sink_format_valid(struct intel_connector *connector,
 
                return MODE_OK;
        case INTEL_OUTPUT_FORMAT_RGB:
+               return MODE_OK;
+       case INTEL_OUTPUT_FORMAT_YCBCR444:
+               if (!(info->color_formats & BIT(DRM_OUTPUT_COLOR_FORMAT_YCBCR444)))
+                       return MODE_BAD;
+
                return MODE_OK;
        default:
                MISSING_CASE(sink_format);
---

It does work for me though at YCBCr 4:4:4 @ 12bpc, 10bpc and 8bpc, so both info
members this relies on are populated correctly.

> +
>  static enum drm_mode_status
>  intel_hdmi_mode_valid(struct drm_connector *_connector,
>  		      const struct drm_display_mode *mode)
> @@ -2246,20 +2270,6 @@ static bool intel_hdmi_has_audio(struct intel_encoder *encoder,
>  		return intel_conn_state->force_audio == HDMI_AUDIO_ON;
>  }
>  
> -static enum intel_output_format
> -intel_hdmi_sink_format(const struct intel_crtc_state *crtc_state,
> -		       struct intel_connector *connector,
> -		       bool ycbcr_420_output)
> -{
> -	if (!crtc_state->has_hdmi_sink)
> -		return INTEL_OUTPUT_FORMAT_RGB;
> -
> -	if (connector->base.ycbcr_420_allowed && ycbcr_420_output)
> -		return INTEL_OUTPUT_FORMAT_YCBCR420;
> -	else
> -		return INTEL_OUTPUT_FORMAT_RGB;
> -}
> -
>  static enum intel_output_format
>  intel_hdmi_output_format(const struct intel_crtc_state *crtc_state)
>  {
> @@ -2268,37 +2278,55 @@ intel_hdmi_output_format(const struct intel_crtc_state *crtc_state)
>  
>  static int intel_hdmi_compute_output_format(struct intel_encoder *encoder,
>  					    struct intel_crtc_state *crtc_state,
> -					    const struct drm_connector_state *conn_state,
> -					    bool respect_downstream_limits)
> +					    struct intel_connector *connector,
> +					    bool respect_downstream_limits,
> +					    enum intel_output_format sink_format)
>  {
> -	struct intel_display *display = to_intel_display(encoder);
> -	struct intel_connector *connector = to_intel_connector(conn_state->connector);
>  	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
> -	const struct drm_display_info *info = &connector->base.display_info;
> -	bool ycbcr_420_only = drm_mode_is_420_only(info, adjusted_mode);
> -	int ret;
>  
> -	crtc_state->sink_format =
> -		intel_hdmi_sink_format(crtc_state, connector, ycbcr_420_only);
> -
> -	if (ycbcr_420_only && crtc_state->sink_format != INTEL_OUTPUT_FORMAT_YCBCR420) {
> -		drm_dbg_kms(display->drm,
> -			    "YCbCr 4:2:0 mode but YCbCr 4:2:0 output not possible. Falling back to RGB.\n");
> -		crtc_state->sink_format = INTEL_OUTPUT_FORMAT_RGB;
> -	}
> +	if (intel_hdmi_sink_format_valid(connector, adjusted_mode,
> +					 crtc_state->has_hdmi_sink, sink_format) != MODE_OK)
> +		return -EINVAL;
>  
> +	crtc_state->sink_format = sink_format;
>  	crtc_state->output_format = intel_hdmi_output_format(crtc_state);
> -	ret = intel_hdmi_compute_clock(encoder, crtc_state, respect_downstream_limits);
> -	if (ret) {
> -		if (crtc_state->sink_format == INTEL_OUTPUT_FORMAT_YCBCR420 ||
> -		    !crtc_state->has_hdmi_sink ||
> -		    !connector->base.ycbcr_420_allowed ||
> -		    !drm_mode_is_420_also(info, adjusted_mode))
> -			return ret;
> -
> -		crtc_state->sink_format = INTEL_OUTPUT_FORMAT_YCBCR420;
> -		crtc_state->output_format = intel_hdmi_output_format(crtc_state);
> -		ret = intel_hdmi_compute_clock(encoder, crtc_state, respect_downstream_limits);
> +
> +	return intel_hdmi_compute_clock(encoder, crtc_state, respect_downstream_limits);
> +}
> +
> +static int intel_hdmi_compute_formats(struct intel_encoder *encoder,
> +				      struct intel_crtc_state *crtc_state,
> +				      const struct drm_connector_state *conn_state,
> +				      bool respect_downstream_limits)
> +{
> +	struct intel_display *display = to_intel_display(encoder);
> +	struct intel_connector *connector = to_intel_connector(conn_state->connector);
> +	const struct drm_display_mode *adjusted_mode = &crtc_state->hw.adjusted_mode;
> +	const struct drm_display_info *info = &connector->base.display_info;
> +	int ret;
> +
> +	if (drm_mode_is_420_only(info, adjusted_mode)) {
> +		ret = intel_hdmi_compute_output_format(encoder, crtc_state, connector,
> +						       respect_downstream_limits,
> +						       INTEL_OUTPUT_FORMAT_YCBCR420);
> +
> +		if (ret) {
> +			drm_dbg_kms(display->drm,
> +				    "YCbCr 4:2:0 mode but YCbCr 4:2:0 output not possible. Falling back to RGB.\n");
> +
> +			ret = intel_hdmi_compute_output_format(encoder, crtc_state, connector,
> +							       respect_downstream_limits,
> +							       INTEL_OUTPUT_FORMAT_RGB);
> +		}
> +	} else {
> +		ret = intel_hdmi_compute_output_format(encoder, crtc_state, connector,
> +						       respect_downstream_limits,
> +						       INTEL_OUTPUT_FORMAT_RGB);
> +
> +		if (ret && drm_mode_is_420_also(info, adjusted_mode))
> +			ret = intel_hdmi_compute_output_format(encoder, crtc_state, connector,
> +							       respect_downstream_limits,
> +							       INTEL_OUTPUT_FORMAT_YCBCR420);
>  	}
>  
>  	return ret;
> @@ -2375,9 +2403,9 @@ int intel_hdmi_compute_config(struct intel_encoder *encoder,
>  	 * Try to respect downstream TMDS clock limits first, if
>  	 * that fails assume the user might know something we don't.
>  	 */
> -	ret = intel_hdmi_compute_output_format(encoder, pipe_config, conn_state, true);
> +	ret = intel_hdmi_compute_formats(encoder, pipe_config, conn_state, true);
>  	if (ret)
> -		ret = intel_hdmi_compute_output_format(encoder, pipe_config, conn_state, false);
> +		ret = intel_hdmi_compute_formats(encoder, pipe_config, conn_state, false);
>  	if (ret) {
>  		drm_dbg_kms(display->drm,
>  			    "unsupported HDMI clock (%d kHz), rejecting mode\n",
>