[PATCH v3 0/4] drm/atomic: Atomic Private State debugging

[PATCH v3 0/4] drm/atomic: Atomic Private State debugging

[Maxime Ripard]

Hi,

This series adds an atomic_print_state hook for drm_private_obj to ease the
debugging of driver-specific sub-classes, and adds one for vc4.

It also changes the call site of drm_atomic_print_new_state to make it more
consistent.

Let me know what you think,
Maxime

Changes from v2:
  - Reworded the commit message of the first patch
  - Printed the state before running atomic_check

Changes from v1:
  - Added Daniel tags
  - Added drm_private_state documentation
  - Fixed unused variable warning
  - Removed the drm_atomic_nonblocking_commit() logging

Maxime Ripard (4):
  drm/atomic: Print the state every non-blocking commit
  drm/atomic: Add atomic_print_state to private objects
  drm/vc4: Constify private state accessors
  drm/vc4: Implement atomic_print_state for HVS channel state

 drivers/gpu/drm/drm_atomic.c      | 20 ++++++++++++++++++++
 drivers/gpu/drm/drm_atomic_uapi.c |  4 ----
 drivers/gpu/drm/vc4/vc4_kms.c     | 24 +++++++++++++++++++++---
 include/drm/drm_atomic.h          | 27 +++++++++++++++++++++++----
 4 files changed, 64 insertions(+), 11 deletions(-)

-- 
2.35.1

[PATCH v3 1/4] drm/atomic: Print the state every non-blocking commit

[Maxime Ripard]

The DRM_UT_STATE controls whether we're calling
drm_atomic_print_new_state() whenever a new state is committed. However,
that call is made in the drm_mode_atomic_ioctl(), whereas we have
multiple users of the drm_atomic_commit() function in the kernel
(framebuffer emulation, drm_atomic_helper_dirtyfb, etc.).

This leads to multiple states being committed but never actually
displayed even though we asked to have verbose atomic state debugging.

Let's move the call to drm_atomic_print_new_state() to
drm_atomic_commit() to make sure we don't miss any. Non-blocking commits
were never logged though, and it would create too much churn in the logs
to do so, so leave them out for now.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/drm_atomic.c      | 4 ++++
 drivers/gpu/drm/drm_atomic_uapi.c | 4 ----
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 88cd992df356..637df126c2d9 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1423,8 +1423,12 @@ EXPORT_SYMBOL(drm_atomic_check_only);
 int drm_atomic_commit(struct drm_atomic_state *state)
 {
 	struct drm_mode_config *config = &state->dev->mode_config;
+	struct drm_printer p = drm_info_printer(state->dev->dev);
 	int ret;
 
+	if (drm_debug_enabled(DRM_UT_STATE))
+		drm_atomic_print_new_state(state, &p);
+
 	ret = drm_atomic_check_only(state);
 	if (ret)
 		return ret;
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 9781722519c3..45e6d3c62a9a 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -1326,7 +1326,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	struct drm_out_fence_state *fence_state;
 	int ret = 0;
 	unsigned int i, j, num_fences;
-	struct drm_printer p = drm_info_printer(dev->dev);
 
 	/* disallow for drivers not supporting atomic: */
 	if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
@@ -1458,9 +1457,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
 	} else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
 		ret = drm_atomic_nonblocking_commit(state);
 	} else {
-		if (drm_debug_enabled(DRM_UT_STATE))
-			drm_atomic_print_new_state(state, &p);
-
 		ret = drm_atomic_commit(state);
 	}
 
-- 
2.35.1

[PATCH v3 2/4] drm/atomic: Add atomic_print_state to private objects

[Maxime Ripard]

A number of drivers (amdgpu, komeda, vc4, etc.) leverage the
drm_private_state structure, but we don't have any infrastructure to
provide debugging like we do for the other components state. Let's add
an atomic_print_state hook to be consistent.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/drm_atomic.c | 16 ++++++++++++++++
 include/drm/drm_atomic.h     | 27 +++++++++++++++++++++++----
 2 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 637df126c2d9..58c0283fb6b0 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -789,6 +789,8 @@ drm_atomic_private_obj_init(struct drm_device *dev,
 	obj->state = state;
 	obj->funcs = funcs;
 	list_add_tail(&obj->head, &dev->mode_config.privobj_list);
+
+	state->obj = obj;
 }
 EXPORT_SYMBOL(drm_atomic_private_obj_init);
 
@@ -1636,6 +1638,15 @@ int __drm_atomic_helper_set_config(struct drm_mode_set *set,
 }
 EXPORT_SYMBOL(__drm_atomic_helper_set_config);
 
+static void drm_atomic_private_obj_print_state(struct drm_printer *p,
+					       const struct drm_private_state *state)
+{
+	struct drm_private_obj *obj = state->obj;
+
+	if (obj->funcs->atomic_print_state)
+		obj->funcs->atomic_print_state(p, state);
+}
+
 /**
  * drm_atomic_print_new_state - prints drm atomic state
  * @state: atomic configuration to check
@@ -1656,6 +1667,8 @@ void drm_atomic_print_new_state(const struct drm_atomic_state *state,
 	struct drm_crtc_state *crtc_state;
 	struct drm_connector *connector;
 	struct drm_connector_state *connector_state;
+	struct drm_private_obj *obj;
+	struct drm_private_state *obj_state;
 	int i;
 
 	if (!p) {
@@ -1673,6 +1686,9 @@ void drm_atomic_print_new_state(const struct drm_atomic_state *state,
 
 	for_each_new_connector_in_state(state, connector, connector_state, i)
 		drm_atomic_connector_print_state(p, connector_state);
+
+	for_each_new_private_obj_in_state(state, obj, obj_state, i)
+		drm_atomic_private_obj_print_state(p, obj_state);
 }
 EXPORT_SYMBOL(drm_atomic_print_new_state);
 
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 1701c2128a5c..0777725085df 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -227,6 +227,18 @@ struct drm_private_state_funcs {
 	 */
 	void (*atomic_destroy_state)(struct drm_private_obj *obj,
 				     struct drm_private_state *state);
+
+	/**
+	 * @atomic_print_state:
+	 *
+	 * If driver subclasses &struct drm_private_state, it should implement
+	 * this optional hook for printing additional driver specific state.
+	 *
+	 * Do not call this directly, use drm_atomic_private_obj_print_state()
+	 * instead.
+	 */
+	void (*atomic_print_state)(struct drm_printer *p,
+				   const struct drm_private_state *state);
 };
 
 /**
@@ -311,14 +323,21 @@ struct drm_private_obj {
 
 /**
  * struct drm_private_state - base struct for driver private object state
- * @state: backpointer to global drm_atomic_state
  *
- * Currently only contains a backpointer to the overall atomic update, but in
- * the future also might hold synchronization information similar to e.g.
- * &drm_crtc.commit.
+ * Currently only contains a backpointer to the overall atomic update,
+ * and the relevant private object but in the future also might hold
+ * synchronization information similar to e.g. &drm_crtc.commit.
  */
 struct drm_private_state {
+	/**
+	 * @state: backpointer to global drm_atomic_state
+	 */
 	struct drm_atomic_state *state;
+
+	/**
+	 * @obj: backpointer to the private object
+	 */
+	struct drm_private_obj *obj;
 };
 
 struct __drm_private_objs_state {
-- 
2.35.1

[PATCH v3 3/4] drm/vc4: Constify private state accessors

[Maxime Ripard]

None of those helpers modify the pointed data, let's make them const.

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_kms.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 24de29bc1cda..26b771c919b1 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -32,7 +32,8 @@ struct vc4_ctm_state {
 	int fifo;
 };
 
-static struct vc4_ctm_state *to_vc4_ctm_state(struct drm_private_state *priv)
+static struct vc4_ctm_state *
+to_vc4_ctm_state(const struct drm_private_state *priv)
 {
 	return container_of(priv, struct vc4_ctm_state, base);
 }
@@ -49,7 +50,7 @@ struct vc4_hvs_state {
 };
 
 static struct vc4_hvs_state *
-to_vc4_hvs_state(struct drm_private_state *priv)
+to_vc4_hvs_state(const struct drm_private_state *priv)
 {
 	return container_of(priv, struct vc4_hvs_state, base);
 }
@@ -61,7 +62,7 @@ struct vc4_load_tracker_state {
 };
 
 static struct vc4_load_tracker_state *
-to_vc4_load_tracker_state(struct drm_private_state *priv)
+to_vc4_load_tracker_state(const struct drm_private_state *priv)
 {
 	return container_of(priv, struct vc4_load_tracker_state, base);
 }
-- 
2.35.1

[PATCH v3 4/4] drm/vc4: Implement atomic_print_state for HVS channel state

[Maxime Ripard]

The HVS state configuration is useful when debugging what's going on in
the vc4 hardware pipeline. Add an implementation of .atomic_print_state.

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_kms.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 26b771c919b1..bffd81d4bfcf 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -701,9 +701,26 @@ static void vc4_hvs_channels_destroy_state(struct drm_private_obj *obj,
 	kfree(hvs_state);
 }
 
+static void vc4_hvs_channels_print_state(struct drm_printer *p,
+					 const struct drm_private_state *state)
+{
+	struct vc4_hvs_state *hvs_state = to_vc4_hvs_state(state);
+	unsigned int i;
+
+	drm_printf(p, "HVS State\n");
+	drm_printf(p, "\tCore Clock Rate: %lu\n", hvs_state->core_clock_rate);
+
+	for (i = 0; i < HVS_NUM_CHANNELS; i++) {
+		drm_printf(p, "\tChannel %d\n", i);
+		drm_printf(p, "\t\tin use=%d\n", hvs_state->fifo_state[i].in_use);
+		drm_printf(p, "\t\tload=%lu\n", hvs_state->fifo_state[i].fifo_load);
+	}
+}
+
 static const struct drm_private_state_funcs vc4_hvs_state_funcs = {
 	.atomic_duplicate_state = vc4_hvs_channels_duplicate_state,
 	.atomic_destroy_state = vc4_hvs_channels_destroy_state,
+	.atomic_print_state = vc4_hvs_channels_print_state,
 };
 
 static void vc4_hvs_channels_obj_fini(struct drm_device *dev, void *unused)
-- 
2.35.1

Re: [PATCH v3 1/4] drm/atomic: Print the state every non-blocking commit

[Daniel Vetter]

On Mon, Mar 28, 2022 at 02:43:01PM +0200, Maxime Ripard wrote:
> The DRM_UT_STATE controls whether we're calling
> drm_atomic_print_new_state() whenever a new state is committed. However,
> that call is made in the drm_mode_atomic_ioctl(), whereas we have
> multiple users of the drm_atomic_commit() function in the kernel
> (framebuffer emulation, drm_atomic_helper_dirtyfb, etc.).
> 
> This leads to multiple states being committed but never actually
> displayed even though we asked to have verbose atomic state debugging.
> 
> Let's move the call to drm_atomic_print_new_state() to
> drm_atomic_commit() to make sure we don't miss any. Non-blocking commits
> were never logged though, and it would create too much churn in the logs
> to do so, so leave them out for now.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_atomic.c      | 4 ++++
>  drivers/gpu/drm/drm_atomic_uapi.c | 4 ----
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 88cd992df356..637df126c2d9 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1423,8 +1423,12 @@ EXPORT_SYMBOL(drm_atomic_check_only);
>  int drm_atomic_commit(struct drm_atomic_state *state)
>  {
>  	struct drm_mode_config *config = &state->dev->mode_config;
> +	struct drm_printer p = drm_info_printer(state->dev->dev);
>  	int ret;
>  
> +	if (drm_debug_enabled(DRM_UT_STATE))
> +		drm_atomic_print_new_state(state, &p);
> +
>  	ret = drm_atomic_check_only(state);
>  	if (ret)
>  		return ret;
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 9781722519c3..45e6d3c62a9a 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -1326,7 +1326,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  	struct drm_out_fence_state *fence_state;
>  	int ret = 0;
>  	unsigned int i, j, num_fences;
> -	struct drm_printer p = drm_info_printer(dev->dev);
>  
>  	/* disallow for drivers not supporting atomic: */
>  	if (!drm_core_check_feature(dev, DRIVER_ATOMIC))
> @@ -1458,9 +1457,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
>  	} else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
>  		ret = drm_atomic_nonblocking_commit(state);
>  	} else {
> -		if (drm_debug_enabled(DRM_UT_STATE))
> -			drm_atomic_print_new_state(state, &p);
> -
>  		ret = drm_atomic_commit(state);
>  	}
>  
> -- 
> 2.35.1
> 

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

Re: [PATCH v3 0/4] drm/atomic: Atomic Private State debugging

[Maxime Ripard]

On Mon, 28 Mar 2022 14:43:00 +0200, Maxime Ripard wrote:
> This series adds an atomic_print_state hook for drm_private_obj to ease the
> debugging of driver-specific sub-classes, and adds one for vc4.
> 
> It also changes the call site of drm_atomic_print_new_state to make it more
> consistent.
> 
> Let me know what you think,
> Maxime
> 
> [...]

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime