[PATCH] drm/i915: provide more error output when mode sets fail

[PATCH] drm/i915: provide more error output when mode sets fail

[Jesse Barnes]

If a mode set fails we may get a message from drm_crtc_helper if we're lucky,
but it won't tell us anything about *why* we failed to set a mode.  So
add a few DRM_ERRORs for the cases that shouldn't happen so we can debug
things more easily.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_display.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0f1c799..2434441 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1934,7 +1934,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 
 	/* no fb bound */
 	if (!crtc->fb) {
-		DRM_DEBUG_KMS("No FB bound\n");
+		DRM_ERROR("No FB bound\n");
 		return 0;
 	}
 
@@ -1943,6 +1943,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 	case 1:
 		break;
 	default:
+		DRM_ERROR("no plane for crtc\n");
 		return -EINVAL;
 	}
 
@@ -1952,6 +1953,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 					 NULL);
 	if (ret != 0) {
 		mutex_unlock(&dev->struct_mutex);
+		DRM_ERROR("pin & fence failed\n");
 		return ret;
 	}
 
@@ -1980,6 +1982,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 	if (ret) {
 		i915_gem_object_unpin(to_intel_framebuffer(crtc->fb)->obj);
 		mutex_unlock(&dev->struct_mutex);
+		DRM_ERROR("failed to update base address\n");
 		return ret;
 	}
 
-- 
1.7.1

Re: [PATCH] drm/i915: provide more error output when mode sets fail

[Chris Wilson]

On Tue, 19 Jul 2011 15:38:56 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> If a mode set fails we may get a message from drm_crtc_helper if we're lucky,
> but it won't tell us anything about *why* we failed to set a mode.  So
> add a few DRM_ERRORs for the cases that shouldn't happen so we can debug
> things more easily.

Also fix drm not to simply ignore the error codes would also be useful.
But how does logging the error without logging the state help? Presumably
you are targeting the unreproducible cases?

> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0f1c799..2434441 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1934,7 +1934,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>  
>  	/* no fb bound */
>  	if (!crtc->fb) {
> -		DRM_DEBUG_KMS("No FB bound\n");
> +		DRM_ERROR("No FB bound\n");
>  		return 0;
>  	}

Should this be flagged as en error then?

>  
> @@ -1943,6 +1943,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>  	case 1:
>  		break;
>  	default:
> +		DRM_ERROR("no plane for crtc\n");
>  		return -EINVAL;
>  	}
>  
> @@ -1952,6 +1953,7 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
>  					 NULL);
>  	if (ret != 0) {
>  		mutex_unlock(&dev->struct_mutex);
> +		DRM_ERROR("pin & fence failed\n");

In this and the following, report the reason/error code for the failure.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: provide more error output when mode sets fail

[Jesse Barnes]

On Wed, 20 Jul 2011 01:14:47 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Tue, 19 Jul 2011 15:38:56 -0700, Jesse Barnes
> <jbarnes@virtuousgeek.org> wrote:
> > If a mode set fails we may get a message from drm_crtc_helper if
> > we're lucky, but it won't tell us anything about *why* we failed to
> > set a mode.  So add a few DRM_ERRORs for the cases that shouldn't
> > happen so we can debug things more easily.
> 
> Also fix drm not to simply ignore the error codes would also be
> useful. But how does logging the error without logging the state
> help? Presumably you are targeting the unreproducible cases?

I added these to catch a mode setting failure Keith was seeing
following a GPU hang.  The simple debug info was enough to track things
down.  But I agree, really the DRM hooks should return ints and give us
more info about what happened, and try to roll back the config.

> >  	if (!crtc->fb) {
> > -		DRM_DEBUG_KMS("No FB bound\n");
> > +		DRM_ERROR("No FB bound\n");
> >  		return 0;
> >  	}
> 
> Should this be flagged as en error then?

Possibly, unless we shut off displays this way??

> > @@ -1952,6 +1953,7 @@ intel_pipe_set_base(struct drm_crtc *crtc,
> > int x, int y, NULL);
> >  	if (ret != 0) {
> >  		mutex_unlock(&dev->struct_mutex);
> > +		DRM_ERROR("pin & fence failed\n");
> 
> In this and the following, report the reason/error code for the
> failure. -Chris

Sure.

Jesse