Re: [PATCH v12 2/5] drm/atomic-helper: Separate out bridge pre_enable/post_disable from enable/disable

[Thomas Zimmermann <tzimmermann@suse.de>]

Am 06.04.25 um 15:16 schrieb Aradhya Bhatia:
> The encoder-bridge ops occur by looping over the new connector states of
> the display pipelines. The enable sequence runs as follows -
>
> 	- pre_enable(bridge),
> 	- enable(encoder),
> 	- enable(bridge),
>
> while the disable sequnce runs as follows -
>
> 	- disable(bridge),
> 	- disable(encoder),
> 	- post_disable(bridge).
>
> Separate out the pre_enable(bridge), and the post_disable(bridge)
> operations into separate functions each.
>
> This patch keeps the sequence same for any singular disaplay pipe, but
> changes the sequence across multiple display pipelines.
>
> This patch is meant to be an interim patch, to cleanly pave the way for
> the sequence re-ordering patch, and maintain bisectability in the
> process.
>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Reviewed-by: Jayesh Choudhary <j-choudhary@ti.com>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Tested-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> Tested-by: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
> ---
>   drivers/gpu/drm/drm_atomic_helper.c | 91 ++++++++++++++++++++++++++++-
>   1 file changed, 88 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index d185486071c5..86824f769623 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1223,8 +1223,6 @@ encoder_bridge_disable(struct drm_device *dev, struct drm_atomic_state *state)
>   			else if (funcs->dpms)
>   				funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
>   		}
> -
> -		drm_atomic_bridge_chain_post_disable(bridge, state);
>   	}
>   }
>   
> @@ -1281,11 +1279,65 @@ crtc_disable(struct drm_device *dev, struct drm_atomic_state *state)
>   	}
>   }
>   
> +static void
> +encoder_bridge_post_disable(struct drm_device *dev, struct drm_atomic_state *state)
> +{
> +	struct drm_connector *connector;
> +	struct drm_connector_state *old_conn_state, *new_conn_state;
> +	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> +	int i;
> +
> +	for_each_oldnew_connector_in_state(state, connector, old_conn_state, new_conn_state, i) {
> +		struct drm_encoder *encoder;
> +		struct drm_bridge *bridge;
> +
> +		/*
> +		 * Shut down everything that's in the changeset and currently
> +		 * still on. So need to check the old, saved state.
> +		 */
> +		if (!old_conn_state->crtc)
> +			continue;
> +
> +		old_crtc_state = drm_atomic_get_old_crtc_state(state, old_conn_state->crtc);
> +
> +		if (new_conn_state->crtc)
> +			new_crtc_state = drm_atomic_get_new_crtc_state(
> +						state,
> +						new_conn_state->crtc);

Indention is odd here. You can use up to 100 chars per line.

> +		else
> +			new_crtc_state = NULL;
> +
> +		if (!crtc_needs_disable(old_crtc_state, new_crtc_state) ||
> +		    !drm_atomic_crtc_needs_modeset(old_conn_state->crtc->state))
> +			continue;
> +
> +		encoder = old_conn_state->best_encoder;
> +
> +		/* We shouldn't get this far if we didn't previously have
> +		 * an encoder.. but WARN_ON() rather than explode.
> +		 */

Comment style should use an empty line first

  /*
   * foo bla
   */

With style issues fixed:

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

> +		if (WARN_ON(!encoder))
> +			continue;
> +
> +		drm_dbg_atomic(dev, "post-disabling bridges [ENCODER:%d:%s]\n",
> +			       encoder->base.id, encoder->name);
> +
> +		/*
> +		 * Each encoder has at most one connector (since we always steal
> +		 * it away), so we won't call disable hooks twice.
> +		 */
> +		bridge = drm_bridge_chain_get_first_bridge(encoder);
> +		drm_atomic_bridge_chain_post_disable(bridge, state);
> +	}
> +}
> +
>   static void
>   disable_outputs(struct drm_device *dev, struct drm_atomic_state *state)
>   {
>   	encoder_bridge_disable(dev, state);
>   
> +	encoder_bridge_post_disable(dev, state);
> +
>   	crtc_disable(dev, state);
>   }
>   
> @@ -1498,6 +1550,38 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
>   	}
>   }
>   
> +static void
> +encoder_bridge_pre_enable(struct drm_device *dev, struct drm_atomic_state *state)
> +{
> +	struct drm_connector *connector;
> +	struct drm_connector_state *new_conn_state;
> +	int i;
> +
> +	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> +		struct drm_encoder *encoder;
> +		struct drm_bridge *bridge;
> +
> +		if (!new_conn_state->best_encoder)
> +			continue;
> +
> +		if (!new_conn_state->crtc->state->active ||
> +		    !drm_atomic_crtc_needs_modeset(new_conn_state->crtc->state))
> +			continue;
> +
> +		encoder = new_conn_state->best_encoder;
> +
> +		drm_dbg_atomic(dev, "pre-enabling bridges [ENCODER:%d:%s]\n",
> +			       encoder->base.id, encoder->name);
> +
> +		/*
> +		 * Each encoder has at most one connector (since we always steal
> +		 * it away), so we won't call enable hooks twice.
> +		 */
> +		bridge = drm_bridge_chain_get_first_bridge(encoder);
> +		drm_atomic_bridge_chain_pre_enable(bridge, state);
> +	}
> +}
> +
>   static void
>   crtc_enable(struct drm_device *dev, struct drm_atomic_state *state)
>   {
> @@ -1559,7 +1643,6 @@ encoder_bridge_enable(struct drm_device *dev, struct drm_atomic_state *state)
>   		 * it away), so we won't call enable hooks twice.
>   		 */
>   		bridge = drm_bridge_chain_get_first_bridge(encoder);
> -		drm_atomic_bridge_chain_pre_enable(bridge, state);
>   
>   		if (funcs) {
>   			if (funcs->atomic_enable)
> @@ -1593,6 +1676,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>   {
>   	crtc_enable(dev, state);
>   
> +	encoder_bridge_pre_enable(dev, state);
> +
>   	encoder_bridge_enable(dev, state);
>   
>   	drm_atomic_helper_commit_writebacks(dev, state);

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)