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.
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.
Maxime
Attachment:
signature.asc
Description: PGP signature
