From nobody Thu Dec 18 21:13:34 2025 Received: from out-170.mta0.migadu.com (out-170.mta0.migadu.com [91.218.175.170]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 8E325233553 for ; Tue, 14 Jan 2025 05:58:02 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=91.218.175.170 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736834284; cv=none; b=B9tIUwNQMW0OrZtw0PDRo3rS7j6RWcExGzISVA4qn4a6HVpMvXuwVWvMgaWbohXbZQm4RzVRzzTjb4xcE9W0B3q3/r94TwSKklmzwtJ87ZMqzatkWtURkF1M0mLiPmEtYppKpWAV802y52QuzrAEPaKNSfD+BK3pLEu4nDfYwBI= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1736834284; c=relaxed/simple; bh=kA6xDgzlKQeiLxqzgGLQXYTqFqtzS62F4MJbNCWGQ5k=; h=From:To:Cc:Subject:Date:Message-Id:In-Reply-To:References: MIME-Version; b=LZU8geXp1yNOPsrlLlFUoS0rxtW3sR3/hTIucWLR3/MXjtaK77Dbb3gSVqY9XFnJu5WK3cKT4Q+/UDwTcHVSTrG0oVZ0V/he68VV1ukT/2NxApTmwcyFXYj0MddyheKtIaOwDhHVKL1QpTMgkxbfcn4zI9S1n5p5hfuQPE/Yq6s= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=nMAhaMeN; arc=none smtp.client-ip=91.218.175.170 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="nMAhaMeN" X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1736834275; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=r69bcEgf4AdnBQxjp1E80vclTFc1FX9Eq26gq83uybY=; b=nMAhaMeNtKf9G4SXwV0nAS+HUhDg4mrHtwIdS9CWy7pvgIK1mCLn6fxh2hiYSgIga/LoLh s8YDb4eYy03uq94zKn+Ku9HJeNlT3FWt//roGP0y0cOqnh8kuCu1t2zrY/blC89m3SHaZN XbblyfeaQoKV8f0OBSw0Xx0sbGUIbE0= From: Aradhya Bhatia To: Tomi Valkeinen , Dmitry Baryshkov , Laurent Pinchart , Andrzej Hajda , Neil Armstrong , Robert Foss , Jonas Karlman , Jernej Skrabec , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , Simona Vetter Cc: Nishanth Menon , Vignesh Raghavendra , Devarsh Thakkar , Praneeth Bajjuri , Udit Kumar , Jayesh Choudhary , DRI Development List , Linux Kernel List , Aradhya Bhatia Subject: [PATCH v7 11/12] drm/atomic-helper: Re-order bridge chain pre-enable and post-disable Date: Tue, 14 Jan 2025 11:26:25 +0530 Message-Id: <20250114055626.18816-12-aradhya.bhatia@linux.dev> In-Reply-To: <20250114055626.18816-1-aradhya.bhatia@linux.dev> References: <20250114055626.18816-1-aradhya.bhatia@linux.dev> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-Migadu-Flow: FLOW_OUT Content-Type: text/plain; charset="utf-8" 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, the disable sequence for the display pipeline will look like: bridge[n]_disable ... bridge[1]_disable encoder_disable crtc_disable bridge[1]_post_disable ... bridge[n]_post_disable 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 Signed-off-by: Aradhya Bhatia --- drivers/gpu/drm/drm_atomic_helper.c | 300 +++++++++++++++++----------- 1 file changed, 181 insertions(+), 119 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atom= ic_helper.c index 5186d2114a50..ad6290a4a528 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -74,6 +74,12 @@ * also shares the &struct drm_plane_helper_funcs function table with the = plane * helpers. */ + +enum bridge_chain_operation_type { + DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE, + DRM_BRIDGE_ENABLE_OR_DISABLE, +}; + static void drm_atomic_helper_plane_changed(struct drm_atomic_state *state, struct drm_plane_state *old_plane_state, @@ -1122,74 +1128,12 @@ crtc_needs_disable(struct drm_crtc_state *old_state, } =20 static void -disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) +crtc_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 *crtc; struct drm_crtc_state *old_crtc_state, *new_crtc_state; int i; =20 - for_each_oldnew_connector_in_state(old_state, connector, old_conn_state, = new_conn_state, i) { - const struct drm_encoder_helper_funcs *funcs; - 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 =3D drm_atomic_get_old_crtc_state(old_state, old_conn_sta= te->crtc); - - if (new_conn_state->crtc) - new_crtc_state =3D drm_atomic_get_new_crtc_state( - old_state, - new_conn_state->crtc); - else - new_crtc_state =3D NULL; - - if (!crtc_needs_disable(old_crtc_state, new_crtc_state) || - !drm_atomic_crtc_needs_modeset(old_conn_state->crtc->state)) - continue; - - encoder =3D 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; - - funcs =3D 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 =3D 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); - } - - drm_atomic_bridge_chain_post_disable(bridge, old_state); - } - for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_s= tate, i) { const struct drm_crtc_helper_funcs *funcs; int ret; @@ -1206,7 +1150,6 @@ disable_outputs(struct drm_device *dev, struct drm_at= omic_state *old_state) drm_dbg_atomic(dev, "disabling [CRTC:%d:%s]\n", crtc->base.id, crtc->name); =20 - /* Right function depends upon target state. */ if (new_crtc_state->enable && funcs->prepare) funcs->prepare(crtc); @@ -1236,6 +1179,97 @@ disable_outputs(struct drm_device *dev, struct drm_a= tomic_state *old_state) } } =20 +static void +encoder_bridge_chain_disable(struct drm_device *dev, struct drm_atomic_sta= te *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_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) { + const struct drm_encoder_helper_funcs *funcs; + 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 =3D drm_atomic_get_old_crtc_state(old_state, old_conn_sta= te->crtc); + + if (new_conn_state->crtc) + new_crtc_state =3D drm_atomic_get_new_crtc_state( + old_state, + new_conn_state->crtc); + else + new_crtc_state =3D NULL; + + if (!crtc_needs_disable(old_crtc_state, new_crtc_state) || + !drm_atomic_crtc_needs_modeset(old_conn_state->crtc->state)) + continue; + + encoder =3D 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; + + /* + * Each encoder has at most one connector (since we always steal + * it away), so we won't call disable hooks twice. + */ + bridge =3D drm_bridge_chain_get_first_bridge(encoder); + + switch (op_type) { + case DRM_BRIDGE_ENABLE_OR_DISABLE: + funcs =3D 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_PRE_ENABLE_OR_POST_DISABLE: + drm_atomic_bridge_chain_post_disable(bridge, old_state); + break; + + default: + drm_err(dev, "Unrecognized Encoder/Bridge operation (%d).\n", op_type); + } + } +} + +static void +disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state) +{ + encoder_bridge_chain_disable(dev, old_state, DRM_BRIDGE_ENABLE_OR_DISABLE= ); + + crtc_disable(dev, old_state); + + encoder_bridge_chain_disable(dev, old_state, DRM_BRIDGE_PRE_ENABLE_OR_POS= T_DISABLE); +} + /** * drm_atomic_helper_update_legacy_modeset_state - update legacy modeset s= tate * @dev: DRM device @@ -1445,28 +1479,69 @@ static void drm_atomic_helper_commit_writebacks(str= uct drm_device *dev, } } =20 -/** - * drm_atomic_helper_commit_modeset_enables - modeset commit to enable out= puts - * @dev: DRM device - * @old_state: atomic state object with old state structures - * - * This function enables all the outputs with the new configuration which = had to - * be turned off for the update. - * - * For compatibility with legacy CRTC helpers this should be called after - * drm_atomic_helper_commit_planes(), which is what the default commit fun= ction - * does. But drivers with different needs can group the modeset commits to= gether - * and do the plane commits at the end. This is useful for drivers doing r= untime - * PM since planes updates then only happen when the CRTC is actually enab= led. - */ -void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, - struct drm_atomic_state *old_state) +static void +encoder_bridge_chain_enable(struct drm_device *dev, struct drm_atomic_stat= e *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 =3D 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 =3D drm_bridge_chain_get_first_bridge(encoder); + + switch (op_type) { + case DRM_BRIDGE_PRE_ENABLE_OR_POST_DISABLE: + drm_atomic_bridge_chain_pre_enable(bridge, old_state); + break; + + case DRM_BRIDGE_ENABLE_OR_DISABLE: + funcs =3D 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; + } + } +} + +static void +crtc_enable(struct drm_device *dev, struct drm_atomic_state *old_state) { 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; =20 for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_s= tate, i) { @@ -1490,43 +1565,30 @@ void drm_atomic_helper_commit_modeset_enables(struc= t drm_device *dev, funcs->commit(crtc); } } - - 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 =3D new_conn_state->best_encoder; - funcs =3D 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 =3D 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); - } +} + +/** + * drm_atomic_helper_commit_modeset_enables - modeset commit to enable out= puts + * @dev: DRM device + * @old_state: atomic state object with old state structures + * + * This function enables all the outputs with the new configuration which = had to + * be turned off for the update. + * + * For compatibility with legacy CRTC helpers this should be called after + * drm_atomic_helper_commit_planes(), which is what the default commit fun= ction + * does. But drivers with different needs can group the modeset commits to= gether + * and do the plane commits at the end. This is useful for drivers doing r= untime + * PM since planes updates then only happen when the CRTC is actually enab= led. + */ +void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, + struct drm_atomic_state *old_state) +{ + encoder_bridge_chain_enable(dev, old_state, DRM_BRIDGE_PRE_ENABLE_OR_POST= _DISABLE); + + crtc_enable(dev, old_state); + + encoder_bridge_chain_enable(dev, old_state, DRM_BRIDGE_ENABLE_OR_DISABLE); =20 drm_atomic_helper_commit_writebacks(dev, old_state); } --=20 2.34.1