The drm_atomic_bridge_check() runs the atomic_check callback if needed,
or falls back to mode_fixup if it's not there.

Going forward, we'll need to setup the bridge atomic state even though
drivers might not be implementing atomic_check at all.

We can thus switch to using drm_bridge_is_atomic() to take the atomic
path, and do additional things there in the future, or go the mode_fixup
route.

Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/drm_bridge.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index d2525d0b1324cc3a63e32f5bf6ca6c4f9034eb4e..b6d24092674c8fa33d9b6ebab9ece0f91fb8f8ea 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -803,23 +803,25 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);
 
 static int drm_atomic_bridge_check(struct drm_bridge *bridge,
 				   struct drm_crtc_state *crtc_state,
 				   struct drm_connector_state *conn_state)
 {
-	if (bridge->funcs->atomic_check) {
+	if (drm_bridge_is_atomic(bridge)) {
 		struct drm_bridge_state *bridge_state;
 		int ret;
 
 		bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state,
 							       bridge);
 		if (WARN_ON(!bridge_state))
 			return -EINVAL;
 
-		ret = bridge->funcs->atomic_check(bridge, bridge_state,
-						  crtc_state, conn_state);
-		if (ret)
-			return ret;
+		if (bridge->funcs->atomic_check) {
+			ret = bridge->funcs->atomic_check(bridge, bridge_state,
+							  crtc_state, conn_state);
+			if (ret)
+				return ret;
+		}
 	} else if (bridge->funcs->mode_fixup) {
 		if (!bridge->funcs->mode_fixup(bridge, &crtc_state->mode,
 					       &crtc_state->adjusted_mode))
 			return -EINVAL;
 	}

-- 
2.48.0

On Thu, Feb 13, 2025 at 03:43:49PM +0100, Maxime Ripard wrote:
> The drm_atomic_bridge_check() runs the atomic_check callback if needed,
> or falls back to mode_fixup if it's not there.
> 
> Going forward, we'll need to setup the bridge atomic state even though
> drivers might not be implementing atomic_check at all.
> 
> We can thus switch to using drm_bridge_is_atomic() to take the atomic
> path, and do additional things there in the future, or go the mode_fixup
> route.

This will break bridges like tc358768, rcar_lvds and mtk_hdmi: they
implement atomic_reset() and mode_fixup().

> 
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  drivers/gpu/drm/drm_bridge.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index d2525d0b1324cc3a63e32f5bf6ca6c4f9034eb4e..b6d24092674c8fa33d9b6ebab9ece0f91fb8f8ea 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -803,23 +803,25 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);
>  
>  static int drm_atomic_bridge_check(struct drm_bridge *bridge,
>  				   struct drm_crtc_state *crtc_state,
>  				   struct drm_connector_state *conn_state)
>  {
> -	if (bridge->funcs->atomic_check) {
> +	if (drm_bridge_is_atomic(bridge)) {
>  		struct drm_bridge_state *bridge_state;
>  		int ret;
>  
>  		bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state,
>  							       bridge);
>  		if (WARN_ON(!bridge_state))
>  			return -EINVAL;
>  
> -		ret = bridge->funcs->atomic_check(bridge, bridge_state,
> -						  crtc_state, conn_state);
> -		if (ret)
> -			return ret;
> +		if (bridge->funcs->atomic_check) {
> +			ret = bridge->funcs->atomic_check(bridge, bridge_state,
> +							  crtc_state, conn_state);
> +			if (ret)
> +				return ret;
> +		}
>  	} else if (bridge->funcs->mode_fixup) {
>  		if (!bridge->funcs->mode_fixup(bridge, &crtc_state->mode,
>  					       &crtc_state->adjusted_mode))
>  			return -EINVAL;
>  	}
> 
> -- 
> 2.48.0
> 

-- 
With best wishes
Dmitry

On Thu, Feb 13, 2025 at 06:29:46PM +0200, Dmitry Baryshkov wrote:
> On Thu, Feb 13, 2025 at 03:43:49PM +0100, Maxime Ripard wrote:
> > The drm_atomic_bridge_check() runs the atomic_check callback if needed,
> > or falls back to mode_fixup if it's not there.
> > 
> > Going forward, we'll need to setup the bridge atomic state even though
> > drivers might not be implementing atomic_check at all.
> > 
> > We can thus switch to using drm_bridge_is_atomic() to take the atomic
> > path, and do additional things there in the future, or go the mode_fixup
> > route.
> 
> This will break bridges like tc358768, rcar_lvds and mtk_hdmi: they
> implement atomic_reset() and mode_fixup().

What is your suggestion then? I kind of did what you were suggesting on
patch 1, but it wasn't working. Then you want me to roll back to that,
or something else?

Maxime

On Fri, Feb 14, 2025 at 01:59:01PM +0100, Maxime Ripard wrote:
> On Thu, Feb 13, 2025 at 06:29:46PM +0200, Dmitry Baryshkov wrote:
> > On Thu, Feb 13, 2025 at 03:43:49PM +0100, Maxime Ripard wrote:
> > > The drm_atomic_bridge_check() runs the atomic_check callback if needed,
> > > or falls back to mode_fixup if it's not there.
> > > 
> > > Going forward, we'll need to setup the bridge atomic state even though
> > > drivers might not be implementing atomic_check at all.
> > > 
> > > We can thus switch to using drm_bridge_is_atomic() to take the atomic
> > > path, and do additional things there in the future, or go the mode_fixup
> > > route.
> > 
> > This will break bridges like tc358768, rcar_lvds and mtk_hdmi: they
> > implement atomic_reset() and mode_fixup().
> 
> What is your suggestion then? I kind of did what you were suggesting on
> patch 1, but it wasn't working. Then you want me to roll back to that,
> or something else?

Well, we just need to call mode_fixup. So something like:

	if (drm_bridge_is_atomic(bridge)) {
		bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state,
							       bridge);
		if (WARN_ON(!bridge_state))
			return -EINVAL;
	}

	if (bridge->funcs->atomic_check) {
		/* drm_bridge_add() returns void, so there is no way to
		 * reject non-atomic bridges with atomic_check()
		 * callback.
		 */
		if (!bridge_state)
			return -EINVAL;
		ret = bridge->funcs->atomic_check(bridge, bridge_state,
						  crtc_state, conn_state);
		if (ret)
			return ret;
	} else if (bridge->funcs->mode_fixup) {
		if (!bridge->funcs->mode_fixup(bridge, &crtc_state->mode,
					       &crtc_state->adjusted_mode))
			return -EINVAL;
	}



-- 
With best wishes
Dmitry