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 boris.brezillon@collabora.com Signed-off-by: Neil Armstrong narmstrong@baylibre.com Reviewed-by: Neil Armstrong narmstrong@baylibre.com Acked-by: Daniel Vetter daniel.vetter@ffwll.ch --- Changes in v9: * Clarify the fact that the bridge->atomic_reset() and {connector,plane,crtc,...}->reset() semantics are different * Move the drm_atomic_private_obj_init() call back to drm_bridge_attach() * Check the presence of ->atomic_duplicate_state instead of ->atomic_reset in drm_atomic_add_encoder_bridges() * Fix copy&paste errors in the atomic bridge state helpers doc * Add A-b/R-b tags

Changes in v8: * Move bridge state helpers out of the CONFIG_DEBUGFS section

Changes in v7: * Move helpers, struct-defs, ... to atomic helper files to avoid the drm -> drm_kms_helper -> drm circular dep * Stop providing default implementation for atomic state reset, duplicate and destroy hooks (has to do with the helper/core split) * Drop all R-b/T-b as helpers have now be moved to other places --- drivers/gpu/drm/drm_atomic.c | 116 ++++++++++++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 20 ++++ drivers/gpu/drm/drm_atomic_state_helper.c | 103 +++++++++++++++++++ drivers/gpu/drm/drm_bridge.c | 59 ++++++++++- include/drm/drm_atomic.h | 34 +++++++ include/drm/drm_atomic_state_helper.h | 13 +++ include/drm/drm_bridge.h | 60 +++++++++++ 7 files changed, 400 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index d33691512a8e..65c46ed049c5 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> @@ -1017,6 +1018,121 @@ static void drm_atomic_connector_print_state(struct drm_printer *p, connector->funcs->atomic_print_state(p, state); }

+/** + * drm_atomic_get_bridge_state - get bridge state + * @state: global atomic state object + * @bridge: bridge to get state object for + * + * This function returns the bridge state for the given bridge, allocating it + * if needed. It will also grab the relevant bridge lock to make sure that the + * state is consistent. + * + * Returns: + * + * Either the allocated state or the error code encoded into the pointer. When + * the error is EDEADLK then the w/w mutex code has detected a deadlock and the + * entire atomic sequence must be restarted. + */ +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); +} +EXPORT_SYMBOL(drm_atomic_get_bridge_state); + +/** + * drm_atomic_get_old_bridge_state - get old bridge state, if it exists + * @state: global atomic state object + * @bridge: bridge to grab + * + * This function returns the old bridge state for the given bridge, or NULL if + * the bridge is not part of the global atomic state. + */ +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); +} +EXPORT_SYMBOL(drm_atomic_get_old_bridge_state); + +/** + * drm_atomic_get_new_bridge_state - get new bridge state, if it exists + * @state: global atomic state object + * @bridge: bridge to grab + * + * This function returns the new bridge state for the given bridge, or NULL if + * the bridge is not part of the global atomic state. + */ +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); +} +EXPORT_SYMBOL(drm_atomic_get_new_bridge_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) { + /* Skip bridges that don't implement the atomic state hooks. */ + if (!bridge->funcs->atomic_duplicate_state) + continue; + + 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 4511c2e07bb9..ad8eae98d9e8 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_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c index 7cf3cf936547..33141d2cdad4 100644 --- a/drivers/gpu/drm/drm_atomic_state_helper.c +++ b/drivers/gpu/drm/drm_atomic_state_helper.c @@ -26,6 +26,7 @@

#include <drm/drm_atomic.h> #include <drm/drm_atomic_state_helper.h> +#include <drm/drm_bridge.h> #include <drm/drm_connector.h> #include <drm/drm_crtc.h> #include <drm/drm_device.h> @@ -551,3 +552,105 @@ void __drm_atomic_helper_private_obj_duplicate_state(struct drm_private_obj *obj memcpy(state, obj->state, sizeof(*state)); } EXPORT_SYMBOL(__drm_atomic_helper_private_obj_duplicate_state); + +/** + * __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_bridge_duplicate_state() - Duplicate a bridge state object + * @bridge: bridge object + * + * Allocates a new bridge state and initializes it with the current bridge + * state values. This helper is meant to be used as a bridge + * &drm_bridge_funcs.atomic_duplicate_state hook for bridges that don't + * subclass the bridge state. + */ +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); + +/** + * drm_atomic_helper_bridge_destroy_state() - Destroy a bridge state object + * @bridge: the bridge this state refers to + * @state: bridge state to destroy + * + * Destroys a bridge state previously created by + * &drm_atomic_helper_bridge_reset() or + * &drm_atomic_helper_bridge_duplicate_state(). This helper is meant to be + * used as a bridge &drm_bridge_funcs.atomic_destroy_state hook for bridges + * that don't subclass the bridge state. + */ +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 refers to + * @state: bridge state to initialize + * + * Initializes the bridge state to default values. This is meant to be called + * by the bridge &drm_bridge_funcs.atomic_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() - Allocate and initialize a bridge state + * to its default + * @bridge: the bridge this state refers to + * @state: bridge state to initialize + * + * Allocates the bridge state and initializes it to default values. This helper + * is meant to be used as a bridge &drm_bridge_funcs.atomic_reset hook for + * bridges that don't subclass the bridge state. + */ +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); diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index c2cf0c90fa26..6b08158ebb7e 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -25,6 +25,7 @@ #include <linux/module.h> #include <linux/mutex.h>

+#include <drm/drm_atomic_state_helper.h> #include <drm/drm_bridge.h> #include <drm/drm_encoder.h>

@@ -89,6 +90,31 @@ 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; + + state = bridge->funcs->atomic_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); + + bridge->funcs->atomic_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 * @@ -135,15 +161,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) { + struct drm_bridge_state *state; + + state = bridge->funcs->atomic_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 +201,9 @@ void drm_bridge_detach(struct drm_bridge *bridge) if (WARN_ON(!bridge->dev)) return;

+ if (bridge->funcs->atomic_reset) + drm_atomic_private_obj_fini(&bridge->base); + if (bridge->funcs->detach) bridge->funcs->detach(bridge);

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index 951dfb15c27b..82a888769b3d 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); @@ -992,4 +995,35 @@ drm_atomic_crtc_effectively_active(const struct drm_crtc_state *state) return state->active || state->self_refresh_active; }

+/** + * struct drm_bridge_state - Atomic bridge state object + */ +struct drm_bridge_state { + /** + * @base: inherit from &drm_private_state + */ + struct drm_private_state base; + + /** + * @bridge: the bridge this state refers to + */ + 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_state * +drm_atomic_get_bridge_state(struct drm_atomic_state *state, + struct drm_bridge *bridge); +struct drm_bridge_state * +drm_atomic_get_old_bridge_state(struct drm_atomic_state *state, + struct drm_bridge *bridge); +struct drm_bridge_state * +drm_atomic_get_new_bridge_state(struct drm_atomic_state *state, + struct drm_bridge *bridge); + #endif /* DRM_ATOMIC_H_ */ diff --git a/include/drm/drm_atomic_state_helper.h b/include/drm/drm_atomic_state_helper.h index 8171dea4cc22..3f8f1d627f7c 100644 --- a/include/drm/drm_atomic_state_helper.h +++ b/include/drm/drm_atomic_state_helper.h @@ -26,6 +26,8 @@

#include <linux/types.h>

+struct drm_bridge; +struct drm_bridge_state; struct drm_crtc; struct drm_crtc_state; struct drm_plane; @@ -80,3 +82,14 @@ void drm_atomic_helper_connector_destroy_state(struct drm_connector *connector, struct drm_connector_state *state); void __drm_atomic_helper_private_obj_duplicate_state(struct drm_private_obj *obj, struct drm_private_state *state); + +void __drm_atomic_helper_bridge_duplicate_state(struct drm_bridge *bridge, + struct drm_bridge_state *state); +struct drm_bridge_state * +drm_atomic_helper_bridge_duplicate_state(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_reset(struct drm_bridge *bridge, + struct drm_bridge_state *state); +struct drm_bridge_state * +drm_atomic_helper_bridge_reset(struct drm_bridge *bridge); diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 694e153a7531..8883dcfe63cb 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -25,6 +25,8 @@

#include <linux/list.h> #include <linux/ctype.h> + +#include <drm/drm_atomic.h> #include <drm/drm_encoder.h> #include <drm/drm_mode_object.h> #include <drm/drm_modes.h> @@ -338,6 +340,56 @@ 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 mandatory if the bridge implements any of + * the atomic hooks, and should be left unassigned otherwise. + * + * Note that the atomic_reset() semantics is not exactly matching the + * reset() semantics found on other components (connector, plane, ...). + * 1/ The reset operation happens when the bridge is attached, not when + * drm_mode_config_reset() is called + * 2/ It's meant to be used exclusively on bridges that have been + * converted to the ATOMIC API + * + * 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 +432,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 +458,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);

This is needed to pass a bridge state to all atomic hooks, if we don't do that, the core can't duplicate/create bridge states.

Signed-off-by: Boris Brezillon boris.brezillon@collabora.com Reviewed-by: Neil Armstrong narmstrong@baylibre.com --- Changes in v9: * Add Neil's R-b * Move earlier in the series

Changes in v7: * New patch --- drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index 6effe532f820..aaaec39ec2a7 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c @@ -1563,6 +1563,9 @@ static void analogix_dp_bridge_mode_set(struct drm_bridge *bridge, }

static const struct drm_bridge_funcs analogix_dp_bridge_funcs = { + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, + .atomic_reset = drm_atomic_helper_bridge_reset, .atomic_pre_enable = analogix_dp_bridge_atomic_pre_enable, .atomic_enable = analogix_dp_bridge_atomic_enable, .atomic_disable = analogix_dp_bridge_atomic_disable,

So that bridge drivers have a way to check/reject an atomic operation. The drm_atomic_bridge_chain_check() (which is just a wrapper around the ->atomic_check() hook) is called in place of drm_bridge_chain_mode_fixup() (when ->atomic_check() is not implemented, the core falls back on ->mode_fixup(), so the behavior should stay the same for existing bridge drivers).

Signed-off-by: Boris Brezillon boris.brezillon@collabora.com Reviewed-by: Neil Armstrong narmstrong@baylibre.com Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Neil Armstrong narmstrong@baylibre.com Reviewed by: Jernej Skrabec jernej.skrabec@siol.net Tested-by: Jonas Karlman jonas@kwiboo.se --- Changes in v7: * Fix a NULL pointer dereference --- drivers/gpu/drm/drm_atomic_helper.c | 12 +++--- drivers/gpu/drm/drm_bridge.c | 66 +++++++++++++++++++++++++++++ include/drm/drm_bridge.h | 29 ++++++++++++- 3 files changed, 100 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ad8eae98d9e8..afe14f72a824 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -437,12 +437,12 @@ mode_fixup(struct drm_atomic_state *state) funcs = encoder->helper_private;

bridge = drm_bridge_chain_get_first_bridge(encoder); - ret = drm_bridge_chain_mode_fixup(bridge, - &new_crtc_state->mode, - &new_crtc_state->adjusted_mode); - if (!ret) { - DRM_DEBUG_ATOMIC("Bridge fixup failed\n"); - return -EINVAL; + ret = drm_atomic_bridge_chain_check(bridge, + new_crtc_state, + new_conn_state); + if (ret) { + DRM_DEBUG_ATOMIC("Bridge atomic check failed\n"); + return ret; }

if (funcs && funcs->atomic_check) { diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index f6206e655da4..c8beb1385771 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -602,6 +602,72 @@ void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge, } EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);

+static int drm_atomic_bridge_check(struct drm_bridge *bridge, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state) +{ + if (bridge->funcs->atomic_check) { + struct drm_bridge_state *bridge_state; + int ret; + + bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state, + bridge); + if (WARN_ON(!bridge_state)) + return -EINVAL; + + ret = bridge->funcs->atomic_check(bridge, bridge_state, + crtc_state, conn_state); + if (ret) + return ret; + } else if (bridge->funcs->mode_fixup) { + if (!bridge->funcs->mode_fixup(bridge, &crtc_state->mode, + &crtc_state->adjusted_mode)) + return -EINVAL; + } + + return 0; +} + +/** + * drm_atomic_bridge_chain_check() - Do an atomic check on the bridge chain + * @bridge: bridge control structure + * @crtc_state: new CRTC state + * @conn_state: new connector state + * + * Calls &drm_bridge_funcs.atomic_check() (falls back on + * &drm_bridge_funcs.mode_fixup()) op for all the bridges in the encoder chain, + * starting from the last bridge to the first. These are called before calling + * &drm_encoder_helper_funcs.atomic_check() + * + * RETURNS: + * 0 on success, a negative error code on failure + */ +int drm_atomic_bridge_chain_check(struct drm_bridge *bridge, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state) +{ + struct drm_encoder *encoder; + struct drm_bridge *iter; + + if (!bridge) + return 0; + + encoder = bridge->encoder; + list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) { + int ret; + + ret = drm_atomic_bridge_check(iter, crtc_state, conn_state); + if (ret) + return ret; + + if (iter == bridge) + break; + } + + return 0; +} +EXPORT_SYMBOL(drm_atomic_bridge_chain_check); + #ifdef CONFIG_OF /** * of_drm_find_bridge - find the bridge corresponding to the device node in diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index df22f909141f..ff4df6eb2689 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -111,7 +111,9 @@ struct drm_bridge_funcs { * this function passes all other callbacks must succeed for this * configuration. * - * The @mode_fixup callback is optional. + * The mode_fixup callback is optional. &drm_bridge_funcs.mode_fixup() + * is not called when &drm_bridge_funcs.atomic_check() is implemented, + * so only one of them should be provided. * * NOTE: * @@ -368,6 +370,28 @@ struct drm_bridge_funcs { void (*atomic_destroy_state)(struct drm_bridge *bridge, struct drm_bridge_state *state);

+ /** + * @atomic_check: + * + * This method is responsible for checking bridge state correctness. + * It can also check the state of the surrounding components in chain + * to make sure the whole pipeline can work properly. + * + * &drm_bridge_funcs.atomic_check() hooks are called in reverse + * order (from the last to the first bridge). + * + * This method is optional. &drm_bridge_funcs.mode_fixup() is not + * called when &drm_bridge_funcs.atomic_check() is implemented, so only + * one of them should be provided. + * + * RETURNS: + * zero if the check passed, a negative error code otherwise. + */ + int (*atomic_check)(struct drm_bridge *bridge, + struct drm_bridge_state *bridge_state, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state); + /** * @atomic_reset: * @@ -542,6 +566,9 @@ void drm_bridge_chain_mode_set(struct drm_bridge *bridge, void drm_bridge_chain_pre_enable(struct drm_bridge *bridge); void drm_bridge_chain_enable(struct drm_bridge *bridge);

+int drm_atomic_bridge_chain_check(struct drm_bridge *bridge, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state); void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge, struct drm_atomic_state *state); void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,

Now that bridges can expose the bus format/flags they expect, we can use those instead of the relying on the display_info provided by the connector (which is only valid if the encoder is directly connected to bridge element driving the panel/display).

We also explicitly expose the bus formats supported by our encoder by filling encoder->output_bus_caps with proper info.

Signed-off-by: Boris Brezillon boris.brezillon@collabora.com --- Changes in v7: * Add an imx_pd_format_supported() helper (suggested by Philipp) * Simplify imx_pd_bridge_atomic_get_output_bus_fmts() (suggested by Philipp) * Simplify imx_pd_bridge_atomic_get_input_bus_fmts() * Explicitly set the duplicate/destro_state() and reset() hooks

Changes in v3 (all suggested by Philipp): * Adjust to match core changes * Propagate output format to input format * Pick a default value when output_fmt = _FIXED * Add missing BGR888 and GBR888 fmts to imx_pd_bus_fmts[]

Changes in v2: * Adjust things to match the new bus-format negotiation infra --- drivers/gpu/drm/imx/parallel-display.c | 176 +++++++++++++++++++++---- 1 file changed, 151 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/imx/parallel-display.c b/drivers/gpu/drm/imx/parallel-display.c index 3dca424059f7..031579aa7fc2 100644 --- a/drivers/gpu/drm/imx/parallel-display.c +++ b/drivers/gpu/drm/imx/parallel-display.c @@ -24,6 +24,7 @@ struct imx_parallel_display { struct drm_connector connector; struct drm_encoder encoder; + struct drm_bridge bridge; struct device *dev; void *edid; int edid_len; @@ -31,7 +32,7 @@ struct imx_parallel_display { u32 bus_flags; struct drm_display_mode mode; struct drm_panel *panel; - struct drm_bridge *bridge; + struct drm_bridge *next_bridge; };

static inline struct imx_parallel_display *con_to_imxpd(struct drm_connector *c) @@ -44,6 +45,11 @@ static inline struct imx_parallel_display *enc_to_imxpd(struct drm_encoder *e) return container_of(e, struct imx_parallel_display, encoder); }

+static inline struct imx_parallel_display *bridge_to_imxpd(struct drm_bridge *b) +{ + return container_of(b, struct imx_parallel_display, bridge); +} + static int imx_pd_connector_get_modes(struct drm_connector *connector) { struct imx_parallel_display *imxpd = con_to_imxpd(connector); @@ -89,37 +95,148 @@ static struct drm_encoder *imx_pd_connector_best_encoder( return &imxpd->encoder; }

-static void imx_pd_encoder_enable(struct drm_encoder *encoder) +static void imx_pd_bridge_enable(struct drm_bridge *bridge) { - struct imx_parallel_display *imxpd = enc_to_imxpd(encoder); + struct imx_parallel_display *imxpd = bridge_to_imxpd(bridge);

drm_panel_prepare(imxpd->panel); drm_panel_enable(imxpd->panel); }

-static void imx_pd_encoder_disable(struct drm_encoder *encoder) +static void imx_pd_bridge_disable(struct drm_bridge *bridge) { - struct imx_parallel_display *imxpd = enc_to_imxpd(encoder); + struct imx_parallel_display *imxpd = bridge_to_imxpd(bridge);

drm_panel_disable(imxpd->panel); drm_panel_unprepare(imxpd->panel); }

-static int imx_pd_encoder_atomic_check(struct drm_encoder *encoder, - struct drm_crtc_state *crtc_state, - struct drm_connector_state *conn_state) +static const u32 imx_pd_bus_fmts[] = { + MEDIA_BUS_FMT_RGB888_1X24, + MEDIA_BUS_FMT_BGR888_1X24, + MEDIA_BUS_FMT_GBR888_1X24, + MEDIA_BUS_FMT_RGB666_1X18, + MEDIA_BUS_FMT_RGB666_1X24_CPADHI, + MEDIA_BUS_FMT_RGB565_1X16, +}; + +static u32 * +imx_pd_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge, + struct drm_bridge_state *bridge_state, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state, + unsigned int *num_output_fmts) { - struct imx_crtc_state *imx_crtc_state = to_imx_crtc_state(crtc_state); struct drm_display_info *di = &conn_state->connector->display_info; - struct imx_parallel_display *imxpd = enc_to_imxpd(encoder); + struct imx_parallel_display *imxpd = bridge_to_imxpd(bridge); + u32 *output_fmts;

- if (!imxpd->bus_format && di->num_bus_formats) { - imx_crtc_state->bus_flags = di->bus_flags; - imx_crtc_state->bus_format = di->bus_formats[0]; - } else { - imx_crtc_state->bus_flags = imxpd->bus_flags; - imx_crtc_state->bus_format = imxpd->bus_format; + if (!imxpd->bus_format && !di->num_bus_formats) { + *num_output_fmts = ARRAY_SIZE(imx_pd_bus_fmts); + return kmemdup(imx_pd_bus_fmts, sizeof(imx_pd_bus_fmts), + GFP_KERNEL); + } + + *num_output_fmts = 1; + output_fmts = kcalloc(*num_output_fmts, sizeof(*output_fmts), + GFP_KERNEL); + if (!output_fmts) + return NULL; + + if (!imxpd->bus_format && di->num_bus_formats) + output_fmts[0] = di->bus_formats[0]; + else + output_fmts[0] = imxpd->bus_format; + + return output_fmts; +} + +static bool imx_pd_format_supported(u32 output_fmt) +{ + unsigned int i; + + for (i = 0; i < ARRAY_SIZE(imx_pd_bus_fmts); i++) { + if (imx_pd_bus_fmts[i] == output_fmt) + return true; } + + return false; +} + +static u32 * +imx_pd_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge, + struct drm_bridge_state *bridge_state, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state, + u32 output_fmt, + unsigned int *num_input_fmts) +{ + struct imx_parallel_display *imxpd = bridge_to_imxpd(bridge); + u32 *input_fmts; + + /* + * If the next bridge does not support bus format negotiation, let's + * use the static bus format definition (imxpd->bus_format) if it's + * specified, RGB888 when it's not. + */ + if (output_fmt == MEDIA_BUS_FMT_FIXED) + output_fmt = imxpd->bus_format ? : MEDIA_BUS_FMT_RGB888_1X24; + + /* Now make sure the requested output format is supported. */ + if ((imxpd->bus_format && imxpd->bus_format != output_fmt) || + !imx_pd_format_supported(output_fmt)) { + *num_input_fmts = 0; + return NULL; + } + + *num_input_fmts = 1; + input_fmts = kcalloc(*num_input_fmts, sizeof(*input_fmts), + GFP_KERNEL); + if (!input_fmts) + return NULL; + + input_fmts[0] = output_fmt; + return input_fmts; +} + +static int imx_pd_bridge_atomic_check(struct drm_bridge *bridge, + struct drm_bridge_state *bridge_state, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state) +{ + struct imx_crtc_state *imx_crtc_state = to_imx_crtc_state(crtc_state); + struct drm_display_info *di = &conn_state->connector->display_info; + struct imx_parallel_display *imxpd = bridge_to_imxpd(bridge); + struct drm_bridge_state *next_bridge_state = NULL; + struct drm_bridge *next_bridge; + u32 bus_flags, bus_fmt; + + next_bridge = drm_bridge_get_next_bridge(bridge); + if (next_bridge) + next_bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state, + next_bridge); + + if (next_bridge_state) + bus_flags = next_bridge_state->input_bus_cfg.flags; + else if (!imxpd->bus_format && di->num_bus_formats) + bus_flags = di->bus_flags; + else + bus_flags = imxpd->bus_flags; + + bus_fmt = bridge_state->input_bus_cfg.format; + if (!imx_pd_format_supported(bus_fmt)) + return -EINVAL; + + if (bus_flags & + ~(DRM_BUS_FLAG_DE_LOW | DRM_BUS_FLAG_DE_HIGH | + DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE | + DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)) + return -EINVAL; + + bridge_state->output_bus_cfg.flags = bus_flags; + bridge_state->input_bus_cfg.flags = bus_flags; + imx_crtc_state->bus_flags = bus_flags; + imx_crtc_state->bus_format = bridge_state->input_bus_cfg.format; imx_crtc_state->di_hsync_pin = 2; imx_crtc_state->di_vsync_pin = 3;

@@ -143,10 +260,15 @@ static const struct drm_encoder_funcs imx_pd_encoder_funcs = { .destroy = imx_drm_encoder_destroy, };

-static const struct drm_encoder_helper_funcs imx_pd_encoder_helper_funcs = { - .enable = imx_pd_encoder_enable, - .disable = imx_pd_encoder_disable, - .atomic_check = imx_pd_encoder_atomic_check, +static const struct drm_bridge_funcs imx_pd_bridge_funcs = { + .enable = imx_pd_bridge_enable, + .disable = imx_pd_bridge_disable, + .atomic_reset = drm_atomic_helper_bridge_reset, + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, + .atomic_check = imx_pd_bridge_atomic_check, + .atomic_get_input_bus_fmts = imx_pd_bridge_atomic_get_input_bus_fmts, + .atomic_get_output_bus_fmts = imx_pd_bridge_atomic_get_output_bus_fmts, };

static int imx_pd_register(struct drm_device *drm, @@ -166,11 +288,13 @@ static int imx_pd_register(struct drm_device *drm, */ imxpd->connector.dpms = DRM_MODE_DPMS_OFF;

- drm_encoder_helper_add(encoder, &imx_pd_encoder_helper_funcs); drm_encoder_init(drm, encoder, &imx_pd_encoder_funcs, DRM_MODE_ENCODER_NONE, NULL);

- if (!imxpd->bridge) { + imxpd->bridge.funcs = &imx_pd_bridge_funcs; + drm_bridge_attach(encoder, &imxpd->bridge, NULL); + + if (!imxpd->next_bridge) { drm_connector_helper_add(&imxpd->connector, &imx_pd_connector_helper_funcs); drm_connector_init(drm, &imxpd->connector, @@ -181,8 +305,9 @@ static int imx_pd_register(struct drm_device *drm, if (imxpd->panel) drm_panel_attach(imxpd->panel, &imxpd->connector);

- if (imxpd->bridge) { - ret = drm_bridge_attach(encoder, imxpd->bridge, NULL); + if (imxpd->next_bridge) { + ret = drm_bridge_attach(encoder, imxpd->next_bridge, + &imxpd->bridge); if (ret < 0) { dev_err(imxpd->dev, "failed to attach bridge: %d\n", ret); @@ -227,7 +352,8 @@ static int imx_pd_bind(struct device *dev, struct device *master, void *data) imxpd->bus_format = bus_format;

/* port@1 is the output port */ - ret = drm_of_find_panel_or_bridge(np, 1, 0, &imxpd->panel, &imxpd->bridge); + ret = drm_of_find_panel_or_bridge(np, 1, 0, &imxpd->panel, + &imxpd->next_bridge); if (ret && ret != -ENODEV) return ret;

Some DPI -> LVDS encoders might support several input bus width. Add a DT property to describe the bus width used on a specific design.

Signed-off-by: Boris Brezillon boris.brezillon@collabora.com --- Changes in v7: * Fix a use-after-release problem * Get rid of the data-mapping parsing * Use kmalloc instead of kcalloc.

Changes in v3: * Use bus-width for the rgb24/rgb18 distinction * Adjust code to match core changes * Get rid of of_get_data_mapping() * Don't implement ->atomic_check() (the core now takes care of bus flags propagation)

Changes in v2: * Make the bus-format negotiation logic more generic --- drivers/gpu/drm/bridge/lvds-codec.c | 64 ++++++++++++++++++++++++++--- 1 file changed, 58 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/lvds-codec.c b/drivers/gpu/drm/bridge/lvds-codec.c index 5f04cc11227e..f4fd8472c335 100644 --- a/drivers/gpu/drm/bridge/lvds-codec.c +++ b/drivers/gpu/drm/bridge/lvds-codec.c @@ -11,6 +11,7 @@ #include <linux/of_graph.h> #include <linux/platform_device.h>

+#include <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h> #include <drm/drm_panel.h>

@@ -19,6 +20,7 @@ struct lvds_codec { struct drm_bridge *panel_bridge; struct gpio_desc *powerdown_gpio; u32 connector_type; + u32 input_fmt; };

static int lvds_codec_attach(struct drm_bridge *bridge) @@ -48,18 +50,47 @@ static void lvds_codec_disable(struct drm_bridge *bridge) gpiod_set_value_cansleep(lvds_codec->powerdown_gpio, 1); }

+static u32 * +lvds_codec_atomic_get_input_bus_fmts(struct drm_bridge *bridge, + struct drm_bridge_state *bridge_state, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state, + u32 output_fmt, + unsigned int *num_input_fmts) +{ + struct lvds_codec *lvds_codec = container_of(bridge, + struct lvds_codec, bridge); + u32 *input_fmts; + + input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL); + if (!input_fmts) { + *num_input_fmts = 0; + return NULL; + } + + *num_input_fmts = 1; + input_fmts[0] = lvds_codec->input_fmt; + return input_fmts; +} + static struct drm_bridge_funcs funcs = { .attach = lvds_codec_attach, .enable = lvds_codec_enable, .disable = lvds_codec_disable, + .atomic_reset = drm_atomic_helper_bridge_reset, + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state, + .atomic_get_input_bus_fmts = lvds_codec_atomic_get_input_bus_fmts, };

static int lvds_codec_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; - struct device_node *panel_node; + struct device_node *np; struct drm_panel *panel; struct lvds_codec *lvds_codec; + u32 input_bus_width; + int err;

lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL); if (!lvds_codec) @@ -69,22 +100,43 @@ static int lvds_codec_probe(struct platform_device *pdev) lvds_codec->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", GPIOD_OUT_HIGH); if (IS_ERR(lvds_codec->powerdown_gpio)) { - int err = PTR_ERR(lvds_codec->powerdown_gpio); + err = PTR_ERR(lvds_codec->powerdown_gpio);

if (err != -EPROBE_DEFER) dev_err(dev, "powerdown GPIO failure: %d\n", err); return err; }

+ np = of_graph_get_port_by_id(dev->of_node, 0); + if (!np) { + dev_dbg(dev, "port 0 not found\n"); + return -ENXIO; + } + + err = of_property_read_u32(np, "bus-width", &input_bus_width); + of_node_put(np); + + if (err) { + lvds_codec->input_fmt = MEDIA_BUS_FMT_FIXED; + } else if (input_bus_width == 18) { + lvds_codec->input_fmt = MEDIA_BUS_FMT_RGB666_1X18; + } else if (input_bus_width == 24) { + lvds_codec->input_fmt = MEDIA_BUS_FMT_RGB888_1X24; + } else { + dev_dbg(dev, "unsupported bus-width value %u on port 0\n", + input_bus_width); + return -ENOTSUPP; + } + /* Locate the panel DT node. */ - panel_node = of_graph_get_remote_node(dev->of_node, 1, 0); - if (!panel_node) { + np = of_graph_get_remote_node(dev->of_node, 1, 0); + if (!np) { dev_dbg(dev, "panel DT node not found\n"); return -ENXIO; }

- panel = of_drm_find_panel(panel_node); - of_node_put(panel_node); + panel = of_drm_find_panel(np); + of_node_put(np); if (IS_ERR(panel)) { dev_dbg(dev, "panel not found, deferring probe\n"); return PTR_ERR(panel);

The lt089ac29000 panel is an LVDS panel, not a DPI one. Fix the definition to reflect this fact.

Signed-off-by: Boris Brezillon boris.brezillon@collabora.com Suggested-by: Laurent Pinchart laurent.pinchart@ideasonboard.com --- Changes in v7: * New patch --- drivers/gpu/drm/panel/panel-simple.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index a0dd84e11db7..6f906a439cc8 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -3108,7 +3108,7 @@ static const struct panel_desc toshiba_lt089ac29000 = { .width = 194, .height = 116, }, - .bus_format = MEDIA_BUS_FMT_RGB888_1X24, + .bus_format = MEDIA_BUS_FMT_RGB888_1X7X4_JEIDA, .bus_flags = DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE, .connector_type = DRM_MODE_CONNECTOR_LVDS, };