Re: [PATCH v8 11/13] drm/atomic-helper: Separate out bridge pre_enable/post_disable from enable/disable

[Aradhya Bhatia <aradhya.bhatia@linux.dev>]

Hi Jayesh,

Thank you for testing this out, and reporting the error I had
overlooked.

On 04/02/25 17:59, Jayesh Choudhary wrote:
> Hello Aradhya,
> 
> On 27/01/25 00:45, Aradhya Bhatia wrote:
>> The encoder-bridge ops occur by looping over the new connector states of
>> the display pipelines. The enable sequence runs as follows -
>>
>>     - pre_enable(bridge),
>>     - enable(encoder),
>>     - enable(bridge),
>>
>> while the disable sequnce runs as follows -
>>
>>     - disable(bridge),
>>     - disable(encoder),
>>     - post_disable(bridge).
>>
>> Separate out the pre_enable(bridge), and the post_disable(bridge)
>> operations into separate functions each.
>>
>> This patch keeps the sequence same for any singular disaplay pipe, but
>> changes the sequence across multiple display pipelines.
>>
>> This patch is meant to be an interim patch, to cleanly pave the way for
>> the sequence re-ordering patch, and maintain bisectability in the
>> process.
>>
>> Signed-off-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
>> ---
>>   drivers/gpu/drm/drm_atomic_helper.c | 92 +++++++++++++++++++++++++++--
>>   1 file changed, 88 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/
>> drm_atomic_helper.c
>> index e805fd0a54c5..f5532e3646e1 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -1185,8 +1185,6 @@ encoder_bridge_disable(struct drm_device *dev,
>> struct drm_atomic_state *old_stat
>>               else if (funcs->dpms)
>>                   funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
>>           }
>> -
>> -        drm_atomic_bridge_chain_post_disable(bridge, old_state);
>>       }
>>   }
>>   @@ -1243,11 +1241,65 @@ crtc_disable(struct drm_device *dev, struct
>> drm_atomic_state *old_state)
>>       }
>>   }
>>   +static void
>> +encoder_bridge_post_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_state *old_crtc_state, *new_crtc_state;
>> +    int i;
>> +
>> +    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, "post-disabling bridges [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);
>> +    }
>> +}
>> +
>>   static void
>>   disable_outputs(struct drm_device *dev, struct drm_atomic_state
>> *old_state)
>>   {
>>       encoder_bridge_disable(dev, old_state);
>>   +    encoder_bridge_post_disable(dev, old_state);
>> +
>>       crtc_disable(dev, old_state);
>>   }
>>   @@ -1460,6 +1512,38 @@ static void
>> drm_atomic_helper_commit_writebacks(struct drm_device *dev,
>>       }
>>   }
>>   +static void
>> +encoder_bridge_pre_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) {
>> +        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, "pre-enabling bridges [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);
>> +    }
>> +}
>> +
>>   static void
>>   crtc_enable(struct drm_device *dev, struct drm_atomic_state *old_state)
>>   {
>> @@ -1531,8 +1615,6 @@ encoder_bridge_enable(struct drm_device *dev,
>> struct drm_atomic_state *old_state
>>               else if (funcs->commit)
>>                   funcs->commit(encoder);
>>           }
>> -
>> -        drm_atomic_bridge_chain_enable(bridge, old_state);
>>       }
>>   }
>>   @@ -1555,6 +1637,8 @@ void
>> drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>>   {
>>       crtc_enable(dev, old_state);
>>   +    encoder_bridge_pre_enable(dev, old_state);
>> +
>>       encoder_bridge_enable(dev, old_state);
>>   
> 
> After separating enable and pre_enable, bridge_chain_enable hook is not
> called. This breaks display.

That is true.

> 
> In encoder_bridge_enable call, you need to call
> bridge_chain_enable call instead of bridge_chain_pre_enable.

Yes, the encoder_bridge_enable() is supposed to call
bridge_chain_enable() instead of bridge_chain_pre_enable().


> 
> 
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/
> drm_atomic_helper.c
> index d2f19df9f418..1b580dc068bf 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1605,7 +1605,7 @@ encoder_bridge_enable(struct drm_device *dev,
> struct drm_atomic_state *old_state
>                  * 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);
> +               drm_atomic_bridge_chain_enable(bridge, old_state);

While your report is right, I couldn't take this diff as is. The
bridge_chain_enable has to still happen _after_ all the encoders are
enabled inside this function (unlike the bridge_chain_pre_enable that
would happen before the encoder_enable).

> 
>                 if (funcs) {
>                         if (funcs->atomic_enable)
> 
> I have tested display on J784S4-EVM for MHDP and DSI with this diff on
> top of your series.
> 
> With the above change addressed,
> 
> Reviewed-by: Jayesh Choudhary <j-choudhary@ti.com>

Thank you!

I have posted the patch[0], and I have taken the liberty to accept the
tag despite the change in the way I have fixed the error. If that is
unacceptable, please do let me know and I will remove it in a newer
revision.


-- 
Regards
Aradhya

[0]: [PATCH v9 11/13] drm/atomic-helper: Separate out bridge pre_enable/
post_disable from enable/disable
https://lore.kernel.org/all/20250209121621.34677-4-aradhya.bhatia@linux.dev/