[PATCH 0/6] drm: enforce rules for drm_atomic_helper_check_modeset()

[PATCH 0/6] drm: enforce rules for drm_atomic_helper_check_modeset()

[Dmitry Baryshkov]

As pointed out by Simona, the drm_atomic_helper_check_modeset() and
drm_atomic_helper_check() require the former function is rerun if the
driver's callbacks modify crtc_state->mode_changed. MSM is one of the
drivers which failed to follow this requirement.

As suggested by Simona, implement generic code to verify that the
drivers abide to those requirement and rework MSM driver to follow that
restrictions.

There are no dependencies between core and MSM parts, so they can go
separately via corresponding trees.

Reported-by: Simona Vetter <simona.vetter@ffwll.ch>
Link: https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
Dmitry Baryshkov (6):
      drm/atomic-helper: document drm_atomic_helper_check() restrictions
      drm/atomic: prepare to check that drivers follow restrictions for needs_modeset
      drm/msm/dpu: don't use active in atomic_check()
      drm/msm/dpu: move needs_cdm setting to dpu_encoder_get_topology()
      drm/msm/dpu: simplify dpu_encoder_get_topology() interface
      drm/msm/dpu: don't set crtc_state->mode_changed from atomic_check()

 drivers/gpu/drm/drm_atomic.c                |  3 +
 drivers/gpu/drm/drm_atomic_helper.c         | 86 ++++++++++++++++++++++++++---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    |  4 --
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 82 +++++++++++++++++----------
 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            | 13 ++++-
 drivers/gpu/drm/msm/msm_kms.h               |  7 +++
 include/drm/drm_atomic.h                    | 10 ++++
 9 files changed, 192 insertions(+), 43 deletions(-)
---
base-commit: b72747fdde637ebf52e181671bf6f41cd773b3e1
change-id: 20241222-drm-dirty-modeset-88079bd27ae6

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

[PATCH 1/6] drm/atomic-helper: document drm_atomic_helper_check() restrictions

[Dmitry Baryshkov]

The drm_atomic_helper_check() calls drm_atomic_helper_check_modeset()
insternally. Document that corresponding restrictions also apply to the
drivers that call the former function (as it's easy to miss the
documentation for the latter function).

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/drm_atomic_helper.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 5186d2114a503701e228e382cc45180b0c578d0c..f26887c3fe8b194137200f9f2426653274c50fda 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1059,6 +1059,15 @@ EXPORT_SYMBOL(drm_atomic_helper_check_planes);
  * For example enable/disable of a cursor plane which have fixed zpos value
  * would trigger all other enabled planes to be forced to the state change.
  *
+ * IMPORTANT:
+ *
+ * As this function calls drm_atomic_helper_check_modeset() internally, its
+ * restrictions also apply:
+ * Drivers which set &drm_crtc_state.mode_changed (e.g. in their
+ * &drm_plane_helper_funcs.atomic_check hooks if a plane update can't be done
+ * without a full modeset) _must_ call drm_atomic_helper_check_modeset()
+ * function again after that change.
+ *
  * RETURNS:
  * Zero for success or -errno
  */

-- 
2.39.5

Re: [PATCH 1/6] drm/atomic-helper: document drm_atomic_helper_check() restrictions

[Simona Vetter]

On Sun, Dec 22, 2024 at 07:00:41AM +0200, Dmitry Baryshkov wrote:
> The drm_atomic_helper_check() calls drm_atomic_helper_check_modeset()
> insternally. Document that corresponding restrictions also apply to the
> drivers that call the former function (as it's easy to miss the
> documentation for the latter function).
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Simona Vetter <simona.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 5186d2114a503701e228e382cc45180b0c578d0c..f26887c3fe8b194137200f9f2426653274c50fda 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1059,6 +1059,15 @@ EXPORT_SYMBOL(drm_atomic_helper_check_planes);
>   * For example enable/disable of a cursor plane which have fixed zpos value
>   * would trigger all other enabled planes to be forced to the state change.
>   *
> + * IMPORTANT:
> + *
> + * As this function calls drm_atomic_helper_check_modeset() internally, its
> + * restrictions also apply:
> + * Drivers which set &drm_crtc_state.mode_changed (e.g. in their
> + * &drm_plane_helper_funcs.atomic_check hooks if a plane update can't be done
> + * without a full modeset) _must_ call drm_atomic_helper_check_modeset()
> + * function again after that change.
> + *
>   * RETURNS:
>   * Zero for success or -errno
>   */
> 
> -- 
> 2.39.5
> 

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

Re: [PATCH 1/6] drm/atomic-helper: document drm_atomic_helper_check() restrictions

[Abhinav Kumar]

On 12/21/2024 9:00 PM, Dmitry Baryshkov wrote:
> The drm_atomic_helper_check() calls drm_atomic_helper_check_modeset()
> insternally. Document that corresponding restrictions also apply to the

insternally ---> internally


> drivers that call the former function (as it's easy to miss the
> documentation for the latter function).
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/drm_atomic_helper.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 

With that typo fixed,

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

[PATCH 2/6] drm/atomic: prepare to check that drivers follow restrictions for needs_modeset

[Dmitry Baryshkov]

Some drivers might fail to follow the restrictions documented for
drm_atomic_helper_check_modesets(). In order to catch such an issues,
add the drm_atomic_state->dirty_needs_modeset field and check it in
drm_atomic_check_only(). Make sure that neither of atomic_check()
callbacks can set that field without calling
drm_atomic_helper_check_modesets() again.

Suggested-by: Simona Vetter <simona.vetter@ffwll.ch>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/drm_atomic.c        |  3 ++
 drivers/gpu/drm/drm_atomic_helper.c | 77 +++++++++++++++++++++++++++++++++----
 include/drm/drm_atomic.h            | 10 +++++
 3 files changed, 82 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 9ea2611770f43ce7ccba410406d5f2c528aab022..202e4e64bd31921d0a4d4b86605b501311e14c33 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1449,6 +1449,9 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
 		}
 	}
 
+	WARN_RATELIMIT(state->dirty_needs_modeset,
+		       "Driver changed needs_modeset under drm_atomic_helper_check_modeset()");
+
 	if (!state->allow_modeset) {
 		for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
 			if (drm_atomic_crtc_needs_modeset(new_crtc_state)) {
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index f26887c3fe8b194137200f9f2426653274c50fda..2c62840416f4b807d6a880b5c30ae024a16af528 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -433,6 +433,7 @@ mode_fixup(struct drm_atomic_state *state)
 
 	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
 		const struct drm_encoder_helper_funcs *funcs;
+		bool old_needs_modeset = false;
 		struct drm_encoder *encoder;
 		struct drm_bridge *bridge;
 
@@ -451,6 +452,9 @@ mode_fixup(struct drm_atomic_state *state)
 		encoder = new_conn_state->best_encoder;
 		funcs = encoder->helper_private;
 
+		if (new_crtc_state)
+			old_needs_modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
+
 		bridge = drm_bridge_chain_get_first_bridge(encoder);
 		ret = drm_atomic_bridge_chain_check(bridge,
 						    new_crtc_state,
@@ -479,6 +483,12 @@ mode_fixup(struct drm_atomic_state *state)
 				return -EINVAL;
 			}
 		}
+
+		if (new_crtc_state) {
+			bool new_needs_modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
+
+			state->dirty_needs_modeset |= (new_needs_modeset != old_needs_modeset);
+		}
 	}
 
 	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
@@ -574,6 +584,36 @@ mode_valid(struct drm_atomic_state *state)
 	return 0;
 }
 
+static int
+connector_atomic_check(struct drm_atomic_state *state,
+		       struct drm_connector *connector,
+		       struct drm_connector_state *old_connector_state,
+		       struct drm_connector_state *new_connector_state)
+{
+	const struct drm_connector_helper_funcs *funcs = connector->helper_private;
+	struct drm_crtc_state *new_crtc_state;
+	bool old_needs_modeset = false;
+	int ret;
+
+	if (new_connector_state->crtc)
+		new_crtc_state = drm_atomic_get_new_crtc_state(state, new_connector_state->crtc);
+	if (new_crtc_state)
+		old_needs_modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
+
+	if (funcs->atomic_check)
+		ret = funcs->atomic_check(connector, state);
+	else
+		ret = 0;
+
+	if (new_crtc_state) {
+		bool new_needs_modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
+
+		state->dirty_needs_modeset |= (new_needs_modeset != old_needs_modeset);
+	}
+
+	return ret;
+}
+
 /**
  * drm_atomic_helper_check_modeset - validate state object for modeset changes
  * @dev: DRM device
@@ -628,6 +668,8 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 	int i, ret;
 	unsigned int connectors_mask = 0, user_connectors_mask = 0;
 
+	state->dirty_needs_modeset = false;
+
 	for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i)
 		user_connectors_mask |= BIT(i);
 
@@ -683,8 +725,6 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 		return ret;
 
 	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;
-
 		WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
 		/*
@@ -710,8 +750,8 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 				new_crtc_state->connectors_changed = true;
 		}
 
-		if (funcs->atomic_check)
-			ret = funcs->atomic_check(connector, state);
+		ret = connector_atomic_check(state, connector,
+					     old_connector_state, new_connector_state);
 		if (ret) {
 			drm_dbg_atomic(dev,
 				       "[CONNECTOR:%d:%s] driver check failed\n",
@@ -752,13 +792,11 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
 	 * has been called on them when a modeset is forced.
 	 */
 	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;
-
 		if (connectors_mask & BIT(i))
 			continue;
 
-		if (funcs->atomic_check)
-			ret = funcs->atomic_check(connector, state);
+		ret = connector_atomic_check(state, connector,
+					     old_connector_state, new_connector_state);
 		if (ret) {
 			drm_dbg_atomic(dev,
 				       "[CONNECTOR:%d:%s] driver check failed\n",
@@ -994,6 +1032,7 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
 
 	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
 		const struct drm_plane_helper_funcs *funcs;
+		bool old_needs_modeset = false;
 
 		WARN_ON(!drm_modeset_is_locked(&plane->mutex));
 
@@ -1006,6 +1045,12 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
 		if (!funcs || !funcs->atomic_check)
 			continue;
 
+		if (new_plane_state->crtc)
+			new_crtc_state = drm_atomic_get_new_crtc_state(state,
+								       new_plane_state->crtc);
+		if (new_crtc_state)
+			old_needs_modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
+
 		ret = funcs->atomic_check(plane, state);
 		if (ret) {
 			drm_dbg_atomic(plane->dev,
@@ -1013,16 +1058,26 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
 				       plane->base.id, plane->name);
 			return ret;
 		}
+
+		if (new_crtc_state) {
+			bool new_needs_modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
+
+			state->dirty_needs_modeset |= (new_needs_modeset != old_needs_modeset);
+		}
 	}
 
 	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
 		const struct drm_crtc_helper_funcs *funcs;
+		bool old_needs_modeset = false;
 
 		funcs = crtc->helper_private;
 
 		if (!funcs || !funcs->atomic_check)
 			continue;
 
+		if (new_crtc_state)
+			old_needs_modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
+
 		ret = funcs->atomic_check(crtc, state);
 		if (ret) {
 			drm_dbg_atomic(crtc->dev,
@@ -1030,6 +1085,12 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
 				       crtc->base.id, crtc->name);
 			return ret;
 		}
+
+		if (new_crtc_state) {
+			bool new_needs_modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
+
+			state->dirty_needs_modeset |= (new_needs_modeset != old_needs_modeset);
+		}
 	}
 
 	return ret;
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 31ca88deb10d262fb3a3f8e14d2afe24f8410cb1..7b0dbd3c8a3df340399a458aaf79263f0fdc24e5 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -408,6 +408,16 @@ struct drm_atomic_state {
 	 */
 	bool duplicated : 1;
 
+	/**
+	 * @dirty_needs_modeset:
+	 *
+	 * Indicates whether the drm_atomic_crtc_needs_modeset() changed in an
+	 * unexpected way. Usually this means that driver implements atomic
+	 * helpers using drm_atomic_crtc_needs_modeset(), but mode_changed has
+	 * toggled by one of its atomic_check() callbacks.
+	 */
+	bool dirty_needs_modeset : 1;
+
 	/**
 	 * @planes:
 	 *

-- 
2.39.5

Re: [PATCH 2/6] drm/atomic: prepare to check that drivers follow restrictions for needs_modeset

[Simona Vetter]

On Sun, Dec 22, 2024 at 07:00:42AM +0200, Dmitry Baryshkov wrote:
> Some drivers might fail to follow the restrictions documented for
> drm_atomic_helper_check_modesets(). In order to catch such an issues,
> add the drm_atomic_state->dirty_needs_modeset field and check it in
> drm_atomic_check_only(). Make sure that neither of atomic_check()
> callbacks can set that field without calling
> drm_atomic_helper_check_modesets() again.
> 
> Suggested-by: Simona Vetter <simona.vetter@ffwll.ch>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Thanks a lot of creating this patch. But looking at it I'm not so sure I
actually had a good idea, since there's still lots of ways for drivers to
change drm_atomic_crtc_needs_modeset() that we miss. And trying to use an
inverted bitfield of all crtc that we've run through in check_modeset, and
then in atomic_check_only compare it against all crtc that need a modeset
also has corner cases it gets wrong I think, like just not using the
helpers in specific case, I think something like i915's fastset would trip
that.

Plus there's lots more corners that drivers have gotten creatively wrong,
so I feel like really clear docs is the best we can do.

So unless you think it was really useful to fix msm I feel like best to
skip this. Apologies for making you put work in here :-/
-Sima

> ---
>  drivers/gpu/drm/drm_atomic.c        |  3 ++
>  drivers/gpu/drm/drm_atomic_helper.c | 77 +++++++++++++++++++++++++++++++++----
>  include/drm/drm_atomic.h            | 10 +++++
>  3 files changed, 82 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 9ea2611770f43ce7ccba410406d5f2c528aab022..202e4e64bd31921d0a4d4b86605b501311e14c33 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1449,6 +1449,9 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>  		}
>  	}
>  
> +	WARN_RATELIMIT(state->dirty_needs_modeset,
> +		       "Driver changed needs_modeset under drm_atomic_helper_check_modeset()");
> +
>  	if (!state->allow_modeset) {
>  		for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
>  			if (drm_atomic_crtc_needs_modeset(new_crtc_state)) {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index f26887c3fe8b194137200f9f2426653274c50fda..2c62840416f4b807d6a880b5c30ae024a16af528 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -433,6 +433,7 @@ mode_fixup(struct drm_atomic_state *state)
>  
>  	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
>  		const struct drm_encoder_helper_funcs *funcs;
> +		bool old_needs_modeset = false;
>  		struct drm_encoder *encoder;
>  		struct drm_bridge *bridge;
>  
> @@ -451,6 +452,9 @@ mode_fixup(struct drm_atomic_state *state)
>  		encoder = new_conn_state->best_encoder;
>  		funcs = encoder->helper_private;
>  
> +		if (new_crtc_state)
> +			old_needs_modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
> +
>  		bridge = drm_bridge_chain_get_first_bridge(encoder);
>  		ret = drm_atomic_bridge_chain_check(bridge,
>  						    new_crtc_state,
> @@ -479,6 +483,12 @@ mode_fixup(struct drm_atomic_state *state)
>  				return -EINVAL;
>  			}
>  		}
> +
> +		if (new_crtc_state) {
> +			bool new_needs_modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
> +
> +			state->dirty_needs_modeset |= (new_needs_modeset != old_needs_modeset);
> +		}
>  	}
>  
>  	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> @@ -574,6 +584,36 @@ mode_valid(struct drm_atomic_state *state)
>  	return 0;
>  }
>  
> +static int
> +connector_atomic_check(struct drm_atomic_state *state,
> +		       struct drm_connector *connector,
> +		       struct drm_connector_state *old_connector_state,
> +		       struct drm_connector_state *new_connector_state)
> +{
> +	const struct drm_connector_helper_funcs *funcs = connector->helper_private;
> +	struct drm_crtc_state *new_crtc_state;
> +	bool old_needs_modeset = false;
> +	int ret;
> +
> +	if (new_connector_state->crtc)
> +		new_crtc_state = drm_atomic_get_new_crtc_state(state, new_connector_state->crtc);
> +	if (new_crtc_state)
> +		old_needs_modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
> +
> +	if (funcs->atomic_check)
> +		ret = funcs->atomic_check(connector, state);
> +	else
> +		ret = 0;
> +
> +	if (new_crtc_state) {
> +		bool new_needs_modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
> +
> +		state->dirty_needs_modeset |= (new_needs_modeset != old_needs_modeset);
> +	}
> +
> +	return ret;
> +}
> +
>  /**
>   * drm_atomic_helper_check_modeset - validate state object for modeset changes
>   * @dev: DRM device
> @@ -628,6 +668,8 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  	int i, ret;
>  	unsigned int connectors_mask = 0, user_connectors_mask = 0;
>  
> +	state->dirty_needs_modeset = false;
> +
>  	for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i)
>  		user_connectors_mask |= BIT(i);
>  
> @@ -683,8 +725,6 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  		return ret;
>  
>  	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;
> -
>  		WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>  
>  		/*
> @@ -710,8 +750,8 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  				new_crtc_state->connectors_changed = true;
>  		}
>  
> -		if (funcs->atomic_check)
> -			ret = funcs->atomic_check(connector, state);
> +		ret = connector_atomic_check(state, connector,
> +					     old_connector_state, new_connector_state);
>  		if (ret) {
>  			drm_dbg_atomic(dev,
>  				       "[CONNECTOR:%d:%s] driver check failed\n",
> @@ -752,13 +792,11 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  	 * has been called on them when a modeset is forced.
>  	 */
>  	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;
> -
>  		if (connectors_mask & BIT(i))
>  			continue;
>  
> -		if (funcs->atomic_check)
> -			ret = funcs->atomic_check(connector, state);
> +		ret = connector_atomic_check(state, connector,
> +					     old_connector_state, new_connector_state);
>  		if (ret) {
>  			drm_dbg_atomic(dev,
>  				       "[CONNECTOR:%d:%s] driver check failed\n",
> @@ -994,6 +1032,7 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
>  
>  	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
>  		const struct drm_plane_helper_funcs *funcs;
> +		bool old_needs_modeset = false;
>  
>  		WARN_ON(!drm_modeset_is_locked(&plane->mutex));
>  
> @@ -1006,6 +1045,12 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
>  		if (!funcs || !funcs->atomic_check)
>  			continue;
>  
> +		if (new_plane_state->crtc)
> +			new_crtc_state = drm_atomic_get_new_crtc_state(state,
> +								       new_plane_state->crtc);
> +		if (new_crtc_state)
> +			old_needs_modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
> +
>  		ret = funcs->atomic_check(plane, state);
>  		if (ret) {
>  			drm_dbg_atomic(plane->dev,
> @@ -1013,16 +1058,26 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
>  				       plane->base.id, plane->name);
>  			return ret;
>  		}
> +
> +		if (new_crtc_state) {
> +			bool new_needs_modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
> +
> +			state->dirty_needs_modeset |= (new_needs_modeset != old_needs_modeset);
> +		}
>  	}
>  
>  	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
>  		const struct drm_crtc_helper_funcs *funcs;
> +		bool old_needs_modeset = false;
>  
>  		funcs = crtc->helper_private;
>  
>  		if (!funcs || !funcs->atomic_check)
>  			continue;
>  
> +		if (new_crtc_state)
> +			old_needs_modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
> +
>  		ret = funcs->atomic_check(crtc, state);
>  		if (ret) {
>  			drm_dbg_atomic(crtc->dev,
> @@ -1030,6 +1085,12 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
>  				       crtc->base.id, crtc->name);
>  			return ret;
>  		}
> +
> +		if (new_crtc_state) {
> +			bool new_needs_modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
> +
> +			state->dirty_needs_modeset |= (new_needs_modeset != old_needs_modeset);
> +		}
>  	}
>  
>  	return ret;
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 31ca88deb10d262fb3a3f8e14d2afe24f8410cb1..7b0dbd3c8a3df340399a458aaf79263f0fdc24e5 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -408,6 +408,16 @@ struct drm_atomic_state {
>  	 */
>  	bool duplicated : 1;
>  
> +	/**
> +	 * @dirty_needs_modeset:
> +	 *
> +	 * Indicates whether the drm_atomic_crtc_needs_modeset() changed in an
> +	 * unexpected way. Usually this means that driver implements atomic
> +	 * helpers using drm_atomic_crtc_needs_modeset(), but mode_changed has
> +	 * toggled by one of its atomic_check() callbacks.
> +	 */
> +	bool dirty_needs_modeset : 1;
> +
>  	/**
>  	 * @planes:
>  	 *
> 
> -- 
> 2.39.5
> 

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

Re: [PATCH 2/6] drm/atomic: prepare to check that drivers follow restrictions for needs_modeset

[Dmitry Baryshkov]

On Wed, Jan 08, 2025 at 06:53:51PM +0100, Simona Vetter wrote:
> On Sun, Dec 22, 2024 at 07:00:42AM +0200, Dmitry Baryshkov wrote:
> > Some drivers might fail to follow the restrictions documented for
> > drm_atomic_helper_check_modesets(). In order to catch such an issues,
> > add the drm_atomic_state->dirty_needs_modeset field and check it in
> > drm_atomic_check_only(). Make sure that neither of atomic_check()
> > callbacks can set that field without calling
> > drm_atomic_helper_check_modesets() again.
> > 
> > Suggested-by: Simona Vetter <simona.vetter@ffwll.ch>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> Thanks a lot of creating this patch. But looking at it I'm not so sure I
> actually had a good idea, since there's still lots of ways for drivers to
> change drm_atomic_crtc_needs_modeset() that we miss. And trying to use an
> inverted bitfield of all crtc that we've run through in check_modeset, and
> then in atomic_check_only compare it against all crtc that need a modeset
> also has corner cases it gets wrong I think, like just not using the
> helpers in specific case, I think something like i915's fastset would trip
> that.

I think we should try to get something merged. I mean, the documentation
was there, but it didn't prevent driver authors from being creative, as
you wrote. So, having false negatives should be fine. We just have not
to trigger false warning reports. I will try giving it another tought.

> 
> Plus there's lots more corners that drivers have gotten creatively wrong,
> so I feel like really clear docs is the best we can do.
> 
> So unless you think it was really useful to fix msm I feel like best to
> skip this. Apologies for making you put work in here :-/

I think it's useful to prevent us (authors) from doing nasty things :-/

> -Sima
> 
> > ---
> >  drivers/gpu/drm/drm_atomic.c        |  3 ++
> >  drivers/gpu/drm/drm_atomic_helper.c | 77 +++++++++++++++++++++++++++++++++----
> >  include/drm/drm_atomic.h            | 10 +++++
> >  3 files changed, 82 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 9ea2611770f43ce7ccba410406d5f2c528aab022..202e4e64bd31921d0a4d4b86605b501311e14c33 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -1449,6 +1449,9 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
> >  		}
> >  	}
> >  
> > +	WARN_RATELIMIT(state->dirty_needs_modeset,
> > +		       "Driver changed needs_modeset under drm_atomic_helper_check_modeset()");
> > +

Maybe it should be drm_warn or drm_info for now, instead of full WARN().

> >  	if (!state->allow_modeset) {
> >  		for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> >  			if (drm_atomic_crtc_needs_modeset(new_crtc_state)) {
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index f26887c3fe8b194137200f9f2426653274c50fda..2c62840416f4b807d6a880b5c30ae024a16af528 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -433,6 +433,7 @@ mode_fixup(struct drm_atomic_state *state)
> >  
> >  	for_each_new_connector_in_state(state, connector, new_conn_state, i) {
> >  		const struct drm_encoder_helper_funcs *funcs;
> > +		bool old_needs_modeset = false;
> >  		struct drm_encoder *encoder;
> >  		struct drm_bridge *bridge;
> >  
> > @@ -451,6 +452,9 @@ mode_fixup(struct drm_atomic_state *state)
> >  		encoder = new_conn_state->best_encoder;
> >  		funcs = encoder->helper_private;
> >  
> > +		if (new_crtc_state)
> > +			old_needs_modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
> > +
> >  		bridge = drm_bridge_chain_get_first_bridge(encoder);
> >  		ret = drm_atomic_bridge_chain_check(bridge,
> >  						    new_crtc_state,
> > @@ -479,6 +483,12 @@ mode_fixup(struct drm_atomic_state *state)
> >  				return -EINVAL;
> >  			}
> >  		}
> > +
> > +		if (new_crtc_state) {
> > +			bool new_needs_modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
> > +
> > +			state->dirty_needs_modeset |= (new_needs_modeset != old_needs_modeset);
> > +		}
> >  	}
> >  
> >  	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> > @@ -574,6 +584,36 @@ mode_valid(struct drm_atomic_state *state)
> >  	return 0;
> >  }
> >  
> > +static int
> > +connector_atomic_check(struct drm_atomic_state *state,
> > +		       struct drm_connector *connector,
> > +		       struct drm_connector_state *old_connector_state,
> > +		       struct drm_connector_state *new_connector_state)
> > +{
> > +	const struct drm_connector_helper_funcs *funcs = connector->helper_private;
> > +	struct drm_crtc_state *new_crtc_state;
> > +	bool old_needs_modeset = false;
> > +	int ret;
> > +
> > +	if (new_connector_state->crtc)
> > +		new_crtc_state = drm_atomic_get_new_crtc_state(state, new_connector_state->crtc);
> > +	if (new_crtc_state)
> > +		old_needs_modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
> > +
> > +	if (funcs->atomic_check)
> > +		ret = funcs->atomic_check(connector, state);
> > +	else
> > +		ret = 0;
> > +
> > +	if (new_crtc_state) {
> > +		bool new_needs_modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
> > +
> > +		state->dirty_needs_modeset |= (new_needs_modeset != old_needs_modeset);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  /**
> >   * drm_atomic_helper_check_modeset - validate state object for modeset changes
> >   * @dev: DRM device
> > @@ -628,6 +668,8 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> >  	int i, ret;
> >  	unsigned int connectors_mask = 0, user_connectors_mask = 0;
> >  
> > +	state->dirty_needs_modeset = false;
> > +
> >  	for_each_oldnew_connector_in_state(state, connector, old_connector_state, new_connector_state, i)
> >  		user_connectors_mask |= BIT(i);
> >  
> > @@ -683,8 +725,6 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> >  		return ret;
> >  
> >  	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;
> > -
> >  		WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> >  
> >  		/*
> > @@ -710,8 +750,8 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> >  				new_crtc_state->connectors_changed = true;
> >  		}
> >  
> > -		if (funcs->atomic_check)
> > -			ret = funcs->atomic_check(connector, state);
> > +		ret = connector_atomic_check(state, connector,
> > +					     old_connector_state, new_connector_state);
> >  		if (ret) {
> >  			drm_dbg_atomic(dev,
> >  				       "[CONNECTOR:%d:%s] driver check failed\n",
> > @@ -752,13 +792,11 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
> >  	 * has been called on them when a modeset is forced.
> >  	 */
> >  	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;
> > -
> >  		if (connectors_mask & BIT(i))
> >  			continue;
> >  
> > -		if (funcs->atomic_check)
> > -			ret = funcs->atomic_check(connector, state);
> > +		ret = connector_atomic_check(state, connector,
> > +					     old_connector_state, new_connector_state);
> >  		if (ret) {
> >  			drm_dbg_atomic(dev,
> >  				       "[CONNECTOR:%d:%s] driver check failed\n",
> > @@ -994,6 +1032,7 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
> >  
> >  	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
> >  		const struct drm_plane_helper_funcs *funcs;
> > +		bool old_needs_modeset = false;
> >  
> >  		WARN_ON(!drm_modeset_is_locked(&plane->mutex));
> >  
> > @@ -1006,6 +1045,12 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
> >  		if (!funcs || !funcs->atomic_check)
> >  			continue;
> >  
> > +		if (new_plane_state->crtc)
> > +			new_crtc_state = drm_atomic_get_new_crtc_state(state,
> > +								       new_plane_state->crtc);
> > +		if (new_crtc_state)
> > +			old_needs_modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
> > +
> >  		ret = funcs->atomic_check(plane, state);
> >  		if (ret) {
> >  			drm_dbg_atomic(plane->dev,
> > @@ -1013,16 +1058,26 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
> >  				       plane->base.id, plane->name);
> >  			return ret;
> >  		}
> > +
> > +		if (new_crtc_state) {
> > +			bool new_needs_modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
> > +
> > +			state->dirty_needs_modeset |= (new_needs_modeset != old_needs_modeset);
> > +		}
> >  	}
> >  
> >  	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
> >  		const struct drm_crtc_helper_funcs *funcs;
> > +		bool old_needs_modeset = false;
> >  
> >  		funcs = crtc->helper_private;
> >  
> >  		if (!funcs || !funcs->atomic_check)
> >  			continue;
> >  
> > +		if (new_crtc_state)
> > +			old_needs_modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
> > +
> >  		ret = funcs->atomic_check(crtc, state);
> >  		if (ret) {
> >  			drm_dbg_atomic(crtc->dev,
> > @@ -1030,6 +1085,12 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
> >  				       crtc->base.id, crtc->name);
> >  			return ret;
> >  		}
> > +
> > +		if (new_crtc_state) {
> > +			bool new_needs_modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
> > +
> > +			state->dirty_needs_modeset |= (new_needs_modeset != old_needs_modeset);
> > +		}
> >  	}
> >  
> >  	return ret;
> > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> > index 31ca88deb10d262fb3a3f8e14d2afe24f8410cb1..7b0dbd3c8a3df340399a458aaf79263f0fdc24e5 100644
> > --- a/include/drm/drm_atomic.h
> > +++ b/include/drm/drm_atomic.h
> > @@ -408,6 +408,16 @@ struct drm_atomic_state {
> >  	 */
> >  	bool duplicated : 1;
> >  
> > +	/**
> > +	 * @dirty_needs_modeset:
> > +	 *
> > +	 * Indicates whether the drm_atomic_crtc_needs_modeset() changed in an
> > +	 * unexpected way. Usually this means that driver implements atomic
> > +	 * helpers using drm_atomic_crtc_needs_modeset(), but mode_changed has
> > +	 * toggled by one of its atomic_check() callbacks.
> > +	 */
> > +	bool dirty_needs_modeset : 1;
> > +
> >  	/**
> >  	 * @planes:
> >  	 *
> > 
> > -- 
> > 2.39.5
> > 
> 
> -- 
> Simona Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
With best wishes
Dmitry

[PATCH 3/6] drm/msm/dpu: don't use active in atomic_check()

[Dmitry Baryshkov]

The driver isn't supposed to consult crtc_state->active/active_check for
resource allocation. Instead all resources should be allocated if
crtc_state->enabled is set. Stop consulting active / active_changed in
order to determine whether the hardware resources should be
(re)allocated.

Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
Reported-by: Simona Vetter <simona.vetter@ffwll.ch>
Closes: https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 4 ----
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 +--
 2 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
index 7191b1a6d41b3a96f956d199398f12b2923e8c82..65e33eba61726929b740831c95330756b2817e28 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
@@ -1264,10 +1264,6 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
 
 	DRM_DEBUG_ATOMIC("%s: check\n", dpu_crtc->name);
 
-	/* force a full mode set if active state changed */
-	if (crtc_state->active_changed)
-		crtc_state->mode_changed = true;
-
 	if (cstate->num_mixers) {
 		rc = _dpu_crtc_check_and_setup_lm_bounds(crtc, crtc_state);
 		if (rc)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 83de7564e2c1fe14fcf8c4f82335cafc937e1b99..d1ccdca6044353f110bf5b507788efe368f223a3 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -793,12 +793,11 @@ static int dpu_encoder_virt_atomic_check(
 		crtc_state->mode_changed = true;
 	/*
 	 * Release and Allocate resources on every modeset
-	 * Dont allocate when active is false.
 	 */
 	if (drm_atomic_crtc_needs_modeset(crtc_state)) {
 		dpu_rm_release(global_state, drm_enc);
 
-		if (!crtc_state->active_changed || crtc_state->enable)
+		if (crtc_state->enable)
 			ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
 					drm_enc, crtc_state, topology);
 		if (!ret)

-- 
2.39.5

Re: [PATCH 3/6] drm/msm/dpu: don't use active in atomic_check()

[Simona Vetter]

On Sun, Dec 22, 2024 at 07:00:43AM +0200, Dmitry Baryshkov wrote:
> The driver isn't supposed to consult crtc_state->active/active_check for
> resource allocation. Instead all resources should be allocated if
> crtc_state->enabled is set. Stop consulting active / active_changed in
> order to determine whether the hardware resources should be
> (re)allocated.
> 
> Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
> Reported-by: Simona Vetter <simona.vetter@ffwll.ch>
> Closes: https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

This is well-contained enough that I feel like I can actually review this
without making a fool of myself :-)

Reviewed-by: Simona Vetter <simona.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 4 ----
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 +--
>  2 files changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 7191b1a6d41b3a96f956d199398f12b2923e8c82..65e33eba61726929b740831c95330756b2817e28 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -1264,10 +1264,6 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
>  
>  	DRM_DEBUG_ATOMIC("%s: check\n", dpu_crtc->name);
>  
> -	/* force a full mode set if active state changed */
> -	if (crtc_state->active_changed)
> -		crtc_state->mode_changed = true;
> -
>  	if (cstate->num_mixers) {
>  		rc = _dpu_crtc_check_and_setup_lm_bounds(crtc, crtc_state);
>  		if (rc)
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 83de7564e2c1fe14fcf8c4f82335cafc937e1b99..d1ccdca6044353f110bf5b507788efe368f223a3 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -793,12 +793,11 @@ static int dpu_encoder_virt_atomic_check(
>  		crtc_state->mode_changed = true;
>  	/*
>  	 * Release and Allocate resources on every modeset
> -	 * Dont allocate when active is false.
>  	 */
>  	if (drm_atomic_crtc_needs_modeset(crtc_state)) {
>  		dpu_rm_release(global_state, drm_enc);
>  
> -		if (!crtc_state->active_changed || crtc_state->enable)
> +		if (crtc_state->enable)
>  			ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
>  					drm_enc, crtc_state, topology);
>  		if (!ret)
> 
> -- 
> 2.39.5
> 

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

Re: [PATCH 3/6] drm/msm/dpu: don't use active in atomic_check()

[Abhinav Kumar]

On 12/21/2024 9:00 PM, Dmitry Baryshkov wrote:
> The driver isn't supposed to consult crtc_state->active/active_check for
> resource allocation. Instead all resources should be allocated if
> crtc_state->enabled is set. Stop consulting active / active_changed in
> order to determine whether the hardware resources should be
> (re)allocated.
> 
> Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
> Reported-by: Simona Vetter <simona.vetter@ffwll.ch>
> Closes: https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 4 ----
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 +--
>   2 files changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index 7191b1a6d41b3a96f956d199398f12b2923e8c82..65e33eba61726929b740831c95330756b2817e28 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -1264,10 +1264,6 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
>   
>   	DRM_DEBUG_ATOMIC("%s: check\n", dpu_crtc->name);
>   
> -	/* force a full mode set if active state changed */
> -	if (crtc_state->active_changed)
> -		crtc_state->mode_changed = true;
> -
>   	if (cstate->num_mixers) {
>   		rc = _dpu_crtc_check_and_setup_lm_bounds(crtc, crtc_state);
>   		if (rc)
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 83de7564e2c1fe14fcf8c4f82335cafc937e1b99..d1ccdca6044353f110bf5b507788efe368f223a3 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -793,12 +793,11 @@ static int dpu_encoder_virt_atomic_check(
>   		crtc_state->mode_changed = true;
>   	/*
>   	 * Release and Allocate resources on every modeset
> -	 * Dont allocate when active is false.
>   	 */
>   	if (drm_atomic_crtc_needs_modeset(crtc_state)) {
>   		dpu_rm_release(global_state, drm_enc);
>   
> -		if (!crtc_state->active_changed || crtc_state->enable)

I think this was leftover code.

What happened was, we used to have dpu_rm_reserve() both in 
dpu_encoder's atomic_check and mode_set. Hence this is checking 
!active_changed because that case was expected to get handled in 
mode_set to avoid duplicate dpu_rm_reserve() calls. Code has progressed 
since then to drop the dpu_rm_reserve() from mode_set and only use 
atomic_check.

So the correct fixes tag for this should be:

Fixes: de3916c70a24 ("drm/msm/dpu: Track resources in global state")

With that addressed, this is

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

> +		if (crtc_state->enable)
>   			ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
>   					drm_enc, crtc_state, topology);
>   		if (!ret)
> 

Re: [PATCH 3/6] drm/msm/dpu: don't use active in atomic_check()

[Dmitry Baryshkov]

On Wed, Jan 08, 2025 at 05:19:40PM -0800, Abhinav Kumar wrote:
> 
> 
> On 12/21/2024 9:00 PM, Dmitry Baryshkov wrote:
> > The driver isn't supposed to consult crtc_state->active/active_check for
> > resource allocation. Instead all resources should be allocated if
> > crtc_state->enabled is set. Stop consulting active / active_changed in
> > order to determine whether the hardware resources should be
> > (re)allocated.
> > 
> > Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
> > Reported-by: Simona Vetter <simona.vetter@ffwll.ch>
> > Closes: https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 4 ----
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 +--
> >   2 files changed, 1 insertion(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index 7191b1a6d41b3a96f956d199398f12b2923e8c82..65e33eba61726929b740831c95330756b2817e28 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -1264,10 +1264,6 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
> >   	DRM_DEBUG_ATOMIC("%s: check\n", dpu_crtc->name);
> > -	/* force a full mode set if active state changed */
> > -	if (crtc_state->active_changed)
> > -		crtc_state->mode_changed = true;
> > -
> >   	if (cstate->num_mixers) {
> >   		rc = _dpu_crtc_check_and_setup_lm_bounds(crtc, crtc_state);
> >   		if (rc)
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index 83de7564e2c1fe14fcf8c4f82335cafc937e1b99..d1ccdca6044353f110bf5b507788efe368f223a3 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -793,12 +793,11 @@ static int dpu_encoder_virt_atomic_check(
> >   		crtc_state->mode_changed = true;
> >   	/*
> >   	 * Release and Allocate resources on every modeset
> > -	 * Dont allocate when active is false.
> >   	 */
> >   	if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> >   		dpu_rm_release(global_state, drm_enc);
> > -		if (!crtc_state->active_changed || crtc_state->enable)
> 
> I think this was leftover code.
> 
> What happened was, we used to have dpu_rm_reserve() both in dpu_encoder's
> atomic_check and mode_set. Hence this is checking !active_changed because
> that case was expected to get handled in mode_set to avoid duplicate
> dpu_rm_reserve() calls. Code has progressed since then to drop the
> dpu_rm_reserve() from mode_set and only use atomic_check.
> 
> So the correct fixes tag for this should be:
> 
> Fixes: de3916c70a24 ("drm/msm/dpu: Track resources in global state")

Actually it should be:

Fixes: ccc862b957c6 ("drm/msm/dpu: Fix reservation failures in modeset")

> With that addressed, this is
> 
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> 
> > +		if (crtc_state->enable)
> >   			ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
> >   					drm_enc, crtc_state, topology);
> >   		if (!ret)
> > 

-- 
With best wishes
Dmitry

Re: [PATCH 3/6] drm/msm/dpu: don't use active in atomic_check()

[Abhinav Kumar]

On 1/8/2025 8:22 PM, Dmitry Baryshkov wrote:
> On Wed, Jan 08, 2025 at 05:19:40PM -0800, Abhinav Kumar wrote:
>>
>>
>> On 12/21/2024 9:00 PM, Dmitry Baryshkov wrote:
>>> The driver isn't supposed to consult crtc_state->active/active_check for
>>> resource allocation. Instead all resources should be allocated if
>>> crtc_state->enabled is set. Stop consulting active / active_changed in
>>> order to determine whether the hardware resources should be
>>> (re)allocated.
>>>
>>> Fixes: 25fdd5933e4c ("drm/msm: Add SDM845 DPU support")
>>> Reported-by: Simona Vetter <simona.vetter@ffwll.ch>
>>> Closes: https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    | 4 ----
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 3 +--
>>>    2 files changed, 1 insertion(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> index 7191b1a6d41b3a96f956d199398f12b2923e8c82..65e33eba61726929b740831c95330756b2817e28 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
>>> @@ -1264,10 +1264,6 @@ static int dpu_crtc_atomic_check(struct drm_crtc *crtc,
>>>    	DRM_DEBUG_ATOMIC("%s: check\n", dpu_crtc->name);
>>> -	/* force a full mode set if active state changed */
>>> -	if (crtc_state->active_changed)
>>> -		crtc_state->mode_changed = true;
>>> -
>>>    	if (cstate->num_mixers) {
>>>    		rc = _dpu_crtc_check_and_setup_lm_bounds(crtc, crtc_state);
>>>    		if (rc)
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> index 83de7564e2c1fe14fcf8c4f82335cafc937e1b99..d1ccdca6044353f110bf5b507788efe368f223a3 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>> @@ -793,12 +793,11 @@ static int dpu_encoder_virt_atomic_check(
>>>    		crtc_state->mode_changed = true;
>>>    	/*
>>>    	 * Release and Allocate resources on every modeset
>>> -	 * Dont allocate when active is false.
>>>    	 */
>>>    	if (drm_atomic_crtc_needs_modeset(crtc_state)) {
>>>    		dpu_rm_release(global_state, drm_enc);
>>> -		if (!crtc_state->active_changed || crtc_state->enable)
>>
>> I think this was leftover code.
>>
>> What happened was, we used to have dpu_rm_reserve() both in dpu_encoder's
>> atomic_check and mode_set. Hence this is checking !active_changed because
>> that case was expected to get handled in mode_set to avoid duplicate
>> dpu_rm_reserve() calls. Code has progressed since then to drop the
>> dpu_rm_reserve() from mode_set and only use atomic_check.
>>
>> So the correct fixes tag for this should be:
>>
>> Fixes: de3916c70a24 ("drm/msm/dpu: Track resources in global state")
> 
> Actually it should be:
> 
> Fixes: ccc862b957c6 ("drm/msm/dpu: Fix reservation failures in modeset")
> 

Ack.

>> With that addressed, this is
>>
>> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>
>>> +		if (crtc_state->enable)
>>>    			ret = dpu_rm_reserve(&dpu_kms->rm, global_state,
>>>    					drm_enc, crtc_state, topology);
>>>    		if (!ret)
>>>
> 

[PATCH 4/6] drm/msm/dpu: move needs_cdm setting to dpu_encoder_get_topology()

[Dmitry Baryshkov]

As a preparation for calling dpu_encoder_get_topology() from different
places, move the code setting topology->needs_cdm to that function
(instead of patching topology separately).

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 41 ++++++++++++++++-------------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index d1ccdca6044353f110bf5b507788efe368f223a3..88690191a9c9485e052d37749d1b61f50328916e 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -652,8 +652,11 @@ static struct msm_display_topology dpu_encoder_get_topology(
 			struct dpu_kms *dpu_kms,
 			struct drm_display_mode *mode,
 			struct drm_crtc_state *crtc_state,
+			struct drm_connector_state *conn_state,
 			struct drm_dsc_config *dsc)
 {
+	struct msm_drm_private *priv = dpu_enc->base.dev->dev_private;
+	struct msm_display_info *disp_info = &dpu_enc->disp_info;
 	struct msm_display_topology topology = {0};
 	int i, intf_count = 0;
 
@@ -696,6 +699,23 @@ static struct msm_display_topology dpu_encoder_get_topology(
 		topology.num_intf = 1;
 	}
 
+	/*
+	 * Use CDM only for writeback or DP at the moment as other interfaces cannot handle it.
+	 * If writeback itself cannot handle cdm for some reason it will fail in its atomic_check()
+	 * earlier.
+	 */
+	if (disp_info->intf_type == INTF_WB && conn_state->writeback_job) {
+		struct drm_framebuffer *fb;
+
+		fb = conn_state->writeback_job->fb;
+
+		if (fb && MSM_FORMAT_IS_YUV(msm_framebuffer_format(fb)))
+			topology.needs_cdm = true;
+	} else if (disp_info->intf_type == INTF_DP) {
+		if (msm_dp_is_yuv_420_enabled(priv->dp[disp_info->h_tile_instance[0]], mode))
+			topology.needs_cdm = true;
+	}
+
 	return topology;
 }
 
@@ -743,9 +763,7 @@ static int dpu_encoder_virt_atomic_check(
 	struct dpu_kms *dpu_kms;
 	struct drm_display_mode *adj_mode;
 	struct msm_display_topology topology;
-	struct msm_display_info *disp_info;
 	struct dpu_global_state *global_state;
-	struct drm_framebuffer *fb;
 	struct drm_dsc_config *dsc;
 	int ret = 0;
 
@@ -759,7 +777,6 @@ static int dpu_encoder_virt_atomic_check(
 	DPU_DEBUG_ENC(dpu_enc, "\n");
 
 	priv = drm_enc->dev->dev_private;
-	disp_info = &dpu_enc->disp_info;
 	dpu_kms = to_dpu_kms(priv->kms);
 	adj_mode = &crtc_state->adjusted_mode;
 	global_state = dpu_kms_get_global_state(crtc_state->state);
@@ -770,22 +787,8 @@ static int dpu_encoder_virt_atomic_check(
 
 	dsc = dpu_encoder_get_dsc_config(drm_enc);
 
-	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state, dsc);
-
-	/*
-	 * Use CDM only for writeback or DP at the moment as other interfaces cannot handle it.
-	 * If writeback itself cannot handle cdm for some reason it will fail in its atomic_check()
-	 * earlier.
-	 */
-	if (disp_info->intf_type == INTF_WB && conn_state->writeback_job) {
-		fb = conn_state->writeback_job->fb;
-
-		if (fb && MSM_FORMAT_IS_YUV(msm_framebuffer_format(fb)))
-			topology.needs_cdm = true;
-	} else if (disp_info->intf_type == INTF_DP) {
-		if (msm_dp_is_yuv_420_enabled(priv->dp[disp_info->h_tile_instance[0]], adj_mode))
-			topology.needs_cdm = true;
-	}
+	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state, conn_state,
+					    dsc);
 
 	if (topology.needs_cdm && !dpu_enc->cur_master->hw_cdm)
 		crtc_state->mode_changed = true;

-- 
2.39.5

Re: [PATCH 4/6] drm/msm/dpu: move needs_cdm setting to dpu_encoder_get_topology()

[Abhinav Kumar]

On 12/21/2024 9:00 PM, Dmitry Baryshkov wrote:
> As a preparation for calling dpu_encoder_get_topology() from different
> places, move the code setting topology->needs_cdm to that function
> (instead of patching topology separately).
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 41 ++++++++++++++++-------------
>   1 file changed, 22 insertions(+), 19 deletions(-)
> 

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

[PATCH 5/6] drm/msm/dpu: simplify dpu_encoder_get_topology() interface

[Dmitry Baryshkov]

As a preparation for calling dpu_encoder_get_topology() from different
code paths, simplify its calling interface, obtaining some data pointers
internally instead passing them via arguments.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 12 ++++--------
 1 file changed, 4 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 88690191a9c9485e052d37749d1b61f50328916e..209e6fb605b2d8724935b62001032e7d39540366 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -649,14 +649,14 @@ struct drm_dsc_config *dpu_encoder_get_dsc_config(struct drm_encoder *drm_enc)
 
 static struct msm_display_topology dpu_encoder_get_topology(
 			struct dpu_encoder_virt *dpu_enc,
-			struct dpu_kms *dpu_kms,
 			struct drm_display_mode *mode,
 			struct drm_crtc_state *crtc_state,
-			struct drm_connector_state *conn_state,
-			struct drm_dsc_config *dsc)
+			struct drm_connector_state *conn_state)
 {
 	struct msm_drm_private *priv = dpu_enc->base.dev->dev_private;
 	struct msm_display_info *disp_info = &dpu_enc->disp_info;
+	struct dpu_kms *dpu_kms = to_dpu_kms(priv->kms);
+	struct drm_dsc_config *dsc = dpu_encoder_get_dsc_config(&dpu_enc->base);
 	struct msm_display_topology topology = {0};
 	int i, intf_count = 0;
 
@@ -764,7 +764,6 @@ static int dpu_encoder_virt_atomic_check(
 	struct drm_display_mode *adj_mode;
 	struct msm_display_topology topology;
 	struct dpu_global_state *global_state;
-	struct drm_dsc_config *dsc;
 	int ret = 0;
 
 	if (!drm_enc || !crtc_state || !conn_state) {
@@ -785,10 +784,7 @@ static int dpu_encoder_virt_atomic_check(
 
 	trace_dpu_enc_atomic_check(DRMID(drm_enc));
 
-	dsc = dpu_encoder_get_dsc_config(drm_enc);
-
-	topology = dpu_encoder_get_topology(dpu_enc, dpu_kms, adj_mode, crtc_state, conn_state,
-					    dsc);
+	topology = dpu_encoder_get_topology(dpu_enc, adj_mode, crtc_state, conn_state);
 
 	if (topology.needs_cdm && !dpu_enc->cur_master->hw_cdm)
 		crtc_state->mode_changed = true;

-- 
2.39.5

Re: [PATCH 5/6] drm/msm/dpu: simplify dpu_encoder_get_topology() interface

[Abhinav Kumar]

On 12/21/2024 9:00 PM, Dmitry Baryshkov wrote:
> As a preparation for calling dpu_encoder_get_topology() from different
> code paths, simplify its calling interface, obtaining some data pointers
> internally instead passing them via arguments.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 12 ++++--------
>   1 file changed, 4 insertions(+), 8 deletions(-)
> 

Nice!

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

[PATCH 6/6] drm/msm/dpu: don't set crtc_state->mode_changed from atomic_check()

[Dmitry Baryshkov]

The MSM driver uses drm_atomic_helper_check() which mandates that none
of the atomic_check() callbacks toggles crtc_state->mode_changed.
Perform corresponding check before calling the drm_atomic_helper_check()
function.

Fixes: 8b45a26f2ba9 ("drm/msm/dpu: reserve cdm blocks for writeback in case of YUV output")
Reported-by: Simona Vetter <simona.vetter@ffwll.ch>
Closes: https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 32 +++++++++++++++++++++++++----
 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            | 13 +++++++++++-
 drivers/gpu/drm/msm/msm_kms.h               |  7 +++++++
 5 files changed, 77 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 209e6fb605b2d8724935b62001032e7d39540366..b7c3aa8d0e2ca58091deacdeaccb0819d2bf045c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -753,6 +753,34 @@ 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)
+{
+	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
+	struct msm_display_topology topology;
+
+	DPU_DEBUG_ENC(dpu_enc, "\n");
+
+	/* 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;
+	else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
+		crtc_state->mode_changed = true;
+
+	return 0;
+}
+
 static int dpu_encoder_virt_atomic_check(
 		struct drm_encoder *drm_enc,
 		struct drm_crtc_state *crtc_state,
@@ -786,10 +814,6 @@ static int dpu_encoder_virt_atomic_check(
 
 	topology = dpu_encoder_get_topology(dpu_enc, adj_mode, crtc_state, conn_state);
 
-	if (topology.needs_cdm && !dpu_enc->cur_master->hw_cdm)
-		crtc_state->mode_changed = true;
-	else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
-		crtc_state->mode_changed = true;
 	/*
 	 * Release and Allocate resources on every modeset
 	 */
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
index 92b5ee390788d16e85e195a664417896a2bf1cae..da133ee4701a329f566f6f9a7255f2f6d050f891 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
@@ -88,4 +88,8 @@ 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 dae8a94d3366abfb8937d5f44d8968f1d0691c2d..e2d822f7d785dc0debcb28595029a3e2050b0cf4 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -446,6 +446,31 @@ 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);
@@ -1049,6 +1074,7 @@ 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,
diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index a7a2384044ffdb13579cc9a10f56f8de9beca761..364df245e3a209094782ca1b47b752a729b32a5b 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -183,10 +183,16 @@ static unsigned get_crtc_mask(struct drm_atomic_state *state)
 
 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;
+	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) ||
@@ -196,6 +202,11 @@ int msm_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
 		}
 	}
 
+	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);
 }
 
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index e60162744c669773b6e5aef824a173647626ab4e..ec2a75af89b09754faef1a07adc9338f7d78161e 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -59,6 +59,13 @@ 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 6/6] drm/msm/dpu: don't set crtc_state->mode_changed from atomic_check()

[Simona Vetter]

On Sun, Dec 22, 2024 at 07:00:46AM +0200, Dmitry Baryshkov wrote:
> The MSM driver uses drm_atomic_helper_check() which mandates that none
> of the atomic_check() callbacks toggles crtc_state->mode_changed.
> Perform corresponding check before calling the drm_atomic_helper_check()
> function.
> 
> Fixes: 8b45a26f2ba9 ("drm/msm/dpu: reserve cdm blocks for writeback in case of YUV output")
> Reported-by: Simona Vetter <simona.vetter@ffwll.ch>
> Closes: https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 32 +++++++++++++++++++++++++----
>  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            | 13 +++++++++++-
>  drivers/gpu/drm/msm/msm_kms.h               |  7 +++++++
>  5 files changed, 77 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 209e6fb605b2d8724935b62001032e7d39540366..b7c3aa8d0e2ca58091deacdeaccb0819d2bf045c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -753,6 +753,34 @@ 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)
> +{
> +	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> +	struct msm_display_topology topology;
> +
> +	DPU_DEBUG_ENC(dpu_enc, "\n");
> +
> +	/* 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;
> +	else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
> +		crtc_state->mode_changed = true;
> +
> +	return 0;
> +}
> +
>  static int dpu_encoder_virt_atomic_check(
>  		struct drm_encoder *drm_enc,
>  		struct drm_crtc_state *crtc_state,
> @@ -786,10 +814,6 @@ static int dpu_encoder_virt_atomic_check(
>  
>  	topology = dpu_encoder_get_topology(dpu_enc, adj_mode, crtc_state, conn_state);
>  
> -	if (topology.needs_cdm && !dpu_enc->cur_master->hw_cdm)
> -		crtc_state->mode_changed = true;
> -	else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
> -		crtc_state->mode_changed = true;
>  	/*
>  	 * Release and Allocate resources on every modeset
>  	 */
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 92b5ee390788d16e85e195a664417896a2bf1cae..da133ee4701a329f566f6f9a7255f2f6d050f891 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -88,4 +88,8 @@ 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 dae8a94d3366abfb8937d5f44d8968f1d0691c2d..e2d822f7d785dc0debcb28595029a3e2050b0cf4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -446,6 +446,31 @@ 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);
> @@ -1049,6 +1074,7 @@ 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,
> diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
> index a7a2384044ffdb13579cc9a10f56f8de9beca761..364df245e3a209094782ca1b47b752a729b32a5b 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -183,10 +183,16 @@ static unsigned get_crtc_mask(struct drm_atomic_state *state)
>  
>  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;
> +	int i, ret = 0;
>  
> +	/*
> +	 * FIXME: stop setting allow_modeset and move this check to the DPU
> +	 * driver.
> +	 */

I guess there's more work to stop setting allow_modeset? Or was the issue
there that it breaks userspace that expects ctm changes to be doable
without modesets?

Either way msm patches lgtm, but don't feel confident enough for acks
except on the first one that reworks the active_change logic to use
crtc->enable instead for resource allocation.
-Sima

>  	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
>  				      new_crtc_state, i) {
>  		if ((old_crtc_state->ctm && !new_crtc_state->ctm) ||
> @@ -196,6 +202,11 @@ int msm_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>  		}
>  	}
>  
> +	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);
>  }
>  
> diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> index e60162744c669773b6e5aef824a173647626ab4e..ec2a75af89b09754faef1a07adc9338f7d78161e 100644
> --- a/drivers/gpu/drm/msm/msm_kms.h
> +++ b/drivers/gpu/drm/msm/msm_kms.h
> @@ -59,6 +59,13 @@ 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
> 

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

Re: [PATCH 6/6] drm/msm/dpu: don't set crtc_state->mode_changed from atomic_check()

[Dmitry Baryshkov]

On Wed, 8 Jan 2025 at 19:55, Simona Vetter <simona.vetter@ffwll.ch> wrote:
>
> On Sun, Dec 22, 2024 at 07:00:46AM +0200, Dmitry Baryshkov wrote:
> > The MSM driver uses drm_atomic_helper_check() which mandates that none
> > of the atomic_check() callbacks toggles crtc_state->mode_changed.
> > Perform corresponding check before calling the drm_atomic_helper_check()
> > function.
> >
> > Fixes: 8b45a26f2ba9 ("drm/msm/dpu: reserve cdm blocks for writeback in case of YUV output")
> > Reported-by: Simona Vetter <simona.vetter@ffwll.ch>
> > Closes: https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 32 +++++++++++++++++++++++++----
> >  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            | 13 +++++++++++-
> >  drivers/gpu/drm/msm/msm_kms.h               |  7 +++++++
> >  5 files changed, 77 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index 209e6fb605b2d8724935b62001032e7d39540366..b7c3aa8d0e2ca58091deacdeaccb0819d2bf045c 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -753,6 +753,34 @@ 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)
> > +{
> > +     struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> > +     struct msm_display_topology topology;
> > +
> > +     DPU_DEBUG_ENC(dpu_enc, "\n");
> > +
> > +     /* 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;
> > +     else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
> > +             crtc_state->mode_changed = true;
> > +
> > +     return 0;
> > +}
> > +
> >  static int dpu_encoder_virt_atomic_check(
> >               struct drm_encoder *drm_enc,
> >               struct drm_crtc_state *crtc_state,
> > @@ -786,10 +814,6 @@ static int dpu_encoder_virt_atomic_check(
> >
> >       topology = dpu_encoder_get_topology(dpu_enc, adj_mode, crtc_state, conn_state);
> >
> > -     if (topology.needs_cdm && !dpu_enc->cur_master->hw_cdm)
> > -             crtc_state->mode_changed = true;
> > -     else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
> > -             crtc_state->mode_changed = true;
> >       /*
> >        * Release and Allocate resources on every modeset
> >        */
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > index 92b5ee390788d16e85e195a664417896a2bf1cae..da133ee4701a329f566f6f9a7255f2f6d050f891 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > @@ -88,4 +88,8 @@ 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 dae8a94d3366abfb8937d5f44d8968f1d0691c2d..e2d822f7d785dc0debcb28595029a3e2050b0cf4 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> > @@ -446,6 +446,31 @@ 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);
> > @@ -1049,6 +1074,7 @@ 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,
> > diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
> > index a7a2384044ffdb13579cc9a10f56f8de9beca761..364df245e3a209094782ca1b47b752a729b32a5b 100644
> > --- a/drivers/gpu/drm/msm/msm_atomic.c
> > +++ b/drivers/gpu/drm/msm/msm_atomic.c
> > @@ -183,10 +183,16 @@ static unsigned get_crtc_mask(struct drm_atomic_state *state)
> >
> >  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;
> > +     int i, ret = 0;
> >
> > +     /*
> > +      * FIXME: stop setting allow_modeset and move this check to the DPU
> > +      * driver.
> > +      */
>
> I guess there's more work to stop setting allow_modeset? Or was the issue
> there that it breaks userspace that expects ctm changes to be doable
> without modesets?

Yes. And I currently have no way to check that userspace, so for me
it's easier to add a TODO rather than to risk breaking it.
And with your patch going in I think we should add a check that the
allow_modeset flag hasn't been tampered with.

>
> Either way msm patches lgtm, but don't feel confident enough for acks
> except on the first one that reworks the active_change logic to use
> crtc->enable instead for resource allocation.
> -Sima
>
> >       for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
> >                                     new_crtc_state, i) {
> >               if ((old_crtc_state->ctm && !new_crtc_state->ctm) ||
> > @@ -196,6 +202,11 @@ int msm_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
> >               }
> >       }
> >
> > +     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);
> >  }
> >
> > diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> > index e60162744c669773b6e5aef824a173647626ab4e..ec2a75af89b09754faef1a07adc9338f7d78161e 100644
> > --- a/drivers/gpu/drm/msm/msm_kms.h
> > +++ b/drivers/gpu/drm/msm/msm_kms.h
> > @@ -59,6 +59,13 @@ 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
> >
>
> --
> Simona Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch



-- 
With best wishes
Dmitry

Re: [PATCH 6/6] drm/msm/dpu: don't set crtc_state->mode_changed from atomic_check()

[Abhinav Kumar]

On 12/21/2024 9:00 PM, Dmitry Baryshkov wrote:
> The MSM driver uses drm_atomic_helper_check() which mandates that none
> of the atomic_check() callbacks toggles crtc_state->mode_changed.
> Perform corresponding check before calling the drm_atomic_helper_check()
> function.
> 
> Fixes: 8b45a26f2ba9 ("drm/msm/dpu: reserve cdm blocks for writeback in case of YUV output")
> Reported-by: Simona Vetter <simona.vetter@ffwll.ch>
> Closes: https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 32 +++++++++++++++++++++++++----
>   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            | 13 +++++++++++-
>   drivers/gpu/drm/msm/msm_kms.h               |  7 +++++++
>   5 files changed, 77 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 209e6fb605b2d8724935b62001032e7d39540366..b7c3aa8d0e2ca58091deacdeaccb0819d2bf045c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -753,6 +753,34 @@ 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)
> +{
> +	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> +	struct msm_display_topology topology;
> +
> +	DPU_DEBUG_ENC(dpu_enc, "\n");
> +
> +	/* 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;
> +	else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
> +		crtc_state->mode_changed = true;
> +
> +	return 0;
> +}

How will this work exactly?

needs_cdm is set in the encoder's atomic_check which is called inside 
drm_atomic_helper_check(). But this function is called before that.

So needs_cdm will never hit.


> +
>   static int dpu_encoder_virt_atomic_check(
>   		struct drm_encoder *drm_enc,
>   		struct drm_crtc_state *crtc_state,
> @@ -786,10 +814,6 @@ static int dpu_encoder_virt_atomic_check(
>   
>   	topology = dpu_encoder_get_topology(dpu_enc, adj_mode, crtc_state, conn_state);
>   
> -	if (topology.needs_cdm && !dpu_enc->cur_master->hw_cdm)
> -		crtc_state->mode_changed = true;
> -	else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
> -		crtc_state->mode_changed = true;
>   	/*
>   	 * Release and Allocate resources on every modeset
>   	 */
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> index 92b5ee390788d16e85e195a664417896a2bf1cae..da133ee4701a329f566f6f9a7255f2f6d050f891 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> @@ -88,4 +88,8 @@ 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 dae8a94d3366abfb8937d5f44d8968f1d0691c2d..e2d822f7d785dc0debcb28595029a3e2050b0cf4 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -446,6 +446,31 @@ 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);
> @@ -1049,6 +1074,7 @@ 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,
> diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
> index a7a2384044ffdb13579cc9a10f56f8de9beca761..364df245e3a209094782ca1b47b752a729b32a5b 100644
> --- a/drivers/gpu/drm/msm/msm_atomic.c
> +++ b/drivers/gpu/drm/msm/msm_atomic.c
> @@ -183,10 +183,16 @@ static unsigned get_crtc_mask(struct drm_atomic_state *state)
>   
>   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;
> +	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) ||
> @@ -196,6 +202,11 @@ int msm_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
>   		}
>   	}
>   
> +	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);
>   }
>   
> diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
> index e60162744c669773b6e5aef824a173647626ab4e..ec2a75af89b09754faef1a07adc9338f7d78161e 100644
> --- a/drivers/gpu/drm/msm/msm_kms.h
> +++ b/drivers/gpu/drm/msm/msm_kms.h
> @@ -59,6 +59,13 @@ 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.
> 

Re: [PATCH 6/6] drm/msm/dpu: don't set crtc_state->mode_changed from atomic_check()

[Abhinav Kumar]

On 1/8/2025 6:27 PM, Abhinav Kumar wrote:
> 
> 
> On 12/21/2024 9:00 PM, Dmitry Baryshkov wrote:
>> The MSM driver uses drm_atomic_helper_check() which mandates that none
>> of the atomic_check() callbacks toggles crtc_state->mode_changed.
>> Perform corresponding check before calling the drm_atomic_helper_check()
>> function.
>>
>> Fixes: 8b45a26f2ba9 ("drm/msm/dpu: reserve cdm blocks for writeback in 
>> case of YUV output")
>> Reported-by: Simona Vetter <simona.vetter@ffwll.ch>
>> Closes: 
>> https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 32 
>> +++++++++++++++++++++++++----
>>   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            | 13 +++++++++++-
>>   drivers/gpu/drm/msm/msm_kms.h               |  7 +++++++
>>   5 files changed, 77 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> index 
>> 209e6fb605b2d8724935b62001032e7d39540366..b7c3aa8d0e2ca58091deacdeaccb0819d2bf045c 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> @@ -753,6 +753,34 @@ 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)
>> +{
>> +    struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
>> +    struct msm_display_topology topology;
>> +
>> +    DPU_DEBUG_ENC(dpu_enc, "\n");
>> +
>> +    /* 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;
>> +    else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
>> +        crtc_state->mode_changed = true;
>> +
>> +    return 0;
>> +}
> 
> How will this work exactly?
> 
> needs_cdm is set in the encoder's atomic_check which is called inside 
> drm_atomic_helper_check(). But this function is called before that.
> 
> So needs_cdm will never hit.
>

Sorry, my bad. after change (4) of this series needs_cdm is also 
populated within  dpu_encoder_get_topology().

To follow up on 
https://patchwork.freedesktop.org/patch/629231/?series=137975&rev=4#comment_1148651

So is the plan for CWB to add a dpu_crtc_check_mode_changed() like 
dpu_encoder's and call it?


> 
>> +
>>   static int dpu_encoder_virt_atomic_check(
>>           struct drm_encoder *drm_enc,
>>           struct drm_crtc_state *crtc_state,
>> @@ -786,10 +814,6 @@ static int dpu_encoder_virt_atomic_check(
>>       topology = dpu_encoder_get_topology(dpu_enc, adj_mode, 
>> crtc_state, conn_state);
>> -    if (topology.needs_cdm && !dpu_enc->cur_master->hw_cdm)
>> -        crtc_state->mode_changed = true;
>> -    else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
>> -        crtc_state->mode_changed = true;
>>       /*
>>        * Release and Allocate resources on every modeset
>>        */
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> index 
>> 92b5ee390788d16e85e195a664417896a2bf1cae..da133ee4701a329f566f6f9a7255f2f6d050f891 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
>> @@ -88,4 +88,8 @@ 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 
>> dae8a94d3366abfb8937d5f44d8968f1d0691c2d..e2d822f7d785dc0debcb28595029a3e2050b0cf4 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -446,6 +446,31 @@ 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);
>> @@ -1049,6 +1074,7 @@ 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,
>> diff --git a/drivers/gpu/drm/msm/msm_atomic.c 
>> b/drivers/gpu/drm/msm/msm_atomic.c
>> index 
>> a7a2384044ffdb13579cc9a10f56f8de9beca761..364df245e3a209094782ca1b47b752a729b32a5b 100644
>> --- a/drivers/gpu/drm/msm/msm_atomic.c
>> +++ b/drivers/gpu/drm/msm/msm_atomic.c
>> @@ -183,10 +183,16 @@ static unsigned get_crtc_mask(struct 
>> drm_atomic_state *state)
>>   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;
>> +    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) ||
>> @@ -196,6 +202,11 @@ int msm_atomic_check(struct drm_device *dev, 
>> struct drm_atomic_state *state)
>>           }
>>       }
>> +    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);
>>   }
>> diff --git a/drivers/gpu/drm/msm/msm_kms.h 
>> b/drivers/gpu/drm/msm/msm_kms.h
>> index 
>> e60162744c669773b6e5aef824a173647626ab4e..ec2a75af89b09754faef1a07adc9338f7d78161e 100644
>> --- a/drivers/gpu/drm/msm/msm_kms.h
>> +++ b/drivers/gpu/drm/msm/msm_kms.h
>> @@ -59,6 +59,13 @@ 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.
>>

Re: [PATCH 6/6] drm/msm/dpu: don't set crtc_state->mode_changed from atomic_check()

[Dmitry Baryshkov]

On Wed, Jan 08, 2025 at 08:11:27PM -0800, Abhinav Kumar wrote:
> 
> 
> On 1/8/2025 6:27 PM, Abhinav Kumar wrote:
> > 
> > 
> > On 12/21/2024 9:00 PM, Dmitry Baryshkov wrote:
> > > The MSM driver uses drm_atomic_helper_check() which mandates that none
> > > of the atomic_check() callbacks toggles crtc_state->mode_changed.
> > > Perform corresponding check before calling the drm_atomic_helper_check()
> > > function.
> > > 
> > > Fixes: 8b45a26f2ba9 ("drm/msm/dpu: reserve cdm blocks for writeback
> > > in case of YUV output")
> > > Reported-by: Simona Vetter <simona.vetter@ffwll.ch>
> > > Closes:
> > > https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > >   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 32
> > > +++++++++++++++++++++++++----
> > >   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            | 13 +++++++++++-
> > >   drivers/gpu/drm/msm/msm_kms.h               |  7 +++++++
> > >   5 files changed, 77 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > index 209e6fb605b2d8724935b62001032e7d39540366..b7c3aa8d0e2ca58091deacdeaccb0819d2bf045c
> > > 100644
> > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > @@ -753,6 +753,34 @@ 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)
> > > +{
> > > +    struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> > > +    struct msm_display_topology topology;
> > > +
> > > +    DPU_DEBUG_ENC(dpu_enc, "\n");
> > > +
> > > +    /* 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;
> > > +    else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
> > > +        crtc_state->mode_changed = true;
> > > +
> > > +    return 0;
> > > +}
> > 
> > How will this work exactly?
> > 
> > needs_cdm is set in the encoder's atomic_check which is called inside
> > drm_atomic_helper_check(). But this function is called before that.
> > 
> > So needs_cdm will never hit.
> > 
> 
> Sorry, my bad. after change (4) of this series needs_cdm is also populated
> within  dpu_encoder_get_topology().
> 
> To follow up on https://patchwork.freedesktop.org/patch/629231/?series=137975&rev=4#comment_1148651
> 
> So is the plan for CWB to add a dpu_crtc_check_mode_changed() like
> dpu_encoder's and call it?

I think dpu_encoder_virt_check_mode_changed() would transform into the
dpu_crtc_check_mode_changed() together with one of the patches that
moves resource allocation and refactors topology handling.

> 
> 
> > 
> > > +
> > >   static int dpu_encoder_virt_atomic_check(
> > >           struct drm_encoder *drm_enc,
> > >           struct drm_crtc_state *crtc_state,

-- 
With best wishes
Dmitry

Re: [PATCH 6/6] drm/msm/dpu: don't set crtc_state->mode_changed from atomic_check()

[Abhinav Kumar]

On 1/8/2025 8:26 PM, Dmitry Baryshkov wrote:
> On Wed, Jan 08, 2025 at 08:11:27PM -0800, Abhinav Kumar wrote:
>>
>>
>> On 1/8/2025 6:27 PM, Abhinav Kumar wrote:
>>>
>>>
>>> On 12/21/2024 9:00 PM, Dmitry Baryshkov wrote:
>>>> The MSM driver uses drm_atomic_helper_check() which mandates that none
>>>> of the atomic_check() callbacks toggles crtc_state->mode_changed.
>>>> Perform corresponding check before calling the drm_atomic_helper_check()
>>>> function.
>>>>
>>>> Fixes: 8b45a26f2ba9 ("drm/msm/dpu: reserve cdm blocks for writeback
>>>> in case of YUV output")
>>>> Reported-by: Simona Vetter <simona.vetter@ffwll.ch>
>>>> Closes:
>>>> https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 32
>>>> +++++++++++++++++++++++++----
>>>>    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            | 13 +++++++++++-
>>>>    drivers/gpu/drm/msm/msm_kms.h               |  7 +++++++
>>>>    5 files changed, 77 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> index 209e6fb605b2d8724935b62001032e7d39540366..b7c3aa8d0e2ca58091deacdeaccb0819d2bf045c
>>>> 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>>>> @@ -753,6 +753,34 @@ 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)
>>>> +{
>>>> +    struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
>>>> +    struct msm_display_topology topology;
>>>> +
>>>> +    DPU_DEBUG_ENC(dpu_enc, "\n");
>>>> +
>>>> +    /* 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;
>>>> +    else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
>>>> +        crtc_state->mode_changed = true;
>>>> +
>>>> +    return 0;
>>>> +}
>>>
>>> How will this work exactly?
>>>
>>> needs_cdm is set in the encoder's atomic_check which is called inside
>>> drm_atomic_helper_check(). But this function is called before that.
>>>
>>> So needs_cdm will never hit.
>>>
>>
>> Sorry, my bad. after change (4) of this series needs_cdm is also populated
>> within  dpu_encoder_get_topology().
>>
>> To follow up on https://patchwork.freedesktop.org/patch/629231/?series=137975&rev=4#comment_1148651
>>
>> So is the plan for CWB to add a dpu_crtc_check_mode_changed() like
>> dpu_encoder's and call it?
> 
> I think dpu_encoder_virt_check_mode_changed() would transform into the
> dpu_crtc_check_mode_changed() together with one of the patches that
> moves resource allocation and refactors topology handling.
>

hmm we need the cur_master for cdm. That will not be accessible in 
dpu_crtc.c so we will end up with a separate 
dpu_crtc_check_mode_changed() for CWB from what I see. We will discuss 
it further when we re-post CWB.

But overall, I think we can make CWB work on top of this.

Hence,

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

I do not know how important patch 2 is for this series and I would 
prefer not delaying CWB even more than what it already has been.

If we cannot reach a conclusion on patch 2, can you break that one out 
of this series so that the rest of it is ready to land?

>>
>>
>>>
>>>> +
>>>>    static int dpu_encoder_virt_atomic_check(
>>>>            struct drm_encoder *drm_enc,
>>>>            struct drm_crtc_state *crtc_state,
> 

Re: [PATCH 6/6] drm/msm/dpu: don't set crtc_state->mode_changed from atomic_check()

[Dmitry Baryshkov]

On Wed, Jan 08, 2025 at 09:22:56PM -0800, Abhinav Kumar wrote:
> 
> 
> On 1/8/2025 8:26 PM, Dmitry Baryshkov wrote:
> > On Wed, Jan 08, 2025 at 08:11:27PM -0800, Abhinav Kumar wrote:
> > > 
> > > 
> > > On 1/8/2025 6:27 PM, Abhinav Kumar wrote:
> > > > 
> > > > 
> > > > On 12/21/2024 9:00 PM, Dmitry Baryshkov wrote:
> > > > > The MSM driver uses drm_atomic_helper_check() which mandates that none
> > > > > of the atomic_check() callbacks toggles crtc_state->mode_changed.
> > > > > Perform corresponding check before calling the drm_atomic_helper_check()
> > > > > function.
> > > > > 
> > > > > Fixes: 8b45a26f2ba9 ("drm/msm/dpu: reserve cdm blocks for writeback
> > > > > in case of YUV output")
> > > > > Reported-by: Simona Vetter <simona.vetter@ffwll.ch>
> > > > > Closes:
> > > > > https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
> > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > ---
> > > > >    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 32
> > > > > +++++++++++++++++++++++++----
> > > > >    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            | 13 +++++++++++-
> > > > >    drivers/gpu/drm/msm/msm_kms.h               |  7 +++++++
> > > > >    5 files changed, 77 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > > > index 209e6fb605b2d8724935b62001032e7d39540366..b7c3aa8d0e2ca58091deacdeaccb0819d2bf045c
> > > > > 100644
> > > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > > > @@ -753,6 +753,34 @@ 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)
> > > > > +{
> > > > > +    struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> > > > > +    struct msm_display_topology topology;
> > > > > +
> > > > > +    DPU_DEBUG_ENC(dpu_enc, "\n");
> > > > > +
> > > > > +    /* 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;
> > > > > +    else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
> > > > > +        crtc_state->mode_changed = true;
> > > > > +
> > > > > +    return 0;
> > > > > +}
> > > > 
> > > > How will this work exactly?
> > > > 
> > > > needs_cdm is set in the encoder's atomic_check which is called inside
> > > > drm_atomic_helper_check(). But this function is called before that.
> > > > 
> > > > So needs_cdm will never hit.
> > > > 
> > > 
> > > Sorry, my bad. after change (4) of this series needs_cdm is also populated
> > > within  dpu_encoder_get_topology().
> > > 
> > > To follow up on https://patchwork.freedesktop.org/patch/629231/?series=137975&rev=4#comment_1148651
> > > 
> > > So is the plan for CWB to add a dpu_crtc_check_mode_changed() like
> > > dpu_encoder's and call it?
> > 
> > I think dpu_encoder_virt_check_mode_changed() would transform into the
> > dpu_crtc_check_mode_changed() together with one of the patches that
> > moves resource allocation and refactors topology handling.
> > 
> 
> hmm we need the cur_master for cdm. That will not be accessible in
> dpu_crtc.c so we will end up with a separate dpu_crtc_check_mode_changed()
> for CWB from what I see. We will discuss it further when we re-post CWB.
> 
> But overall, I think we can make CWB work on top of this.
> 
> Hence,
> 
> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> 
> I do not know how important patch 2 is for this series and I would prefer
> not delaying CWB even more than what it already has been.
> 
> If we cannot reach a conclusion on patch 2, can you break that one out of
> this series so that the rest of it is ready to land?

Yes, there is no dependency between patches 1-2 and 3-6.

> 
> > > 
> > > 
> > > > 
> > > > > +
> > > > >    static int dpu_encoder_virt_atomic_check(
> > > > >            struct drm_encoder *drm_enc,
> > > > >            struct drm_crtc_state *crtc_state,
> > 

-- 
With best wishes
Dmitry

Re: [PATCH 6/6] drm/msm/dpu: don't set crtc_state->mode_changed from atomic_check()

[Dmitry Baryshkov]

On Wed, Jan 08, 2025 at 06:27:13PM -0800, Abhinav Kumar wrote:
> 
> 
> On 12/21/2024 9:00 PM, Dmitry Baryshkov wrote:
> > The MSM driver uses drm_atomic_helper_check() which mandates that none
> > of the atomic_check() callbacks toggles crtc_state->mode_changed.
> > Perform corresponding check before calling the drm_atomic_helper_check()
> > function.
> > 
> > Fixes: 8b45a26f2ba9 ("drm/msm/dpu: reserve cdm blocks for writeback in case of YUV output")
> > Reported-by: Simona Vetter <simona.vetter@ffwll.ch>
> > Closes: https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 32 +++++++++++++++++++++++++----
> >   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            | 13 +++++++++++-
> >   drivers/gpu/drm/msm/msm_kms.h               |  7 +++++++
> >   5 files changed, 77 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index 209e6fb605b2d8724935b62001032e7d39540366..b7c3aa8d0e2ca58091deacdeaccb0819d2bf045c 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -753,6 +753,34 @@ 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)
> > +{
> > +	struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> > +	struct msm_display_topology topology;
> > +
> > +	DPU_DEBUG_ENC(dpu_enc, "\n");
> > +
> > +	/* 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;
> > +	else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
> > +		crtc_state->mode_changed = true;
> > +
> > +	return 0;
> > +}
> 
> How will this work exactly?
> 
> needs_cdm is set in the encoder's atomic_check which is called inside
> drm_atomic_helper_check(). But this function is called before that.
> 
> So needs_cdm will never hit.
> 

Please refer to the previous patch, it should answer your question.

> 
> > +
> >   static int dpu_encoder_virt_atomic_check(
> >   		struct drm_encoder *drm_enc,
> >   		struct drm_crtc_state *crtc_state,
> > @@ -786,10 +814,6 @@ static int dpu_encoder_virt_atomic_check(
> >   	topology = dpu_encoder_get_topology(dpu_enc, adj_mode, crtc_state, conn_state);
> > -	if (topology.needs_cdm && !dpu_enc->cur_master->hw_cdm)
> > -		crtc_state->mode_changed = true;
> > -	else if (!topology.needs_cdm && dpu_enc->cur_master->hw_cdm)
> > -		crtc_state->mode_changed = true;
> >   	/*
> >   	 * Release and Allocate resources on every modeset
> >   	 */

-- 
With best wishes
Dmitry

Re: [PATCH 0/6] drm: enforce rules for drm_atomic_helper_check_modeset()

[Thomas Zimmermann]

Hi


Am 22.12.24 um 06:00 schrieb Dmitry Baryshkov:
> As pointed out by Simona, the drm_atomic_helper_check_modeset() and
> drm_atomic_helper_check() require the former function is rerun if the
> driver's callbacks modify crtc_state->mode_changed. MSM is one of the
> drivers which failed to follow this requirement.

I'm concerned about the implications of this series. How does a driver 
upgrade from simple pageflip to full modeset if necessary? The solution 
in msm appears to be to run the related test before 
drm_atomic_helper_check(). (Right?)

My corner case is in mgag200, which has to reprogram the PLL if the 
color mode changes. So it sets mode_changed to true in the primary 
plane's atomic_check. [1] This works in practice because the plane 
checks run before the CRTC checks. So the CRTC code will do the correct 
thing. Reprogramming the PLL means to disable the display at some point. 
So it comes down to a full modeset.

You mention that drm_atomic_helper_check() needs to rerun if 
mode_changed flips. Would it be possible to implement this instead 
within the helper?

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v6.12/source/drivers/gpu/drm/mgag200/mgag200_mode.c#L493

>
> As suggested by Simona, implement generic code to verify that the
> drivers abide to those requirement and rework MSM driver to follow that
> restrictions.
>
> There are no dependencies between core and MSM parts, so they can go
> separately via corresponding trees.
>
> Reported-by: Simona Vetter <simona.vetter@ffwll.ch>
> Link: https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
> Dmitry Baryshkov (6):
>        drm/atomic-helper: document drm_atomic_helper_check() restrictions
>        drm/atomic: prepare to check that drivers follow restrictions for needs_modeset
>        drm/msm/dpu: don't use active in atomic_check()
>        drm/msm/dpu: move needs_cdm setting to dpu_encoder_get_topology()
>        drm/msm/dpu: simplify dpu_encoder_get_topology() interface
>        drm/msm/dpu: don't set crtc_state->mode_changed from atomic_check()
>
>   drivers/gpu/drm/drm_atomic.c                |  3 +
>   drivers/gpu/drm/drm_atomic_helper.c         | 86 ++++++++++++++++++++++++++---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    |  4 --
>   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 82 +++++++++++++++++----------
>   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            | 13 ++++-
>   drivers/gpu/drm/msm/msm_kms.h               |  7 +++
>   include/drm/drm_atomic.h                    | 10 ++++
>   9 files changed, 192 insertions(+), 43 deletions(-)
> ---
> base-commit: b72747fdde637ebf52e181671bf6f41cd773b3e1
> change-id: 20241222-drm-dirty-modeset-88079bd27ae6
>
> Best regards,

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH 0/6] drm: enforce rules for drm_atomic_helper_check_modeset()

[Dmitry Baryshkov]

On Thu, Jan 09, 2025 at 02:53:16PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> 
> Am 22.12.24 um 06:00 schrieb Dmitry Baryshkov:
> > As pointed out by Simona, the drm_atomic_helper_check_modeset() and
> > drm_atomic_helper_check() require the former function is rerun if the
> > driver's callbacks modify crtc_state->mode_changed. MSM is one of the
> > drivers which failed to follow this requirement.
> 
> I'm concerned about the implications of this series. How does a driver
> upgrade from simple pageflip to full modeset if necessary? The solution in
> msm appears to be to run the related test before drm_atomic_helper_check().
> (Right?)
> 
> My corner case is in mgag200, which has to reprogram the PLL if the color
> mode changes. So it sets mode_changed to true in the primary plane's
> atomic_check. [1] This works in practice because the plane checks run before
> the CRTC checks. So the CRTC code will do the correct thing. Reprogramming
> the PLL means to disable the display at some point. So it comes down to a
> full modeset.
> 
> You mention that drm_atomic_helper_check() needs to rerun if mode_changed
> flips. Would it be possible to implement this instead within the helper?

I think this should be a driver's decision. For MSM it was easier to
move the mode_changed changes and to isolate that before calling into
the drm_atomic_helper_check_modeset() code. Other drivers might prefer
to rerun the helper.

> 
> Best regards
> Thomas
> 
> [1] https://elixir.bootlin.com/linux/v6.12/source/drivers/gpu/drm/mgag200/mgag200_mode.c#L493
> 
> > 
> > As suggested by Simona, implement generic code to verify that the
> > drivers abide to those requirement and rework MSM driver to follow that
> > restrictions.
> > 
> > There are no dependencies between core and MSM parts, so they can go
> > separately via corresponding trees.
> > 
> > Reported-by: Simona Vetter <simona.vetter@ffwll.ch>
> > Link: https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> > Dmitry Baryshkov (6):
> >        drm/atomic-helper: document drm_atomic_helper_check() restrictions
> >        drm/atomic: prepare to check that drivers follow restrictions for needs_modeset
> >        drm/msm/dpu: don't use active in atomic_check()
> >        drm/msm/dpu: move needs_cdm setting to dpu_encoder_get_topology()
> >        drm/msm/dpu: simplify dpu_encoder_get_topology() interface
> >        drm/msm/dpu: don't set crtc_state->mode_changed from atomic_check()
> > 
> >   drivers/gpu/drm/drm_atomic.c                |  3 +
> >   drivers/gpu/drm/drm_atomic_helper.c         | 86 ++++++++++++++++++++++++++---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    |  4 --
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 82 +++++++++++++++++----------
> >   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            | 13 ++++-
> >   drivers/gpu/drm/msm/msm_kms.h               |  7 +++
> >   include/drm/drm_atomic.h                    | 10 ++++
> >   9 files changed, 192 insertions(+), 43 deletions(-)
> > ---
> > base-commit: b72747fdde637ebf52e181671bf6f41cd773b3e1
> > change-id: 20241222-drm-dirty-modeset-88079bd27ae6
> > 
> > Best regards,
> 
> -- 
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstrasse 146, 90461 Nuernberg, Germany
> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
> HRB 36809 (AG Nuernberg)
> 

-- 
With best wishes
Dmitry

Re: [PATCH 0/6] drm: enforce rules for drm_atomic_helper_check_modeset()

[Thomas Zimmermann]

Hi


Am 10.01.25 um 00:57 schrieb Dmitry Baryshkov:
> On Thu, Jan 09, 2025 at 02:53:16PM +0100, Thomas Zimmermann wrote:
>> Hi
>>
>>
>> Am 22.12.24 um 06:00 schrieb Dmitry Baryshkov:
>>> As pointed out by Simona, the drm_atomic_helper_check_modeset() and
>>> drm_atomic_helper_check() require the former function is rerun if the
>>> driver's callbacks modify crtc_state->mode_changed. MSM is one of the
>>> drivers which failed to follow this requirement.
>> I'm concerned about the implications of this series. How does a driver
>> upgrade from simple pageflip to full modeset if necessary? The solution in
>> msm appears to be to run the related test before drm_atomic_helper_check().
>> (Right?)
>>
>> My corner case is in mgag200, which has to reprogram the PLL if the color
>> mode changes. So it sets mode_changed to true in the primary plane's
>> atomic_check. [1] This works in practice because the plane checks run before
>> the CRTC checks. So the CRTC code will do the correct thing. Reprogramming
>> the PLL means to disable the display at some point. So it comes down to a
>> full modeset.
>>
>> You mention that drm_atomic_helper_check() needs to rerun if mode_changed
>> flips. Would it be possible to implement this instead within the helper?
> I think this should be a driver's decision. For MSM it was easier to
> move the mode_changed changes and to isolate that before calling into
> the drm_atomic_helper_check_modeset() code. Other drivers might prefer
> to rerun the helper.

Is it legal to do something like

int atomic_check(state)
{
   ret = drm_atomic_helper_check(state)
   if (state->dirty_needs_modeset)
     ret = drm_atomic_helper_check(state)
   return ret;
}

within the driver ? It appears that the atomic helpers warn then.

Best regards
Thomas

>
>> Best regards
>> Thomas
>>
>> [1] https://elixir.bootlin.com/linux/v6.12/source/drivers/gpu/drm/mgag200/mgag200_mode.c#L493
>>
>>> As suggested by Simona, implement generic code to verify that the
>>> drivers abide to those requirement and rework MSM driver to follow that
>>> restrictions.
>>>
>>> There are no dependencies between core and MSM parts, so they can go
>>> separately via corresponding trees.
>>>
>>> Reported-by: Simona Vetter <simona.vetter@ffwll.ch>
>>> Link: https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>>> Dmitry Baryshkov (6):
>>>         drm/atomic-helper: document drm_atomic_helper_check() restrictions
>>>         drm/atomic: prepare to check that drivers follow restrictions for needs_modeset
>>>         drm/msm/dpu: don't use active in atomic_check()
>>>         drm/msm/dpu: move needs_cdm setting to dpu_encoder_get_topology()
>>>         drm/msm/dpu: simplify dpu_encoder_get_topology() interface
>>>         drm/msm/dpu: don't set crtc_state->mode_changed from atomic_check()
>>>
>>>    drivers/gpu/drm/drm_atomic.c                |  3 +
>>>    drivers/gpu/drm/drm_atomic_helper.c         | 86 ++++++++++++++++++++++++++---
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    |  4 --
>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 82 +++++++++++++++++----------
>>>    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            | 13 ++++-
>>>    drivers/gpu/drm/msm/msm_kms.h               |  7 +++
>>>    include/drm/drm_atomic.h                    | 10 ++++
>>>    9 files changed, 192 insertions(+), 43 deletions(-)
>>> ---
>>> base-commit: b72747fdde637ebf52e181671bf6f41cd773b3e1
>>> change-id: 20241222-drm-dirty-modeset-88079bd27ae6
>>>
>>> Best regards,
>> -- 
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Frankenstrasse 146, 90461 Nuernberg, Germany
>> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
>> HRB 36809 (AG Nuernberg)
>>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH 0/6] drm: enforce rules for drm_atomic_helper_check_modeset()

[Dmitry Baryshkov]

On Fri, Jan 10, 2025 at 02:30:55PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> 
> Am 10.01.25 um 00:57 schrieb Dmitry Baryshkov:
> > On Thu, Jan 09, 2025 at 02:53:16PM +0100, Thomas Zimmermann wrote:
> > > Hi
> > > 
> > > 
> > > Am 22.12.24 um 06:00 schrieb Dmitry Baryshkov:
> > > > As pointed out by Simona, the drm_atomic_helper_check_modeset() and
> > > > drm_atomic_helper_check() require the former function is rerun if the
> > > > driver's callbacks modify crtc_state->mode_changed. MSM is one of the
> > > > drivers which failed to follow this requirement.
> > > I'm concerned about the implications of this series. How does a driver
> > > upgrade from simple pageflip to full modeset if necessary? The solution in
> > > msm appears to be to run the related test before drm_atomic_helper_check().
> > > (Right?)
> > > 
> > > My corner case is in mgag200, which has to reprogram the PLL if the color
> > > mode changes. So it sets mode_changed to true in the primary plane's
> > > atomic_check. [1] This works in practice because the plane checks run before
> > > the CRTC checks. So the CRTC code will do the correct thing. Reprogramming
> > > the PLL means to disable the display at some point. So it comes down to a
> > > full modeset.
> > > 
> > > You mention that drm_atomic_helper_check() needs to rerun if mode_changed
> > > flips. Would it be possible to implement this instead within the helper?
> > I think this should be a driver's decision. For MSM it was easier to
> > move the mode_changed changes and to isolate that before calling into
> > the drm_atomic_helper_check_modeset() code. Other drivers might prefer
> > to rerun the helper.
> 
> Is it legal to do something like
> 
> int atomic_check(state)
> {
>   ret = drm_atomic_helper_check(state)
>   if (state->dirty_needs_modeset)
>     ret = drm_atomic_helper_check(state)
>   return ret;
> }
> 
> within the driver ? It appears that the atomic helpers warn then.

No, it is legal if I understand it correctly. The warning comes up if
the dirty_needs_modeset is set after the driver returns from its
atomic_check() callback, see drm_atomic_check_only() patched in the
second patch.

> 
> Best regards
> Thomas
> 
> > 
> > > Best regards
> > > Thomas
> > > 
> > > [1] https://elixir.bootlin.com/linux/v6.12/source/drivers/gpu/drm/mgag200/mgag200_mode.c#L493
> > > 
> > > > As suggested by Simona, implement generic code to verify that the
> > > > drivers abide to those requirement and rework MSM driver to follow that
> > > > restrictions.
> > > > 
> > > > There are no dependencies between core and MSM parts, so they can go
> > > > separately via corresponding trees.
> > > > 
> > > > Reported-by: Simona Vetter <simona.vetter@ffwll.ch>
> > > > Link: https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > ---
> > > > Dmitry Baryshkov (6):
> > > >         drm/atomic-helper: document drm_atomic_helper_check() restrictions
> > > >         drm/atomic: prepare to check that drivers follow restrictions for needs_modeset
> > > >         drm/msm/dpu: don't use active in atomic_check()
> > > >         drm/msm/dpu: move needs_cdm setting to dpu_encoder_get_topology()
> > > >         drm/msm/dpu: simplify dpu_encoder_get_topology() interface
> > > >         drm/msm/dpu: don't set crtc_state->mode_changed from atomic_check()
> > > > 
> > > >    drivers/gpu/drm/drm_atomic.c                |  3 +
> > > >    drivers/gpu/drm/drm_atomic_helper.c         | 86 ++++++++++++++++++++++++++---
> > > >    drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    |  4 --
> > > >    drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 82 +++++++++++++++++----------
> > > >    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            | 13 ++++-
> > > >    drivers/gpu/drm/msm/msm_kms.h               |  7 +++
> > > >    include/drm/drm_atomic.h                    | 10 ++++
> > > >    9 files changed, 192 insertions(+), 43 deletions(-)
> > > > ---
> > > > base-commit: b72747fdde637ebf52e181671bf6f41cd773b3e1
> > > > change-id: 20241222-drm-dirty-modeset-88079bd27ae6
> > > > 
> > > > Best regards,
> > > -- 
> > > --
> > > Thomas Zimmermann
> > > Graphics Driver Developer
> > > SUSE Software Solutions Germany GmbH
> > > Frankenstrasse 146, 90461 Nuernberg, Germany
> > > GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
> > > HRB 36809 (AG Nuernberg)
> > > 
> 
> -- 
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstrasse 146, 90461 Nuernberg, Germany
> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
> HRB 36809 (AG Nuernberg)
> 

-- 
With best wishes
Dmitry

Re: [PATCH 0/6] drm: enforce rules for drm_atomic_helper_check_modeset()

[Dmitry Baryshkov]

On Thu, Jan 09, 2025 at 02:53:16PM +0100, Thomas Zimmermann wrote:
> Hi
> 
> 
> Am 22.12.24 um 06:00 schrieb Dmitry Baryshkov:
> > As pointed out by Simona, the drm_atomic_helper_check_modeset() and
> > drm_atomic_helper_check() require the former function is rerun if the
> > driver's callbacks modify crtc_state->mode_changed. MSM is one of the
> > drivers which failed to follow this requirement.
> 
> I'm concerned about the implications of this series. How does a driver
> upgrade from simple pageflip to full modeset if necessary? The solution in
> msm appears to be to run the related test before drm_atomic_helper_check().
> (Right?)
> 
> My corner case is in mgag200, which has to reprogram the PLL if the color
> mode changes. So it sets mode_changed to true in the primary plane's
> atomic_check. [1] This works in practice because the plane checks run before
> the CRTC checks. So the CRTC code will do the correct thing. Reprogramming
> the PLL means to disable the display at some point. So it comes down to a
> full modeset.

After giving this a second thought, I see an obvious issue from the
generic code perspective. If you set new_crtc_state->mode_changed from
your atomic_check(), then it's already too late for
drm_atomic_helper_check_modeset() to add all affected (aka routed to the
CRTC) planes to the state. Their atomic_check() callback will be
skipped. So even if the end-result works in the MGAg200 case at this
moment, I think we should still disallow that.

Another option might be similar to what we had to do in the DPU driver:
check whether the mode_changed has to be set before calling
drm_atomic_helper_check(). Maybe we should consider adding new set of
callbacks to the drm_*_helper_funcs, which are executed at the beginning
of the drm_atomic_helper_check_modeset().

WDYT?

> You mention that drm_atomic_helper_check() needs to rerun if mode_changed
> flips. Would it be possible to implement this instead within the helper?
> 
> Best regards
> Thomas
> 
> [1] https://elixir.bootlin.com/linux/v6.12/source/drivers/gpu/drm/mgag200/mgag200_mode.c#L493
> 
> > 
> > As suggested by Simona, implement generic code to verify that the
> > drivers abide to those requirement and rework MSM driver to follow that
> > restrictions.
> > 
> > There are no dependencies between core and MSM parts, so they can go
> > separately via corresponding trees.
> > 
> > Reported-by: Simona Vetter <simona.vetter@ffwll.ch>
> > Link: https://lore.kernel.org/dri-devel/ZtW_S0j5AEr4g0QW@phenom.ffwll.local/
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> > Dmitry Baryshkov (6):
> >        drm/atomic-helper: document drm_atomic_helper_check() restrictions
> >        drm/atomic: prepare to check that drivers follow restrictions for needs_modeset
> >        drm/msm/dpu: don't use active in atomic_check()
> >        drm/msm/dpu: move needs_cdm setting to dpu_encoder_get_topology()
> >        drm/msm/dpu: simplify dpu_encoder_get_topology() interface
> >        drm/msm/dpu: don't set crtc_state->mode_changed from atomic_check()
> > 
> >   drivers/gpu/drm/drm_atomic.c                |  3 +
> >   drivers/gpu/drm/drm_atomic_helper.c         | 86 ++++++++++++++++++++++++++---
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    |  4 --
> >   drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 82 +++++++++++++++++----------
> >   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            | 13 ++++-
> >   drivers/gpu/drm/msm/msm_kms.h               |  7 +++
> >   include/drm/drm_atomic.h                    | 10 ++++
> >   9 files changed, 192 insertions(+), 43 deletions(-)
> > ---
> > base-commit: b72747fdde637ebf52e181671bf6f41cd773b3e1
> > change-id: 20241222-drm-dirty-modeset-88079bd27ae6
> > 
> > Best regards,
> 
> -- 
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstrasse 146, 90461 Nuernberg, Germany
> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
> HRB 36809 (AG Nuernberg)
> 

-- 
With best wishes
Dmitry

Re: (subset) [PATCH 0/6] drm: enforce rules for drm_atomic_helper_check_modeset()

[Dmitry Baryshkov]

On Sun, 22 Dec 2024 07:00:40 +0200, Dmitry Baryshkov wrote:
> As pointed out by Simona, the drm_atomic_helper_check_modeset() and
> drm_atomic_helper_check() require the former function is rerun if the
> driver's callbacks modify crtc_state->mode_changed. MSM is one of the
> drivers which failed to follow this requirement.
> 
> As suggested by Simona, implement generic code to verify that the
> drivers abide to those requirement and rework MSM driver to follow that
> restrictions.
> 
> [...]

Applied to drm-misc-next, thanks!

[1/6] drm/atomic-helper: document drm_atomic_helper_check() restrictions
      commit: 0936f0e54426177b0f0263ddf806ed5e13487db6

Best regards,
-- 
With best wishes
Dmitry