Re: [PATCH 08/12] drm/i915: Update context_fini

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Ben Widawsky <ben@bwidawsk.net> writes:

> Make resets optional for fini. fini is only ever called in
> module_unload. It was therefore a good assumption that the GPU reset
> (see the comment in the function) was what we wanted. With an upcoming
> patch, we're going to add a few more callsites, one of which is GPU
> reset, where doing the extra reset isn't usual.
>
> On that same note, with more callers it makes sense to make the default
> context state consistent with the actual state (via NULLing the
> pointer).
>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_dma.c         | 2 +-
>  drivers/gpu/drm/i915/i915_drv.h         | 2 +-
>  drivers/gpu/drm/i915/i915_gem_context.c | 7 +++++--
>  3 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 3b315ba..a141b8a 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1764,7 +1764,7 @@ int i915_driver_unload(struct drm_device *dev)
>  		mutex_lock(&dev->struct_mutex);
>  		i915_gem_free_all_phys_object(dev);
>  		i915_gem_cleanup_ringbuffer(dev);
> -		i915_gem_context_fini(dev);
> +		i915_gem_context_fini(dev, true);
>  		mutex_unlock(&dev->struct_mutex);
>  		i915_gem_cleanup_aliasing_ppgtt(dev);
>  		i915_gem_cleanup_stolen(dev);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9717c69..618845e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1698,7 +1698,7 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev,
>  
>  /* i915_gem_context.c */
>  void i915_gem_context_init(struct drm_device *dev);
> -void i915_gem_context_fini(struct drm_device *dev);
> +void i915_gem_context_fini(struct drm_device *dev, bool reset);
>  void i915_gem_context_close(struct drm_device *dev, struct drm_file *file);
>  int i915_switch_context(struct intel_ring_buffer *ring,
>  			struct drm_file *file, int to_id);
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 411ace0..6030d83 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -259,7 +259,7 @@ void i915_gem_context_init(struct drm_device *dev)
>  	DRM_DEBUG_DRIVER("HW context support initialized\n");
>  }
>  
> -void i915_gem_context_fini(struct drm_device *dev)
> +void i915_gem_context_fini(struct drm_device *dev, bool reset)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> @@ -269,11 +269,14 @@ void i915_gem_context_fini(struct drm_device *dev)
>  	/* The only known way to stop the gpu from accessing the hw context is
>  	 * to reset it. Do this as the very last operation to avoid confusing
>  	 * other code, leading to spurious errors. */

Should we switch to default context in here to be sure that 
the running context will get unreferenced correctly?...

> -	intel_gpu_reset(dev);
> +	if (reset)
> +		intel_gpu_reset(dev);
>  
>  	i915_gem_object_unpin(dev_priv->ring[RCS].default_context->obj);
>

...and unreference the default context here so that it will get freed
on module unload.

--Mika

>  	do_destroy(dev_priv->ring[RCS].default_context);
> +
> +	dev_priv->ring[RCS].default_context = NULL;
>  }
>  
>  static int context_idr_cleanup(int id, void *p, void *data)
> -- 
> 1.8.2.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx