From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: dri-devel@lists.freedesktop.org
Cc: Jani Nikula <jani.nikula@intel.com>
Subject: Re: [PATCH v2] drm/atomic: Use DRM_DEBUG_KMS instead of DRM_DEBUG_ATOMIC in error paths
Date: Fri, 24 Nov 2017 18:13:56 +0200 [thread overview]
Message-ID: <20171124161356.GN10981@intel.com> (raw)
In-Reply-To: <20171115183841.23436-1-ville.syrjala@linux.intel.com>
On Wed, Nov 15, 2017 at 08:38:41PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> 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@intel.com>
> Reviewed-by: Jani Nikula <jani.nikula@intel.com> #v1
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2017-11-24 16:13 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-13 15:13 [PATCH] drm/atomic: Use DRM_DEBUG_KMS instead of DRM_DEBUG_ATOMIC in error paths ville.syrjala
2016-05-16 7:47 ` Jani Nikula
2016-05-16 12:21 ` Emil Velikov
2017-11-15 18:38 ` [PATCH v2] " Ville Syrjala
2017-11-24 16:13 ` Ville Syrjälä [this message]
2017-11-27 10:14 ` Daniel Vetter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171124161356.GN10981@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=jani.nikula@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.