Hi Maxime, Thank you for the patch.
On Wed, Jun 17, 2026 at 12:15:02PM +0200, Maxime Ripard wrote: > All bridge drivers have been converted to the atomic variants of the > enable, disable, pre_enable, and post_disable callbacks. > > Remove the deprecated legacy hooks from drm_bridge_funcs, the > drm_bridge_is_atomic() helper that was only needed to distinguish > between atomic and non-atomic bridges, and the legacy bridge test > cases. > > Since all bridges are now atomic, unconditionally initialize the > private object state at attach time. > > Signed-off-by: Maxime Ripard <[email protected]> > --- > drivers/gpu/drm/drm_bridge.c | 27 ++------- > drivers/gpu/drm/tests/drm_bridge_test.c | 104 > -------------------------------- > include/drm/drm_bridge.h | 103 ------------------------------- > 3 files changed, 5 insertions(+), 229 deletions(-) > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index 0cd93f966998..01101c900a39 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -513,15 +513,10 @@ static const struct drm_private_state_funcs > drm_bridge_priv_state_funcs = { > .atomic_create_state = drm_bridge_atomic_create_priv_state, > .atomic_duplicate_state = drm_bridge_atomic_duplicate_priv_state, > .atomic_destroy_state = drm_bridge_atomic_destroy_priv_state, > }; > > -static bool drm_bridge_is_atomic(struct drm_bridge *bridge) > -{ > - return bridge->funcs->atomic_create_state != NULL; > -} > - > /** > * drm_bridge_attach - attach the bridge to an encoder's chain > * > * @encoder: DRM encoder > * @bridge: bridge to attach > @@ -587,13 +582,12 @@ int drm_bridge_attach(struct drm_encoder *encoder, > struct drm_bridge *bridge, > ret = bridge->funcs->attach(bridge, encoder, flags); > if (ret < 0) > goto err_reset_bridge; > } > > - if (drm_bridge_is_atomic(bridge)) > - drm_atomic_private_obj_init(bridge->dev, &bridge->base, > - &drm_bridge_priv_state_funcs); > + drm_atomic_private_obj_init(bridge->dev, &bridge->base, > + &drm_bridge_priv_state_funcs); > > return 0; > > err_reset_bridge: > bridge->dev = NULL; > @@ -622,12 +616,11 @@ void drm_bridge_detach(struct drm_bridge *bridge) > return; > > if (WARN_ON(!bridge->dev)) > return; > > - if (drm_bridge_is_atomic(bridge)) > - drm_atomic_private_obj_fini(&bridge->base); > + drm_atomic_private_obj_fini(&bridge->base); > > if (bridge->funcs->detach) > bridge->funcs->detach(bridge); > > list_del(&bridge->chain_node); > @@ -810,15 +803,12 @@ void drm_atomic_bridge_chain_disable(struct drm_bridge > *bridge, > return; > > encoder = bridge->encoder; > mutex_lock(&encoder->bridge_chain_mutex); > list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) { > - if (iter->funcs->atomic_disable) { > + if (iter->funcs->atomic_disable) > iter->funcs->atomic_disable(iter, state); > - } else if (iter->funcs->disable) { > - iter->funcs->disable(iter); > - } > > if (iter == bridge) > break; > } > mutex_unlock(&encoder->bridge_chain_mutex); > @@ -828,12 +818,10 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_disable); > static void drm_atomic_bridge_call_post_disable(struct drm_bridge *bridge, > struct drm_atomic_commit *state) > { > if (state && bridge->funcs->atomic_post_disable) > bridge->funcs->atomic_post_disable(bridge, state); > - else if (bridge->funcs->post_disable) > - bridge->funcs->post_disable(bridge); > } > > /** > * drm_atomic_bridge_chain_post_disable - cleans up after disabling all > bridges > * in the encoder chain > @@ -925,12 +913,10 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable); > static void drm_atomic_bridge_call_pre_enable(struct drm_bridge *bridge, > struct drm_atomic_commit *state) > { > if (state && bridge->funcs->atomic_pre_enable) > bridge->funcs->atomic_pre_enable(bridge, state); > - else if (bridge->funcs->pre_enable) > - bridge->funcs->pre_enable(bridge); > } > > /** > * drm_atomic_bridge_chain_pre_enable - prepares for enabling all bridges in > * the encoder chain > @@ -1029,15 +1015,12 @@ void drm_atomic_bridge_chain_enable(struct drm_bridge > *first_bridge, There's a mention of the .disable() callback in the documentation of this function: * Calls &drm_bridge_funcs.atomic_disable (falls back on * &drm_bridge_funcs.disable) op for all the bridges in the encoder chain, Same for the three other functions in the same family. With that removed, Reviewed-by: Laurent Pinchart <[email protected]> > { > if (!first_bridge) > return; > > drm_for_each_bridge_in_chain_from(first_bridge, bridge) > - if (bridge->funcs->atomic_enable) { > + if (bridge->funcs->atomic_enable) > bridge->funcs->atomic_enable(bridge, state); > - } else if (bridge->funcs->enable) { > - bridge->funcs->enable(bridge); > - } > } > EXPORT_SYMBOL(drm_atomic_bridge_chain_enable); > > static int drm_atomic_bridge_check(struct drm_bridge *bridge, > struct drm_crtc_state *crtc_state, > diff --git a/drivers/gpu/drm/tests/drm_bridge_test.c > b/drivers/gpu/drm/tests/drm_bridge_test.c > index 3f0269e52432..99428fc03f29 100644 > --- a/drivers/gpu/drm/tests/drm_bridge_test.c > +++ b/drivers/gpu/drm/tests/drm_bridge_test.c > @@ -48,30 +48,10 @@ static void drm_test_bridge_priv_destroy(struct > drm_bridge *bridge) > struct drm_bridge_init_priv *priv = (struct drm_bridge_init_priv > *)bridge_priv->data; > > priv->destroyed = true; > } > > -static void drm_test_bridge_enable(struct drm_bridge *bridge) > -{ > - struct drm_bridge_priv *priv = bridge_to_priv(bridge); > - > - priv->enable_count++; > -} > - > -static void drm_test_bridge_disable(struct drm_bridge *bridge) > -{ > - struct drm_bridge_priv *priv = bridge_to_priv(bridge); > - > - priv->disable_count++; > -} > - > -static const struct drm_bridge_funcs drm_test_bridge_legacy_funcs = { > - .destroy = drm_test_bridge_priv_destroy, > - .enable = drm_test_bridge_enable, > - .disable = drm_test_bridge_disable, > -}; > - > static void drm_test_bridge_atomic_enable(struct drm_bridge *bridge, > struct drm_atomic_commit *state) > { > struct drm_bridge_priv *priv = bridge_to_priv(bridge); > > @@ -234,39 +214,12 @@ static void > drm_test_drm_bridge_get_current_state_atomic(struct kunit *test) > > drm_modeset_drop_locks(&ctx); > drm_modeset_acquire_fini(&ctx); > } > > -/* > - * Test that drm_bridge_get_current_state() returns NULL for a > - * non-atomic bridge. > - */ > -static void drm_test_drm_bridge_get_current_state_legacy(struct kunit *test) > -{ > - struct drm_bridge_init_priv *priv; > - struct drm_bridge *bridge; > - > - priv = drm_test_bridge_init(test, &drm_test_bridge_legacy_funcs); > - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv); > - > - /* > - * NOTE: Strictly speaking, we should take the bridge->base.lock > - * before calling that function. However, bridge->base is only > - * initialized if the bridge is atomic, while we explicitly > - * initialize one that isn't there. > - * > - * In order to avoid unnecessary warnings, let's skip the > - * locking. The function would return NULL in all cases anyway, > - * so we don't really have any concurrency to worry about. > - */ > - bridge = &priv->test_bridge->bridge; > - KUNIT_EXPECT_NULL(test, drm_bridge_get_current_state(bridge)); > -} > - > static struct kunit_case drm_bridge_get_current_state_tests[] = { > KUNIT_CASE(drm_test_drm_bridge_get_current_state_atomic), > - KUNIT_CASE(drm_test_drm_bridge_get_current_state_legacy), > { } > }; > > > static struct kunit_suite drm_bridge_get_current_state_test_suite = { > @@ -367,70 +320,13 @@ static void > drm_test_drm_bridge_helper_reset_crtc_atomic_disabled(struct kunit * > > KUNIT_EXPECT_EQ(test, bridge_priv->enable_count, 0); > KUNIT_EXPECT_EQ(test, bridge_priv->disable_count, 0); > } > > -/* > - * Test that a non-atomic bridge is properly power-cycled when calling > - * drm_bridge_helper_reset_crtc(). > - */ > -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_priv *bridge_priv; > - int ret; > - > - priv = drm_test_bridge_init(test, &drm_test_bridge_legacy_funcs); > - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv); > - > - mode = drm_kunit_display_mode_from_cea_vic(test, &priv->drm, 16); > - KUNIT_ASSERT_NOT_ERR_OR_NULL(test, mode); > - > - drm_modeset_acquire_init(&ctx, 0); > - > -retry_commit: > - ret = drm_kunit_helper_enable_crtc_connector(test, > - &priv->drm, priv->crtc, > - priv->connector, > - mode, > - &ctx); > - if (ret == -EDEADLK) { > - drm_modeset_backoff(&ctx); > - goto retry_commit; > - } > - KUNIT_ASSERT_EQ(test, ret, 0); > - > - drm_modeset_drop_locks(&ctx); > - drm_modeset_acquire_fini(&ctx); > - > - bridge_priv = priv->test_bridge; > - KUNIT_ASSERT_EQ(test, bridge_priv->enable_count, 1); > - KUNIT_ASSERT_EQ(test, bridge_priv->disable_count, 0); > - > - drm_modeset_acquire_init(&ctx, 0); > - > -retry_reset: > - ret = drm_bridge_helper_reset_crtc(&bridge_priv->bridge, &ctx); > - if (ret == -EDEADLK) { > - drm_modeset_backoff(&ctx); > - goto retry_reset; > - } > - KUNIT_ASSERT_EQ(test, ret, 0); > - > - drm_modeset_drop_locks(&ctx); > - drm_modeset_acquire_fini(&ctx); > - > - KUNIT_EXPECT_EQ(test, bridge_priv->enable_count, 2); > - KUNIT_EXPECT_EQ(test, bridge_priv->disable_count, 1); > -} > - > static struct kunit_case drm_bridge_helper_reset_crtc_tests[] = { > KUNIT_CASE(drm_test_drm_bridge_helper_reset_crtc_atomic), > KUNIT_CASE(drm_test_drm_bridge_helper_reset_crtc_atomic_disabled), > - KUNIT_CASE(drm_test_drm_bridge_helper_reset_crtc_legacy), > { } > }; > > static struct kunit_suite drm_bridge_helper_reset_crtc_test_suite = { > .name = "drm_test_bridge_helper_reset_crtc", > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > index 18f3db367dc1..123edad5fa65 100644 > --- a/include/drm/drm_bridge.h > +++ b/include/drm/drm_bridge.h > @@ -171,56 +171,10 @@ struct drm_bridge_funcs { > * operation should be rejected. > */ > bool (*mode_fixup)(struct drm_bridge *bridge, > const struct drm_display_mode *mode, > struct drm_display_mode *adjusted_mode); > - /** > - * @disable: > - * > - * This callback should disable the bridge. It is called right before > - * the preceding element in the display pipe is disabled. If the > - * preceding element is a bridge this means it's called before that > - * bridge's @disable vfunc. If the preceding element is a &drm_encoder > - * it's called right before the &drm_encoder_helper_funcs.disable, > - * &drm_encoder_helper_funcs.prepare or &drm_encoder_helper_funcs.dpms > - * hook. > - * > - * The bridge can assume that the display pipe (i.e. clocks and timing > - * signals) feeding it is still running when this callback is called. > - * > - * The @disable callback is optional. > - * > - * NOTE: > - * > - * This is deprecated, do not use! > - * New drivers shall use &drm_bridge_funcs.atomic_disable. > - */ > - void (*disable)(struct drm_bridge *bridge); > - > - /** > - * @post_disable: > - * > - * This callback should disable the bridge. It is called right after the > - * preceding element in the display pipe is disabled. If the preceding > - * element is a bridge this means it's called after that bridge's > - * @post_disable function. If the preceding element is a &drm_encoder > - * it's called right after the encoder's > - * &drm_encoder_helper_funcs.disable, &drm_encoder_helper_funcs.prepare > - * or &drm_encoder_helper_funcs.dpms hook. > - * > - * The bridge must assume that the display pipe (i.e. clocks and timing > - * signals) feeding it is no longer running when this callback is > - * called. > - * > - * The @post_disable callback is optional. > - * > - * NOTE: > - * > - * This is deprecated, do not use! > - * New drivers shall use &drm_bridge_funcs.atomic_post_disable. > - */ > - void (*post_disable)(struct drm_bridge *bridge); > > /** > * @mode_set: > * > * This callback should set the given mode on the bridge. It is called > @@ -247,59 +201,10 @@ struct drm_bridge_funcs { > * &drm_bridge_funcs.atomic_enable operation. > */ > void (*mode_set)(struct drm_bridge *bridge, > const struct drm_display_mode *mode, > const struct drm_display_mode *adjusted_mode); > - /** > - * @pre_enable: > - * > - * This callback should enable the bridge. It is called right before > - * the preceding element in the display pipe is enabled. If the > - * preceding element is a bridge this means it's called before that > - * bridge's @pre_enable function. If the preceding element is a > - * &drm_encoder it's called right before the encoder's > - * &drm_encoder_helper_funcs.enable, &drm_encoder_helper_funcs.commit or > - * &drm_encoder_helper_funcs.dpms hook. > - * > - * The display pipe (i.e. clocks and timing signals) feeding this bridge > - * will not yet be running when this callback is called. The bridge must > - * not enable the display link feeding the next bridge in the chain (if > - * there is one) when this callback is called. > - * > - * The @pre_enable callback is optional. > - * > - * NOTE: > - * > - * This is deprecated, do not use! > - * New drivers shall use &drm_bridge_funcs.atomic_pre_enable. > - */ > - void (*pre_enable)(struct drm_bridge *bridge); > - > - /** > - * @enable: > - * > - * This callback should enable the bridge. It is called right after > - * the preceding element in the display pipe is enabled. If the > - * preceding element is a bridge this means it's called after that > - * bridge's @enable function. If the preceding element is a > - * &drm_encoder it's called right after the encoder's > - * &drm_encoder_helper_funcs.enable, &drm_encoder_helper_funcs.commit or > - * &drm_encoder_helper_funcs.dpms hook. > - * > - * The bridge can assume that the display pipe (i.e. clocks and timing > - * signals) feeding it is running when this callback is called. This > - * callback must enable the display link feeding the next bridge in the > - * chain if there is one. > - * > - * The @enable callback is optional. > - * > - * NOTE: > - * > - * This is deprecated, do not use! > - * New drivers shall use &drm_bridge_funcs.atomic_enable. > - */ > - void (*enable)(struct drm_bridge *bridge); > > /** > * @atomic_pre_enable: > * > * This callback should enable the bridge. It is called right before > @@ -1357,18 +1262,10 @@ static inline struct drm_bridge_state * > drm_bridge_get_current_state(struct drm_bridge *bridge) > { > if (!bridge) > return NULL; > > - /* > - * Only atomic bridges will have bridge->base initialized by > - * drm_atomic_private_obj_init(), so we need to make sure we're > - * working with one before we try to use the lock. > - */ > - if (!bridge->funcs || !bridge->funcs->atomic_create_state) > - return NULL; > - > drm_modeset_lock_assert_held(&bridge->base.lock); > > if (!bridge->base.state) > return NULL; > -- Regards, Laurent Pinchart
