On Fri, Nov 24, 2017 at 06:13:56PM +0200, Ville Syrjälä wrote:
> On Wed, Nov 15, 2017 at 08:38:41PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrj...@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?
Yeah a DRM_DEBUG_ATOMIC_CHECK which does this sounds like a very good
idea. I even thought of encapsulating the return -EINVAL, but that's maybe
a bit too nasty control flow :-) But maybe something like
#define atomic_check_failure_return(reason, ..)
wouldn't be too eye-gauging ...
-Daniel
>
> >
> > v2: Rebase and handle a few new cases
> >
> > Cc: Jani Nikula <jani.nik...@intel.com>
> > Reviewed-by: Jani Nikula <jani.nik...@intel.com> #v1
> > Signed-off-by: Ville Syrjälä <ville.syrj...@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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
