* [PATCH 1/2] drm/gem: simplify object initialization
@ 2013-07-11 9:56 David Herrmann
2013-07-11 9:56 ` [PATCH 2/2] drm/pci: remove useles #if 1 David Herrmann
` (4 more replies)
0 siblings, 5 replies; 6+ messages in thread
From: David Herrmann @ 2013-07-11 9:56 UTC (permalink / raw)
To: dri-devel; +Cc: Dave Airlie
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@gmail.com>
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
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, >t->gem, size) != 0)
- goto free_gtt;
+
+ drm_gem_private_object_init(dev, >t->gem, size);
if (drm_gem_handle_create(file, >t->gem, handle) == 0)
return 0;
-free_gtt:
+
+ drm_gem_object_release(>t->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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] drm/pci: remove useles #if 1
2013-07-11 9:56 [PATCH 1/2] drm/gem: simplify object initialization David Herrmann
@ 2013-07-11 9:56 ` David Herrmann
2013-07-11 10:27 ` [PATCH 1/2] drm/gem: simplify object initialization Chris Wilson
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: David Herrmann @ 2013-07-11 9:56 UTC (permalink / raw)
To: dri-devel; +Cc: Dave Airlie
These don't make any sense, really..
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
drivers/gpu/drm/drm_pci.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_pci.c b/drivers/gpu/drm/drm_pci.c
index 80c0b2b..a7b46ff 100644
--- a/drivers/gpu/drm/drm_pci.c
+++ b/drivers/gpu/drm/drm_pci.c
@@ -52,10 +52,8 @@
drm_dma_handle_t *drm_pci_alloc(struct drm_device * dev, size_t size, size_t align)
{
drm_dma_handle_t *dmah;
-#if 1
unsigned long addr;
size_t sz;
-#endif
/* pci_alloc_consistent only guarantees alignment to the smallest
* PAGE_SIZE order which is greater than or equal to the requested size.
@@ -97,10 +95,8 @@ EXPORT_SYMBOL(drm_pci_alloc);
*/
void __drm_pci_free(struct drm_device * dev, drm_dma_handle_t * dmah)
{
-#if 1
unsigned long addr;
size_t sz;
-#endif
if (dmah->vaddr) {
/* XXX - Is virt_to_page() legal for consistent mem? */
--
1.8.3.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm/gem: simplify object initialization
2013-07-11 9:56 [PATCH 1/2] drm/gem: simplify object initialization David Herrmann
2013-07-11 9:56 ` [PATCH 2/2] drm/pci: remove useles #if 1 David Herrmann
@ 2013-07-11 10:27 ` Chris Wilson
2013-07-11 10:32 ` Daniel Vetter
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Chris Wilson @ 2013-07-11 10:27 UTC (permalink / raw)
To: David Herrmann; +Cc: Dave Airlie, dri-devel
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.
>
> Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
You should also CC any driver maintainers that the patch touches. They
should be acking it at the very least.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm/gem: simplify object initialization
2013-07-11 9:56 [PATCH 1/2] drm/gem: simplify object initialization David Herrmann
2013-07-11 9:56 ` [PATCH 2/2] drm/pci: remove useles #if 1 David Herrmann
2013-07-11 10:27 ` [PATCH 1/2] drm/gem: simplify object initialization Chris Wilson
@ 2013-07-11 10:32 ` Daniel Vetter
2013-07-11 11:10 ` Patrik Jakobsson
2013-07-11 11:57 ` Rob Clark
4 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2013-07-11 10:32 UTC (permalink / raw)
To: David Herrmann; +Cc: Dave Airlie, dri-devel
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@ffwll.ch>
>
> Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> 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, >t->gem, size) != 0)
> - goto free_gtt;
> +
> + drm_gem_private_object_init(dev, >t->gem, size);
> if (drm_gem_handle_create(file, >t->gem, handle) == 0)
> return 0;
> -free_gtt:
> +
> + drm_gem_object_release(>t->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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm/gem: simplify object initialization
2013-07-11 9:56 [PATCH 1/2] drm/gem: simplify object initialization David Herrmann
` (2 preceding siblings ...)
2013-07-11 10:32 ` Daniel Vetter
@ 2013-07-11 11:10 ` Patrik Jakobsson
2013-07-11 11:57 ` Rob Clark
4 siblings, 0 replies; 6+ messages in thread
From: Patrik Jakobsson @ 2013-07-11 11:10 UTC (permalink / raw)
To: David Herrmann; +Cc: Dave Airlie, dri-devel
On Thu, Jul 11, 2013 at 11:56 AM, David Herrmann <dh.herrmann@gmail.com> 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@gmail.com>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> 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, >t->gem, size) != 0)
> - goto free_gtt;
> +
> + drm_gem_private_object_init(dev, >t->gem, size);
> if (drm_gem_handle_create(file, >t->gem, handle) == 0)
> return 0;
> -free_gtt:
> +
> + drm_gem_object_release(>t->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@gmail.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm/gem: simplify object initialization
2013-07-11 9:56 [PATCH 1/2] drm/gem: simplify object initialization David Herrmann
` (3 preceding siblings ...)
2013-07-11 11:10 ` Patrik Jakobsson
@ 2013-07-11 11:57 ` Rob Clark
4 siblings, 0 replies; 6+ messages in thread
From: Rob Clark @ 2013-07-11 11:57 UTC (permalink / raw)
To: David Herrmann; +Cc: Dave Airlie, dri-devel
On Thu, Jul 11, 2013 at 5:56 AM, David Herrmann <dh.herrmann@gmail.com> 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@gmail.com>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Acked-by: Rob Clark <robdclark@gmail.com>
> ---
> 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, >t->gem, size) != 0)
> - goto free_gtt;
> +
> + drm_gem_private_object_init(dev, >t->gem, size);
> if (drm_gem_handle_create(file, >t->gem, handle) == 0)
> return 0;
> -free_gtt:
> +
> + drm_gem_object_release(>t->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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-07-11 11:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-07-11 9:56 [PATCH 1/2] drm/gem: simplify object initialization David Herrmann
2013-07-11 9:56 ` [PATCH 2/2] drm/pci: remove useles #if 1 David Herrmann
2013-07-11 10:27 ` [PATCH 1/2] drm/gem: simplify object initialization Chris Wilson
2013-07-11 10:32 ` Daniel Vetter
2013-07-11 11:10 ` Patrik Jakobsson
2013-07-11 11:57 ` Rob Clark
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).