Re: [PATCH 01/10] drm/i915: remove do_retire from i915_wait_request

[Ben Widawsky <ben@bwidawsk.net>]

On Sat, 21 Apr 2012 19:17:11 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Fri, Apr 20, 2012 at 06:23:23PM -0700, Ben Widawsky wrote:
> > This originates from a hack by me to quickly fix a bug in an earlier
> > patch where we needed control over whether or not waiting on a seqno
> > actually did any retire list processing. Since the two operations
> > aren't clearly related, we should pull the parameter out of the
> > wait function, and make the caller responsible for retiring if the
> > action is desired.
> > 
> > NOTE: this patch has a functional change. I've only made the callers
> > which are requiring the retirement do the retirement. This move was
> > blasted by Keith when I tried it before in a more subtle manner; so
> > I am making it very clear this time around.
> 
> See below for why it's still not a good idea to combine refactoring
> with code changes ;-)

I think in this case it's quite obvious what went wrong if things blow
up, and the patch is so simple that separating it really makes no
difference to anybody when trying to track down a bug.

I agree without the NOTE in the comment this would be a fairly evil
thing to do, which is why Keith got upset before... 

> 
> > Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c            |    2 +-
> >  drivers/gpu/drm/i915/i915_drv.h            |    5 ++---
> >  drivers/gpu/drm/i915/i915_gem.c            |   33
> > +++++++++-------------------
> > drivers/gpu/drm/i915/i915_gem_evict.c      |   14 ++++++++++--
> > drivers/gpu/drm/i915/i915_gem_execbuffer.c |    2 +-
> > drivers/gpu/drm/i915/i915_gem_gtt.c        |    2 +-
> > drivers/gpu/drm/i915/intel_overlay.c       |    6 ++---
> > drivers/gpu/drm/i915/intel_ringbuffer.c    |    4 +++- 8 files
> > changed, 32 insertions(+), 36 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c
> > b/drivers/gpu/drm/i915/i915_dma.c index a813f65..f8fdc5b 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> 
> [cut]
> 
> > @@ -3440,7 +3427,7 @@ i915_gem_idle(struct drm_device *dev)
> >  		return 0;
> >  	}
> >  
> > -	ret = i915_gpu_idle(dev, true);
> > +	ret = i915_gpu_idle(dev);
> >  	if (ret) {
> >  		mutex_unlock(&dev->struct_mutex);
> >  		return ret;
> 
> 
> gem_idle is called by our suspend freeze function and leaking
> unretired seqnos over a s/r cycle was the root cause our -rc2
> regression on gen3. In other words: I'm pretty sure this will blow
> up. I do like the idea of the patch, but:
> 
> Please separate refactoring from actual code changes.

Fine.

Just curious, it blows up on gen3 only? If not, how can I make it blow
up?

> 
> Cheers, Daniel
>