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

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

On Monday, 6 April 2026 07:26:04 Central European Summer Time Nautiyal, Ankit K wrote:
> 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.

Makes sense. Instead of defining this policy for automatic selection,
I'll add the necessary code to handle it in my color format series,
where such an output format can be explicitly requested through a DRM
property. I don't think using it as a fallback in automatic selection
makes any sense: it saves no bandwidth over RGB444 and I'm pretty sure
HDMI mandates that anything supported in YCBCR444 is also supported in
RGB444.

Kind regards,
Nicolas Frattaroli

> 
> 
> Regards,
> 
> Ankit
> 
> 
> > [... snip ...]