[PATCH v1 0/12] drm bridge updates

[PATCH v1 0/12] drm bridge updates

[Sam Ravnborg]

This is a collection of bridge patches that I had left unfinished
more than a year ago. Now where I have some spare time I dusted them
off for review and testing.

The patches builds, but has seen no run-time testing.

The patches are grouped like this:

1+2:
  - Updates parade-ps8640 for atomic operation thus allowing
    the use of the atomic variants of drm_bridge_chain_pre_enable/
    post_disable.
    There is thus no users left of the non-atomic variants of
    drm_bridge_chain* so drop them all.
    If other drivers needs this functionality they are supposed
    to use the atomic API.
    This is a DSI to eDP converter, and I did not see a need for
    format negotiation - but I may be wrong?

3+4:
  - Drop chain_mode_fixup in mediatek, and as there are no more
    users, drop the implementation of drm_bridge_chain_mode_fixup.
    The crucial part to review is the updated documentation for
    drm_bridge_funcs.atomic_check().

5-11:
  - Convert all bridge drivers from mode_fixup to atomic_check.
    To do so add the necessary hooks to have an atomic state.
    I think the drivers will work using the atomic API,
    but it has seen no testing!
    I did not see any need for format negotiation, but if needed
    let me know so we can fix it.
    The last patch removes the mode_fixup support in drm_bridge.

12:
  - Add a few todo items, in the hope others will jump in and
    convert some of the bridge drivers to the atomic API.

The patch groups are more or less independent, and I may land them out
of order if one group is reviewed before the rest.
A few of the patches has already seen a review, but they need a
bit more exposure as last review was more than a year ago.

	Sam


Sam Ravnborg (12):
      drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs
      drm/bridge: Drop unused drm_bridge_chain functions
      drm/mediatek: Drop chain_mode_fixup call in mode_valid()
      drm/bridge: Drop drm_bridge_chain_mode_fixup
      drm/bridge: sii8620: Use drm_bridge_funcs.atomic_check
      drm/bridge: cros-ec-anx7688: Use drm_bridge_funcs.atomic_check
      drm/bridge: tc358767: Use drm_bridge_funcs.atomic_check
      drm/mediatek: Drop mtk_hdmi_bridge_mode_fixup
      drm/rcar-du: lvds: Use drm_bridge_funcs.atomic_check
      drm/omapdrm: Use drm_bridge_funcs.atomic_check
      drm/bridge: Drop drm_bridge_funcs.mode_fixup
      drm/todo: Add bridge related todo items

 Documentation/gpu/todo.rst               |  20 ++++
 drivers/gpu/drm/bridge/cros-ec-anx7688.c |  28 ++++--
 drivers/gpu/drm/bridge/parade-ps8640.c   |  18 ++--
 drivers/gpu/drm/bridge/sil-sii8620.c     |  17 ++--
 drivers/gpu/drm/bridge/tc358767.c        |  21 ++---
 drivers/gpu/drm/drm_bridge.c             | 154 +------------------------------
 drivers/gpu/drm/mediatek/mtk_hdmi.c      |  19 ----
 drivers/gpu/drm/omapdrm/dss/dpi.c        |  31 ++++---
 drivers/gpu/drm/omapdrm/dss/sdi.c        |  31 ++++---
 drivers/gpu/drm/omapdrm/dss/venc.c       |  28 ++++--
 drivers/gpu/drm/rcar-du/rcar_lvds.c      |  12 ++-
 include/drm/drm_bridge.h                 |  91 ++++--------------
 12 files changed, 147 insertions(+), 323 deletions(-)

[PATCH v1 01/12] 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:
  - Init state operations in drm_bridge_funcs (Laurent)

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
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 | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 31e88cb39f8a..bb8076fb8625 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -15,6 +15,7 @@
 
 #include <drm/display/drm_dp_aux_bus.h>
 #include <drm/display/drm_dp_helper.h>
+#include <drm/drm_atomic_state_helper.h>
 #include <drm/drm_bridge.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_mipi_dsi.h>
@@ -409,7 +410,8 @@ static const struct dev_pm_ops ps8640_pm_ops = {
 				pm_runtime_force_resume)
 };
 
-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);
 	struct regmap *map = ps_bridge->regmap[PAGE2_TOP_CNTL];
@@ -443,7 +445,8 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
 	ps_bridge->pre_enabled = true;
 }
 
-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);
 
@@ -521,7 +524,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);
@@ -531,7 +534,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;
 }
@@ -546,8 +549,11 @@ 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,
+	.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,
 };
 
 static int ps8640_bridge_get_dsi_resources(struct device *dev, struct ps8640 *ps_bridge)
-- 
2.34.1

Re: [PATCH v1 01/12] drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs

[Laurent Pinchart]

Hi Sam,

Thank you for the patch, and sorry for the review delay. The series only
recently jumped to the top of my inbox.

On Sun, Jul 17, 2022 at 07:44:43PM +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:
>   - Init state operations in drm_bridge_funcs (Laurent)
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Reviewed-by: Maxime Ripard <mripard@kernel.org>
> 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>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/bridge/parade-ps8640.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 31e88cb39f8a..bb8076fb8625 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -15,6 +15,7 @@
>  
>  #include <drm/display/drm_dp_aux_bus.h>
>  #include <drm/display/drm_dp_helper.h>
> +#include <drm/drm_atomic_state_helper.h>
>  #include <drm/drm_bridge.h>
>  #include <drm/drm_edid.h>
>  #include <drm/drm_mipi_dsi.h>
> @@ -409,7 +410,8 @@ static const struct dev_pm_ops ps8640_pm_ops = {
>  				pm_runtime_force_resume)
>  };
>  
> -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);
>  	struct regmap *map = ps_bridge->regmap[PAGE2_TOP_CNTL];
> @@ -443,7 +445,8 @@ static void ps8640_pre_enable(struct drm_bridge *bridge)
>  	ps_bridge->pre_enabled = true;
>  }
>  
> -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);
>  
> @@ -521,7 +524,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);
> @@ -531,7 +534,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;
>  }
> @@ -546,8 +549,11 @@ 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,
> +	.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,
>  };
>  
>  static int ps8640_bridge_get_dsi_resources(struct device *dev, struct ps8640 *ps_bridge)

-- 
Regards,

Laurent Pinchart

[PATCH v1 02/12] 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 <mripard@kernel.org>
Reviewed-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_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 1545c50fd1c8..bb7fc09267af 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -509,61 +509,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
@@ -593,61 +538,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 dba5d81e3b4a..1eca9c4c3346 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,
@@ -876,13 +852,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.34.1

[PATCH v1 03/12] 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 <mripard@kernel.org>
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 3196189429bc..a63b76055f81 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) {
 		if (hdmi->conf->cea_modes_only && !drm_match_cea_mode(mode))
 			return MODE_BAD;
-- 
2.34.1

Re: [PATCH v1 03/12] drm/mediatek: Drop chain_mode_fixup call in mode_valid()

[Chun-Kuang Hu]

Hi, Sam:

Sam Ravnborg <sam@ravnborg.org> 於 2022年7月18日 週一 凌晨1:45寫道：
>
> 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.

Acked-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>

>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Reviewed-by: Maxime Ripard <mripard@kernel.org>
> 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 3196189429bc..a63b76055f81 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) {
>                 if (hdmi->conf->cea_modes_only && !drm_match_cea_mode(mode))
>                         return MODE_BAD;
> --
> 2.34.1
>

Re: [PATCH v1 03/12] drm/mediatek: Drop chain_mode_fixup call in mode_valid()

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Sun, Jul 17, 2022 at 07:44:45PM +0200, Sam Ravnborg wrote:
> 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 <mripard@kernel.org>
> 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

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  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 3196189429bc..a63b76055f81 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) {
>  		if (hdmi->conf->cea_modes_only && !drm_match_cea_mode(mode))
>  			return MODE_BAD;

-- 
Regards,

Laurent Pinchart

[PATCH v1 04/12] 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 <mripard@kernel.org>
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 bb7fc09267af..b6f56d8f3547 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -430,43 +430,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 1eca9c4c3346..7496f41535b1 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -845,9 +845,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.34.1

Re: [PATCH v1 04/12] drm/bridge: Drop drm_bridge_chain_mode_fixup

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Sun, Jul 17, 2022 at 07:44:46PM +0200, Sam Ravnborg wrote:
> 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 <mripard@kernel.org>
> 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>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  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 bb7fc09267af..b6f56d8f3547 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -430,43 +430,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 1eca9c4c3346..7496f41535b1 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -845,9 +845,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,

-- 
Regards,

Laurent Pinchart

[PATCH v1 05/12] drm/bridge: sii8620: Use drm_bridge_funcs.atomic_check

[Sam Ravnborg]

Replace the deprecated drm_bridge_funcs.mode_fixup() with
drm_bridge_funcs.atomic_check().

drm_bridge_funcs.atomic_check() requires the atomic state operations,
update these to the default implementations.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Andrzej Hajda <andrzej.hajda@intel.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/sil-sii8620.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
index ab0bce4a988c..b6e5c285c8ea 100644
--- a/drivers/gpu/drm/bridge/sil-sii8620.c
+++ b/drivers/gpu/drm/bridge/sil-sii8620.c
@@ -8,6 +8,7 @@
 
 #include <asm/unaligned.h>
 
+#include <drm/drm_atomic_state_helper.h>
 #include <drm/bridge/mhl.h>
 #include <drm/drm_bridge.h>
 #include <drm/drm_crtc.h>
@@ -2262,26 +2263,30 @@ static enum drm_mode_status sii8620_mode_valid(struct drm_bridge *bridge,
 	}
 }
 
-static bool sii8620_mode_fixup(struct drm_bridge *bridge,
-			       const struct drm_display_mode *mode,
-			       struct drm_display_mode *adjusted_mode)
+static int sii8620_atomic_check(struct drm_bridge *bridge,
+				struct drm_bridge_state *bridge_state,
+				struct drm_crtc_state *crtc_state,
+				struct drm_connector_state *conn_state)
 {
 	struct sii8620 *ctx = bridge_to_sii8620(bridge);
 
 	mutex_lock(&ctx->lock);
 
-	ctx->use_packed_pixel = sii8620_is_packing_required(ctx, adjusted_mode);
+	ctx->use_packed_pixel = sii8620_is_packing_required(ctx, &crtc_state->adjusted_mode);
 
 	mutex_unlock(&ctx->lock);
 
-	return true;
+	return 0;
 }
 
 static const struct drm_bridge_funcs sii8620_bridge_funcs = {
 	.attach = sii8620_attach,
 	.detach = sii8620_detach,
-	.mode_fixup = sii8620_mode_fixup,
+	.atomic_check = sii8620_atomic_check,
 	.mode_valid = sii8620_mode_valid,
+	.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,
 };
 
 static int sii8620_probe(struct i2c_client *client,
-- 
2.34.1

Re: [PATCH v1 05/12] drm/bridge: sii8620: Use drm_bridge_funcs.atomic_check

[Dave Stevenson]

On Sun, 17 Jul 2022 at 18:45, Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Replace the deprecated drm_bridge_funcs.mode_fixup() with
> drm_bridge_funcs.atomic_check().
>
> drm_bridge_funcs.atomic_check() requires the atomic state operations,
> update these to the default implementations.
>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> Cc: Andrzej Hajda <andrzej.hajda@intel.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/sil-sii8620.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
> index ab0bce4a988c..b6e5c285c8ea 100644
> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> @@ -8,6 +8,7 @@
>
>  #include <asm/unaligned.h>
>
> +#include <drm/drm_atomic_state_helper.h>
>  #include <drm/bridge/mhl.h>
>  #include <drm/drm_bridge.h>
>  #include <drm/drm_crtc.h>
> @@ -2262,26 +2263,30 @@ static enum drm_mode_status sii8620_mode_valid(struct drm_bridge *bridge,
>         }
>  }
>
> -static bool sii8620_mode_fixup(struct drm_bridge *bridge,
> -                              const struct drm_display_mode *mode,
> -                              struct drm_display_mode *adjusted_mode)
> +static int sii8620_atomic_check(struct drm_bridge *bridge,
> +                               struct drm_bridge_state *bridge_state,
> +                               struct drm_crtc_state *crtc_state,
> +                               struct drm_connector_state *conn_state)
>  {
>         struct sii8620 *ctx = bridge_to_sii8620(bridge);
>
>         mutex_lock(&ctx->lock);
>
> -       ctx->use_packed_pixel = sii8620_is_packing_required(ctx, adjusted_mode);
> +       ctx->use_packed_pixel = sii8620_is_packing_required(ctx, &crtc_state->adjusted_mode);
>
>         mutex_unlock(&ctx->lock);
>
> -       return true;
> +       return 0;
>  }
>
>  static const struct drm_bridge_funcs sii8620_bridge_funcs = {
>         .attach = sii8620_attach,
>         .detach = sii8620_detach,
> -       .mode_fixup = sii8620_mode_fixup,
> +       .atomic_check = sii8620_atomic_check,
>         .mode_valid = sii8620_mode_valid,
> +       .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,
>  };
>
>  static int sii8620_probe(struct i2c_client *client,
> --
> 2.34.1
>

Re: [PATCH v1 05/12] drm/bridge: sii8620: Use drm_bridge_funcs.atomic_check

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Sun, Jul 17, 2022 at 07:44:47PM +0200, Sam Ravnborg wrote:
> Replace the deprecated drm_bridge_funcs.mode_fixup() with
> drm_bridge_funcs.atomic_check().
> 
> drm_bridge_funcs.atomic_check() requires the atomic state operations,
> update these to the default implementations.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Andrzej Hajda <andrzej.hajda@intel.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/sil-sii8620.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/sil-sii8620.c b/drivers/gpu/drm/bridge/sil-sii8620.c
> index ab0bce4a988c..b6e5c285c8ea 100644
> --- a/drivers/gpu/drm/bridge/sil-sii8620.c
> +++ b/drivers/gpu/drm/bridge/sil-sii8620.c
> @@ -8,6 +8,7 @@
>  
>  #include <asm/unaligned.h>
>  
> +#include <drm/drm_atomic_state_helper.h>

I'd move this one line down, in alphabetical order.

>  #include <drm/bridge/mhl.h>
>  #include <drm/drm_bridge.h>
>  #include <drm/drm_crtc.h>
> @@ -2262,26 +2263,30 @@ static enum drm_mode_status sii8620_mode_valid(struct drm_bridge *bridge,
>  	}
>  }
>  
> -static bool sii8620_mode_fixup(struct drm_bridge *bridge,
> -			       const struct drm_display_mode *mode,
> -			       struct drm_display_mode *adjusted_mode)
> +static int sii8620_atomic_check(struct drm_bridge *bridge,
> +				struct drm_bridge_state *bridge_state,
> +				struct drm_crtc_state *crtc_state,
> +				struct drm_connector_state *conn_state)
>  {
>  	struct sii8620 *ctx = bridge_to_sii8620(bridge);
>  
>  	mutex_lock(&ctx->lock);
>  
> -	ctx->use_packed_pixel = sii8620_is_packing_required(ctx, adjusted_mode);
> +	ctx->use_packed_pixel = sii8620_is_packing_required(ctx, &crtc_state->adjusted_mode);

Shouldn't this be moved to atomic_enable ? A test commit should change
the device state.

As this code was initially in mode_fixup I suppose this patch could be
merged as-is, with the problem fixed on top, so

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

>  
>  	mutex_unlock(&ctx->lock);
>  
> -	return true;
> +	return 0;
>  }
>  
>  static const struct drm_bridge_funcs sii8620_bridge_funcs = {
>  	.attach = sii8620_attach,
>  	.detach = sii8620_detach,
> -	.mode_fixup = sii8620_mode_fixup,
> +	.atomic_check = sii8620_atomic_check,
>  	.mode_valid = sii8620_mode_valid,
> +	.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,
>  };
>  
>  static int sii8620_probe(struct i2c_client *client,

-- 
Regards,

Laurent Pinchart

[PATCH v1 06/12] drm/bridge: cros-ec-anx7688: Use drm_bridge_funcs.atomic_check

[Sam Ravnborg]

Replace the deprecated drm_bridge_funcs.mode_fixup() with
drm_bridge_funcs.atomic_check().

drm_bridge_funcs.atomic_check() requires the atomic state operations,
update these to the default implementations.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Andrzej Hajda <andrzej.hajda@intel.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>
Cc: Benson Leung <bleung@chromium.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: chrome-platform@lists.linux.dev
---
 drivers/gpu/drm/bridge/cros-ec-anx7688.c | 28 +++++++++++++++---------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cros-ec-anx7688.c b/drivers/gpu/drm/bridge/cros-ec-anx7688.c
index 0f6d907432e3..fc19ea87926f 100644
--- a/drivers/gpu/drm/bridge/cros-ec-anx7688.c
+++ b/drivers/gpu/drm/bridge/cros-ec-anx7688.c
@@ -5,6 +5,7 @@
  * Copyright 2020 Google LLC
  */
 
+#include <drm/drm_atomic_state_helper.h>
 #include <drm/drm_bridge.h>
 #include <drm/drm_print.h>
 #include <linux/i2c.h>
@@ -45,9 +46,10 @@ bridge_to_cros_ec_anx7688(struct drm_bridge *bridge)
 	return container_of(bridge, struct cros_ec_anx7688, bridge);
 }
 
-static bool cros_ec_anx7688_bridge_mode_fixup(struct drm_bridge *bridge,
-					      const struct drm_display_mode *mode,
-					      struct drm_display_mode *adjusted_mode)
+static int cros_ec_anx7688_bridge_atomic_check(struct drm_bridge *bridge,
+					       struct drm_bridge_state *bridge_state,
+					       struct drm_crtc_state *crtc_state,
+					       struct drm_connector_state *conn_state)
 {
 	struct cros_ec_anx7688 *anx = bridge_to_cros_ec_anx7688(bridge);
 	int totalbw, requiredbw;
@@ -56,13 +58,13 @@ static bool cros_ec_anx7688_bridge_mode_fixup(struct drm_bridge *bridge,
 	int ret;
 
 	if (!anx->filter)
-		return true;
+		return 0;
 
 	/* Read both regs 0x85 (bandwidth) and 0x86 (lane count). */
 	ret = regmap_bulk_read(anx->regmap, ANX7688_DP_BANDWIDTH_REG, regs, 2);
 	if (ret < 0) {
 		DRM_ERROR("Failed to read bandwidth/lane count\n");
-		return false;
+		return ret;
 	}
 	dpbw = regs[0];
 	lanecount = regs[1];
@@ -71,28 +73,34 @@ static bool cros_ec_anx7688_bridge_mode_fixup(struct drm_bridge *bridge,
 	if (dpbw > 0x19 || lanecount > 2) {
 		DRM_ERROR("Invalid bandwidth/lane count (%02x/%d)\n", dpbw,
 			  lanecount);
-		return false;
+		return -EINVAL;
 	}
 
 	/* Compute available bandwidth (kHz) */
 	totalbw = dpbw * lanecount * 270000 * 8 / 10;
 
 	/* Required bandwidth (8 bpc, kHz) */
-	requiredbw = mode->clock * 8 * 3;
+	requiredbw = crtc_state->mode.clock * 8 * 3;
 
 	DRM_DEBUG_KMS("DP bandwidth: %d kHz (%02x/%d); mode requires %d Khz\n",
 		      totalbw, dpbw, lanecount, requiredbw);
 
 	if (totalbw == 0) {
 		DRM_ERROR("Bandwidth/lane count are 0, not rejecting modes\n");
-		return true;
+		return 0;
 	}
 
-	return totalbw >= requiredbw;
+	if (totalbw < requiredbw)
+		return -EINVAL;
+
+	return 0;
 }
 
 static const struct drm_bridge_funcs cros_ec_anx7688_bridge_funcs = {
-	.mode_fixup = cros_ec_anx7688_bridge_mode_fixup,
+	.atomic_check = cros_ec_anx7688_bridge_atomic_check,
+	.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,
 };
 
 static int cros_ec_anx7688_bridge_probe(struct i2c_client *client)
-- 
2.34.1

Re: [PATCH v1 06/12] drm/bridge: cros-ec-anx7688: Use drm_bridge_funcs.atomic_check

[Dave Stevenson]

On Sun, 17 Jul 2022 at 18:45, Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Replace the deprecated drm_bridge_funcs.mode_fixup() with
> drm_bridge_funcs.atomic_check().
>
> drm_bridge_funcs.atomic_check() requires the atomic state operations,
> update these to the default implementations.
>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> Cc: Andrzej Hajda <andrzej.hajda@intel.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>
> Cc: Benson Leung <bleung@chromium.org>
> Cc: Guenter Roeck <groeck@chromium.org>
> Cc: chrome-platform@lists.linux.dev
> ---
>  drivers/gpu/drm/bridge/cros-ec-anx7688.c | 28 +++++++++++++++---------
>  1 file changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/cros-ec-anx7688.c b/drivers/gpu/drm/bridge/cros-ec-anx7688.c
> index 0f6d907432e3..fc19ea87926f 100644
> --- a/drivers/gpu/drm/bridge/cros-ec-anx7688.c
> +++ b/drivers/gpu/drm/bridge/cros-ec-anx7688.c
> @@ -5,6 +5,7 @@
>   * Copyright 2020 Google LLC
>   */
>
> +#include <drm/drm_atomic_state_helper.h>
>  #include <drm/drm_bridge.h>
>  #include <drm/drm_print.h>
>  #include <linux/i2c.h>
> @@ -45,9 +46,10 @@ bridge_to_cros_ec_anx7688(struct drm_bridge *bridge)
>         return container_of(bridge, struct cros_ec_anx7688, bridge);
>  }
>
> -static bool cros_ec_anx7688_bridge_mode_fixup(struct drm_bridge *bridge,
> -                                             const struct drm_display_mode *mode,
> -                                             struct drm_display_mode *adjusted_mode)
> +static int cros_ec_anx7688_bridge_atomic_check(struct drm_bridge *bridge,
> +                                              struct drm_bridge_state *bridge_state,
> +                                              struct drm_crtc_state *crtc_state,
> +                                              struct drm_connector_state *conn_state)
>  {
>         struct cros_ec_anx7688 *anx = bridge_to_cros_ec_anx7688(bridge);
>         int totalbw, requiredbw;
> @@ -56,13 +58,13 @@ static bool cros_ec_anx7688_bridge_mode_fixup(struct drm_bridge *bridge,
>         int ret;
>
>         if (!anx->filter)
> -               return true;
> +               return 0;
>
>         /* Read both regs 0x85 (bandwidth) and 0x86 (lane count). */
>         ret = regmap_bulk_read(anx->regmap, ANX7688_DP_BANDWIDTH_REG, regs, 2);
>         if (ret < 0) {
>                 DRM_ERROR("Failed to read bandwidth/lane count\n");
> -               return false;
> +               return ret;
>         }
>         dpbw = regs[0];
>         lanecount = regs[1];
> @@ -71,28 +73,34 @@ static bool cros_ec_anx7688_bridge_mode_fixup(struct drm_bridge *bridge,
>         if (dpbw > 0x19 || lanecount > 2) {
>                 DRM_ERROR("Invalid bandwidth/lane count (%02x/%d)\n", dpbw,
>                           lanecount);
> -               return false;
> +               return -EINVAL;
>         }
>
>         /* Compute available bandwidth (kHz) */
>         totalbw = dpbw * lanecount * 270000 * 8 / 10;
>
>         /* Required bandwidth (8 bpc, kHz) */
> -       requiredbw = mode->clock * 8 * 3;
> +       requiredbw = crtc_state->mode.clock * 8 * 3;
>
>         DRM_DEBUG_KMS("DP bandwidth: %d kHz (%02x/%d); mode requires %d Khz\n",
>                       totalbw, dpbw, lanecount, requiredbw);
>
>         if (totalbw == 0) {
>                 DRM_ERROR("Bandwidth/lane count are 0, not rejecting modes\n");
> -               return true;
> +               return 0;
>         }
>
> -       return totalbw >= requiredbw;
> +       if (totalbw < requiredbw)
> +               return -EINVAL;
> +
> +       return 0;
>  }
>
>  static const struct drm_bridge_funcs cros_ec_anx7688_bridge_funcs = {
> -       .mode_fixup = cros_ec_anx7688_bridge_mode_fixup,
> +       .atomic_check = cros_ec_anx7688_bridge_atomic_check,
> +       .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,
>  };
>
>  static int cros_ec_anx7688_bridge_probe(struct i2c_client *client)
> --
> 2.34.1
>

Re: [PATCH v1 06/12] drm/bridge: cros-ec-anx7688: Use drm_bridge_funcs.atomic_check

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Sun, Jul 17, 2022 at 07:44:48PM +0200, Sam Ravnborg wrote:
> Replace the deprecated drm_bridge_funcs.mode_fixup() with
> drm_bridge_funcs.atomic_check().
> 
> drm_bridge_funcs.atomic_check() requires the atomic state operations,
> update these to the default implementations.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Andrzej Hajda <andrzej.hajda@intel.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>
> Cc: Benson Leung <bleung@chromium.org>
> Cc: Guenter Roeck <groeck@chromium.org>
> Cc: chrome-platform@lists.linux.dev

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  drivers/gpu/drm/bridge/cros-ec-anx7688.c | 28 +++++++++++++++---------
>  1 file changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cros-ec-anx7688.c b/drivers/gpu/drm/bridge/cros-ec-anx7688.c
> index 0f6d907432e3..fc19ea87926f 100644
> --- a/drivers/gpu/drm/bridge/cros-ec-anx7688.c
> +++ b/drivers/gpu/drm/bridge/cros-ec-anx7688.c
> @@ -5,6 +5,7 @@
>   * Copyright 2020 Google LLC
>   */
>  
> +#include <drm/drm_atomic_state_helper.h>
>  #include <drm/drm_bridge.h>
>  #include <drm/drm_print.h>
>  #include <linux/i2c.h>
> @@ -45,9 +46,10 @@ bridge_to_cros_ec_anx7688(struct drm_bridge *bridge)
>  	return container_of(bridge, struct cros_ec_anx7688, bridge);
>  }
>  
> -static bool cros_ec_anx7688_bridge_mode_fixup(struct drm_bridge *bridge,
> -					      const struct drm_display_mode *mode,
> -					      struct drm_display_mode *adjusted_mode)
> +static int cros_ec_anx7688_bridge_atomic_check(struct drm_bridge *bridge,
> +					       struct drm_bridge_state *bridge_state,
> +					       struct drm_crtc_state *crtc_state,
> +					       struct drm_connector_state *conn_state)
>  {
>  	struct cros_ec_anx7688 *anx = bridge_to_cros_ec_anx7688(bridge);
>  	int totalbw, requiredbw;
> @@ -56,13 +58,13 @@ static bool cros_ec_anx7688_bridge_mode_fixup(struct drm_bridge *bridge,
>  	int ret;
>  
>  	if (!anx->filter)
> -		return true;
> +		return 0;
>  
>  	/* Read both regs 0x85 (bandwidth) and 0x86 (lane count). */
>  	ret = regmap_bulk_read(anx->regmap, ANX7688_DP_BANDWIDTH_REG, regs, 2);
>  	if (ret < 0) {
>  		DRM_ERROR("Failed to read bandwidth/lane count\n");
> -		return false;
> +		return ret;
>  	}
>  	dpbw = regs[0];
>  	lanecount = regs[1];
> @@ -71,28 +73,34 @@ static bool cros_ec_anx7688_bridge_mode_fixup(struct drm_bridge *bridge,
>  	if (dpbw > 0x19 || lanecount > 2) {
>  		DRM_ERROR("Invalid bandwidth/lane count (%02x/%d)\n", dpbw,
>  			  lanecount);
> -		return false;
> +		return -EINVAL;
>  	}
>  
>  	/* Compute available bandwidth (kHz) */
>  	totalbw = dpbw * lanecount * 270000 * 8 / 10;
>  
>  	/* Required bandwidth (8 bpc, kHz) */
> -	requiredbw = mode->clock * 8 * 3;
> +	requiredbw = crtc_state->mode.clock * 8 * 3;
>  
>  	DRM_DEBUG_KMS("DP bandwidth: %d kHz (%02x/%d); mode requires %d Khz\n",
>  		      totalbw, dpbw, lanecount, requiredbw);
>  
>  	if (totalbw == 0) {
>  		DRM_ERROR("Bandwidth/lane count are 0, not rejecting modes\n");
> -		return true;
> +		return 0;
>  	}
>  
> -	return totalbw >= requiredbw;
> +	if (totalbw < requiredbw)
> +		return -EINVAL;
> +
> +	return 0;
>  }
>  
>  static const struct drm_bridge_funcs cros_ec_anx7688_bridge_funcs = {
> -	.mode_fixup = cros_ec_anx7688_bridge_mode_fixup,
> +	.atomic_check = cros_ec_anx7688_bridge_atomic_check,
> +	.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,
>  };
>  
>  static int cros_ec_anx7688_bridge_probe(struct i2c_client *client)

-- 
Regards,

Laurent Pinchart

[PATCH v1 07/12] drm/bridge: tc358767: Use drm_bridge_funcs.atomic_check

[Sam Ravnborg]

When atomic_check() is defined, then mode_fixup() is ignored,
so it had no effect that drm_bridge_funcs.mode_fixup was assigned.
Embed the original implementation in the caller and drop the function.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Andrzej Hajda <andrzej.hajda@intel.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/tc358767.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 02bd757a8987..b2ab967504af 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1496,26 +1496,18 @@ tc_edp_bridge_atomic_disable(struct drm_bridge *bridge,
 		dev_err(tc->dev, "main link disable error: %d\n", ret);
 }
 
-static bool tc_bridge_mode_fixup(struct drm_bridge *bridge,
-				 const struct drm_display_mode *mode,
-				 struct drm_display_mode *adj)
-{
-	/* Fixup sync polarities, both hsync and vsync are active low */
-	adj->flags = mode->flags;
-	adj->flags |= (DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
-	adj->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
-
-	return true;
-}
-
 static int tc_common_atomic_check(struct drm_bridge *bridge,
 				  struct drm_bridge_state *bridge_state,
 				  struct drm_crtc_state *crtc_state,
 				  struct drm_connector_state *conn_state,
 				  const unsigned int max_khz)
 {
-	tc_bridge_mode_fixup(bridge, &crtc_state->mode,
-			     &crtc_state->adjusted_mode);
+	struct drm_display_mode *adj = &crtc_state->adjusted_mode;
+
+	/* Fixup sync polarities, both hsync and vsync are active low */
+	adj->flags = crtc_state->mode.flags;
+	adj->flags |= (DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
+	adj->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
 
 	if (crtc_state->adjusted_mode.clock > max_khz)
 		return -EINVAL;
@@ -1783,7 +1775,6 @@ static const struct drm_bridge_funcs tc_edp_bridge_funcs = {
 	.atomic_check = tc_edp_atomic_check,
 	.atomic_enable = tc_edp_bridge_atomic_enable,
 	.atomic_disable = tc_edp_bridge_atomic_disable,
-	.mode_fixup = tc_bridge_mode_fixup,
 	.detect = tc_bridge_detect,
 	.get_edid = tc_get_edid,
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
-- 
2.34.1

Re: [PATCH v1 07/12] drm/bridge: tc358767: Use drm_bridge_funcs.atomic_check

[Dave Stevenson]

On Sun, 17 Jul 2022 at 18:45, Sam Ravnborg <sam@ravnborg.org> wrote:
>
> When atomic_check() is defined, then mode_fixup() is ignored,
> so it had no effect that drm_bridge_funcs.mode_fixup was assigned.
> Embed the original implementation in the caller and drop the function.
>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

(There was a point when drm_bridge_chain_mode_fixup still existed, but
that's gone/going now).

> Cc: Andrzej Hajda <andrzej.hajda@intel.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/tc358767.c | 21 ++++++---------------
>  1 file changed, 6 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 02bd757a8987..b2ab967504af 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1496,26 +1496,18 @@ tc_edp_bridge_atomic_disable(struct drm_bridge *bridge,
>                 dev_err(tc->dev, "main link disable error: %d\n", ret);
>  }
>
> -static bool tc_bridge_mode_fixup(struct drm_bridge *bridge,
> -                                const struct drm_display_mode *mode,
> -                                struct drm_display_mode *adj)
> -{
> -       /* Fixup sync polarities, both hsync and vsync are active low */
> -       adj->flags = mode->flags;
> -       adj->flags |= (DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
> -       adj->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
> -
> -       return true;
> -}
> -
>  static int tc_common_atomic_check(struct drm_bridge *bridge,
>                                   struct drm_bridge_state *bridge_state,
>                                   struct drm_crtc_state *crtc_state,
>                                   struct drm_connector_state *conn_state,
>                                   const unsigned int max_khz)
>  {
> -       tc_bridge_mode_fixup(bridge, &crtc_state->mode,
> -                            &crtc_state->adjusted_mode);
> +       struct drm_display_mode *adj = &crtc_state->adjusted_mode;
> +
> +       /* Fixup sync polarities, both hsync and vsync are active low */
> +       adj->flags = crtc_state->mode.flags;
> +       adj->flags |= (DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
> +       adj->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
>
>         if (crtc_state->adjusted_mode.clock > max_khz)
>                 return -EINVAL;
> @@ -1783,7 +1775,6 @@ static const struct drm_bridge_funcs tc_edp_bridge_funcs = {
>         .atomic_check = tc_edp_atomic_check,
>         .atomic_enable = tc_edp_bridge_atomic_enable,
>         .atomic_disable = tc_edp_bridge_atomic_disable,
> -       .mode_fixup = tc_bridge_mode_fixup,
>         .detect = tc_bridge_detect,
>         .get_edid = tc_get_edid,
>         .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> --
> 2.34.1
>

Re: [PATCH v1 07/12] drm/bridge: tc358767: Use drm_bridge_funcs.atomic_check

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Sun, Jul 17, 2022 at 07:44:49PM +0200, Sam Ravnborg wrote:
> When atomic_check() is defined, then mode_fixup() is ignored,
> so it had no effect that drm_bridge_funcs.mode_fixup was assigned.
> Embed the original implementation in the caller and drop the function.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Andrzej Hajda <andrzej.hajda@intel.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>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  drivers/gpu/drm/bridge/tc358767.c | 21 ++++++---------------
>  1 file changed, 6 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 02bd757a8987..b2ab967504af 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1496,26 +1496,18 @@ tc_edp_bridge_atomic_disable(struct drm_bridge *bridge,
>  		dev_err(tc->dev, "main link disable error: %d\n", ret);
>  }
>  
> -static bool tc_bridge_mode_fixup(struct drm_bridge *bridge,
> -				 const struct drm_display_mode *mode,
> -				 struct drm_display_mode *adj)
> -{
> -	/* Fixup sync polarities, both hsync and vsync are active low */
> -	adj->flags = mode->flags;
> -	adj->flags |= (DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
> -	adj->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
> -
> -	return true;
> -}
> -
>  static int tc_common_atomic_check(struct drm_bridge *bridge,
>  				  struct drm_bridge_state *bridge_state,
>  				  struct drm_crtc_state *crtc_state,
>  				  struct drm_connector_state *conn_state,
>  				  const unsigned int max_khz)
>  {
> -	tc_bridge_mode_fixup(bridge, &crtc_state->mode,
> -			     &crtc_state->adjusted_mode);
> +	struct drm_display_mode *adj = &crtc_state->adjusted_mode;
> +
> +	/* Fixup sync polarities, both hsync and vsync are active low */
> +	adj->flags = crtc_state->mode.flags;
> +	adj->flags |= (DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
> +	adj->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
>  
>  	if (crtc_state->adjusted_mode.clock > max_khz)
>  		return -EINVAL;
> @@ -1783,7 +1775,6 @@ static const struct drm_bridge_funcs tc_edp_bridge_funcs = {
>  	.atomic_check = tc_edp_atomic_check,
>  	.atomic_enable = tc_edp_bridge_atomic_enable,
>  	.atomic_disable = tc_edp_bridge_atomic_disable,
> -	.mode_fixup = tc_bridge_mode_fixup,
>  	.detect = tc_bridge_detect,
>  	.get_edid = tc_get_edid,
>  	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 07/12] drm/bridge: tc358767: Use drm_bridge_funcs.atomic_check

[Laurent Pinchart]

On Mon, Sep 19, 2022 at 06:29:41PM +0300, Laurent Pinchart wrote:
> On Sun, Jul 17, 2022 at 07:44:49PM +0200, Sam Ravnborg wrote:
> > When atomic_check() is defined, then mode_fixup() is ignored,
> > so it had no effect that drm_bridge_funcs.mode_fixup was assigned.
> > Embed the original implementation in the caller and drop the function.
> > 
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > Cc: Andrzej Hajda <andrzej.hajda@intel.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>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

But this doesn't apply anymore, as the mode_fixup operation has been
removed from the driver already. You can just drop this patch.

> > ---
> >  drivers/gpu/drm/bridge/tc358767.c | 21 ++++++---------------
> >  1 file changed, 6 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> > index 02bd757a8987..b2ab967504af 100644
> > --- a/drivers/gpu/drm/bridge/tc358767.c
> > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > @@ -1496,26 +1496,18 @@ tc_edp_bridge_atomic_disable(struct drm_bridge *bridge,
> >  		dev_err(tc->dev, "main link disable error: %d\n", ret);
> >  }
> >  
> > -static bool tc_bridge_mode_fixup(struct drm_bridge *bridge,
> > -				 const struct drm_display_mode *mode,
> > -				 struct drm_display_mode *adj)
> > -{
> > -	/* Fixup sync polarities, both hsync and vsync are active low */
> > -	adj->flags = mode->flags;
> > -	adj->flags |= (DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
> > -	adj->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
> > -
> > -	return true;
> > -}
> > -
> >  static int tc_common_atomic_check(struct drm_bridge *bridge,
> >  				  struct drm_bridge_state *bridge_state,
> >  				  struct drm_crtc_state *crtc_state,
> >  				  struct drm_connector_state *conn_state,
> >  				  const unsigned int max_khz)
> >  {
> > -	tc_bridge_mode_fixup(bridge, &crtc_state->mode,
> > -			     &crtc_state->adjusted_mode);
> > +	struct drm_display_mode *adj = &crtc_state->adjusted_mode;
> > +
> > +	/* Fixup sync polarities, both hsync and vsync are active low */
> > +	adj->flags = crtc_state->mode.flags;
> > +	adj->flags |= (DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
> > +	adj->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
> >  
> >  	if (crtc_state->adjusted_mode.clock > max_khz)
> >  		return -EINVAL;
> > @@ -1783,7 +1775,6 @@ static const struct drm_bridge_funcs tc_edp_bridge_funcs = {
> >  	.atomic_check = tc_edp_atomic_check,
> >  	.atomic_enable = tc_edp_bridge_atomic_enable,
> >  	.atomic_disable = tc_edp_bridge_atomic_disable,
> > -	.mode_fixup = tc_bridge_mode_fixup,
> >  	.detect = tc_bridge_detect,
> >  	.get_edid = tc_get_edid,
> >  	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,

-- 
Regards,

Laurent Pinchart

[PATCH v1 08/12] drm/mediatek: Drop mtk_hdmi_bridge_mode_fixup

[Sam Ravnborg]

The implementation of drm_bridge_funcs.mode_fixup is optional
so there is no need to provide an empty implementation.
Drop mtk_hdmi_bridge_mode_fixup() so the driver no longer uses the
deprecated drm_bridge_funcs.mode_fixup() operation.

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

diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c
index a63b76055f81..7321aa1ee6f0 100644
--- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
+++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
@@ -1293,13 +1293,6 @@ static int mtk_hdmi_bridge_attach(struct drm_bridge *bridge,
 	return 0;
 }
 
-static bool mtk_hdmi_bridge_mode_fixup(struct drm_bridge *bridge,
-				       const struct drm_display_mode *mode,
-				       struct drm_display_mode *adjusted_mode)
-{
-	return true;
-}
-
 static void mtk_hdmi_bridge_atomic_disable(struct drm_bridge *bridge,
 					   struct drm_bridge_state *old_bridge_state)
 {
@@ -1399,7 +1392,6 @@ static const struct drm_bridge_funcs mtk_hdmi_bridge_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
 	.atomic_reset = drm_atomic_helper_bridge_reset,
 	.attach = mtk_hdmi_bridge_attach,
-	.mode_fixup = mtk_hdmi_bridge_mode_fixup,
 	.atomic_disable = mtk_hdmi_bridge_atomic_disable,
 	.atomic_post_disable = mtk_hdmi_bridge_atomic_post_disable,
 	.mode_set = mtk_hdmi_bridge_mode_set,
-- 
2.34.1

[PATCH v1 09/12] drm/rcar-du: lvds: Use drm_bridge_funcs.atomic_check

[Sam Ravnborg]

Replace the deprecated drm_bridge_funcs.mode_fixup() with
drm_bridge_funcs.atomic_check().
The driver implements the state operations, so no other changes
are required for the replacement.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Cc: linux-renesas-soc@vger.kernel.org
---
 drivers/gpu/drm/rcar-du/rcar_lvds.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
index 830aac0a2cb4..c4adbcede090 100644
--- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
+++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
@@ -554,10 +554,12 @@ static void rcar_lvds_atomic_disable(struct drm_bridge *bridge,
 	clk_disable_unprepare(lvds->clocks.mod);
 }
 
-static bool rcar_lvds_mode_fixup(struct drm_bridge *bridge,
-				 const struct drm_display_mode *mode,
-				 struct drm_display_mode *adjusted_mode)
+static int rcar_lvds_atomic_check(struct drm_bridge *bridge,
+				  struct drm_bridge_state *bridge_state,
+				  struct drm_crtc_state *crtc_state,
+				  struct drm_connector_state *conn_state)
 {
+	struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
 	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
 	int min_freq;
 
@@ -569,7 +571,7 @@ static bool rcar_lvds_mode_fixup(struct drm_bridge *bridge,
 	min_freq = lvds->info->quirks & RCAR_LVDS_QUIRK_EXT_PLL ? 5000 : 31000;
 	adjusted_mode->clock = clamp(adjusted_mode->clock, min_freq, 148500);
 
-	return true;
+	return 0;
 }
 
 static int rcar_lvds_attach(struct drm_bridge *bridge,
@@ -591,7 +593,7 @@ static const struct drm_bridge_funcs rcar_lvds_bridge_ops = {
 	.atomic_reset = drm_atomic_helper_bridge_reset,
 	.atomic_enable = rcar_lvds_atomic_enable,
 	.atomic_disable = rcar_lvds_atomic_disable,
-	.mode_fixup = rcar_lvds_mode_fixup,
+	.atomic_check = rcar_lvds_atomic_check,
 };
 
 bool rcar_lvds_dual_link(struct drm_bridge *bridge)
-- 
2.34.1

Re: [PATCH v1 09/12] drm/rcar-du: lvds: Use drm_bridge_funcs.atomic_check

[Dave Stevenson]

On Sun, 17 Jul 2022 at 18:58, Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Replace the deprecated drm_bridge_funcs.mode_fixup() with
> drm_bridge_funcs.atomic_check().
> The driver implements the state operations, so no other changes
> are required for the replacement.
>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Cc: linux-renesas-soc@vger.kernel.org
> ---
>  drivers/gpu/drm/rcar-du/rcar_lvds.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index 830aac0a2cb4..c4adbcede090 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -554,10 +554,12 @@ static void rcar_lvds_atomic_disable(struct drm_bridge *bridge,
>         clk_disable_unprepare(lvds->clocks.mod);
>  }
>
> -static bool rcar_lvds_mode_fixup(struct drm_bridge *bridge,
> -                                const struct drm_display_mode *mode,
> -                                struct drm_display_mode *adjusted_mode)
> +static int rcar_lvds_atomic_check(struct drm_bridge *bridge,
> +                                 struct drm_bridge_state *bridge_state,
> +                                 struct drm_crtc_state *crtc_state,
> +                                 struct drm_connector_state *conn_state)
>  {
> +       struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
>         struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
>         int min_freq;
>
> @@ -569,7 +571,7 @@ static bool rcar_lvds_mode_fixup(struct drm_bridge *bridge,
>         min_freq = lvds->info->quirks & RCAR_LVDS_QUIRK_EXT_PLL ? 5000 : 31000;
>         adjusted_mode->clock = clamp(adjusted_mode->clock, min_freq, 148500);
>
> -       return true;
> +       return 0;
>  }
>
>  static int rcar_lvds_attach(struct drm_bridge *bridge,
> @@ -591,7 +593,7 @@ static const struct drm_bridge_funcs rcar_lvds_bridge_ops = {
>         .atomic_reset = drm_atomic_helper_bridge_reset,
>         .atomic_enable = rcar_lvds_atomic_enable,
>         .atomic_disable = rcar_lvds_atomic_disable,
> -       .mode_fixup = rcar_lvds_mode_fixup,
> +       .atomic_check = rcar_lvds_atomic_check,
>  };
>
>  bool rcar_lvds_dual_link(struct drm_bridge *bridge)
> --
> 2.34.1
>

Re: [PATCH v1 09/12] drm/rcar-du: lvds: Use drm_bridge_funcs.atomic_check

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Sun, Jul 17, 2022 at 07:57:58PM +0200, Sam Ravnborg wrote:
> Replace the deprecated drm_bridge_funcs.mode_fixup() with
> drm_bridge_funcs.atomic_check().
> The driver implements the state operations, so no other changes
> are required for the replacement.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Cc: linux-renesas-soc@vger.kernel.org

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

I assume you'll merge this through drm-misc with the rest of the series,
so I won't take it in my tree unless you ask me so.

> ---
>  drivers/gpu/drm/rcar-du/rcar_lvds.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_lvds.c b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> index 830aac0a2cb4..c4adbcede090 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_lvds.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_lvds.c
> @@ -554,10 +554,12 @@ static void rcar_lvds_atomic_disable(struct drm_bridge *bridge,
>  	clk_disable_unprepare(lvds->clocks.mod);
>  }
>  
> -static bool rcar_lvds_mode_fixup(struct drm_bridge *bridge,
> -				 const struct drm_display_mode *mode,
> -				 struct drm_display_mode *adjusted_mode)
> +static int rcar_lvds_atomic_check(struct drm_bridge *bridge,
> +				  struct drm_bridge_state *bridge_state,
> +				  struct drm_crtc_state *crtc_state,
> +				  struct drm_connector_state *conn_state)
>  {
> +	struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
>  	struct rcar_lvds *lvds = bridge_to_rcar_lvds(bridge);
>  	int min_freq;
>  
> @@ -569,7 +571,7 @@ static bool rcar_lvds_mode_fixup(struct drm_bridge *bridge,
>  	min_freq = lvds->info->quirks & RCAR_LVDS_QUIRK_EXT_PLL ? 5000 : 31000;
>  	adjusted_mode->clock = clamp(adjusted_mode->clock, min_freq, 148500);
>  
> -	return true;
> +	return 0;
>  }
>  
>  static int rcar_lvds_attach(struct drm_bridge *bridge,
> @@ -591,7 +593,7 @@ static const struct drm_bridge_funcs rcar_lvds_bridge_ops = {
>  	.atomic_reset = drm_atomic_helper_bridge_reset,
>  	.atomic_enable = rcar_lvds_atomic_enable,
>  	.atomic_disable = rcar_lvds_atomic_disable,
> -	.mode_fixup = rcar_lvds_mode_fixup,
> +	.atomic_check = rcar_lvds_atomic_check,
>  };
>  
>  bool rcar_lvds_dual_link(struct drm_bridge *bridge)

-- 
Regards,

Laurent Pinchart

[PATCH v1 10/12] drm/omapdrm: Use drm_bridge_funcs.atomic_check

[Sam Ravnborg]

Replace the deprecated drm_bridge_funcs.mode_fixup() with
drm_bridge_funcs.atomic_check().

drm_bridge_funcs.atomic_check() requires the atomic state operations,
update these to the default implementations.
Likewise update enable/disable to their atomic variants.

With these changes omapdrm now implement the full bridge atomic API.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Cc: Cai Huoqing <cai.huoqing@linux.dev>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/gpu/drm/omapdrm/dss/dpi.c  | 31 ++++++++++++++++++------------
 drivers/gpu/drm/omapdrm/dss/sdi.c  | 31 ++++++++++++++++++------------
 drivers/gpu/drm/omapdrm/dss/venc.c | 28 +++++++++++++++++----------
 3 files changed, 56 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/dss/dpi.c b/drivers/gpu/drm/omapdrm/dss/dpi.c
index 030f997eccd0..0a0b49750eca 100644
--- a/drivers/gpu/drm/omapdrm/dss/dpi.c
+++ b/drivers/gpu/drm/omapdrm/dss/dpi.c
@@ -21,6 +21,7 @@
 #include <linux/string.h>
 #include <linux/sys_soc.h>
 
+#include <drm/drm_atomic_state_helper.h>
 #include <drm/drm_bridge.h>
 
 #include "dss.h"
@@ -454,21 +455,22 @@ dpi_bridge_mode_valid(struct drm_bridge *bridge,
 	return MODE_OK;
 }
 
-static bool dpi_bridge_mode_fixup(struct drm_bridge *bridge,
-				   const struct drm_display_mode *mode,
-				   struct drm_display_mode *adjusted_mode)
+static int dpi_bridge_atomic_check(struct drm_bridge *bridge,
+				   struct drm_bridge_state *bridge_state,
+				   struct drm_crtc_state *crtc_state,
+				   struct drm_connector_state *conn_state)
 {
 	struct dpi_data *dpi = drm_bridge_to_dpi(bridge);
-	unsigned long clock = mode->clock * 1000;
+	unsigned long clock = crtc_state->mode.clock * 1000;
 	int ret;
 
 	ret = dpi_clock_update(dpi, &clock);
 	if (ret < 0)
-		return false;
+		return ret;
 
-	adjusted_mode->clock = clock / 1000;
+	crtc_state->adjusted_mode.clock = clock / 1000;
 
-	return true;
+	return 0;
 }
 
 static void dpi_bridge_mode_set(struct drm_bridge *bridge,
@@ -480,7 +482,8 @@ static void dpi_bridge_mode_set(struct drm_bridge *bridge,
 	dpi->pixelclock = adjusted_mode->clock * 1000;
 }
 
-static void dpi_bridge_enable(struct drm_bridge *bridge)
+static void dpi_bridge_atomic_enable(struct drm_bridge *bridge,
+				     struct drm_bridge_state *old_bridge_state)
 {
 	struct dpi_data *dpi = drm_bridge_to_dpi(bridge);
 	int r;
@@ -531,7 +534,8 @@ static void dpi_bridge_enable(struct drm_bridge *bridge)
 		regulator_disable(dpi->vdds_dsi_reg);
 }
 
-static void dpi_bridge_disable(struct drm_bridge *bridge)
+static void dpi_bridge_atomic_disable(struct drm_bridge *bridge,
+				      struct drm_bridge_state *old_bridge_state)
 {
 	struct dpi_data *dpi = drm_bridge_to_dpi(bridge);
 
@@ -552,10 +556,13 @@ static void dpi_bridge_disable(struct drm_bridge *bridge)
 static const struct drm_bridge_funcs dpi_bridge_funcs = {
 	.attach = dpi_bridge_attach,
 	.mode_valid = dpi_bridge_mode_valid,
-	.mode_fixup = dpi_bridge_mode_fixup,
+	.atomic_check = dpi_bridge_atomic_check,
 	.mode_set = dpi_bridge_mode_set,
-	.enable = dpi_bridge_enable,
-	.disable = dpi_bridge_disable,
+	.atomic_enable = dpi_bridge_atomic_enable,
+	.atomic_disable = dpi_bridge_atomic_disable,
+	.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,
 };
 
 static void dpi_bridge_init(struct dpi_data *dpi)
diff --git a/drivers/gpu/drm/omapdrm/dss/sdi.c b/drivers/gpu/drm/omapdrm/dss/sdi.c
index 91eaae3b9481..73b728722c2f 100644
--- a/drivers/gpu/drm/omapdrm/dss/sdi.c
+++ b/drivers/gpu/drm/omapdrm/dss/sdi.c
@@ -15,6 +15,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/string.h>
 
+#include <drm/drm_atomic_state_helper.h>
 #include <drm/drm_bridge.h>
 
 #include "dss.h"
@@ -159,12 +160,13 @@ sdi_bridge_mode_valid(struct drm_bridge *bridge,
 	return MODE_OK;
 }
 
-static bool sdi_bridge_mode_fixup(struct drm_bridge *bridge,
-				  const struct drm_display_mode *mode,
-				  struct drm_display_mode *adjusted_mode)
+static int sdi_bridge_atomic_check(struct drm_bridge *bridge,
+				   struct drm_bridge_state *bridge_state,
+				   struct drm_crtc_state *crtc_state,
+				   struct drm_connector_state *conn_state)
 {
 	struct sdi_device *sdi = drm_bridge_to_sdi(bridge);
-	unsigned long pixelclock = mode->clock * 1000;
+	unsigned long pixelclock = crtc_state->mode.clock * 1000;
 	struct dispc_clock_info dispc_cinfo;
 	unsigned long fck;
 	unsigned long pck;
@@ -172,7 +174,7 @@ static bool sdi_bridge_mode_fixup(struct drm_bridge *bridge,
 
 	ret = sdi_calc_clock_div(sdi, pixelclock, &fck, &dispc_cinfo);
 	if (ret < 0)
-		return false;
+		return ret;
 
 	pck = fck / dispc_cinfo.lck_div / dispc_cinfo.pck_div;
 
@@ -181,9 +183,9 @@ static bool sdi_bridge_mode_fixup(struct drm_bridge *bridge,
 			"pixel clock adjusted from %lu Hz to %lu Hz\n",
 			pixelclock, pck);
 
-	adjusted_mode->clock = pck / 1000;
+	crtc_state->adjusted_mode.clock = pck / 1000;
 
-	return true;
+	return 0;
 }
 
 static void sdi_bridge_mode_set(struct drm_bridge *bridge,
@@ -195,7 +197,8 @@ static void sdi_bridge_mode_set(struct drm_bridge *bridge,
 	sdi->pixelclock = adjusted_mode->clock * 1000;
 }
 
-static void sdi_bridge_enable(struct drm_bridge *bridge)
+static void sdi_bridge_atomic_enable(struct drm_bridge *bridge,
+				     struct drm_bridge_state *old_bridge_state)
 {
 	struct sdi_device *sdi = drm_bridge_to_sdi(bridge);
 	struct dispc_clock_info dispc_cinfo;
@@ -258,7 +261,8 @@ static void sdi_bridge_enable(struct drm_bridge *bridge)
 	regulator_disable(sdi->vdds_sdi_reg);
 }
 
-static void sdi_bridge_disable(struct drm_bridge *bridge)
+static void sdi_bridge_atomic_disable(struct drm_bridge *bridge,
+				      struct drm_bridge_state *old_bridge_state)
 {
 	struct sdi_device *sdi = drm_bridge_to_sdi(bridge);
 
@@ -274,10 +278,13 @@ static void sdi_bridge_disable(struct drm_bridge *bridge)
 static const struct drm_bridge_funcs sdi_bridge_funcs = {
 	.attach = sdi_bridge_attach,
 	.mode_valid = sdi_bridge_mode_valid,
-	.mode_fixup = sdi_bridge_mode_fixup,
+	.atomic_check = sdi_bridge_atomic_check,
 	.mode_set = sdi_bridge_mode_set,
-	.enable = sdi_bridge_enable,
-	.disable = sdi_bridge_disable,
+	.atomic_enable = sdi_bridge_atomic_enable,
+	.atomic_disable = sdi_bridge_atomic_disable,
+	.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,
 };
 
 static void sdi_bridge_init(struct sdi_device *sdi)
diff --git a/drivers/gpu/drm/omapdrm/dss/venc.c b/drivers/gpu/drm/omapdrm/dss/venc.c
index 4480b69ab5a7..994e6399d574 100644
--- a/drivers/gpu/drm/omapdrm/dss/venc.c
+++ b/drivers/gpu/drm/omapdrm/dss/venc.c
@@ -25,6 +25,7 @@
 #include <linux/component.h>
 #include <linux/sys_soc.h>
 
+#include <drm/drm_atomic_state_helper.h>
 #include <drm/drm_bridge.h>
 
 #include "omapdss.h"
@@ -564,11 +565,13 @@ venc_bridge_mode_valid(struct drm_bridge *bridge,
 	}
 }
 
-static bool venc_bridge_mode_fixup(struct drm_bridge *bridge,
-				   const struct drm_display_mode *mode,
-				   struct drm_display_mode *adjusted_mode)
+static int venc_bridge_atomic_check(struct drm_bridge *bridge,
+				    struct drm_bridge_state *bridge_state,
+				    struct drm_crtc_state *crtc_state,
+				    struct drm_connector_state *conn_state)
 {
 	const struct drm_display_mode *venc_mode;
+	struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
 
 	switch (venc_get_videomode(adjusted_mode)) {
 	case VENC_MODE_PAL:
@@ -580,14 +583,14 @@ static bool venc_bridge_mode_fixup(struct drm_bridge *bridge,
 		break;
 
 	default:
-		return false;
+		return -EINVAL;
 	}
 
 	drm_mode_copy(adjusted_mode, venc_mode);
 	drm_mode_set_crtcinfo(adjusted_mode, CRTC_INTERLACE_HALVE_V);
 	drm_mode_set_name(adjusted_mode);
 
-	return true;
+	return 0;
 }
 
 static void venc_bridge_mode_set(struct drm_bridge *bridge,
@@ -613,14 +616,16 @@ static void venc_bridge_mode_set(struct drm_bridge *bridge,
 	dispc_set_tv_pclk(venc->dss->dispc, 13500000);
 }
 
-static void venc_bridge_enable(struct drm_bridge *bridge)
+static void venc_bridge_atomic_enable(struct drm_bridge *bridge,
+				      struct drm_bridge_state *old_bridge_state)
 {
 	struct venc_device *venc = drm_bridge_to_venc(bridge);
 
 	venc_power_on(venc);
 }
 
-static void venc_bridge_disable(struct drm_bridge *bridge)
+static void venc_bridge_atomic_disable(struct drm_bridge *bridge,
+				       struct drm_bridge_state *old_bridge_state)
 {
 	struct venc_device *venc = drm_bridge_to_venc(bridge);
 
@@ -654,11 +659,14 @@ static int venc_bridge_get_modes(struct drm_bridge *bridge,
 static const struct drm_bridge_funcs venc_bridge_funcs = {
 	.attach = venc_bridge_attach,
 	.mode_valid = venc_bridge_mode_valid,
-	.mode_fixup = venc_bridge_mode_fixup,
+	.atomic_check = venc_bridge_atomic_check,
 	.mode_set = venc_bridge_mode_set,
-	.enable = venc_bridge_enable,
-	.disable = venc_bridge_disable,
+	.atomic_enable = venc_bridge_atomic_enable,
+	.atomic_disable = venc_bridge_atomic_disable,
 	.get_modes = venc_bridge_get_modes,
+	.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,
 };
 
 static void venc_bridge_init(struct venc_device *venc)
-- 
2.34.1

Re: [PATCH v1 10/12] drm/omapdrm: Use drm_bridge_funcs.atomic_check

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Sun, Jul 17, 2022 at 07:57:59PM +0200, Sam Ravnborg wrote:
> Replace the deprecated drm_bridge_funcs.mode_fixup() with
> drm_bridge_funcs.atomic_check().
> 
> drm_bridge_funcs.atomic_check() requires the atomic state operations,
> update these to the default implementations.
> Likewise update enable/disable to their atomic variants.
> 
> With these changes omapdrm now implement the full bridge atomic API.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Cc: Cai Huoqing <cai.huoqing@linux.dev>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  drivers/gpu/drm/omapdrm/dss/dpi.c  | 31 ++++++++++++++++++------------
>  drivers/gpu/drm/omapdrm/dss/sdi.c  | 31 ++++++++++++++++++------------
>  drivers/gpu/drm/omapdrm/dss/venc.c | 28 +++++++++++++++++----------
>  3 files changed, 56 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dpi.c b/drivers/gpu/drm/omapdrm/dss/dpi.c
> index 030f997eccd0..0a0b49750eca 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dpi.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dpi.c
> @@ -21,6 +21,7 @@
>  #include <linux/string.h>
>  #include <linux/sys_soc.h>
>  
> +#include <drm/drm_atomic_state_helper.h>
>  #include <drm/drm_bridge.h>
>  
>  #include "dss.h"
> @@ -454,21 +455,22 @@ dpi_bridge_mode_valid(struct drm_bridge *bridge,
>  	return MODE_OK;
>  }
>  
> -static bool dpi_bridge_mode_fixup(struct drm_bridge *bridge,
> -				   const struct drm_display_mode *mode,
> -				   struct drm_display_mode *adjusted_mode)
> +static int dpi_bridge_atomic_check(struct drm_bridge *bridge,
> +				   struct drm_bridge_state *bridge_state,
> +				   struct drm_crtc_state *crtc_state,
> +				   struct drm_connector_state *conn_state)
>  {
>  	struct dpi_data *dpi = drm_bridge_to_dpi(bridge);
> -	unsigned long clock = mode->clock * 1000;
> +	unsigned long clock = crtc_state->mode.clock * 1000;
>  	int ret;
>  
>  	ret = dpi_clock_update(dpi, &clock);
>  	if (ret < 0)
> -		return false;
> +		return ret;
>  
> -	adjusted_mode->clock = clock / 1000;
> +	crtc_state->adjusted_mode.clock = clock / 1000;
>  
> -	return true;
> +	return 0;
>  }
>  
>  static void dpi_bridge_mode_set(struct drm_bridge *bridge,
> @@ -480,7 +482,8 @@ static void dpi_bridge_mode_set(struct drm_bridge *bridge,
>  	dpi->pixelclock = adjusted_mode->clock * 1000;
>  }
>  
> -static void dpi_bridge_enable(struct drm_bridge *bridge)
> +static void dpi_bridge_atomic_enable(struct drm_bridge *bridge,
> +				     struct drm_bridge_state *old_bridge_state)
>  {
>  	struct dpi_data *dpi = drm_bridge_to_dpi(bridge);
>  	int r;
> @@ -531,7 +534,8 @@ static void dpi_bridge_enable(struct drm_bridge *bridge)
>  		regulator_disable(dpi->vdds_dsi_reg);
>  }
>  
> -static void dpi_bridge_disable(struct drm_bridge *bridge)
> +static void dpi_bridge_atomic_disable(struct drm_bridge *bridge,
> +				      struct drm_bridge_state *old_bridge_state)
>  {
>  	struct dpi_data *dpi = drm_bridge_to_dpi(bridge);
>  
> @@ -552,10 +556,13 @@ static void dpi_bridge_disable(struct drm_bridge *bridge)
>  static const struct drm_bridge_funcs dpi_bridge_funcs = {
>  	.attach = dpi_bridge_attach,
>  	.mode_valid = dpi_bridge_mode_valid,
> -	.mode_fixup = dpi_bridge_mode_fixup,
> +	.atomic_check = dpi_bridge_atomic_check,
>  	.mode_set = dpi_bridge_mode_set,
> -	.enable = dpi_bridge_enable,
> -	.disable = dpi_bridge_disable,
> +	.atomic_enable = dpi_bridge_atomic_enable,
> +	.atomic_disable = dpi_bridge_atomic_disable,
> +	.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,
>  };
>  
>  static void dpi_bridge_init(struct dpi_data *dpi)
> diff --git a/drivers/gpu/drm/omapdrm/dss/sdi.c b/drivers/gpu/drm/omapdrm/dss/sdi.c
> index 91eaae3b9481..73b728722c2f 100644
> --- a/drivers/gpu/drm/omapdrm/dss/sdi.c
> +++ b/drivers/gpu/drm/omapdrm/dss/sdi.c
> @@ -15,6 +15,7 @@
>  #include <linux/regulator/consumer.h>
>  #include <linux/string.h>
>  
> +#include <drm/drm_atomic_state_helper.h>
>  #include <drm/drm_bridge.h>
>  
>  #include "dss.h"
> @@ -159,12 +160,13 @@ sdi_bridge_mode_valid(struct drm_bridge *bridge,
>  	return MODE_OK;
>  }
>  
> -static bool sdi_bridge_mode_fixup(struct drm_bridge *bridge,
> -				  const struct drm_display_mode *mode,
> -				  struct drm_display_mode *adjusted_mode)
> +static int sdi_bridge_atomic_check(struct drm_bridge *bridge,
> +				   struct drm_bridge_state *bridge_state,
> +				   struct drm_crtc_state *crtc_state,
> +				   struct drm_connector_state *conn_state)
>  {
>  	struct sdi_device *sdi = drm_bridge_to_sdi(bridge);
> -	unsigned long pixelclock = mode->clock * 1000;
> +	unsigned long pixelclock = crtc_state->mode.clock * 1000;
>  	struct dispc_clock_info dispc_cinfo;
>  	unsigned long fck;
>  	unsigned long pck;
> @@ -172,7 +174,7 @@ static bool sdi_bridge_mode_fixup(struct drm_bridge *bridge,
>  
>  	ret = sdi_calc_clock_div(sdi, pixelclock, &fck, &dispc_cinfo);
>  	if (ret < 0)
> -		return false;
> +		return ret;
>  
>  	pck = fck / dispc_cinfo.lck_div / dispc_cinfo.pck_div;
>  
> @@ -181,9 +183,9 @@ static bool sdi_bridge_mode_fixup(struct drm_bridge *bridge,
>  			"pixel clock adjusted from %lu Hz to %lu Hz\n",
>  			pixelclock, pck);
>  
> -	adjusted_mode->clock = pck / 1000;
> +	crtc_state->adjusted_mode.clock = pck / 1000;
>  
> -	return true;
> +	return 0;
>  }
>  
>  static void sdi_bridge_mode_set(struct drm_bridge *bridge,
> @@ -195,7 +197,8 @@ static void sdi_bridge_mode_set(struct drm_bridge *bridge,
>  	sdi->pixelclock = adjusted_mode->clock * 1000;
>  }
>  
> -static void sdi_bridge_enable(struct drm_bridge *bridge)
> +static void sdi_bridge_atomic_enable(struct drm_bridge *bridge,
> +				     struct drm_bridge_state *old_bridge_state)
>  {
>  	struct sdi_device *sdi = drm_bridge_to_sdi(bridge);
>  	struct dispc_clock_info dispc_cinfo;
> @@ -258,7 +261,8 @@ static void sdi_bridge_enable(struct drm_bridge *bridge)
>  	regulator_disable(sdi->vdds_sdi_reg);
>  }
>  
> -static void sdi_bridge_disable(struct drm_bridge *bridge)
> +static void sdi_bridge_atomic_disable(struct drm_bridge *bridge,
> +				      struct drm_bridge_state *old_bridge_state)
>  {
>  	struct sdi_device *sdi = drm_bridge_to_sdi(bridge);
>  
> @@ -274,10 +278,13 @@ static void sdi_bridge_disable(struct drm_bridge *bridge)
>  static const struct drm_bridge_funcs sdi_bridge_funcs = {
>  	.attach = sdi_bridge_attach,
>  	.mode_valid = sdi_bridge_mode_valid,
> -	.mode_fixup = sdi_bridge_mode_fixup,
> +	.atomic_check = sdi_bridge_atomic_check,
>  	.mode_set = sdi_bridge_mode_set,
> -	.enable = sdi_bridge_enable,
> -	.disable = sdi_bridge_disable,
> +	.atomic_enable = sdi_bridge_atomic_enable,
> +	.atomic_disable = sdi_bridge_atomic_disable,
> +	.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,
>  };
>  
>  static void sdi_bridge_init(struct sdi_device *sdi)
> diff --git a/drivers/gpu/drm/omapdrm/dss/venc.c b/drivers/gpu/drm/omapdrm/dss/venc.c
> index 4480b69ab5a7..994e6399d574 100644
> --- a/drivers/gpu/drm/omapdrm/dss/venc.c
> +++ b/drivers/gpu/drm/omapdrm/dss/venc.c
> @@ -25,6 +25,7 @@
>  #include <linux/component.h>
>  #include <linux/sys_soc.h>
>  
> +#include <drm/drm_atomic_state_helper.h>
>  #include <drm/drm_bridge.h>
>  
>  #include "omapdss.h"
> @@ -564,11 +565,13 @@ venc_bridge_mode_valid(struct drm_bridge *bridge,
>  	}
>  }
>  
> -static bool venc_bridge_mode_fixup(struct drm_bridge *bridge,
> -				   const struct drm_display_mode *mode,
> -				   struct drm_display_mode *adjusted_mode)
> +static int venc_bridge_atomic_check(struct drm_bridge *bridge,
> +				    struct drm_bridge_state *bridge_state,
> +				    struct drm_crtc_state *crtc_state,
> +				    struct drm_connector_state *conn_state)
>  {
>  	const struct drm_display_mode *venc_mode;
> +	struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
>  
>  	switch (venc_get_videomode(adjusted_mode)) {
>  	case VENC_MODE_PAL:
> @@ -580,14 +583,14 @@ static bool venc_bridge_mode_fixup(struct drm_bridge *bridge,
>  		break;
>  
>  	default:
> -		return false;
> +		return -EINVAL;
>  	}
>  
>  	drm_mode_copy(adjusted_mode, venc_mode);
>  	drm_mode_set_crtcinfo(adjusted_mode, CRTC_INTERLACE_HALVE_V);
>  	drm_mode_set_name(adjusted_mode);
>  
> -	return true;
> +	return 0;
>  }
>  
>  static void venc_bridge_mode_set(struct drm_bridge *bridge,
> @@ -613,14 +616,16 @@ static void venc_bridge_mode_set(struct drm_bridge *bridge,
>  	dispc_set_tv_pclk(venc->dss->dispc, 13500000);
>  }
>  
> -static void venc_bridge_enable(struct drm_bridge *bridge)
> +static void venc_bridge_atomic_enable(struct drm_bridge *bridge,
> +				      struct drm_bridge_state *old_bridge_state)
>  {
>  	struct venc_device *venc = drm_bridge_to_venc(bridge);
>  
>  	venc_power_on(venc);
>  }
>  
> -static void venc_bridge_disable(struct drm_bridge *bridge)
> +static void venc_bridge_atomic_disable(struct drm_bridge *bridge,
> +				       struct drm_bridge_state *old_bridge_state)
>  {
>  	struct venc_device *venc = drm_bridge_to_venc(bridge);
>  
> @@ -654,11 +659,14 @@ static int venc_bridge_get_modes(struct drm_bridge *bridge,
>  static const struct drm_bridge_funcs venc_bridge_funcs = {
>  	.attach = venc_bridge_attach,
>  	.mode_valid = venc_bridge_mode_valid,
> -	.mode_fixup = venc_bridge_mode_fixup,
> +	.atomic_check = venc_bridge_atomic_check,
>  	.mode_set = venc_bridge_mode_set,
> -	.enable = venc_bridge_enable,
> -	.disable = venc_bridge_disable,
> +	.atomic_enable = venc_bridge_atomic_enable,
> +	.atomic_disable = venc_bridge_atomic_disable,
>  	.get_modes = venc_bridge_get_modes,
> +	.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,
>  };
>  
>  static void venc_bridge_init(struct venc_device *venc)

-- 
Regards,

Laurent Pinchart

[PATCH v1 11/12] drm/bridge: Drop drm_bridge_funcs.mode_fixup

[Sam Ravnborg]

All users are converted over to drm_bridge_funcs.atomic_check()
so it is safe to drop the mode_fixup support.

Update the comment for atomic_check with relevant parts from mode_fixup.

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
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 |  7 +----
 include/drm/drm_bridge.h     | 60 ++++++++++--------------------------
 2 files changed, 17 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index b6f56d8f3547..3f5acb19957c 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -685,10 +685,6 @@ static int drm_atomic_bridge_check(struct drm_bridge *bridge,
 						  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;
@@ -934,8 +930,7 @@ drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge,
  * @conn_state: new connector state
  *
  * 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,
+ * &drm_bridge_funcs.atomic_check() 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()
  *
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 7496f41535b1..8c93369bcc74 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -106,7 +106,7 @@ struct drm_bridge_funcs {
 	 * to look at anything else but the passed-in mode, and validate it
 	 * against configuration-invariant hardward constraints. Any further
 	 * limits which depend upon the configuration can only be checked in
-	 * @mode_fixup.
+	 * @atomic_check.
 	 *
 	 * RETURNS:
 	 *
@@ -116,46 +116,6 @@ struct drm_bridge_funcs {
 					   const struct drm_display_info *info,
 					   const struct drm_display_mode *mode);
 
-	/**
-	 * @mode_fixup:
-	 *
-	 * This callback is used to validate and adjust a mode. The parameter
-	 * mode is the display mode that should be fed to the next element in
-	 * the display chain, either the final &drm_connector or the next
-	 * &drm_bridge. The parameter adjusted_mode is the input mode the bridge
-	 * requires. It can be modified by this callback and does not need to
-	 * match mode. See also &drm_crtc_state.adjusted_mode for more details.
-	 *
-	 * This is the only hook that allows a bridge to reject a modeset. If
-	 * this function passes all other callbacks must succeed for this
-	 * configuration.
-	 *
-	 * 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:
-	 *
-	 * This function is called in the check phase of atomic modesets, which
-	 * can be aborted for any reason (including on userspace's request to
-	 * just check whether a configuration would be possible). Drivers MUST
-	 * NOT touch any persistent state (hardware or software) or data
-	 * structures except the passed in @state parameter.
-	 *
-	 * Also beware that userspace can request its own custom modes, neither
-	 * core nor helpers filter modes to the list of probe modes reported by
-	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
-	 * that modes are filtered consistently put any bridge constraints and
-	 * limits checks into @mode_valid.
-	 *
-	 * RETURNS:
-	 *
-	 * True if an acceptable configuration is possible, false if the modeset
-	 * operation should be rejected.
-	 */
-	bool (*mode_fixup)(struct drm_bridge *bridge,
-			   const struct drm_display_mode *mode,
-			   struct drm_display_mode *adjusted_mode);
 	/**
 	 * @disable:
 	 *
@@ -466,9 +426,7 @@ struct drm_bridge_funcs {
 	 * &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.
+	 * This method is optional.
 	 *
 	 * If drivers need to tweak &drm_bridge_state.input_bus_cfg.flags or
 	 * &drm_bridge_state.output_bus_cfg.flags it should happen in
@@ -478,6 +436,20 @@ struct drm_bridge_funcs {
 	 * &drm_connector.display_info.bus_flags if the bridge is the last
 	 * element in the chain.
 	 *
+	 * NOTE:
+	 *
+	 * This function is called in the check phase of atomic modesets, which
+	 * can be aborted for any reason (including on userspace's request to
+	 * just check whether a configuration would be possible). Drivers MUST
+	 * NOT touch any persistent state (hardware or software) or data
+	 * structures except the passed in @state parameter.
+	 *
+	 * Also beware that userspace can request its own custom modes, neither
+	 * core nor helpers filter modes to the list of probe modes reported by
+	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
+	 * that modes are filtered consistently put any bridge constraints and
+	 * limits checks into @mode_valid.
+	 *
 	 * RETURNS:
 	 * zero if the check passed, a negative error code otherwise.
 	 */
-- 
2.34.1

Re: [PATCH v1 11/12] drm/bridge: Drop drm_bridge_funcs.mode_fixup

[Dave Stevenson]

On Sun, 17 Jul 2022 at 18:58, Sam Ravnborg <sam@ravnborg.org> wrote:
>
> All users are converted over to drm_bridge_funcs.atomic_check()
> so it is safe to drop the mode_fixup support.
>
> Update the comment for atomic_check with relevant parts from mode_fixup.
>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.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 |  7 +----
>  include/drm/drm_bridge.h     | 60 ++++++++++--------------------------
>  2 files changed, 17 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index b6f56d8f3547..3f5acb19957c 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -685,10 +685,6 @@ static int drm_atomic_bridge_check(struct drm_bridge *bridge,
>                                                   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;
> @@ -934,8 +930,7 @@ drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge,
>   * @conn_state: new connector state
>   *
>   * 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,
> + * &drm_bridge_funcs.atomic_check() 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()
>   *
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 7496f41535b1..8c93369bcc74 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -106,7 +106,7 @@ struct drm_bridge_funcs {
>          * to look at anything else but the passed-in mode, and validate it
>          * against configuration-invariant hardward constraints. Any further
>          * limits which depend upon the configuration can only be checked in
> -        * @mode_fixup.
> +        * @atomic_check.
>          *
>          * RETURNS:
>          *
> @@ -116,46 +116,6 @@ struct drm_bridge_funcs {
>                                            const struct drm_display_info *info,
>                                            const struct drm_display_mode *mode);
>
> -       /**
> -        * @mode_fixup:
> -        *
> -        * This callback is used to validate and adjust a mode. The parameter
> -        * mode is the display mode that should be fed to the next element in
> -        * the display chain, either the final &drm_connector or the next
> -        * &drm_bridge. The parameter adjusted_mode is the input mode the bridge
> -        * requires. It can be modified by this callback and does not need to
> -        * match mode. See also &drm_crtc_state.adjusted_mode for more details.
> -        *
> -        * This is the only hook that allows a bridge to reject a modeset. If
> -        * this function passes all other callbacks must succeed for this
> -        * configuration.
> -        *
> -        * 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:
> -        *
> -        * This function is called in the check phase of atomic modesets, which
> -        * can be aborted for any reason (including on userspace's request to
> -        * just check whether a configuration would be possible). Drivers MUST
> -        * NOT touch any persistent state (hardware or software) or data
> -        * structures except the passed in @state parameter.
> -        *
> -        * Also beware that userspace can request its own custom modes, neither
> -        * core nor helpers filter modes to the list of probe modes reported by
> -        * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
> -        * that modes are filtered consistently put any bridge constraints and
> -        * limits checks into @mode_valid.
> -        *
> -        * RETURNS:
> -        *
> -        * True if an acceptable configuration is possible, false if the modeset
> -        * operation should be rejected.
> -        */
> -       bool (*mode_fixup)(struct drm_bridge *bridge,
> -                          const struct drm_display_mode *mode,
> -                          struct drm_display_mode *adjusted_mode);
>         /**
>          * @disable:
>          *
> @@ -466,9 +426,7 @@ struct drm_bridge_funcs {
>          * &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.
> +        * This method is optional.
>          *
>          * If drivers need to tweak &drm_bridge_state.input_bus_cfg.flags or
>          * &drm_bridge_state.output_bus_cfg.flags it should happen in
> @@ -478,6 +436,20 @@ struct drm_bridge_funcs {
>          * &drm_connector.display_info.bus_flags if the bridge is the last
>          * element in the chain.
>          *
> +        * NOTE:
> +        *
> +        * This function is called in the check phase of atomic modesets, which
> +        * can be aborted for any reason (including on userspace's request to
> +        * just check whether a configuration would be possible). Drivers MUST
> +        * NOT touch any persistent state (hardware or software) or data
> +        * structures except the passed in @state parameter.
> +        *
> +        * Also beware that userspace can request its own custom modes, neither
> +        * core nor helpers filter modes to the list of probe modes reported by
> +        * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
> +        * that modes are filtered consistently put any bridge constraints and
> +        * limits checks into @mode_valid.
> +        *
>          * RETURNS:
>          * zero if the check passed, a negative error code otherwise.
>          */
> --
> 2.34.1
>

Re: [PATCH v1 11/12] drm/bridge: Drop drm_bridge_funcs.mode_fixup

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Sun, Jul 17, 2022 at 07:58:00PM +0200, Sam Ravnborg wrote:
> All users are converted over to drm_bridge_funcs.atomic_check()
> so it is safe to drop the mode_fixup support.
> 
> Update the comment for atomic_check with relevant parts from mode_fixup.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> 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>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  drivers/gpu/drm/drm_bridge.c |  7 +----
>  include/drm/drm_bridge.h     | 60 ++++++++++--------------------------
>  2 files changed, 17 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index b6f56d8f3547..3f5acb19957c 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -685,10 +685,6 @@ static int drm_atomic_bridge_check(struct drm_bridge *bridge,
>  						  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;
> @@ -934,8 +930,7 @@ drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge,
>   * @conn_state: new connector state
>   *
>   * 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,
> + * &drm_bridge_funcs.atomic_check() 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()
>   *
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 7496f41535b1..8c93369bcc74 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -106,7 +106,7 @@ struct drm_bridge_funcs {
>  	 * to look at anything else but the passed-in mode, and validate it
>  	 * against configuration-invariant hardward constraints. Any further
>  	 * limits which depend upon the configuration can only be checked in
> -	 * @mode_fixup.
> +	 * @atomic_check.
>  	 *
>  	 * RETURNS:
>  	 *
> @@ -116,46 +116,6 @@ struct drm_bridge_funcs {
>  					   const struct drm_display_info *info,
>  					   const struct drm_display_mode *mode);
>  
> -	/**
> -	 * @mode_fixup:
> -	 *
> -	 * This callback is used to validate and adjust a mode. The parameter
> -	 * mode is the display mode that should be fed to the next element in
> -	 * the display chain, either the final &drm_connector or the next
> -	 * &drm_bridge. The parameter adjusted_mode is the input mode the bridge
> -	 * requires. It can be modified by this callback and does not need to
> -	 * match mode. See also &drm_crtc_state.adjusted_mode for more details.
> -	 *
> -	 * This is the only hook that allows a bridge to reject a modeset. If
> -	 * this function passes all other callbacks must succeed for this
> -	 * configuration.
> -	 *
> -	 * 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:
> -	 *
> -	 * This function is called in the check phase of atomic modesets, which
> -	 * can be aborted for any reason (including on userspace's request to
> -	 * just check whether a configuration would be possible). Drivers MUST
> -	 * NOT touch any persistent state (hardware or software) or data
> -	 * structures except the passed in @state parameter.
> -	 *
> -	 * Also beware that userspace can request its own custom modes, neither
> -	 * core nor helpers filter modes to the list of probe modes reported by
> -	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
> -	 * that modes are filtered consistently put any bridge constraints and
> -	 * limits checks into @mode_valid.
> -	 *
> -	 * RETURNS:
> -	 *
> -	 * True if an acceptable configuration is possible, false if the modeset
> -	 * operation should be rejected.
> -	 */
> -	bool (*mode_fixup)(struct drm_bridge *bridge,
> -			   const struct drm_display_mode *mode,
> -			   struct drm_display_mode *adjusted_mode);
>  	/**
>  	 * @disable:
>  	 *
> @@ -466,9 +426,7 @@ struct drm_bridge_funcs {
>  	 * &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.
> +	 * This method is optional.
>  	 *
>  	 * If drivers need to tweak &drm_bridge_state.input_bus_cfg.flags or
>  	 * &drm_bridge_state.output_bus_cfg.flags it should happen in
> @@ -478,6 +436,20 @@ struct drm_bridge_funcs {
>  	 * &drm_connector.display_info.bus_flags if the bridge is the last
>  	 * element in the chain.
>  	 *
> +	 * NOTE:
> +	 *
> +	 * This function is called in the check phase of atomic modesets, which
> +	 * can be aborted for any reason (including on userspace's request to
> +	 * just check whether a configuration would be possible). Drivers MUST
> +	 * NOT touch any persistent state (hardware or software) or data
> +	 * structures except the passed in @state parameter.
> +	 *
> +	 * Also beware that userspace can request its own custom modes, neither
> +	 * core nor helpers filter modes to the list of probe modes reported by
> +	 * the GETCONNECTOR IOCTL and stored in &drm_connector.modes. To ensure
> +	 * that modes are filtered consistently put any bridge constraints and
> +	 * limits checks into @mode_valid.
> +	 *
>  	 * RETURNS:
>  	 * zero if the check passed, a negative error code otherwise.
>  	 */

-- 
Regards,

Laurent Pinchart

[PATCH v1 12/12] drm/todo: Add bridge related todo items

[Sam Ravnborg]

Add todo in the hope someone will help updating the bridge drivers.

v2:
  - Updated descriptions in todo.rst

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Acked-by: Maxime Ripard <mripard@kernel.org>
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 | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 10bfb50908d1..fbcc232e0bc1 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -480,6 +480,26 @@ Contact: Thomas Zimmermann <tzimmermann@suse.de>
 
 Level: Starter
 
+Drop use of deprecated operations in bridge drivers
+--------------------------------------------------
+
+&struct drm_bridge_funcs contains a number of deprecated operations
+which can be replaced by the atomic variants.
+
+The following is more or less 1:1 replacements with 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.
+
+Contact: bridge maintainers, Sam Ravnborg <sam@ravnborg.org>,
+         Laurent Pinchart <laurent.pinchart@ideasonboard.com>
+
+Level: Beginner or intermediate (depending on the driver)
 
 Core refactorings
 =================
-- 
2.34.1

Re: [PATCH v1 12/12] drm/todo: Add bridge related todo items

[Dave Stevenson]

Hi Sam

On Sun, 17 Jul 2022 at 18:58, Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Add todo in the hope someone will help updating the bridge drivers.
>
> v2:
>   - Updated descriptions in todo.rst
>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Acked-by: Maxime Ripard <mripard@kernel.org>
> 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 | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 10bfb50908d1..fbcc232e0bc1 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -480,6 +480,26 @@ Contact: Thomas Zimmermann <tzimmermann@suse.de>
>
>  Level: Starter
>
> +Drop use of deprecated operations in bridge drivers
> +--------------------------------------------------
> +
> +&struct drm_bridge_funcs contains a number of deprecated operations
> +which can be replaced by the atomic variants.
> +
> +The following is more or less 1:1 replacements with 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.

The dw-mipi-dsi and msm DSI host controller bridge drivers are
currently relying on mode_set as a convenient hook to power up the DSI
host prior to pre_enable of the bridge chain. Removing it is therefore
going to break those.

There is a proposed mechanism to allow for the removal of this hack
[1], but it's still waiting on R-B tags and/or discussion from bridge
maintainers (gentle nudge as you are one of those maintainers).

And do you mean merge with atomic_enable, or merge with
atomic_pre_enable? atomic_pre_enable would be more applicable for
almost all the bridges I'm aware of as they want to be configured
before video starts.

Cheers,
  Dave

[1] https://lists.freedesktop.org/archives/dri-devel/2022-March/345466.html

> +Contact: bridge maintainers, Sam Ravnborg <sam@ravnborg.org>,
> +         Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> +
> +Level: Beginner or intermediate (depending on the driver)
>
>  Core refactorings
>  =================
> --
> 2.34.1
>

Re: [PATCH v1 12/12] drm/todo: Add bridge related todo items

[Sam Ravnborg]

Hi Dave,

On Mon, Jul 18, 2022 at 11:27:37AM +0100, Dave Stevenson wrote:
> Hi Sam
> 
> On Sun, 17 Jul 2022 at 18:58, Sam Ravnborg <sam@ravnborg.org> wrote:
> >
> > Add todo in the hope someone will help updating the bridge drivers.
> >
> > v2:
> >   - Updated descriptions in todo.rst
> >
> > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > Acked-by: Maxime Ripard <mripard@kernel.org>
> > 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 | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> > index 10bfb50908d1..fbcc232e0bc1 100644
> > --- a/Documentation/gpu/todo.rst
> > +++ b/Documentation/gpu/todo.rst
> > @@ -480,6 +480,26 @@ Contact: Thomas Zimmermann <tzimmermann@suse.de>
> >
> >  Level: Starter
> >
> > +Drop use of deprecated operations in bridge drivers
> > +--------------------------------------------------
> > +
> > +&struct drm_bridge_funcs contains a number of deprecated operations
> > +which can be replaced by the atomic variants.
> > +
> > +The following is more or less 1:1 replacements with 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.
> 
> The dw-mipi-dsi and msm DSI host controller bridge drivers are
> currently relying on mode_set as a convenient hook to power up the DSI
> host prior to pre_enable of the bridge chain. Removing it is therefore
> going to break those.
> 
> There is a proposed mechanism to allow for the removal of this hack
> [1], but it's still waiting on R-B tags and/or discussion from bridge
> maintainers (gentle nudge as you are one of those maintainers).
I have over time gained some knowledge of how bridges works and have
applied a few patches - but the maintainer role belongs to others.
I just try to help a bit.

I will review the referenced series - could you then in return
review this series?
> 
> And do you mean merge with atomic_enable, or merge with
> atomic_pre_enable? atomic_pre_enable would be more applicable for
> almost all the bridges I'm aware of as they want to be configured
> before video starts.
Thanks, yes you are right. I will update the text to refer to
pre_enable as this is often the right choice. Looking at how many
bridges who implements mode_set, or are not yet atomic, this will
take a while before we can drop it.

	Sam

Re: [PATCH v1 12/12] drm/todo: Add bridge related todo items

[Dave Stevenson]

Hi Sam

On Mon, 18 Jul 2022 at 19:00, Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Dave,
>
> On Mon, Jul 18, 2022 at 11:27:37AM +0100, Dave Stevenson wrote:
> > Hi Sam
> >
> > On Sun, 17 Jul 2022 at 18:58, Sam Ravnborg <sam@ravnborg.org> wrote:
> > >
> > > Add todo in the hope someone will help updating the bridge drivers.
> > >
> > > v2:
> > >   - Updated descriptions in todo.rst
> > >
> > > Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> > > Acked-by: Maxime Ripard <mripard@kernel.org>
> > > 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 | 20 ++++++++++++++++++++
> > >  1 file changed, 20 insertions(+)
> > >
> > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> > > index 10bfb50908d1..fbcc232e0bc1 100644
> > > --- a/Documentation/gpu/todo.rst
> > > +++ b/Documentation/gpu/todo.rst
> > > @@ -480,6 +480,26 @@ Contact: Thomas Zimmermann <tzimmermann@suse.de>
> > >
> > >  Level: Starter
> > >
> > > +Drop use of deprecated operations in bridge drivers
> > > +--------------------------------------------------
> > > +
> > > +&struct drm_bridge_funcs contains a number of deprecated operations
> > > +which can be replaced by the atomic variants.
> > > +
> > > +The following is more or less 1:1 replacements with 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.
> >
> > The dw-mipi-dsi and msm DSI host controller bridge drivers are
> > currently relying on mode_set as a convenient hook to power up the DSI
> > host prior to pre_enable of the bridge chain. Removing it is therefore
> > going to break those.
> >
> > There is a proposed mechanism to allow for the removal of this hack
> > [1], but it's still waiting on R-B tags and/or discussion from bridge
> > maintainers (gentle nudge as you are one of those maintainers).
>
> I have over time gained some knowledge of how bridges works and have
> applied a few patches - but the maintainer role belongs to others.
> I just try to help a bit.

Apologies, I'd misread the text in this patch
+Contact: bridge maintainers, Sam Ravnborg <sam@ravnborg.org>,
+         Laurent Pinchart <laurent.pinchart@ideasonboard.com>
as being that you and Laurent were the bridge maintainers. Colon
instead of comma :-/

> I will review the referenced series - could you then in return
> review this series?

Sure, these look to be within my levels of knowledge.

> >
> > And do you mean merge with atomic_enable, or merge with
> > atomic_pre_enable? atomic_pre_enable would be more applicable for
> > almost all the bridges I'm aware of as they want to be configured
> > before video starts.
> Thanks, yes you are right. I will update the text to refer to
> pre_enable as this is often the right choice. Looking at how many
> bridges who implements mode_set, or are not yet atomic, this will
> take a while before we can drop it.

Thanks. That makes more logical sense to me for the majority of cases.
As to timescales, it always takes longer than ideal to migrate older drivers.

Thanks
  Dave

Re: [PATCH v1 08/12] drm/mediatek: Drop mtk_hdmi_bridge_mode_fixup

[Dave Stevenson]

On Sun, 17 Jul 2022 at 18:58, Sam Ravnborg <sam@ravnborg.org> wrote:
>
> The implementation of drm_bridge_funcs.mode_fixup is optional
> so there is no need to provide an empty implementation.
> Drop mtk_hdmi_bridge_mode_fixup() so the driver no longer uses the
> deprecated drm_bridge_funcs.mode_fixup() operation.
>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

> Cc: Chun-Kuang Hu <chunkuang.hu@kernel.org>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Matthias Brugger <matthias.bgg@gmail.com>
> Cc: linux-mediatek@lists.infradead.org
> Cc: linux-arm-kernel@lists.infradead.org
> ---
>  drivers/gpu/drm/mediatek/mtk_hdmi.c | 8 --------
>  1 file changed, 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> index a63b76055f81..7321aa1ee6f0 100644
> --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> @@ -1293,13 +1293,6 @@ static int mtk_hdmi_bridge_attach(struct drm_bridge *bridge,
>         return 0;
>  }
>
> -static bool mtk_hdmi_bridge_mode_fixup(struct drm_bridge *bridge,
> -                                      const struct drm_display_mode *mode,
> -                                      struct drm_display_mode *adjusted_mode)
> -{
> -       return true;
> -}
> -
>  static void mtk_hdmi_bridge_atomic_disable(struct drm_bridge *bridge,
>                                            struct drm_bridge_state *old_bridge_state)
>  {
> @@ -1399,7 +1392,6 @@ static const struct drm_bridge_funcs mtk_hdmi_bridge_funcs = {
>         .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>         .atomic_reset = drm_atomic_helper_bridge_reset,
>         .attach = mtk_hdmi_bridge_attach,
> -       .mode_fixup = mtk_hdmi_bridge_mode_fixup,
>         .atomic_disable = mtk_hdmi_bridge_atomic_disable,
>         .atomic_post_disable = mtk_hdmi_bridge_atomic_post_disable,
>         .mode_set = mtk_hdmi_bridge_mode_set,
> --
> 2.34.1
>

Re: [PATCH v1 08/12] drm/mediatek: Drop mtk_hdmi_bridge_mode_fixup

[Chun-Kuang Hu]

Hi, Sam:

Sam Ravnborg <sam@ravnborg.org> 於 2022年7月18日 週一 凌晨1:58寫道：
>
> The implementation of drm_bridge_funcs.mode_fixup is optional
> so there is no need to provide an empty implementation.
> Drop mtk_hdmi_bridge_mode_fixup() so the driver no longer uses the
> deprecated drm_bridge_funcs.mode_fixup() operation.

Acked-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>

>
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Chun-Kuang Hu <chunkuang.hu@kernel.org>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Matthias Brugger <matthias.bgg@gmail.com>
> Cc: linux-mediatek@lists.infradead.org
> Cc: linux-arm-kernel@lists.infradead.org
> ---
>  drivers/gpu/drm/mediatek/mtk_hdmi.c | 8 --------
>  1 file changed, 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> index a63b76055f81..7321aa1ee6f0 100644
> --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> @@ -1293,13 +1293,6 @@ static int mtk_hdmi_bridge_attach(struct drm_bridge *bridge,
>         return 0;
>  }
>
> -static bool mtk_hdmi_bridge_mode_fixup(struct drm_bridge *bridge,
> -                                      const struct drm_display_mode *mode,
> -                                      struct drm_display_mode *adjusted_mode)
> -{
> -       return true;
> -}
> -
>  static void mtk_hdmi_bridge_atomic_disable(struct drm_bridge *bridge,
>                                            struct drm_bridge_state *old_bridge_state)
>  {
> @@ -1399,7 +1392,6 @@ static const struct drm_bridge_funcs mtk_hdmi_bridge_funcs = {
>         .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>         .atomic_reset = drm_atomic_helper_bridge_reset,
>         .attach = mtk_hdmi_bridge_attach,
> -       .mode_fixup = mtk_hdmi_bridge_mode_fixup,
>         .atomic_disable = mtk_hdmi_bridge_atomic_disable,
>         .atomic_post_disable = mtk_hdmi_bridge_atomic_post_disable,
>         .mode_set = mtk_hdmi_bridge_mode_set,
> --
> 2.34.1
>

Re: [PATCH v1 08/12] drm/mediatek: Drop mtk_hdmi_bridge_mode_fixup

[Laurent Pinchart]

Hi Sam,

Thank you for the patch.

On Sun, Jul 17, 2022 at 07:57:57PM +0200, Sam Ravnborg wrote:
> The implementation of drm_bridge_funcs.mode_fixup is optional
> so there is no need to provide an empty implementation.
> Drop mtk_hdmi_bridge_mode_fixup() so the driver no longer uses the
> deprecated drm_bridge_funcs.mode_fixup() operation.
> 
> Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
> Cc: Chun-Kuang Hu <chunkuang.hu@kernel.org>
> Cc: Philipp Zabel <p.zabel@pengutronix.de>
> Cc: Matthias Brugger <matthias.bgg@gmail.com>
> Cc: linux-mediatek@lists.infradead.org
> Cc: linux-arm-kernel@lists.infradead.org

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
>  drivers/gpu/drm/mediatek/mtk_hdmi.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> index a63b76055f81..7321aa1ee6f0 100644
> --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> @@ -1293,13 +1293,6 @@ static int mtk_hdmi_bridge_attach(struct drm_bridge *bridge,
>  	return 0;
>  }
>  
> -static bool mtk_hdmi_bridge_mode_fixup(struct drm_bridge *bridge,
> -				       const struct drm_display_mode *mode,
> -				       struct drm_display_mode *adjusted_mode)
> -{
> -	return true;
> -}
> -
>  static void mtk_hdmi_bridge_atomic_disable(struct drm_bridge *bridge,
>  					   struct drm_bridge_state *old_bridge_state)
>  {
> @@ -1399,7 +1392,6 @@ static const struct drm_bridge_funcs mtk_hdmi_bridge_funcs = {
>  	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>  	.atomic_reset = drm_atomic_helper_bridge_reset,
>  	.attach = mtk_hdmi_bridge_attach,
> -	.mode_fixup = mtk_hdmi_bridge_mode_fixup,
>  	.atomic_disable = mtk_hdmi_bridge_atomic_disable,
>  	.atomic_post_disable = mtk_hdmi_bridge_atomic_post_disable,
>  	.mode_set = mtk_hdmi_bridge_mode_set,

-- 
Regards,

Laurent Pinchart