[Intel-gfx] [PATCH 09/32] drm/i915: Prevent leaking of -EIO from i915_wait_request()

[Daniel Vetter]

On Wed, Dec 16, 2015 at 11:06:20AM +0000, Chris Wilson wrote:
> On Wed, Dec 16, 2015 at 10:52:06AM +0100, Daniel Vetter wrote:
> > On Fri, Dec 11, 2015 at 11:33:05AM +0000, Chris Wilson wrote:
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index d7bbd015de35..875bdf814d73 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -13405,10 +13405,6 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
> > >  
> > >  			ret = __i915_wait_request(intel_plane_state->wait_req,
> > >  						  true, NULL, NULL);
> > > -
> > > -			/* Swallow -EIO errors to allow updates during hw lockup. */
> > > -			if (ret == -EIO)
> > 
> > I'd like to keep a WARN_ON here since it's ABI relevant, and consistent
> > with the other places we've had a WARN_ON(-EIO).
> > 
> > > -				ret = 0;
> > >  			if (ret) {
> > >  				mutex_lock(&dev->struct_mutex);
> > >  				drm_atomic_helper_cleanup_planes(dev, state);
> > > @@ -13744,9 +13740,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> > >  		 */
> > >  		if (needs_modeset(crtc_state))
> > >  			ret = i915_gem_object_wait_rendering(old_obj, true);
> > > -
> > > -		/* Swallow -EIO errors to allow updates during hw lockup. */
> > > -		if (ret && ret != -EIO)
> > > +		if (ret)
> > 
> > Same here really.
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 875bdf814d73..1586d9107cba 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13406,6 +13406,8 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
>                         ret = __i915_wait_request(intel_plane_state->wait_req,
>                                                   true, NULL, NULL);
>                         if (ret) {
> +                               /* Any hang should be swallowed by the wait */
> +                               WARN_ON(ret == -EIO);
>                                 mutex_lock(&dev->struct_mutex);
>                                 drm_atomic_helper_cleanup_planes(dev, state);
>                                 mutex_unlock(&dev->struct_mutex);
> @@ -13740,8 +13742,11 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>                  */
>                 if (needs_modeset(crtc_state))
>                         ret = i915_gem_object_wait_rendering(old_obj, true);
> -               if (ret)
> +               if (ret) {
> +                       /* GPU hangs should have been swallowed by the wait */
> +                       WARN_ON(ret == -EIO);
>                         return ret;
> +               }
>         }
>  
>         /* For framebuffer backed by dmabuf, wait for fence */

lgtm.

> > We already have a WARN_ON in the mmio_flip, so are covered there already.
> > 
> > Otherwise looks good, and with the above addressed and under the
> > assumption that the last caller of check_wedge is in request_alloc (which
> > I was too lazy to check perfectly):
> 
> Yes, later it is moved to the new i915_gem_request.c and made static
> alongside it's only caller request_alloc().

Yeah, r-b confirmed ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch