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

Nautiyal, Ankit K
2026-04-06T05:26:18Z

On 3/31/2026 5:42 PM, Nicolas Frattaroli wrote:
> 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:

Hmm.. Trying YCBCR444 format, is not yet implemented.

We need to have a well defined policy as to when to use YCBCR444, and
that should be in the newly created intel_hdmi_compute_formats(),
perhaps it should be after we have tried and failed with RGB444.

(Although I am not sure in what case compute for RGB 444 will fail and
pass for YCBCR444).

However,this should be a separate patch/series.

Regards,

Ankit

>
> ---
> 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",
>>
>
>
>