Re: [PATCH 1/9] [v2] drm/i915/bdw: Free PPGTT struct

[Imre Deak <imre.deak@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2531 bytes --]

On Thu, 2014-02-20 at 11:47 -0800, Ben Widawsky wrote:
> GEN8 never freed the PPGTT struct. As GEN8 doesn't use full PPGTT, the
> leak is small and only found on a module reload. ie. I don't think this
> needs to go to stable.
> 
> v2: The very naive, kfree in gen8 ppgtt cleanup, is subject to a double
> free on PPGTT initialization failure. (Spotted by Imre). Instead this
> patch pulls the ppgtt struct freeing out of the cleanup and leaves it to
> the allocators/callers or the one doing the last kref_put as in standard
> convention
> 
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 12 ++++++++++--
>  drivers/gpu/drm/i915/i915_gem_gtt.c     |  1 -
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 171a2ef..9096e2a 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -99,9 +99,8 @@
>  static int do_switch(struct intel_ring_buffer *ring,
>  		     struct i915_hw_context *to);
>  
> -static void ppgtt_release(struct kref *kref)
> +static void do_ppgtt_cleanup(struct i915_hw_ppgtt *ppgtt)
>  {
> -	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;
> @@ -135,6 +134,15 @@ static void ppgtt_release(struct kref *kref)
>  	ppgtt->base.cleanup(&ppgtt->base);
>  }
>  
> +static void ppgtt_release(struct kref *kref)
> +{
> +	struct i915_hw_ppgtt *ppgtt =
> +		container_of(kref, struct i915_hw_ppgtt, ref);
> +
> +	do_ppgtt_cleanup(ppgtt);
> +	kfree(ppgtt);
> +}
> +
>  static size_t get_context_alignment(struct drm_device *dev)
>  {
>  	if (IS_GEN6(dev))
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 69a88d4..49e79fb 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -859,7 +859,6 @@ static void gen6_ppgtt_cleanup(struct i915_address_space *vm)
>  	for (i = 0; i < ppgtt->num_pd_entries; i++)
>  		__free_page(ppgtt->pt_pages[i]);
>  	kfree(ppgtt->pt_pages);
> -	kfree(ppgtt);
>  }
>  
>  static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt)


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx