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

[Daniel Vetter <daniel@ffwll.ch>]

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);
> +	}
>  
>  	ppgtt->base.cleanup(&ppgtt->base);
>  }
>  
> -
> -/* 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_stolen.c */
>  int i915_gem_init_stolen(struct drm_device *dev);
>  int i915_gem_stolen_setup_compression(struct drm_device *dev, int size);
> -- 
> 1.8.4.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch