Re: [PATCH v2] drm/atomic: Use DRM_DEBUG_KMS instead of DRM_DEBUG_ATOMIC in error paths

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Nov 15, 2017 at 08:38:41PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> 
> DRM_DEBUG_ATOMIC generates a lot of noise that no one normally cares
> about. However error paths everyone cares about, so hiding thea error
> debugs under DRM_DEBUG_ATOMIC is a bad idea. Let's use DRM_DEBUG_KMS
> for those instead.

Actually, one idea that came to my mind right now is that maybe we want
to keep using _ATOMIC with TEST_ONLY, and only use _KMS w/o TEST_ONLY?

> 
> v2: Rebase and handle a few new cases
> 
> Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxx> #v1
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/drm_atomic.c        | 64 ++++++++++++++++-----------------
>  drivers/gpu/drm/drm_atomic_helper.c | 70 ++++++++++++++++++-------------------
>  2 files changed, 66 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 37445d50816a..594bdd5c33cb 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -572,8 +572,8 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc,
>  	 */
>  
>  	if (state->active && !state->enable) {
> -		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] active without enabled\n",
> -				 crtc->base.id, crtc->name);
> +		DRM_DEBUG_KMS("[CRTC:%d:%s] active without enabled\n",
> +			      crtc->base.id, crtc->name);
>  		return -EINVAL;
>  	}
>  
> @@ -582,15 +582,15 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc,
>  	 * be able to trigger. */
>  	if (drm_core_check_feature(crtc->dev, DRIVER_ATOMIC) &&
>  	    WARN_ON(state->enable && !state->mode_blob)) {
> -		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] enabled without mode blob\n",
> -				 crtc->base.id, crtc->name);
> +		DRM_DEBUG_KMS("[CRTC:%d:%s] enabled without mode blob\n",
> +			      crtc->base.id, crtc->name);
>  		return -EINVAL;
>  	}
>  
>  	if (drm_core_check_feature(crtc->dev, DRIVER_ATOMIC) &&
>  	    WARN_ON(!state->enable && state->mode_blob)) {
> -		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] disabled with mode blob\n",
> -				 crtc->base.id, crtc->name);
> +		DRM_DEBUG_KMS("[CRTC:%d:%s] disabled with mode blob\n",
> +			      crtc->base.id, crtc->name);
>  		return -EINVAL;
>  	}
>  
> @@ -605,8 +605,8 @@ static int drm_atomic_crtc_check(struct drm_crtc *crtc,
>  	 * pipe.
>  	 */
>  	if (state->event && !state->active && !crtc->state->active) {
> -		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] requesting event but off\n",
> -				 crtc->base.id, crtc->name);
> +		DRM_DEBUG_KMS("[CRTC:%d:%s] requesting event but off\n",
> +			      crtc->base.id, crtc->name);
>  		return -EINVAL;
>  	}
>  
> @@ -861,10 +861,10 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
>  
>  	/* either *both* CRTC and FB must be set, or neither */
>  	if (WARN_ON(state->crtc && !state->fb)) {
> -		DRM_DEBUG_ATOMIC("CRTC set but no FB\n");
> +		DRM_DEBUG_KMS("CRTC set but no FB\n");
>  		return -EINVAL;
>  	} else if (WARN_ON(state->fb && !state->crtc)) {
> -		DRM_DEBUG_ATOMIC("FB set but no CRTC\n");
> +		DRM_DEBUG_KMS("FB set but no CRTC\n");
>  		return -EINVAL;
>  	}
>  
> @@ -874,7 +874,7 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
>  
>  	/* Check whether this plane is usable on this CRTC */
>  	if (!(plane->possible_crtcs & drm_crtc_mask(state->crtc))) {
> -		DRM_DEBUG_ATOMIC("Invalid crtc for plane\n");
> +		DRM_DEBUG_KMS("Invalid crtc for plane\n");
>  		return -EINVAL;
>  	}
>  
> @@ -882,9 +882,9 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
>  	ret = drm_plane_check_pixel_format(plane, state->fb->format->format);
>  	if (ret) {
>  		struct drm_format_name_buf format_name;
> -		DRM_DEBUG_ATOMIC("Invalid pixel format %s\n",
> -		                 drm_get_format_name(state->fb->format->format,
> -		                                     &format_name));
> +		DRM_DEBUG_KMS("Invalid pixel format %s\n",
> +			      drm_get_format_name(state->fb->format->format,
> +						  &format_name));
>  		return ret;
>  	}
>  
> @@ -893,9 +893,9 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
>  	    state->crtc_x > INT_MAX - (int32_t) state->crtc_w ||
>  	    state->crtc_h > INT_MAX ||
>  	    state->crtc_y > INT_MAX - (int32_t) state->crtc_h) {
> -		DRM_DEBUG_ATOMIC("Invalid CRTC coordinates %ux%u+%d+%d\n",
> -				 state->crtc_w, state->crtc_h,
> -				 state->crtc_x, state->crtc_y);
> +		DRM_DEBUG_KMS("Invalid CRTC coordinates %ux%u+%d+%d\n",
> +			      state->crtc_w, state->crtc_h,
> +			      state->crtc_x, state->crtc_y);
>  		return -ERANGE;
>  	}
>  
> @@ -907,19 +907,19 @@ static int drm_atomic_plane_check(struct drm_plane *plane,
>  	    state->src_x > fb_width - state->src_w ||
>  	    state->src_h > fb_height ||
>  	    state->src_y > fb_height - state->src_h) {
> -		DRM_DEBUG_ATOMIC("Invalid source coordinates "
> -				 "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n",
> -				 state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,
> -				 state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10,
> -				 state->src_x >> 16, ((state->src_x & 0xffff) * 15625) >> 10,
> -				 state->src_y >> 16, ((state->src_y & 0xffff) * 15625) >> 10,
> -				 state->fb->width, state->fb->height);
> +		DRM_DEBUG_KMS("Invalid source coordinates "
> +			      "%u.%06ux%u.%06u+%u.%06u+%u.%06u (fb %ux%u)\n",
> +			      state->src_w >> 16, ((state->src_w & 0xffff) * 15625) >> 10,
> +			      state->src_h >> 16, ((state->src_h & 0xffff) * 15625) >> 10,
> +			      state->src_x >> 16, ((state->src_x & 0xffff) * 15625) >> 10,
> +			      state->src_y >> 16, ((state->src_y & 0xffff) * 15625) >> 10,
> +			      state->fb->width, state->fb->height);
>  		return -ENOSPC;
>  	}
>  
>  	if (plane_switching_crtc(state->state, plane, state)) {
> -		DRM_DEBUG_ATOMIC("[PLANE:%d:%s] switching CRTC directly\n",
> -				 plane->base.id, plane->name);
> +		DRM_DEBUG_KMS("[PLANE:%d:%s] switching CRTC directly\n",
> +			      plane->base.id, plane->name);
>  		return -EINVAL;
>  	}
>  
> @@ -1596,8 +1596,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>  	for_each_new_plane_in_state(state, plane, plane_state, i) {
>  		ret = drm_atomic_plane_check(plane, plane_state);
>  		if (ret) {
> -			DRM_DEBUG_ATOMIC("[PLANE:%d:%s] atomic core check failed\n",
> -					 plane->base.id, plane->name);
> +			DRM_DEBUG_KMS("[PLANE:%d:%s] atomic core check failed\n",
> +				      plane->base.id, plane->name);
>  			return ret;
>  		}
>  	}
> @@ -1605,8 +1605,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>  	for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>  		ret = drm_atomic_crtc_check(crtc, crtc_state);
>  		if (ret) {
> -			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] atomic core check failed\n",
> -					 crtc->base.id, crtc->name);
> +			DRM_DEBUG_KMS("[CRTC:%d:%s] atomic core check failed\n",
> +				      crtc->base.id, crtc->name);
>  			return ret;
>  		}
>  	}
> @@ -1620,8 +1620,8 @@ int drm_atomic_check_only(struct drm_atomic_state *state)
>  	if (!state->allow_modeset) {
>  		for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>  			if (drm_atomic_crtc_needs_modeset(crtc_state)) {
> -				DRM_DEBUG_ATOMIC("[CRTC:%d:%s] requires full modeset\n",
> -						 crtc->base.id, crtc->name);
> +				DRM_DEBUG_KMS("[CRTC:%d:%s] requires full modeset\n",
> +					      crtc->base.id, crtc->name);
>  				return -EINVAL;
>  			}
>  		}
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index ced1ac8e68a0..8625b181f6b6 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -121,9 +121,9 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
>  
>  		if (new_encoder) {
>  			if (encoder_mask & (1 << drm_encoder_index(new_encoder))) {
> -				DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] on [CONNECTOR:%d:%s] already assigned\n",
> -					new_encoder->base.id, new_encoder->name,
> -					connector->base.id, connector->name);
> +				DRM_DEBUG_KMS("[ENCODER:%d:%s] on [CONNECTOR:%d:%s] already assigned\n",
> +					      new_encoder->base.id, new_encoder->name,
> +					      connector->base.id, connector->name);
>  
>  				return -EINVAL;
>  			}
> @@ -158,11 +158,11 @@ static int handle_conflicting_encoders(struct drm_atomic_state *state,
>  			continue;
>  
>  		if (!disable_conflicting_encoders) {
> -			DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] in use on [CRTC:%d:%s] by [CONNECTOR:%d:%s]\n",
> -					 encoder->base.id, encoder->name,
> -					 connector->state->crtc->base.id,
> -					 connector->state->crtc->name,
> -					 connector->base.id, connector->name);
> +			DRM_DEBUG_KMS("[ENCODER:%d:%s] in use on [CRTC:%d:%s] by [CONNECTOR:%d:%s]\n",
> +				      encoder->base.id, encoder->name,
> +				      connector->state->crtc->base.id,
> +				      connector->state->crtc->name,
> +				      connector->base.id, connector->name);
>  			ret = -EINVAL;
>  			goto out;
>  		}
> @@ -317,18 +317,16 @@ update_connector_routing(struct drm_atomic_state *state,
>  		new_encoder = drm_atomic_helper_best_encoder(connector);
>  
>  	if (!new_encoder) {
> -		DRM_DEBUG_ATOMIC("No suitable encoder found for [CONNECTOR:%d:%s]\n",
> -				 connector->base.id,
> -				 connector->name);
> +		DRM_DEBUG_KMS("No suitable encoder found for [CONNECTOR:%d:%s]\n",
> +			      connector->base.id, connector->name);
>  		return -EINVAL;
>  	}
>  
>  	if (!drm_encoder_crtc_ok(new_encoder, new_connector_state->crtc)) {
> -		DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] incompatible with [CRTC:%d:%s]\n",
> -				 new_encoder->base.id,
> -				 new_encoder->name,
> -				 new_connector_state->crtc->base.id,
> -				 new_connector_state->crtc->name);
> +		DRM_DEBUG_KMS("[ENCODER:%d:%s] incompatible with [CRTC:%d:%s]\n",
> +			      new_encoder->base.id, new_encoder->name,
> +			      new_connector_state->crtc->base.id,
> +			      new_connector_state->crtc->name);
>  		return -EINVAL;
>  	}
>  
> @@ -404,7 +402,7 @@ mode_fixup(struct drm_atomic_state *state)
>  		ret = drm_bridge_mode_fixup(encoder->bridge, &new_crtc_state->mode,
>  				&new_crtc_state->adjusted_mode);
>  		if (!ret) {
> -			DRM_DEBUG_ATOMIC("Bridge fixup failed\n");
> +			DRM_DEBUG_KMS("Bridge fixup failed\n");
>  			return -EINVAL;
>  		}
>  
> @@ -412,16 +410,16 @@ mode_fixup(struct drm_atomic_state *state)
>  			ret = funcs->atomic_check(encoder, new_crtc_state,
>  						  new_conn_state);
>  			if (ret) {
> -				DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] check failed\n",
> -						 encoder->base.id, encoder->name);
> +				DRM_DEBUG_KMS("[ENCODER:%d:%s] check failed\n",
> +					      encoder->base.id, encoder->name);
>  				return ret;
>  			}
>  		} else if (funcs && funcs->mode_fixup) {
>  			ret = funcs->mode_fixup(encoder, &new_crtc_state->mode,
>  						&new_crtc_state->adjusted_mode);
>  			if (!ret) {
> -				DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] fixup failed\n",
> -						 encoder->base.id, encoder->name);
> +				DRM_DEBUG_KMS("[ENCODER:%d:%s] fixup failed\n",
> +					      encoder->base.id, encoder->name);
>  				return -EINVAL;
>  			}
>  		}
> @@ -444,8 +442,8 @@ mode_fixup(struct drm_atomic_state *state)
>  		ret = funcs->mode_fixup(crtc, &new_crtc_state->mode,
>  					&new_crtc_state->adjusted_mode);
>  		if (!ret) {
> -			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] fixup failed\n",
> -					 crtc->base.id, crtc->name);
> +			DRM_DEBUG_KMS("[CRTC:%d:%s] fixup failed\n",
> +				      crtc->base.id, crtc->name);
>  			return -EINVAL;
>  		}
>  	}
> @@ -462,21 +460,21 @@ static enum drm_mode_status mode_valid_path(struct drm_connector *connector,
>  
>  	ret = drm_encoder_mode_valid(encoder, mode);
>  	if (ret != MODE_OK) {
> -		DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] mode_valid() failed\n",
> -				encoder->base.id, encoder->name);
> +		DRM_DEBUG_KMS("[ENCODER:%d:%s] mode_valid() failed\n",
> +			      encoder->base.id, encoder->name);
>  		return ret;
>  	}
>  
>  	ret = drm_bridge_mode_valid(encoder->bridge, mode);
>  	if (ret != MODE_OK) {
> -		DRM_DEBUG_ATOMIC("[BRIDGE] mode_valid() failed\n");
> +		DRM_DEBUG_KMS("[BRIDGE] mode_valid() failed\n");
>  		return ret;
>  	}
>  
>  	ret = drm_crtc_mode_valid(crtc, mode);
>  	if (ret != MODE_OK) {
> -		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] mode_valid() failed\n",
> -				crtc->base.id, crtc->name);
> +		DRM_DEBUG_KMS("[CRTC:%d:%s] mode_valid() failed\n",
> +			      crtc->base.id, crtc->name);
>  		return ret;
>  	}
>  
> @@ -605,8 +603,8 @@ drm_atomic_helper_check_modeset(struct drm_device *dev,
>  		}
>  
>  		if (new_crtc_state->enable != has_connectors) {
> -			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] enabled/connectors mismatch\n",
> -					 crtc->base.id, crtc->name);
> +			DRM_DEBUG_KMS("[CRTC:%d:%s] enabled/connectors mismatch\n",
> +				      crtc->base.id, crtc->name);
>  
>  			return -EINVAL;
>  		}
> @@ -735,8 +733,8 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
>  
>  		ret = funcs->atomic_check(plane, new_plane_state);
>  		if (ret) {
> -			DRM_DEBUG_ATOMIC("[PLANE:%d:%s] atomic driver check failed\n",
> -					 plane->base.id, plane->name);
> +			DRM_DEBUG_KMS("[PLANE:%d:%s] atomic driver check failed\n",
> +				      plane->base.id, plane->name);
>  			return ret;
>  		}
>  	}
> @@ -751,8 +749,8 @@ drm_atomic_helper_check_planes(struct drm_device *dev,
>  
>  		ret = funcs->atomic_check(crtc, new_crtc_state);
>  		if (ret) {
> -			DRM_DEBUG_ATOMIC("[CRTC:%d:%s] atomic driver check failed\n",
> -					 crtc->base.id, crtc->name);
> +			DRM_DEBUG_KMS("[CRTC:%d:%s] atomic driver check failed\n",
> +				      crtc->base.id, crtc->name);
>  			return ret;
>  		}
>  	}
> @@ -3098,8 +3096,8 @@ static int page_flip_common(struct drm_atomic_state *state,
>  	/* Make sure we don't accidentally do a full modeset. */
>  	state->allow_modeset = false;
>  	if (!crtc_state->active) {
> -		DRM_DEBUG_ATOMIC("[CRTC:%d:%s] disabled, rejecting legacy flip\n",
> -				 crtc->base.id, crtc->name);
> +		DRM_DEBUG_KMS("[CRTC:%d:%s] disabled, rejecting legacy flip\n",
> +			      crtc->base.id, crtc->name);
>  		return -EINVAL;
>  	}
>  
> -- 
> 2.13.6

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux