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

[PATCH v2 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 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 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 v2 1/4] drm/atomic: Print the state every 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.). Similarly,
it's only called for a blocking atomic commit.

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.

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..73865c147b4b 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1423,6 +1423,7 @@ 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;
 
 	ret = drm_atomic_check_only(state);
@@ -1431,6 +1432,9 @@ int drm_atomic_commit(struct drm_atomic_state *state)
 
 	drm_dbg_atomic(state->dev, "committing %p\n", state);
 
+	if (drm_debug_enabled(DRM_UT_STATE))
+		drm_atomic_print_new_state(state, &p);
+
 	return config->funcs->atomic_commit(state->dev, state, false);
 }
 EXPORT_SYMBOL(drm_atomic_commit);
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

Re: [PATCH v2 1/4] drm/atomic: Print the state every commit

[Daniel Vetter]

On Fri, Mar 25, 2022 at 01:48:19PM +0100, 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.). Similarly,
> it's only called for a blocking atomic commit.
> 
> 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.

Commit message doesn't match the patch anymore.

> 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..73865c147b4b 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1423,6 +1423,7 @@ 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;
>  
>  	ret = drm_atomic_check_only(state);
> @@ -1431,6 +1432,9 @@ int drm_atomic_commit(struct drm_atomic_state *state)
>  
>  	drm_dbg_atomic(state->dev, "committing %p\n", state);
>  
> +	if (drm_debug_enabled(DRM_UT_STATE))
> +		drm_atomic_print_new_state(state, &p);

Just realized that this changes things now, you no longer print the state
when it fails the atomic check. Not sure we want that behaviour change ...
-Daniel

> +
>  	return config->funcs->atomic_commit(state->dev, state, false);
>  }
>  EXPORT_SYMBOL(drm_atomic_commit);
> 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

[PATCH v2 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 73865c147b4b..7a52a9f4f487 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

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

[kernel test robot]

Hi Maxime,

I love your patch! Perhaps something to improve:

[auto build test WARNING on drm/drm-next]
[also build test WARNING on drm-intel/for-linux-next drm-tip/drm-tip v5.17 next-20220325]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Maxime-Ripard/drm-atomic-Atomic-Private-State-debugging/20220325-205019
base:   git://anongit.freedesktop.org/drm/drm drm-next
config: ia64-defconfig (https://download.01.org/0day-ci/archive/20220326/202203260001.Gfx0MJJ4-lkp@intel.com/config)
compiler: ia64-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/dc4288c88376cba127b0280246b77566b18d9f1d
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Maxime-Ripard/drm-atomic-Atomic-Private-State-debugging/20220325-205019
        git checkout dc4288c88376cba127b0280246b77566b18d9f1d
        # save the config file to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=ia64 SHELL=/bin/bash drivers/gpu/drm/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/drm_atomic.c: In function 'drm_atomic_print_new_state':
>> drivers/gpu/drm/drm_atomic.c:1670:33: warning: variable 'obj' set but not used [-Wunused-but-set-variable]
    1670 |         struct drm_private_obj *obj;
         |                                 ^~~


vim +/obj +1670 drivers/gpu/drm/drm_atomic.c

  1649	
  1650	/**
  1651	 * drm_atomic_print_new_state - prints drm atomic state
  1652	 * @state: atomic configuration to check
  1653	 * @p: drm printer
  1654	 *
  1655	 * This functions prints the drm atomic state snapshot using the drm printer
  1656	 * which is passed to it. This snapshot can be used for debugging purposes.
  1657	 *
  1658	 * Note that this function looks into the new state objects and hence its not
  1659	 * safe to be used after the call to drm_atomic_helper_commit_hw_done().
  1660	 */
  1661	void drm_atomic_print_new_state(const struct drm_atomic_state *state,
  1662			struct drm_printer *p)
  1663	{
  1664		struct drm_plane *plane;
  1665		struct drm_plane_state *plane_state;
  1666		struct drm_crtc *crtc;
  1667		struct drm_crtc_state *crtc_state;
  1668		struct drm_connector *connector;
  1669		struct drm_connector_state *connector_state;
> 1670		struct drm_private_obj *obj;
  1671		struct drm_private_state *obj_state;
  1672		int i;
  1673	
  1674		if (!p) {
  1675			drm_err(state->dev, "invalid drm printer\n");
  1676			return;
  1677		}
  1678	
  1679		drm_dbg_atomic(state->dev, "checking %p\n", state);
  1680	
  1681		for_each_new_plane_in_state(state, plane, plane_state, i)
  1682			drm_atomic_plane_print_state(p, plane_state);
  1683	
  1684		for_each_new_crtc_in_state(state, crtc, crtc_state, i)
  1685			drm_atomic_crtc_print_state(p, crtc_state);
  1686	
  1687		for_each_new_connector_in_state(state, connector, connector_state, i)
  1688			drm_atomic_connector_print_state(p, connector_state);
  1689	
  1690		for_each_new_private_obj_in_state(state, obj, obj_state, i)
  1691			drm_atomic_private_obj_print_state(p, obj_state);
  1692	}
  1693	EXPORT_SYMBOL(drm_atomic_print_new_state);
  1694	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

[PATCH v2 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 v2 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