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

[Maxime Ripard <mripard@kernel.org>]

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

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.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;
> +};
> +

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]