On Sun, Oct 20, 2024 at 01:35:28AM +0530, Aradhya Bhatia wrote:
> From: Aradhya Bhatia <a-bhatia1@xxxxxx>
>
> The way any singular display pipeline, in need of a modeset, gets
> enabled is as follows -
>
> CRTC Enable
> All Bridge Pre-Enable
> Encoder Enable
> All Bridge Enable
>
> - and the disable path is exactly the reverse of this.
>
> The CRTC enable/disable occurs by looping over the old and new CRTC
> states, while the bridge pre-enable, encoder enable, and bridge enable
> occur by looping through the new connector states of the display
> pipelines.
>
> At the moment, the encoder and bridge operations are grouped together
> and occur under the same loops.
>
> Separate out the enable/disable loops of the encoder and bridge
> operations into a different function, to make way for the re-ordering of
> the enable and disable sequence of all these display elements.
>
> This patch doesn't alter in any way the ordering of CRTC/encoder/bridge
> enable and disable, but merely helps to cleanly set up for the next
> patch and to make sure that the bisectibility remains intact.
>
> Signed-off-by: Aradhya Bhatia <a-bhatia1@xxxxxx>
> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@xxxxxxxxx>
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 97 +++++++++++++++++------------
> 1 file changed, 57 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 5186d2114a50..7741fbcc8fc7 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1122,11 +1122,10 @@ crtc_needs_disable(struct drm_crtc_state *old_state,
> }
>
> static void
> -disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> +encoder_bridge_chain_disable(struct drm_device *dev, struct drm_atomic_state *old_state)
> {
> struct drm_connector *connector;
> struct drm_connector_state *old_conn_state, *new_conn_state;
> - struct drm_crtc *crtc;
> struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> int i;
>
> @@ -1189,6 +1188,16 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>
> drm_atomic_bridge_chain_post_disable(bridge, old_state);
> }
> +}
> +
> +static void
> +disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> +{
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> + int i;
> +
> + encoder_bridge_chain_disable(dev, old_state);
>
> for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
> const struct drm_crtc_helper_funcs *funcs;
> @@ -1445,6 +1454,51 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
> }
> }
>
> +static void
> +encoder_bridge_chain_enable(struct drm_device *dev, struct drm_atomic_state *old_state)
> +{
> + struct drm_connector *connector;
> + struct drm_connector_state *new_conn_state;
> + int i;
> +
> + for_each_new_connector_in_state(old_state, connector, new_conn_state, i) {
> + const struct drm_encoder_helper_funcs *funcs;
> + 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;
> + funcs = encoder->helper_private;
> +
> + drm_dbg_atomic(dev, "enabling [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, old_state);
> +
> + if (funcs) {
> + if (funcs->atomic_enable)
> + funcs->atomic_enable(encoder, old_state);
> + else if (funcs->enable)
> + funcs->enable(encoder);
> + else if (funcs->commit)
> + funcs->commit(encoder);
> + }
> +
> + drm_atomic_bridge_chain_enable(bridge, old_state);
> + }
> +}
> +
> /**
> * drm_atomic_helper_commit_modeset_enables - modeset commit to enable outputs
> * @dev: DRM device
> @@ -1465,8 +1519,6 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
> struct drm_crtc *crtc;
> struct drm_crtc_state *old_crtc_state;
> struct drm_crtc_state *new_crtc_state;
> - struct drm_connector *connector;
> - struct drm_connector_state *new_conn_state;
> int i;
>
> for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
> @@ -1491,42 +1543,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
> }
> }
I'd say, if you want for it to be cleaner, split both loops from
drm_atomic_helper_commit_modeset_enables() and call them separately.
This save you from the code move, which is harder to review (and also
helps with the function complexity).
>
> - for_each_new_connector_in_state(old_state, connector, new_conn_state, i) {
> - const struct drm_encoder_helper_funcs *funcs;
> - 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;
> - funcs = encoder->helper_private;
> -
> - drm_dbg_atomic(dev, "enabling [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, old_state);
> -
> - if (funcs) {
> - if (funcs->atomic_enable)
> - funcs->atomic_enable(encoder, old_state);
> - else if (funcs->enable)
> - funcs->enable(encoder);
> - else if (funcs->commit)
> - funcs->commit(encoder);
> - }
> -
> - drm_atomic_bridge_chain_enable(bridge, old_state);
> - }
> + encoder_bridge_chain_enable(dev, old_state);
>
> drm_atomic_helper_commit_writebacks(dev, old_state);
> }
> --
> 2.34.1
>
--
With best wishes
Dmitry
