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

Nautiyal, Ankit K
2026-04-06T05:27:41Z

On 3/31/2026 5:23 AM, 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

Nitpick : Extra 'with'.

Apart from above, the change looks good to me.

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

Regards,

Ankit

> 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;
> + }
> +}
> +
> 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",