| Message ID | 20241019200530.270738-5-aradhya.bhatia@linux.dev (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/bridge: cdns-dsi: Fix the color-shift issue | expand |
On Sun, Oct 20, 2024 at 01:35:29AM +0530, Aradhya Bhatia wrote: > From: Aradhya Bhatia <a-bhatia1@ti.com> > > 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> > Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev> > --- > drivers/gpu/drm/drm_atomic_helper.c | 102 ++++++++++++++++++---------- > include/drm/drm_atomic_helper.h | 5 ++ > 2 files changed, 71 insertions(+), 36 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index 7741fbcc8fc7..6ebd869df79b 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -1122,7 +1122,8 @@ crtc_needs_disable(struct drm_crtc_state *old_state, > } > > static void > -encoder_bridge_chain_disable(struct drm_device *dev, struct drm_atomic_state *old_state) > +encoder_bridge_chain_disable(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; > @@ -1162,31 +1163,43 @@ encoder_bridge_chain_disable(struct drm_device *dev, struct drm_atomic_state *ol > 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_BRIDGE_ENABLE_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); > + } > > - drm_atomic_bridge_chain_post_disable(bridge, old_state); > + break; > + > + case DRM_BRIDGE_PRE_ENABLE_POST_DISABLE: > + drm_atomic_bridge_chain_post_disable(bridge, old_state); > + break; > + > + default: > + drm_err(dev, "Unrecognized Encoder/Bridge Operation (%d).\n", op_type); > + break; > + } > } > } > > @@ -1197,7 +1210,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) > struct drm_crtc_state *old_crtc_state, *new_crtc_state; > int i; > > - encoder_bridge_chain_disable(dev, old_state); > + encoder_bridge_chain_disable(dev, old_state, DRM_BRIDGE_ENABLE_DISABLE); > > for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) { > const struct drm_crtc_helper_funcs *funcs; > @@ -1243,6 +1256,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) > if (ret == 0) > drm_crtc_vblank_put(crtc); > } > + > + encoder_bridge_chain_disable(dev, old_state, DRM_BRIDGE_PRE_ENABLE_POST_DISABLE); > } > > /** > @@ -1455,7 +1470,8 @@ 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) > +encoder_bridge_chain_enable(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; > @@ -1474,28 +1490,40 @@ encoder_bridge_chain_enable(struct drm_device *dev, struct drm_atomic_state *old > 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); > - } > + switch (op_type) { > + case DRM_BRIDGE_PRE_ENABLE_POST_DISABLE: > + drm_atomic_bridge_chain_pre_enable(bridge, old_state); > + break; > + > + case DRM_BRIDGE_ENABLE_DISABLE: > + funcs = encoder->helper_private; > + > + drm_dbg_atomic(dev, "enabling [ENCODER:%d:%s]\n", > + encoder->base.id, encoder->name); > > - drm_atomic_bridge_chain_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); > + break; > + > + default: > + drm_err(dev, "Unrecognized Encoder/Bridge Operation (%d).\n", op_type); > + break; > + } > } > } > > @@ -1521,6 +1549,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, > struct drm_crtc_state *new_crtc_state; > int i; > > + encoder_bridge_chain_enable(dev, old_state, DRM_BRIDGE_PRE_ENABLE_POST_DISABLE); > + > for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) { > const struct drm_crtc_helper_funcs *funcs; > > @@ -1543,7 +1573,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, > } > } > > - encoder_bridge_chain_enable(dev, old_state); > + encoder_bridge_chain_enable(dev, old_state, DRM_BRIDGE_ENABLE_DISABLE); > > 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..92a5812adc6c 100644 > --- a/include/drm/drm_atomic_helper.h > +++ b/include/drm/drm_atomic_helper.h > @@ -43,6 +43,11 @@ > */ > #define DRM_PLANE_NO_SCALING (1<<16) > > +enum bridge_chain_operation_type { > + DRM_BRIDGE_PRE_ENABLE_POST_DISABLE, > + DRM_BRIDGE_ENABLE_DISABLE, > +}; > + The enum doesn't need to be public, does it? > struct drm_atomic_state; > struct drm_private_obj; > struct drm_private_state; > -- > 2.34.1 >
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 7741fbcc8fc7..6ebd869df79b 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1122,7 +1122,8 @@ crtc_needs_disable(struct drm_crtc_state *old_state, } static void -encoder_bridge_chain_disable(struct drm_device *dev, struct drm_atomic_state *old_state) +encoder_bridge_chain_disable(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; @@ -1162,31 +1163,43 @@ encoder_bridge_chain_disable(struct drm_device *dev, struct drm_atomic_state *ol 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_BRIDGE_ENABLE_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); + } - drm_atomic_bridge_chain_post_disable(bridge, old_state); + break; + + case DRM_BRIDGE_PRE_ENABLE_POST_DISABLE: + drm_atomic_bridge_chain_post_disable(bridge, old_state); + break; + + default: + drm_err(dev, "Unrecognized Encoder/Bridge Operation (%d).\n", op_type); + break; + } } } @@ -1197,7 +1210,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) struct drm_crtc_state *old_crtc_state, *new_crtc_state; int i; - encoder_bridge_chain_disable(dev, old_state); + encoder_bridge_chain_disable(dev, old_state, DRM_BRIDGE_ENABLE_DISABLE); for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) { const struct drm_crtc_helper_funcs *funcs; @@ -1243,6 +1256,8 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) if (ret == 0) drm_crtc_vblank_put(crtc); } + + encoder_bridge_chain_disable(dev, old_state, DRM_BRIDGE_PRE_ENABLE_POST_DISABLE); } /** @@ -1455,7 +1470,8 @@ 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) +encoder_bridge_chain_enable(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; @@ -1474,28 +1490,40 @@ encoder_bridge_chain_enable(struct drm_device *dev, struct drm_atomic_state *old 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); - } + switch (op_type) { + case DRM_BRIDGE_PRE_ENABLE_POST_DISABLE: + drm_atomic_bridge_chain_pre_enable(bridge, old_state); + break; + + case DRM_BRIDGE_ENABLE_DISABLE: + funcs = encoder->helper_private; + + drm_dbg_atomic(dev, "enabling [ENCODER:%d:%s]\n", + encoder->base.id, encoder->name); - drm_atomic_bridge_chain_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); + break; + + default: + drm_err(dev, "Unrecognized Encoder/Bridge Operation (%d).\n", op_type); + break; + } } } @@ -1521,6 +1549,8 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, struct drm_crtc_state *new_crtc_state; int i; + encoder_bridge_chain_enable(dev, old_state, DRM_BRIDGE_PRE_ENABLE_POST_DISABLE); + for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) { const struct drm_crtc_helper_funcs *funcs; @@ -1543,7 +1573,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, } } - encoder_bridge_chain_enable(dev, old_state); + encoder_bridge_chain_enable(dev, old_state, DRM_BRIDGE_ENABLE_DISABLE); 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..92a5812adc6c 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -43,6 +43,11 @@ */ #define DRM_PLANE_NO_SCALING (1<<16) +enum bridge_chain_operation_type { + DRM_BRIDGE_PRE_ENABLE_POST_DISABLE, + DRM_BRIDGE_ENABLE_DISABLE, +}; + struct drm_atomic_state; struct drm_private_obj; struct drm_private_state;