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 <[email protected]>
> > > ---
> > > 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
signature.asc
Description: PGP signature
