[PATCH 0/6] drm: introduce atomic_needs_modeset() callbacks

[PATCH 0/6] drm: introduce atomic_needs_modeset() callbacks

[Dmitry Baryshkov]

There are several drivers which attempt to upgrading the commit to the
full mode set from their per-object atomic_check() callbacks without
calling the drm_atomic_helper_check_modeset() or
drm_atomic_helper_check() again (as requested by those functions).

As discussed on IRC, add separate atomic_needs_modeset() callbacks,
whose only purpose is to allow the plane, connector, encoder or CRTC to
specify that for whatever reasons corresponding CRTC should undergo a
full modeset. The helpers will call these callbacks in a proper place,
adding affected objects and calling required functions as required.

The MSM patches depend on the msm-next tree and the series at [1]. The
plan is to land core changes through drm-misc, then merge drm-misc-next
into msm-next and merge remaining msm-specific patches through the
msm-next tree.

[1] https://lore.kernel.org/dri-devel/20250123-drm-dirty-modeset-v2-0-bbfd3a6cd1a4@linaro.org/

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
Dmitry Baryshkov (6):
      drm/atomic-helper: add atomic_needs_modeset callbacks
      drm/mgag200: move format check to drm_plane_helper.atomic_needs_modeset
      drm/msm/dpu: stop upgrading commits by enabling allow_modeset
      drm/msm/dpu: move CTM check to drm_crtc_helper.atomic_needs_modeset
      drm/msm/dpu: use atomic_needs_modeset for CDM check
      drm/msm: drop msm_atomic_check wrapper

 drivers/gpu/drm/drm_atomic_helper.c         | 59 ++++++++++++++++++
 drivers/gpu/drm/mgag200/mgag200_drv.h       |  2 +
 drivers/gpu/drm/mgag200/mgag200_mode.c      | 27 ++++++---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 15 +++++
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 44 +++++++++-----
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 --
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 26 --------
 drivers/gpu/drm/msm/msm_atomic.c            | 29 ---------
 drivers/gpu/drm/msm/msm_drv.h               |  1 -
 drivers/gpu/drm/msm/msm_kms.c               |  2 +-
 drivers/gpu/drm/msm/msm_kms.h               |  7 ---
 include/drm/drm_modeset_helper_vtables.h    | 92 +++++++++++++++++++++++++++++
 12 files changed, 219 insertions(+), 89 deletions(-)
---
base-commit: 0936f0e54426177b0f0263ddf806ed5e13487db6
change-id: 20250123-atomic-needs-modeset-8f6a8243a3f7
prerequisite-change-id: 20241222-drm-dirty-modeset-88079bd27ae6:v2
prerequisite-patch-id: 0c61aabfcd13651203f476985380cbf4d3c299e6
prerequisite-patch-id: c6026f08011c288fd301676e9fa6f46d0cc1dab7
prerequisite-patch-id: b0cb06d5c88791d6e4755d879ced0d5050aa3cbf
prerequisite-patch-id: fd72ddde9dba0df053113bc505c213961a9760da

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

[PATCH 1/6] drm/atomic-helper: add atomic_needs_modeset callbacks

[Dmitry Baryshkov]

Despite drm_atomic_helper_check_modeset() and drm_atomic_helper_check()
documenting that the function should be run again if atomic_check()
callback changes drm_crtc_state.mode_changed some drivers ignore it and
don't rerun the helpers. To simplify such drivers and remove the need to
rerun the helper functions provide additional set of callbacks,
atomic_needs_modeset(). These callbacks are executed at a proper time,
they return a boolean which signifies that corresponding CRTC should
undergo a full modeset. This way the atomic_check() callbacks can stop
updating the drm_crtc_state.mode_changed.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/drm_atomic_helper.c      | 59 ++++++++++++++++++++
 include/drm/drm_modeset_helper_vtables.h | 92 ++++++++++++++++++++++++++++++++
 2 files changed, 151 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 3034ba09c0ee5791ffd2f4130bd84b4cc4676fae..1f689ccd9b0d7f655c6a49e642d652b815a0e8e1 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -649,6 +649,8 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	struct drm_connector *connector;
 	struct drm_connector_state *old_connector_state, *new_connector_state;
+	struct drm_plane *plane;
+	struct drm_plane_state *new_plane_state, *old_plane_state;
 	int i, ret;
 	unsigned int connectors_mask = 0, user_connectors_mask = 0;
 
@@ -708,6 +710,9 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 
 	for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i) {
 		const struct drm_connector_helper_funcs *funcs = connector->helper_private;
+		struct drm_encoder *encoder = new_connector_state->best_encoder;
+		const struct drm_encoder_helper_funcs *enc_funcs =
+			encoder ? encoder->helper_private : NULL;
 
 		WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
@@ -734,6 +739,25 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 				new_crtc_state->connectors_changed = true;
 		}
 
+		if ((funcs->atomic_needs_modeset &&
+		     funcs->atomic_needs_modeset(connector, state)) ||
+		    (enc_funcs && enc_funcs->atomic_needs_modeset &&
+		     enc_funcs->atomic_needs_modeset(encoder, state))) {
+			if (new_connector_state->crtc) {
+				new_crtc_state =
+					drm_atomic_get_new_crtc_state(state,
+								      new_connector_state->crtc);
+				new_crtc_state->mode_changed = true;
+			}
+
+			if (old_connector_state->crtc) {
+				new_crtc_state =
+					drm_atomic_get_new_crtc_state(state,
+								      old_connector_state->crtc);
+				new_crtc_state->mode_changed = true;
+			}
+		}
+
 		if (funcs->atomic_check)
 			ret = funcs->atomic_check(connector, state);
 		if (ret) {
@@ -746,6 +770,29 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 		connectors_mask |= BIT(i);
 	}
 
+	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
+		const struct drm_plane_helper_funcs *funcs;
+
+		funcs = plane->helper_private;
+		if (!funcs || !funcs->atomic_needs_modeset)
+			continue;
+
+		if (!funcs->atomic_needs_modeset(plane, state))
+			continue;
+
+		if (new_plane_state->crtc) {
+			new_crtc_state = drm_atomic_get_new_crtc_state(state,
+								       new_plane_state->crtc);
+			new_crtc_state->mode_changed = true;
+		}
+
+		if (old_plane_state->crtc) {
+			new_crtc_state = drm_atomic_get_new_crtc_state(state,
+								       old_plane_state->crtc);
+			new_crtc_state->mode_changed = true;
+		}
+	}
+
 	/*
 	 * After all the routing has been prepared we need to add in any
 	 * connector which is itself unchanged, but whose CRTC changes its
@@ -753,6 +800,18 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 	 * crtc only changed its mode but has the same set of connectors.
 	 */
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+		const struct drm_crtc_helper_funcs *funcs;
+
+		if (!new_crtc_state->crtc)
+			continue;
+
+		funcs = crtc->helper_private;
+		if (!funcs || !funcs->atomic_needs_modeset)
+			continue;
+
+		if (funcs->atomic_needs_modeset(crtc, state))
+			new_crtc_state->mode_changed = true;
+
 		if (!drm_atomic_crtc_needs_modeset(new_crtc_state))
 			continue;
 
diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
index b62f41f489625e5177bdc05eef950e6c18c219fd..a83999109a49321536956c8b1b93fb4b0d0f7ce2 100644
--- a/include/drm/drm_modeset_helper_vtables.h
+++ b/include/drm/drm_modeset_helper_vtables.h
@@ -304,6 +304,29 @@ struct drm_crtc_helper_funcs {
 	 */
 	void (*disable)(struct drm_crtc *crtc);
 
+	/**
+	 * @atomic_needs_modeset:
+	 *
+	 * This function is called at the beginning of
+	 * drm_atomic_helper_check_modeset() to verify whether the CRTC needs a
+	 * full mode set or not.
+	 *
+	 * This hook is optional.
+	 *
+	 * NOTE:
+	 *
+	 * This function is called in the check phase of an atomic update. The
+	 * driver is not allowed to change anything outside of the
+	 * &drm_atomic_state update tracking structure passed in.
+	 *
+	 * RETURNS:
+	 *
+	 * True if the corresponding CRTC should undergo a full modeset, False
+	 * otherwise.
+	 */
+	bool (*atomic_needs_modeset)(struct drm_crtc *crtc,
+				     struct drm_atomic_state *state);
+
 	/**
 	 * @atomic_check:
 	 *
@@ -800,6 +823,29 @@ struct drm_encoder_helper_funcs {
 	 */
 	void (*enable)(struct drm_encoder *encoder);
 
+	/*
+	 * @atomic_needs_modeset:
+	 *
+	 * This function is called at the beginning of
+	 * drm_atomic_helper_check_modeset() to verify whether the connected
+	 * CRTC needs a full mode set or not.
+	 *
+	 * This hook is optional.
+	 *
+	 * NOTE:
+	 *
+	 * This function is called in the check phase of an atomic update. The
+	 * driver is not allowed to change anything outside of the
+	 * &drm_atomic_state update tracking structure passed in.
+	 *
+	 * RETURNS:
+	 *
+	 * True if the corresponding CRTC should undergo a full modeset, False
+	 * otherwise.
+	 */
+	bool (*atomic_needs_modeset)(struct drm_encoder *encoder,
+				     struct drm_atomic_state *state);
+
 	/**
 	 * @atomic_check:
 	 *
@@ -1067,6 +1113,29 @@ struct drm_connector_helper_funcs {
 	struct drm_encoder *(*atomic_best_encoder)(struct drm_connector *connector,
 						   struct drm_atomic_state *state);
 
+	/**
+	 * @atomic_needs_modeset:
+	 *
+	 * This function is called at the beginning of
+	 * drm_atomic_helper_check_modeset() to verify whether the connected
+	 * CRTC needs a full mode set or not.
+	 *
+	 * This hook is optional.
+	 *
+	 * NOTE:
+	 *
+	 * This function is called in the check phase of an atomic update. The
+	 * driver is not allowed to change anything outside of the
+	 * &drm_atomic_state update tracking structure passed in.
+	 *
+	 * RETURNS:
+	 *
+	 * True if the corresponding CRTC should undergo a full modeset, False
+	 * otherwise.
+	 */
+	bool (*atomic_needs_modeset)(struct drm_connector *connector,
+				     struct drm_atomic_state *state);
+
 	/**
 	 * @atomic_check:
 	 *
@@ -1284,6 +1353,29 @@ struct drm_plane_helper_funcs {
 	 */
 	void (*end_fb_access)(struct drm_plane *plane, struct drm_plane_state *new_plane_state);
 
+	/**
+	 * @atomic_needs_modeset:
+	 *
+	 * This function is called at the beginning of
+	 * drm_atomic_helper_check_modeset() to verify whether the connected
+	 * CRTC needs a full mode set or not.
+	 *
+	 * This hook is optional.
+	 *
+	 * NOTE:
+	 *
+	 * This function is called in the check phase of an atomic update. The
+	 * driver is not allowed to change anything outside of the
+	 * &drm_atomic_state update tracking structure passed in.
+	 *
+	 * RETURNS:
+	 *
+	 * True if the corresponding CRTC should undergo a full modeset, False
+	 * otherwise.
+	 */
+	bool (*atomic_needs_modeset)(struct drm_plane *plane,
+				     struct drm_atomic_state *state);
+
 	/**
 	 * @atomic_check:
 	 *

-- 
2.39.5

[PATCH 2/6] drm/mgag200: move format check to drm_plane_helper.atomic_needs_modeset

[Dmitry Baryshkov]

For the mgag200 driver if the format of the plane changes, then the PLL
rate needs to be changed. This requires performing a full CRTC modeset.
Current code sets drm_crtc_state.mode_changed from the plane's
atomic_check() callback and then doesn't call drm_atomic_helper_check()
again. It works for the mgag200 driver, but it breaks calling convention
of the drm_atomic_helper_check() function.

Move the check to the new atomic_needs_modeset() callback, removing the
need to set the flag in the atomic_check().

Note: this also checks the check to compare format to the
old_plane_state->fb->format instead of using plane->state->fb->format.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/mgag200/mgag200_drv.h  |  2 ++
 drivers/gpu/drm/mgag200/mgag200_mode.c | 27 ++++++++++++++++++++-------
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 0608fc63e588bb60f1b087d263a34cfd11624b52..42cf0826ed14e0e9e4ed1b7920486bda008a0f99 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -354,6 +354,8 @@ extern const uint32_t mgag200_primary_plane_formats[];
 extern const size_t   mgag200_primary_plane_formats_size;
 extern const uint64_t mgag200_primary_plane_fmtmods[];
 
+bool mgag200_primary_plane_helper_atomic_needs_modeset(struct drm_plane *plane,
+						       struct drm_atomic_state *new_state);
 int mgag200_primary_plane_helper_atomic_check(struct drm_plane *plane,
 					      struct drm_atomic_state *new_state);
 void mgag200_primary_plane_helper_atomic_update(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index fb71658c3117b25311f19276d6f4ffdee157ac17..63285b356326a13b465387e5d7ac90ec9fe867cf 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -463,12 +463,31 @@ const uint64_t mgag200_primary_plane_fmtmods[] = {
 	DRM_FORMAT_MOD_INVALID
 };
 
+bool mgag200_primary_plane_helper_atomic_needs_modeset(struct drm_plane *plane,
+						       struct drm_atomic_state *new_state)
+{
+	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
+	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(new_state, plane);
+	struct drm_framebuffer *new_fb = new_plane_state->fb;
+	struct drm_framebuffer *fb = NULL;
+
+	if (old_plane_state)
+		fb = old_plane_state->fb;
+
+	if (!new_fb)
+		return false;
+
+	if (!fb || (fb->format != new_fb->format))
+		return true;
+
+	return false;
+}
+
 int mgag200_primary_plane_helper_atomic_check(struct drm_plane *plane,
 					      struct drm_atomic_state *new_state)
 {
 	struct drm_plane_state *new_plane_state = drm_atomic_get_new_plane_state(new_state, plane);
 	struct drm_framebuffer *new_fb = new_plane_state->fb;
-	struct drm_framebuffer *fb = NULL;
 	struct drm_crtc *new_crtc = new_plane_state->crtc;
 	struct drm_crtc_state *new_crtc_state = NULL;
 	struct mgag200_crtc_state *new_mgag200_crtc_state;
@@ -486,12 +505,6 @@ int mgag200_primary_plane_helper_atomic_check(struct drm_plane *plane,
 	else if (!new_plane_state->visible)
 		return 0;
 
-	if (plane->state)
-		fb = plane->state->fb;
-
-	if (!fb || (fb->format != new_fb->format))
-		new_crtc_state->mode_changed = true; /* update PLL settings */
-
 	new_mgag200_crtc_state = to_mgag200_crtc_state(new_crtc_state);
 	new_mgag200_crtc_state->format = new_fb->format;
 

-- 
2.39.5

[PATCH 3/6] drm/msm/dpu: stop upgrading commits by enabling allow_modeset

[Dmitry Baryshkov]

As pointed out by the commit c5e3306a424b ("drm/atomic: clarify the
rules around drm_atomic_state->allow_modeset"), the drivers are now
allowed to set the drm_atomic_state.allow_modeset flag, as it might
break userspace API. Stop upgrading the commit to full modeset. Instead
set the drm_crtc_state.mode_changed if modeset is allowed and if CTM has
been enabled or disabled AND check that DSPPs are assigned to the CRTC
if CTM is enabled.

NOTE: This change has userspace impact, as now non-modeset commits which
enable CTM will fail.

Fixes: 82836692d5d7 ("drm/msm/dpu: manage DPU resources if CTM is requested")
Reported-by: Simona Vetter <simona.vetter@ffwll.ch>
Closes: https://lore.kernel.org/r/20231010170746.617366-1-daniel.vetter@ffwll.ch/
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  9 +++++++++
 drivers/gpu/drm/msm/msm_atomic.c            | 10 ++--------
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index a24fedb5ba4f1c84777b71c669bac0241acdd421..84313bc1f9888452914612fab559b390cf38c705 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -828,6 +828,15 @@ static int dpu_encoder_virt_atomic_check(
 							  global_state, crtc_state);
 	}
 
+	if (crtc_state->ctm) {
+		struct dpu_crtc_state *cstate = to_dpu_crtc_state(crtc_state);
+		int i;
+
+		for (i = 0; i < cstate->num_mixers; i++)
+			if (!cstate->mixers[i].hw_dspp)
+				return -EINVAL;
+	}
+
 	trace_dpu_enc_atomic_check_flags(DRMID(drm_enc), adj_mode->flags);
 
 	return ret;
diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index 5c8c0661cfcd85445950e0f273b8879e7f077727..fdbe49edf2e1506ebeab500e782d456d77ba4fcf 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -189,17 +189,11 @@ int msm_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 	struct drm_crtc *crtc;
 	int i, ret = 0;
 
-	/*
-	 * FIXME: stop setting allow_modeset and move this check to the DPU
-	 * driver.
-	 */
 	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
 				      new_crtc_state, i) {
-		if ((old_crtc_state->ctm && !new_crtc_state->ctm) ||
-		    (!old_crtc_state->ctm && new_crtc_state->ctm)) {
+		if ((!!old_crtc_state->ctm != !!new_crtc_state->ctm) &&
+		    state->allow_modeset)
 			new_crtc_state->mode_changed = true;
-			state->allow_modeset = true;
-		}
 	}
 
 	if (kms && kms->funcs && kms->funcs->check_mode_changed)

-- 
2.39.5

[PATCH 4/6] drm/msm/dpu: move CTM check to drm_crtc_helper.atomic_needs_modeset

[Dmitry Baryshkov]

For the msm/dpu driver if the CTM gets enabled or disabled, the CRTC
should perform resource reallocation (to get or to free the DSPP). This
requires performing a full CRTC modeset.  Current code sets
drm_crtc_state.mode_changed from the msm_atomic_check(), from the
generic code path.

Move the check to the new atomic_needs_modeset() callback, removing the
need to set the flag from the generic code path.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 15 +++++++++++++++
 drivers/gpu/drm/msm/msm_atomic.c         | 11 +----------
 2 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 65e33eba61726929b740831c95330756b2817e28..9a990386570e037f2b1c994a0140f2b7c4c84669 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1189,6 +1189,20 @@ static bool dpu_crtc_needs_dirtyfb(struct drm_crtc_state *cstate)
 	return false;
 }
 
+static bool dpu_crtc_atomic_needs_modeset(struct drm_crtc *crtc,
+					  struct drm_atomic_state *state)
+{
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
+
+	if (!state->allow_modeset)
+		return false;
+
+	old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
+	new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
+
+	return !!old_crtc_state->ctm != !!new_crtc_state->ctm;
+}
+
 static int dpu_crtc_reassign_planes(struct drm_crtc *crtc, struct drm_crtc_state *crtc_state)
 {
 	int total_planes = crtc->dev->mode_config.num_total_plane;
@@ -1535,6 +1549,7 @@ static const struct drm_crtc_funcs dpu_crtc_funcs = {
 static const struct drm_crtc_helper_funcs dpu_crtc_helper_funcs = {
 	.atomic_disable = dpu_crtc_disable,
 	.atomic_enable = dpu_crtc_enable,
+	.atomic_needs_modeset = dpu_crtc_atomic_needs_modeset,
 	.atomic_check = dpu_crtc_atomic_check,
 	.atomic_begin = dpu_crtc_atomic_begin,
 	.atomic_flush = dpu_crtc_atomic_flush,
diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index fdbe49edf2e1506ebeab500e782d456d77ba4fcf..4377233bd6447060b1300cc0d6877b6a777b1edb 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -185,16 +185,7 @@ int msm_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 {
 	struct msm_drm_private *priv = dev->dev_private;
 	struct msm_kms *kms = priv->kms;
-	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
-	struct drm_crtc *crtc;
-	int i, ret = 0;
-
-	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
-				      new_crtc_state, i) {
-		if ((!!old_crtc_state->ctm != !!new_crtc_state->ctm) &&
-		    state->allow_modeset)
-			new_crtc_state->mode_changed = true;
-	}
+	int ret = 0;
 
 	if (kms && kms->funcs && kms->funcs->check_mode_changed)
 		ret = kms->funcs->check_mode_changed(kms, state);

-- 
2.39.5

[PATCH 5/6] drm/msm/dpu: use atomic_needs_modeset for CDM check

[Dmitry Baryshkov]

Rework the CDM check into using the new atomic_needs_modeset()
callbacks. This removes a need for the dpu-specific check_mode_changed()
callback in the msm_kms_funcs structure.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 35 +++++++++++++++++------------
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 ----
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 26 ---------------------
 3 files changed, 21 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 84313bc1f9888452914612fab559b390cf38c705..d09a5c682031b840d486cf48871964e395226d33 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -753,32 +753,38 @@ static void dpu_encoder_assign_crtc_resources(struct dpu_kms *dpu_kms,
 	cstate->num_mixers = num_lm;
 }
 
-/**
- * dpu_encoder_virt_check_mode_changed: check if full modeset is required
- * @drm_enc:    Pointer to drm encoder structure
- * @crtc_state:	Corresponding CRTC state to be checked
- * @conn_state: Corresponding Connector's state to be checked
- *
- * Check if the changes in the object properties demand full mode set.
- */
-int dpu_encoder_virt_check_mode_changed(struct drm_encoder *drm_enc,
-					struct drm_crtc_state *crtc_state,
-					struct drm_connector_state *conn_state)
+static bool dpu_encoder_virt_atomic_needs_modeset(struct drm_encoder *drm_enc,
+						  struct drm_atomic_state *state)
 {
 	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
+	struct drm_connector *connector;
+	struct drm_connector_state *conn_state;
+	struct drm_crtc_state *crtc_state;
 	struct msm_display_topology topology;
 
 	DPU_DEBUG_ENC(dpu_enc, "\n");
 
+	connector = drm_atomic_get_new_connector_for_encoder(state, drm_enc);
+	if (!connector)
+		return false;
+
+	conn_state = drm_atomic_get_new_connector_state(state, connector);
+	if (!conn_state || !conn_state->crtc)
+		return false;
+
+	crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
+	if (!crtc_state)
+		return false;
+
 	/* Using mode instead of adjusted_mode as it wasn't computed yet */
 	topology = dpu_encoder_get_topology(dpu_enc, &crtc_state->mode, crtc_state, conn_state);
 
 	if (topology.needs_cdm && !dpu_enc->cur_master->hw_cdm)
-		crtc_state->mode_changed = true;
+		return true;
 	else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
-		crtc_state->mode_changed = true;
+		return true;
 
-	return 0;
+	return false;
 }
 
 static int dpu_encoder_virt_atomic_check(
@@ -2658,6 +2664,7 @@ static const struct drm_encoder_helper_funcs dpu_encoder_helper_funcs = {
 	.atomic_mode_set = dpu_encoder_virt_atomic_mode_set,
 	.atomic_disable = dpu_encoder_virt_atomic_disable,
 	.atomic_enable = dpu_encoder_virt_atomic_enable,
+	.atomic_needs_modeset = dpu_encoder_virt_atomic_needs_modeset,
 	.atomic_check = dpu_encoder_virt_atomic_check,
 };
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index da133ee4701a329f566f6f9a7255f2f6d050f891..92b5ee390788d16e85e195a664417896a2bf1cae 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -88,8 +88,4 @@ void dpu_encoder_cleanup_wb_job(struct drm_encoder *drm_enc,
 
 bool dpu_encoder_is_valid_for_commit(struct drm_encoder *drm_enc);
 
-int dpu_encoder_virt_check_mode_changed(struct drm_encoder *drm_enc,
-					struct drm_crtc_state *crtc_state,
-					struct drm_connector_state *conn_state);
-
 #endif /* __DPU_ENCODER_H__ */
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 9748d24b3ffde45992d28b697a88db5992b00c69..97e9cb8c2b099f4757169cadf7e941148d2bfb16 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -446,31 +446,6 @@ static void dpu_kms_disable_commit(struct msm_kms *kms)
 	pm_runtime_put_sync(&dpu_kms->pdev->dev);
 }
 
-static int dpu_kms_check_mode_changed(struct msm_kms *kms, struct drm_atomic_state *state)
-{
-	struct drm_crtc_state *new_crtc_state;
-	struct drm_connector *connector;
-	struct drm_connector_state *new_conn_state;
-	int i;
-
-	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
-		struct drm_encoder *encoder;
-
-		WARN_ON(!!new_conn_state->best_encoder != !!new_conn_state->crtc);
-
-		if (!new_conn_state->crtc || !new_conn_state->best_encoder)
-			continue;
-
-		new_crtc_state = drm_atomic_get_new_crtc_state(state, new_conn_state->crtc);
-
-		encoder = new_conn_state->best_encoder;
-
-		dpu_encoder_virt_check_mode_changed(encoder, new_crtc_state, new_conn_state);
-	}
-
-	return 0;
-}
-
 static void dpu_kms_flush_commit(struct msm_kms *kms, unsigned crtc_mask)
 {
 	struct dpu_kms *dpu_kms = to_dpu_kms(kms);
@@ -1087,7 +1062,6 @@ static const struct msm_kms_funcs kms_funcs = {
 	.irq             = dpu_core_irq,
 	.enable_commit   = dpu_kms_enable_commit,
 	.disable_commit  = dpu_kms_disable_commit,
-	.check_mode_changed = dpu_kms_check_mode_changed,
 	.flush_commit    = dpu_kms_flush_commit,
 	.wait_flush      = dpu_kms_wait_flush,
 	.complete_commit = dpu_kms_complete_commit,

-- 
2.39.5

[PATCH 6/6] drm/msm: drop msm_atomic_check wrapper

[Dmitry Baryshkov]

With the CTM and CDM checks now being handled by the component
callbacks there is no need for additional wrappers around
drm_atomic_helper_check() wrapper. Drop the msm_atomic_check() function
and use the helper directly.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/msm_atomic.c | 14 --------------
 drivers/gpu/drm/msm/msm_drv.h    |  1 -
 drivers/gpu/drm/msm/msm_kms.c    |  2 +-
 drivers/gpu/drm/msm/msm_kms.h    |  7 -------
 4 files changed, 1 insertion(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index 4377233bd6447060b1300cc0d6877b6a777b1edb..550bcc77acb0426df1f06b08032307c0deef6c4c 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -181,20 +181,6 @@ static unsigned get_crtc_mask(struct drm_atomic_state *state)
 	return mask;
 }
 
-int msm_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
-{
-	struct msm_drm_private *priv = dev->dev_private;
-	struct msm_kms *kms = priv->kms;
-	int ret = 0;
-
-	if (kms && kms->funcs && kms->funcs->check_mode_changed)
-		ret = kms->funcs->check_mode_changed(kms, state);
-	if (ret)
-		return ret;
-
-	return drm_atomic_helper_check(dev, state);
-}
-
 void msm_atomic_commit_tail(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index fee31680a6d540d87cada67342bd5bee721662df..321f3c1f4932434e7fb6d18548f27df13c56eb71 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -232,7 +232,6 @@ int msm_atomic_init_pending_timer(struct msm_pending_timer *timer,
 		struct msm_kms *kms, int crtc_idx);
 void msm_atomic_destroy_pending_timer(struct msm_pending_timer *timer);
 void msm_atomic_commit_tail(struct drm_atomic_state *state);
-int msm_atomic_check(struct drm_device *dev, struct drm_atomic_state *state);
 struct drm_atomic_state *msm_atomic_state_alloc(struct drm_device *dev);
 
 int msm_crtc_enable_vblank(struct drm_crtc *crtc);
diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
index 4cfad12f4dc1f91a78b36572f6643ac135e00067..2d934fc0fcfd90a67f934d01fd9b194d4eb97609 100644
--- a/drivers/gpu/drm/msm/msm_kms.c
+++ b/drivers/gpu/drm/msm/msm_kms.c
@@ -22,7 +22,7 @@
 
 static const struct drm_mode_config_funcs mode_config_funcs = {
 	.fb_create = msm_framebuffer_create,
-	.atomic_check = msm_atomic_check,
+	.atomic_check = drm_atomic_helper_check,
 	.atomic_commit = drm_atomic_helper_commit,
 };
 
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index ec2a75af89b09754faef1a07adc9338f7d78161e..e60162744c669773b6e5aef824a173647626ab4e 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -59,13 +59,6 @@ struct msm_kms_funcs {
 	void (*enable_commit)(struct msm_kms *kms);
 	void (*disable_commit)(struct msm_kms *kms);
 
-	/**
-	 * @check_mode_changed:
-	 *
-	 * Verify if the commit requires a full modeset on one of CRTCs.
-	 */
-	int (*check_mode_changed)(struct msm_kms *kms, struct drm_atomic_state *state);
-
 	/**
 	 * Prepare for atomic commit.  This is called after any previous
 	 * (async or otherwise) commit has completed.

-- 
2.39.5

Re: [PATCH 0/6] drm: introduce atomic_needs_modeset() callbacks

[Ville Syrjälä]

On Fri, Jan 24, 2025 at 01:14:18PM +0200, Dmitry Baryshkov wrote:
> There are several drivers which attempt to upgrading the commit to the
> full mode set from their per-object atomic_check() callbacks without
> calling the drm_atomic_helper_check_modeset() or
> drm_atomic_helper_check() again (as requested by those functions).

I don't really understand why any of that is supposedly necessary.
drm_atomic_helper_check_modeset() is really all about the
connector routing stuff, so if none of that is changing then there
is no point in calling it again. Eg. in i915 we call it just at
the start, and later on we flag internal modesets all over the
place, but none of those need drm_atomic_helper_check_modeset()
because nothing routing related will have changed.

> 
> As discussed on IRC, add separate atomic_needs_modeset() callbacks,
> whose only purpose is to allow the plane, connector, encoder or CRTC to
> specify that for whatever reasons corresponding CRTC should undergo a
> full modeset. The helpers will call these callbacks in a proper place,
> adding affected objects and calling required functions as required.
> 
> The MSM patches depend on the msm-next tree and the series at [1]. The
> plan is to land core changes through drm-misc, then merge drm-misc-next
> into msm-next and merge remaining msm-specific patches through the
> msm-next tree.
> 
> [1] https://lore.kernel.org/dri-devel/20250123-drm-dirty-modeset-v2-0-bbfd3a6cd1a4@linaro.org/
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> Dmitry Baryshkov (6):
>       drm/atomic-helper: add atomic_needs_modeset callbacks
>       drm/mgag200: move format check to drm_plane_helper.atomic_needs_modeset
>       drm/msm/dpu: stop upgrading commits by enabling allow_modeset
>       drm/msm/dpu: move CTM check to drm_crtc_helper.atomic_needs_modeset
>       drm/msm/dpu: use atomic_needs_modeset for CDM check
>       drm/msm: drop msm_atomic_check wrapper
> 
>  drivers/gpu/drm/drm_atomic_helper.c         | 59 ++++++++++++++++++
>  drivers/gpu/drm/mgag200/mgag200_drv.h       |  2 +
>  drivers/gpu/drm/mgag200/mgag200_mode.c      | 27 ++++++---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 15 +++++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 44 +++++++++-----
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 --
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 26 --------
>  drivers/gpu/drm/msm/msm_atomic.c            | 29 ---------
>  drivers/gpu/drm/msm/msm_drv.h               |  1 -
>  drivers/gpu/drm/msm/msm_kms.c               |  2 +-
>  drivers/gpu/drm/msm/msm_kms.h               |  7 ---
>  include/drm/drm_modeset_helper_vtables.h    | 92 +++++++++++++++++++++++++++++
>  12 files changed, 219 insertions(+), 89 deletions(-)
> ---
> base-commit: 0936f0e54426177b0f0263ddf806ed5e13487db6
> change-id: 20250123-atomic-needs-modeset-8f6a8243a3f7
> prerequisite-change-id: 20241222-drm-dirty-modeset-88079bd27ae6:v2
> prerequisite-patch-id: 0c61aabfcd13651203f476985380cbf4d3c299e6
> prerequisite-patch-id: c6026f08011c288fd301676e9fa6f46d0cc1dab7
> prerequisite-patch-id: b0cb06d5c88791d6e4755d879ced0d5050aa3cbf
> prerequisite-patch-id: fd72ddde9dba0df053113bc505c213961a9760da
> 
> Best regards,
> -- 
> Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
Ville Syrjälä
Intel

Re: [PATCH 0/6] drm: introduce atomic_needs_modeset() callbacks

[Simona Vetter]

On Fri, Jan 24, 2025 at 02:10:54PM +0200, Ville Syrjälä wrote:
> On Fri, Jan 24, 2025 at 01:14:18PM +0200, Dmitry Baryshkov wrote:
> > There are several drivers which attempt to upgrading the commit to the
> > full mode set from their per-object atomic_check() callbacks without
> > calling the drm_atomic_helper_check_modeset() or
> > drm_atomic_helper_check() again (as requested by those functions).
> 
> I don't really understand why any of that is supposedly necessary.
> drm_atomic_helper_check_modeset() is really all about the
> connector routing stuff, so if none of that is changing then there
> is no point in calling it again. Eg. in i915 we call it just at
> the start, and later on we flag internal modesets all over the
> place, but none of those need drm_atomic_helper_check_modeset()
> because nothing routing related will have changed.

i915 doesn't need this because as you say, it doesn't rely on the atomic
helper modeset tracking much at all, but it's all internal. This is for
drivers which rely more or less entirely on
drm_atomic_crtc_needs_modeset().

Also note that it's not just about connector routing, but about adding all
the necessary additional states with
drm_atomic_add_affected_connectors/planes and re-running all the various
state computation hooks for those. Again i915 hand-rolls that all.

So yeah i915 doesn't need this.
-Sima

> 
> > 
> > As discussed on IRC, add separate atomic_needs_modeset() callbacks,
> > whose only purpose is to allow the plane, connector, encoder or CRTC to
> > specify that for whatever reasons corresponding CRTC should undergo a
> > full modeset. The helpers will call these callbacks in a proper place,
> > adding affected objects and calling required functions as required.
> > 
> > The MSM patches depend on the msm-next tree and the series at [1]. The
> > plan is to land core changes through drm-misc, then merge drm-misc-next
> > into msm-next and merge remaining msm-specific patches through the
> > msm-next tree.
> > 
> > [1] https://lore.kernel.org/dri-devel/20250123-drm-dirty-modeset-v2-0-bbfd3a6cd1a4@linaro.org/
> > 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> > Dmitry Baryshkov (6):
> >       drm/atomic-helper: add atomic_needs_modeset callbacks
> >       drm/mgag200: move format check to drm_plane_helper.atomic_needs_modeset
> >       drm/msm/dpu: stop upgrading commits by enabling allow_modeset
> >       drm/msm/dpu: move CTM check to drm_crtc_helper.atomic_needs_modeset
> >       drm/msm/dpu: use atomic_needs_modeset for CDM check
> >       drm/msm: drop msm_atomic_check wrapper
> > 
> >  drivers/gpu/drm/drm_atomic_helper.c         | 59 ++++++++++++++++++
> >  drivers/gpu/drm/mgag200/mgag200_drv.h       |  2 +
> >  drivers/gpu/drm/mgag200/mgag200_mode.c      | 27 ++++++---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 15 +++++
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 44 +++++++++-----
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 --
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 26 --------
> >  drivers/gpu/drm/msm/msm_atomic.c            | 29 ---------
> >  drivers/gpu/drm/msm/msm_drv.h               |  1 -
> >  drivers/gpu/drm/msm/msm_kms.c               |  2 +-
> >  drivers/gpu/drm/msm/msm_kms.h               |  7 ---
> >  include/drm/drm_modeset_helper_vtables.h    | 92 +++++++++++++++++++++++++++++
> >  12 files changed, 219 insertions(+), 89 deletions(-)
> > ---
> > base-commit: 0936f0e54426177b0f0263ddf806ed5e13487db6
> > change-id: 20250123-atomic-needs-modeset-8f6a8243a3f7
> > prerequisite-change-id: 20241222-drm-dirty-modeset-88079bd27ae6:v2
> > prerequisite-patch-id: 0c61aabfcd13651203f476985380cbf4d3c299e6
> > prerequisite-patch-id: c6026f08011c288fd301676e9fa6f46d0cc1dab7
> > prerequisite-patch-id: b0cb06d5c88791d6e4755d879ced0d5050aa3cbf
> > prerequisite-patch-id: fd72ddde9dba0df053113bc505c213961a9760da
> > 
> > Best regards,
> > -- 
> > Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/6] drm: introduce atomic_needs_modeset() callbacks

[Ville Syrjälä]

On Fri, Jan 24, 2025 at 01:59:15PM +0100, Simona Vetter wrote:
> On Fri, Jan 24, 2025 at 02:10:54PM +0200, Ville Syrjälä wrote:
> > On Fri, Jan 24, 2025 at 01:14:18PM +0200, Dmitry Baryshkov wrote:
> > > There are several drivers which attempt to upgrading the commit to the
> > > full mode set from their per-object atomic_check() callbacks without
> > > calling the drm_atomic_helper_check_modeset() or
> > > drm_atomic_helper_check() again (as requested by those functions).
> > 
> > I don't really understand why any of that is supposedly necessary.
> > drm_atomic_helper_check_modeset() is really all about the
> > connector routing stuff, so if none of that is changing then there
> > is no point in calling it again. Eg. in i915 we call it just at
> > the start, and later on we flag internal modesets all over the
> > place, but none of those need drm_atomic_helper_check_modeset()
> > because nothing routing related will have changed.
> 
> i915 doesn't need this because as you say, it doesn't rely on the atomic
> helper modeset tracking much at all, but it's all internal. This is for
> drivers which rely more or less entirely on
> drm_atomic_crtc_needs_modeset().
> 
> Also note that it's not just about connector routing, but about adding all
> the necessary additional states with
> drm_atomic_add_affected_connectors/planes and re-running all the various
> state computation hooks for those. Again i915 hand-rolls that all.

IIRC it only runs the connectors' atomic_check(),
nothing else really. But maybe that's changed since I last
looked at it.

Anyways it feels like we're throwing everything and the
kitchen sink into a single function here. Maybe it should be
split into two or more functions with clear responsibilities?

> 
> So yeah i915 doesn't need this.
> -Sima
> 
> > 
> > > 
> > > As discussed on IRC, add separate atomic_needs_modeset() callbacks,
> > > whose only purpose is to allow the plane, connector, encoder or CRTC to
> > > specify that for whatever reasons corresponding CRTC should undergo a
> > > full modeset. The helpers will call these callbacks in a proper place,
> > > adding affected objects and calling required functions as required.
> > > 
> > > The MSM patches depend on the msm-next tree and the series at [1]. The
> > > plan is to land core changes through drm-misc, then merge drm-misc-next
> > > into msm-next and merge remaining msm-specific patches through the
> > > msm-next tree.
> > > 
> > > [1] https://lore.kernel.org/dri-devel/20250123-drm-dirty-modeset-v2-0-bbfd3a6cd1a4@linaro.org/
> > > 
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > > Dmitry Baryshkov (6):
> > >       drm/atomic-helper: add atomic_needs_modeset callbacks
> > >       drm/mgag200: move format check to drm_plane_helper.atomic_needs_modeset
> > >       drm/msm/dpu: stop upgrading commits by enabling allow_modeset
> > >       drm/msm/dpu: move CTM check to drm_crtc_helper.atomic_needs_modeset
> > >       drm/msm/dpu: use atomic_needs_modeset for CDM check
> > >       drm/msm: drop msm_atomic_check wrapper
> > > 
> > >  drivers/gpu/drm/drm_atomic_helper.c         | 59 ++++++++++++++++++
> > >  drivers/gpu/drm/mgag200/mgag200_drv.h       |  2 +
> > >  drivers/gpu/drm/mgag200/mgag200_mode.c      | 27 ++++++---
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 15 +++++
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 44 +++++++++-----
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 --
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 26 --------
> > >  drivers/gpu/drm/msm/msm_atomic.c            | 29 ---------
> > >  drivers/gpu/drm/msm/msm_drv.h               |  1 -
> > >  drivers/gpu/drm/msm/msm_kms.c               |  2 +-
> > >  drivers/gpu/drm/msm/msm_kms.h               |  7 ---
> > >  include/drm/drm_modeset_helper_vtables.h    | 92 +++++++++++++++++++++++++++++
> > >  12 files changed, 219 insertions(+), 89 deletions(-)
> > > ---
> > > base-commit: 0936f0e54426177b0f0263ddf806ed5e13487db6
> > > change-id: 20250123-atomic-needs-modeset-8f6a8243a3f7
> > > prerequisite-change-id: 20241222-drm-dirty-modeset-88079bd27ae6:v2
> > > prerequisite-patch-id: 0c61aabfcd13651203f476985380cbf4d3c299e6
> > > prerequisite-patch-id: c6026f08011c288fd301676e9fa6f46d0cc1dab7
> > > prerequisite-patch-id: b0cb06d5c88791d6e4755d879ced0d5050aa3cbf
> > > prerequisite-patch-id: fd72ddde9dba0df053113bc505c213961a9760da
> > > 
> > > Best regards,
> > > -- 
> > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> 
> -- 
> Simona Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel

Re: [PATCH 0/6] drm: introduce atomic_needs_modeset() callbacks

[Simona Vetter]

On Fri, Jan 24, 2025 at 03:12:41PM +0200, Ville Syrjälä wrote:
> On Fri, Jan 24, 2025 at 01:59:15PM +0100, Simona Vetter wrote:
> > On Fri, Jan 24, 2025 at 02:10:54PM +0200, Ville Syrjälä wrote:
> > > On Fri, Jan 24, 2025 at 01:14:18PM +0200, Dmitry Baryshkov wrote:
> > > > There are several drivers which attempt to upgrading the commit to the
> > > > full mode set from their per-object atomic_check() callbacks without
> > > > calling the drm_atomic_helper_check_modeset() or
> > > > drm_atomic_helper_check() again (as requested by those functions).
> > > 
> > > I don't really understand why any of that is supposedly necessary.
> > > drm_atomic_helper_check_modeset() is really all about the
> > > connector routing stuff, so if none of that is changing then there
> > > is no point in calling it again. Eg. in i915 we call it just at
> > > the start, and later on we flag internal modesets all over the
> > > place, but none of those need drm_atomic_helper_check_modeset()
> > > because nothing routing related will have changed.
> > 
> > i915 doesn't need this because as you say, it doesn't rely on the atomic
> > helper modeset tracking much at all, but it's all internal. This is for
> > drivers which rely more or less entirely on
> > drm_atomic_crtc_needs_modeset().
> > 
> > Also note that it's not just about connector routing, but about adding all
> > the necessary additional states with
> > drm_atomic_add_affected_connectors/planes and re-running all the various
> > state computation hooks for those. Again i915 hand-rolls that all.
> 
> IIRC it only runs the connectors' atomic_check(),
> nothing else really. But maybe that's changed since I last
> looked at it.

It calls into connector/bridge/crtc callbacks related to modesets and mode
validation.

The thing is a few hundred lines in total if you include all the split out
subfunctions. Like the kerneldoc pretty clearly spells out that it does a
lot more than what you've listed here. Just i915 doesn't used most of
that.

> Anyways it feels like we're throwing everything and the
> kitchen sink into a single function here. Maybe it should be
> split into two or more functions with clear responsibilities?

I'm not sure you can split it up much, because modesetting is complicated.
Like even if you'd want to split out just the routing update logic that's
a pretty big mess with a bunch of callbacks so that we can pick the right
encoders to add the right bridges. And then have a 2nd function that does
the actual state computation/validation.

Not sure that's worth it, since only benefit would be for drivers like
i915 that almost entirely hand-roll their own atomic check flow and really
only need the connector routing bits. I guess if you're bored you could
give it a stab.
-Sima

> 
> > 
> > So yeah i915 doesn't need this.
> > -Sima
> > 
> > > 
> > > > 
> > > > As discussed on IRC, add separate atomic_needs_modeset() callbacks,
> > > > whose only purpose is to allow the plane, connector, encoder or CRTC to
> > > > specify that for whatever reasons corresponding CRTC should undergo a
> > > > full modeset. The helpers will call these callbacks in a proper place,
> > > > adding affected objects and calling required functions as required.
> > > > 
> > > > The MSM patches depend on the msm-next tree and the series at [1]. The
> > > > plan is to land core changes through drm-misc, then merge drm-misc-next
> > > > into msm-next and merge remaining msm-specific patches through the
> > > > msm-next tree.
> > > > 
> > > > [1] https://lore.kernel.org/dri-devel/20250123-drm-dirty-modeset-v2-0-bbfd3a6cd1a4@linaro.org/
> > > > 
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > ---
> > > > Dmitry Baryshkov (6):
> > > >       drm/atomic-helper: add atomic_needs_modeset callbacks
> > > >       drm/mgag200: move format check to drm_plane_helper.atomic_needs_modeset
> > > >       drm/msm/dpu: stop upgrading commits by enabling allow_modeset
> > > >       drm/msm/dpu: move CTM check to drm_crtc_helper.atomic_needs_modeset
> > > >       drm/msm/dpu: use atomic_needs_modeset for CDM check
> > > >       drm/msm: drop msm_atomic_check wrapper
> > > > 
> > > >  drivers/gpu/drm/drm_atomic_helper.c         | 59 ++++++++++++++++++
> > > >  drivers/gpu/drm/mgag200/mgag200_drv.h       |  2 +
> > > >  drivers/gpu/drm/mgag200/mgag200_mode.c      | 27 ++++++---
> > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 15 +++++
> > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 44 +++++++++-----
> > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 --
> > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 26 --------
> > > >  drivers/gpu/drm/msm/msm_atomic.c            | 29 ---------
> > > >  drivers/gpu/drm/msm/msm_drv.h               |  1 -
> > > >  drivers/gpu/drm/msm/msm_kms.c               |  2 +-
> > > >  drivers/gpu/drm/msm/msm_kms.h               |  7 ---
> > > >  include/drm/drm_modeset_helper_vtables.h    | 92 +++++++++++++++++++++++++++++
> > > >  12 files changed, 219 insertions(+), 89 deletions(-)
> > > > ---
> > > > base-commit: 0936f0e54426177b0f0263ddf806ed5e13487db6
> > > > change-id: 20250123-atomic-needs-modeset-8f6a8243a3f7
> > > > prerequisite-change-id: 20241222-drm-dirty-modeset-88079bd27ae6:v2
> > > > prerequisite-patch-id: 0c61aabfcd13651203f476985380cbf4d3c299e6
> > > > prerequisite-patch-id: c6026f08011c288fd301676e9fa6f46d0cc1dab7
> > > > prerequisite-patch-id: b0cb06d5c88791d6e4755d879ced0d5050aa3cbf
> > > > prerequisite-patch-id: fd72ddde9dba0df053113bc505c213961a9760da
> > > > 
> > > > Best regards,
> > > > -- 
> > > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> > 
> > -- 
> > Simona Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/6] drm: introduce atomic_needs_modeset() callbacks

[Ville Syrjälä]

On Fri, Jan 24, 2025 at 04:37:39PM +0100, Simona Vetter wrote:
> On Fri, Jan 24, 2025 at 03:12:41PM +0200, Ville Syrjälä wrote:
> > On Fri, Jan 24, 2025 at 01:59:15PM +0100, Simona Vetter wrote:
> > > On Fri, Jan 24, 2025 at 02:10:54PM +0200, Ville Syrjälä wrote:
> > > > On Fri, Jan 24, 2025 at 01:14:18PM +0200, Dmitry Baryshkov wrote:
> > > > > There are several drivers which attempt to upgrading the commit to the
> > > > > full mode set from their per-object atomic_check() callbacks without
> > > > > calling the drm_atomic_helper_check_modeset() or
> > > > > drm_atomic_helper_check() again (as requested by those functions).
> > > > 
> > > > I don't really understand why any of that is supposedly necessary.
> > > > drm_atomic_helper_check_modeset() is really all about the
> > > > connector routing stuff, so if none of that is changing then there
> > > > is no point in calling it again. Eg. in i915 we call it just at
> > > > the start, and later on we flag internal modesets all over the
> > > > place, but none of those need drm_atomic_helper_check_modeset()
> > > > because nothing routing related will have changed.
> > > 
> > > i915 doesn't need this because as you say, it doesn't rely on the atomic
> > > helper modeset tracking much at all, but it's all internal. This is for
> > > drivers which rely more or less entirely on
> > > drm_atomic_crtc_needs_modeset().
> > > 
> > > Also note that it's not just about connector routing, but about adding all
> > > the necessary additional states with
> > > drm_atomic_add_affected_connectors/planes and re-running all the various
> > > state computation hooks for those. Again i915 hand-rolls that all.
> > 
> > IIRC it only runs the connectors' atomic_check(),
> > nothing else really. But maybe that's changed since I last
> > looked at it.
> 
> It calls into connector/bridge/crtc callbacks related to modesets and mode
> validation.

The pre-atomic mode_fixup stuff? Are people still using that in
atomic drivers? Hmm, it does look like someone added some real
atomic_check() calls in there, which is a slightly surprising
place to find them.

> 
> The thing is a few hundred lines in total if you include all the split out
> subfunctions. Like the kerneldoc pretty clearly spells out that it does a
> lot more than what you've listed here. Just i915 doesn't used most of
> that.

In my book a function should do one thing. And if you do have some
kind of massive dispatcher function then it should be very abstract
and just call some smaller functions to do each step.
tldr; I don't like any function that doesn't fit on my screen.

Anyways, my main worry is that someone adds some new logic/checks
somewhere that assumes that you can't flag modesets later without
calling the helper. Which is clearly not correct. Eg. most of the
modesets we do are just done to get the hardware turned off while
we reprogram some global resource that doesn't know how to
synchronize with active pipes, not because anything changed that
would need further checks/recomputation/etc.

> 
> > Anyways it feels like we're throwing everything and the
> > kitchen sink into a single function here. Maybe it should be
> > split into two or more functions with clear responsibilities?
> 
> I'm not sure you can split it up much, because modesetting is complicated.
> Like even if you'd want to split out just the routing update logic that's
> a pretty big mess with a bunch of callbacks so that we can pick the right
> encoders to add the right bridges. And then have a 2nd function that does
> the actual state computation/validation.
> 
> Not sure that's worth it, since only benefit would be for drivers like
> i915 that almost entirely hand-roll their own atomic check flow and really
> only need the connector routing bits. I guess if you're bored you could
> give it a stab.

Yeah, I guess I could try to carve it up a bit when I get bored
with other stuff.

> -Sima
> 
> > 
> > > 
> > > So yeah i915 doesn't need this.
> > > -Sima
> > > 
> > > > 
> > > > > 
> > > > > As discussed on IRC, add separate atomic_needs_modeset() callbacks,
> > > > > whose only purpose is to allow the plane, connector, encoder or CRTC to
> > > > > specify that for whatever reasons corresponding CRTC should undergo a
> > > > > full modeset. The helpers will call these callbacks in a proper place,
> > > > > adding affected objects and calling required functions as required.
> > > > > 
> > > > > The MSM patches depend on the msm-next tree and the series at [1]. The
> > > > > plan is to land core changes through drm-misc, then merge drm-misc-next
> > > > > into msm-next and merge remaining msm-specific patches through the
> > > > > msm-next tree.
> > > > > 
> > > > > [1] https://lore.kernel.org/dri-devel/20250123-drm-dirty-modeset-v2-0-bbfd3a6cd1a4@linaro.org/
> > > > > 
> > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > ---
> > > > > Dmitry Baryshkov (6):
> > > > >       drm/atomic-helper: add atomic_needs_modeset callbacks
> > > > >       drm/mgag200: move format check to drm_plane_helper.atomic_needs_modeset
> > > > >       drm/msm/dpu: stop upgrading commits by enabling allow_modeset
> > > > >       drm/msm/dpu: move CTM check to drm_crtc_helper.atomic_needs_modeset
> > > > >       drm/msm/dpu: use atomic_needs_modeset for CDM check
> > > > >       drm/msm: drop msm_atomic_check wrapper
> > > > > 
> > > > >  drivers/gpu/drm/drm_atomic_helper.c         | 59 ++++++++++++++++++
> > > > >  drivers/gpu/drm/mgag200/mgag200_drv.h       |  2 +
> > > > >  drivers/gpu/drm/mgag200/mgag200_mode.c      | 27 ++++++---
> > > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 15 +++++
> > > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 44 +++++++++-----
> > > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 --
> > > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 26 --------
> > > > >  drivers/gpu/drm/msm/msm_atomic.c            | 29 ---------
> > > > >  drivers/gpu/drm/msm/msm_drv.h               |  1 -
> > > > >  drivers/gpu/drm/msm/msm_kms.c               |  2 +-
> > > > >  drivers/gpu/drm/msm/msm_kms.h               |  7 ---
> > > > >  include/drm/drm_modeset_helper_vtables.h    | 92 +++++++++++++++++++++++++++++
> > > > >  12 files changed, 219 insertions(+), 89 deletions(-)
> > > > > ---
> > > > > base-commit: 0936f0e54426177b0f0263ddf806ed5e13487db6
> > > > > change-id: 20250123-atomic-needs-modeset-8f6a8243a3f7
> > > > > prerequisite-change-id: 20241222-drm-dirty-modeset-88079bd27ae6:v2
> > > > > prerequisite-patch-id: 0c61aabfcd13651203f476985380cbf4d3c299e6
> > > > > prerequisite-patch-id: c6026f08011c288fd301676e9fa6f46d0cc1dab7
> > > > > prerequisite-patch-id: b0cb06d5c88791d6e4755d879ced0d5050aa3cbf
> > > > > prerequisite-patch-id: fd72ddde9dba0df053113bc505c213961a9760da
> > > > > 
> > > > > Best regards,
> > > > > -- 
> > > > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > 
> > > > -- 
> > > > Ville Syrjälä
> > > > Intel
> > > 
> > > -- 
> > > Simona Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> 
> -- 
> Simona Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel

Re: [PATCH 0/6] drm: introduce atomic_needs_modeset() callbacks

[Dmitry Baryshkov]

On Fri, Jan 24, 2025 at 03:12:41PM +0200, Ville Syrjälä wrote:
> On Fri, Jan 24, 2025 at 01:59:15PM +0100, Simona Vetter wrote:
> > On Fri, Jan 24, 2025 at 02:10:54PM +0200, Ville Syrjälä wrote:
> > > On Fri, Jan 24, 2025 at 01:14:18PM +0200, Dmitry Baryshkov wrote:
> > > > There are several drivers which attempt to upgrading the commit to the
> > > > full mode set from their per-object atomic_check() callbacks without
> > > > calling the drm_atomic_helper_check_modeset() or
> > > > drm_atomic_helper_check() again (as requested by those functions).
> > > 
> > > I don't really understand why any of that is supposedly necessary.
> > > drm_atomic_helper_check_modeset() is really all about the
> > > connector routing stuff, so if none of that is changing then there
> > > is no point in calling it again. Eg. in i915 we call it just at
> > > the start, and later on we flag internal modesets all over the
> > > place, but none of those need drm_atomic_helper_check_modeset()
> > > because nothing routing related will have changed.
> > 
> > i915 doesn't need this because as you say, it doesn't rely on the atomic
> > helper modeset tracking much at all, but it's all internal. This is for
> > drivers which rely more or less entirely on
> > drm_atomic_crtc_needs_modeset().
> > 
> > Also note that it's not just about connector routing, but about adding all
> > the necessary additional states with
> > drm_atomic_add_affected_connectors/planes and re-running all the various
> > state computation hooks for those. Again i915 hand-rolls that all.
> 
> IIRC it only runs the connectors' atomic_check(),
> nothing else really. But maybe that's changed since I last
> looked at it.

It also calls encoder's atomic_check() or mode_fixup() callbacks (see
the mode_fixup() call at the end of the helper). And the other function,
drm_atomic_helper_check() calls drm_atomic_helper_check_modeset() and
then calls atomic_check() for the planes and for the CRTCs that are a
part of the state.

For example, if the the encoder requires a modeset change on the CRTC,
then that CRTC (and all connected planes) should also be added to the
state. However with the default code flow it's already too late. Even if
we just split those functions, it's still too late, as mode_fixup()
comes after adding of the CRTC and planes.

Likewise, if the plane wants to upgrade the commit, then all other
planes blended on the corresponding CRTC must also be added to the
state.

> Anyways it feels like we're throwing everything and the
> kitchen sink into a single function here. Maybe it should be
> split into two or more functions with clear responsibilities?

I'm not sure, what do you mean. Do you mean splitting those two
drm_atomic_helper functions? It is possible, but it will also result in
a higher amount of boilerplate code (and still being easy to break). For
example, the mentioned depdency series adds this kind of 'oh, do we need
a modeset?' call to the DPU driver. But then each other driver needs
similar code.

I did a quick non-comprehensive grepping (also I skipped i915 and AMDGPU
drivers, too complicated to analyse):

+ malidp, malidp_crtc_atomic_check_gamma(),
	manually calls drm_atomic_helper_check_modeset() again
		good citizen

+ GUD: gud_pipe_check(),
	plane setting mode_changed,
		pardoned, single plane on a CRTC
+ mcde, mcde_display_check(),
	plane setting mode_changed
		pardoned, single plane on a CRTC
+ mgag200, mgag200_primary_plane_helper_atomic_check(),
	plane setting mode_changed
		pardoned, single plane on a CRTC
+ tilcdc, tilcdc_plane_atomic_check()
	plane setting mode_changed
		pardoned, single plane on a CRTC
+ pl111, pl111_display_check(),
	plane setting mode_changed
		pardoned, single plane on a CRTC
+ tve200, tve200_display_check(),
	plane setting mode_changed
		pardoned, single plane on a CRTC

+ IPUv3, ipu_plane_atomic_check(),
	plane setting mode_changed
		no excuse, it can come from the overlay plane
+ ingenic, ingenic_ipu_plane_atomic_check()
	plane setting mode_changed
		no idea
+ ingenic, ingenic_drm_plane_atomic_check(),
	plane setting mode_changed
		no idea

+ nouveau/nv50, nv50_outp_atomic_check_view(),
	encoder setting mode_changed
		no idea of the possible impact
+ msm, dpu_encoder_virt_atomic_check()
	encoder setting mode_changed
		possible issues once resource allocation is moved to
		CRTC

+ imx/lcdcl: imx_lcdc_pipe_update(),
	ignores crtc_state->mode_changed,
	forces mode_set on its own
		no excuse? needs to be refactored anyway?

+ meson, meson_encoder_hdmi_atomic_check()
	bridge setting mode_changed
		no idea of the possible impact
+ nwl-dsp, nwl_dsi_bridge_atomic_check()
	bridge upgrading active_changed to mode_changed
		no idea of the possible impact

+ drm_atomic_helper_connector_tv_check()
	connector setting mode_changed
		should be okay...
+ drm_atomic_helper_connector_hdmi_check()
	connector setting mode_changed
		should be okay...
+ cadence, cdns_mhdp_connector_atomic_check()
	connector setting mode_changed
		should be okay...
+ synopsis, dw_hdmi_connector_atomic_check
	connector setting mode_changed
		should be okay...
+ vc4, vc4_hdmi_connector_atomic_check()
	connector setting mode_changed
		should be okay...
+ vmwgfx, vmw_stdu_connector_atomic_check()
	connector setting mode_changed
		should be okay...

+ msm, dpu_crtc_atomic_check()
	CRTC upgrading active_changed to mode_changed
		patch pending

+ drm_dp_mst_add_affected_dsc_crtcs()
	when is it supposed to be called at all?

Please note, that those 'pardoned' or 'should be okay' drivers are
breaking the defined function API and might stop functioning at some
point.

> 
> > 
> > So yeah i915 doesn't need this.
> > -Sima
> > 
> > > 
> > > > 
> > > > As discussed on IRC, add separate atomic_needs_modeset() callbacks,
> > > > whose only purpose is to allow the plane, connector, encoder or CRTC to
> > > > specify that for whatever reasons corresponding CRTC should undergo a
> > > > full modeset. The helpers will call these callbacks in a proper place,
> > > > adding affected objects and calling required functions as required.
> > > > 
> > > > The MSM patches depend on the msm-next tree and the series at [1]. The
> > > > plan is to land core changes through drm-misc, then merge drm-misc-next
> > > > into msm-next and merge remaining msm-specific patches through the
> > > > msm-next tree.
> > > > 
> > > > [1] https://lore.kernel.org/dri-devel/20250123-drm-dirty-modeset-v2-0-bbfd3a6cd1a4@linaro.org/
> > > > 
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > ---
> > > > Dmitry Baryshkov (6):
> > > >       drm/atomic-helper: add atomic_needs_modeset callbacks
> > > >       drm/mgag200: move format check to drm_plane_helper.atomic_needs_modeset
> > > >       drm/msm/dpu: stop upgrading commits by enabling allow_modeset
> > > >       drm/msm/dpu: move CTM check to drm_crtc_helper.atomic_needs_modeset
> > > >       drm/msm/dpu: use atomic_needs_modeset for CDM check
> > > >       drm/msm: drop msm_atomic_check wrapper
> > > > 
> > > >  drivers/gpu/drm/drm_atomic_helper.c         | 59 ++++++++++++++++++
> > > >  drivers/gpu/drm/mgag200/mgag200_drv.h       |  2 +
> > > >  drivers/gpu/drm/mgag200/mgag200_mode.c      | 27 ++++++---
> > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 15 +++++
> > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 44 +++++++++-----
> > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 --
> > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 26 --------
> > > >  drivers/gpu/drm/msm/msm_atomic.c            | 29 ---------
> > > >  drivers/gpu/drm/msm/msm_drv.h               |  1 -
> > > >  drivers/gpu/drm/msm/msm_kms.c               |  2 +-
> > > >  drivers/gpu/drm/msm/msm_kms.h               |  7 ---
> > > >  include/drm/drm_modeset_helper_vtables.h    | 92 +++++++++++++++++++++++++++++
> > > >  12 files changed, 219 insertions(+), 89 deletions(-)
> > > > ---
> > > > base-commit: 0936f0e54426177b0f0263ddf806ed5e13487db6
> > > > change-id: 20250123-atomic-needs-modeset-8f6a8243a3f7
> > > > prerequisite-change-id: 20241222-drm-dirty-modeset-88079bd27ae6:v2
> > > > prerequisite-patch-id: 0c61aabfcd13651203f476985380cbf4d3c299e6
> > > > prerequisite-patch-id: c6026f08011c288fd301676e9fa6f46d0cc1dab7
> > > > prerequisite-patch-id: b0cb06d5c88791d6e4755d879ced0d5050aa3cbf
> > > > prerequisite-patch-id: fd72ddde9dba0df053113bc505c213961a9760da
> > > > 
> > > > Best regards,
> > > > -- 
> > > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> > 
> > -- 
> > Simona Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel

-- 
With best wishes
Dmitry

Re: [PATCH 0/6] drm: introduce atomic_needs_modeset() callbacks

[Simona Vetter]

On Fri, Jan 24, 2025 at 06:14:23PM +0200, Ville Syrjälä wrote:
> On Fri, Jan 24, 2025 at 04:37:39PM +0100, Simona Vetter wrote:
> > On Fri, Jan 24, 2025 at 03:12:41PM +0200, Ville Syrjälä wrote:
> > > On Fri, Jan 24, 2025 at 01:59:15PM +0100, Simona Vetter wrote:
> > > > On Fri, Jan 24, 2025 at 02:10:54PM +0200, Ville Syrjälä wrote:
> > > > > On Fri, Jan 24, 2025 at 01:14:18PM +0200, Dmitry Baryshkov wrote:
> > > > > > There are several drivers which attempt to upgrading the commit to the
> > > > > > full mode set from their per-object atomic_check() callbacks without
> > > > > > calling the drm_atomic_helper_check_modeset() or
> > > > > > drm_atomic_helper_check() again (as requested by those functions).
> > > > > 
> > > > > I don't really understand why any of that is supposedly necessary.
> > > > > drm_atomic_helper_check_modeset() is really all about the
> > > > > connector routing stuff, so if none of that is changing then there
> > > > > is no point in calling it again. Eg. in i915 we call it just at
> > > > > the start, and later on we flag internal modesets all over the
> > > > > place, but none of those need drm_atomic_helper_check_modeset()
> > > > > because nothing routing related will have changed.
> > > > 
> > > > i915 doesn't need this because as you say, it doesn't rely on the atomic
> > > > helper modeset tracking much at all, but it's all internal. This is for
> > > > drivers which rely more or less entirely on
> > > > drm_atomic_crtc_needs_modeset().
> > > > 
> > > > Also note that it's not just about connector routing, but about adding all
> > > > the necessary additional states with
> > > > drm_atomic_add_affected_connectors/planes and re-running all the various
> > > > state computation hooks for those. Again i915 hand-rolls that all.
> > > 
> > > IIRC it only runs the connectors' atomic_check(),
> > > nothing else really. But maybe that's changed since I last
> > > looked at it.
> > 
> > It calls into connector/bridge/crtc callbacks related to modesets and mode
> > validation.
> 
> The pre-atomic mode_fixup stuff? Are people still using that in
> atomic drivers? Hmm, it does look like someone added some real
> atomic_check() calls in there, which is a slightly surprising
> place to find them.

Well for modesets you do need hooks to compute the state, and that means a
modern one that passes the actual atomic state stuff as parameters.

> > The thing is a few hundred lines in total if you include all the split out
> > subfunctions. Like the kerneldoc pretty clearly spells out that it does a
> > lot more than what you've listed here. Just i915 doesn't used most of
> > that.
> 
> In my book a function should do one thing. And if you do have some
> kind of massive dispatcher function then it should be very abstract
> and just call some smaller functions to do each step.
> tldr; I don't like any function that doesn't fit on my screen.
> 
> Anyways, my main worry is that someone adds some new logic/checks
> somewhere that assumes that you can't flag modesets later without
> calling the helper. Which is clearly not correct. Eg. most of the
> modesets we do are just done to get the hardware turned off while
> we reprogram some global resource that doesn't know how to
> synchronize with active pipes, not because anything changed that
> would need further checks/recomputation/etc.

That is a bit a risk, but I think thus far all that function has done is
figure out the routing and call _lots_ of callbacks on all the various
objects to figure out modesets. And with connectors, bridges and crtcs
(it's kidna convenient for many drivers to split the plane flip from the
modeset related crtc state computations) there's just a lot of these.

Beyond the routing (which could be split out I guess) I think we've
succeeded at keeping random funny things out of these functions, because
they are documented to be idempotent (if all your callbacks are ofc).
-Sima

> > > Anyways it feels like we're throwing everything and the
> > > kitchen sink into a single function here. Maybe it should be
> > > split into two or more functions with clear responsibilities?
> > 
> > I'm not sure you can split it up much, because modesetting is complicated.
> > Like even if you'd want to split out just the routing update logic that's
> > a pretty big mess with a bunch of callbacks so that we can pick the right
> > encoders to add the right bridges. And then have a 2nd function that does
> > the actual state computation/validation.
> > 
> > Not sure that's worth it, since only benefit would be for drivers like
> > i915 that almost entirely hand-roll their own atomic check flow and really
> > only need the connector routing bits. I guess if you're bored you could
> > give it a stab.
> 
> Yeah, I guess I could try to carve it up a bit when I get bored
> with other stuff.
> 
> > -Sima
> > 
> > > 
> > > > 
> > > > So yeah i915 doesn't need this.
> > > > -Sima
> > > > 
> > > > > 
> > > > > > 
> > > > > > As discussed on IRC, add separate atomic_needs_modeset() callbacks,
> > > > > > whose only purpose is to allow the plane, connector, encoder or CRTC to
> > > > > > specify that for whatever reasons corresponding CRTC should undergo a
> > > > > > full modeset. The helpers will call these callbacks in a proper place,
> > > > > > adding affected objects and calling required functions as required.
> > > > > > 
> > > > > > The MSM patches depend on the msm-next tree and the series at [1]. The
> > > > > > plan is to land core changes through drm-misc, then merge drm-misc-next
> > > > > > into msm-next and merge remaining msm-specific patches through the
> > > > > > msm-next tree.
> > > > > > 
> > > > > > [1] https://lore.kernel.org/dri-devel/20250123-drm-dirty-modeset-v2-0-bbfd3a6cd1a4@linaro.org/
> > > > > > 
> > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > ---
> > > > > > Dmitry Baryshkov (6):
> > > > > >       drm/atomic-helper: add atomic_needs_modeset callbacks
> > > > > >       drm/mgag200: move format check to drm_plane_helper.atomic_needs_modeset
> > > > > >       drm/msm/dpu: stop upgrading commits by enabling allow_modeset
> > > > > >       drm/msm/dpu: move CTM check to drm_crtc_helper.atomic_needs_modeset
> > > > > >       drm/msm/dpu: use atomic_needs_modeset for CDM check
> > > > > >       drm/msm: drop msm_atomic_check wrapper
> > > > > > 
> > > > > >  drivers/gpu/drm/drm_atomic_helper.c         | 59 ++++++++++++++++++
> > > > > >  drivers/gpu/drm/mgag200/mgag200_drv.h       |  2 +
> > > > > >  drivers/gpu/drm/mgag200/mgag200_mode.c      | 27 ++++++---
> > > > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 15 +++++
> > > > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 44 +++++++++-----
> > > > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  4 --
> > > > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c     | 26 --------
> > > > > >  drivers/gpu/drm/msm/msm_atomic.c            | 29 ---------
> > > > > >  drivers/gpu/drm/msm/msm_drv.h               |  1 -
> > > > > >  drivers/gpu/drm/msm/msm_kms.c               |  2 +-
> > > > > >  drivers/gpu/drm/msm/msm_kms.h               |  7 ---
> > > > > >  include/drm/drm_modeset_helper_vtables.h    | 92 +++++++++++++++++++++++++++++
> > > > > >  12 files changed, 219 insertions(+), 89 deletions(-)
> > > > > > ---
> > > > > > base-commit: 0936f0e54426177b0f0263ddf806ed5e13487db6
> > > > > > change-id: 20250123-atomic-needs-modeset-8f6a8243a3f7
> > > > > > prerequisite-change-id: 20241222-drm-dirty-modeset-88079bd27ae6:v2
> > > > > > prerequisite-patch-id: 0c61aabfcd13651203f476985380cbf4d3c299e6
> > > > > > prerequisite-patch-id: c6026f08011c288fd301676e9fa6f46d0cc1dab7
> > > > > > prerequisite-patch-id: b0cb06d5c88791d6e4755d879ced0d5050aa3cbf
> > > > > > prerequisite-patch-id: fd72ddde9dba0df053113bc505c213961a9760da
> > > > > > 
> > > > > > Best regards,
> > > > > > -- 
> > > > > > Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > 
> > > > > -- 
> > > > > Ville Syrjälä
> > > > > Intel
> > > > 
> > > > -- 
> > > > Simona Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> > 
> > -- 
> > Simona Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch