| Message ID | 20240530093621.1925863-9-a-bhatia1@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/bridge: cdns-dsi: Fix the color-shift issue | expand |
On Thu, May 30, 2024 at 03:06:20PM +0530, 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@ti.com> > --- > drivers/gpu/drm/drm_atomic_helper.c | 70 +++++++++++++++++++++++++++-- > 1 file changed, 67 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index fb97b51b38f1..02e093950218 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1186,8 +1186,6 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) > else if (funcs->dpms) > funcs->dpms(encoder, DRM_MODE_DPMS_OFF); > } > - > - drm_atomic_bridge_chain_post_disable(bridge, old_state); > } > > for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) { > @@ -1234,6 +1232,49 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) > if (ret == 0) > drm_crtc_vblank_put(crtc); > } > + > + for_each_oldnew_connector_in_state(old_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(old_state, old_conn_state->crtc); > + > + if (new_conn_state->crtc) > + new_crtc_state = drm_atomic_get_new_crtc_state(old_state, > + new_conn_state->crtc); > + 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. > + */ > + if (WARN_ON(!encoder)) > + continue; > + > + drm_dbg_atomic(dev, "disabling [ENCODER:%d:%s]\n", > + encoder->base.id, encoder->name); This duplicates the code in the loop around drm_atomic_bridge_chain_disable(). Can it be extracted to a separate function? Code duplication is nearly always a bad idea. The same comment applies to the next chunk too. > + > + /* > + * 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, old_state); > + } > } > > /**
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index fb97b51b38f1..02e093950218 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1186,8 +1186,6 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) else if (funcs->dpms) funcs->dpms(encoder, DRM_MODE_DPMS_OFF); } - - drm_atomic_bridge_chain_post_disable(bridge, old_state); } for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) { @@ -1234,6 +1232,49 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) if (ret == 0) drm_crtc_vblank_put(crtc); } + + for_each_oldnew_connector_in_state(old_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(old_state, old_conn_state->crtc); + + if (new_conn_state->crtc) + new_crtc_state = drm_atomic_get_new_crtc_state(old_state, + new_conn_state->crtc); + 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. + */ + if (WARN_ON(!encoder)) + continue; + + 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_post_disable(bridge, old_state); + } } /** @@ -1469,6 +1510,30 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, struct drm_connector_state *new_conn_state; int i; + for_each_new_connector_in_state(old_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, "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); + } + for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) { const struct drm_crtc_helper_funcs *funcs; @@ -1514,7 +1579,6 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, * 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)
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@ti.com> --- drivers/gpu/drm/drm_atomic_helper.c | 70 +++++++++++++++++++++++++++-- 1 file changed, 67 insertions(+), 3 deletions(-)