| Message ID | 20190808151150.16336-11-boris.brezillon@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Add support for bus-format negotiation | expand |
Hi Boris, Thank you for the patch. On Thu, Aug 08, 2019 at 05:11:41PM +0200, Boris Brezillon wrote: > One of the last remaining object to not have its atomic state. > > This is being motivated by our attempt to support runtime bus-format > negotiation between elements of the bridge chain. > This patch just paves the road for such a feature by adding a new > drm_bridge_state object inheriting from drm_private_obj so we can > re-use some of the state initialization/tracking logic. > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> > --- > drivers/gpu/drm/drm_atomic.c | 40 ++++++++ > drivers/gpu/drm/drm_atomic_helper.c | 18 ++++ > drivers/gpu/drm/drm_bridge.c | 141 +++++++++++++++++++++++++++- > include/drm/drm_atomic.h | 3 + > include/drm/drm_bridge.h | 99 +++++++++++++++++++ > 5 files changed, 296 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 419381abbdd1..ed6fd3e31b56 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -30,6 +30,7 @@ > > #include <drm/drm_atomic.h> > #include <drm/drm_atomic_uapi.h> > +#include <drm/drm_bridge.h> > #include <drm/drm_debugfs.h> > #include <drm/drm_device.h> > #include <drm/drm_drv.h> > @@ -1010,6 +1011,45 @@ static void drm_atomic_connector_print_state(struct drm_printer *p, > connector->funcs->atomic_print_state(p, state); > } > > +/** > + * drm_atomic_add_encoder_bridges - add bridges attached to an encoder > + * @state: atomic state > + * @encoder: DRM encoder > + * > + * This function adds all bridges attached to @encoder. This is needed to add > + * bridge states to @state and make them available when > + * &bridge_funcs.atomic_{check,pre_enable,enable,disable_post_disable}() are > + * called > + * > + * Returns: > + * 0 on success or can fail with -EDEADLK or -ENOMEM. When the error is EDEADLK > + * then the w/w mutex code has detected a deadlock and the entire atomic > + * sequence must be restarted. All other errors are fatal. > + */ > +int > +drm_atomic_add_encoder_bridges(struct drm_atomic_state *state, > + struct drm_encoder *encoder) > +{ > + struct drm_bridge_state *bridge_state; > + struct drm_bridge *bridge; > + > + if (!encoder) > + return 0; > + > + DRM_DEBUG_ATOMIC("Adding all bridges for [encoder:%d:%s] to %p\n", > + encoder->base.id, encoder->name, state); > + > + for (bridge = drm_bridge_chain_get_first_bridge(encoder); bridge; > + bridge = drm_bridge_chain_get_next_bridge(bridge)) { Would a for_each_bridge() macro make sense ? I'd implement it on top of list_for_each_entry() to make it simple. > + bridge_state = drm_atomic_get_bridge_state(state, bridge); > + if (IS_ERR(bridge_state)) > + return PTR_ERR(bridge_state); > + } > + > + return 0; > +} > +EXPORT_SYMBOL(drm_atomic_add_encoder_bridges); > + > /** > * drm_atomic_add_affected_connectors - add connectors for crtc > * @state: atomic state > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 0deb54099570..e76ec9648b6f 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -736,6 +736,24 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, > return ret; > } > > + /* > + * Iterate over all connectors again, and add all affected bridges to > + * the state. > + */ > + for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i) { > + struct drm_encoder *encoder; > + > + encoder = old_connector_state->best_encoder; > + ret = drm_atomic_add_encoder_bridges(state, encoder); > + if (ret) > + return ret; > + > + encoder = new_connector_state->best_encoder; > + ret = drm_atomic_add_encoder_bridges(state, encoder); > + if (ret) > + return ret; > + } > + > ret = mode_valid(state); > if (ret) > return ret; > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > index ed2410f49581..0280da6be1e6 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -89,6 +89,38 @@ void drm_bridge_remove(struct drm_bridge *bridge) > } > EXPORT_SYMBOL(drm_bridge_remove); > > +static struct drm_private_state * > +drm_bridge_atomic_duplicate_priv_state(struct drm_private_obj *obj) > +{ > + struct drm_bridge *bridge = drm_priv_to_bridge(obj); > + struct drm_bridge_state *state; > + > + if (bridge->funcs->atomic_duplicate_state) > + state = bridge->funcs->atomic_duplicate_state(bridge); > + else > + state = drm_atomic_helper_duplicate_bridge_state(bridge); > + > + return &state->base; > +} > + > +static void > +drm_bridge_atomic_destroy_priv_state(struct drm_private_obj *obj, > + struct drm_private_state *s) > +{ > + struct drm_bridge_state *state = drm_priv_to_bridge_state(s); > + struct drm_bridge *bridge = drm_priv_to_bridge(obj); > + > + if (bridge->funcs->atomic_destroy_state) > + bridge->funcs->atomic_destroy_state(bridge, state); > + else > + drm_atomic_helper_destroy_bridge_state(bridge, state); > +} > + > +static const struct drm_private_state_funcs drm_bridge_priv_state_funcs = { > + .atomic_duplicate_state = drm_bridge_atomic_duplicate_priv_state, > + .atomic_destroy_state = drm_bridge_atomic_destroy_priv_state, > +}; > + > /** > * drm_bridge_attach - attach the bridge to an encoder's chain > * > @@ -114,6 +146,7 @@ EXPORT_SYMBOL(drm_bridge_remove); > int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, > struct drm_bridge *previous) > { > + struct drm_bridge_state *state; > LIST_HEAD(tmp_list); > int ret; > > @@ -132,19 +165,39 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, > > if (bridge->funcs->attach) { > ret = bridge->funcs->attach(bridge); > - if (ret < 0) { > - bridge->dev = NULL; > - bridge->encoder = NULL; > - return ret; > - } > + if (ret < 0) > + goto err_reset_bridge; > } > > + if (bridge->funcs->atomic_duplicate_state) > + state = bridge->funcs->atomic_duplicate_state(bridge); > + else > + state = drm_atomic_helper_duplicate_bridge_state(bridge); > + > + if (!state) { > + ret = -ENOMEM; > + goto err_detach_bridge; > + } > + > + drm_atomic_private_obj_init(bridge->dev, &bridge->base, > + &state->base, > + &drm_bridge_priv_state_funcs); > + > if (previous) > list_splice(&tmp_list, &previous->chain_node); > else > list_splice(&tmp_list, &encoder->bridge_chain); > > return 0; > + > +err_detach_bridge: > + if (bridge->funcs->detach) > + bridge->funcs->detach(bridge); > + > +err_reset_bridge: > + bridge->dev = NULL; > + bridge->encoder = NULL; > + return ret; > } > EXPORT_SYMBOL(drm_bridge_attach); > > @@ -156,6 +209,8 @@ void drm_bridge_detach(struct drm_bridge *bridge) > if (WARN_ON(!bridge->dev)) > return; > > + drm_atomic_private_obj_fini(&bridge->base); > + > if (bridge->funcs->detach) > bridge->funcs->detach(bridge); > > @@ -495,6 +550,82 @@ void drm_atomic_bridge_chain_enable(struct drm_encoder *encoder, > } > EXPORT_SYMBOL(drm_atomic_bridge_chain_enable); > > +/** > + * drm_atomic_helper_init_bridge_state() - Initializes a bridge state s/Initializes/Initialize/ > + * @bridge this state is referring to Did you mean * @bridge: the bridge this state is referring to ? > + * @state: bridge state to initialize > + * > + * For now it's just a memset(0) plus a state->bridge assignment. Might > + * be extended in the future. > + */ > +void drm_atomic_helper_init_bridge_state(struct drm_bridge *bridge, > + struct drm_bridge_state *state) > +{ > + memset(state, 0, sizeof(*state)); > + state->bridge = bridge; > +} > +EXPORT_SYMBOL(drm_atomic_helper_init_bridge_state); Other objects don't export a state init function helper, but state reset and state duplicate helpers. Is there a reason to depart from that model ? Same for the copy helper below. > + > +/** > + * drm_atomic_helper_copy_bridge_state() - Copy the content of a bridge state > + * @bridge: bridge the old and new state are referring to > + * @old: previous bridge state to copy from > + * @new: new bridge state to copy to > + * > + * Should be used by custom &drm_bridge_funcs.atomic_duplicate() implementation > + * to copy the previous state into the new object. > + */ > +void drm_atomic_helper_copy_bridge_state(struct drm_bridge *bridge, > + const struct drm_bridge_state *old, > + struct drm_bridge_state *new) > +{ > + *new = *old; > +} > +EXPORT_SYMBOL(drm_atomic_helper_copy_bridge_state); > + > +/** > + * drm_atomic_helper_duplicate_bridge_state() - Default duplicate state helper > + * @bridge: bridge containing the state to duplicate > + * > + * Default implementation of &drm_bridge_funcs.atomic_duplicate(). > + * > + * RETURNS: > + * a valid state object or NULL if the allocation fails. > + */ > +struct drm_bridge_state * > +drm_atomic_helper_duplicate_bridge_state(struct drm_bridge *bridge) > +{ > + struct drm_bridge_state *old = NULL, *new; > + > + new = kzalloc(sizeof(*new), GFP_KERNEL); > + if (!new) > + return NULL; > + > + if (bridge->base.state) { > + old = drm_priv_to_bridge_state(bridge->base.state); > + drm_atomic_helper_copy_bridge_state(bridge, old, new); > + } else { > + drm_atomic_helper_init_bridge_state(bridge, new); > + } > + > + return new; > +} > +EXPORT_SYMBOL(drm_atomic_helper_duplicate_bridge_state); > + > +/** > + * drm_atomic_helper_destroy_bridge_state() - Default destroy state helper > + * @bridge: the bridge this state refers to > + * @state: state object to destroy > + * > + * Just a simple kfree() for now. > + */ > +void drm_atomic_helper_destroy_bridge_state(struct drm_bridge *bridge, > + struct drm_bridge_state *state) > +{ > + kfree(state); > +} > +EXPORT_SYMBOL(drm_atomic_helper_destroy_bridge_state); > + > #ifdef CONFIG_OF > /** > * of_drm_find_bridge - find the bridge corresponding to the device node in > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > index 927e1205d7aa..5ee25fd51e80 100644 > --- a/include/drm/drm_atomic.h > +++ b/include/drm/drm_atomic.h > @@ -660,6 +660,9 @@ __drm_atomic_get_current_plane_state(struct drm_atomic_state *state, > return plane->state; > } > > +int __must_check > +drm_atomic_add_encoder_bridges(struct drm_atomic_state *state, > + struct drm_encoder *encoder); > int __must_check > drm_atomic_add_affected_connectors(struct drm_atomic_state *state, > struct drm_crtc *crtc); > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > index f40181274ed7..8ca25d266d0c 100644 > --- a/include/drm/drm_bridge.h > +++ b/include/drm/drm_bridge.h > @@ -25,6 +25,7 @@ > > #include <linux/list.h> > #include <linux/ctype.h> > +#include <drm/drm_atomic.h> > #include <drm/drm_mode_object.h> > #include <drm/drm_modes.h> > > @@ -32,6 +33,23 @@ struct drm_bridge; > struct drm_bridge_timings; > struct drm_panel; > > +/** > + * struct drm_bridge_state - Atomic bridge state object > + * @base: inherit from &drm_private_state > + * @bridge: the bridge this state refers to > + */ > +struct drm_bridge_state { > + struct drm_private_state base; > + > + struct drm_bridge *bridge; > +}; > + > +static inline struct drm_bridge_state * > +drm_priv_to_bridge_state(struct drm_private_state *priv) > +{ > + return container_of(priv, struct drm_bridge_state, base); > +} > + > /** > * struct drm_bridge_funcs - drm_bridge control functions > */ > @@ -337,6 +355,30 @@ struct drm_bridge_funcs { > */ > void (*atomic_post_disable)(struct drm_bridge *bridge, > struct drm_atomic_state *state); > + > + /** > + * @atomic_duplicate_state: > + * > + * Duplicate the current bridge state object. > + * > + * Note that this function can be called when current bridge state is > + * NULL. In this case, implementations should either implement HW > + * readback or initialize the state with sensible default values. Related to the question above, shouldn't we have a reset state operation instead ? > + * > + * RETURNS: > + * A valid drm_bridge_state object or NULL if the allocation fails. > + */ > + struct drm_bridge_state *(*atomic_duplicate_state)(struct drm_bridge *bridge); > + > + /** > + * @atomic_duplicate_state: > + * > + * Destroy a bridge state object previously allocated by > + * &drm_bridge_funcs.atomic_duplicate_state() or > + * &drm_bridge_funcs.atomic_get_initial_state(). > + */ > + void (*atomic_destroy_state)(struct drm_bridge *bridge, > + struct drm_bridge_state *state); > }; > > /** > @@ -379,6 +421,8 @@ struct drm_bridge_timings { > * struct drm_bridge - central DRM bridge control structure > */ > struct drm_bridge { > + /** @base: inherit from &drm_private_object */ > + struct drm_private_obj base; > /** @dev: DRM device this bridge belongs to */ > struct drm_device *dev; > /** @encoder: encoder to which this bridge is connected */ > @@ -403,6 +447,12 @@ struct drm_bridge { > void *driver_private; > }; > > +static inline struct drm_bridge * > +drm_priv_to_bridge(struct drm_private_obj *priv) > +{ > + return container_of(priv, struct drm_bridge, base); > +} > + > void drm_bridge_add(struct drm_bridge *bridge); > void drm_bridge_remove(struct drm_bridge *bridge); > struct drm_bridge *of_drm_find_bridge(struct device_node *np); > @@ -438,6 +488,55 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_encoder *encoder, > void drm_atomic_bridge_chain_enable(struct drm_encoder *encoder, > struct drm_atomic_state *state); > > +void drm_atomic_helper_init_bridge_state(struct drm_bridge *bridge, > + struct drm_bridge_state *state); > +void drm_atomic_helper_copy_bridge_state(struct drm_bridge *bridge, > + const struct drm_bridge_state *old, > + struct drm_bridge_state *new); > +struct drm_bridge_state * > +drm_atomic_helper_duplicate_bridge_state(struct drm_bridge *bridge); > +void drm_atomic_helper_destroy_bridge_state(struct drm_bridge *bridge, > + struct drm_bridge_state *state); > + > +static inline struct drm_bridge_state * > +drm_atomic_get_bridge_state(struct drm_atomic_state *state, > + struct drm_bridge *bridge) > +{ > + struct drm_private_state *obj_state; > + > + obj_state = drm_atomic_get_private_obj_state(state, &bridge->base); > + if (!obj_state) > + return NULL; > + > + return drm_priv_to_bridge_state(obj_state); > +} > + > +static inline struct drm_bridge_state * > +drm_atomic_get_old_bridge_state(struct drm_atomic_state *state, > + struct drm_bridge *bridge) > +{ > + struct drm_private_state *obj_state; > + > + obj_state = drm_atomic_get_old_private_obj_state(state, &bridge->base); > + if (!obj_state) > + return NULL; > + > + return drm_priv_to_bridge_state(obj_state); > +} > + > +static inline struct drm_bridge_state * > +drm_atomic_get_new_bridge_state(struct drm_atomic_state *state, > + struct drm_bridge *bridge) > +{ > + struct drm_private_state *obj_state; > + > + obj_state = drm_atomic_get_new_private_obj_state(state, &bridge->base); > + if (!obj_state) > + return NULL; > + > + return drm_priv_to_bridge_state(obj_state); > +} > + > #ifdef CONFIG_DRM_PANEL_BRIDGE > struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel, > u32 connector_type);
On Wed, 21 Aug 2019 23:14:56 +0300 Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > +int > > +drm_atomic_add_encoder_bridges(struct drm_atomic_state *state, > > + struct drm_encoder *encoder) > > +{ > > + struct drm_bridge_state *bridge_state; > > + struct drm_bridge *bridge; > > + > > + if (!encoder) > > + return 0; > > + > > + DRM_DEBUG_ATOMIC("Adding all bridges for [encoder:%d:%s] to %p\n", > > + encoder->base.id, encoder->name, state); > > + > > + for (bridge = drm_bridge_chain_get_first_bridge(encoder); bridge; > > + bridge = drm_bridge_chain_get_next_bridge(bridge)) { > > Would a for_each_bridge() macro make sense ? I'd implement it on top of > list_for_each_entry() to make it simple. I can add this helper. Note that for_each_bridge() will iterate over the whole bridge chain, and we might need a for_each_bridge_from() at some point if we want to iterate over a sub-chain. > > > + bridge_state = drm_atomic_get_bridge_state(state, bridge); > > + if (IS_ERR(bridge_state)) > > + return PTR_ERR(bridge_state); > > + } > > + > > + return 0; > > +} > > +EXPORT_SYMBOL(drm_atomic_add_encoder_bridges); [...] > > > > +/** > > + * drm_atomic_helper_init_bridge_state() - Initializes a bridge state > > s/Initializes/Initialize/ > > > + * @bridge this state is referring to > > Did you mean > > * @bridge: the bridge this state is referring to ? Yes. > > > + * @state: bridge state to initialize > > + * > > + * For now it's just a memset(0) plus a state->bridge assignment. Might > > + * be extended in the future. > > + */ > > +void drm_atomic_helper_init_bridge_state(struct drm_bridge *bridge, > > + struct drm_bridge_state *state) > > +{ > > + memset(state, 0, sizeof(*state)); > > + state->bridge = bridge; > > +} > > +EXPORT_SYMBOL(drm_atomic_helper_init_bridge_state); > > Other objects don't export a state init function helper, but state reset > and state duplicate helpers. Is there a reason to depart from that model > ? Same for the copy helper below. I thought those names better reflect what the helpers do. __drm_atomic_helper_crtc_duplicate_state() for instance, it actually does not duplicate the state, but just copies current state info into the new state object which has been allocated by the caller. Same for the reset helpers, they actually do not reset anything but just initialize the state to a default. I also try to avoid prefixing functions with __ when I can (names can be clarified/extended to avoid conflicts most of the time). Will switch back to reset/dup names if you prefer. > > > + > > +/** > > + * drm_atomic_helper_copy_bridge_state() - Copy the content of a bridge state > > + * @bridge: bridge the old and new state are referring to > > + * @old: previous bridge state to copy from > > + * @new: new bridge state to copy to > > + * > > + * Should be used by custom &drm_bridge_funcs.atomic_duplicate() implementation > > + * to copy the previous state into the new object. > > + */ > > +void drm_atomic_helper_copy_bridge_state(struct drm_bridge *bridge, > > + const struct drm_bridge_state *old, > > + struct drm_bridge_state *new) > > +{ > > + *new = *old; > > +} > > +EXPORT_SYMBOL(drm_atomic_helper_copy_bridge_state); > > + > > +/** > > + * drm_atomic_helper_duplicate_bridge_state() - Default duplicate state helper > > + * @bridge: bridge containing the state to duplicate > > + * > > + * Default implementation of &drm_bridge_funcs.atomic_duplicate(). > > + * > > + * RETURNS: > > + * a valid state object or NULL if the allocation fails. > > + */ > > +struct drm_bridge_state * > > +drm_atomic_helper_duplicate_bridge_state(struct drm_bridge *bridge) > > +{ > > + struct drm_bridge_state *old = NULL, *new; > > + > > + new = kzalloc(sizeof(*new), GFP_KERNEL); > > + if (!new) > > + return NULL; > > + > > + if (bridge->base.state) { > > + old = drm_priv_to_bridge_state(bridge->base.state); > > + drm_atomic_helper_copy_bridge_state(bridge, old, new); > > + } else { > > + drm_atomic_helper_init_bridge_state(bridge, new); > > + } > > + > > + return new; > > +} > > +EXPORT_SYMBOL(drm_atomic_helper_duplicate_bridge_state); > > + > > +/** > > + * drm_atomic_helper_destroy_bridge_state() - Default destroy state helper > > + * @bridge: the bridge this state refers to > > + * @state: state object to destroy > > + * > > + * Just a simple kfree() for now. > > + */ > > +void drm_atomic_helper_destroy_bridge_state(struct drm_bridge *bridge, > > + struct drm_bridge_state *state) > > +{ > > + kfree(state); > > +} > > +EXPORT_SYMBOL(drm_atomic_helper_destroy_bridge_state); > > + > > #ifdef CONFIG_OF > > /** > > * of_drm_find_bridge - find the bridge corresponding to the device node in > > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > > index 927e1205d7aa..5ee25fd51e80 100644 > > --- a/include/drm/drm_atomic.h > > +++ b/include/drm/drm_atomic.h > > @@ -660,6 +660,9 @@ __drm_atomic_get_current_plane_state(struct drm_atomic_state *state, > > return plane->state; > > } > > > > +int __must_check > > +drm_atomic_add_encoder_bridges(struct drm_atomic_state *state, > > + struct drm_encoder *encoder); > > int __must_check > > drm_atomic_add_affected_connectors(struct drm_atomic_state *state, > > struct drm_crtc *crtc); > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > > index f40181274ed7..8ca25d266d0c 100644 > > --- a/include/drm/drm_bridge.h > > +++ b/include/drm/drm_bridge.h > > @@ -25,6 +25,7 @@ > > > > #include <linux/list.h> > > #include <linux/ctype.h> > > +#include <drm/drm_atomic.h> > > #include <drm/drm_mode_object.h> > > #include <drm/drm_modes.h> > > > > @@ -32,6 +33,23 @@ struct drm_bridge; > > struct drm_bridge_timings; > > struct drm_panel; > > > > +/** > > + * struct drm_bridge_state - Atomic bridge state object > > + * @base: inherit from &drm_private_state > > + * @bridge: the bridge this state refers to > > + */ > > +struct drm_bridge_state { > > + struct drm_private_state base; > > + > > + struct drm_bridge *bridge; > > +}; > > + > > +static inline struct drm_bridge_state * > > +drm_priv_to_bridge_state(struct drm_private_state *priv) > > +{ > > + return container_of(priv, struct drm_bridge_state, base); > > +} > > + > > /** > > * struct drm_bridge_funcs - drm_bridge control functions > > */ > > @@ -337,6 +355,30 @@ struct drm_bridge_funcs { > > */ > > void (*atomic_post_disable)(struct drm_bridge *bridge, > > struct drm_atomic_state *state); > > + > > + /** > > + * @atomic_duplicate_state: > > + * > > + * Duplicate the current bridge state object. > > + * > > + * Note that this function can be called when current bridge state is > > + * NULL. In this case, implementations should either implement HW > > + * readback or initialize the state with sensible default values. > > Related to the question above, shouldn't we have a reset state operation > instead ? I had a version with a reset hook and a helper that was falling back to ->duplicate_state() when not specified, but I decided to drop it to simplify things (I think most drivers will just use the default helper, and those that actually want to implement HW readback can easily do it from their ->duplicate_state() hook by testing bridge->state value). I can re-introduce it if you like.
Hi Boris, On Thu, Aug 22, 2019 at 11:00:56AM +0200, Boris Brezillon wrote: > On Wed, 21 Aug 2019 23:14:56 +0300 Laurent Pinchart wrote: > > > > +int > > > +drm_atomic_add_encoder_bridges(struct drm_atomic_state *state, > > > + struct drm_encoder *encoder) > > > +{ > > > + struct drm_bridge_state *bridge_state; > > > + struct drm_bridge *bridge; > > > + > > > + if (!encoder) > > > + return 0; > > > + > > > + DRM_DEBUG_ATOMIC("Adding all bridges for [encoder:%d:%s] to %p\n", > > > + encoder->base.id, encoder->name, state); > > > + > > > + for (bridge = drm_bridge_chain_get_first_bridge(encoder); bridge; > > > + bridge = drm_bridge_chain_get_next_bridge(bridge)) { > > > > Would a for_each_bridge() macro make sense ? I'd implement it on top of > > list_for_each_entry() to make it simple. > > I can add this helper. Note that for_each_bridge() will iterate over > the whole bridge chain, and we might need a for_each_bridge_from() at > some point if we want to iterate over a sub-chain. > > > > + bridge_state = drm_atomic_get_bridge_state(state, bridge); > > > + if (IS_ERR(bridge_state)) > > > + return PTR_ERR(bridge_state); > > > + } > > > + > > > + return 0; > > > +} > > > +EXPORT_SYMBOL(drm_atomic_add_encoder_bridges); > > [...] > > > > +/** > > > + * drm_atomic_helper_init_bridge_state() - Initializes a bridge state > > > > s/Initializes/Initialize/ > > > > > + * @bridge this state is referring to > > > > Did you mean > > > > * @bridge: the bridge this state is referring to ? > > Yes. > > > > + * @state: bridge state to initialize > > > + * > > > + * For now it's just a memset(0) plus a state->bridge assignment. Might > > > + * be extended in the future. > > > + */ > > > +void drm_atomic_helper_init_bridge_state(struct drm_bridge *bridge, > > > + struct drm_bridge_state *state) > > > +{ > > > + memset(state, 0, sizeof(*state)); > > > + state->bridge = bridge; > > > +} > > > +EXPORT_SYMBOL(drm_atomic_helper_init_bridge_state); > > > > Other objects don't export a state init function helper, but state reset > > and state duplicate helpers. Is there a reason to depart from that model > > ? Same for the copy helper below. > > I thought those names better reflect what the helpers do. > __drm_atomic_helper_crtc_duplicate_state() for instance, it actually > does not duplicate the state, but just copies current state info into > the new state object which has been allocated by the caller. Same for > the reset helpers, they actually do not reset anything but just > initialize the state to a default. I also try to avoid prefixing > functions with __ when I can (names can be clarified/extended to avoid > conflicts most of the time). > Will switch back to reset/dup names if you prefer. I think consistency would help. Regarding the reset operation, unless I'm mistaken, it got its name from the fact that it should create a software state that reflects the current hardware state, in order to support fastboot (taking over a running display controller started by the boot loader without causing any glitch). That being said, I'm not sure reset was the best name even for that :-) > > > + > > > +/** > > > + * drm_atomic_helper_copy_bridge_state() - Copy the content of a bridge state > > > + * @bridge: bridge the old and new state are referring to > > > + * @old: previous bridge state to copy from > > > + * @new: new bridge state to copy to > > > + * > > > + * Should be used by custom &drm_bridge_funcs.atomic_duplicate() implementation > > > + * to copy the previous state into the new object. > > > + */ > > > +void drm_atomic_helper_copy_bridge_state(struct drm_bridge *bridge, > > > + const struct drm_bridge_state *old, > > > + struct drm_bridge_state *new) > > > +{ > > > + *new = *old; > > > +} > > > +EXPORT_SYMBOL(drm_atomic_helper_copy_bridge_state); > > > + > > > +/** > > > + * drm_atomic_helper_duplicate_bridge_state() - Default duplicate state helper > > > + * @bridge: bridge containing the state to duplicate > > > + * > > > + * Default implementation of &drm_bridge_funcs.atomic_duplicate(). > > > + * > > > + * RETURNS: > > > + * a valid state object or NULL if the allocation fails. > > > + */ > > > +struct drm_bridge_state * > > > +drm_atomic_helper_duplicate_bridge_state(struct drm_bridge *bridge) > > > +{ > > > + struct drm_bridge_state *old = NULL, *new; > > > + > > > + new = kzalloc(sizeof(*new), GFP_KERNEL); > > > + if (!new) > > > + return NULL; > > > + > > > + if (bridge->base.state) { > > > + old = drm_priv_to_bridge_state(bridge->base.state); > > > + drm_atomic_helper_copy_bridge_state(bridge, old, new); > > > + } else { > > > + drm_atomic_helper_init_bridge_state(bridge, new); > > > + } > > > + > > > + return new; > > > +} > > > +EXPORT_SYMBOL(drm_atomic_helper_duplicate_bridge_state); > > > + > > > +/** > > > + * drm_atomic_helper_destroy_bridge_state() - Default destroy state helper > > > + * @bridge: the bridge this state refers to > > > + * @state: state object to destroy > > > + * > > > + * Just a simple kfree() for now. > > > + */ > > > +void drm_atomic_helper_destroy_bridge_state(struct drm_bridge *bridge, > > > + struct drm_bridge_state *state) > > > +{ > > > + kfree(state); > > > +} > > > +EXPORT_SYMBOL(drm_atomic_helper_destroy_bridge_state); > > > + > > > #ifdef CONFIG_OF > > > /** > > > * of_drm_find_bridge - find the bridge corresponding to the device node in > > > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > > > index 927e1205d7aa..5ee25fd51e80 100644 > > > --- a/include/drm/drm_atomic.h > > > +++ b/include/drm/drm_atomic.h > > > @@ -660,6 +660,9 @@ __drm_atomic_get_current_plane_state(struct drm_atomic_state *state, > > > return plane->state; > > > } > > > > > > +int __must_check > > > +drm_atomic_add_encoder_bridges(struct drm_atomic_state *state, > > > + struct drm_encoder *encoder); > > > int __must_check > > > drm_atomic_add_affected_connectors(struct drm_atomic_state *state, > > > struct drm_crtc *crtc); > > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > > > index f40181274ed7..8ca25d266d0c 100644 > > > --- a/include/drm/drm_bridge.h > > > +++ b/include/drm/drm_bridge.h > > > @@ -25,6 +25,7 @@ > > > > > > #include <linux/list.h> > > > #include <linux/ctype.h> > > > +#include <drm/drm_atomic.h> > > > #include <drm/drm_mode_object.h> > > > #include <drm/drm_modes.h> > > > > > > @@ -32,6 +33,23 @@ struct drm_bridge; > > > struct drm_bridge_timings; > > > struct drm_panel; > > > > > > +/** > > > + * struct drm_bridge_state - Atomic bridge state object > > > + * @base: inherit from &drm_private_state > > > + * @bridge: the bridge this state refers to > > > + */ > > > +struct drm_bridge_state { > > > + struct drm_private_state base; > > > + > > > + struct drm_bridge *bridge; > > > +}; > > > + > > > +static inline struct drm_bridge_state * > > > +drm_priv_to_bridge_state(struct drm_private_state *priv) > > > +{ > > > + return container_of(priv, struct drm_bridge_state, base); > > > +} > > > + > > > /** > > > * struct drm_bridge_funcs - drm_bridge control functions > > > */ > > > @@ -337,6 +355,30 @@ struct drm_bridge_funcs { > > > */ > > > void (*atomic_post_disable)(struct drm_bridge *bridge, > > > struct drm_atomic_state *state); > > > + > > > + /** > > > + * @atomic_duplicate_state: > > > + * > > > + * Duplicate the current bridge state object. > > > + * > > > + * Note that this function can be called when current bridge state is > > > + * NULL. In this case, implementations should either implement HW > > > + * readback or initialize the state with sensible default values. > > > > Related to the question above, shouldn't we have a reset state operation > > instead ? > > I had a version with a reset hook and a helper that was falling back to > ->duplicate_state() when not specified, but I decided to drop it to > simplify things (I think most drivers will just use the default > helper, and those that actually want to implement HW readback can > easily do it from their ->duplicate_state() hook by testing > bridge->state value). > I can re-introduce it if you like. It would help with consistency, but I'll leave that to you.
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 419381abbdd1..ed6fd3e31b56 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -30,6 +30,7 @@ #include <drm/drm_atomic.h> #include <drm/drm_atomic_uapi.h> +#include <drm/drm_bridge.h> #include <drm/drm_debugfs.h> #include <drm/drm_device.h> #include <drm/drm_drv.h> @@ -1010,6 +1011,45 @@ static void drm_atomic_connector_print_state(struct drm_printer *p, connector->funcs->atomic_print_state(p, state); } +/** + * drm_atomic_add_encoder_bridges - add bridges attached to an encoder + * @state: atomic state + * @encoder: DRM encoder + * + * This function adds all bridges attached to @encoder. This is needed to add + * bridge states to @state and make them available when + * &bridge_funcs.atomic_{check,pre_enable,enable,disable_post_disable}() are + * called + * + * Returns: + * 0 on success or can fail with -EDEADLK or -ENOMEM. When the error is EDEADLK + * then the w/w mutex code has detected a deadlock and the entire atomic + * sequence must be restarted. All other errors are fatal. + */ +int +drm_atomic_add_encoder_bridges(struct drm_atomic_state *state, + struct drm_encoder *encoder) +{ + struct drm_bridge_state *bridge_state; + struct drm_bridge *bridge; + + if (!encoder) + return 0; + + DRM_DEBUG_ATOMIC("Adding all bridges for [encoder:%d:%s] to %p\n", + encoder->base.id, encoder->name, state); + + for (bridge = drm_bridge_chain_get_first_bridge(encoder); bridge; + bridge = drm_bridge_chain_get_next_bridge(bridge)) { + bridge_state = drm_atomic_get_bridge_state(state, bridge); + if (IS_ERR(bridge_state)) + return PTR_ERR(bridge_state); + } + + return 0; +} +EXPORT_SYMBOL(drm_atomic_add_encoder_bridges); + /** * drm_atomic_add_affected_connectors - add connectors for crtc * @state: atomic state diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 0deb54099570..e76ec9648b6f 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -736,6 +736,24 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, return ret; } + /* + * Iterate over all connectors again, and add all affected bridges to + * the state. + */ + for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i) { + struct drm_encoder *encoder; + + encoder = old_connector_state->best_encoder; + ret = drm_atomic_add_encoder_bridges(state, encoder); + if (ret) + return ret; + + encoder = new_connector_state->best_encoder; + ret = drm_atomic_add_encoder_bridges(state, encoder); + if (ret) + return ret; + } + ret = mode_valid(state); if (ret) return ret; diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index ed2410f49581..0280da6be1e6 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -89,6 +89,38 @@ void drm_bridge_remove(struct drm_bridge *bridge) } EXPORT_SYMBOL(drm_bridge_remove); +static struct drm_private_state * +drm_bridge_atomic_duplicate_priv_state(struct drm_private_obj *obj) +{ + struct drm_bridge *bridge = drm_priv_to_bridge(obj); + struct drm_bridge_state *state; + + if (bridge->funcs->atomic_duplicate_state) + state = bridge->funcs->atomic_duplicate_state(bridge); + else + state = drm_atomic_helper_duplicate_bridge_state(bridge); + + return &state->base; +} + +static void +drm_bridge_atomic_destroy_priv_state(struct drm_private_obj *obj, + struct drm_private_state *s) +{ + struct drm_bridge_state *state = drm_priv_to_bridge_state(s); + struct drm_bridge *bridge = drm_priv_to_bridge(obj); + + if (bridge->funcs->atomic_destroy_state) + bridge->funcs->atomic_destroy_state(bridge, state); + else + drm_atomic_helper_destroy_bridge_state(bridge, state); +} + +static const struct drm_private_state_funcs drm_bridge_priv_state_funcs = { + .atomic_duplicate_state = drm_bridge_atomic_duplicate_priv_state, + .atomic_destroy_state = drm_bridge_atomic_destroy_priv_state, +}; + /** * drm_bridge_attach - attach the bridge to an encoder's chain * @@ -114,6 +146,7 @@ EXPORT_SYMBOL(drm_bridge_remove); int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, struct drm_bridge *previous) { + struct drm_bridge_state *state; LIST_HEAD(tmp_list); int ret; @@ -132,19 +165,39 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, if (bridge->funcs->attach) { ret = bridge->funcs->attach(bridge); - if (ret < 0) { - bridge->dev = NULL; - bridge->encoder = NULL; - return ret; - } + if (ret < 0) + goto err_reset_bridge; } + if (bridge->funcs->atomic_duplicate_state) + state = bridge->funcs->atomic_duplicate_state(bridge); + else + state = drm_atomic_helper_duplicate_bridge_state(bridge); + + if (!state) { + ret = -ENOMEM; + goto err_detach_bridge; + } + + drm_atomic_private_obj_init(bridge->dev, &bridge->base, + &state->base, + &drm_bridge_priv_state_funcs); + if (previous) list_splice(&tmp_list, &previous->chain_node); else list_splice(&tmp_list, &encoder->bridge_chain); return 0; + +err_detach_bridge: + if (bridge->funcs->detach) + bridge->funcs->detach(bridge); + +err_reset_bridge: + bridge->dev = NULL; + bridge->encoder = NULL; + return ret; } EXPORT_SYMBOL(drm_bridge_attach); @@ -156,6 +209,8 @@ void drm_bridge_detach(struct drm_bridge *bridge) if (WARN_ON(!bridge->dev)) return; + drm_atomic_private_obj_fini(&bridge->base); + if (bridge->funcs->detach) bridge->funcs->detach(bridge); @@ -495,6 +550,82 @@ void drm_atomic_bridge_chain_enable(struct drm_encoder *encoder, } EXPORT_SYMBOL(drm_atomic_bridge_chain_enable); +/** + * drm_atomic_helper_init_bridge_state() - Initializes a bridge state + * @bridge this state is referring to + * @state: bridge state to initialize + * + * For now it's just a memset(0) plus a state->bridge assignment. Might + * be extended in the future. + */ +void drm_atomic_helper_init_bridge_state(struct drm_bridge *bridge, + struct drm_bridge_state *state) +{ + memset(state, 0, sizeof(*state)); + state->bridge = bridge; +} +EXPORT_SYMBOL(drm_atomic_helper_init_bridge_state); + +/** + * drm_atomic_helper_copy_bridge_state() - Copy the content of a bridge state + * @bridge: bridge the old and new state are referring to + * @old: previous bridge state to copy from + * @new: new bridge state to copy to + * + * Should be used by custom &drm_bridge_funcs.atomic_duplicate() implementation + * to copy the previous state into the new object. + */ +void drm_atomic_helper_copy_bridge_state(struct drm_bridge *bridge, + const struct drm_bridge_state *old, + struct drm_bridge_state *new) +{ + *new = *old; +} +EXPORT_SYMBOL(drm_atomic_helper_copy_bridge_state); + +/** + * drm_atomic_helper_duplicate_bridge_state() - Default duplicate state helper + * @bridge: bridge containing the state to duplicate + * + * Default implementation of &drm_bridge_funcs.atomic_duplicate(). + * + * RETURNS: + * a valid state object or NULL if the allocation fails. + */ +struct drm_bridge_state * +drm_atomic_helper_duplicate_bridge_state(struct drm_bridge *bridge) +{ + struct drm_bridge_state *old = NULL, *new; + + new = kzalloc(sizeof(*new), GFP_KERNEL); + if (!new) + return NULL; + + if (bridge->base.state) { + old = drm_priv_to_bridge_state(bridge->base.state); + drm_atomic_helper_copy_bridge_state(bridge, old, new); + } else { + drm_atomic_helper_init_bridge_state(bridge, new); + } + + return new; +} +EXPORT_SYMBOL(drm_atomic_helper_duplicate_bridge_state); + +/** + * drm_atomic_helper_destroy_bridge_state() - Default destroy state helper + * @bridge: the bridge this state refers to + * @state: state object to destroy + * + * Just a simple kfree() for now. + */ +void drm_atomic_helper_destroy_bridge_state(struct drm_bridge *bridge, + struct drm_bridge_state *state) +{ + kfree(state); +} +EXPORT_SYMBOL(drm_atomic_helper_destroy_bridge_state); + #ifdef CONFIG_OF /** * of_drm_find_bridge - find the bridge corresponding to the device node in diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 927e1205d7aa..5ee25fd51e80 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -660,6 +660,9 @@ __drm_atomic_get_current_plane_state(struct drm_atomic_state *state, return plane->state; } +int __must_check +drm_atomic_add_encoder_bridges(struct drm_atomic_state *state, + struct drm_encoder *encoder); int __must_check drm_atomic_add_affected_connectors(struct drm_atomic_state *state, struct drm_crtc *crtc); diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index f40181274ed7..8ca25d266d0c 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -25,6 +25,7 @@ #include <linux/list.h> #include <linux/ctype.h> +#include <drm/drm_atomic.h> #include <drm/drm_mode_object.h> #include <drm/drm_modes.h> @@ -32,6 +33,23 @@ struct drm_bridge; struct drm_bridge_timings; struct drm_panel; +/** + * struct drm_bridge_state - Atomic bridge state object + * @base: inherit from &drm_private_state + * @bridge: the bridge this state refers to + */ +struct drm_bridge_state { + struct drm_private_state base; + + struct drm_bridge *bridge; +}; + +static inline struct drm_bridge_state * +drm_priv_to_bridge_state(struct drm_private_state *priv) +{ + return container_of(priv, struct drm_bridge_state, base); +} + /** * struct drm_bridge_funcs - drm_bridge control functions */ @@ -337,6 +355,30 @@ struct drm_bridge_funcs { */ void (*atomic_post_disable)(struct drm_bridge *bridge, struct drm_atomic_state *state); + + /** + * @atomic_duplicate_state: + * + * Duplicate the current bridge state object. + * + * Note that this function can be called when current bridge state is + * NULL. In this case, implementations should either implement HW + * readback or initialize the state with sensible default values. + * + * RETURNS: + * A valid drm_bridge_state object or NULL if the allocation fails. + */ + struct drm_bridge_state *(*atomic_duplicate_state)(struct drm_bridge *bridge); + + /** + * @atomic_duplicate_state: + * + * Destroy a bridge state object previously allocated by + * &drm_bridge_funcs.atomic_duplicate_state() or + * &drm_bridge_funcs.atomic_get_initial_state(). + */ + void (*atomic_destroy_state)(struct drm_bridge *bridge, + struct drm_bridge_state *state); }; /** @@ -379,6 +421,8 @@ struct drm_bridge_timings { * struct drm_bridge - central DRM bridge control structure */ struct drm_bridge { + /** @base: inherit from &drm_private_object */ + struct drm_private_obj base; /** @dev: DRM device this bridge belongs to */ struct drm_device *dev; /** @encoder: encoder to which this bridge is connected */ @@ -403,6 +447,12 @@ struct drm_bridge { void *driver_private; }; +static inline struct drm_bridge * +drm_priv_to_bridge(struct drm_private_obj *priv) +{ + return container_of(priv, struct drm_bridge, base); +} + void drm_bridge_add(struct drm_bridge *bridge); void drm_bridge_remove(struct drm_bridge *bridge); struct drm_bridge *of_drm_find_bridge(struct device_node *np); @@ -438,6 +488,55 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_encoder *encoder, void drm_atomic_bridge_chain_enable(struct drm_encoder *encoder, struct drm_atomic_state *state); +void drm_atomic_helper_init_bridge_state(struct drm_bridge *bridge, + struct drm_bridge_state *state); +void drm_atomic_helper_copy_bridge_state(struct drm_bridge *bridge, + const struct drm_bridge_state *old, + struct drm_bridge_state *new); +struct drm_bridge_state * +drm_atomic_helper_duplicate_bridge_state(struct drm_bridge *bridge); +void drm_atomic_helper_destroy_bridge_state(struct drm_bridge *bridge, + struct drm_bridge_state *state); + +static inline struct drm_bridge_state * +drm_atomic_get_bridge_state(struct drm_atomic_state *state, + struct drm_bridge *bridge) +{ + struct drm_private_state *obj_state; + + obj_state = drm_atomic_get_private_obj_state(state, &bridge->base); + if (!obj_state) + return NULL; + + return drm_priv_to_bridge_state(obj_state); +} + +static inline struct drm_bridge_state * +drm_atomic_get_old_bridge_state(struct drm_atomic_state *state, + struct drm_bridge *bridge) +{ + struct drm_private_state *obj_state; + + obj_state = drm_atomic_get_old_private_obj_state(state, &bridge->base); + if (!obj_state) + return NULL; + + return drm_priv_to_bridge_state(obj_state); +} + +static inline struct drm_bridge_state * +drm_atomic_get_new_bridge_state(struct drm_atomic_state *state, + struct drm_bridge *bridge) +{ + struct drm_private_state *obj_state; + + obj_state = drm_atomic_get_new_private_obj_state(state, &bridge->base); + if (!obj_state) + return NULL; + + return drm_priv_to_bridge_state(obj_state); +} + #ifdef CONFIG_DRM_PANEL_BRIDGE struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel, u32 connector_type);
One of the last remaining object to not have its atomic state. This is being motivated by our attempt to support runtime bus-format negotiation between elements of the bridge chain. This patch just paves the road for such a feature by adding a new drm_bridge_state object inheriting from drm_private_obj so we can re-use some of the state initialization/tracking logic. Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com> --- drivers/gpu/drm/drm_atomic.c | 40 ++++++++ drivers/gpu/drm/drm_atomic_helper.c | 18 ++++ drivers/gpu/drm/drm_bridge.c | 141 +++++++++++++++++++++++++++- include/drm/drm_atomic.h | 3 + include/drm/drm_bridge.h | 99 +++++++++++++++++++ 5 files changed, 296 insertions(+), 5 deletions(-)