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

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

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