Re: [PATCH 38/48] drm/i915: Clean up VMAs before freeing

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Dec 18, 2013 at 03:55:05PM +0100, Daniel Vetter wrote:
> On Fri, Dec 06, 2013 at 02:11:23PM -0800, Ben Widawsky wrote:
> > From: Ben Widawsky <ben@bwidawsk.net>
> > 
> > It's quite common for an object to simply be on the inactive list (and
> > not unbound) when we want to free the context. This of course happens
> > with lazy unbinding. Simply, this is needed when an object isn't fully
> > unbound but we want to free one VMA of the object, for whatever reason.
> > 
> > NOTE: The aliasing PPGTT is not a proper VM, so it needs special casing.
> > 
> > This addresses the fixup requirement mentioned in:
> > drm/915: Better reset handling for contexts
> > 
> > In the flink, and dmabuf case, we can't assert that the object isn't
> > still active. To keep it more generic, just check the vma's link in the
> > object vma list. If we wanted to do a better job, we could track last
> > seqno (and active) per VMA. It was decided not to do this in the last
> > iteration. Unfortunately this means the assertion can miss real bugs
> > when using flink/dmabuf.
> > 
> > v2: Use the newer introduced i915_gem_evict_vm(). Note that handling the
> > aliasing PPGTT is special.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> 
> Presuming we have proper refcounting for the stuff involved this just
> papers over bugs. I've added a big FIXME here.
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h | 48 ++++++++++++++++++++++++++++++-----------
> >  1 file changed, 36 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index ff30f3f..fe13023 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2257,6 +2257,17 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> >  int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
> >  				   struct drm_file *file);
> >  
> > +/* i915_gem_evict.c */
> > +int __must_check i915_gem_evict_something(struct drm_device *dev,
> > +					  struct i915_address_space *vm,
> > +					  int min_size,
> > +					  unsigned alignment,
> > +					  unsigned cache_level,
> > +					  bool mappable,
> > +					  bool nonblock);
> > +int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle);
> > +int i915_gem_evict_everything(struct drm_device *dev);
> > +
> >  /* i915_gem_gtt.c */
> >  void i915_check_and_clear_faults(struct drm_device *dev);
> >  void i915_gem_suspend_gtt_mappings(struct drm_device *dev);
> > @@ -2294,22 +2305,35 @@ static inline bool intel_enable_ppgtt(struct drm_device *dev, bool full)
> >  static inline void ppgtt_release(struct kref *kref)
> >  {
> >  	struct i915_hw_ppgtt *ppgtt = container_of(kref, struct i915_hw_ppgtt, ref);
> > +	struct drm_device *dev = ppgtt->base.dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct i915_address_space *vm = &ppgtt->base;
> > +
> > +	if (ppgtt == dev_priv->mm.aliasing_ppgtt ||
> > +	    (list_empty(&vm->active_list) && list_empty(&vm->inactive_list))) {
> > +		ppgtt->base.cleanup(&ppgtt->base);
> > +		return;
> > +	}
> > +
> > +	/* Make sure vmas are unbound before we take down the drm_mm
> > +	 */
> > +	if (!list_empty(&vm->active_list)) {
> > +		struct i915_vma *vma;
> > +
> > +		list_for_each_entry(vma, &vm->active_list, mm_list)
> > +			if (WARN_ON(list_empty(&vma->vma_link) ||
> > +				    list_is_singular(&vma->vma_link)))
> > +				break;
> > +
> > +		i915_gem_evict_vm(&ppgtt->base, true);
> > +	} else {
> > +		i915_gem_retire_requests(dev);
> > +		i915_gem_evict_vm(&ppgtt->base, false);
> > +	}

Also style points for doing this in a static inline helper function.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch