Re: [PATCH v4 23/23] drm/tests: hdmi: Add test for unsupported RGB/YUV420 mode

[Maxime Ripard <mripard@kernel.org>]

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

On Thu, May 22, 2025 at 08:37:31PM +0300, Cristian Ciocaltea wrote:
> Hi Maxime,
> 
> On 5/22/25 7:16 PM, Maxime Ripard wrote:
> > Hi,
> > 
> > On Mon, May 19, 2025 at 01:55:10PM +0300, Cristian Ciocaltea wrote:
> >> On 5/19/25 11:42 AM, Maxime Ripard wrote:
> >>> Hi,
> >>>
> >>> On Fri, Apr 25, 2025 at 01:27:14PM +0300, Cristian Ciocaltea wrote:
> >>>> Provide a test to verify that if both driver and screen support RGB and
> >>>> YUV420 formats, drm_atomic_helper_connector_hdmi_check() cannot succeed
> >>>> when trying to set a mode unsupported by the display.
> >>>>
> >>>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
> >>>> ---
> >>>>  drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 66 ++++++++++++++++++++++
> >>>>  1 file changed, 66 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> >>>> index d79084cfb516b69c4244098c0767d604ad02f2c3..6337a1c52b86810c638f446c4995e7ee63dbc084 100644
> >>>> --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> >>>> +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> >>>> @@ -1622,6 +1622,71 @@ static void drm_test_check_driver_unsupported_fallback_yuv420(struct kunit *test
> >>>>  	drm_modeset_acquire_fini(&ctx);
> >>>>  }
> >>>>  
> >>>> +/*
> >>>> + * Test that if a driver and screen supports RGB and YUV420 formats, but the
> >>>> + * chosen mode cannot be supported by the screen, we end up with unsuccessful
> >>>> + * fallback attempts.
> >>>> + */
> >>>> +static void drm_test_check_display_unsupported_fallback_rgb_yuv420(struct kunit *test)
> >>>> +{
> >>>> +	struct drm_atomic_helper_connector_hdmi_priv *priv;
> >>>> +	struct drm_modeset_acquire_ctx ctx;
> >>>> +	struct drm_crtc_state *crtc_state;
> >>>> +	struct drm_atomic_state *state;
> >>>> +	struct drm_display_info *info;
> >>>> +	struct drm_display_mode *preferred, *unsupported_mode;
> >>>> +	struct drm_connector *conn;
> >>>> +	struct drm_device *drm;
> >>>> +	struct drm_crtc *crtc;
> >>>> +	int ret;
> >>>> +
> >>>> +	priv = drm_kunit_helper_connector_hdmi_init_with_edid_funcs(test,
> >>>> +				BIT(HDMI_COLORSPACE_RGB) |
> >>>> +				BIT(HDMI_COLORSPACE_YUV420),
> >>>> +				10,
> >>>> +				&dummy_connector_hdmi_funcs,
> >>>> +				test_edid_hdmi_4k_rgb_yuv420_dc_max_340mhz);
> >>>> +	KUNIT_ASSERT_NOT_NULL(test, priv);
> >>>> +
> >>>> +	drm = &priv->drm;
> >>>> +	crtc = priv->crtc;
> >>>> +	conn = &priv->connector;
> >>>> +	info = &conn->display_info;
> >>>> +	KUNIT_ASSERT_TRUE(test, info->is_hdmi);
> >>>> +	KUNIT_ASSERT_TRUE(test, conn->ycbcr_420_allowed);
> >>>> +
> >>>> +	preferred = find_preferred_mode(conn);
> >>>> +	KUNIT_ASSERT_NOT_NULL(test, preferred);
> >>>> +
> >>>> +	unsupported_mode = drm_kunit_display_mode_from_cea_vic(test, drm, 96);
> >>>> +	KUNIT_ASSERT_NOT_NULL(test, unsupported_mode);
> >>>
> >>> I'm not sure what this one is supposed to test. If the mode is
> >>> unsupported by the screen, it will be for both YUV and RGB, right? So
> >>> what are we testing here?
> >>
> >> That would be the case suggested at [1]:
> >>
> >> "We still need to do the same with a driver that supports both, but the
> >> monitor doesn't."
> >>
> >> Should we drop it?
> > 
> > Ah, I see. I meant that we should normally end up with YUV420 (so mode
> > is supported by the monitor, but the resolution is too high for RGB and
> > we should pick YUV instead), but the monitor doesn't support it and thus
> > we fail.
> 
> If I get it right, to verify this scenario we'd need a new test EDID
> that basically advertises an RGB mode which is not actually supported by
> the screen, i.e. the mode requires a TMDS rate which exceeds the maximum
> supported by the display for any of the available color depths.
> 
> But that's not something we should encounter in practice, is it?  I mean
> the EDID would be wrong, as it doesn't match the hardware capabilities.
> 
> Regardless, assuming this is solely for the purpose of testing the
> framework, I will try to come up with a test case.  The only problem
> is generating the EDID - which is a really annoying process, as I've
> already mentioned a while ago [1].  
> 
> Hence I'm wondering if we could move on without it for the moment (I'll
> get back to it ASAP).

Oh, right, you're correct, I think we don't need to care, and we can
just skip that patch.

Maxime

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