All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v6 0/4] drm: Add support for bus-format negotiation
@ 2020-01-06 14:34 ` Neil Armstrong
  0 siblings, 0 replies; 22+ messages in thread
From: Neil Armstrong @ 2020-01-06 14:34 UTC (permalink / raw)
  To: dri-devel
  Cc: Neil Armstrong, Mark Rutland, Thierry Reding, Laurent Pinchart,
	kernel, Sam Ravnborg, Nikita Yushchenko, Andrey Smirnov,
	Kyungmin Park, Chris Healy, devicetree, Jonas Karlman,
	Rob Herring, Jernej Skrabec, Seung-Woo Kim, Boris Brezillon

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 Major changes in this v6, I addressed the slight changed requested
by Boris from Laurent on the patch 1, and rebased on drm-misc-next. 
Note that this version only contains core changes.
Once those changes are merged I'll send the imx/panel/lvds-codec specific bits.

A more detailed changelog is provided in each patch.

This patch series is also available here [1].

Thanks,

~Boris~ Neil

[1] https://github.com/superna9999/linux/commits/drm-bridge-busfmt-v6

Boris Brezillon (4):
  drm/bridge: Add a drm_bridge_state object
  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/bridge/analogix/analogix_dp_core.c    |  41 +-
 drivers/gpu/drm/drm_atomic.c                  |  39 ++
 drivers/gpu/drm/drm_atomic_helper.c           |  32 +-
 drivers/gpu/drm/drm_bridge.c                  | 527 +++++++++++++++++-
 drivers/gpu/drm/rcar-du/rcar_lvds.c           |   5 +-
 include/drm/drm_atomic.h                      |   3 +
 include/drm/drm_bridge.h                      | 275 ++++++++-
 7 files changed, 874 insertions(+), 48 deletions(-)

-- 
2.22.0


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v6 0/4] drm: Add support for bus-format negotiation
@ 2020-01-06 14:34 ` Neil Armstrong
  0 siblings, 0 replies; 22+ messages in thread
From: Neil Armstrong @ 2020-01-06 14:34 UTC (permalink / raw)
  To: dri-devel
  Cc: Mark Rutland, Nikita Yushchenko, devicetree, Neil Armstrong,
	Andrey Smirnov, Jonas Karlman, Seung-Woo Kim, Jernej Skrabec,
	Rob Herring, Kyungmin Park, Thierry Reding, Laurent Pinchart,
	Boris Brezillon, kernel, Sam Ravnborg, Chris Healy

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 Major changes in this v6, I addressed the slight changed requested
by Boris from Laurent on the patch 1, and rebased on drm-misc-next. 
Note that this version only contains core changes.
Once those changes are merged I'll send the imx/panel/lvds-codec specific bits.

A more detailed changelog is provided in each patch.

This patch series is also available here [1].

Thanks,

~Boris~ Neil

[1] https://github.com/superna9999/linux/commits/drm-bridge-busfmt-v6

Boris Brezillon (4):
  drm/bridge: Add a drm_bridge_state object
  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/bridge/analogix/analogix_dp_core.c    |  41 +-
 drivers/gpu/drm/drm_atomic.c                  |  39 ++
 drivers/gpu/drm/drm_atomic_helper.c           |  32 +-
 drivers/gpu/drm/drm_bridge.c                  | 527 +++++++++++++++++-
 drivers/gpu/drm/rcar-du/rcar_lvds.c           |   5 +-
 include/drm/drm_atomic.h                      |   3 +
 include/drm/drm_bridge.h                      | 275 ++++++++-
 7 files changed, 874 insertions(+), 48 deletions(-)

-- 
2.22.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v6 1/4] drm/bridge: Add a drm_bridge_state object
  2020-01-06 14:34 ` Neil Armstrong
@ 2020-01-06 14:34   ` Neil Armstrong
  -1 siblings, 0 replies; 22+ messages in thread
From: Neil Armstrong @ 2020-01-06 14:34 UTC (permalink / raw)
  To: dri-devel
  Cc: Boris Brezillon, Mark Rutland, Neil Armstrong, Thierry Reding,
	Laurent Pinchart, kernel, Sam Ravnborg, Nikita Yushchenko,
	Andrey Smirnov, Kyungmin Park, Chris Healy, devicetree,
	Jonas Karlman, Rob Herring, Jernej Skrabec, Seung-Woo Kim

From: Boris Brezillon <boris.brezillon@collabora.com>

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>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
Changes in v6:
* Made helpers private, removed doc and moved them to satisfy dependencies
* Renamed helpers to _default_

Changes in v5:
* Re-introduced the helpers from v4

Changes in v4:
* Fix the doc
* Kill default helpers (inlined)
* Fix drm_atomic_get_bridge_state() to check for an ERR_PTR()
* Add Neil's R-b

Changes in v3:
* None

Changes in v2:
* Use drm_for_each_bridge_in_chain()
* Rename helpers to be more consistent with the rest of the DRM API
* Improve/fix the doc
---
 drivers/gpu/drm/drm_atomic.c        |  39 ++++++++
 drivers/gpu/drm/drm_atomic_helper.c |  20 ++++
 drivers/gpu/drm/drm_bridge.c        | 139 ++++++++++++++++++++++++++--
 include/drm/drm_atomic.h            |   3 +
 include/drm/drm_bridge.h            | 114 +++++++++++++++++++++++
 5 files changed, 309 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index d33691512a8e..bf1b9c37d515 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,44 @@ static void drm_atomic_connector_print_state(struct drm_printer *p,
 		connector->funcs->atomic_print_state(p, state);
 }
 
+/**
+ * drm_atomic_add_encoder_bridges - add bridges attached to an encoder
+ * @state: atomic state
+ * @encoder: DRM encoder
+ *
+ * This function adds all bridges attached to @encoder. This is needed to add
+ * bridge states to @state and make them available when
+ * &bridge_funcs.atomic_{check,pre_enable,enable,disable_post_disable}() are
+ * called
+ *
+ * Returns:
+ * 0 on success or can fail with -EDEADLK or -ENOMEM. When the error is EDEADLK
+ * then the w/w mutex code has detected a deadlock and the entire atomic
+ * sequence must be restarted. All other errors are fatal.
+ */
+int
+drm_atomic_add_encoder_bridges(struct drm_atomic_state *state,
+			       struct drm_encoder *encoder)
+{
+	struct drm_bridge_state *bridge_state;
+	struct drm_bridge *bridge;
+
+	if (!encoder)
+		return 0;
+
+	DRM_DEBUG_ATOMIC("Adding all bridges for [encoder:%d:%s] to %p\n",
+			 encoder->base.id, encoder->name, state);
+
+	drm_for_each_bridge_in_chain(encoder, bridge) {
+		bridge_state = drm_atomic_get_bridge_state(state, bridge);
+		if (IS_ERR(bridge_state))
+			return PTR_ERR(bridge_state);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_atomic_add_encoder_bridges);
+
 /**
  * drm_atomic_add_affected_connectors - add connectors for CRTC
  * @state: atomic state
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 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_bridge.c b/drivers/gpu/drm/drm_bridge.c
index c2cf0c90fa26..a213c9042f2c 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,74 @@ void drm_bridge_remove(struct drm_bridge *bridge)
 }
 EXPORT_SYMBOL(drm_bridge_remove);
 
+static struct drm_bridge_state *
+drm_atomic_default_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;
+}
+
+static struct drm_private_state *
+drm_bridge_atomic_duplicate_priv_state(struct drm_private_obj *obj)
+{
+	struct drm_bridge *bridge = drm_priv_to_bridge(obj);
+	struct drm_bridge_state *state;
+
+	if (bridge->funcs->atomic_duplicate_state)
+		state = bridge->funcs->atomic_duplicate_state(bridge);
+	else
+		state = drm_atomic_default_bridge_duplicate_state(bridge);
+
+	return state ? &state->base : NULL;
+}
+
+static void
+drm_atomic_default_bridge_destroy_state(struct drm_bridge *bridge,
+					struct drm_bridge_state *state)
+{
+	/* Just a simple kfree() for now */
+	kfree(state);
+}
+
+static void
+drm_bridge_atomic_destroy_priv_state(struct drm_private_obj *obj,
+				     struct drm_private_state *s)
+{
+	struct drm_bridge_state *state = drm_priv_to_bridge_state(s);
+	struct drm_bridge *bridge = drm_priv_to_bridge(obj);
+
+	if (bridge->funcs->atomic_destroy_state)
+		bridge->funcs->atomic_destroy_state(bridge, state);
+	else
+		drm_atomic_default_bridge_destroy_state(bridge, state);
+}
+
+static const struct drm_private_state_funcs drm_bridge_priv_state_funcs = {
+	.atomic_duplicate_state = drm_bridge_atomic_duplicate_priv_state,
+	.atomic_destroy_state = drm_bridge_atomic_destroy_priv_state,
+};
+
+static struct drm_bridge_state *
+drm_atomic_default_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;
+}
+
 /**
  * drm_bridge_attach - attach the bridge to an encoder's chain
  *
@@ -114,6 +183,7 @@ EXPORT_SYMBOL(drm_bridge_remove);
 int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 		      struct drm_bridge *previous)
 {
+	struct drm_bridge_state *state;
 	int ret;
 
 	if (!encoder || !bridge)
@@ -135,15 +205,35 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 
 	if (bridge->funcs->attach) {
 		ret = bridge->funcs->attach(bridge);
-		if (ret < 0) {
-			list_del(&bridge->chain_node);
-			bridge->dev = NULL;
-			bridge->encoder = NULL;
-			return ret;
-		}
+		if (ret < 0)
+			goto err_reset_bridge;
 	}
 
+	if (bridge->funcs->atomic_reset)
+		state = bridge->funcs->atomic_reset(bridge);
+	else
+		state = drm_atomic_default_bridge_reset(bridge);
+
+	if (IS_ERR(state)) {
+		ret = PTR_ERR(state);
+		goto err_detach_bridge;
+	}
+
+	drm_atomic_private_obj_init(bridge->dev, &bridge->base,
+				    &state->base,
+				    &drm_bridge_priv_state_funcs);
+
 	return 0;
+
+err_detach_bridge:
+	if (bridge->funcs->detach)
+		bridge->funcs->detach(bridge);
+
+err_reset_bridge:
+	bridge->dev = NULL;
+	bridge->encoder = NULL;
+	list_del(&bridge->chain_node);
+	return ret;
 }
 EXPORT_SYMBOL(drm_bridge_attach);
 
@@ -155,6 +245,8 @@ void drm_bridge_detach(struct drm_bridge *bridge)
 	if (WARN_ON(!bridge->dev))
 		return;
 
+	drm_atomic_private_obj_fini(&bridge->base);
+
 	if (bridge->funcs->detach)
 		bridge->funcs->detach(bridge);
 
@@ -516,6 +608,41 @@ void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge,
 }
 EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);
 
+/**
+ * __drm_atomic_helper_bridge_reset() - Initialize a bridge state to its
+ *					default
+ * @bridge: the bridge this state is refers to
+ * @state: bridge state to initialize
+ *
+ * Initialize the bridge state to default values. This is meant to be* called
+ * by the bridge &drm_plane_funcs.reset hook for bridges that subclass the
+ * bridge state.
+ */
+void __drm_atomic_helper_bridge_reset(struct drm_bridge *bridge,
+				      struct drm_bridge_state *state)
+{
+	memset(state, 0, sizeof(*state));
+	state->bridge = bridge;
+}
+EXPORT_SYMBOL(__drm_atomic_helper_bridge_reset);
+
+/**
+ * __drm_atomic_helper_bridge_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);
+
 #ifdef CONFIG_OF
 /**
  * of_drm_find_bridge - find the bridge corresponding to the device node in
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 951dfb15c27b..ccce65e14917 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -669,6 +669,9 @@ __drm_atomic_get_current_plane_state(struct drm_atomic_state *state,
 	return plane->state;
 }
 
+int __must_check
+drm_atomic_add_encoder_bridges(struct drm_atomic_state *state,
+			       struct drm_encoder *encoder);
 int __must_check
 drm_atomic_add_affected_connectors(struct drm_atomic_state *state,
 				   struct drm_crtc *crtc);
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 694e153a7531..fc7c71f4de55 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>
@@ -33,6 +35,23 @@ struct drm_bridge;
 struct drm_bridge_timings;
 struct drm_panel;
 
+/**
+ * struct drm_bridge_state - Atomic bridge state object
+ * @base: inherit from &drm_private_state
+ * @bridge: the bridge this state refers to
+ */
+struct drm_bridge_state {
+	struct drm_private_state base;
+
+	struct drm_bridge *bridge;
+};
+
+static inline struct drm_bridge_state *
+drm_priv_to_bridge_state(struct drm_private_state *priv)
+{
+	return container_of(priv, struct drm_bridge_state, base);
+}
+
 /**
  * struct drm_bridge_funcs - drm_bridge control functions
  */
@@ -338,6 +357,49 @@ struct drm_bridge_funcs {
 	 */
 	void (*atomic_post_disable)(struct drm_bridge *bridge,
 				    struct drm_atomic_state *old_state);
+
+	/**
+	 * @atomic_duplicate_state:
+	 *
+	 * Duplicate the current bridge state object (which is guaranteed to be
+	 * non-NULL).
+	 *
+	 * The atomic_duplicate_state() is optional. When not implemented the
+	 * core allocates a drm_bridge_state object and calls
+	 * &__drm_atomic_helper_bridge_duplicate_state() to initialize it.
+	 *
+	 * RETURNS:
+	 * A valid drm_bridge_state object or NULL if the allocation fails.
+	 */
+	struct drm_bridge_state *(*atomic_duplicate_state)(struct drm_bridge *bridge);
+
+	/**
+	 * @atomic_destroy_state:
+	 *
+	 * Destroy a bridge state object previously allocated by
+	 * &drm_bridge_funcs.atomic_duplicate_state().
+	 *
+	 * The atomic_destroy_state hook is optional. When not implemented the
+	 * core calls kfree() on the state.
+	 */
+	void (*atomic_destroy_state)(struct drm_bridge *bridge,
+				     struct drm_bridge_state *state);
+
+	/**
+	 * @atomic_reset:
+	 *
+	 * Reset the bridge to a predefined state (or retrieve its current
+	 * state) and return a &drm_bridge_state object matching this state.
+	 * This function is called at attach time.
+	 *
+	 * The atomic_reset hook is optional. When not implemented the core
+	 * allocates a new state and calls &__drm_atomic_helper_bridge_reset().
+	 *
+	 * RETURNS:
+	 * A valid drm_bridge_state object in case of success, an ERR_PTR()
+	 * giving the reason of the failure otherwise.
+	 */
+	struct drm_bridge_state *(*atomic_reset)(struct drm_bridge *bridge);
 };
 
 /**
@@ -380,6 +442,8 @@ struct drm_bridge_timings {
  * struct drm_bridge - central DRM bridge control structure
  */
 struct drm_bridge {
+	/** @base: inherit from &drm_private_object */
+	struct drm_private_obj base;
 	/** @dev: DRM device this bridge belongs to */
 	struct drm_device *dev;
 	/** @encoder: encoder to which this bridge is connected */
@@ -404,6 +468,12 @@ struct drm_bridge {
 	void *driver_private;
 };
 
+static inline struct drm_bridge *
+drm_priv_to_bridge(struct drm_private_obj *priv)
+{
+	return container_of(priv, struct drm_bridge, base);
+}
+
 void drm_bridge_add(struct drm_bridge *bridge);
 void drm_bridge_remove(struct drm_bridge *bridge);
 struct drm_bridge *of_drm_find_bridge(struct device_node *np);
@@ -491,6 +561,50 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
 void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge,
 				    struct drm_atomic_state *state);
 
+void __drm_atomic_helper_bridge_reset(struct drm_bridge *bridge,
+				      struct drm_bridge_state *state);
+void __drm_atomic_helper_bridge_duplicate_state(struct drm_bridge *bridge,
+						struct drm_bridge_state *new);
+
+static inline struct drm_bridge_state *
+drm_atomic_get_bridge_state(struct drm_atomic_state *state,
+			    struct drm_bridge *bridge)
+{
+	struct drm_private_state *obj_state;
+
+	obj_state = drm_atomic_get_private_obj_state(state, &bridge->base);
+	if (IS_ERR(obj_state))
+		return ERR_CAST(obj_state);
+
+	return drm_priv_to_bridge_state(obj_state);
+}
+
+static inline struct drm_bridge_state *
+drm_atomic_get_old_bridge_state(struct drm_atomic_state *state,
+				struct drm_bridge *bridge)
+{
+	struct drm_private_state *obj_state;
+
+	obj_state = drm_atomic_get_old_private_obj_state(state, &bridge->base);
+	if (!obj_state)
+		return NULL;
+
+	return drm_priv_to_bridge_state(obj_state);
+}
+
+static inline struct drm_bridge_state *
+drm_atomic_get_new_bridge_state(struct drm_atomic_state *state,
+				struct drm_bridge *bridge)
+{
+	struct drm_private_state *obj_state;
+
+	obj_state = drm_atomic_get_new_private_obj_state(state, &bridge->base);
+	if (!obj_state)
+		return NULL;
+
+	return drm_priv_to_bridge_state(obj_state);
+}
+
 #ifdef CONFIG_DRM_PANEL_BRIDGE
 struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel);
 struct drm_bridge *drm_panel_bridge_add_typed(struct drm_panel *panel,
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v6 1/4] drm/bridge: Add a drm_bridge_state object
@ 2020-01-06 14:34   ` Neil Armstrong
  0 siblings, 0 replies; 22+ messages in thread
From: Neil Armstrong @ 2020-01-06 14:34 UTC (permalink / raw)
  To: dri-devel
  Cc: Mark Rutland, Nikita Yushchenko, devicetree, Neil Armstrong,
	Andrey Smirnov, Jonas Karlman, Seung-Woo Kim, Jernej Skrabec,
	Rob Herring, Kyungmin Park, Boris Brezillon, Thierry Reding,
	Laurent Pinchart, kernel, Sam Ravnborg, Chris Healy

From: Boris Brezillon <boris.brezillon@collabora.com>

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>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
Changes in v6:
* Made helpers private, removed doc and moved them to satisfy dependencies
* Renamed helpers to _default_

Changes in v5:
* Re-introduced the helpers from v4

Changes in v4:
* Fix the doc
* Kill default helpers (inlined)
* Fix drm_atomic_get_bridge_state() to check for an ERR_PTR()
* Add Neil's R-b

Changes in v3:
* None

Changes in v2:
* Use drm_for_each_bridge_in_chain()
* Rename helpers to be more consistent with the rest of the DRM API
* Improve/fix the doc
---
 drivers/gpu/drm/drm_atomic.c        |  39 ++++++++
 drivers/gpu/drm/drm_atomic_helper.c |  20 ++++
 drivers/gpu/drm/drm_bridge.c        | 139 ++++++++++++++++++++++++++--
 include/drm/drm_atomic.h            |   3 +
 include/drm/drm_bridge.h            | 114 +++++++++++++++++++++++
 5 files changed, 309 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index d33691512a8e..bf1b9c37d515 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,44 @@ static void drm_atomic_connector_print_state(struct drm_printer *p,
 		connector->funcs->atomic_print_state(p, state);
 }
 
+/**
+ * drm_atomic_add_encoder_bridges - add bridges attached to an encoder
+ * @state: atomic state
+ * @encoder: DRM encoder
+ *
+ * This function adds all bridges attached to @encoder. This is needed to add
+ * bridge states to @state and make them available when
+ * &bridge_funcs.atomic_{check,pre_enable,enable,disable_post_disable}() are
+ * called
+ *
+ * Returns:
+ * 0 on success or can fail with -EDEADLK or -ENOMEM. When the error is EDEADLK
+ * then the w/w mutex code has detected a deadlock and the entire atomic
+ * sequence must be restarted. All other errors are fatal.
+ */
+int
+drm_atomic_add_encoder_bridges(struct drm_atomic_state *state,
+			       struct drm_encoder *encoder)
+{
+	struct drm_bridge_state *bridge_state;
+	struct drm_bridge *bridge;
+
+	if (!encoder)
+		return 0;
+
+	DRM_DEBUG_ATOMIC("Adding all bridges for [encoder:%d:%s] to %p\n",
+			 encoder->base.id, encoder->name, state);
+
+	drm_for_each_bridge_in_chain(encoder, bridge) {
+		bridge_state = drm_atomic_get_bridge_state(state, bridge);
+		if (IS_ERR(bridge_state))
+			return PTR_ERR(bridge_state);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_atomic_add_encoder_bridges);
+
 /**
  * drm_atomic_add_affected_connectors - add connectors for CRTC
  * @state: atomic state
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 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_bridge.c b/drivers/gpu/drm/drm_bridge.c
index c2cf0c90fa26..a213c9042f2c 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,74 @@ void drm_bridge_remove(struct drm_bridge *bridge)
 }
 EXPORT_SYMBOL(drm_bridge_remove);
 
+static struct drm_bridge_state *
+drm_atomic_default_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;
+}
+
+static struct drm_private_state *
+drm_bridge_atomic_duplicate_priv_state(struct drm_private_obj *obj)
+{
+	struct drm_bridge *bridge = drm_priv_to_bridge(obj);
+	struct drm_bridge_state *state;
+
+	if (bridge->funcs->atomic_duplicate_state)
+		state = bridge->funcs->atomic_duplicate_state(bridge);
+	else
+		state = drm_atomic_default_bridge_duplicate_state(bridge);
+
+	return state ? &state->base : NULL;
+}
+
+static void
+drm_atomic_default_bridge_destroy_state(struct drm_bridge *bridge,
+					struct drm_bridge_state *state)
+{
+	/* Just a simple kfree() for now */
+	kfree(state);
+}
+
+static void
+drm_bridge_atomic_destroy_priv_state(struct drm_private_obj *obj,
+				     struct drm_private_state *s)
+{
+	struct drm_bridge_state *state = drm_priv_to_bridge_state(s);
+	struct drm_bridge *bridge = drm_priv_to_bridge(obj);
+
+	if (bridge->funcs->atomic_destroy_state)
+		bridge->funcs->atomic_destroy_state(bridge, state);
+	else
+		drm_atomic_default_bridge_destroy_state(bridge, state);
+}
+
+static const struct drm_private_state_funcs drm_bridge_priv_state_funcs = {
+	.atomic_duplicate_state = drm_bridge_atomic_duplicate_priv_state,
+	.atomic_destroy_state = drm_bridge_atomic_destroy_priv_state,
+};
+
+static struct drm_bridge_state *
+drm_atomic_default_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;
+}
+
 /**
  * drm_bridge_attach - attach the bridge to an encoder's chain
  *
@@ -114,6 +183,7 @@ EXPORT_SYMBOL(drm_bridge_remove);
 int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 		      struct drm_bridge *previous)
 {
+	struct drm_bridge_state *state;
 	int ret;
 
 	if (!encoder || !bridge)
@@ -135,15 +205,35 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge,
 
 	if (bridge->funcs->attach) {
 		ret = bridge->funcs->attach(bridge);
-		if (ret < 0) {
-			list_del(&bridge->chain_node);
-			bridge->dev = NULL;
-			bridge->encoder = NULL;
-			return ret;
-		}
+		if (ret < 0)
+			goto err_reset_bridge;
 	}
 
+	if (bridge->funcs->atomic_reset)
+		state = bridge->funcs->atomic_reset(bridge);
+	else
+		state = drm_atomic_default_bridge_reset(bridge);
+
+	if (IS_ERR(state)) {
+		ret = PTR_ERR(state);
+		goto err_detach_bridge;
+	}
+
+	drm_atomic_private_obj_init(bridge->dev, &bridge->base,
+				    &state->base,
+				    &drm_bridge_priv_state_funcs);
+
 	return 0;
+
+err_detach_bridge:
+	if (bridge->funcs->detach)
+		bridge->funcs->detach(bridge);
+
+err_reset_bridge:
+	bridge->dev = NULL;
+	bridge->encoder = NULL;
+	list_del(&bridge->chain_node);
+	return ret;
 }
 EXPORT_SYMBOL(drm_bridge_attach);
 
@@ -155,6 +245,8 @@ void drm_bridge_detach(struct drm_bridge *bridge)
 	if (WARN_ON(!bridge->dev))
 		return;
 
+	drm_atomic_private_obj_fini(&bridge->base);
+
 	if (bridge->funcs->detach)
 		bridge->funcs->detach(bridge);
 
@@ -516,6 +608,41 @@ void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge,
 }
 EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);
 
+/**
+ * __drm_atomic_helper_bridge_reset() - Initialize a bridge state to its
+ *					default
+ * @bridge: the bridge this state is refers to
+ * @state: bridge state to initialize
+ *
+ * Initialize the bridge state to default values. This is meant to be* called
+ * by the bridge &drm_plane_funcs.reset hook for bridges that subclass the
+ * bridge state.
+ */
+void __drm_atomic_helper_bridge_reset(struct drm_bridge *bridge,
+				      struct drm_bridge_state *state)
+{
+	memset(state, 0, sizeof(*state));
+	state->bridge = bridge;
+}
+EXPORT_SYMBOL(__drm_atomic_helper_bridge_reset);
+
+/**
+ * __drm_atomic_helper_bridge_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);
+
 #ifdef CONFIG_OF
 /**
  * of_drm_find_bridge - find the bridge corresponding to the device node in
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 951dfb15c27b..ccce65e14917 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -669,6 +669,9 @@ __drm_atomic_get_current_plane_state(struct drm_atomic_state *state,
 	return plane->state;
 }
 
+int __must_check
+drm_atomic_add_encoder_bridges(struct drm_atomic_state *state,
+			       struct drm_encoder *encoder);
 int __must_check
 drm_atomic_add_affected_connectors(struct drm_atomic_state *state,
 				   struct drm_crtc *crtc);
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 694e153a7531..fc7c71f4de55 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>
@@ -33,6 +35,23 @@ struct drm_bridge;
 struct drm_bridge_timings;
 struct drm_panel;
 
+/**
+ * struct drm_bridge_state - Atomic bridge state object
+ * @base: inherit from &drm_private_state
+ * @bridge: the bridge this state refers to
+ */
+struct drm_bridge_state {
+	struct drm_private_state base;
+
+	struct drm_bridge *bridge;
+};
+
+static inline struct drm_bridge_state *
+drm_priv_to_bridge_state(struct drm_private_state *priv)
+{
+	return container_of(priv, struct drm_bridge_state, base);
+}
+
 /**
  * struct drm_bridge_funcs - drm_bridge control functions
  */
@@ -338,6 +357,49 @@ struct drm_bridge_funcs {
 	 */
 	void (*atomic_post_disable)(struct drm_bridge *bridge,
 				    struct drm_atomic_state *old_state);
+
+	/**
+	 * @atomic_duplicate_state:
+	 *
+	 * Duplicate the current bridge state object (which is guaranteed to be
+	 * non-NULL).
+	 *
+	 * The atomic_duplicate_state() is optional. When not implemented the
+	 * core allocates a drm_bridge_state object and calls
+	 * &__drm_atomic_helper_bridge_duplicate_state() to initialize it.
+	 *
+	 * RETURNS:
+	 * A valid drm_bridge_state object or NULL if the allocation fails.
+	 */
+	struct drm_bridge_state *(*atomic_duplicate_state)(struct drm_bridge *bridge);
+
+	/**
+	 * @atomic_destroy_state:
+	 *
+	 * Destroy a bridge state object previously allocated by
+	 * &drm_bridge_funcs.atomic_duplicate_state().
+	 *
+	 * The atomic_destroy_state hook is optional. When not implemented the
+	 * core calls kfree() on the state.
+	 */
+	void (*atomic_destroy_state)(struct drm_bridge *bridge,
+				     struct drm_bridge_state *state);
+
+	/**
+	 * @atomic_reset:
+	 *
+	 * Reset the bridge to a predefined state (or retrieve its current
+	 * state) and return a &drm_bridge_state object matching this state.
+	 * This function is called at attach time.
+	 *
+	 * The atomic_reset hook is optional. When not implemented the core
+	 * allocates a new state and calls &__drm_atomic_helper_bridge_reset().
+	 *
+	 * RETURNS:
+	 * A valid drm_bridge_state object in case of success, an ERR_PTR()
+	 * giving the reason of the failure otherwise.
+	 */
+	struct drm_bridge_state *(*atomic_reset)(struct drm_bridge *bridge);
 };
 
 /**
@@ -380,6 +442,8 @@ struct drm_bridge_timings {
  * struct drm_bridge - central DRM bridge control structure
  */
 struct drm_bridge {
+	/** @base: inherit from &drm_private_object */
+	struct drm_private_obj base;
 	/** @dev: DRM device this bridge belongs to */
 	struct drm_device *dev;
 	/** @encoder: encoder to which this bridge is connected */
@@ -404,6 +468,12 @@ struct drm_bridge {
 	void *driver_private;
 };
 
+static inline struct drm_bridge *
+drm_priv_to_bridge(struct drm_private_obj *priv)
+{
+	return container_of(priv, struct drm_bridge, base);
+}
+
 void drm_bridge_add(struct drm_bridge *bridge);
 void drm_bridge_remove(struct drm_bridge *bridge);
 struct drm_bridge *of_drm_find_bridge(struct device_node *np);
@@ -491,6 +561,50 @@ void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
 void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge,
 				    struct drm_atomic_state *state);
 
+void __drm_atomic_helper_bridge_reset(struct drm_bridge *bridge,
+				      struct drm_bridge_state *state);
+void __drm_atomic_helper_bridge_duplicate_state(struct drm_bridge *bridge,
+						struct drm_bridge_state *new);
+
+static inline struct drm_bridge_state *
+drm_atomic_get_bridge_state(struct drm_atomic_state *state,
+			    struct drm_bridge *bridge)
+{
+	struct drm_private_state *obj_state;
+
+	obj_state = drm_atomic_get_private_obj_state(state, &bridge->base);
+	if (IS_ERR(obj_state))
+		return ERR_CAST(obj_state);
+
+	return drm_priv_to_bridge_state(obj_state);
+}
+
+static inline struct drm_bridge_state *
+drm_atomic_get_old_bridge_state(struct drm_atomic_state *state,
+				struct drm_bridge *bridge)
+{
+	struct drm_private_state *obj_state;
+
+	obj_state = drm_atomic_get_old_private_obj_state(state, &bridge->base);
+	if (!obj_state)
+		return NULL;
+
+	return drm_priv_to_bridge_state(obj_state);
+}
+
+static inline struct drm_bridge_state *
+drm_atomic_get_new_bridge_state(struct drm_atomic_state *state,
+				struct drm_bridge *bridge)
+{
+	struct drm_private_state *obj_state;
+
+	obj_state = drm_atomic_get_new_private_obj_state(state, &bridge->base);
+	if (!obj_state)
+		return NULL;
+
+	return drm_priv_to_bridge_state(obj_state);
+}
+
 #ifdef CONFIG_DRM_PANEL_BRIDGE
 struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel);
 struct drm_bridge *drm_panel_bridge_add_typed(struct drm_panel *panel,
-- 
2.22.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v6 2/4] drm/bridge: Patch atomic hooks to take a drm_bridge_state
  2020-01-06 14:34 ` Neil Armstrong
@ 2020-01-06 14:34   ` Neil Armstrong
  -1 siblings, 0 replies; 22+ messages in thread
From: Neil Armstrong @ 2020-01-06 14:34 UTC (permalink / raw)
  To: dri-devel
  Cc: Boris Brezillon, Mark Rutland, Neil Armstrong, Thierry Reding,
	Laurent Pinchart, kernel, Sam Ravnborg, Nikita Yushchenko,
	Andrey Smirnov, Kyungmin Park, Chris Healy, devicetree,
	Jonas Karlman, Rob Herring, Jernej Skrabec, Seung-Woo Kim

From: Boris Brezillon <boris.brezillon@collabora.com>

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>
---
Changes in v6:
* Also fixed rcar-du/rcar_lvds.c same as analogix/analogix_dp_core.c

Changes in v5:
* None

Changes in v4:
* Rename func params into old_bridge_state
* Add Laurent's Rb

Changes in v3:
* Old state clarification moved to a separate patch

Changes in v2:
* Pass the old bridge state
---
 .../drm/bridge/analogix/analogix_dp_core.c    | 41 +++++++------
 drivers/gpu/drm/drm_bridge.c                  | 61 +++++++++++++++----
 drivers/gpu/drm/rcar-du/rcar_lvds.c           |  5 +-
 include/drm/drm_bridge.h                      |  8 +--
 4 files changed, 80 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 6effe532f820..6fab71985cd4 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 a213c9042f2c..872e159fcb42 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -501,10 +501,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;
@@ -535,10 +544,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);
@@ -567,10 +586,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;
@@ -600,10 +628,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 8ffa4fbbdeb3..b8b22dc55bdb 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 *state)
 {
 	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
 
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index fc7c71f4de55..52d3ed150618 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -282,7 +282,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:
@@ -307,7 +307,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:
 	 *
@@ -330,7 +330,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:
@@ -356,7 +356,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.22.0


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v6 2/4] drm/bridge: Patch atomic hooks to take a drm_bridge_state
@ 2020-01-06 14:34   ` Neil Armstrong
  0 siblings, 0 replies; 22+ messages in thread
From: Neil Armstrong @ 2020-01-06 14:34 UTC (permalink / raw)
  To: dri-devel
  Cc: Mark Rutland, Nikita Yushchenko, devicetree, Neil Armstrong,
	Andrey Smirnov, Jonas Karlman, Seung-Woo Kim, Jernej Skrabec,
	Rob Herring, Kyungmin Park, Boris Brezillon, Thierry Reding,
	Laurent Pinchart, kernel, Sam Ravnborg, Chris Healy

From: Boris Brezillon <boris.brezillon@collabora.com>

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>
---
Changes in v6:
* Also fixed rcar-du/rcar_lvds.c same as analogix/analogix_dp_core.c

Changes in v5:
* None

Changes in v4:
* Rename func params into old_bridge_state
* Add Laurent's Rb

Changes in v3:
* Old state clarification moved to a separate patch

Changes in v2:
* Pass the old bridge state
---
 .../drm/bridge/analogix/analogix_dp_core.c    | 41 +++++++------
 drivers/gpu/drm/drm_bridge.c                  | 61 +++++++++++++++----
 drivers/gpu/drm/rcar-du/rcar_lvds.c           |  5 +-
 include/drm/drm_bridge.h                      |  8 +--
 4 files changed, 80 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 6effe532f820..6fab71985cd4 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 a213c9042f2c..872e159fcb42 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -501,10 +501,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;
@@ -535,10 +544,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);
@@ -567,10 +586,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;
@@ -600,10 +628,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 8ffa4fbbdeb3..b8b22dc55bdb 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 *state)
 {
 	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
 
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index fc7c71f4de55..52d3ed150618 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -282,7 +282,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:
@@ -307,7 +307,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:
 	 *
@@ -330,7 +330,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:
@@ -356,7 +356,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.22.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v6 3/4] drm/bridge: Add an ->atomic_check() hook
  2020-01-06 14:34 ` Neil Armstrong
@ 2020-01-06 14:34   ` Neil Armstrong
  -1 siblings, 0 replies; 22+ messages in thread
From: Neil Armstrong @ 2020-01-06 14:34 UTC (permalink / raw)
  To: dri-devel
  Cc: Boris Brezillon, Mark Rutland, Neil Armstrong, Thierry Reding,
	Laurent Pinchart, kernel, Sam Ravnborg, Nikita Yushchenko,
	Andrey Smirnov, Kyungmin Park, Chris Healy, devicetree,
	Jonas Karlman, Rob Herring, Jernej Skrabec, Seung-Woo Kim

From: Boris Brezillon <boris.brezillon@collabora.com>

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>
---
Changes in v6:
* None

Changes in v5:
* None

Changes in v4:
* Add R-bs

Changes in v3:
* None

Changes in v2:
* Clarify the fact that ->atomic_check() is replacing ->mode_fixup()
---
 drivers/gpu/drm/drm_atomic_helper.c | 12 +++---
 drivers/gpu/drm/drm_bridge.c        | 62 +++++++++++++++++++++++++++++
 include/drm/drm_bridge.h            | 29 +++++++++++++-
 3 files changed, 96 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 872e159fcb42..8e4b799150b0 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -645,6 +645,68 @@ 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 = bridge->encoder;
+	struct drm_bridge *iter;
+
+	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);
+
 /**
  * __drm_atomic_helper_bridge_reset() - Initialize a bridge state to its
  *					default
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 52d3ed150618..ae0595c70132 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -128,7 +128,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:
 	 *
@@ -385,6 +387,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:
 	 *
@@ -552,6 +576,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.22.0


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v6 3/4] drm/bridge: Add an ->atomic_check() hook
@ 2020-01-06 14:34   ` Neil Armstrong
  0 siblings, 0 replies; 22+ messages in thread
From: Neil Armstrong @ 2020-01-06 14:34 UTC (permalink / raw)
  To: dri-devel
  Cc: Mark Rutland, Nikita Yushchenko, devicetree, Neil Armstrong,
	Andrey Smirnov, Jonas Karlman, Seung-Woo Kim, Jernej Skrabec,
	Rob Herring, Kyungmin Park, Boris Brezillon, Thierry Reding,
	Laurent Pinchart, kernel, Sam Ravnborg, Chris Healy

From: Boris Brezillon <boris.brezillon@collabora.com>

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>
---
Changes in v6:
* None

Changes in v5:
* None

Changes in v4:
* Add R-bs

Changes in v3:
* None

Changes in v2:
* Clarify the fact that ->atomic_check() is replacing ->mode_fixup()
---
 drivers/gpu/drm/drm_atomic_helper.c | 12 +++---
 drivers/gpu/drm/drm_bridge.c        | 62 +++++++++++++++++++++++++++++
 include/drm/drm_bridge.h            | 29 +++++++++++++-
 3 files changed, 96 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 872e159fcb42..8e4b799150b0 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -645,6 +645,68 @@ 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 = bridge->encoder;
+	struct drm_bridge *iter;
+
+	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);
+
 /**
  * __drm_atomic_helper_bridge_reset() - Initialize a bridge state to its
  *					default
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 52d3ed150618..ae0595c70132 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -128,7 +128,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:
 	 *
@@ -385,6 +387,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:
 	 *
@@ -552,6 +576,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.22.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v6 4/4] drm/bridge: Add the necessary bits to support bus format negotiation
  2020-01-06 14:34 ` Neil Armstrong
@ 2020-01-06 14:34   ` Neil Armstrong
  -1 siblings, 0 replies; 22+ messages in thread
From: Neil Armstrong @ 2020-01-06 14:34 UTC (permalink / raw)
  To: dri-devel
  Cc: Boris Brezillon, Mark Rutland, Neil Armstrong, Thierry Reding,
	Laurent Pinchart, kernel, Sam Ravnborg, Nikita Yushchenko,
	Andrey Smirnov, Kyungmin Park, Chris Healy, devicetree,
	Jonas Karlman, Rob Herring, Jernej Skrabec, Seung-Woo Kim

From: Boris Brezillon <boris.brezillon@collabora.com>

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>
---
Changes in v6:
* None

Changes in v5:
* None

Changes in v4:
* Enhance the doc
* Fix typos
* Rename some parameters/fields
* Reword the commit message

Changes in v3:
* Fix the commit message (Reported by Laurent)
* Document the fact that bus formats should not be directly modified by
  drivers (Suggested by Laurent)
* Document the fact that format order matters (Suggested by Laurent)
* Propagate bus flags by default
* Document the fact that drivers can tweak bus flags if needed
* Let ->atomic_get_{output,input}_bus_fmts() allocate the bus format
  array (Suggested by Laurent)
* Add a drm_atomic_helper_bridge_propagate_bus_fmt()
* Mandate that bridge drivers return accurate input_fmts even if they
  are known to be the first element in the bridge chain

Changes in v2:
* Rework things to support more complex use cases
---
 drivers/gpu/drm/drm_bridge.c | 267 ++++++++++++++++++++++++++++++++++-
 include/drm/drm_bridge.h     | 124 ++++++++++++++++
 2 files changed, 390 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 8e4b799150b0..37400607e9b7 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -671,13 +671,261 @@ static int drm_atomic_bridge_check(struct drm_bridge *bridge,
 	return 0;
 }
 
+/**
+ * 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);
+
+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 (WARN_ON(!cur_state))
+		return -EINVAL;
+
+	/*
+	 * 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;
+		}
+
+		cur_state->input_bus_cfg.format = MEDIA_BUS_FMT_FIXED;
+		cur_state->output_bus_cfg.format = out_bus_fmt;
+		return 0;
+	}
+
+	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 (WARN_ON(!last_bridge_state))
+		return -EINVAL;
+
+	if (last_bridge->funcs->atomic_get_output_bus_fmts) {
+		const struct drm_bridge_funcs *funcs = last_bridge->funcs;
+
+		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;
+
+	bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
+	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);
+		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()
@@ -689,12 +937,29 @@ 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 = bridge->encoder;
 	struct drm_bridge *iter;
+	int ret;
+
+	ret = drm_atomic_bridge_chain_select_bus_fmts(bridge, crtc_state,
+						      conn_state);
+	if (ret)
+		return ret;
 
 	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_bridge.h b/include/drm/drm_bridge.h
index ae0595c70132..39a5363dac40 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -35,6 +35,38 @@ struct drm_bridge;
 struct drm_bridge_timings;
 struct drm_panel;
 
+/**
+ * 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 {
+	/**
+	 * @fmt: 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
  * @base: inherit from &drm_private_state
@@ -44,6 +76,16 @@ struct drm_bridge_state {
 	struct drm_private_state base;
 
 	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 *
@@ -387,6 +429,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:
 	 *
@@ -401,6 +509,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.
 	 */
@@ -588,6 +704,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);
+
 void __drm_atomic_helper_bridge_reset(struct drm_bridge *bridge,
 				      struct drm_bridge_state *state);
 void __drm_atomic_helper_bridge_duplicate_state(struct drm_bridge *bridge,
-- 
2.22.0


^ permalink raw reply related	[flat|nested] 22+ messages in thread

* [PATCH v6 4/4] drm/bridge: Add the necessary bits to support bus format negotiation
@ 2020-01-06 14:34   ` Neil Armstrong
  0 siblings, 0 replies; 22+ messages in thread
From: Neil Armstrong @ 2020-01-06 14:34 UTC (permalink / raw)
  To: dri-devel
  Cc: Mark Rutland, Nikita Yushchenko, devicetree, Neil Armstrong,
	Andrey Smirnov, Jonas Karlman, Seung-Woo Kim, Jernej Skrabec,
	Rob Herring, Kyungmin Park, Boris Brezillon, Thierry Reding,
	Laurent Pinchart, kernel, Sam Ravnborg, Chris Healy

From: Boris Brezillon <boris.brezillon@collabora.com>

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>
---
Changes in v6:
* None

Changes in v5:
* None

Changes in v4:
* Enhance the doc
* Fix typos
* Rename some parameters/fields
* Reword the commit message

Changes in v3:
* Fix the commit message (Reported by Laurent)
* Document the fact that bus formats should not be directly modified by
  drivers (Suggested by Laurent)
* Document the fact that format order matters (Suggested by Laurent)
* Propagate bus flags by default
* Document the fact that drivers can tweak bus flags if needed
* Let ->atomic_get_{output,input}_bus_fmts() allocate the bus format
  array (Suggested by Laurent)
* Add a drm_atomic_helper_bridge_propagate_bus_fmt()
* Mandate that bridge drivers return accurate input_fmts even if they
  are known to be the first element in the bridge chain

Changes in v2:
* Rework things to support more complex use cases
---
 drivers/gpu/drm/drm_bridge.c | 267 ++++++++++++++++++++++++++++++++++-
 include/drm/drm_bridge.h     | 124 ++++++++++++++++
 2 files changed, 390 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 8e4b799150b0..37400607e9b7 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -671,13 +671,261 @@ static int drm_atomic_bridge_check(struct drm_bridge *bridge,
 	return 0;
 }
 
+/**
+ * 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);
+
+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 (WARN_ON(!cur_state))
+		return -EINVAL;
+
+	/*
+	 * 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;
+		}
+
+		cur_state->input_bus_cfg.format = MEDIA_BUS_FMT_FIXED;
+		cur_state->output_bus_cfg.format = out_bus_fmt;
+		return 0;
+	}
+
+	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 (WARN_ON(!last_bridge_state))
+		return -EINVAL;
+
+	if (last_bridge->funcs->atomic_get_output_bus_fmts) {
+		const struct drm_bridge_funcs *funcs = last_bridge->funcs;
+
+		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;
+
+	bridge_state = drm_atomic_get_new_bridge_state(state, bridge);
+	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);
+		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()
@@ -689,12 +937,29 @@ 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 = bridge->encoder;
 	struct drm_bridge *iter;
+	int ret;
+
+	ret = drm_atomic_bridge_chain_select_bus_fmts(bridge, crtc_state,
+						      conn_state);
+	if (ret)
+		return ret;
 
 	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_bridge.h b/include/drm/drm_bridge.h
index ae0595c70132..39a5363dac40 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -35,6 +35,38 @@ struct drm_bridge;
 struct drm_bridge_timings;
 struct drm_panel;
 
+/**
+ * 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 {
+	/**
+	 * @fmt: 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
  * @base: inherit from &drm_private_state
@@ -44,6 +76,16 @@ struct drm_bridge_state {
 	struct drm_private_state base;
 
 	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 *
@@ -387,6 +429,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:
 	 *
@@ -401,6 +509,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.
 	 */
@@ -588,6 +704,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);
+
 void __drm_atomic_helper_bridge_reset(struct drm_bridge *bridge,
 				      struct drm_bridge_state *state);
 void __drm_atomic_helper_bridge_duplicate_state(struct drm_bridge *bridge,
-- 
2.22.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 22+ messages in thread

* Re: [PATCH v6 0/4] drm: Add support for bus-format negotiation
  2020-01-06 14:34 ` Neil Armstrong
@ 2020-01-06 14:37   ` Boris Brezillon
  -1 siblings, 0 replies; 22+ messages in thread
From: Boris Brezillon @ 2020-01-06 14:37 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: dri-devel, Mark Rutland, Thierry Reding, Laurent Pinchart,
	kernel, Sam Ravnborg, Nikita Yushchenko, Andrey Smirnov,
	Kyungmin Park, Chris Healy, devicetree, Jonas Karlman,
	Rob Herring, Jernej Skrabec, Seung-Woo Kim

On Mon,  6 Jan 2020 15:34:05 +0100
Neil Armstrong <narmstrong@baylibre.com> wrote:

> 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 Major changes in this v6, I addressed the slight changed requested
> by Boris from Laurent on the patch 1, and rebased on drm-misc-next. 
> Note that this version only contains core changes.
> Once those changes are merged I'll send the imx/panel/lvds-codec specific bits.
> 
> A more detailed changelog is provided in each patch.
> 
> This patch series is also available here [1].
> 
> Thanks,
> 
> ~Boris~ Neil
> 
> [1] https://github.com/superna9999/linux/commits/drm-bridge-busfmt-v6
> 
> Boris Brezillon (4):
>   drm/bridge: Add a drm_bridge_state object
>   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

This version looks good to me.

Acked-by: Boris Brezillon <boris.brezillon@collabora.com>

> 
>  .../drm/bridge/analogix/analogix_dp_core.c    |  41 +-
>  drivers/gpu/drm/drm_atomic.c                  |  39 ++
>  drivers/gpu/drm/drm_atomic_helper.c           |  32 +-
>  drivers/gpu/drm/drm_bridge.c                  | 527 +++++++++++++++++-
>  drivers/gpu/drm/rcar-du/rcar_lvds.c           |   5 +-
>  include/drm/drm_atomic.h                      |   3 +
>  include/drm/drm_bridge.h                      | 275 ++++++++-
>  7 files changed, 874 insertions(+), 48 deletions(-)
> 


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v6 0/4] drm: Add support for bus-format negotiation
@ 2020-01-06 14:37   ` Boris Brezillon
  0 siblings, 0 replies; 22+ messages in thread
From: Boris Brezillon @ 2020-01-06 14:37 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: Mark Rutland, Nikita Yushchenko, Jernej Skrabec, devicetree,
	Andrey Smirnov, Jonas Karlman, Seung-Woo Kim, dri-devel,
	Rob Herring, Kyungmin Park, Thierry Reding, Laurent Pinchart,
	kernel, Sam Ravnborg, Chris Healy

On Mon,  6 Jan 2020 15:34:05 +0100
Neil Armstrong <narmstrong@baylibre.com> wrote:

> 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 Major changes in this v6, I addressed the slight changed requested
> by Boris from Laurent on the patch 1, and rebased on drm-misc-next. 
> Note that this version only contains core changes.
> Once those changes are merged I'll send the imx/panel/lvds-codec specific bits.
> 
> A more detailed changelog is provided in each patch.
> 
> This patch series is also available here [1].
> 
> Thanks,
> 
> ~Boris~ Neil
> 
> [1] https://github.com/superna9999/linux/commits/drm-bridge-busfmt-v6
> 
> Boris Brezillon (4):
>   drm/bridge: Add a drm_bridge_state object
>   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

This version looks good to me.

Acked-by: Boris Brezillon <boris.brezillon@collabora.com>

> 
>  .../drm/bridge/analogix/analogix_dp_core.c    |  41 +-
>  drivers/gpu/drm/drm_atomic.c                  |  39 ++
>  drivers/gpu/drm/drm_atomic_helper.c           |  32 +-
>  drivers/gpu/drm/drm_bridge.c                  | 527 +++++++++++++++++-
>  drivers/gpu/drm/rcar-du/rcar_lvds.c           |   5 +-
>  include/drm/drm_atomic.h                      |   3 +
>  include/drm/drm_bridge.h                      | 275 ++++++++-
>  7 files changed, 874 insertions(+), 48 deletions(-)
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v6 2/4] drm/bridge: Patch atomic hooks to take a drm_bridge_state
  2020-01-06 14:34   ` Neil Armstrong
@ 2020-01-06 14:40     ` Boris Brezillon
  -1 siblings, 0 replies; 22+ messages in thread
From: Boris Brezillon @ 2020-01-06 14:40 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: dri-devel, Mark Rutland, Thierry Reding, Laurent Pinchart,
	kernel, Sam Ravnborg, Nikita Yushchenko, Andrey Smirnov,
	Kyungmin Park, Chris Healy, devicetree, Jonas Karlman,
	Rob Herring, Jernej Skrabec, Seung-Woo Kim

On Mon,  6 Jan 2020 15:34:07 +0100
Neil Armstrong <narmstrong@baylibre.com> wrote:

> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index 8ffa4fbbdeb3..b8b22dc55bdb 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 *state)

Just a nit: maybe you should name that one old_bridge_state for
consistency.

>  {
>  	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
>  

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v6 2/4] drm/bridge: Patch atomic hooks to take a drm_bridge_state
@ 2020-01-06 14:40     ` Boris Brezillon
  0 siblings, 0 replies; 22+ messages in thread
From: Boris Brezillon @ 2020-01-06 14:40 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: Mark Rutland, Nikita Yushchenko, Jernej Skrabec, devicetree,
	Andrey Smirnov, Jonas Karlman, Seung-Woo Kim, dri-devel,
	Rob Herring, Kyungmin Park, Thierry Reding, Laurent Pinchart,
	kernel, Sam Ravnborg, Chris Healy

On Mon,  6 Jan 2020 15:34:07 +0100
Neil Armstrong <narmstrong@baylibre.com> wrote:

> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index 8ffa4fbbdeb3..b8b22dc55bdb 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 *state)

Just a nit: maybe you should name that one old_bridge_state for
consistency.

>  {
>  	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
>  
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v6 2/4] drm/bridge: Patch atomic hooks to take a drm_bridge_state
  2020-01-06 14:40     ` Boris Brezillon
@ 2020-01-06 14:46       ` Neil Armstrong
  -1 siblings, 0 replies; 22+ messages in thread
From: Neil Armstrong @ 2020-01-06 14:46 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: dri-devel, Mark Rutland, Thierry Reding, Laurent Pinchart,
	kernel, Sam Ravnborg, Nikita Yushchenko, Andrey Smirnov,
	Kyungmin Park, Chris Healy, devicetree, Jonas Karlman,
	Rob Herring, Jernej Skrabec, Seung-Woo Kim

On 06/01/2020 15:40, Boris Brezillon wrote:
> On Mon,  6 Jan 2020 15:34:07 +0100
> Neil Armstrong <narmstrong@baylibre.com> wrote:
> 
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
>> index 8ffa4fbbdeb3..b8b22dc55bdb 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 *state)
> 
> Just a nit: maybe you should name that one old_bridge_state for
> consistency.
> 
>>  {
>>  	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
>>  

Sure, will update while applying.

Neil

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v6 2/4] drm/bridge: Patch atomic hooks to take a drm_bridge_state
@ 2020-01-06 14:46       ` Neil Armstrong
  0 siblings, 0 replies; 22+ messages in thread
From: Neil Armstrong @ 2020-01-06 14:46 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: Mark Rutland, Nikita Yushchenko, Jernej Skrabec, devicetree,
	Andrey Smirnov, Jonas Karlman, Seung-Woo Kim, dri-devel,
	Rob Herring, Kyungmin Park, Thierry Reding, Laurent Pinchart,
	kernel, Sam Ravnborg, Chris Healy

On 06/01/2020 15:40, Boris Brezillon wrote:
> On Mon,  6 Jan 2020 15:34:07 +0100
> Neil Armstrong <narmstrong@baylibre.com> wrote:
> 
>> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
>> index 8ffa4fbbdeb3..b8b22dc55bdb 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 *state)
> 
> Just a nit: maybe you should name that one old_bridge_state for
> consistency.
> 
>>  {
>>  	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
>>  

Sure, will update while applying.

Neil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v6 4/4] drm/bridge: Add the necessary bits to support bus format negotiation
  2020-01-06 14:34   ` Neil Armstrong
@ 2020-01-06 22:30     ` Jonas Karlman
  -1 siblings, 0 replies; 22+ messages in thread
From: Jonas Karlman @ 2020-01-06 22:30 UTC (permalink / raw)
  To: Neil Armstrong, dri-devel
  Cc: Boris Brezillon, Mark Rutland, Thierry Reding, Laurent Pinchart,
	kernel, Sam Ravnborg, Nikita Yushchenko, Andrey Smirnov,
	Kyungmin Park, Chris Healy, devicetree, Rob Herring,
	Jernej Skrabec, Seung-Woo Kim

On 2020-01-06 15:34, Neil Armstrong wrote:
> From: Boris Brezillon <boris.brezillon@collabora.com>
>
> 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).

I have tested this series on a Rockchip RK3328 Rock64 device along with early work
on rockchip dw-hdmi bus format negotiation at [1]. All output modes supported
on RK3328 works (RGB444, YUV420/444, 8/10-bit).

So for this entire series:

Tested-by: Jonas Karlman <jonas@kwiboo.se>

[1] https://github.com/Kwiboo/linux-rockchip/commits/next-20200106-bus-format

Regards,
Jonas

>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
> Changes in v6:
> * None
>
> Changes in v5:
> * None
>
> Changes in v4:
> * Enhance the doc
> * Fix typos
> * Rename some parameters/fields
> * Reword the commit message
>
> Changes in v3:
> * Fix the commit message (Reported by Laurent)
> * Document the fact that bus formats should not be directly modified by
>   drivers (Suggested by Laurent)
> * Document the fact that format order matters (Suggested by Laurent)
> * Propagate bus flags by default
> * Document the fact that drivers can tweak bus flags if needed
> * Let ->atomic_get_{output,input}_bus_fmts() allocate the bus format
>   array (Suggested by Laurent)
> * Add a drm_atomic_helper_bridge_propagate_bus_fmt()
> * Mandate that bridge drivers return accurate input_fmts even if they
>   are known to be the first element in the bridge chain
>
> Changes in v2:
> * Rework things to support more complex use cases
> ---
>  drivers/gpu/drm/drm_bridge.c | 267 ++++++++++++++++++++++++++++++++++-
>  include/drm/drm_bridge.h     | 124 ++++++++++++++++
>  2 files changed, 390 insertions(+), 1 deletion(-)
>
> (...)


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v6 4/4] drm/bridge: Add the necessary bits to support bus format negotiation
@ 2020-01-06 22:30     ` Jonas Karlman
  0 siblings, 0 replies; 22+ messages in thread
From: Jonas Karlman @ 2020-01-06 22:30 UTC (permalink / raw)
  To: Neil Armstrong, dri-devel
  Cc: Mark Rutland, Nikita Yushchenko, Jernej Skrabec, devicetree,
	Andrey Smirnov, Seung-Woo Kim, Rob Herring, Kyungmin Park,
	Boris Brezillon, Thierry Reding, Laurent Pinchart, kernel,
	Sam Ravnborg, Chris Healy

On 2020-01-06 15:34, Neil Armstrong wrote:
> From: Boris Brezillon <boris.brezillon@collabora.com>
>
> 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).

I have tested this series on a Rockchip RK3328 Rock64 device along with early work
on rockchip dw-hdmi bus format negotiation at [1]. All output modes supported
on RK3328 works (RGB444, YUV420/444, 8/10-bit).

So for this entire series:

Tested-by: Jonas Karlman <jonas@kwiboo.se>

[1] https://github.com/Kwiboo/linux-rockchip/commits/next-20200106-bus-format

Regards,
Jonas

>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
> Changes in v6:
> * None
>
> Changes in v5:
> * None
>
> Changes in v4:
> * Enhance the doc
> * Fix typos
> * Rename some parameters/fields
> * Reword the commit message
>
> Changes in v3:
> * Fix the commit message (Reported by Laurent)
> * Document the fact that bus formats should not be directly modified by
>   drivers (Suggested by Laurent)
> * Document the fact that format order matters (Suggested by Laurent)
> * Propagate bus flags by default
> * Document the fact that drivers can tweak bus flags if needed
> * Let ->atomic_get_{output,input}_bus_fmts() allocate the bus format
>   array (Suggested by Laurent)
> * Add a drm_atomic_helper_bridge_propagate_bus_fmt()
> * Mandate that bridge drivers return accurate input_fmts even if they
>   are known to be the first element in the bridge chain
>
> Changes in v2:
> * Rework things to support more complex use cases
> ---
>  drivers/gpu/drm/drm_bridge.c | 267 ++++++++++++++++++++++++++++++++++-
>  include/drm/drm_bridge.h     | 124 ++++++++++++++++
>  2 files changed, 390 insertions(+), 1 deletion(-)
>
> (...)

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v6 0/4] drm: Add support for bus-format negotiation
  2020-01-06 14:34 ` Neil Armstrong
@ 2020-01-07  9:24   ` Neil Armstrong
  -1 siblings, 0 replies; 22+ messages in thread
From: Neil Armstrong @ 2020-01-07  9:24 UTC (permalink / raw)
  To: dri-devel
  Cc: Mark Rutland, Thierry Reding, Laurent Pinchart, kernel,
	Sam Ravnborg, Nikita Yushchenko, Andrey Smirnov, Kyungmin Park,
	Chris Healy, devicetree, Jonas Karlman, Rob Herring,
	Jernej Skrabec, Seung-Woo Kim, Boris Brezillon

On 06/01/2020 15:34, Neil Armstrong wrote:
> 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 Major changes in this v6, I addressed the slight changed requested
> by Boris from Laurent on the patch 1, and rebased on drm-misc-next. 
> Note that this version only contains core changes.
> Once those changes are merged I'll send the imx/panel/lvds-codec specific bits.
> 
> A more detailed changelog is provided in each patch.
> 
> This patch series is also available here [1].
> 
> Thanks,
> 
> ~Boris~ Neil
> 
> [1] https://github.com/superna9999/linux/commits/drm-bridge-busfmt-v6
> 
> Boris Brezillon (4):
>   drm/bridge: Add a drm_bridge_state object
>   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/bridge/analogix/analogix_dp_core.c    |  41 +-
>  drivers/gpu/drm/drm_atomic.c                  |  39 ++
>  drivers/gpu/drm/drm_atomic_helper.c           |  32 +-
>  drivers/gpu/drm/drm_bridge.c                  | 527 +++++++++++++++++-
>  drivers/gpu/drm/rcar-du/rcar_lvds.c           |   5 +-
>  include/drm/drm_atomic.h                      |   3 +
>  include/drm/drm_bridge.h                      | 275 ++++++++-
>  7 files changed, 874 insertions(+), 48 deletions(-)
> 

Applied to drm-misc-next with slight change in patch 1 and doc fix in patch 4

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v6 0/4] drm: Add support for bus-format negotiation
@ 2020-01-07  9:24   ` Neil Armstrong
  0 siblings, 0 replies; 22+ messages in thread
From: Neil Armstrong @ 2020-01-07  9:24 UTC (permalink / raw)
  To: dri-devel
  Cc: Mark Rutland, Nikita Yushchenko, Jernej Skrabec, devicetree,
	Andrey Smirnov, Jonas Karlman, Seung-Woo Kim, Rob Herring,
	Kyungmin Park, Thierry Reding, Laurent Pinchart, Boris Brezillon,
	kernel, Sam Ravnborg, Chris Healy

On 06/01/2020 15:34, Neil Armstrong wrote:
> 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 Major changes in this v6, I addressed the slight changed requested
> by Boris from Laurent on the patch 1, and rebased on drm-misc-next. 
> Note that this version only contains core changes.
> Once those changes are merged I'll send the imx/panel/lvds-codec specific bits.
> 
> A more detailed changelog is provided in each patch.
> 
> This patch series is also available here [1].
> 
> Thanks,
> 
> ~Boris~ Neil
> 
> [1] https://github.com/superna9999/linux/commits/drm-bridge-busfmt-v6
> 
> Boris Brezillon (4):
>   drm/bridge: Add a drm_bridge_state object
>   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/bridge/analogix/analogix_dp_core.c    |  41 +-
>  drivers/gpu/drm/drm_atomic.c                  |  39 ++
>  drivers/gpu/drm/drm_atomic_helper.c           |  32 +-
>  drivers/gpu/drm/drm_bridge.c                  | 527 +++++++++++++++++-
>  drivers/gpu/drm/rcar-du/rcar_lvds.c           |   5 +-
>  include/drm/drm_atomic.h                      |   3 +
>  include/drm/drm_bridge.h                      | 275 ++++++++-
>  7 files changed, 874 insertions(+), 48 deletions(-)
> 

Applied to drm-misc-next with slight change in patch 1 and doc fix in patch 4
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v6 1/4] drm/bridge: Add a drm_bridge_state object
  2020-01-06 14:34   ` Neil Armstrong
@ 2020-01-07 14:46     ` Imre Deak
  -1 siblings, 0 replies; 22+ messages in thread
From: Imre Deak @ 2020-01-07 14:46 UTC (permalink / raw)
  To: Neil Armstrong, Boris Brezillon
  Cc: dri-devel, Mark Rutland, Nikita Yushchenko, devicetree,
	Andrey Smirnov, Jonas Karlman, Seung-Woo Kim, Jernej Skrabec,
	Rob Herring, Kyungmin Park, Boris Brezillon, Thierry Reding,
	Laurent Pinchart, kernel, Sam Ravnborg, Chris Healy

Hi,

On Mon, Jan 06, 2020 at 03:34:06PM +0100, Neil Armstrong wrote:
> From: Boris Brezillon <boris.brezillon@collabora.com>
> 
> 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>
> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
> [...]
> +
> +/**
> + * __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);

When DRM and DRM_KMS_HELPER are built as modules the above will cause the
following build error:

depmod: ERROR: Cycle detected: drm_kms_helper -> drm -> drm_kms_helper
depmod: ERROR: Found 2 modules in dependency cycles!

Using __drm_atomic_helper_private_obj_duplicate_state will add the
drm.ko->drm_kms_helper.ko dependency but drm_kms_helper.ko already
depends on drm.ko

--Imre

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v6 1/4] drm/bridge: Add a drm_bridge_state object
@ 2020-01-07 14:46     ` Imre Deak
  0 siblings, 0 replies; 22+ messages in thread
From: Imre Deak @ 2020-01-07 14:46 UTC (permalink / raw)
  To: Neil Armstrong, Boris Brezillon
  Cc: Mark Rutland, Nikita Yushchenko, Jernej Skrabec, Jonas Karlman,
	Andrey Smirnov, Seung-Woo Kim, Kyungmin Park, dri-devel,
	devicetree, Rob Herring, Thierry Reding, Laurent Pinchart,
	Boris Brezillon, kernel, Sam Ravnborg, Chris Healy

Hi,

On Mon, Jan 06, 2020 at 03:34:06PM +0100, Neil Armstrong wrote:
> From: Boris Brezillon <boris.brezillon@collabora.com>
> 
> 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>
> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
> [...]
> +
> +/**
> + * __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);

When DRM and DRM_KMS_HELPER are built as modules the above will cause the
following build error:

depmod: ERROR: Cycle detected: drm_kms_helper -> drm -> drm_kms_helper
depmod: ERROR: Found 2 modules in dependency cycles!

Using __drm_atomic_helper_private_obj_duplicate_state will add the
drm.ko->drm_kms_helper.ko dependency but drm_kms_helper.ko already
depends on drm.ko

--Imre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2020-01-07 14:46 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-06 14:34 [PATCH v6 0/4] drm: Add support for bus-format negotiation Neil Armstrong
2020-01-06 14:34 ` Neil Armstrong
2020-01-06 14:34 ` [PATCH v6 1/4] drm/bridge: Add a drm_bridge_state object Neil Armstrong
2020-01-06 14:34   ` Neil Armstrong
2020-01-07 14:46   ` Imre Deak
2020-01-07 14:46     ` Imre Deak
2020-01-06 14:34 ` [PATCH v6 2/4] drm/bridge: Patch atomic hooks to take a drm_bridge_state Neil Armstrong
2020-01-06 14:34   ` Neil Armstrong
2020-01-06 14:40   ` Boris Brezillon
2020-01-06 14:40     ` Boris Brezillon
2020-01-06 14:46     ` Neil Armstrong
2020-01-06 14:46       ` Neil Armstrong
2020-01-06 14:34 ` [PATCH v6 3/4] drm/bridge: Add an ->atomic_check() hook Neil Armstrong
2020-01-06 14:34   ` Neil Armstrong
2020-01-06 14:34 ` [PATCH v6 4/4] drm/bridge: Add the necessary bits to support bus format negotiation Neil Armstrong
2020-01-06 14:34   ` Neil Armstrong
2020-01-06 22:30   ` Jonas Karlman
2020-01-06 22:30     ` Jonas Karlman
2020-01-06 14:37 ` [PATCH v6 0/4] drm: Add support for bus-format negotiation Boris Brezillon
2020-01-06 14:37   ` Boris Brezillon
2020-01-07  9:24 ` Neil Armstrong
2020-01-07  9:24   ` Neil Armstrong

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.