On Fri, May 16, 2025 at 06:48:37PM +0200, Luca Ceresoli 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
> 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.ceres...@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;
> +};
> +
If we want to remain consistent with the rest of the names, I guess
drm_bridge_priv would be a better choice.
> 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)
bridge_to_priv
> +{
> + 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);
and priv for the variable name is enough here too.
>
> - 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;
and bridge_priv here.
The rest looks good
Maxime
signature.asc
Description: PGP signature
