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

List overview All Threads
Download

newer

older

[PATCH 1/6] drm/ttm: move the LRU...

[REPOST PATCH v4 00/13] drm/msm:...

Maxime Ripard

24 Mar 2022 24 Mar '22

2:47 p.m.

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

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 | 24 ++++++++++++++++++++++++ drivers/gpu/drm/drm_atomic_uapi.c | 3 --- drivers/gpu/drm/vc4/vc4_kms.c | 24 +++++++++++++++++++++--- include/drm/drm_atomic.h | 13 +++++++++++++ 4 files changed, 58 insertions(+), 6 deletions(-)

-- 2.35.1

Show replies by date

Maxime Ripard

24 Mar 24 Mar

2:47 p.m.

New subject: [PATCH 1/4] drm/atomic: Print the state every commit

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() and drm_atomic_nonblocking_commit() to make sure we don't miss any.

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

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 88cd992df356..ee2496ff3dcc 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); @@ -1452,6 +1456,7 @@ EXPORT_SYMBOL(drm_atomic_commit); int drm_atomic_nonblocking_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); @@ -1460,6 +1465,9 @@ int drm_atomic_nonblocking_commit(struct drm_atomic_state *state)

drm_dbg_atomic(state->dev, "committing %p nonblocking\n", state);

+ if (drm_debug_enabled(DRM_UT_STATE)) + drm_atomic_print_new_state(state, &p); + return config->funcs->atomic_commit(state->dev, state, true); } EXPORT_SYMBOL(drm_atomic_nonblocking_commit); diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 9781722519c3..e9bb136c7a7c 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -1458,9 +1458,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

5:13 p.m.

New subject: [PATCH 1/4] drm/atomic: Print the state every commit

On Thu, Mar 24, 2022 at 03:47:21PM +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() and drm_atomic_nonblocking_commit() to make sure we don't miss any.

Signed-off-by: Maxime Ripard maxime@cerno.tech

Iirc the idea was that if you get a printing for every nonblocking then the logs get completely flooded. git blame + mailing lists should bring up the people who's ack you need (and maybe cc on v2 of this).

I think we might need a new DRM_UT_NONBLOCKING_STATE or something like that to handle this. -Daniel

...

drivers/gpu/drm/drm_atomic.c | 8 ++++++++ drivers/gpu/drm/drm_atomic_uapi.c | 3 --- 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 88cd992df356..ee2496ff3dcc 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); @@ -1452,6 +1456,7 @@ EXPORT_SYMBOL(drm_atomic_commit); int drm_atomic_nonblocking_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);

@@ -1460,6 +1465,9 @@ int drm_atomic_nonblocking_commit(struct drm_atomic_state *state)

drm_dbg_atomic(state->dev, "committing %p nonblocking\n", state);
if (drm_debug_enabled(DRM_UT_STATE))
drm_atomic_print_new_state(state, &p);
return config->funcs->atomic_commit(state->dev, state, true);
} EXPORT_SYMBOL(drm_atomic_nonblocking_commit); diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 9781722519c3..e9bb136c7a7c 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -1458,9 +1458,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

Maxime Ripard

2:47 p.m.

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

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.

Signed-off-by: Maxime Ripard maxime@cerno.tech --- drivers/gpu/drm/drm_atomic.c | 16 ++++++++++++++++ include/drm/drm_atomic.h | 13 +++++++++++++ 2 files changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index ee2496ff3dcc..b9246206ed54 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);

@@ -1640,6 +1642,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 @@ -1660,6 +1671,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) { @@ -1677,6 +1690,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..7e188cd9452b 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); };

/** @@ -319,6 +331,7 @@ struct drm_private_obj { */ struct drm_private_state { struct drm_atomic_state *state; + struct drm_private_obj *obj; };

struct __drm_private_objs_state {

-- 2.35.1

Daniel Vetter

5:14 p.m.

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

On Thu, Mar 24, 2022 at 03:47:22PM +0100, Maxime Ripard wrote:

...

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.

Signed-off-by: Maxime Ripard maxime@cerno.tech

drivers/gpu/drm/drm_atomic.c | 16 ++++++++++++++++ include/drm/drm_atomic.h | 13 +++++++++++++ 2 files changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index ee2496ff3dcc..b9246206ed54 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);

@@ -1640,6 +1642,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

@@ -1660,6 +1671,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) {

@@ -1677,6 +1690,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..7e188cd9452b 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);
};

/** @@ -319,6 +331,7 @@ struct drm_private_obj { */ struct drm_private_state { struct drm_atomic_state *state;

struct drm_private_obj *obj;

kerneldoc for this is missing. Maybe switch to inline style while at it. With that:

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

...

};

struct __drm_private_objs_state {

2.35.1

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

Maxime Ripard

2:47 p.m.

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

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

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

Maxime Ripard

2:47 p.m.

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

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

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

Daniel Vetter

5:14 p.m.

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

On Thu, Mar 24, 2022 at 03:47:24PM +0100, Maxime Ripard wrote:

...

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

Signed-off-by: Maxime Ripard maxime@cerno.tech

On the two vc4 patches:

Acked-by: Daniel Vetter daniel.vetter@ffwll.ch

...

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

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

1134

Age (days ago)

1134

Last active (days ago)

dri-devel@lists.freedesktop.org

7 comments

2 participants

tags (0)

participants (2)

Daniel Vetter
Maxime Ripard