Re: [PATCH 1/2] drm/gem: simplify object initialization

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

On Thu, Jul 11, 2013 at 11:56:32AM +0200, David Herrmann wrote:
> drm_gem_object_init() and drm_gem_private_object_init() do exactly the
> same (except for shmem alloc) so make the first use the latter to reduce
> code duplication.
> 
> Also drop the return code from drm_gem_private_object_init(). It seems
> unlikely that we will extend it any time soon so no reason to keep it
> around. This simplifies code paths in drivers, too.
> 
> Last but not least, fix gma500 to call drm_gem_object_release() before
> freeing objects that were allocated via drm_gem_private_object_init().
> That isn't actually necessary for now, but might be in the future.

Generally commmit messages that have too many "also do foo" paragraphs
tack on scream for a patch split up ;-) But the diff here is little enough
that it's imo still ok. So for both patches in this series:

Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>

> 
> Cc: Patrik Jakobsson <patrik.r.jakobsson@xxxxxxxxx>
> Signed-off-by: David Herrmann <dh.herrmann@xxxxxxxxx>
> ---
>  drivers/gpu/drm/drm_gem.c              | 20 ++++++++------------
>  drivers/gpu/drm/gma500/framebuffer.c   |  6 ++----
>  drivers/gpu/drm/gma500/gem.c           |  7 ++++---
>  drivers/gpu/drm/i915/i915_gem_dmabuf.c |  7 +------
>  drivers/gpu/drm/i915/i915_gem_stolen.c |  4 +---
>  drivers/gpu/drm/omapdrm/omap_gem.c     |  3 ++-
>  include/drm/drmP.h                     |  4 ++--
>  7 files changed, 20 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 603f256..1ad9e7e 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -132,16 +132,14 @@ drm_gem_destroy(struct drm_device *dev)
>  int drm_gem_object_init(struct drm_device *dev,
>  			struct drm_gem_object *obj, size_t size)
>  {
> -	BUG_ON((size & (PAGE_SIZE - 1)) != 0);
> +	struct file *filp;
>  
> -	obj->dev = dev;
> -	obj->filp = shmem_file_setup("drm mm object", size, VM_NORESERVE);
> -	if (IS_ERR(obj->filp))
> -		return PTR_ERR(obj->filp);
> +	filp = shmem_file_setup("drm mm object", size, VM_NORESERVE);
> +	if (IS_ERR(filp))
> +		return PTR_ERR(filp);
>  
> -	kref_init(&obj->refcount);
> -	atomic_set(&obj->handle_count, 0);
> -	obj->size = size;
> +	drm_gem_private_object_init(dev, obj, size);
> +	obj->filp = filp;
>  
>  	return 0;
>  }
> @@ -152,8 +150,8 @@ EXPORT_SYMBOL(drm_gem_object_init);
>   * no GEM provided backing store. Instead the caller is responsible for
>   * backing the object and handling it.
>   */
> -int drm_gem_private_object_init(struct drm_device *dev,
> -			struct drm_gem_object *obj, size_t size)
> +void drm_gem_private_object_init(struct drm_device *dev,
> +				 struct drm_gem_object *obj, size_t size)
>  {
>  	BUG_ON((size & (PAGE_SIZE - 1)) != 0);
>  
> @@ -163,8 +161,6 @@ int drm_gem_private_object_init(struct drm_device *dev,
>  	kref_init(&obj->refcount);
>  	atomic_set(&obj->handle_count, 0);
>  	obj->size = size;
> -
> -	return 0;
>  }
>  EXPORT_SYMBOL(drm_gem_private_object_init);
>  
> diff --git a/drivers/gpu/drm/gma500/framebuffer.c b/drivers/gpu/drm/gma500/framebuffer.c
> index 8b1b6d9..362dd2a 100644
> --- a/drivers/gpu/drm/gma500/framebuffer.c
> +++ b/drivers/gpu/drm/gma500/framebuffer.c
> @@ -321,10 +321,8 @@ static struct gtt_range *psbfb_alloc(struct drm_device *dev, int aligned_size)
>  	/* Begin by trying to use stolen memory backing */
>  	backing = psb_gtt_alloc_range(dev, aligned_size, "fb", 1);
>  	if (backing) {
> -		if (drm_gem_private_object_init(dev,
> -					&backing->gem, aligned_size) == 0)
> -			return backing;
> -		psb_gtt_free_range(dev, backing);
> +		drm_gem_private_object_init(dev, &backing->gem, aligned_size);
> +		return backing;
>  	}
>  	return NULL;
>  }
> diff --git a/drivers/gpu/drm/gma500/gem.c b/drivers/gpu/drm/gma500/gem.c
> index eefd6cc..fe1d332 100644
> --- a/drivers/gpu/drm/gma500/gem.c
> +++ b/drivers/gpu/drm/gma500/gem.c
> @@ -261,11 +261,12 @@ static int psb_gem_create_stolen(struct drm_file *file, struct drm_device *dev,
>  	struct gtt_range *gtt = psb_gtt_alloc_range(dev, size, "gem", 1);
>  	if (gtt == NULL)
>  		return -ENOMEM;
> -	if (drm_gem_private_object_init(dev, &gtt->gem, size) != 0)
> -		goto free_gtt;
> +
> +	drm_gem_private_object_init(dev, &gtt->gem, size);
>  	if (drm_gem_handle_create(file, &gtt->gem, handle) == 0)
>  		return 0;
> -free_gtt:
> +
> +	drm_gem_object_release(&gtt->gem);
>  	psb_gtt_free_range(dev, gtt);
>  	return -ENOMEM;
>  }
> diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> index dc53a52..f2e185c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> @@ -289,12 +289,7 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
>  		goto fail_detach;
>  	}
>  
> -	ret = drm_gem_private_object_init(dev, &obj->base, dma_buf->size);
> -	if (ret) {
> -		i915_gem_object_free(obj);
> -		goto fail_detach;
> -	}
> -
> +	drm_gem_private_object_init(dev, &obj->base, dma_buf->size);
>  	i915_gem_object_init(obj, &i915_gem_object_dmabuf_ops);
>  	obj->base.import_attach = attach;
>  
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 982d473..ff27968 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -271,9 +271,7 @@ _i915_gem_object_create_stolen(struct drm_device *dev,
>  	if (obj == NULL)
>  		return NULL;
>  
> -	if (drm_gem_private_object_init(dev, &obj->base, stolen->size))
> -		goto cleanup;
> -
> +	drm_gem_private_object_init(dev, &obj->base, stolen->size);
>  	i915_gem_object_init(obj, &i915_gem_object_stolen_ops);
>  
>  	obj->pages = i915_pages_create_for_stolen(dev,
> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c b/drivers/gpu/drm/omapdrm/omap_gem.c
> index ebbdf41..cbcd71e 100644
> --- a/drivers/gpu/drm/omapdrm/omap_gem.c
> +++ b/drivers/gpu/drm/omapdrm/omap_gem.c
> @@ -1427,8 +1427,9 @@ struct drm_gem_object *omap_gem_new(struct drm_device *dev,
>  		omap_obj->height = gsize.tiled.height;
>  	}
>  
> +	ret = 0;
>  	if (flags & (OMAP_BO_DMA|OMAP_BO_EXT_MEM))
> -		ret = drm_gem_private_object_init(dev, obj, size);
> +		drm_gem_private_object_init(dev, obj, size);
>  	else
>  		ret = drm_gem_object_init(dev, obj, size);
>  
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 12083dc..ee2f049 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -1613,8 +1613,8 @@ struct drm_gem_object *drm_gem_object_alloc(struct drm_device *dev,
>  					    size_t size);
>  int drm_gem_object_init(struct drm_device *dev,
>  			struct drm_gem_object *obj, size_t size);
> -int drm_gem_private_object_init(struct drm_device *dev,
> -			struct drm_gem_object *obj, size_t size);
> +void drm_gem_private_object_init(struct drm_device *dev,
> +				 struct drm_gem_object *obj, size_t size);
>  void drm_gem_object_handle_free(struct drm_gem_object *obj);
>  void drm_gem_vm_open(struct vm_area_struct *vma);
>  void drm_gem_vm_close(struct vm_area_struct *vma);
> -- 
> 1.8.3.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux