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

[Patrik Jakobsson <patrik.r.jakobsson@xxxxxxxxx>]

On Thu, Jul 11, 2013 at 11:56 AM, David Herrmann <dh.herrmann@xxxxxxxxx> 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.
>
> 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
>

Looks good to me. Thanks.

Reviewed-by: Patrik Jakobsson <patrik.r.jakobsson@xxxxxxxxx>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel