On 26/06/24 18:37, Maxime Ripard wrote:
> 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 do agree that the naming is odd. It's supposed to mean both, encoder
and bridge.
When we are enabling/disabling bridges, the encoders are also getting
enabled/disabled right before/after.
But in case of pre_enable/post_disable its only the bridges that are
being operated on.
>>
>> 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
Yes, I can combine them like this.
>
> 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.
>
We did discuss this, which resulted in a bunch of duplicated code in
v2[0]. The same block of code was required before the CRTC enable, as
well as after. Hence this patch.
Maybe all of this should have been explained better. =)
> It should even be two patches imo.
>
Yeah, I can split this in 2 patches.
Regards
Aradhya
[0]:
https://lore.kernel.org/all/20240530093621.1925863-9-a-bhatia1@xxxxxx/
