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

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

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.

> 
> Maxime
> 





_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip