Re: [PATCH v8 1/3] drm/tests: bridge: convert to devm_drm_bridge_alloc() API

[Anusha Srivatsa <asrivats@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 11428 bytes --]

On Fri, May 16, 2025 at 12:48 PM Luca Ceresoli <luca.ceresoli@bootlin.com>
wrote:

> Use the new DRM bridge allocation API, which is the only supported now, for
> the kunit tests.
>
> This change is more massive than for the typical DRM bridge driver because
> struct drm_bridge_init_priv currently embeds a struct drm_bridge, which is
> not supported anymore. We new have to use devm_drm_bridge_alloc() to
>
                                            typo ^^^s/new/now.

Thanks,
Anusha

dynamically allocate a "private driver struct", which is a bit awkward here
> because there is no real bridge driver. Thus let's add a "dummy" DRM bridge
> struct to represent it.
>
> As a nice cleanup we can now move the enable_count and disable_count
> members, which are counting bridge-specific events, into the new "private
> driver struct" (and avoid adding new unnecessary indirections).
>
> Also add a trivial bridge_to_dummy_bridge() just like many drivers do.
>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
>
> ---
>
> This patch was added in v8.
> ---
>  drivers/gpu/drm/tests/drm_bridge_test.c | 95
> +++++++++++++++++++--------------
>  1 file changed, 55 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/tests/drm_bridge_test.c
> b/drivers/gpu/drm/tests/drm_bridge_test.c
> index
> ff88ec2e911c9cc9a718483f09d4c764f45f991a..f3a625c536f610dc8560b56531056df7c613f564
> 100644
> --- a/drivers/gpu/drm/tests/drm_bridge_test.c
> +++ b/drivers/gpu/drm/tests/drm_bridge_test.c
> @@ -10,31 +10,45 @@
>
>  #include <kunit/test.h>
>
> +/*
> + * Mimick the typical struct defined by a bridge driver, which embeds a
> + * bridge plus other fields.
> + *
> + * Having at least one member before @bridge ensures we test non-zero
> + * @bridge offset.
> + */
> +struct dummy_drm_bridge {
> +       unsigned int enable_count;
> +       unsigned int disable_count;
> +       struct drm_bridge bridge;
> +};
> +
>  struct drm_bridge_init_priv {
>         struct drm_device drm;
>         struct drm_plane *plane;
>         struct drm_crtc *crtc;
>         struct drm_encoder encoder;
> -       struct drm_bridge bridge;
> +       struct dummy_drm_bridge *test_bridge;
>         struct drm_connector *connector;
> -       unsigned int enable_count;
> -       unsigned int disable_count;
>  };
>
> +static struct dummy_drm_bridge *bridge_to_dummy_bridge(struct drm_bridge
> *bridge)
> +{
> +       return container_of(bridge, struct dummy_drm_bridge, bridge);
> +}
> +
>  static void drm_test_bridge_enable(struct drm_bridge *bridge)
>  {
> -       struct drm_bridge_init_priv *priv =
> -               container_of(bridge, struct drm_bridge_init_priv, bridge);
> +       struct dummy_drm_bridge *dummy_br = bridge_to_dummy_bridge(bridge);
>
> -       priv->enable_count++;
> +       dummy_br->enable_count++;
>  }
>
>  static void drm_test_bridge_disable(struct drm_bridge *bridge)
>  {
> -       struct drm_bridge_init_priv *priv =
> -               container_of(bridge, struct drm_bridge_init_priv, bridge);
> +       struct dummy_drm_bridge *dummy_br = bridge_to_dummy_bridge(bridge);
>
> -       priv->disable_count++;
> +       dummy_br->disable_count++;
>  }
>
>  static const struct drm_bridge_funcs drm_test_bridge_legacy_funcs = {
> @@ -45,19 +59,17 @@ static const struct drm_bridge_funcs
> drm_test_bridge_legacy_funcs = {
>  static void drm_test_bridge_atomic_enable(struct drm_bridge *bridge,
>                                           struct drm_atomic_state *state)
>  {
> -       struct drm_bridge_init_priv *priv =
> -               container_of(bridge, struct drm_bridge_init_priv, bridge);
> +       struct dummy_drm_bridge *dummy_br = bridge_to_dummy_bridge(bridge);
>
> -       priv->enable_count++;
> +       dummy_br->enable_count++;
>  }
>
>  static void drm_test_bridge_atomic_disable(struct drm_bridge *bridge,
>                                            struct drm_atomic_state *state)
>  {
> -       struct drm_bridge_init_priv *priv =
> -               container_of(bridge, struct drm_bridge_init_priv, bridge);
> +       struct dummy_drm_bridge *dummy_br = bridge_to_dummy_bridge(bridge);
>
> -       priv->disable_count++;
> +       dummy_br->disable_count++;
>  }
>
>  static const struct drm_bridge_funcs drm_test_bridge_atomic_funcs = {
> @@ -102,6 +114,10 @@ drm_test_bridge_init(struct kunit *test, const struct
> drm_bridge_funcs *funcs)
>         if (IS_ERR(priv))
>                 return ERR_CAST(priv);
>
> +       priv->test_bridge = devm_drm_bridge_alloc(dev, struct
> dummy_drm_bridge, bridge, funcs);
> +       if (IS_ERR(priv->test_bridge))
> +               return ERR_CAST(priv->test_bridge);
> +
>         drm = &priv->drm;
>         priv->plane = drm_kunit_helper_create_primary_plane(test, drm,
>                                                             NULL,
> @@ -125,9 +141,8 @@ drm_test_bridge_init(struct kunit *test, const struct
> drm_bridge_funcs *funcs)
>
>         enc->possible_crtcs = drm_crtc_mask(priv->crtc);
>
> -       bridge = &priv->bridge;
> +       bridge = &priv->test_bridge->bridge;
>         bridge->type = DRM_MODE_CONNECTOR_VIRTUAL;
> -       bridge->funcs = funcs;
>
>         ret = drm_kunit_bridge_add(test, bridge);
>         if (ret)
> @@ -173,7 +188,7 @@ static void
> drm_test_drm_bridge_get_current_state_atomic(struct kunit *test)
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
>
>  retry_commit:
> -       bridge = &priv->bridge;
> +       bridge = &priv->test_bridge->bridge;
>         bridge_state = drm_atomic_get_bridge_state(state, bridge);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, bridge_state);
>
> @@ -228,7 +243,7 @@ static void
> drm_test_drm_bridge_get_current_state_legacy(struct kunit *test)
>          * locking. The function would return NULL in all cases anyway,
>          * so we don't really have any concurrency to worry about.
>          */
> -       bridge = &priv->bridge;
> +       bridge = &priv->test_bridge->bridge;
>         KUNIT_EXPECT_NULL(test, drm_bridge_get_current_state(bridge));
>  }
>
> @@ -253,7 +268,7 @@ static void
> drm_test_drm_bridge_helper_reset_crtc_atomic(struct kunit *test)
>         struct drm_modeset_acquire_ctx ctx;
>         struct drm_bridge_init_priv *priv;
>         struct drm_display_mode *mode;
> -       struct drm_bridge *bridge;
> +       struct dummy_drm_bridge *dummy_br;
>         int ret;
>
>         priv = drm_test_bridge_init(test, &drm_test_bridge_atomic_funcs);
> @@ -279,14 +294,14 @@ static void
> drm_test_drm_bridge_helper_reset_crtc_atomic(struct kunit *test)
>         drm_modeset_drop_locks(&ctx);
>         drm_modeset_acquire_fini(&ctx);
>
> -       bridge = &priv->bridge;
> -       KUNIT_ASSERT_EQ(test, priv->enable_count, 1);
> -       KUNIT_ASSERT_EQ(test, priv->disable_count, 0);
> +       dummy_br = priv->test_bridge;
> +       KUNIT_ASSERT_EQ(test, dummy_br->enable_count, 1);
> +       KUNIT_ASSERT_EQ(test, dummy_br->disable_count, 0);
>
>         drm_modeset_acquire_init(&ctx, 0);
>
>  retry_reset:
> -       ret = drm_bridge_helper_reset_crtc(bridge, &ctx);
> +       ret = drm_bridge_helper_reset_crtc(&dummy_br->bridge, &ctx);
>         if (ret == -EDEADLK) {
>                 drm_modeset_backoff(&ctx);
>                 goto retry_reset;
> @@ -296,8 +311,8 @@ static void
> drm_test_drm_bridge_helper_reset_crtc_atomic(struct kunit *test)
>         drm_modeset_drop_locks(&ctx);
>         drm_modeset_acquire_fini(&ctx);
>
> -       KUNIT_EXPECT_EQ(test, priv->enable_count, 2);
> -       KUNIT_EXPECT_EQ(test, priv->disable_count, 1);
> +       KUNIT_EXPECT_EQ(test, dummy_br->enable_count, 2);
> +       KUNIT_EXPECT_EQ(test, dummy_br->disable_count, 1);
>  }
>
>  /*
> @@ -309,7 +324,7 @@ static void
> drm_test_drm_bridge_helper_reset_crtc_atomic_disabled(struct kunit *
>         struct drm_modeset_acquire_ctx ctx;
>         struct drm_bridge_init_priv *priv;
>         struct drm_display_mode *mode;
> -       struct drm_bridge *bridge;
> +       struct dummy_drm_bridge *dummy_br;
>         int ret;
>
>         priv = drm_test_bridge_init(test, &drm_test_bridge_atomic_funcs);
> @@ -318,14 +333,14 @@ static void
> drm_test_drm_bridge_helper_reset_crtc_atomic_disabled(struct kunit *
>         mode = drm_kunit_display_mode_from_cea_vic(test, &priv->drm, 16);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mode);
>
> -       bridge = &priv->bridge;
> -       KUNIT_ASSERT_EQ(test, priv->enable_count, 0);
> -       KUNIT_ASSERT_EQ(test, priv->disable_count, 0);
> +       dummy_br = priv->test_bridge;
> +       KUNIT_ASSERT_EQ(test, dummy_br->enable_count, 0);
> +       KUNIT_ASSERT_EQ(test, dummy_br->disable_count, 0);
>
>         drm_modeset_acquire_init(&ctx, 0);
>
>  retry_reset:
> -       ret = drm_bridge_helper_reset_crtc(bridge, &ctx);
> +       ret = drm_bridge_helper_reset_crtc(&dummy_br->bridge, &ctx);
>         if (ret == -EDEADLK) {
>                 drm_modeset_backoff(&ctx);
>                 goto retry_reset;
> @@ -335,8 +350,8 @@ static void
> drm_test_drm_bridge_helper_reset_crtc_atomic_disabled(struct kunit *
>         drm_modeset_drop_locks(&ctx);
>         drm_modeset_acquire_fini(&ctx);
>
> -       KUNIT_EXPECT_EQ(test, priv->enable_count, 0);
> -       KUNIT_EXPECT_EQ(test, priv->disable_count, 0);
> +       KUNIT_EXPECT_EQ(test, dummy_br->enable_count, 0);
> +       KUNIT_EXPECT_EQ(test, dummy_br->disable_count, 0);
>  }
>
>  /*
> @@ -348,7 +363,7 @@ static void
> drm_test_drm_bridge_helper_reset_crtc_legacy(struct kunit *test)
>         struct drm_modeset_acquire_ctx ctx;
>         struct drm_bridge_init_priv *priv;
>         struct drm_display_mode *mode;
> -       struct drm_bridge *bridge;
> +       struct dummy_drm_bridge *dummy_br;
>         int ret;
>
>         priv = drm_test_bridge_init(test, &drm_test_bridge_legacy_funcs);
> @@ -374,14 +389,14 @@ static void
> drm_test_drm_bridge_helper_reset_crtc_legacy(struct kunit *test)
>         drm_modeset_drop_locks(&ctx);
>         drm_modeset_acquire_fini(&ctx);
>
> -       bridge = &priv->bridge;
> -       KUNIT_ASSERT_EQ(test, priv->enable_count, 1);
> -       KUNIT_ASSERT_EQ(test, priv->disable_count, 0);
> +       dummy_br = priv->test_bridge;
> +       KUNIT_ASSERT_EQ(test, dummy_br->enable_count, 1);
> +       KUNIT_ASSERT_EQ(test, dummy_br->disable_count, 0);
>
>         drm_modeset_acquire_init(&ctx, 0);
>
>  retry_reset:
> -       ret = drm_bridge_helper_reset_crtc(bridge, &ctx);
> +       ret = drm_bridge_helper_reset_crtc(&dummy_br->bridge, &ctx);
>         if (ret == -EDEADLK) {
>                 drm_modeset_backoff(&ctx);
>                 goto retry_reset;
> @@ -391,8 +406,8 @@ static void
> drm_test_drm_bridge_helper_reset_crtc_legacy(struct kunit *test)
>         drm_modeset_drop_locks(&ctx);
>         drm_modeset_acquire_fini(&ctx);
>
> -       KUNIT_EXPECT_EQ(test, priv->enable_count, 2);
> -       KUNIT_EXPECT_EQ(test, priv->disable_count, 1);
> +       KUNIT_EXPECT_EQ(test, dummy_br->enable_count, 2);
> +       KUNIT_EXPECT_EQ(test, dummy_br->disable_count, 1);
>  }
>
>  static struct kunit_case drm_bridge_helper_reset_crtc_tests[] = {
>
> --
> 2.49.0
>
>

[-- Attachment #2: Type: text/html, Size: 13647 bytes --]