Re: [PATCH] drm: Squelch DEBUG_ATOMIC message for pointer manipulations

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 21-08-17 om 10:31 schreef Chris Wilson:
> These messages flood the debug logs conveying very little information
> except that the same actions as performed on the last atomic modeset are
> being repeated on a different pointer. That our identifier is a pointer
> is an indicator that is not interesting enough to be tracked by a human ;)
> An alternative would be to leave these messages compiled and give the
> interesting failure/success messages a new identifier like KMS (since
> they are confirmation messages similar in nature to the existing KMS
> messages), or move these low-level ATOMIC operations to a new id that we
> can always ignore.
>
> A debug message that summarized the action about to be taken would be
> useful. As it stands the signal:noise of a drm.debug=0x1e dmesg is
> verging on the useless. We need to curb the overuse of DRM_DEBUG_ATOMIC
> or stop including it in the recommended debug flags.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
I'm all for silencing, too much noise there. :)

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

With the note that you should see what daniel thinks as well.

We should probably add something to dump the state at some point just before atomic commit, but I'm fine with this.

> ---
>  drivers/gpu/drm/drm_atomic.c | 76 ++++++++++++++++++++++----------------------
>  1 file changed, 38 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 2fd383d7253a..b9796a77dd7d 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -34,6 +34,12 @@
>  
>  #include "drm_crtc_internal.h"
>  
> +#if 0
> +#define DBG(...) DRM_DEBUG_ATOMIC(__VA_ARGS__)
> +#else
> +#define DBG(...)
> +#endif
> +
>  void __drm_crtc_commit_free(struct kref *kref)
>  {
>  	struct drm_crtc_commit *commit =
> @@ -89,7 +95,7 @@ drm_atomic_state_init(struct drm_device *dev, struct drm_atomic_state *state)
>  
>  	state->dev = dev;
>  
> -	DRM_DEBUG_ATOMIC("Allocated atomic state %p\n", state);
> +	DBG("Allocated atomic state %p\n", state);
>  
>  	return 0;
>  fail:
> @@ -139,7 +145,7 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)
>  	struct drm_mode_config *config = &dev->mode_config;
>  	int i;
>  
> -	DRM_DEBUG_ATOMIC("Clearing atomic state %p\n", state);
> +	DBG("Clearing atomic state %p\n", state);
>  
>  	for (i = 0; i < state->num_connector; i++) {
>  		struct drm_connector *connector = state->connectors[i].ptr;
> @@ -242,7 +248,7 @@ void __drm_atomic_state_free(struct kref *ref)
>  
>  	drm_atomic_state_clear(state);
>  
> -	DRM_DEBUG_ATOMIC("Freeing atomic state %p\n", state);
> +	DBG("Freeing atomic state %p\n", state);
>  
>  	if (config->funcs->atomic_state_free) {
>  		config->funcs->atomic_state_free(state);
> @@ -295,8 +301,8 @@ drm_atomic_get_crtc_state(struct drm_atomic_state *state,
>  	state->crtcs[index].ptr = crtc;
>  	crtc_state->state = state;
>  
> -	DRM_DEBUG_ATOMIC("Added [CRTC:%d:%s] %p state to %p\n",
> -			 crtc->base.id, crtc->name, crtc_state, state);
> +	DBG("Added [CRTC:%d:%s] %p state to %p\n",
> +	    crtc->base.id, crtc->name, crtc_state, state);
>  
>  	return crtc_state;
>  }
> @@ -353,13 +359,11 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
>  
>  		drm_mode_copy(&state->mode, mode);
>  		state->enable = true;
> -		DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",
> -				 mode->name, state);
> +		DBG("Set [MODE:%s] for CRTC state %p\n", mode->name, state);
>  	} else {
>  		memset(&state->mode, 0, sizeof(state->mode));
>  		state->enable = false;
> -		DRM_DEBUG_ATOMIC("Set [NOMODE] for CRTC state %p\n",
> -				 state);
> +		DBG("Set [NOMODE] for CRTC state %p\n", state);
>  	}
>  
>  	return 0;
> @@ -399,12 +403,11 @@ int drm_atomic_set_mode_prop_for_crtc(struct drm_crtc_state *state,
>  
>  		state->mode_blob = drm_property_blob_get(blob);
>  		state->enable = true;
> -		DRM_DEBUG_ATOMIC("Set [MODE:%s] for CRTC state %p\n",
> -				 state->mode.name, state);
> +		DBG("Set [MODE:%s] for CRTC state %p\n",
> +		    state->mode.name, state);
>  	} else {
>  		state->enable = false;
> -		DRM_DEBUG_ATOMIC("Set [NOMODE] for CRTC state %p\n",
> -				 state);
> +		DBG("Set [NOMODE] for CRTC state %p\n", state);
>  	}
>  
>  	return 0;
> @@ -682,8 +685,8 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state,
>  	state->planes[index].new_state = plane_state;
>  	plane_state->state = state;
>  
> -	DRM_DEBUG_ATOMIC("Added [PLANE:%d:%s] %p state to %p\n",
> -			 plane->base.id, plane->name, plane_state, state);
> +	DBG("Added [PLANE:%d:%s] %p state to %p\n",
> +	    plane->base.id, plane->name, plane_state, state);
>  
>  	if (plane_state->crtc) {
>  		struct drm_crtc_state *crtc_state;
> @@ -1045,8 +1048,8 @@ drm_atomic_get_private_obj_state(struct drm_atomic_state *state,
>  
>  	state->num_private_objs = num_objs;
>  
> -	DRM_DEBUG_ATOMIC("Added new private object %p state %p to %p\n",
> -			 obj, obj_state, state);
> +	DBG("Added new private object %p state %p to %p\n",
> +	    obj, obj_state, state);
>  
>  	return obj_state;
>  }
> @@ -1112,9 +1115,9 @@ drm_atomic_get_connector_state(struct drm_atomic_state *state,
>  	state->connectors[index].ptr = connector;
>  	connector_state->state = state;
>  
> -	DRM_DEBUG_ATOMIC("Added [CONNECTOR:%d:%s] %p state to %p\n",
> -			 connector->base.id, connector->name,
> -			 connector_state, state);
> +	DBG("Added [CONNECTOR:%d:%s] %p state to %p\n",
> +	    connector->base.id, connector->name,
> +	    connector_state, state);
>  
>  	if (connector_state->crtc) {
>  		struct drm_crtc_state *crtc_state;
> @@ -1368,11 +1371,10 @@ drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
>  	}
>  
>  	if (crtc)
> -		DRM_DEBUG_ATOMIC("Link plane state %p to [CRTC:%d:%s]\n",
> -				 plane_state, crtc->base.id, crtc->name);
> +		DBG("Link plane state %p to [CRTC:%d:%s]\n",
> +		    plane_state, crtc->base.id, crtc->name);
>  	else
> -		DRM_DEBUG_ATOMIC("Link plane state %p to [NOCRTC]\n",
> -				 plane_state);
> +		DBG("Link plane state %p to [NOCRTC]\n", plane_state);
>  
>  	return 0;
>  }
> @@ -1393,11 +1395,10 @@ drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
>  			    struct drm_framebuffer *fb)
>  {
>  	if (fb)
> -		DRM_DEBUG_ATOMIC("Set [FB:%d] for plane state %p\n",
> -				 fb->base.id, plane_state);
> +		DBG("Set [FB:%d] for plane state %p\n",
> +		    fb->base.id, plane_state);
>  	else
> -		DRM_DEBUG_ATOMIC("Set [NOFB] for plane state %p\n",
> -				 plane_state);
> +		DBG("Set [NOFB] for plane state %p\n", plane_state);
>  
>  	drm_framebuffer_assign(&plane_state->fb, fb);
>  }
> @@ -1477,11 +1478,10 @@ drm_atomic_set_crtc_for_connector(struct drm_connector_state *conn_state,
>  		drm_connector_get(conn_state->connector);
>  		conn_state->crtc = crtc;
>  
> -		DRM_DEBUG_ATOMIC("Link connector state %p to [CRTC:%d:%s]\n",
> -				 conn_state, crtc->base.id, crtc->name);
> +		DBG("Link connector state %p to [CRTC:%d:%s]\n",
> +		    conn_state, crtc->base.id, crtc->name);
>  	} else {
> -		DRM_DEBUG_ATOMIC("Link connector state %p to [NOCRTC]\n",
> -				 conn_state);
> +		DBG("Link connector state %p to [NOCRTC]\n", conn_state);
>  	}
>  
>  	return 0;
> @@ -1524,8 +1524,8 @@ drm_atomic_add_affected_connectors(struct drm_atomic_state *state,
>  	if (ret)
>  		return ret;
>  
> -	DRM_DEBUG_ATOMIC("Adding all current connectors for [CRTC:%d:%s] to %p\n",
> -			 crtc->base.id, crtc->name, state);
> +	DBG("Adding all current connectors for [CRTC:%d:%s] to %p\n",
> +	    crtc->base.id, crtc->name, state);
>  
>  	/*
>  	 * Changed connectors are already in @state, so only need to look
> @@ -1608,7 +1608,7 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>  	struct drm_crtc_state *crtc_state;
>  	int i, ret = 0;
>  
> -	DRM_DEBUG_ATOMIC("checking %p\n", state);
> +	DBG("checking %p\n", state);
>  
>  	for_each_new_plane_in_state(state, plane, plane_state, i) {
>  		ret = drm_atomic_plane_check(plane, plane_state);
> @@ -1671,7 +1671,7 @@ int drm_atomic_commit(struct drm_atomic_state *state)
>  	if (ret)
>  		return ret;
>  
> -	DRM_DEBUG_ATOMIC("committing %p\n", state);
> +	DBG("committing %p\n", state);
>  
>  	return config->funcs->atomic_commit(state->dev, state, false);
>  }
> @@ -1700,7 +1700,7 @@ int drm_atomic_nonblocking_commit(struct drm_atomic_state *state)
>  	if (ret)
>  		return ret;
>  
> -	DRM_DEBUG_ATOMIC("committing %p nonblocking\n", state);
> +	DBG("committing %p nonblocking\n", state);
>  
>  	return config->funcs->atomic_commit(state->dev, state, true);
>  }
> @@ -1717,7 +1717,7 @@ static void drm_atomic_print_state(const struct drm_atomic_state *state)
>  	struct drm_connector_state *connector_state;
>  	int i;
>  
> -	DRM_DEBUG_ATOMIC("checking %p\n", state);
> +	DBG("checking %p\n", state);
>  
>  	for_each_new_plane_in_state(state, plane, plane_state, i)
>  		drm_atomic_plane_print_state(&p, plane_state);


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel