Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] drm/tests: state leaks in drm_atomic_sro_compare KUnit tests - [Low] drm/tests: call to unexported drm_atomic_sro_state_get_device() --
commit f4486adadf6731a0cd596ded17489fc32e1af61a Author: Maxime Ripard <[email protected]> drm/tests: Add tests for the atomic state readout This commit adds a KUnit test suite for the DRM atomic state readout infrastructure, covering the drm_atomic_sro_state container API, state readout helpers, state comparison helpers, and initial state creation. > diff --git a/drivers/gpu/drm/tests/drm_atomic_state_readout_test.c > b/drivers/gpu/drm/tests/drm_atomic_state_readout_test.c > new file mode 100644 > index 0000000000000..ada512f7b32bb > --- /dev/null > +++ b/drivers/gpu/drm/tests/drm_atomic_state_readout_test.c [ ... ] > +static void drm_test_sro_state_alloc_free(struct kunit *test) > +{ > + struct sro_test_priv *priv = mock_device_with_sro_create(test); > + struct drm_atomic_sro_state *sro_state; > + > + sro_state = drm_atomic_sro_state_alloc(&priv->drm); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, sro_state); > + > + KUNIT_EXPECT_PTR_EQ(test, drm_atomic_sro_state_get_device(sro_state), > + &priv->drm); [Severity: Low] Will building with CONFIG_DRM_KUNIT_TEST=m fail at modpost here? In drivers/gpu/drm/drm_atomic_state_readout.c, drm_atomic_sro_state_get_device() is defined without EXPORT_SYMBOL(), unlike the other drm_atomic_sro_* functions: drivers/gpu/drm/drm_atomic_state_readout.c: struct drm_device * drm_atomic_sro_state_get_device(struct drm_atomic_sro_state *state) { return state->dev; } When drm_atomic_state_readout_test is built as a module under CONFIG_DRM_KUNIT_TEST=m, calling drm_atomic_sro_state_get_device() from drm_test_sro_state_alloc_free() results in an undefined symbol error. > + > + drm_atomic_sro_state_free(sro_state); > +} [ ... ] > +static void drm_test_sro_compare_plane_identical(struct kunit *test) > +{ > + struct sro_test_priv *priv = mock_device_with_sro_create(test); > + struct drm_plane_state *state1, *state2; > + struct drm_printer *p; > + bool identical; > + > + p = drm_kunit_helper_printer(test, LOGLEVEL_ERR); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, p); > + > + state1 = priv->plane->funcs->atomic_create_state(priv->plane); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state1); > + > + state2 = priv->plane->funcs->atomic_create_state(priv->plane); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state2); > + > + identical = drm_atomic_helper_plane_compare_state(priv->plane, p, > + state1, state2); > + KUNIT_EXPECT_TRUE(test, identical); > +} > + > +/* > + * Test that plane states differing by a single field compare as > + * different, and that the mismatching field is reported through the > + * printer. > + */ > +static void drm_test_sro_compare_plane_mismatch(struct kunit *test) > +{ > + struct sro_test_priv *priv = mock_device_with_sro_create(test); > + struct drm_plane_state *state1, *state2; > + struct drm_printer *p; > + bool identical; > + > + p = drm_kunit_helper_printer(test, LOGLEVEL_ERR); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, p); > + > + state1 = priv->plane->funcs->atomic_create_state(priv->plane); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state1); > + > + state2 = priv->plane->funcs->atomic_create_state(priv->plane); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state2); > + > + state2->crtc = priv->crtc; > + > + identical = drm_atomic_helper_plane_compare_state(priv->plane, p, > + state1, state2); > + KUNIT_EXPECT_FALSE(test, identical); > +} > + > +/* > + * Test that two identical active CRTC states compare as equal. > + */ > +static void drm_test_sro_compare_crtc_identical(struct kunit *test) > +{ > + struct sro_test_priv *priv = mock_device_with_sro_create(test); > + struct drm_crtc_state *state1, *state2; > + struct drm_printer *p; > + bool identical; > + > + p = drm_kunit_helper_printer(test, LOGLEVEL_ERR); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, p); > + > + state1 = priv->crtc->funcs->atomic_create_state(priv->crtc); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state1); > + > + state2 = priv->crtc->funcs->atomic_create_state(priv->crtc); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state2); > + > + state1->enable = true; > + state1->active = true; > + state2->enable = true; > + state2->active = true; > + > + identical = drm_atomic_helper_crtc_compare_state(priv->crtc, p, > + state1, state2); > + KUNIT_EXPECT_TRUE(test, identical); > +} > + > +/* > + * Test that two inactive CRTCs compare as equal even though their > + * modes differ: the modes are meaningless while inactive and must not > + * be compared. > + */ > +static void drm_test_sro_compare_crtc_inactive(struct kunit *test) > +{ > + struct sro_test_priv *priv = mock_device_with_sro_create(test); > + struct drm_display_mode *mode1, *mode2; > + struct drm_crtc_state *state1, *state2; > + struct drm_printer *p; > + bool identical; > + > + p = drm_kunit_helper_printer(test, LOGLEVEL_ERR); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, p); > + > + state1 = priv->crtc->funcs->atomic_create_state(priv->crtc); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state1); > + > + state2 = priv->crtc->funcs->atomic_create_state(priv->crtc); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state2); > + > + /* Two different but valid modes */ > + mode1 = drm_kunit_display_mode_from_cea_vic(test, &priv->drm, 16); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mode1); > + > + mode2 = drm_kunit_display_mode_from_cea_vic(test, &priv->drm, 4); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mode2); > + > + /* > + * Both CRTCs are inactive: the modes are meaningless and must > + * not be compared. > + */ > + drm_mode_copy(&state1->mode, mode1); > + drm_mode_copy(&state2->mode, mode2); > + > + identical = drm_atomic_helper_crtc_compare_state(priv->crtc, p, > + state1, state2); > + KUNIT_EXPECT_TRUE(test, identical); > +} > + > +/* > + * Test that an active CRTC and an inactive one compare as different. > + */ > +static void drm_test_sro_compare_crtc_active_mismatch(struct kunit *test) > +{ > + struct sro_test_priv *priv = mock_device_with_sro_create(test); > + struct drm_crtc_state *state1, *state2; > + struct drm_printer *p; > + bool identical; > + > + p = drm_kunit_helper_printer(test, LOGLEVEL_ERR); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, p); > + > + state1 = priv->crtc->funcs->atomic_create_state(priv->crtc); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state1); > + > + state2 = priv->crtc->funcs->atomic_create_state(priv->crtc); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state2); > + > + state1->enable = true; > + state1->active = true; > + > + identical = drm_atomic_helper_crtc_compare_state(priv->crtc, p, > + state1, state2); > + KUNIT_EXPECT_FALSE(test, identical); > +} > + > +/* > + * Test that two identical connector states compare as equal. > + */ > +static void drm_test_sro_compare_connector_identical(struct kunit *test) > +{ > + struct drm_connector *connector; > + struct sro_test_priv *priv; > + struct drm_connector_state *state1, *state2; > + struct drm_printer *p; > + bool identical; > + > + p = drm_kunit_helper_printer(test, LOGLEVEL_ERR); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, p); > + > + priv = mock_device_with_sro_create(test); > + connector = &priv->connector; > + > + state1 = connector->funcs->atomic_create_state(connector); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state1); > + > + state2 = connector->funcs->atomic_create_state(connector); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state2); > + > + identical = > + drm_atomic_helper_connector_compare_state(connector, p, > + state1, state2); > + KUNIT_EXPECT_TRUE(test, identical); > +} > + > +/* > + * Test that connector states whose CRTCs differ compare as different. > + */ > +static void drm_test_sro_compare_connector_mismatch(struct kunit *test) > +{ > + struct drm_connector *connector; > + struct sro_test_priv *priv; > + struct drm_connector_state *state1, *state2; > + struct drm_printer *p; > + bool identical; > + > + p = drm_kunit_helper_printer(test, LOGLEVEL_ERR); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, p); > + > + priv = mock_device_with_sro_create(test); > + connector = &priv->connector; > + > + state1 = connector->funcs->atomic_create_state(connector); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state1); > + > + state2 = connector->funcs->atomic_create_state(connector); > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state2); > + > + state2->crtc = priv->crtc; > + > + identical = > + drm_atomic_helper_connector_compare_state(connector, p, > + state1, state2); > + KUNIT_EXPECT_FALSE(test, identical); > +} [Severity: Medium] Do these seven plane, CRTC, and connector comparison tests leak state1 and state2? In drm_test_sro_compare_plane_identical(), drm_test_sro_compare_plane_mismatch(), drm_test_sro_compare_crtc_identical(), drm_test_sro_compare_crtc_inactive(), drm_test_sro_compare_crtc_active_mismatch(), drm_test_sro_compare_connector_identical(), and drm_test_sro_compare_connector_mismatch(), state1 and state2 are allocated via ->atomic_create_state() (using unmanaged kzalloc_obj()) and are not installed on the objects or in a drm_atomic_sro_state container. Unlike drm_test_sro_compare_bridge_identical() and drm_test_sro_compare_bridge_mismatch() below, these tests return without calling ->atomic_destroy_state() on state1 and state2. Also, in drm_test_sro_compare_connector_mismatch(), state2->crtc is set to priv->crtc without taking a connector reference via drm_connector_get(connector). When destroying state2 via connector->funcs->atomic_destroy_state(connector, state2), __drm_atomic_helper_connector_destroy_state() will call drm_connector_put(state->connector) because state2->crtc is non-NULL, so either drm_connector_get(connector) should be called when setting state2->crtc or state2->crtc should be cleared before destroying state2. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=25
