[PATCH] drm: Squelch DEBUG_ATOMIC message for pointer manipulations

Daniel Vetter daniel at ffwll.ch
Mon Aug 21 16:33:38 UTC 2017


On Mon, Aug 21, 2017 at 10:36:48AM +0200, Maarten Lankhorst wrote:
> 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 at chris-wilson.co.uk>
> > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> I'm all for silencing, too much noise there. :)
> 
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> 
> With the note that you should see what daniel thinks as well.

Ack, feel free to push.

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

Like the call to drm_atomic_print_state() that we already have if you
enable state debugging?
-Daniel

> 
> > ---
> >  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);
> 
> 

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


More information about the dri-devel mailing list