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(-)
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); }
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
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 {
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-Pri... base: git://anongit.freedesktop.org/drm/drm drm-next config: ia64-defconfig (https://download.01.org/0day-ci/archive/20220326/202203260001.Gfx0MJJ4-lkp@i...) 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/dc4288c88376cba127b0280246b77566b18d... 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
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); }
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)
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
kernel test robot -
Maxime Ripard