Re: [PATCH v3 13/15] drm/tests: hdmi: Add limited range tests for YUV420 mode

[Cristian Ciocaltea <cristian.ciocaltea@collabora.com>]

On 4/10/25 10:18 AM, Maxime Ripard wrote:
> On Wed, Mar 26, 2025 at 12:20:02PM +0200, Cristian Ciocaltea wrote:
>> Provide tests to verify that drm_atomic_helper_connector_hdmi_check()
>> helper behaviour when using YUV420 output format is to always set the
>> limited RGB quantization range to 'limited', no matter what the value of
>> Broadcast RGB property is.
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
>> ---
>>  drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c |  89 +++++++++++++++-
>>  drivers/gpu/drm/tests/drm_kunit_edid.h             | 112 +++++++++++++++++++++
>>  2 files changed, 196 insertions(+), 5 deletions(-)
>>
>> 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 6897515189a0649a267196b246944efc92ace336..3fae7ccf65309a1d8a4acf12de961713b9163096 100644
>> --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
>> +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
>> @@ -731,6 +731,88 @@ static void drm_test_check_broadcast_rgb_limited_cea_mode_vic_1(struct kunit *te
>>  	drm_modeset_acquire_fini(&ctx);
>>  }
>>  
>> +/*
>> + * Test that for an HDMI connector, with an HDMI monitor, we will
>> + * get a limited RGB Quantization Range with a YUV420 mode, no
>> + * matter what the value of the Broadcast RGB property is set to.
>> + */
>> +static void drm_test_check_broadcast_rgb_cea_mode_yuv420(struct kunit *test)
>> +{
>> +	struct drm_atomic_helper_connector_hdmi_priv *priv;
>> +	enum drm_hdmi_broadcast_rgb broadcast_rgb;
>> +	struct drm_modeset_acquire_ctx ctx;
>> +	struct drm_connector_state *conn_state;
>> +	struct drm_atomic_state *state;
>> +	struct drm_display_mode *mode;
>> +	struct drm_connector *conn;
>> +	struct drm_device *drm;
>> +	struct drm_crtc *crtc;
>> +	int ret;
>> +
>> +	broadcast_rgb = *(enum drm_hdmi_broadcast_rgb *)test->param_value;
>> +
>> +	priv = drm_kunit_helper_connector_hdmi_init_with_edid(test,
>> +				BIT(HDMI_COLORSPACE_RGB) |
>> +				BIT(HDMI_COLORSPACE_YUV420),
>> +				8,
>> +				test_edid_hdmi_1080p_rgb_yuv_4k_yuv420_dc_max_200mhz);
>> +	KUNIT_ASSERT_NOT_NULL(test, priv);
>> +
>> +	drm = &priv->drm;
>> +	crtc = priv->crtc;
>> +	conn = &priv->connector;
>> +	KUNIT_ASSERT_TRUE(test, conn->display_info.is_hdmi);
>> +
>> +	mode = drm_kunit_display_mode_from_cea_vic(test, drm, 95);
>> +	KUNIT_ASSERT_NOT_NULL(test, mode);
>> +
>> +	drm_modeset_acquire_init(&ctx, 0);
>> +
>> +	ret = drm_kunit_helper_enable_crtc_connector(test, drm,
>> +						     crtc, conn,
>> +						     mode, &ctx);
>> +	KUNIT_ASSERT_EQ(test, ret, 0);
> 
> drm_kunit_helper_enable_crtc_connector() can return EDEADLK, so you need
> to handle it and restart the sequence if it happens.

Right, there are actually many users of the helper since

6a5c0ad7e08e ("drm/tests: hdmi_state_helpers: Switch to new helper")

Probably a stupid question, as I haven't checked which are the mandatory
operations of the restart sequence, but I wonder if this could be
handled inside the helper instead of trying to fix all test cases.
>> +	state = drm_kunit_helper_atomic_state_alloc(test, drm, &ctx);
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
>> +
>> +	conn_state = drm_atomic_get_connector_state(state, conn);
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state);
> 
> Ditto.
> 
>> +	conn_state->hdmi.broadcast_rgb = broadcast_rgb;
>> +
>> +	ret = drm_atomic_check_only(state);
>> +	KUNIT_ASSERT_EQ(test, ret, 0);
>> +
>> +	conn_state = drm_atomic_get_connector_state(state, conn);
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state);
> 
> Ditto, but I'm not sure you need drm_atomic_get_connector_state() here.
> We know at this point that the state is there and we don't need to
> allocate it anymore. drm_atomic_get_new_connector_state() will probably
> be enough 

Will check.

> and that one can't return EDEADLK.

Same question as above, could we handle EDEADLK at helper(s) level to
avoid open coding the restart sequence?

Thanks,
Cristian