[PATCH v9 00/12] drm: Add support for bus-format negotiation

[PATCH v9 00/12] drm: Add support for bus-format negotiation

[Boris Brezillon]

Hello,

This patch series aims at adding support for runtime bus-format
negotiation between all elements of the
'encoder -> bridges -> connector/display' section of the pipeline.

In order to support that, we need drm bridges to fully take part in the
atomic state validation process, which requires adding a
drm_bridge_state and a new drm_bridge_funcs.atomic_check() hook.
Once those basic building blocks are in place, we can add new hooks to
allow bus format negotiation (those are called just before
->atomic_check()). The bus format selection is done at runtime by
testing all possible combinations across the whole bridge chain until
one is reported to work.

No fundamental changes in this v9. Just a few clarifications in the
bridge atomic hooks/helpers doc, and we also moved the
drm_atomic_private_obj_init() call back to drm_bridge_attach().

This patch series is also available here [1].

Thanks,

Boris

[1]https://github.com/bbrezillon/linux-0day/commits/drm-bridge-busfmt-v9

Boris Brezillon (12):
  drm/bridge: Add a drm_bridge_state object
  drm/rcar-du: Plug atomic state hooks to the default implementation
  drm/bridge: analogix: Plug atomic state hooks to the default
    implementation
  drm/bridge: Patch atomic hooks to take a drm_bridge_state
  drm/bridge: Add an ->atomic_check() hook
  drm/bridge: Add the necessary bits to support bus format negotiation
  drm/imx: pd: Use bus format/flags provided by the bridge when
    available
  drm/bridge: lvds-codec: Implement basic bus format negotiation
  dt-bindings: display: bridge: lvds-codec: Add new bus-width prop
  drm/bridge: panel: Propage bus format/flags
  drm/panel: simple: Fix the lt089ac29000 bus_format
  ARM: dts: imx: imx51-zii-rdu1: Fix the display pipeline definition

 .../bindings/display/bridge/lvds-codec.yaml   |   8 +
 arch/arm/boot/dts/imx51-zii-rdu1.dts          |  24 +-
 .../drm/bridge/analogix/analogix_dp_core.c    |  44 +-
 drivers/gpu/drm/bridge/lvds-codec.c           |  64 ++-
 drivers/gpu/drm/bridge/panel.c                |   4 +
 drivers/gpu/drm/drm_atomic.c                  | 116 +++++
 drivers/gpu/drm/drm_atomic_helper.c           |  73 ++-
 drivers/gpu/drm/drm_atomic_state_helper.c     | 103 +++++
 drivers/gpu/drm/drm_bridge.c                  | 437 +++++++++++++++++-
 drivers/gpu/drm/imx/parallel-display.c        | 176 ++++++-
 drivers/gpu/drm/panel/panel-simple.c          |   2 +-
 drivers/gpu/drm/rcar-du/rcar_lvds.c           |  11 +-
 include/drm/drm_atomic.h                      |  76 +++
 include/drm/drm_atomic_helper.h               |   8 +
 include/drm/drm_atomic_state_helper.h         |  13 +
 include/drm/drm_bridge.h                      | 179 ++++++-
 16 files changed, 1257 insertions(+), 81 deletions(-)

-- 
2.24.1

[PATCH v9 01/12] drm/bridge: Add a drm_bridge_state object

[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 <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);
-- 
2.24.1

Re: [PATCH v9 01/12] drm/bridge: Add a drm_bridge_state object

[Daniel Vetter]

On Mon, Jan 27, 2020 at 12:00:32PM +0100, 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 <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

^^ we generally keep the changelog when committing in drm, since it
contains useful stuff. If you don't want to do that, then at least update
the commit message to make sure all the design discussion is reflected in
there somewhere. Which atm it definitely isn't.
-Daniel

> ---
>  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);
> -- 
> 2.24.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v9 01/12] drm/bridge: Add a drm_bridge_state object

[Boris Brezillon]

On Mon, 27 Jan 2020 18:26:52 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Mon, Jan 27, 2020 at 12:00:32PM +0100, 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 <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  
> 
> ^^ we generally keep the changelog when committing in drm, since it
> contains useful stuff.

Alright (I tend to forget that drm does things differently). I find it a
bit redundant given the final commit also contains a patchwork link
which contains all revisions + the discussion that happened on each
version.

> If you don't want to do that, then at least update
> the commit message to make sure all the design discussion is reflected in
> there somewhere. Which atm it definitely isn't.

I'll update the commit messages to add those changelogs.

[PATCH v9 02/12] drm/rcar-du: Plug atomic state hooks to the default implementation

[Boris Brezillon]

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/rcar-du/rcar_lvds.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 8ffa4fbbdeb3..3a254f1b29fe 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -682,6 +682,9 @@ static void rcar_lvds_detach(struct drm_bridge *bridge)
 static const struct drm_bridge_funcs rcar_lvds_bridge_ops = {
 	.attach = rcar_lvds_attach,
 	.detach = rcar_lvds_detach,
+	.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_enable = rcar_lvds_atomic_enable,
 	.atomic_disable = rcar_lvds_atomic_disable,
 	.mode_fixup = rcar_lvds_mode_fixup,
-- 
2.24.1

[PATCH v9 03/12] drm/bridge: analogix: Plug atomic state hooks to the default implementation

[Boris Brezillon]

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,
-- 
2.24.1

[PATCH v9 04/12] drm/bridge: Patch atomic hooks to take a drm_bridge_state

[Boris Brezillon]

This way the drm_bridge_funcs interface is consistent with the rest of
the subsystem.

The only driver implementing those hooks (analogix DP) is patched too.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
[narmstrong: renamed state as old_bridge_state in rcar_lvds_atomic_disable]
Reviewed by: Jernej Skrabec <jernej.skrabec@siol.net>
Tested-by: Jonas Karlman <jonas@kwiboo.se>
---
Changes in v7:
* None
---
 .../drm/bridge/analogix/analogix_dp_core.c    | 41 +++++++------
 drivers/gpu/drm/drm_bridge.c                  | 61 +++++++++++++++----
 drivers/gpu/drm/rcar-du/rcar_lvds.c           |  8 ++-
 include/drm/drm_bridge.h                      |  8 +--
 4 files changed, 82 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index aaaec39ec2a7..dfb59a5fefea 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1289,19 +1289,21 @@ struct drm_crtc *analogix_dp_get_new_crtc(struct analogix_dp_device *dp,
 	return conn_state->crtc;
 }
 
-static void analogix_dp_bridge_atomic_pre_enable(struct drm_bridge *bridge,
-						 struct drm_atomic_state *state)
+static void
+analogix_dp_bridge_atomic_pre_enable(struct drm_bridge *bridge,
+				     struct drm_bridge_state *old_bridge_state)
 {
+	struct drm_atomic_state *old_state = old_bridge_state->base.state;
 	struct analogix_dp_device *dp = bridge->driver_private;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state;
 	int ret;
 
-	crtc = analogix_dp_get_new_crtc(dp, state);
+	crtc = analogix_dp_get_new_crtc(dp, old_state);
 	if (!crtc)
 		return;
 
-	old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
+	old_crtc_state = drm_atomic_get_old_crtc_state(old_state, crtc);
 	/* Don't touch the panel if we're coming back from PSR */
 	if (old_crtc_state && old_crtc_state->self_refresh_active)
 		return;
@@ -1366,20 +1368,22 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp)
 	return ret;
 }
 
-static void analogix_dp_bridge_atomic_enable(struct drm_bridge *bridge,
-					     struct drm_atomic_state *state)
+static void
+analogix_dp_bridge_atomic_enable(struct drm_bridge *bridge,
+				 struct drm_bridge_state *old_bridge_state)
 {
+	struct drm_atomic_state *old_state = old_bridge_state->base.state;
 	struct analogix_dp_device *dp = bridge->driver_private;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state;
 	int timeout_loop = 0;
 	int ret;
 
-	crtc = analogix_dp_get_new_crtc(dp, state);
+	crtc = analogix_dp_get_new_crtc(dp, old_state);
 	if (!crtc)
 		return;
 
-	old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
+	old_crtc_state = drm_atomic_get_old_crtc_state(old_state, crtc);
 	/* Not a full enable, just disable PSR and continue */
 	if (old_crtc_state && old_crtc_state->self_refresh_active) {
 		ret = analogix_dp_disable_psr(dp);
@@ -1440,18 +1444,20 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
 	dp->dpms_mode = DRM_MODE_DPMS_OFF;
 }
 
-static void analogix_dp_bridge_atomic_disable(struct drm_bridge *bridge,
-					      struct drm_atomic_state *state)
+static void
+analogix_dp_bridge_atomic_disable(struct drm_bridge *bridge,
+				  struct drm_bridge_state *old_bridge_state)
 {
+	struct drm_atomic_state *old_state = old_bridge_state->base.state;
 	struct analogix_dp_device *dp = bridge->driver_private;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *new_crtc_state = NULL;
 
-	crtc = analogix_dp_get_new_crtc(dp, state);
+	crtc = analogix_dp_get_new_crtc(dp, old_state);
 	if (!crtc)
 		goto out;
 
-	new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+	new_crtc_state = drm_atomic_get_new_crtc_state(old_state, crtc);
 	if (!new_crtc_state)
 		goto out;
 
@@ -1463,20 +1469,21 @@ static void analogix_dp_bridge_atomic_disable(struct drm_bridge *bridge,
 	analogix_dp_bridge_disable(bridge);
 }
 
-static
-void analogix_dp_bridge_atomic_post_disable(struct drm_bridge *bridge,
-					    struct drm_atomic_state *state)
+static void
+analogix_dp_bridge_atomic_post_disable(struct drm_bridge *bridge,
+				struct drm_bridge_state *old_bridge_state)
 {
+	struct drm_atomic_state *old_state = old_bridge_state->base.state;
 	struct analogix_dp_device *dp = bridge->driver_private;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *new_crtc_state;
 	int ret;
 
-	crtc = analogix_dp_get_new_crtc(dp, state);
+	crtc = analogix_dp_get_new_crtc(dp, old_state);
 	if (!crtc)
 		return;
 
-	new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+	new_crtc_state = drm_atomic_get_new_crtc_state(old_state, crtc);
 	if (!new_crtc_state || !new_crtc_state->self_refresh_active)
 		return;
 
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 6b08158ebb7e..f6206e655da4 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -458,10 +458,19 @@ void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,
 
 	encoder = bridge->encoder;
 	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
-		if (iter->funcs->atomic_disable)
-			iter->funcs->atomic_disable(iter, old_state);
-		else if (iter->funcs->disable)
+		if (iter->funcs->atomic_disable) {
+			struct drm_bridge_state *old_bridge_state;
+
+			old_bridge_state =
+				drm_atomic_get_old_bridge_state(old_state,
+								iter);
+			if (WARN_ON(!old_bridge_state))
+				return;
+
+			iter->funcs->atomic_disable(iter, old_bridge_state);
+		} else if (iter->funcs->disable) {
 			iter->funcs->disable(iter);
+		}
 
 		if (iter == bridge)
 			break;
@@ -492,10 +501,20 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
 
 	encoder = bridge->encoder;
 	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
-		if (bridge->funcs->atomic_post_disable)
-			bridge->funcs->atomic_post_disable(bridge, old_state);
-		else if (bridge->funcs->post_disable)
+		if (bridge->funcs->atomic_post_disable) {
+			struct drm_bridge_state *old_bridge_state;
+
+			old_bridge_state =
+				drm_atomic_get_old_bridge_state(old_state,
+								bridge);
+			if (WARN_ON(!old_bridge_state))
+				return;
+
+			bridge->funcs->atomic_post_disable(bridge,
+							   old_bridge_state);
+		} else if (bridge->funcs->post_disable) {
 			bridge->funcs->post_disable(bridge);
+		}
 	}
 }
 EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
@@ -524,10 +543,19 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
 
 	encoder = bridge->encoder;
 	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
-		if (iter->funcs->atomic_pre_enable)
-			iter->funcs->atomic_pre_enable(iter, old_state);
-		else if (iter->funcs->pre_enable)
+		if (iter->funcs->atomic_pre_enable) {
+			struct drm_bridge_state *old_bridge_state;
+
+			old_bridge_state =
+				drm_atomic_get_old_bridge_state(old_state,
+								iter);
+			if (WARN_ON(!old_bridge_state))
+				return;
+
+			iter->funcs->atomic_pre_enable(iter, old_bridge_state);
+		} else if (iter->funcs->pre_enable) {
 			iter->funcs->pre_enable(iter);
+		}
 
 		if (iter == bridge)
 			break;
@@ -557,10 +585,19 @@ void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge,
 
 	encoder = bridge->encoder;
 	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
-		if (bridge->funcs->atomic_enable)
-			bridge->funcs->atomic_enable(bridge, old_state);
-		else if (bridge->funcs->enable)
+		if (bridge->funcs->atomic_enable) {
+			struct drm_bridge_state *old_bridge_state;
+
+			old_bridge_state =
+				drm_atomic_get_old_bridge_state(old_state,
+								bridge);
+			if (WARN_ON(!old_bridge_state))
+				return;
+
+			bridge->funcs->atomic_enable(bridge, old_bridge_state);
+		} else if (bridge->funcs->enable) {
 			bridge->funcs->enable(bridge);
+		}
 	}
 }
 EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);
diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 3a254f1b29fe..06432c881e07 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -590,8 +590,9 @@ static void __rcar_lvds_atomic_enable(struct drm_bridge *bridge,
 }
 
 static void rcar_lvds_atomic_enable(struct drm_bridge *bridge,
-				    struct drm_atomic_state *state)
+				    struct drm_bridge_state *old_bridge_state)
 {
+	struct drm_atomic_state *state = old_bridge_state->base.state;
 	struct drm_connector *connector;
 	struct drm_crtc *crtc;
 
@@ -603,7 +604,7 @@ static void rcar_lvds_atomic_enable(struct drm_bridge *bridge,
 }
 
 static void rcar_lvds_atomic_disable(struct drm_bridge *bridge,
-				     struct drm_atomic_state *state)
+				     struct drm_bridge_state *old_bridge_state)
 {
 	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
 
@@ -618,7 +619,8 @@ static void rcar_lvds_atomic_disable(struct drm_bridge *bridge,
 
 	/* Disable the companion LVDS encoder in dual-link mode. */
 	if (lvds->link_type != RCAR_LVDS_SINGLE_LINK && lvds->companion)
-		lvds->companion->funcs->atomic_disable(lvds->companion, state);
+		lvds->companion->funcs->atomic_disable(lvds->companion,
+						       old_bridge_state);
 
 	clk_disable_unprepare(lvds->clocks.mod);
 }
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 8883dcfe63cb..df22f909141f 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -265,7 +265,7 @@ struct drm_bridge_funcs {
 	 * The @atomic_pre_enable callback is optional.
 	 */
 	void (*atomic_pre_enable)(struct drm_bridge *bridge,
-				  struct drm_atomic_state *old_state);
+				  struct drm_bridge_state *old_bridge_state);
 
 	/**
 	 * @atomic_enable:
@@ -290,7 +290,7 @@ struct drm_bridge_funcs {
 	 * The @atomic_enable callback is optional.
 	 */
 	void (*atomic_enable)(struct drm_bridge *bridge,
-			      struct drm_atomic_state *old_state);
+			      struct drm_bridge_state *old_bridge_state);
 	/**
 	 * @atomic_disable:
 	 *
@@ -313,7 +313,7 @@ struct drm_bridge_funcs {
 	 * The @atomic_disable callback is optional.
 	 */
 	void (*atomic_disable)(struct drm_bridge *bridge,
-			       struct drm_atomic_state *old_state);
+			       struct drm_bridge_state *old_bridge_state);
 
 	/**
 	 * @atomic_post_disable:
@@ -339,7 +339,7 @@ struct drm_bridge_funcs {
 	 * The @atomic_post_disable callback is optional.
 	 */
 	void (*atomic_post_disable)(struct drm_bridge *bridge,
-				    struct drm_atomic_state *old_state);
+				    struct drm_bridge_state *old_bridge_state);
 
 	/**
 	 * @atomic_duplicate_state:
-- 
2.24.1

[PATCH v9 05/12] drm/bridge: Add an ->atomic_check() hook

[Boris Brezillon]

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,
-- 
2.24.1

[PATCH v9 06/12] drm/bridge: Add the necessary bits to support bus format negotiation

[Boris Brezillon]

drm_bridge_state is extended to describe the input and output bus
configurations. These bus configurations are exposed through the
drm_bus_cfg struct which encodes the configuration of a physical
bus between two components in an output pipeline, usually between
two bridges, an encoder and a bridge, or a bridge and a connector.

The bus configuration is stored in drm_bridge_state separately for
the input and output buses, as seen from the point of view of each
bridge. The bus configuration of a bridge output is usually identical
to the configuration of the next bridge's input, but may differ if
the signals are modified between the two bridges, for instance by an
inverter on the board. The input and output configurations of a
bridge may differ if the bridge modifies the signals internally,
for instance by performing format conversion, or*modifying signals
polarities.

Bus format negotiation is automated by the core, drivers just have
to implement the ->atomic_get_{output,input}_bus_fmts() hooks if they
want to take part to this negotiation. Negotiation happens in reverse
order, starting from the last element of the chain (the one directly
connected to the display) up to the first element of the chain (the one
connected to the encoder).
During this negotiation all supported formats are tested until we find
one that works, meaning that the formats array should be in decreasing
preference order (assuming the driver has a preference order).

Note that the bus format negotiation works even if some elements in the
chain don't implement the ->atomic_get_{output,input}_bus_fmts() hooks.
In that case, the core advertises only MEDIA_BUS_FMT_FIXED and lets
the previous bridge element decide what to do (most of the time, bridge
drivers will pick a default bus format or extract this piece of
information from somewhere else, like a FW property).

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
[narmstrong: fixed doc in include/drm/drm_bridge.h:69 fmt->format]
Reviewed by: Jernej Skrabec <jernej.skrabec@siol.net>
Tested-by: Jonas Karlman <jonas@kwiboo.se>
---
Changes in v8:
* Fix a test in drm_atomic_bridge_chain_select_bus_fmts() (Reported by
  Jonas)

Changes in v7:
* Adapt the code to deal with the fact that not all bridges in the
  chain have a bridge state
---
 drivers/gpu/drm/drm_atomic_helper.c |  41 +++++
 drivers/gpu/drm/drm_bridge.c        | 253 +++++++++++++++++++++++++++-
 include/drm/drm_atomic.h            |  42 +++++
 include/drm/drm_atomic_helper.h     |   8 +
 include/drm/drm_bridge.h            |  82 +++++++++
 5 files changed, 425 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index afe14f72a824..ea1926b5bb80 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3528,3 +3528,44 @@ int drm_atomic_helper_legacy_gamma_set(struct drm_crtc *crtc,
 	return ret;
 }
 EXPORT_SYMBOL(drm_atomic_helper_legacy_gamma_set);
+
+/**
+ * drm_atomic_helper_bridge_propagate_bus_fmt() - Propagate output format to
+ *						  the input end of a bridge
+ * @bridge: bridge control structure
+ * @bridge_state: new bridge state
+ * @crtc_state: new CRTC state
+ * @conn_state: new connector state
+ * @output_fmt: tested output bus format
+ * @num_input_fmts: will contain the size of the returned array
+ *
+ * This helper is a pluggable implementation of the
+ * &drm_bridge_funcs.atomic_get_input_bus_fmts operation for bridges that don't
+ * modify the bus configuration between their input and their output. It
+ * returns an array of input formats with a single element set to @output_fmt.
+ *
+ * RETURNS:
+ * a valid format array of size @num_input_fmts, or NULL if the allocation
+ * failed
+ */
+u32 *
+drm_atomic_helper_bridge_propagate_bus_fmt(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)
+{
+	u32 *input_fmts;
+
+	input_fmts = kzalloc(sizeof(*input_fmts), GFP_KERNEL);
+	if (!input_fmts) {
+		*num_input_fmts = 0;
+		return NULL;
+	}
+
+	*num_input_fmts = 1;
+	input_fmts[0] = output_fmt;
+	return input_fmts;
+}
+EXPORT_SYMBOL(drm_atomic_helper_bridge_propagate_bus_fmt);
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index c8beb1385771..68ab933ee430 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -628,13 +628,247 @@ static int drm_atomic_bridge_check(struct drm_bridge *bridge,
 	return 0;
 }
 
+static int select_bus_fmt_recursive(struct drm_bridge *first_bridge,
+				    struct drm_bridge *cur_bridge,
+				    struct drm_crtc_state *crtc_state,
+				    struct drm_connector_state *conn_state,
+				    u32 out_bus_fmt)
+{
+	struct drm_bridge_state *cur_state;
+	unsigned int num_in_bus_fmts, i;
+	struct drm_bridge *prev_bridge;
+	u32 *in_bus_fmts;
+	int ret;
+
+	prev_bridge = drm_bridge_get_prev_bridge(cur_bridge);
+	cur_state = drm_atomic_get_new_bridge_state(crtc_state->state,
+						    cur_bridge);
+
+	/*
+	 * If bus format negotiation is not supported by this bridge, let's
+	 * pass MEDIA_BUS_FMT_FIXED to the previous bridge in the chain and
+	 * hope that it can handle this situation gracefully (by providing
+	 * appropriate default values).
+	 */
+	if (!cur_bridge->funcs->atomic_get_input_bus_fmts) {
+		if (cur_bridge != first_bridge) {
+			ret = select_bus_fmt_recursive(first_bridge,
+						       prev_bridge, crtc_state,
+						       conn_state,
+						       MEDIA_BUS_FMT_FIXED);
+			if (ret)
+				return ret;
+		}
+
+		/*
+		 * Driver does not implement the atomic state hooks, but that's
+		 * fine, as long as it does not access the bridge state.
+		 */
+		if (cur_state) {
+			cur_state->input_bus_cfg.format = MEDIA_BUS_FMT_FIXED;
+			cur_state->output_bus_cfg.format = out_bus_fmt;
+		}
+
+		return 0;
+	}
+
+	/*
+	 * If the driver implements ->atomic_get_input_bus_fmts() it
+	 * should also implement the atomic state hooks.
+	 */
+	if (WARN_ON(!cur_state))
+		return -EINVAL;
+
+	in_bus_fmts = cur_bridge->funcs->atomic_get_input_bus_fmts(cur_bridge,
+							cur_state,
+							crtc_state,
+							conn_state,
+							out_bus_fmt,
+							&num_in_bus_fmts);
+	if (!num_in_bus_fmts)
+		return -ENOTSUPP;
+	else if (!in_bus_fmts)
+		return -ENOMEM;
+
+	if (first_bridge == cur_bridge) {
+		cur_state->input_bus_cfg.format = in_bus_fmts[0];
+		cur_state->output_bus_cfg.format = out_bus_fmt;
+		kfree(in_bus_fmts);
+		return 0;
+	}
+
+	for (i = 0; i < num_in_bus_fmts; i++) {
+		ret = select_bus_fmt_recursive(first_bridge, prev_bridge,
+					       crtc_state, conn_state,
+					       in_bus_fmts[i]);
+		if (ret != -ENOTSUPP)
+			break;
+	}
+
+	if (!ret) {
+		cur_state->input_bus_cfg.format = in_bus_fmts[i];
+		cur_state->output_bus_cfg.format = out_bus_fmt;
+	}
+
+	kfree(in_bus_fmts);
+	return ret;
+}
+
+/*
+ * This function is called by &drm_atomic_bridge_chain_check() just before
+ * calling &drm_bridge_funcs.atomic_check() on all elements of the chain.
+ * It performs bus format negotiation between bridge elements. The negotiation
+ * happens in reverse order, starting from the last element in the chain up to
+ * @bridge.
+ *
+ * Negotiation starts by retrieving supported output bus formats on the last
+ * bridge element and testing them one by one. The test is recursive, meaning
+ * that for each tested output format, the whole chain will be walked backward,
+ * and each element will have to choose an input bus format that can be
+ * transcoded to the requested output format. When a bridge element does not
+ * support transcoding into a specific output format -ENOTSUPP is returned and
+ * the next bridge element will have to try a different format. If none of the
+ * combinations worked, -ENOTSUPP is returned and the atomic modeset will fail.
+ *
+ * This implementation is relying on
+ * &drm_bridge_funcs.atomic_get_output_bus_fmts() and
+ * &drm_bridge_funcs.atomic_get_input_bus_fmts() to gather supported
+ * input/output formats.
+ *
+ * When &drm_bridge_funcs.atomic_get_output_bus_fmts() is not implemented by
+ * the last element of the chain, &drm_atomic_bridge_chain_select_bus_fmts()
+ * tries a single format: &drm_connector.display_info.bus_formats[0] if
+ * available, MEDIA_BUS_FMT_FIXED otherwise.
+ *
+ * When &drm_bridge_funcs.atomic_get_input_bus_fmts() is not implemented,
+ * &drm_atomic_bridge_chain_select_bus_fmts() skips the negotiation on the
+ * bridge element that lacks this hook and asks the previous element in the
+ * chain to try MEDIA_BUS_FMT_FIXED. It's up to bridge drivers to decide what
+ * to do in that case (fail if they want to enforce bus format negotiation, or
+ * provide a reasonable default if they need to support pipelines where not
+ * all elements support bus format negotiation).
+ */
+static int
+drm_atomic_bridge_chain_select_bus_fmts(struct drm_bridge *bridge,
+					struct drm_crtc_state *crtc_state,
+					struct drm_connector_state *conn_state)
+{
+	struct drm_connector *conn = conn_state->connector;
+	struct drm_encoder *encoder = bridge->encoder;
+	struct drm_bridge_state *last_bridge_state;
+	unsigned int i, num_out_bus_fmts;
+	struct drm_bridge *last_bridge;
+	u32 *out_bus_fmts;
+	int ret = 0;
+
+	last_bridge = list_last_entry(&encoder->bridge_chain,
+				      struct drm_bridge, chain_node);
+	last_bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state,
+							    last_bridge);
+
+	if (last_bridge->funcs->atomic_get_output_bus_fmts) {
+		const struct drm_bridge_funcs *funcs = last_bridge->funcs;
+
+		/*
+		 * If the driver implements ->atomic_get_output_bus_fmts() it
+		 * should also implement the atomic state hooks.
+		 */
+		if (WARN_ON(!last_bridge_state))
+			return -EINVAL;
+
+		out_bus_fmts = funcs->atomic_get_output_bus_fmts(last_bridge,
+							last_bridge_state,
+							crtc_state,
+							conn_state,
+							&num_out_bus_fmts);
+		if (!num_out_bus_fmts)
+			return -ENOTSUPP;
+		else if (!out_bus_fmts)
+			return -ENOMEM;
+	} else {
+		num_out_bus_fmts = 1;
+		out_bus_fmts = kmalloc(sizeof(*out_bus_fmts), GFP_KERNEL);
+		if (!out_bus_fmts)
+			return -ENOMEM;
+
+		if (conn->display_info.num_bus_formats &&
+		    conn->display_info.bus_formats)
+			out_bus_fmts[0] = conn->display_info.bus_formats[0];
+		else
+			out_bus_fmts[0] = MEDIA_BUS_FMT_FIXED;
+	}
+
+	for (i = 0; i < num_out_bus_fmts; i++) {
+		ret = select_bus_fmt_recursive(bridge, last_bridge, crtc_state,
+					       conn_state, out_bus_fmts[i]);
+		if (ret != -ENOTSUPP)
+			break;
+	}
+
+	kfree(out_bus_fmts);
+
+	return ret;
+}
+
+static void
+drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge,
+				      struct drm_connector *conn,
+				      struct drm_atomic_state *state)
+{
+	struct drm_bridge_state *bridge_state, *next_bridge_state;
+	struct drm_bridge *next_bridge;
+	u32 output_flags = 0;
+
+	bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
+
+	/* No bridge state attached to this bridge => nothing to propagate. */
+	if (!bridge_state)
+		return;
+
+	next_bridge = drm_bridge_get_next_bridge(bridge);
+
+	/*
+	 * Let's try to apply the most common case here, that is, propagate
+	 * display_info flags for the last bridge, and propagate the input
+	 * flags of the next bridge element to the output end of the current
+	 * bridge when the bridge is not the last one.
+	 * There are exceptions to this rule, like when signal inversion is
+	 * happening at the board level, but that's something drivers can deal
+	 * with from their &drm_bridge_funcs.atomic_check() implementation by
+	 * simply overriding the flags value we've set here.
+	 */
+	if (!next_bridge) {
+		output_flags = conn->display_info.bus_flags;
+	} else {
+		next_bridge_state = drm_atomic_get_new_bridge_state(state,
+								next_bridge);
+		/*
+		 * No bridge state attached to the next bridge, just leave the
+		 * flags to 0.
+		 */
+		if (next_bridge_state)
+			output_flags = next_bridge_state->input_bus_cfg.flags;
+	}
+
+	bridge_state->output_bus_cfg.flags = output_flags;
+
+	/*
+	 * Propage the output flags to the input end of the bridge. Again, it's
+	 * not necessarily what all bridges want, but that's what most of them
+	 * do, and by doing that by default we avoid forcing drivers to
+	 * duplicate the "dummy propagation" logic.
+	 */
+	bridge_state->input_bus_cfg.flags = output_flags;
+}
+
 /**
  * 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
+ * First trigger a bus format negotiation before calling
+ * &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()
@@ -646,16 +880,33 @@ int drm_atomic_bridge_chain_check(struct drm_bridge *bridge,
 				  struct drm_crtc_state *crtc_state,
 				  struct drm_connector_state *conn_state)
 {
+	struct drm_connector *conn = conn_state->connector;
 	struct drm_encoder *encoder;
 	struct drm_bridge *iter;
+	int ret;
 
 	if (!bridge)
 		return 0;
 
+	ret = drm_atomic_bridge_chain_select_bus_fmts(bridge, crtc_state,
+						      conn_state);
+	if (ret)
+		return ret;
+
 	encoder = bridge->encoder;
 	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
 		int ret;
 
+		/*
+		 * Bus flags are propagated by default. If a bridge needs to
+		 * tweak the input bus flags for any reason, it should happen
+		 * in its &drm_bridge_funcs.atomic_check() implementation such
+		 * that preceding bridges in the chain can propagate the new
+		 * bus flags.
+		 */
+		drm_atomic_bridge_propagate_bus_flags(iter, conn,
+						      crtc_state->state);
+
 		ret = drm_atomic_bridge_check(iter, crtc_state, conn_state);
 		if (ret)
 			return ret;
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 82a888769b3d..52d65a055491 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -995,6 +995,38 @@ drm_atomic_crtc_effectively_active(const struct drm_crtc_state *state)
 	return state->active || state->self_refresh_active;
 }
 
+/**
+ * struct drm_bus_cfg - bus configuration
+ *
+ * This structure stores the configuration of a physical bus between two
+ * components in an output pipeline, usually between two bridges, an encoder
+ * and a bridge, or a bridge and a connector.
+ *
+ * The bus configuration is stored in &drm_bridge_state separately for the
+ * input and output buses, as seen from the point of view of each bridge. The
+ * bus configuration of a bridge output is usually identical to the
+ * configuration of the next bridge's input, but may differ if the signals are
+ * modified between the two bridges, for instance by an inverter on the board.
+ * The input and output configurations of a bridge may differ if the bridge
+ * modifies the signals internally, for instance by performing format
+ * conversion, or modifying signals polarities.
+ */
+struct drm_bus_cfg {
+	/**
+	 * @format: format used on this bus (one of the MEDIA_BUS_FMT_* format)
+	 *
+	 * This field should not be directly modified by drivers
+	 * (&drm_atomic_bridge_chain_select_bus_fmts() takes care of the bus
+	 * format negotiation).
+	 */
+	u32 format;
+
+	/**
+	 * @flags: DRM_BUS_* flags used on this bus
+	 */
+	u32 flags;
+};
+
 /**
  * struct drm_bridge_state - Atomic bridge state object
  */
@@ -1008,6 +1040,16 @@ struct drm_bridge_state {
 	 * @bridge: the bridge this state refers to
 	 */
 	struct drm_bridge *bridge;
+
+	/**
+	 * @input_bus_cfg: input bus configuration
+	 */
+	struct drm_bus_cfg input_bus_cfg;
+
+	/**
+	 * @output_bus_cfg: input bus configuration
+	 */
+	struct drm_bus_cfg output_bus_cfg;
 };
 
 static inline struct drm_bridge_state *
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index 9db3cac48f4f..b268180c97eb 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -224,4 +224,12 @@ drm_atomic_plane_disabling(struct drm_plane_state *old_plane_state,
 	return old_plane_state->crtc && !new_plane_state->crtc;
 }
 
+u32 *
+drm_atomic_helper_bridge_propagate_bus_fmt(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);
+
 #endif /* DRM_ATOMIC_HELPER_H_ */
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index ff4df6eb2689..45626ecf20f8 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -370,6 +370,72 @@ struct drm_bridge_funcs {
 	void (*atomic_destroy_state)(struct drm_bridge *bridge,
 				     struct drm_bridge_state *state);
 
+	/**
+	 * @atomic_get_output_bus_fmts:
+	 *
+	 * Return the supported bus formats on the output end of a bridge.
+	 * The returned array must be allocated with kmalloc() and will be
+	 * freed by the caller. If the allocation fails, NULL should be
+	 * returned. num_output_fmts must be set to the returned array size.
+	 * Formats listed in the returned array should be listed in decreasing
+	 * preference order (the core will try all formats until it finds one
+	 * that works).
+	 *
+	 * This method is only called on the last element of the bridge chain
+	 * as part of the bus format negotiation process that happens in
+	 * &drm_atomic_bridge_chain_select_bus_fmts().
+	 * This method is optional. When not implemented, the core will
+	 * fall back to &drm_connector.display_info.bus_formats[0] if
+	 * &drm_connector.display_info.num_bus_formats > 0,
+	 * or to MEDIA_BUS_FMT_FIXED otherwise.
+	 */
+	u32 *(*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);
+
+	/**
+	 * @atomic_get_input_bus_fmts:
+	 *
+	 * Return the supported bus formats on the input end of a bridge for
+	 * a specific output bus format.
+	 *
+	 * The returned array must be allocated with kmalloc() and will be
+	 * freed by the caller. If the allocation fails, NULL should be
+	 * returned. num_output_fmts must be set to the returned array size.
+	 * Formats listed in the returned array should be listed in decreasing
+	 * preference order (the core will try all formats until it finds one
+	 * that works). When the format is not supported NULL should be
+	 * returned and *num_output_fmts should be set to 0.
+	 *
+	 * This method is called on all elements of the bridge chain as part of
+	 * the bus format negotiation process that happens in
+	 * &drm_atomic_bridge_chain_select_bus_fmts().
+	 * This method is optional. When not implemented, the core will bypass
+	 * bus format negotiation on this element of the bridge without
+	 * failing, and the previous element in the chain will be passed
+	 * MEDIA_BUS_FMT_FIXED as its output bus format.
+	 *
+	 * Bridge drivers that need to support being linked to bridges that are
+	 * not supporting bus format negotiation should handle the
+	 * output_fmt == MEDIA_BUS_FMT_FIXED case appropriately, by selecting a
+	 * sensible default value or extracting this information from somewhere
+	 * else (FW property, &drm_display_mode, &drm_display_info, ...)
+	 *
+	 * Note: Even if input format selection on the first bridge has no
+	 * impact on the negotiation process (bus format negotiation stops once
+	 * we reach the first element of the chain), drivers are expected to
+	 * return accurate input formats as the input format may be used to
+	 * configure the CRTC output appropriately.
+	 */
+	u32 *(*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);
+
 	/**
 	 * @atomic_check:
 	 *
@@ -384,6 +450,14 @@ struct drm_bridge_funcs {
 	 * called when &drm_bridge_funcs.atomic_check() is implemented, so only
 	 * one of them should be provided.
 	 *
+	 * If drivers need to tweak &drm_bridge_state.input_bus_cfg.flags or
+	 * &drm_bridge_state.output_bus_cfg.flags it should should happen in
+	 * this function. By default the &drm_bridge_state.output_bus_cfg.flags
+	 * field is set to the next bridge
+	 * &drm_bridge_state.input_bus_cfg.flags value or
+	 * &drm_connector.display_info.bus_flags if the bridge is the last
+	 * element in the chain.
+	 *
 	 * RETURNS:
 	 * zero if the check passed, a negative error code otherwise.
 	 */
@@ -578,6 +652,14 @@ 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);
 
+u32 *
+drm_atomic_helper_bridge_propagate_bus_fmt(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);
+
 #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,
-- 
2.24.1

[PATCH v9 07/12] drm/imx: pd: Use bus format/flags provided by the bridge when available

[Boris Brezillon]

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;
 
-- 
2.24.1

Re: [PATCH v9 07/12] drm/imx: pd: Use bus format/flags provided by the bridge when available

[Philipp Zabel]

Hi Boris,

On Mon, 2020-01-27 at 12:00 +0100, Boris Brezillon wrote:
> 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 @@
[...]
> +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);

You could just use kzalloc() here.

> +	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))

Maybe add a warning so the first person to hook up a bridge / panel with
unsupported flags knows where to look.

Otherwise this looks fine to me.

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

[PATCH v9 08/12] drm/bridge: lvds-codec: Implement basic bus format negotiation

[Boris Brezillon]

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);
-- 
2.24.1

[PATCH v9 09/12] dt-bindings: display: bridge: lvds-codec: Add new bus-width prop

[Boris Brezillon]

Add the bus-width property to describe the input bus format.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
Changes in v7:
* Rebase on top of lvds-codec changes
* Drop the data-mapping property

Changes in v3:
* New patch
---
 .../devicetree/bindings/display/bridge/lvds-codec.yaml    | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
index 8f373029f5d2..7c4e42f4de61 100644
--- a/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/lvds-codec.yaml
@@ -55,6 +55,14 @@ properties:
         description: |
           For LVDS encoders, port 0 is the parallel input
           For LVDS decoders, port 0 is the LVDS input
+        properties:
+          bus-width:
+            allOf:
+              - $ref: /schemas/types.yaml#/definitions/uint32
+              - enum: [18, 24]
+              - default: 24
+          description:
+            Number of data lines used to transmit the RGB data.
 
       port@1:
         type: object
-- 
2.24.1

Re: [PATCH v9 09/12] dt-bindings: display: bridge: lvds-codec: Add new bus-width prop

[Rob Herring]

On Mon, 27 Jan 2020 12:00:40 +0100, Boris Brezillon wrote:
> Add the bus-width property to describe the input bus format.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
> Changes in v7:
> * Rebase on top of lvds-codec changes
> * Drop the data-mapping property
> 
> Changes in v3:
> * New patch
> ---
>  .../devicetree/bindings/display/bridge/lvds-codec.yaml    | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH v9 10/12] drm/bridge: panel: Propage bus format/flags

[Boris Brezillon]

So that the previous bridge element in the chain knows which input
format the panel bridge expects.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
Changes in v7:
* Set atomic state hooks explicitly

Changes in v3:
* Adjust things to match the new bus-format negotiation approach
* Use drm_atomic_helper_bridge_propagate_bus_fmt
* Don't implement ->atomic_check() (the core now takes care of bus
  flags propagation)

Changes in v2:
* Adjust things to match the new bus-format negotiation approach
---
 drivers/gpu/drm/bridge/panel.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
index f66777e24968..dcc72bd7df30 100644
--- a/drivers/gpu/drm/bridge/panel.c
+++ b/drivers/gpu/drm/bridge/panel.c
@@ -127,6 +127,10 @@ static const struct drm_bridge_funcs panel_bridge_bridge_funcs = {
 	.enable = panel_bridge_enable,
 	.disable = panel_bridge_disable,
 	.post_disable = panel_bridge_post_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 = drm_atomic_helper_bridge_propagate_bus_fmt,
 };
 
 /**
-- 
2.24.1

[PATCH v9 11/12] drm/panel: simple: Fix the lt089ac29000 bus_format

[Boris Brezillon]

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,
 };
-- 
2.24.1

[PATCH v9 12/12] ARM: dts: imx: imx51-zii-rdu1: Fix the display pipeline definition

[Boris Brezillon]

The current definition does not represent the exact display pipeline we
have on the board: the LVDS panel is actually connected through a
parallel -> LVDS bridge. Let's fix that so the driver can select the
proper bus format on the CRTC end.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
Changes in v7:
* None
---
 arch/arm/boot/dts/imx51-zii-rdu1.dts | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/imx51-zii-rdu1.dts b/arch/arm/boot/dts/imx51-zii-rdu1.dts
index 3596060f52e7..3fb84ea7f993 100644
--- a/arch/arm/boot/dts/imx51-zii-rdu1.dts
+++ b/arch/arm/boot/dts/imx51-zii-rdu1.dts
@@ -95,6 +95,28 @@ port@1 {
 			reg = <1>;
 
 			display_out: endpoint {
+				remote-endpoint = <&lvds_encoder_in>;
+			};
+		};
+	};
+
+	lvds-encoder {
+		compatible = "lvds-encoder";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@0 {
+			reg = <0>;
+			bus-width = <24>;
+			lvds_encoder_in: endpoint {
+				remote-endpoint = <&display_out>;
+			};
+		};
+
+		port@1 {
+			reg = <1>;
+			data-mapping = "jeida-24";
+			lvds_encoder_out: endpoint {
 				remote-endpoint = <&panel_in>;
 			};
 		};
@@ -110,7 +132,7 @@ panel {
 
 		port {
 			panel_in: endpoint {
-				remote-endpoint = <&display_out>;
+				remote-endpoint = <&lvds_encoder_out>;
 			};
 		};
 	};
-- 
2.24.1