From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 24428C43603 for ; Thu, 19 Dec 2019 21:52:31 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id E0B4E222C2 for ; Thu, 19 Dec 2019 21:52:30 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726998AbfLSVwa convert rfc822-to-8bit (ORCPT ); Thu, 19 Dec 2019 16:52:30 -0500 Received: from mailoutvs21.siol.net ([185.57.226.212]:46776 "EHLO mail.siol.net" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726959AbfLSVwa (ORCPT ); Thu, 19 Dec 2019 16:52:30 -0500 Received: from localhost (localhost [127.0.0.1]) by mail.siol.net (Zimbra) with ESMTP id C5C44523947; Thu, 19 Dec 2019 22:52:23 +0100 (CET) X-Virus-Scanned: amavisd-new at psrvmta12.zcs-production.pri Received: from mail.siol.net ([127.0.0.1]) by localhost (psrvmta12.zcs-production.pri [127.0.0.1]) (amavisd-new, port 10032) with ESMTP id 0cPA_XEhSDke; Thu, 19 Dec 2019 22:52:22 +0100 (CET) Received: from mail.siol.net (localhost [127.0.0.1]) by mail.siol.net (Zimbra) with ESMTPS id B154D523AD3; Thu, 19 Dec 2019 22:52:22 +0100 (CET) Received: from jernej-laptop.localnet (cpe-86-58-102-7.static.triera.net [86.58.102.7]) (Authenticated sender: jernej.skrabec@siol.net) by mail.siol.net (Zimbra) with ESMTPA id D8561523947; Thu, 19 Dec 2019 22:52:20 +0100 (CET) From: Jernej =?utf-8?B?xaBrcmFiZWM=?= To: Neil Armstrong Cc: dri-devel@lists.freedesktop.org, Boris Brezillon , Mark Rutland , Thierry Reding , Laurent Pinchart , kernel@collabora.com, Sam Ravnborg , Nikita Yushchenko , Andrey Smirnov , Kyungmin Park , Chris Healy , devicetree@vger.kernel.org, Jonas Karlman , Rob Herring , Seung-Woo Kim Subject: Re: [PATCH v5 1/4] drm/bridge: Add a drm_bridge_state object Date: Thu, 19 Dec 2019 22:52:20 +0100 Message-ID: <2305991.HLB9IM4Dtq@jernej-laptop> In-Reply-To: <20191219101151.28039-2-narmstrong@baylibre.com> References: <20191219101151.28039-1-narmstrong@baylibre.com> <20191219101151.28039-2-narmstrong@baylibre.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8BIT Content-Type: text/plain; charset="UTF-8" Sender: devicetree-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: devicetree@vger.kernel.org Hi! Dne četrtek, 19. december 2019 ob 11:11:48 CET je Neil Armstrong napisal(a): > From: Boris Brezillon > > 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 > Reviewed-by: Neil Armstrong > Reviewed-by: Laurent Pinchart > Signed-off-by: Neil Armstrong > --- Reviewed by: Jernej Skrabec Best regards, Jernej > Changes in v5: > * Re-introduced the helpers from v4 > > Changes in v4: > * Fix the doc > * Kill default helpers (inlined) > * Fix drm_atomic_get_bridge_state() to check for an ERR_PTR() > * Add Neil's R-b > > 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 | 169 +++++++++++++++++++++++++++- > include/drm/drm_atomic.h | 3 + > include/drm/drm_bridge.h | 120 ++++++++++++++++++++ > 5 files changed, 345 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index ab4508f25986..0b7db3d9c610 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 > @@ -1017,6 +1018,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 > + * &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); > + > + 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 57a84555a6bd..b3e1aedd9d7a > 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 c2cf0c90fa26..a3921b45f044 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 state ? &state->base : NULL; > +} > + > +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_bridge_destroy_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 +147,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; > int ret; > > if (!encoder || !bridge) > @@ -135,15 +169,35 @@ int drm_bridge_attach(struct drm_encoder *encoder, > struct drm_bridge *bridge, > > if (bridge->funcs->attach) { > ret = bridge->funcs->attach(bridge); > - if (ret < 0) { > - list_del(&bridge->chain_node); > - bridge->dev = NULL; > - bridge->encoder = NULL; > - return ret; > - } > + if (ret < 0) > + goto err_reset_bridge; > + } > + > + if (bridge->funcs->atomic_reset) > + state = bridge->funcs->atomic_reset(bridge); > + else > + state = drm_atomic_helper_bridge_reset(bridge); > + > + if (IS_ERR(state)) { > + ret = PTR_ERR(state); > + goto err_detach_bridge; > } > > + drm_atomic_private_obj_init(bridge->dev, &bridge->base, > + &state->base, > + &drm_bridge_priv_state_funcs); > + > return 0; > + > +err_detach_bridge: > + if (bridge->funcs->detach) > + bridge->funcs->detach(bridge); > + > +err_reset_bridge: > + bridge->dev = NULL; > + bridge->encoder = NULL; > + list_del(&bridge->chain_node); > + return ret; > } > EXPORT_SYMBOL(drm_bridge_attach); > > @@ -155,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); > > @@ -516,6 +572,107 @@ void drm_atomic_bridge_chain_enable(struct drm_bridge > *bridge, } > EXPORT_SYMBOL(drm_atomic_bridge_chain_enable); > > +/** > + * drm_atomic_helper_bridge_destroy_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_bridge_destroy_state(struct drm_bridge *bridge, > + struct drm_bridge_state *state) > +{ > + kfree(state); > +} > +EXPORT_SYMBOL(drm_atomic_helper_bridge_destroy_state); > + > +/** > + * __drm_atomic_helper_bridge_reset() - Initialize a bridge state to its > + * default > + * @bridge: the bridge this state is refers to > + * @state: bridge state to initialize > + * > + * Initialize the bridge state to default values. This is meant to be* > called + * by the bridge &drm_plane_funcs.reset hook for bridges that > subclass the + * bridge state. > + */ > +void __drm_atomic_helper_bridge_reset(struct drm_bridge *bridge, > + struct drm_bridge_state *state) > +{ > + memset(state, 0, sizeof(*state)); > + state->bridge = bridge; > +} > +EXPORT_SYMBOL(__drm_atomic_helper_bridge_reset); > + > +/** > + * drm_atomic_helper_bridge_reset() - default &drm_plane_funcs.reset hook > for + * bridges > + * @bridge: the bridge to reset state on > + * > + * Resets the atomic state for @bridge by freeing the state pointer (which > + * might be NULL, e.g. at driver load time) and allocating a new empty > state + * object. > + * > + * 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 * > +drm_atomic_helper_bridge_reset(struct drm_bridge *bridge) > +{ > + struct drm_bridge_state *bridge_state; > + > + bridge_state = kzalloc(sizeof(*bridge_state), GFP_KERNEL); > + if (!bridge_state) > + return ERR_PTR(-ENOMEM); > + > + __drm_atomic_helper_bridge_reset(bridge, bridge_state); > + return bridge_state; > +} > +EXPORT_SYMBOL(drm_atomic_helper_bridge_reset); > + > +/** > + * __drm_atomic_helper_bridge_duplicate_state() - Copy atomic bridge state > + * @bridge: bridge object > + * @state: atomic bridge state > + * > + * Copies atomic state from a bridge's current state and resets inferred > values. + * This is useful for drivers that subclass the bridge state. > + */ > +void __drm_atomic_helper_bridge_duplicate_state(struct drm_bridge *bridge, > + struct drm_bridge_state *state) > +{ > + __drm_atomic_helper_private_obj_duplicate_state(&bridge->base, > + &state->base); > + state->bridge = bridge; > +} > +EXPORT_SYMBOL(__drm_atomic_helper_bridge_duplicate_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_bridge_duplicate_state(struct drm_bridge *bridge) > +{ > + struct drm_bridge_state *new; > + > + if (WARN_ON(!bridge->base.state)) > + return NULL; > + > + new = kzalloc(sizeof(*new), GFP_KERNEL); > + if (new) > + __drm_atomic_helper_bridge_duplicate_state(bridge, new); > + > + return new; > +} > +EXPORT_SYMBOL(drm_atomic_helper_bridge_duplicate_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 5923819dcd68..62a2e30d5aaf 100644 > --- a/include/drm/drm_atomic.h > +++ b/include/drm/drm_atomic.h > @@ -669,6 +669,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 694e153a7531..8a926c1a08db 100644 > --- a/include/drm/drm_bridge.h > +++ b/include/drm/drm_bridge.h > @@ -25,6 +25,8 @@ > > #include > #include > + > +#include > #include > #include > #include > @@ -33,6 +35,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 > */ > @@ -338,6 +357,49 @@ struct drm_bridge_funcs { > */ > void (*atomic_post_disable)(struct drm_bridge *bridge, > struct drm_atomic_state *old_state); > + > + /** > + * @atomic_duplicate_state: > + * > + * Duplicate the current bridge state object (which is guaranteed to be > + * non-NULL). > + * > + * The atomic_duplicate_state() is optional. When not implemented the > + * core allocates a drm_bridge_state object and calls > + * &__drm_atomic_helper_bridge_duplicate_state() to initialize it. > + * > + * 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 > + * &drm_bridge_funcs.atomic_duplicate_state(). > + * > + * The atomic_destroy_state hook is optional. When not implemented the > + * core calls kfree() on the state. > + */ > + 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 &drm_bridge_state object matching this state. > + * This function is called at attach time. > + * > + * The atomic_reset hook is optional. When not implemented the core > + * allocates a new state and calls &__drm_atomic_helper_bridge_reset(). > + * > + * RETURNS: > + * A valid drm_bridge_state object in case of success, an ERR_PTR() > + * giving the reason of the failure otherwise. > + */ > + struct drm_bridge_state *(*atomic_reset)(struct drm_bridge *bridge); > }; > > /** > @@ -380,6 +442,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 */ > @@ -404,6 +468,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); > @@ -491,6 +561,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); > +void drm_atomic_helper_bridge_destroy_state(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_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, &bridge->base); > + if (IS_ERR(obj_state)) > + return ERR_CAST(obj_state); > + > + 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); > struct drm_bridge *drm_panel_bridge_add_typed(struct drm_panel *panel,