On Wed, Jun 26, 2024 at 02:28:57PM GMT, Tomi Valkeinen wrote:
> On 22/06/2024 14:09, Aradhya Bhatia wrote:
> > Move the bridge pre_enable call before crtc enable, and the bridge
> > post_disable call after the crtc disable.
> >
> > The sequence of enable after this patch will look like:
> >
> > bridge[n]_pre_enable
> > ...
> > bridge[1]_pre_enable
> >
> > crtc_enable
> > encoder_enable
> >
> > bridge[1]_enable
> > ...
> > bridge[n]__enable
> >
> > and vice-versa for the bridge chain disable sequence.
> >
> > The definition of bridge pre_enable hook says that,
> > "The display pipe (i.e. clocks and timing signals) feeding this bridge
> > will not yet be running when this callback is called".
> >
> > Since CRTC is also a source feeding the bridge, it should not be enabled
> > before the bridges in the pipeline are pre_enabled. Fix that by
> > re-ordering the sequence of bridge pre_enable and bridge post_disable.
> >
> > Signed-off-by: Aradhya Bhatia <a-bhatia1@xxxxxx>
> > ---
> > drivers/gpu/drm/drm_atomic_helper.c | 165 ++++++++++++++++++----------
> > include/drm/drm_atomic_helper.h | 7 ++
> > 2 files changed, 114 insertions(+), 58 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index fb97b51b38f1..e8ad08634f58 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -74,6 +74,7 @@
> > * also shares the &struct drm_plane_helper_funcs function table with the plane
> > * helpers.
> > */
> > +
>
> Extra change.
>
> > static void
> > drm_atomic_helper_plane_changed(struct drm_atomic_state *state,
> > struct drm_plane_state *old_plane_state,
> > @@ -1122,11 +1123,11 @@ crtc_needs_disable(struct drm_crtc_state *old_state,
> > }
> > static void
> > -disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> > +disable_encoder_brige_chain(struct drm_device *dev, struct drm_atomic_state *old_state,
> > + enum bridge_chain_operation_type op_type)
> > {
> > 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;
> > @@ -1163,32 +1164,55 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> > if (WARN_ON(!encoder))
> > continue;
> > - funcs = encoder->helper_private;
> > -
> > - drm_dbg_atomic(dev, "disabling [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_disable(bridge, old_state);
> > - /* Right function depends upon target state. */
> > - if (funcs) {
> > - if (funcs->atomic_disable)
> > - funcs->atomic_disable(encoder, old_state);
> > - else if (new_conn_state->crtc && funcs->prepare)
> > - funcs->prepare(encoder);
> > - else if (funcs->disable)
> > - funcs->disable(encoder);
> > - else if (funcs->dpms)
> > - funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
> > - }
> > + switch (op_type) {
> > + case DRM_ENCODER_BRIDGE_DISABLE:
> > + funcs = encoder->helper_private;
> > +
> > + drm_dbg_atomic(dev, "disabling [ENCODER:%d:%s]\n",
> > + encoder->base.id, encoder->name);
> > +
> > + drm_atomic_bridge_chain_disable(bridge, old_state);
> > +
> > + /* Right function depends upon target state. */
> > + if (funcs) {
> > + if (funcs->atomic_disable)
> > + funcs->atomic_disable(encoder, old_state);
> > + else if (new_conn_state->crtc && funcs->prepare)
> > + funcs->prepare(encoder);
> > + else if (funcs->disable)
> > + funcs->disable(encoder);
> > + else if (funcs->dpms)
> > + funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
> > + }
> > +
> > + break;
> > +
> > + case DRM_BRIDGE_POST_DISABLE:
> > + drm_atomic_bridge_chain_post_disable(bridge, old_state);
> > - drm_atomic_bridge_chain_post_disable(bridge, old_state);
> > + break;
> > +
> > + default:
> > + drm_err(dev, "Unrecognized Encoder/Bridge Operation (%d).\n", op_type);
> > + break;
> > + }
> > }
> > +}
> > +
> > +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;
> > +
> > + disable_encoder_brige_chain(dev, old_state, DRM_ENCODER_BRIDGE_DISABLE);
> > for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
> > const struct drm_crtc_helper_funcs *funcs;
> > @@ -1234,6 +1258,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> > if (ret == 0)
> > drm_crtc_vblank_put(crtc);
> > }
> > +
> > + disable_encoder_brige_chain(dev, old_state, DRM_BRIDGE_POST_DISABLE);
> > }
> > /**
> > @@ -1445,6 +1471,64 @@ static void drm_atomic_helper_commit_writebacks(struct drm_device *dev,
> > }
> > }
> > +static void
> > +enable_encoder_brige_chain(struct drm_device *dev, struct drm_atomic_state *old_state,
> > + enum bridge_chain_operation_type op_type)
> > +{
> > + 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;
> > +
> > + /*
> > + * 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);
> > +
> > + switch (op_type) {
> > + case DRM_BRIDGE_PRE_ENABLE:
> > + drm_atomic_bridge_chain_pre_enable(bridge, old_state);
> > + break;
> > +
> > + case DRM_ENCODER_BRIDGE_ENABLE:
> > + funcs = encoder->helper_private;
> > +
> > + drm_dbg_atomic(dev, "enabling [ENCODER:%d:%s]\n",
> > + encoder->base.id, encoder->name);
> > +
> > + 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);
> > + break;
> > +
> > + default:
> > + drm_err(dev, "Unrecognized Encoder/Bridge Operation (%d).\n", op_type);
> > + break;
> > + }
> > + }
> > +}
> > +
> > /**
> > * drm_atomic_helper_commit_modeset_enables - modeset commit to enable outputs
> > * @dev: DRM device
> > @@ -1465,10 +1549,10 @@ 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;
> > + enable_encoder_brige_chain(dev, old_state, DRM_BRIDGE_PRE_ENABLE);
> > +
> > for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
> > const struct drm_crtc_helper_funcs *funcs;
> > @@ -1491,42 +1575,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
> > }
> > }
> > - 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);
> > - }
> > + enable_encoder_brige_chain(dev, old_state, DRM_ENCODER_BRIDGE_ENABLE);
> > drm_atomic_helper_commit_writebacks(dev, old_state);
> > }
> > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> > index 9aa0a05aa072..b45a175a9f8a 100644
> > --- a/include/drm/drm_atomic_helper.h
> > +++ b/include/drm/drm_atomic_helper.h
> > @@ -43,6 +43,13 @@
> > */
> > #define DRM_PLANE_NO_SCALING (1<<16)
> > +enum bridge_chain_operation_type {
> > + DRM_BRIDGE_PRE_ENABLE,
> > + DRM_BRIDGE_POST_DISABLE,
> > + DRM_ENCODER_BRIDGE_ENABLE,
> > + DRM_ENCODER_BRIDGE_DISABLE,
> > +};
>
> Why are the last two "DRM_ENCODER"?
>
> I don't like the enum... Having "enum bridge_chain_operation_type" as a
> parameter to a function looks like one can pass any of the enum's values,
> which is not the case.
>
> How about an enum with just two values:
>
> DRM_BRIDGE_PRE_ENABLE_POST_DISABLE
> DRM_BRIDGE_ENABLE_DISABLE
I think I'd go a step further and ask why do we need that rework in the
first place. We had a discussion about changing the time the CRTC is
enabled compared to bridges, but it's not clear, nor explained, why we
need to do that rework in the first place.
It should even be two patches imo.
Maxime
Attachment:
signature.asc
Description: PGP signature
