On 3/19/25 5:32 PM, Maxime Ripard wrote:
> On Wed, Mar 12, 2025 at 12:44:26AM +0200, Cristian Ciocaltea wrote:
>> 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.ciocaltea@xxxxxxxxxxxxx>
>>>> ---
>>>> 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()
>
> I'm not even sure we need init_set_edid. We just have to provide the
> dummy_connector_hdmi_funcs pointer to
> drm_kunit_helper_connector_hdmi_init_funcs_set_edid and that's it.
Right, it can be done, but my point is that there are currently ~20 test
cases that benefit from this simplification, and the number is likely to
grow while extending the test suite.
I would rename it to drm_kunit_helper_connector_hdmi_init_with_edid(),
to be consistent with your preference below, but yeah, if you're still
not convinced we should keep the helper, pls let me know and I'll drop it.
> And yeah, I'd really prefer to use
> drm_kunit_helper_connector_hdmi_init_with_edid_funcs() to remain
> consistent with the rest of KMS.
Sure, will do.
Thanks,
Cristian
