Revert "drm/atomic-helper: Re-order bridge chain pre-enable and post-disable"

This reverts commit c9b1150a68d9362a0827609fc0dc1664c0d8bfe1.

Changing the enable/disable sequence has caused regressions on multiple
platforms: R-Car, MCDE, Rockchip. A series (see link below)  was sent to
fix these, but it was decided that it's better to revert the original
patch and change the enable/disable sequence only in the tidss driver.

Reverting this commit breaks tidss's DSI and OLDI outputs, which will be
fixed in the following commits.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Link: https://lore.kernel.org/all/20251202-mcde-drm-regression-thirdfix-v6-0-f1bffd4ec0fa%40kernel.org/
Fixes: c9b1150a68d9 ("drm/atomic-helper: Re-order bridge chain pre-enable and post-disable")
Cc: stable@vger.kernel.org # v6.17+
Reviewed-by: Aradhya Bhatia <aradhya.bhatia@linux.dev>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Reviewed-by: Linus Walleij <linusw@kernel.org>
Tested-by: Linus Walleij <linusw@kernel.org>
Signed-off-by: Linus Walleij <linusw@kernel.org>
Link: https://patch.msgid.link/20251205-drm-seq-fix-v1-1-fda68fa1b3de@ideasonboard.com

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 10adac9397cfb801a94bf6b9da25d1046179992e..ef97f37560b254b1dcee742cd38532b928013ad1 100644 (file)
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1341,9 +1341,9 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *state)
 {
        encoder_bridge_disable(dev, state);
 
-       crtc_disable(dev, state);
-
        encoder_bridge_post_disable(dev, state);
+
+       crtc_disable(dev, state);
 }
 
 /**
@@ -1682,10 +1682,10 @@ encoder_bridge_enable(struct drm_device *dev, struct drm_atomic_state *state)
 void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
                                              struct drm_atomic_state *state)
 {
-       encoder_bridge_pre_enable(dev, state);
-
        crtc_enable(dev, state);
 
+       encoder_bridge_pre_enable(dev, state);
+
        encoder_bridge_enable(dev, state);
 
        drm_atomic_helper_commit_writebacks(dev, state);

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 0ff7ab4aa8689a022458f935a7ffb23a2b715802..dbafe136833f892591404632c55fda085bf9b1ca 100644 (file)
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -176,33 +176,17 @@ struct drm_bridge_funcs {
        /**
         * @disable:
         *
-        * The @disable callback should disable the bridge.
+        * This callback should disable the bridge. It is called right before
+        * the preceding element in the display pipe is disabled. If the
+        * preceding element is a bridge this means it's called before that
+        * bridge's @disable vfunc. If the preceding element is a &drm_encoder
+        * it's called right before the &drm_encoder_helper_funcs.disable,
+        * &drm_encoder_helper_funcs.prepare or &drm_encoder_helper_funcs.dpms
+        * hook.
         *
         * The bridge can assume that the display pipe (i.e. clocks and timing
         * signals) feeding it is still running when this callback is called.
         *
-        *
-        * If the preceding element is a &drm_bridge, then this is called before
-        * that bridge is disabled via one of:
-        *
-        * - &drm_bridge_funcs.disable
-        * - &drm_bridge_funcs.atomic_disable
-        *
-        * If the preceding element of the bridge is a display controller, then
-        * this callback is called before the encoder is disabled via one of:
-        *
-        * - &drm_encoder_helper_funcs.atomic_disable
-        * - &drm_encoder_helper_funcs.prepare
-        * - &drm_encoder_helper_funcs.disable
-        * - &drm_encoder_helper_funcs.dpms
-        *
-        * and the CRTC is disabled via one of:
-        *
-        * - &drm_crtc_helper_funcs.prepare
-        * - &drm_crtc_helper_funcs.atomic_disable
-        * - &drm_crtc_helper_funcs.disable
-        * - &drm_crtc_helper_funcs.dpms.
-        *
         * The @disable callback is optional.
         *
         * NOTE:
@@ -215,34 +199,17 @@ struct drm_bridge_funcs {
        /**
         * @post_disable:
         *
-        * The bridge must assume that the display pipe (i.e. clocks and timing
-        * signals) feeding this bridge is no longer running when the
-        * @post_disable is called.
+        * This callback should disable the bridge. It is called right after the
+        * preceding element in the display pipe is disabled. If the preceding
+        * element is a bridge this means it's called after that bridge's
+        * @post_disable function. If the preceding element is a &drm_encoder
+        * it's called right after the encoder's
+        * &drm_encoder_helper_funcs.disable, &drm_encoder_helper_funcs.prepare
+        * or &drm_encoder_helper_funcs.dpms hook.
         *
-        * This callback should perform all the actions required by the hardware
-        * after it has stopped receiving signals from the preceding element.
-        *
-        * If the preceding element is a &drm_bridge, then this is called after
-        * that bridge is post-disabled (unless marked otherwise by the
-        * @pre_enable_prev_first flag) via one of:
-        *
-        * - &drm_bridge_funcs.post_disable
-        * - &drm_bridge_funcs.atomic_post_disable
-        *
-        * If the preceding element of the bridge is a display controller, then
-        * this callback is called after the encoder is disabled via one of:
-        *
-        * - &drm_encoder_helper_funcs.atomic_disable
-        * - &drm_encoder_helper_funcs.prepare
-        * - &drm_encoder_helper_funcs.disable
-        * - &drm_encoder_helper_funcs.dpms
-        *
-        * and the CRTC is disabled via one of:
-        *
-        * - &drm_crtc_helper_funcs.prepare
-        * - &drm_crtc_helper_funcs.atomic_disable
-        * - &drm_crtc_helper_funcs.disable
-        * - &drm_crtc_helper_funcs.dpms
+        * The bridge must assume that the display pipe (i.e. clocks and timing
+        * signals) feeding it is no longer running when this callback is
+        * called.
         *
         * The @post_disable callback is optional.
         *
@@ -285,30 +252,18 @@ struct drm_bridge_funcs {
        /**
         * @pre_enable:
         *
-        * The display pipe (i.e. clocks and timing signals) feeding this bridge
-        * will not yet be running when the @pre_enable is called.
-        *
-        * This callback should perform all the necessary actions to prepare the
-        * bridge to accept signals from the preceding element.
-        *
-        * If the preceding element is a &drm_bridge, then this is called before
-        * that bridge is pre-enabled (unless marked otherwise by
-        * @pre_enable_prev_first flag) via one of:
-        *
-        * - &drm_bridge_funcs.pre_enable
-        * - &drm_bridge_funcs.atomic_pre_enable
-        *
-        * If the preceding element of the bridge is a display controller, then
-        * this callback is called before the CRTC is enabled via one of:
-        *
-        * - &drm_crtc_helper_funcs.atomic_enable
-        * - &drm_crtc_helper_funcs.commit
-        *
-        * and the encoder is enabled via one of:
+        * This callback should enable the bridge. It is called right before
+        * the preceding element in the display pipe is enabled. If the
+        * preceding element is a bridge this means it's called before that
+        * bridge's @pre_enable function. If the preceding element is a
+        * &drm_encoder it's called right before the encoder's
+        * &drm_encoder_helper_funcs.enable, &drm_encoder_helper_funcs.commit or
+        * &drm_encoder_helper_funcs.dpms hook.
         *
-        * - &drm_encoder_helper_funcs.atomic_enable
-        * - &drm_encoder_helper_funcs.enable
-        * - &drm_encoder_helper_funcs.commit
+        * The display pipe (i.e. clocks and timing signals) feeding this bridge
+        * will not yet be running when this callback is called. The bridge must
+        * not enable the display link feeding the next bridge in the chain (if
+        * there is one) when this callback is called.
         *
         * The @pre_enable callback is optional.
         *
@@ -322,31 +277,19 @@ struct drm_bridge_funcs {
        /**
         * @enable:
         *
-        * The @enable callback should enable the bridge.
+        * This callback should enable the bridge. It is called right after
+        * the preceding element in the display pipe is enabled. If the
+        * preceding element is a bridge this means it's called after that
+        * bridge's @enable function. If the preceding element is a
+        * &drm_encoder it's called right after the encoder's
+        * &drm_encoder_helper_funcs.enable, &drm_encoder_helper_funcs.commit or
+        * &drm_encoder_helper_funcs.dpms hook.
         *
         * The bridge can assume that the display pipe (i.e. clocks and timing
         * signals) feeding it is running when this callback is called. This
         * callback must enable the display link feeding the next bridge in the
         * chain if there is one.
         *
-        * If the preceding element is a &drm_bridge, then this is called after
-        * that bridge is enabled via one of:
-        *
-        * - &drm_bridge_funcs.enable
-        * - &drm_bridge_funcs.atomic_enable
-        *
-        * If the preceding element of the bridge is a display controller, then
-        * this callback is called after the CRTC is enabled via one of:
-        *
-        * - &drm_crtc_helper_funcs.atomic_enable
-        * - &drm_crtc_helper_funcs.commit
-        *
-        * and the encoder is enabled via one of:
-        *
-        * - &drm_encoder_helper_funcs.atomic_enable
-        * - &drm_encoder_helper_funcs.enable
-        * - drm_encoder_helper_funcs.commit
-        *
         * The @enable callback is optional.
         *
         * NOTE:
@@ -359,30 +302,17 @@ struct drm_bridge_funcs {
        /**
         * @atomic_pre_enable:
         *
-        * The display pipe (i.e. clocks and timing signals) feeding this bridge
-        * will not yet be running when the @atomic_pre_enable is called.
-        *
-        * This callback should perform all the necessary actions to prepare the
-        * bridge to accept signals from the preceding element.
-        *
-        * If the preceding element is a &drm_bridge, then this is called before
-        * that bridge is pre-enabled (unless marked otherwise by
-        * @pre_enable_prev_first flag) via one of:
-        *
-        * - &drm_bridge_funcs.pre_enable
-        * - &drm_bridge_funcs.atomic_pre_enable
+        * This callback should enable the bridge. It is called right before
+        * the preceding element in the display pipe is enabled. If the
+        * preceding element is a bridge this means it's called before that
+        * bridge's @atomic_pre_enable or @pre_enable function. If the preceding
+        * element is a &drm_encoder it's called right before the encoder's
+        * &drm_encoder_helper_funcs.atomic_enable hook.
         *
-        * If the preceding element of the bridge is a display controller, then
-        * this callback is called before the CRTC is enabled via one of:
-        *
-        * - &drm_crtc_helper_funcs.atomic_enable
-        * - &drm_crtc_helper_funcs.commit
-        *
-        * and the encoder is enabled via one of:
-        *
-        * - &drm_encoder_helper_funcs.atomic_enable
-        * - &drm_encoder_helper_funcs.enable
-        * - &drm_encoder_helper_funcs.commit
+        * The display pipe (i.e. clocks and timing signals) feeding this bridge
+        * will not yet be running when this callback is called. The bridge must
+        * not enable the display link feeding the next bridge in the chain (if
+        * there is one) when this callback is called.
         *
         * The @atomic_pre_enable callback is optional.
         */
@@ -392,31 +322,18 @@ struct drm_bridge_funcs {
        /**
         * @atomic_enable:
         *
-        * The @atomic_enable callback should enable the bridge.
+        * This callback should enable the bridge. It is called right after
+        * the preceding element in the display pipe is enabled. If the
+        * preceding element is a bridge this means it's called after that
+        * bridge's @atomic_enable or @enable function. If the preceding element
+        * is a &drm_encoder it's called right after the encoder's
+        * &drm_encoder_helper_funcs.atomic_enable hook.
         *
         * The bridge can assume that the display pipe (i.e. clocks and timing
         * signals) feeding it is running when this callback is called. This
         * callback must enable the display link feeding the next bridge in the
         * chain if there is one.
         *
-        * If the preceding element is a &drm_bridge, then this is called after
-        * that bridge is enabled via one of:
-        *
-        * - &drm_bridge_funcs.enable
-        * - &drm_bridge_funcs.atomic_enable
-        *
-        * If the preceding element of the bridge is a display controller, then
-        * this callback is called after the CRTC is enabled via one of:
-        *
-        * - &drm_crtc_helper_funcs.atomic_enable
-        * - &drm_crtc_helper_funcs.commit
-        *
-        * and the encoder is enabled via one of:
-        *
-        * - &drm_encoder_helper_funcs.atomic_enable
-        * - &drm_encoder_helper_funcs.enable
-        * - drm_encoder_helper_funcs.commit
-        *
         * The @atomic_enable callback is optional.
         */
        void (*atomic_enable)(struct drm_bridge *bridge,
@@ -424,32 +341,16 @@ struct drm_bridge_funcs {
        /**
         * @atomic_disable:
         *
-        * The @atomic_disable callback should disable the bridge.
+        * This callback should disable the bridge. It is called right before
+        * the preceding element in the display pipe is disabled. If the
+        * preceding element is a bridge this means it's called before that
+        * bridge's @atomic_disable or @disable vfunc. If the preceding element
+        * is a &drm_encoder it's called right before the
+        * &drm_encoder_helper_funcs.atomic_disable hook.
         *
         * The bridge can assume that the display pipe (i.e. clocks and timing
         * signals) feeding it is still running when this callback is called.
         *
-        * If the preceding element is a &drm_bridge, then this is called before
-        * that bridge is disabled via one of:
-        *
-        * - &drm_bridge_funcs.disable
-        * - &drm_bridge_funcs.atomic_disable
-        *
-        * If the preceding element of the bridge is a display controller, then
-        * this callback is called before the encoder is disabled via one of:
-        *
-        * - &drm_encoder_helper_funcs.atomic_disable
-        * - &drm_encoder_helper_funcs.prepare
-        * - &drm_encoder_helper_funcs.disable
-        * - &drm_encoder_helper_funcs.dpms
-        *
-        * and the CRTC is disabled via one of:
-        *
-        * - &drm_crtc_helper_funcs.prepare
-        * - &drm_crtc_helper_funcs.atomic_disable
-        * - &drm_crtc_helper_funcs.disable
-        * - &drm_crtc_helper_funcs.dpms.
-        *
         * The @atomic_disable callback is optional.
         */
        void (*atomic_disable)(struct drm_bridge *bridge,
@@ -458,34 +359,16 @@ struct drm_bridge_funcs {
        /**
         * @atomic_post_disable:
         *
-        * The bridge must assume that the display pipe (i.e. clocks and timing
-        * signals) feeding this bridge is no longer running when the
-        * @atomic_post_disable is called.
-        *
-        * This callback should perform all the actions required by the hardware
-        * after it has stopped receiving signals from the preceding element.
+        * This callback should disable the bridge. It is called right after the
+        * preceding element in the display pipe is disabled. If the preceding
+        * element is a bridge this means it's called after that bridge's
+        * @atomic_post_disable or @post_disable function. If the preceding
+        * element is a &drm_encoder it's called right after the encoder's
+        * &drm_encoder_helper_funcs.atomic_disable hook.
         *
-        * If the preceding element is a &drm_bridge, then this is called after
-        * that bridge is post-disabled (unless marked otherwise by the
-        * @pre_enable_prev_first flag) via one of:
-        *
-        * - &drm_bridge_funcs.post_disable
-        * - &drm_bridge_funcs.atomic_post_disable
-        *
-        * If the preceding element of the bridge is a display controller, then
-        * this callback is called after the encoder is disabled via one of:
-        *
-        * - &drm_encoder_helper_funcs.atomic_disable
-        * - &drm_encoder_helper_funcs.prepare
-        * - &drm_encoder_helper_funcs.disable
-        * - &drm_encoder_helper_funcs.dpms
-        *
-        * and the CRTC is disabled via one of:
-        *
-        * - &drm_crtc_helper_funcs.prepare
-        * - &drm_crtc_helper_funcs.atomic_disable
-        * - &drm_crtc_helper_funcs.disable
-        * - &drm_crtc_helper_funcs.dpms
+        * The bridge must assume that the display pipe (i.e. clocks and timing
+        * signals) feeding it is no longer running when this callback is
+        * called.
         *
         * The @atomic_post_disable callback is optional.
         */