| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1b9aae38-1097-4036-abe6-2490f5dc6a62@ti.com>
Date: Tue, 4 Feb 2025 17:59:08 +0530
From: Jayesh Choudhary <j-choudhary@...com>
To: Aradhya Bhatia <aradhya.bhatia@...ux.dev>,
Tomi Valkeinen
<tomi.valkeinen@...asonboard.com>,
Dmitry Baryshkov
<dmitry.baryshkov@...aro.org>,
Laurent Pinchart
<laurent.pinchart@...asonboard.com>,
Andrzej Hajda <andrzej.hajda@...el.com>,
Neil Armstrong <neil.armstrong@...aro.org>,
Robert Foss <rfoss@...nel.org>, Jonas Karlman <jonas@...boo.se>,
Jernej Skrabec <jernej.skrabec@...il.com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard
<mripard@...nel.org>,
Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie
<airlied@...il.com>, Simona Vetter <simona@...ll.ch>
CC: Nishanth Menon <nm@...com>, Vignesh Raghavendra <vigneshr@...com>,
Devarsh
Thakkar <devarsht@...com>, Praneeth Bajjuri <praneeth@...com>,
Udit Kumar
<u-kumar1@...com>,
DRI Development List <dri-devel@...ts.freedesktop.org>,
Linux Kernel List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v8 11/13] drm/atomic-helper: Separate out bridge
pre_enable/post_disable from enable/disable
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@...ux.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.
In encoder_bridge_enable call, you need to call
bridge_chain_enable call 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);
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@...com>
> drm_atomic_helper_commit_writebacks(dev, old_state);
Warm Regards,
Jayesh
Powered by blists - more mailing lists