Re: [PATCH v5 06/17] drm/display: hdmi-state-helper: Try subsampling in mode_valid

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3929 bytes --]

On Thu, Dec 11, 2025 at 08:59:14PM +0100, Nicolas Frattaroli wrote:
> On Tuesday, 9 December 2025 15:18:25 Central European Standard Time Maxime Ripard wrote:
> > On Fri, Nov 28, 2025 at 10:05:42PM +0100, Nicolas Frattaroli wrote:
> > > drm_hdmi_connector_mode_valid assumes modes are only valid if they work
> > > with RGB. The reality is more complex however: YCbCr 4:2:0
> > > chroma-subsampled modes only require half the pixel clock that the same
> > > mode would require in RGB.
> > > 
> > > This leads to drm_hdmi_connector_mode_valid rejecting perfectly valid
> > > 420-only modes.
> > > 
> > > Fix this by checking whether the mode is 420-only first. If so, then
> > > proceed by checking it with HDMI_COLORSPACE_YUV420 so long as the
> > > connector has legalized 420, otherwise error out. If the mode is not
> > > 420-only, check with RGB as was previously always the case.
> > > 
> > > Fixes: 47368ab437fd ("drm/display: hdmi: add generic mode_valid helper")
> > > Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> > > ---
> > >  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 12 +++++++++++-
> > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> > > index 5da956bdd68c..1800e00b30c5 100644
> > > --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> > > +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> > > @@ -892,8 +892,18 @@ drm_hdmi_connector_mode_valid(struct drm_connector *connector,
> > >  			      const struct drm_display_mode *mode)
> > >  {
> > >  	unsigned long long clock;
> > > +	enum hdmi_colorspace fmt;
> > > +
> > > +	if (drm_mode_is_420_only(&connector->display_info, mode)) {
> > > +		if (connector->ycbcr_420_allowed)
> > > +			fmt = HDMI_COLORSPACE_YUV420;
> > > +		else
> > > +			return MODE_NO_420;
> > > +	} else {
> > > +		fmt = HDMI_COLORSPACE_RGB;
> > > +	}
> > >  
> > > -	clock = drm_hdmi_compute_mode_clock(mode, 8, HDMI_COLORSPACE_RGB);
> > > +	clock = drm_hdmi_compute_mode_clock(mode, 8, fmt);
> > 
> > I agree on principle, but we need to have a test for this.
> 
> I'd like to change `drm_mode_is_420_only` to `drm_mode_is_420` in
> the next revision, and modify the control flow to work correctly
> in this case, because rejecting 420-also modes on the basis that
> we can't do them in RGB isn't correct either.
> 
> But my concern with adding yet more tests is that I found this bug
> in a function unrelated to the series while adding tests you asked
> for, because the tests relied on this function to not be broken as
> part of the test setup. Yes, I was not be able to get any 4:2:0
> modes on the test connector in the kunit tests because
> drm_hdmi_connector_mode_valid helpfully discarded all of them.
> 
> So now I am wondering whether adding yet more tests will uncover
> more bugs in functions unrelated to implementing the "color format"
> property, that were only called because the new test required them
> to set up some test fixture. And then I have to add another fix and
> another test to this series, rinse and repeat.
>
> Can we just agree that I am not going to expand the scope of this
> series any further? If you want me to send a follow-up series that
> adds tests to some of the hdmi state helper functions, then I can
> do that, but I don't want to do it as a precondition for the 17
> other patches in this series to get merged.

But it is a precondition. See

https://docs.kernel.org/gpu/drm-internals.html#kunit-coverage-rules

You're adding code to a port of DRM that is covered by tests already,
and are fixing a hole in that test coverage. We must add a test to close
that hole too.

Now, if you want to take that fix out of your series and work on those
tests, fine, but we'll still need them.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]