PATCH [v2 0/7] drm/bridge: Drop deprecated functions

PATCH [v2 0/7] drm/bridge: Drop deprecated functions

[Sam Ravnborg]

Over time we have accumulated some deprecated functions etc. in drm_bridge.
This patch-set starts to move over to the atomic variants and deletes what
is not used anymore.

There was only one user of the non-atomic drm_bridge_chain functions in
parade-ps8640 - migrate it to the atomic variants and delete the non-atomic
drm_bridge_chain functions.

There was only one user of drm_bridge_chain_mode_fixup in mediatk. The use
in the mediatek driver was wrong and with the single user gone we could also
delete this function.

Added a few todo items.

Next step is to migrate the easy bridge drivers to use the atomic variants
of drm_bridge_funcs operations. The easy ones are the drivers wihtout
mode_set or mode_fixup.
I have something typed up already, but wanted feedback on this patchset before
sending out additional patches.

v2:
  v2 have been long in the coming as I wanted to wait until I
  started to have some spare time for linux stuff again, and thus time to
  address any comments timely.
  - Added Maxime's r-b (from old thread, so using old mail address [1])
  - Fixed several small issues in patch 3 that introduces a new helper
  - Added a few more folks on cc in hope to see some testing


	Sam


[1] https://lore.kernel.org/dri-devel/20210722062246.2512666-1-sam@ravnborg.org/

Sam Ravnborg (7):
      drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs
      drm/bridge: Drop unused drm_bridge_chain functions
      drm/bridge: Add drm_atomic_get_new_crtc_for_bridge() helper
      drm/bridge: lontium-lt9611: Use atomic variants of drm_bridge_funcs
      drm/mediatek: Drop chain_mode_fixup call in mode_valid()
      drm/bridge: Drop drm_bridge_chain_mode_fixup
      drm/todo: Add bridge related todo items

 Documentation/gpu/todo.rst              |  49 +++++++++++
 drivers/gpu/drm/bridge/lontium-lt9611.c |  75 +++++++---------
 drivers/gpu/drm/bridge/parade-ps8640.c  |  14 +--
 drivers/gpu/drm/drm_atomic.c            |  42 +++++++++
 drivers/gpu/drm/drm_bridge.c            | 147 --------------------------------
 drivers/gpu/drm/mediatek/mtk_hdmi.c     |  11 ---
 include/drm/drm_atomic.h                |   3 +
 include/drm/drm_bridge.h                |  31 -------
 8 files changed, 135 insertions(+), 237 deletions(-)



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/7] drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs

[Sam Ravnborg]

The atomic variants of enable/disable in drm_bridge_funcs are the
preferred operations - introduce these.

The ps8640 driver used the non-atomic variants of the drm_bridge_chain_pre_enable/
drm_bridge_chain_post_disable - convert these to the atomic variants.

v2:
  - Added a few more people to cc: (Jitao, Enric, Philip) to increase
    possibility to get test feedback

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Reviewed-by: Maxime Ripard <maxime@cerno.tech>
Cc: Jitao Shi <jitao.shi@mediatek.com>
Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Cc: Philip Chen <philipchen@chromium.org>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/gpu/drm/bridge/parade-ps8640.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 3aaa90913bf8..0b620afe99c0 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -376,7 +376,8 @@ static void ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
 	ps_bridge->powered = false;
 }
 
-static void ps8640_pre_enable(struct drm_bridge *bridge)
+static void ps8640_atomic_pre_enable(struct drm_bridge *bridge,
+				     struct drm_bridge_state *old_bridge_state)
 {
 	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
 	int ret;
@@ -388,7 +389,8 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
 		ps8640_bridge_poweroff(ps_bridge);
 }
 
-static void ps8640_post_disable(struct drm_bridge *bridge)
+static void ps8640_atomic_post_disable(struct drm_bridge *bridge,
+				       struct drm_bridge_state *old_bridge_state)
 {
 	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
 
@@ -489,7 +491,7 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
 	 * EDID, for this chip, we need to do a full poweron, otherwise it will
 	 * fail.
 	 */
-	drm_bridge_chain_pre_enable(bridge);
+	drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
 
 	edid = drm_get_edid(connector,
 			    ps_bridge->page[PAGE0_DP_CNTL]->adapter);
@@ -499,7 +501,7 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
 	 * before, return the chip to its original power state.
 	 */
 	if (poweroff)
-		drm_bridge_chain_post_disable(bridge);
+		drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
 
 	return edid;
 }
@@ -508,8 +510,8 @@ static const struct drm_bridge_funcs ps8640_bridge_funcs = {
 	.attach = ps8640_bridge_attach,
 	.detach = ps8640_bridge_detach,
 	.get_edid = ps8640_bridge_get_edid,
-	.post_disable = ps8640_post_disable,
-	.pre_enable = ps8640_pre_enable,
+	.atomic_post_disable = ps8640_atomic_post_disable,
+	.atomic_pre_enable = ps8640_atomic_pre_enable,
 };
 
 static int ps8640_probe(struct i2c_client *client)
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/7] drm/bridge: Drop unused drm_bridge_chain functions

[Sam Ravnborg]

The drm_bridge_chain_{pre_enable,enable,disable,post_disable} has no
users left and we have atomic variants that should be used.
Drop them so they do not gain new users.

Adjust a few comments to avoid references to the dropped functions.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Reviewed-by: Maxime Ripard <maxime@cerno.tech>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 drivers/gpu/drm/drm_bridge.c | 110 -----------------------------------
 include/drm/drm_bridge.h     |  28 ---------
 2 files changed, 138 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index c96847fc0ebc..7a57d6816105 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -485,61 +485,6 @@ drm_bridge_chain_mode_valid(struct drm_bridge *bridge,
 }
 EXPORT_SYMBOL(drm_bridge_chain_mode_valid);
 
-/**
- * drm_bridge_chain_disable - disables all bridges in the encoder chain
- * @bridge: bridge control structure
- *
- * Calls &drm_bridge_funcs.disable op for all the bridges in the encoder
- * chain, starting from the last bridge to the first. These are called before
- * calling the encoder's prepare op.
- *
- * Note: the bridge passed should be the one closest to the encoder
- */
-void drm_bridge_chain_disable(struct drm_bridge *bridge)
-{
-	struct drm_encoder *encoder;
-	struct drm_bridge *iter;
-
-	if (!bridge)
-		return;
-
-	encoder = bridge->encoder;
-	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
-		if (iter->funcs->disable)
-			iter->funcs->disable(iter);
-
-		if (iter == bridge)
-			break;
-	}
-}
-EXPORT_SYMBOL(drm_bridge_chain_disable);
-
-/**
- * drm_bridge_chain_post_disable - cleans up after disabling all bridges in the
- *				   encoder chain
- * @bridge: bridge control structure
- *
- * Calls &drm_bridge_funcs.post_disable op for all the bridges in the
- * encoder chain, starting from the first bridge to the last. These are called
- * after completing the encoder's prepare op.
- *
- * Note: the bridge passed should be the one closest to the encoder
- */
-void drm_bridge_chain_post_disable(struct drm_bridge *bridge)
-{
-	struct drm_encoder *encoder;
-
-	if (!bridge)
-		return;
-
-	encoder = bridge->encoder;
-	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
-		if (bridge->funcs->post_disable)
-			bridge->funcs->post_disable(bridge);
-	}
-}
-EXPORT_SYMBOL(drm_bridge_chain_post_disable);
-
 /**
  * drm_bridge_chain_mode_set - set proposed mode for all bridges in the
  *			       encoder chain
@@ -569,61 +514,6 @@ void drm_bridge_chain_mode_set(struct drm_bridge *bridge,
 }
 EXPORT_SYMBOL(drm_bridge_chain_mode_set);
 
-/**
- * drm_bridge_chain_pre_enable - prepares for enabling all bridges in the
- *				 encoder chain
- * @bridge: bridge control structure
- *
- * Calls &drm_bridge_funcs.pre_enable op for all the bridges in the encoder
- * chain, starting from the last bridge to the first. These are called
- * before calling the encoder's commit op.
- *
- * Note: the bridge passed should be the one closest to the encoder
- */
-void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
-{
-	struct drm_encoder *encoder;
-	struct drm_bridge *iter;
-
-	if (!bridge)
-		return;
-
-	encoder = bridge->encoder;
-	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
-		if (iter->funcs->pre_enable)
-			iter->funcs->pre_enable(iter);
-
-		if (iter == bridge)
-			break;
-	}
-}
-EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
-
-/**
- * drm_bridge_chain_enable - enables all bridges in the encoder chain
- * @bridge: bridge control structure
- *
- * Calls &drm_bridge_funcs.enable op for all the bridges in the encoder
- * chain, starting from the first bridge to the last. These are called
- * after completing the encoder's commit op.
- *
- * Note that the bridge passed should be the one closest to the encoder
- */
-void drm_bridge_chain_enable(struct drm_bridge *bridge)
-{
-	struct drm_encoder *encoder;
-
-	if (!bridge)
-		return;
-
-	encoder = bridge->encoder;
-	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
-		if (bridge->funcs->enable)
-			bridge->funcs->enable(bridge);
-	}
-}
-EXPORT_SYMBOL(drm_bridge_chain_enable);
-
 /**
  * drm_atomic_bridge_chain_disable - disables all bridges in the encoder chain
  * @bridge: bridge control structure
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 061d87313fac..f1eb71ff5379 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -297,12 +297,6 @@ struct drm_bridge_funcs {
 	 * not enable the display link feeding the next bridge in the chain (if
 	 * there is one) when this callback is called.
 	 *
-	 * Note that this function will only be invoked in the context of an
-	 * atomic commit. It will not be invoked from
-	 * &drm_bridge_chain_pre_enable. It would be prudent to also provide an
-	 * implementation of @pre_enable if you are expecting driver calls into
-	 * &drm_bridge_chain_pre_enable.
-	 *
 	 * The @atomic_pre_enable callback is optional.
 	 */
 	void (*atomic_pre_enable)(struct drm_bridge *bridge,
@@ -323,11 +317,6 @@ struct drm_bridge_funcs {
 	 * callback must enable the display link feeding the next bridge in the
 	 * chain if there is one.
 	 *
-	 * Note that this function will only be invoked in the context of an
-	 * atomic commit. It will not be invoked from &drm_bridge_chain_enable.
-	 * It would be prudent to also provide an implementation of @enable if
-	 * you are expecting driver calls into &drm_bridge_chain_enable.
-	 *
 	 * The @atomic_enable callback is optional.
 	 */
 	void (*atomic_enable)(struct drm_bridge *bridge,
@@ -345,12 +334,6 @@ struct drm_bridge_funcs {
 	 * The bridge can assume that the display pipe (i.e. clocks and timing
 	 * signals) feeding it is still running when this callback is called.
 	 *
-	 * Note that this function will only be invoked in the context of an
-	 * atomic commit. It will not be invoked from
-	 * &drm_bridge_chain_disable. It would be prudent to also provide an
-	 * implementation of @disable if you are expecting driver calls into
-	 * &drm_bridge_chain_disable.
-	 *
 	 * The @atomic_disable callback is optional.
 	 */
 	void (*atomic_disable)(struct drm_bridge *bridge,
@@ -370,13 +353,6 @@ struct drm_bridge_funcs {
 	 * signals) feeding it is no longer running when this callback is
 	 * called.
 	 *
-	 * Note that this function will only be invoked in the context of an
-	 * atomic commit. It will not be invoked from
-	 * &drm_bridge_chain_post_disable.
-	 * It would be prudent to also provide an implementation of
-	 * @post_disable if you are expecting driver calls into
-	 * &drm_bridge_chain_post_disable.
-	 *
 	 * The @atomic_post_disable callback is optional.
 	 */
 	void (*atomic_post_disable)(struct drm_bridge *bridge,
@@ -868,13 +844,9 @@ enum drm_mode_status
 drm_bridge_chain_mode_valid(struct drm_bridge *bridge,
 			    const struct drm_display_info *info,
 			    const struct drm_display_mode *mode);
-void drm_bridge_chain_disable(struct drm_bridge *bridge);
-void drm_bridge_chain_post_disable(struct drm_bridge *bridge);
 void drm_bridge_chain_mode_set(struct drm_bridge *bridge,
 			       const struct drm_display_mode *mode,
 			       const struct drm_display_mode *adjusted_mode);
-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,
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/7] drm/bridge: Add drm_atomic_get_new_crtc_for_bridge() helper

[Sam Ravnborg]

drm_atomic_get_new_crtc_for_bridge() will be used by bridge
drivers to provide easy access to the mode from the
drm_bridge_funcs operations.

The helper will be useful in the conversion to the atomic
operations of struct drm_bridge_funcs.

v2:
  - Renamed to drm_atomic_get_new_crtc_for_bridge (Maxime)
  - Use atomic_state, not bridge_State (Maxime)
  - Drop WARN on crtc_state as it is a valid case (Maxime)
  - Added small code snip to help readers
  - Updated description, fixed kernel-doc and exported the symbol

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 drivers/gpu/drm/drm_atomic.c | 42 ++++++++++++++++++++++++++++++++++++
 include/drm/drm_atomic.h     |  3 +++
 2 files changed, 45 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index ff1416cd609a..8b107194b157 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1134,6 +1134,48 @@ drm_atomic_get_new_bridge_state(struct drm_atomic_state *state,
 }
 EXPORT_SYMBOL(drm_atomic_get_new_bridge_state);
 
+/**
+ * drm_atomic_get_new_crtc_for_bridge - get new crtc_state for the bridge
+ * @state: state of the bridge
+ * @bridge: bridge object
+ *
+ * This function is often used in the &struct drm_bridge_funcs operations
+ * to provide easy access to the mode like this:
+ *
+ * .. code-block:: c
+ *
+ *	crtc_state = drm_atomic_get_new_crtc_for_bridge(old_bridge_state->base.state,
+ *							bridge);
+ *	if (crtc_state) {
+ *		mode = &crtc_state->mode;
+ *		...
+ *
+ * If no connector can be looked up or if no connector state is available
+ * then this will be logged using WARN().
+ *
+ * Returns:
+ * The &struct drm_crtc_state for the given bridge/state, or NULL
+ * if no crtc_state could be looked up.
+ */
+const struct drm_crtc_state *
+drm_atomic_get_new_crtc_for_bridge(struct drm_atomic_state *state,
+				   struct drm_bridge *bridge)
+{
+	const struct drm_connector_state *conn_state;
+	struct drm_connector *connector;
+
+	connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
+	if (WARN_ON(!connector))
+		return NULL;
+
+	conn_state = drm_atomic_get_new_connector_state(state, connector);
+	if (WARN_ON(!conn_state || !conn_state->crtc))
+		return NULL;
+
+	return drm_atomic_get_new_crtc_state(state, conn_state->crtc);
+}
+EXPORT_SYMBOL(drm_atomic_get_new_crtc_for_bridge);
+
 /**
  * drm_atomic_add_encoder_bridges - add bridges attached to an encoder
  * @state: atomic state
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 1701c2128a5c..f861d73296cc 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -1119,5 +1119,8 @@ drm_atomic_get_old_bridge_state(struct drm_atomic_state *state,
 struct drm_bridge_state *
 drm_atomic_get_new_bridge_state(struct drm_atomic_state *state,
 				struct drm_bridge *bridge);
+const struct drm_crtc_state *
+drm_atomic_get_new_crtc_for_bridge(struct drm_atomic_state *state,
+				   struct drm_bridge *bridge);
 
 #endif /* DRM_ATOMIC_H_ */
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/7] drm/bridge: lontium-lt9611: Use atomic variants of drm_bridge_funcs

[Sam Ravnborg]

The atomic variants of enable/disable in drm_bridge_funcs are the
preferred operations - introduce these.

Use of mode_set is deprecated - merge the functionality with
atomic_enable()

v2:
  - Added check if crtc_state is NULL (Maxime)
  - Updated to use drm_atomic_get_new_crtc_for_bridge()

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Andrzej Hajda <a.hajda@samsung.com>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Robert Foss <robert.foss@linaro.org>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
---
 drivers/gpu/drm/bridge/lontium-lt9611.c | 75 +++++++++++--------------
 1 file changed, 33 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/bridge/lontium-lt9611.c b/drivers/gpu/drm/bridge/lontium-lt9611.c
index 29b1ce2140ab..cdc3d5f8dcac 100644
--- a/drivers/gpu/drm/bridge/lontium-lt9611.c
+++ b/drivers/gpu/drm/bridge/lontium-lt9611.c
@@ -700,9 +700,23 @@ lt9611_connector_mode_valid(struct drm_connector *connector,
 }
 
 /* bridge funcs */
-static void lt9611_bridge_enable(struct drm_bridge *bridge)
+static void lt9611_bridge_atomic_enable(struct drm_bridge *bridge,
+					struct drm_bridge_state *old_bridge_state)
 {
 	struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
+	const struct drm_display_mode *mode;
+	const struct drm_crtc_state *crtc_state;
+	struct hdmi_avi_infoframe avi_frame;
+	int ret;
+
+	crtc_state = drm_atomic_get_new_crtc_for_bridge(old_bridge_state->base.state,
+							bridge);
+	if (!crtc_state) {
+		dev_err(lt9611->dev, "no crtc_state available\n");
+		return;
+	}
+
+	mode = &crtc_state->mode;
 
 	if (lt9611_power_on(lt9611)) {
 		dev_err(lt9611->dev, "power on failed\n");
@@ -719,9 +733,21 @@ static void lt9611_bridge_enable(struct drm_bridge *bridge)
 
 	/* Enable HDMI output */
 	regmap_write(lt9611->regmap, 0x8130, 0xea);
+
+	lt9611_mipi_input_digital(lt9611, mode);
+	lt9611_pll_setup(lt9611, mode);
+	lt9611_mipi_video_setup(lt9611, mode);
+	lt9611_pcr_setup(lt9611, mode);
+
+	ret = drm_hdmi_avi_infoframe_from_display_mode(&avi_frame,
+						       &lt9611->connector,
+						       mode);
+	if (!ret)
+		lt9611->vic = avi_frame.video_code;
 }
 
-static void lt9611_bridge_disable(struct drm_bridge *bridge)
+static void lt9611_bridge_atomic_disable(struct drm_bridge *bridge,
+					 struct drm_bridge_state *old_bridge_state)
 {
 	struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
 	int ret;
@@ -877,48 +903,14 @@ static enum drm_mode_status lt9611_bridge_mode_valid(struct drm_bridge *bridge,
 		return MODE_OK;
 }
 
-static void lt9611_bridge_pre_enable(struct drm_bridge *bridge)
-{
-	struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
-
-	if (!lt9611->sleep)
-		return;
-
-	lt9611_reset(lt9611);
-	regmap_write(lt9611->regmap, 0x80ee, 0x01);
-
-	lt9611->sleep = false;
-}
-
-static void lt9611_bridge_post_disable(struct drm_bridge *bridge)
+static void lt9611_bridge_atomic_post_disable(struct drm_bridge *bridge,
+					      struct drm_bridge_state *old_bridge_state)
 {
 	struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
 
 	lt9611_sleep_setup(lt9611);
 }
 
-static void lt9611_bridge_mode_set(struct drm_bridge *bridge,
-				   const struct drm_display_mode *mode,
-				   const struct drm_display_mode *adj_mode)
-{
-	struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
-	struct hdmi_avi_infoframe avi_frame;
-	int ret;
-
-	lt9611_bridge_pre_enable(bridge);
-
-	lt9611_mipi_input_digital(lt9611, mode);
-	lt9611_pll_setup(lt9611, mode);
-	lt9611_mipi_video_setup(lt9611, mode);
-	lt9611_pcr_setup(lt9611, mode);
-
-	ret = drm_hdmi_avi_infoframe_from_display_mode(&avi_frame,
-						       &lt9611->connector,
-						       mode);
-	if (!ret)
-		lt9611->vic = avi_frame.video_code;
-}
-
 static enum drm_connector_status lt9611_bridge_detect(struct drm_bridge *bridge)
 {
 	struct lt9611 *lt9611 = bridge_to_lt9611(bridge);
@@ -954,10 +946,9 @@ static const struct drm_bridge_funcs lt9611_bridge_funcs = {
 	.attach = lt9611_bridge_attach,
 	.detach = lt9611_bridge_detach,
 	.mode_valid = lt9611_bridge_mode_valid,
-	.enable = lt9611_bridge_enable,
-	.disable = lt9611_bridge_disable,
-	.post_disable = lt9611_bridge_post_disable,
-	.mode_set = lt9611_bridge_mode_set,
+	.atomic_enable = lt9611_bridge_atomic_enable,
+	.atomic_disable = lt9611_bridge_atomic_disable,
+	.atomic_post_disable = lt9611_bridge_atomic_post_disable,
 	.detect = lt9611_bridge_detect,
 	.get_edid = lt9611_bridge_get_edid,
 	.hpd_enable = lt9611_bridge_hpd_enable,
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 5/7] drm/mediatek: Drop chain_mode_fixup call in mode_valid()

[Sam Ravnborg]

The mode_valid implementation had a call to
drm_bridge_chain_mode_fixup() which would be wrong as the mode_valid is
not allowed to change anything - only to validate the mode.

As the next bridge is often/always a connector the call had no effect
anyway. So drop it.

From the git history I could see this call was included in the original
version of the driver so there was no help there to find out why it was
added in the first place. But a lot has changed since the initial driver
were added and is seems safe to remove the call now.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Reviewed-by: Maxime Ripard <maxime@cerno.tech>
Cc: Chun-Kuang Hu <chunkuang.hu@kernel.org>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Matthias Brugger <matthias.bgg@gmail.com>
Cc: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Cc: linux-mediatek@lists.infradead.org
Cc: linux-arm-kernel@lists.infradead.org
---
 drivers/gpu/drm/mediatek/mtk_hdmi.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c
index 5838c44cbf6f..bade1cbd782d 100644
--- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
+++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
@@ -1208,22 +1208,11 @@ static int mtk_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
 				      const struct drm_display_mode *mode)
 {
 	struct mtk_hdmi *hdmi = hdmi_ctx_from_bridge(bridge);
-	struct drm_bridge *next_bridge;
 
 	dev_dbg(hdmi->dev, "xres=%d, yres=%d, refresh=%d, intl=%d clock=%d\n",
 		mode->hdisplay, mode->vdisplay, drm_mode_vrefresh(mode),
 		!!(mode->flags & DRM_MODE_FLAG_INTERLACE), mode->clock * 1000);
 
-	next_bridge = drm_bridge_get_next_bridge(&hdmi->bridge);
-	if (next_bridge) {
-		struct drm_display_mode adjusted_mode;
-
-		drm_mode_copy(&adjusted_mode, mode);
-		if (!drm_bridge_chain_mode_fixup(next_bridge, mode,
-						 &adjusted_mode))
-			return MODE_BAD;
-	}
-
 	if (hdmi->conf->cea_modes_only && !drm_match_cea_mode(mode))
 		return MODE_BAD;
 
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 6/7] drm/bridge: Drop drm_bridge_chain_mode_fixup

[Sam Ravnborg]

There are no users left of drm_bridge_chain_mode_fixup() and we
do not want to have this function available, so drop it.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Reviewed-by: Maxime Ripard <maxime@cerno.tech>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 drivers/gpu/drm/drm_bridge.c | 37 ------------------------------------
 include/drm/drm_bridge.h     |  3 ---
 2 files changed, 40 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 7a57d6816105..57a864d9a87f 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -406,43 +406,6 @@ void drm_bridge_detach(struct drm_bridge *bridge)
  *   needed, in order to gradually transition to the new model.
  */
 
-/**
- * drm_bridge_chain_mode_fixup - fixup proposed mode for all bridges in the
- *				 encoder chain
- * @bridge: bridge control structure
- * @mode: desired mode to be set for the bridge
- * @adjusted_mode: updated mode that works for this bridge
- *
- * Calls &drm_bridge_funcs.mode_fixup for all the bridges in the
- * encoder chain, starting from the first bridge to the last.
- *
- * Note: the bridge passed should be the one closest to the encoder
- *
- * RETURNS:
- * true on success, false on failure
- */
-bool drm_bridge_chain_mode_fixup(struct drm_bridge *bridge,
-				 const struct drm_display_mode *mode,
-				 struct drm_display_mode *adjusted_mode)
-{
-	struct drm_encoder *encoder;
-
-	if (!bridge)
-		return true;
-
-	encoder = bridge->encoder;
-	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
-		if (!bridge->funcs->mode_fixup)
-			continue;
-
-		if (!bridge->funcs->mode_fixup(bridge, mode, adjusted_mode))
-			return false;
-	}
-
-	return true;
-}
-EXPORT_SYMBOL(drm_bridge_chain_mode_fixup);
-
 /**
  * drm_bridge_chain_mode_valid - validate the mode against all bridges in the
  *				 encoder chain.
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index f1eb71ff5379..c8d07bd27f63 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -837,9 +837,6 @@ drm_bridge_chain_get_first_bridge(struct drm_encoder *encoder)
 #define drm_for_each_bridge_in_chain(encoder, bridge)			\
 	list_for_each_entry(bridge, &(encoder)->bridge_chain, chain_node)
 
-bool drm_bridge_chain_mode_fixup(struct drm_bridge *bridge,
-				 const struct drm_display_mode *mode,
-				 struct drm_display_mode *adjusted_mode);
 enum drm_mode_status
 drm_bridge_chain_mode_valid(struct drm_bridge *bridge,
 			    const struct drm_display_info *info,
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 7/7] drm/todo: Add bridge related todo items

[Sam Ravnborg]

- deprecated callbacks in drm_bridge_funcs
- move connector creation to display drivers

v2:
  - Updated descriptions in todo.rst

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Acked-by: Maxime Ripard <maxime@cerno.tech>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: David Airlie <airlied@linux.ie>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 Documentation/gpu/todo.rst | 49 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 60d1d7ee0719..17c03e7c41e5 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -463,6 +463,55 @@ Contact: Thomas Zimmermann <tzimmermann@suse.de>, Christian König, Daniel Vette
 
 Level: Intermediate
 
+Drop use of deprecated operations in bridge drivers
+--------------------------------------------------
+
+&struct drm_bridge_funcs contains a number of deprecated operations
+which use can be replaced by the atomic variants.
+
+The following is more or less 1:1 replacements whit the arguments
+and names adjusted:
+* pre_enable => atomic_pre_enable
+* enable => atomic_enable
+* disable => atomic_disable
+* post_disable => atomic_post_disable
+
+* mode_set is no longer required and the implementation shall be merged
+  with atomic_enable.
+
+* mode_fixup => atomic_check
+  mode_fixup() was created a long time ago, when we were supposed to have
+  a single bridge at the output of the CRTC. The bridge could then instruct
+  the CRTC to output a different mode than what the display requires.
+  Now that we have support for multiple bridges, it's not as straightforward,
+  and we've so far just pretended to ignore the problem. The .mode_fixup()
+  operation is used and abused, and just telling people to use .atomic_check()
+  will likely make things worse as that operation has access to the full atomic
+  commit and can alter the mode of pretty much anything. We need to define clear
+  semantics for .atomic_check() in bridges.
+
+
+Contact: bridge maintainers, Sam Ravnborg <sam@ravnborg.org>,
+         Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+
+Level: Beginner or intermediate (depending on the driver)
+
+Move connector creation to display drivers
+------------------------------------------
+
+With the introduction of chained bridges the creation of connectors are moved
+to the display drivers. The flag DRM_BRIDGE_ATTACH_NO_CONNECTOR is used to
+signal to the bridge driver that no connector shall be created and that the
+display driver will take care. Display drivers will in most cases be able to
+utilise drm_bridge_connector_init() for all the logic.
+
+Step 1 is to have all bridge drivers supporting DRM_BRIDGE_ATTACH_NO_CONNECTOR.
+Step 2 is to move connector creation to all relevant display drivers, utilizing
+the drm_bridge_connector where possible.
+
+Contact: Sam Ravnborg <sam@ravnborg.org>, bridge and/or driver maintainer(s)
+
+Level: Intermediate or advanced (depending on the driver)
 
 Core refactorings
 =================
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/7] drm/bridge: Add drm_atomic_get_new_crtc_for_bridge() helper

[Maxime Ripard]

Hi,

On Wed, Oct 20, 2021 at 08:18:57PM +0200, Sam Ravnborg wrote:
> drm_atomic_get_new_crtc_for_bridge() will be used by bridge
> drivers to provide easy access to the mode from the
> drm_bridge_funcs operations.
> 
> The helper will be useful in the conversion to the atomic
> operations of struct drm_bridge_funcs.
> 
> v2:
>   - Renamed to drm_atomic_get_new_crtc_for_bridge (Maxime)
>   - Use atomic_state, not bridge_State (Maxime)
>   - Drop WARN on crtc_state as it is a valid case (Maxime)
>   - Added small code snip to help readers
>   - Updated description, fixed kernel-doc and exported the symbol
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_atomic.c | 42 ++++++++++++++++++++++++++++++++++++
>  include/drm/drm_atomic.h     |  3 +++
>  2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index ff1416cd609a..8b107194b157 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1134,6 +1134,48 @@ drm_atomic_get_new_bridge_state(struct drm_atomic_state *state,
>  }
>  EXPORT_SYMBOL(drm_atomic_get_new_bridge_state);
>  
> +/**
> + * drm_atomic_get_new_crtc_for_bridge - get new crtc_state for the bridge
> + * @state: state of the bridge
> + * @bridge: bridge object

I appreciate that the function name is fairly long already, but given
its name I'd expect it to return a drm_crtc, not drm_crtc_state.

All the other state related functions are named using the pattern
drm_atomic_get_(old|new)_$object_state.

Moreover, we already have a drm_atomic_get_new_connector_for_encoder
function that does return a drm_connector, so I think we should make it
consistent there and call it drm_atomic_get_new_crtc_state_for_bridge

Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/7] drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Wed, Oct 20, 2021 at 08:18:55PM +0200, Sam Ravnborg wrote:
> The atomic variants of enable/disable in drm_bridge_funcs are the
> preferred operations - introduce these.
> 
> The ps8640 driver used the non-atomic variants of the drm_bridge_chain_pre_enable/
> drm_bridge_chain_post_disable - convert these to the atomic variants.
> 
> v2:
>   - Added a few more people to cc: (Jitao, Enric, Philip) to increase
>     possibility to get test feedback
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Reviewed-by: Maxime Ripard <maxime@cerno.tech>
> Cc: Jitao Shi <jitao.shi@mediatek.com>
> Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Cc: Philip Chen <philipchen@chromium.org>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Robert Foss <robert.foss@linaro.org>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> ---
>  drivers/gpu/drm/bridge/parade-ps8640.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 3aaa90913bf8..0b620afe99c0 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -376,7 +376,8 @@ static void ps8640_bridge_poweroff(struct ps8640 *ps_bridge)
>  	ps_bridge->powered = false;
>  }
>  
> -static void ps8640_pre_enable(struct drm_bridge *bridge)
> +static void ps8640_atomic_pre_enable(struct drm_bridge *bridge,
> +				     struct drm_bridge_state *old_bridge_state)
>  {
>  	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>  	int ret;
> @@ -388,7 +389,8 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
>  		ps8640_bridge_poweroff(ps_bridge);
>  }
>  
> -static void ps8640_post_disable(struct drm_bridge *bridge)
> +static void ps8640_atomic_post_disable(struct drm_bridge *bridge,
> +				       struct drm_bridge_state *old_bridge_state)
>  {
>  	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>  
> @@ -489,7 +491,7 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
>  	 * EDID, for this chip, we need to do a full poweron, otherwise it will
>  	 * fail.
>  	 */
> -	drm_bridge_chain_pre_enable(bridge);
> +	drm_atomic_bridge_chain_pre_enable(bridge, connector->state->state);
>  
>  	edid = drm_get_edid(connector,
>  			    ps_bridge->page[PAGE0_DP_CNTL]->adapter);
> @@ -499,7 +501,7 @@ static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
>  	 * before, return the chip to its original power state.
>  	 */
>  	if (poweroff)
> -		drm_bridge_chain_post_disable(bridge);
> +		drm_atomic_bridge_chain_post_disable(bridge, connector->state->state);
>  
>  	return edid;
>  }
> @@ -508,8 +510,8 @@ static const struct drm_bridge_funcs ps8640_bridge_funcs = {
>  	.attach = ps8640_bridge_attach,
>  	.detach = ps8640_bridge_detach,
>  	.get_edid = ps8640_bridge_get_edid,
> -	.post_disable = ps8640_post_disable,
> -	.pre_enable = ps8640_pre_enable,
> +	.atomic_post_disable = ps8640_atomic_post_disable,
> +	.atomic_pre_enable = ps8640_atomic_pre_enable,

Don't you also need to implement .atomic_duplicate_state(),
.atomic_destroy_state() and .atomic_reset() to use the atomic API ?

>  };
>  
>  static int ps8640_probe(struct i2c_client *client)

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/7] drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs

[Sam Ravnborg]

Hi Laurent,

> > @@ -508,8 +510,8 @@ static const struct drm_bridge_funcs ps8640_bridge_funcs = {
> >  	.attach = ps8640_bridge_attach,
> >  	.detach = ps8640_bridge_detach,
> >  	.get_edid = ps8640_bridge_get_edid,
> > -	.post_disable = ps8640_post_disable,
> > -	.pre_enable = ps8640_pre_enable,
> > +	.atomic_post_disable = ps8640_atomic_post_disable,
> > +	.atomic_pre_enable = ps8640_atomic_pre_enable,
> 
> Don't you also need to implement .atomic_duplicate_state(),
> .atomic_destroy_state() and .atomic_reset() to use the atomic API ?

What I think happens is that the atomic drivers uses the bridges and
the implementation in drm_bridge has fall-backs to the non-atomic
variants.

So for example drm_atomic_bridge_chain_pre_enable() will call
atomic_pre_enable() if it exists, and if not it will call pre_enable()
if it exists.

With this patch-set I show that the non-atomic versions of several of
the drm_bridge helper functions are almost not used and easy to drop.
So based on this I concluded that the bridge drivers were used
in a way so we only would need to implement the atomic variants
of the functions.

That said I did not consider .atomic_duplicate_state(),
.atomic_destroy_state(), or .atomic_reset().

From a quick look only cadence/cdns-mhdp8546 subclass
drm_bridge_state and I wonder if the right thing to do would be to
implement fallback to the helpers if the bridge driver do not set
any of the .atomic_duplicate_state(), .atomic_destroy_state(), or .atomic_reset().

That would drop the following from a few bridges:
        .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
        .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
        .atomic_reset = drm_atomic_helper_bridge_reset,

And new bridges (or bridges we convert to use the atomic operations)
would not have to specify them.

I would prefer implementing the fallback - rather than adding the above
to this driver.

	Sam

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/7] drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs

[Sam Ravnborg]

Hi Laurent,

> 
> >From a quick look only cadence/cdns-mhdp8546 subclass
> drm_bridge_state and I wonder if the right thing to do would be to
> implement fallback to the helpers if the bridge driver do not set
> any of the .atomic_duplicate_state(), .atomic_destroy_state(), or .atomic_reset().
> 
> That would drop the following from a few bridges:
>         .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>         .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>         .atomic_reset = drm_atomic_helper_bridge_reset,

To answer myself here. This would create a dependency from the core to
the helpers which is not OK so idea dropped again.

	Sam

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/7] drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs

[Laurent Pinchart]

Hi Sam,

On Fri, Oct 22, 2021 at 07:13:58PM +0200, Sam Ravnborg wrote:
> Hi Laurent,
> 
> > From a quick look only cadence/cdns-mhdp8546 subclass
> > drm_bridge_state and I wonder if the right thing to do would be to
> > implement fallback to the helpers if the bridge driver do not set
> > any of the .atomic_duplicate_state(), .atomic_destroy_state(), or .atomic_reset().
> > 
> > That would drop the following from a few bridges:
> >         .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> >         .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> >         .atomic_reset = drm_atomic_helper_bridge_reset,
> 
> To answer myself here. This would create a dependency from the core to
> the helpers which is not OK so idea dropped again.

I agree it would be nicer, but the dependency is likely a problem. That
being said, we have multiple types of helpers. The first set is the
modeset helpers, which were designed as one implementation of KMS
operations, with an opt-in API for drivers. The core should not depend
on those. There are however other helpers that are only default
implementations of some operations, without any dependency on other
components. The atomic state helpers fall in this category, they
implement .atomic_* operations of the drm_*_funcs structures, not
drm_*_helper_funcs. It could make sense to move them to the DRM core.

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/7] drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs

[Sam Ravnborg]

Hi Laurent,

On Fri, Oct 22, 2021 at 09:42:37PM +0300, Laurent Pinchart wrote:
> Hi Sam,
> 
> On Fri, Oct 22, 2021 at 07:13:58PM +0200, Sam Ravnborg wrote:
> > Hi Laurent,
> > 
> > > From a quick look only cadence/cdns-mhdp8546 subclass
> > > drm_bridge_state and I wonder if the right thing to do would be to
> > > implement fallback to the helpers if the bridge driver do not set
> > > any of the .atomic_duplicate_state(), .atomic_destroy_state(), or .atomic_reset().
> > > 
> > > That would drop the following from a few bridges:
> > >         .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> > >         .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> > >         .atomic_reset = drm_atomic_helper_bridge_reset,
> > 
> > To answer myself here. This would create a dependency from the core to
> > the helpers which is not OK so idea dropped again.
> 
> I agree it would be nicer, but the dependency is likely a problem. That
> being said, we have multiple types of helpers. The first set is the
> modeset helpers, which were designed as one implementation of KMS
> operations, with an opt-in API for drivers. The core should not depend
> on those. There are however other helpers that are only default
> implementations of some operations, without any dependency on other
> components. The atomic state helpers fall in this category, they
> implement .atomic_* operations of the drm_*_funcs structures, not
> drm_*_helper_funcs. It could make sense to move them to the DRM core.

For now I went with a simple macro:

+/**
+ * DRM_BRIDGE_STATE_OPS - Default drm_bridge state funcs
+ *
+ * Bridge driver that do not subclass &drm_bridge_state can use the helpers
+ * for reset, duplicate, and destroy. This macro provides a shortcut for
+ * setting the helpers in the &drm_bridge_funcs structure.
+ */
+#define DRM_BRIDGE_STATE_OPS \
+       .atomic_reset = drm_atomic_helper_bridge_reset,                         \
+       .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,     \
+       .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state
+

Thomas Z. is trying to make the core smaller so pulling in these helpers
would be counterproductive to that. So I took the simpler approach here
which we have already done in several places.
It will be part of v3 when I post it.

Drop a note if you (or any other reader) have better ideas.

	Sam

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/7] drm/bridge: Add drm_atomic_get_new_crtc_for_bridge() helper

[Sam Ravnborg]

Hi Maxime,

On Fri, Oct 22, 2021 at 01:03:55PM +0200, Maxime Ripard wrote:
> Hi,
> 
> On Wed, Oct 20, 2021 at 08:18:57PM +0200, Sam Ravnborg wrote:
> > drm_atomic_get_new_crtc_for_bridge() will be used by bridge
> > drivers to provide easy access to the mode from the
> > drm_bridge_funcs operations.
> > 
> > The helper will be useful in the conversion to the atomic
> > operations of struct drm_bridge_funcs.
> > 
> > v2:
> >   - Renamed to drm_atomic_get_new_crtc_for_bridge (Maxime)
> >   - Use atomic_state, not bridge_State (Maxime)
> >   - Drop WARN on crtc_state as it is a valid case (Maxime)
> >   - Added small code snip to help readers
> >   - Updated description, fixed kernel-doc and exported the symbol
> > 
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > Suggested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Maxime Ripard <mripard@kernel.org>
> > Cc: Thomas Zimmermann <tzimmermann@suse.de>
> > Cc: Andrzej Hajda <a.hajda@samsung.com>
> > Cc: Neil Armstrong <narmstrong@baylibre.com>
> > Cc: Robert Foss <robert.foss@linaro.org>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > ---
> >  drivers/gpu/drm/drm_atomic.c | 42 ++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_atomic.h     |  3 +++
> >  2 files changed, 45 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index ff1416cd609a..8b107194b157 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1134,6 +1134,48 @@ drm_atomic_get_new_bridge_state(struct drm_atomic_state *state,
> >  }
> >  EXPORT_SYMBOL(drm_atomic_get_new_bridge_state);
> >  
> > +/**
> > + * drm_atomic_get_new_crtc_for_bridge - get new crtc_state for the bridge
> > + * @state: state of the bridge
> > + * @bridge: bridge object
> 
> I appreciate that the function name is fairly long already, but given
> its name I'd expect it to return a drm_crtc, not drm_crtc_state.
> 
> All the other state related functions are named using the pattern
> drm_atomic_get_(old|new)_$object_state.
> 
> Moreover, we already have a drm_atomic_get_new_connector_for_encoder
> function that does return a drm_connector, so I think we should make it
> consistent there and call it drm_atomic_get_new_crtc_state_for_bridge

That's a better name - thanks! I will fix it in v3.

	Sam

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/7] drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs

[Laurent Pinchart]

Hi Sam,

On Fri, Oct 22, 2021 at 09:32:08PM +0200, Sam Ravnborg wrote:
> On Fri, Oct 22, 2021 at 09:42:37PM +0300, Laurent Pinchart wrote:
> > On Fri, Oct 22, 2021 at 07:13:58PM +0200, Sam Ravnborg wrote:
> > > Hi Laurent,
> > > 
> > > > From a quick look only cadence/cdns-mhdp8546 subclass
> > > > drm_bridge_state and I wonder if the right thing to do would be to
> > > > implement fallback to the helpers if the bridge driver do not set
> > > > any of the .atomic_duplicate_state(), .atomic_destroy_state(), or .atomic_reset().
> > > > 
> > > > That would drop the following from a few bridges:
> > > >         .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> > > >         .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> > > >         .atomic_reset = drm_atomic_helper_bridge_reset,
> > > 
> > > To answer myself here. This would create a dependency from the core to
> > > the helpers which is not OK so idea dropped again.
> > 
> > I agree it would be nicer, but the dependency is likely a problem. That
> > being said, we have multiple types of helpers. The first set is the
> > modeset helpers, which were designed as one implementation of KMS
> > operations, with an opt-in API for drivers. The core should not depend
> > on those. There are however other helpers that are only default
> > implementations of some operations, without any dependency on other
> > components. The atomic state helpers fall in this category, they
> > implement .atomic_* operations of the drm_*_funcs structures, not
> > drm_*_helper_funcs. It could make sense to move them to the DRM core.
> 
> For now I went with a simple macro:
> 
> +/**
> + * DRM_BRIDGE_STATE_OPS - Default drm_bridge state funcs
> + *
> + * Bridge driver that do not subclass &drm_bridge_state can use the helpers
> + * for reset, duplicate, and destroy. This macro provides a shortcut for
> + * setting the helpers in the &drm_bridge_funcs structure.
> + */
> +#define DRM_BRIDGE_STATE_OPS \
> +       .atomic_reset = drm_atomic_helper_bridge_reset,                         \
> +       .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,     \
> +       .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state
> +
> 
> Thomas Z. is trying to make the core smaller so pulling in these helpers
> would be counterproductive to that. So I took the simpler approach here
> which we have already done in several places.

Those helpers are in the same file as the other state helpers, which are
used by all atomic drivers as far as I can tell, so I'm not sure we can
really make anything smaller (except if we moved the bridge helpers to a
separate file, but I don't think it would be worth it).

> It will be part of v3 when I post it.
> 
> Drop a note if you (or any other reader) have better ideas.

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel