On 3/11/25 6:12 PM, Maxime Ripard wrote:
> On Tue, Mar 11, 2025 at 12:57:37PM +0200, Cristian Ciocaltea wrote:
>> Introduce a few macros to facilitate setting custom (i.e. non-default)
>> EDID data during connector initialization.
>>
>> This helps reducing boilerplate code while also drops some redundant
>> calls to set_connector_edid().
>>
>> Signed-off-by: Cristian Ciocaltea <cristian.ciocal...@collabora.com>
>> ---
>> drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c | 245
>> ++++++++-------------
>> 1 file changed, 93 insertions(+), 152 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
>> e97efd3af9ed18e6cf8ee66b4923dfc805b34e19..a3f7f3ce31c73335c2c2643bdc5395b6ceb6f071
>> 100644
>> --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
>> +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
>> @@ -183,10 +183,12 @@ static const struct drm_connector_funcs
>> dummy_connector_funcs = {
>>
>> static
>> struct drm_atomic_helper_connector_hdmi_priv *
>> -drm_kunit_helper_connector_hdmi_init_funcs(struct kunit *test,
>> - unsigned int formats,
>> - unsigned int max_bpc,
>> - const struct
>> drm_connector_hdmi_funcs *hdmi_funcs)
>> +connector_hdmi_init_funcs_set_edid(struct kunit *test,
>> + unsigned int formats,
>> + unsigned int max_bpc,
>> + const struct drm_connector_hdmi_funcs
>> *hdmi_funcs,
>> + const char *edid_data,
>> + size_t edid_len)
>> {
>> struct drm_atomic_helper_connector_hdmi_priv *priv;
>> struct drm_connector *conn;
>> @@ -240,30 +242,27 @@ drm_kunit_helper_connector_hdmi_init_funcs(struct
>> kunit *test,
>>
>> drm_mode_config_reset(drm);
>>
>> + if (edid_data && edid_len) {
>> + ret = set_connector_edid(test, &priv->connector, edid_data,
>> edid_len);
>> + KUNIT_ASSERT_GT(test, ret, 0);
>> + }
>> +
>> return priv;
>> }
>>
>> -static
>> -struct drm_atomic_helper_connector_hdmi_priv *
>> -drm_kunit_helper_connector_hdmi_init(struct kunit *test,
>> - unsigned int formats,
>> - unsigned int max_bpc)
>> -{
>> - struct drm_atomic_helper_connector_hdmi_priv *priv;
>> - int ret;
>> +#define drm_kunit_helper_connector_hdmi_init_funcs_set_edid(test, formats,
>> max_bpc, funcs, edid) \
>> + connector_hdmi_init_funcs_set_edid(test, formats, max_bpc, funcs, edid,
>> ARRAY_SIZE(edid))
>>
>> - priv = drm_kunit_helper_connector_hdmi_init_funcs(test,
>> - formats, max_bpc,
>> -
>> &dummy_connector_hdmi_funcs);
>> - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv);
>> +#define drm_kunit_helper_connector_hdmi_init_funcs(test, formats, max_bpc,
>> funcs) \
>> + connector_hdmi_init_funcs_set_edid(test, formats, max_bpc, funcs, NULL,
>> 0)
>>
>> - ret = set_connector_edid(test, &priv->connector,
>> - test_edid_hdmi_1080p_rgb_max_200mhz,
>> -
>> ARRAY_SIZE(test_edid_hdmi_1080p_rgb_max_200mhz));
>> - KUNIT_ASSERT_GT(test, ret, 0);
>> +#define drm_kunit_helper_connector_hdmi_init_set_edid(test, formats,
>> max_bpc, edid) \
>> + drm_kunit_helper_connector_hdmi_init_funcs_set_edid(test, formats,
>> max_bpc, \
>> +
>> &dummy_connector_hdmi_funcs, edid)
>>
>> - return priv;
>> -}
>> +#define drm_kunit_helper_connector_hdmi_init(test, formats, max_bpc)
>> \
>> + drm_kunit_helper_connector_hdmi_init_set_edid(test, formats, max_bpc,
>> \
>> +
>> test_edid_hdmi_1080p_rgb_max_200mhz)
>
> I'd really prefer to have functions to macros here. They are easier to
> read, extend, and don't have any particular drawbacks.
Yeah, the main reason I opted for macros was to allow dropping
ARRAY_SIZE(edid) from the caller side, hence making the API as simple as
possible.
> I also don't think we need that many, looking at the tests:
>
> - We need drm_kunit_helper_connector_hdmi_init() to setup a connector
> with test_edid_hdmi_1080p_rgb_max_200mhz and
> dummy_connector_hdmi_funcs()
Correct.
> - We need to create a
> drm_kunit_helper_connector_hdmi_init_with_edid_funcs() to pass both
> the funcs and edid pointers
That's drm_kunit_helper_connector_hdmi_init_funcs_set_edid(), but I can
rename it if you prefer - I've just tried to keep the name length under
control, as there are already some indentation challenges when calling
the helpers.
Currently it's only used to implement
drm_kunit_helper_connector_hdmi_init_set_edid() by passing
&dummy_connector_hdmi_funcs, but there are a few test cases that can
benefit of it and help extend the cleanup - will do for v3.
drm_kunit_helper_connector_hdmi_init_set_edid() should also stay as it's
already being used to drop boilerplate code from a lot of places.
> And that's it, right?
There's also drm_kunit_helper_connector_hdmi_init_funcs() which has been
used for a few times, but it can be further optimized out via
drm_kunit_helper_connector_hdmi_init_funcs_set_edid(), so I'll drop it.
That means we could end up with just the following:
- drm_kunit_helper_connector_hdmi_init()
- drm_kunit_helper_connector_hdmi_init_set_edid()
- drm_kunit_helper_connector_hdmi_init_funcs_set_edid()
>> /*
>> * Test that if we change the RGB quantization property to a different
>> @@ -771,19 +770,15 @@ static void
>> drm_test_check_output_bpc_crtc_mode_changed(struct kunit *test)
>> struct drm_crtc *crtc;
>> int ret;
>>
>> - priv = drm_kunit_helper_connector_hdmi_init(test,
>> - BIT(HDMI_COLORSPACE_RGB),
>> - 10);
>> + priv = drm_kunit_helper_connector_hdmi_init_set_edid(test,
>> + BIT(HDMI_COLORSPACE_RGB),
>> + 10,
>> + test_edid_hdmi_1080p_rgb_yuv_dc_max_200mhz);
>
> I think that convertion should be part of another patch.
Ack, will move all conversions to a dedicated patch.
Thanks,
Cristian
