Re: [PATCH v2 2/4] drm/i915: Ironlake mobile GPU with VT-d fix

[Ben Widawsky <ben@bwidawsk.net>]

On Mon, 26 Sep 2011 09:23:17 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Sun, Sep 25, 2011 at 04:42:57PM -0700, Ben Widawsky wrote:
> > > >  static int sandybridge_write_fence_reg(struct drm_i915_gem_object
> > > > *obj, diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > b/drivers/gpu/drm/i915/i915_gem_gtt.c index 7a709cd..0c6226b 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > > > @@ -49,10 +49,39 @@ static unsigned int
> > > > cache_level_to_agp_type(struct drm_device *dev, }
> > > >  }
> > > >  
> > > > +static bool do_idling(struct drm_i915_private *dev_priv)
> > > > +{
> > > > +	bool ret = dev_priv->mm.interruptible;
> > > > +
> > > > +	if (dev_priv->mm.gtt->do_idle_maps) {
> > > > +		dev_priv->mm.interruptible = false;
> > > > +		if (i915_gpu_idle(dev_priv->dev))
> > > > +			DRM_ERROR("couldn't idle GPU %d\n", ret);
> > > > +	}
> > > 
> > > I don't like the uninterruptible sleep in core gem - this is only for
> > > modesetting where backtracking is just not really feasible. I've
> > > quickly checked the code and I haven't seen any problems in handing
> > > back the -ERESTARTSYS to gem_object_unbind.
> > > 
> > 
> > While I agree that we could probably return at idle time, we have to
> > guarantee that no GPU activity occurs until the unmap has completed. I
> > checked the code and don't really understand how it can occur, but I
> > believe it *can* occur as I've seen it happen in the idle function. Any
> > other proposal how to fix this, or any proof that the GPU can't wake up
> > to do display stuff while doing the unmap?
> 
> Hm, looks like some misunderstanding. I understand why the gpu_idle needs
> to be here, my gripes are solely with the uninterruptible sleep that you
> use. I think a normal interruptible sleep with passing back the return
> value to gem_unbind_object should work: By the time we're calling
> gtt_unbind_object in gem_unbind_object we have not yet committed to the
> unbound state, so can just bail out if the mappings are still there. And
> gtt_unbind_object idles the gpu before touching the mappings, so when we
> have to bail out with -ERESTARTSYS the mapping is untouched and everything
> safe.
> -Daniel

Okay. I agree your suggestion works. I've changed the code to do this. I
will not listen to you for a while if someone comes in and just says
make it non interruptible, because it seems to be happening a lot
recently :P

Ben