Re: [PATCH v3 09/21] drm/bridge: Add a drm_bridge_state object
Hi Boris, Thank you for the patch. On Wed, Oct 23, 2019 at 05:45:00PM +0200, Boris Brezillon wrote: > One of the last remaining objects 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 existing state initialization/tracking logic. > > Signed-off-by: Boris Brezillon > --- > Changes in v3: > * None > > Changes in v2: > * Use drm_for_each_bridge_in_chain() > * Rename helpers to be more consistent with the rest of the DRM API > * Improve/fix the doc > --- > drivers/gpu/drm/drm_atomic.c| 39 +++ > drivers/gpu/drm/drm_atomic_helper.c | 20 > drivers/gpu/drm/drm_bridge.c| 168 +++- > include/drm/drm_atomic.h| 3 + > include/drm/drm_bridge.h| 118 +++ > 5 files changed, 343 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 419381abbdd1..6c249ce39380 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -30,6 +30,7 @@ > > #include > #include > +#include > #include > #include > #include > @@ -1010,6 +1011,44 @@ 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 > + * _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); > + > + drm_for_each_bridge_in_chain(encoder, 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 f02ddffd4960..de985ba7ce2d 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -730,6 +730,26 @@ 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 c5cf8a9c4237..c53966767782 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -25,6 +25,7 @@ > #include > #include > > +#include > #include > #include > > @@ -89,6 +90,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 =
Re: [PATCH v3 09/21] drm/bridge: Add a drm_bridge_state object
On Tue, 5 Nov 2019 17:05:16 +0100 Neil Armstrong wrote: > > 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); > > @@ -475,6 +543,56 @@ void drm_atomic_bridge_chain_pre_enable(struct > > drm_bridge *bridge, > > void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge, > > struct drm_atomic_state *state); > > > > +void __drm_atomic_helper_bridge_reset(struct drm_bridge *bridge, > > + struct drm_bridge_state *state); > > +struct drm_bridge_state * > > +drm_atomic_helper_bridge_reset(struct drm_bridge *bridge); > > +void drm_atomic_helper_bridge_destroy_state(struct drm_bridge *bridge, > > + struct drm_bridge_state *state); > > +void __drm_atomic_helper_bridge_duplicate_state(struct drm_bridge *bridge, > > + struct drm_bridge_state *new); > > +struct drm_bridge_state * > > +drm_atomic_helper_bridge_duplicate_state(struct drm_bridge *bridge); > > + > > +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, >base); > > + if (!obj_state) > > + return NULL; > > drm_atomic_get_private_obj_state will return an error object on error, so > should be: > if (IS_ERR(obj_state)) > return ERR_CAST(obj_state); Right, I'll fix it in v4. Thanks, Boris ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 09/21] drm/bridge: Add a drm_bridge_state object
On 23/10/2019 17:45, Boris Brezillon wrote: > One of the last remaining objects 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 existing state initialization/tracking logic. > > Signed-off-by: Boris Brezillon > --- > Changes in v3: > * None > > Changes in v2: > * Use drm_for_each_bridge_in_chain() > * Rename helpers to be more consistent with the rest of the DRM API > * Improve/fix the doc > --- > drivers/gpu/drm/drm_atomic.c| 39 +++ > drivers/gpu/drm/drm_atomic_helper.c | 20 > drivers/gpu/drm/drm_bridge.c| 168 +++- > include/drm/drm_atomic.h| 3 + > include/drm/drm_bridge.h| 118 +++ > 5 files changed, 343 insertions(+), 5 deletions(-) > [...] > --- 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 238e84ab63a3..a608c47d1de5 100644 > --- a/include/drm/drm_bridge.h > +++ b/include/drm/drm_bridge.h > @@ -25,6 +25,7 @@ > > #include > #include > +#include > #include > #include > #include > @@ -33,6 +34,23 @@ struct drm_bridge; > struct drm_bridge_timings; > struct drm_panel; > > +/** > + * struct drm_bridge_state - Atomic bridge state object > + * @base: inherit from _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 > */ > @@ -338,6 +356,48 @@ 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 (which is guaranteed to be > + * non-NULL). > + * > + * The atomic_duplicate_state() is optional, the core falls back on > + * _atomic_helper_bridge_duplicate_state() when not implemented. > + * > + * 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_destroy_state: > + * > + * Destroy a bridge state object previously allocated by > + * _bridge_funcs.atomic_duplicate_state(). > + * > + * The atomic_destroy_state hook is optional, the coref falls back on > + * _atomic_helper_bridge_destroy_state() when not implemented. > + */ > + void (*atomic_destroy_state)(struct drm_bridge *bridge, > + struct drm_bridge_state *state); > + > + /** > + * @atomic_reset: > + * > + * Reset the bridge to a predefined state (or retrieve its current > + * state) and return a _bridge_state object matching this state. > + * This function is called at attach time. > + * > + * The atomic_reset hook is optional, the core falls back on > + * _atomic_helper_bridge_reset() when not implemented. > + * > + * RETURNS: > + * A valid drm_bridge_state object in case of success, an ERR_PTR() > + * giving the reaon of the failure otherwise. > + */ > + struct drm_bridge_state *(*atomic_reset)(struct drm_bridge *bridge); > }; > > /** > @@ -380,6 +440,8 @@ struct drm_bridge_timings { > * struct drm_bridge - central DRM bridge control structure > */ > struct drm_bridge { > + /** @base: inherit from _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 */ > @@ -404,6 +466,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
Re: [PATCH v3 09/21] drm/bridge: Add a drm_bridge_state object
On 23/10/2019 17:45, Boris Brezillon wrote: > One of the last remaining objects 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 existing state initialization/tracking logic. > > Signed-off-by: Boris Brezillon > --- > Changes in v3: > * None > > Changes in v2: > * Use drm_for_each_bridge_in_chain() > * Rename helpers to be more consistent with the rest of the DRM API > * Improve/fix the doc > --- > drivers/gpu/drm/drm_atomic.c| 39 +++ > drivers/gpu/drm/drm_atomic_helper.c | 20 > drivers/gpu/drm/drm_bridge.c| 168 +++- > include/drm/drm_atomic.h| 3 + > include/drm/drm_bridge.h| 118 +++ > 5 files changed, 343 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 419381abbdd1..6c249ce39380 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -30,6 +30,7 @@ > > #include > #include > +#include > #include > #include > #include > @@ -1010,6 +1011,44 @@ 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 > + * _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); > + > + drm_for_each_bridge_in_chain(encoder, 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 f02ddffd4960..de985ba7ce2d 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -730,6 +730,26 @@ 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 c5cf8a9c4237..c53966767782 100644 > --- a/drivers/gpu/drm/drm_bridge.c > +++ b/drivers/gpu/drm/drm_bridge.c > @@ -25,6 +25,7 @@ > #include > #include > > +#include > #include > #include > > @@ -89,6 +90,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_bridge_duplicate_state(bridge); > + > + return >base; > +} > + >