Re: [PATCH v3] dma-buf: cleanup dma_buf_export() to make it easily extensible
On 28/01/15 12:54, Sumit Semwal wrote:
> At present, dma_buf_export() takes a series of parameters, which
> makes it difficult to add any new parameters for exporters, if required.
>
> Make it simpler by moving all these parameters into a struct, and pass
> the struct * as parameter to dma_buf_export().
>
> While at it, unite dma_buf_export_named() with dma_buf_export(), and
> change all callers accordingly.
>
> Signed-off-by: Sumit Semwal
Sorry, a few more comments. Should have sent these before but at least
the are all related only to documentation. Once that is fixed then:
Reviewed-by: Daniel Thompson
> ---
> v3: Daniel Thompson caught the C99 warning issue w/ using {0}; using
> {.exp_name = xxx} instead.
>
> v2: add macro to zero out local struct, and fill KBUILD_MODNAME by default
>
> drivers/dma-buf/dma-buf.c | 47
> +-
> drivers/gpu/drm/armada/armada_gem.c| 10 --
> drivers/gpu/drm/drm_prime.c| 12 ---
> drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 9 +++--
> drivers/gpu/drm/i915/i915_gem_dmabuf.c | 10 --
> drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c | 9 -
> drivers/gpu/drm/tegra/gem.c| 10 --
> drivers/gpu/drm/ttm/ttm_object.c | 9 +++--
> drivers/gpu/drm/udl/udl_dmabuf.c | 9 -
> drivers/media/v4l2-core/videobuf2-dma-contig.c | 8 -
> drivers/media/v4l2-core/videobuf2-dma-sg.c | 8 -
> drivers/media/v4l2-core/videobuf2-vmalloc.c| 8 -
> drivers/staging/android/ion/ion.c | 9 +++--
> include/linux/dma-buf.h| 34 +++
Documentation/dma-buf-sharing.txt needs updating as a result of these
changes but its not in the diffstat.
> 14 files changed, 142 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 5be225c2ba98..6d3df3dd9310 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -265,7 +265,7 @@ static inline int is_dma_buf_file(struct file *file)
> }
>
> /**
> - * dma_buf_export_named - Creates a new dma_buf, and associates an anon file
> + * dma_buf_export - Creates a new dma_buf, and associates an anon file
> * with this buffer, so it can be exported.
> * Also connect the allocator specific data and ops to the buffer.
> * Additionally, provide a name string for exporter; useful in debugging.
> @@ -277,31 +277,32 @@ static inline int is_dma_buf_file(struct file *file)
> * @exp_name:[in]name of the exporting module - useful for
> debugging.
> * @resv:[in]reservation-object, NULL to allocate default one.
> *
> + * All the above info comes from struct dma_buf_export_info.
> + *
I'm not at all sure about this. Its a novel trick but won't this make
the HTML docs come out looking a bit weird? Is there any prior art for
double-documenting the structure members like this?
Daniel.
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] dma-buf: cleanup dma_buf_export() to make it easily extensible
At present, dma_buf_export() takes a series of parameters, which
makes it difficult to add any new parameters for exporters, if required.
Make it simpler by moving all these parameters into a struct, and pass
the struct * as parameter to dma_buf_export().
While at it, unite dma_buf_export_named() with dma_buf_export(), and
change all callers accordingly.
Signed-off-by: Sumit Semwal
---
v3: Daniel Thompson caught the C99 warning issue w/ using {0}; using
{.exp_name = xxx} instead.
v2: add macro to zero out local struct, and fill KBUILD_MODNAME by default
drivers/dma-buf/dma-buf.c | 47 +-
drivers/gpu/drm/armada/armada_gem.c| 10 --
drivers/gpu/drm/drm_prime.c| 12 ---
drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 9 +++--
drivers/gpu/drm/i915/i915_gem_dmabuf.c | 10 --
drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c | 9 -
drivers/gpu/drm/tegra/gem.c| 10 --
drivers/gpu/drm/ttm/ttm_object.c | 9 +++--
drivers/gpu/drm/udl/udl_dmabuf.c | 9 -
drivers/media/v4l2-core/videobuf2-dma-contig.c | 8 -
drivers/media/v4l2-core/videobuf2-dma-sg.c | 8 -
drivers/media/v4l2-core/videobuf2-vmalloc.c| 8 -
drivers/staging/android/ion/ion.c | 9 +++--
include/linux/dma-buf.h| 34 +++
14 files changed, 142 insertions(+), 50 deletions(-)
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 5be225c2ba98..6d3df3dd9310 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -265,7 +265,7 @@ static inline int is_dma_buf_file(struct file *file)
}
/**
- * dma_buf_export_named - Creates a new dma_buf, and associates an anon file
+ * dma_buf_export - Creates a new dma_buf, and associates an anon file
* with this buffer, so it can be exported.
* Also connect the allocator specific data and ops to the buffer.
* Additionally, provide a name string for exporter; useful in debugging.
@@ -277,31 +277,32 @@ static inline int is_dma_buf_file(struct file *file)
* @exp_name: [in]name of the exporting module - useful for debugging.
* @resv: [in]reservation-object, NULL to allocate default one.
*
+ * All the above info comes from struct dma_buf_export_info.
+ *
* Returns, on success, a newly created dma_buf object, which wraps the
* supplied private data and operations for dma_buf_ops. On either missing
* ops, or error in allocating struct dma_buf, will return negative error.
*
*/
-struct dma_buf *dma_buf_export_named(void *priv, const struct dma_buf_ops *ops,
- size_t size, int flags, const char *exp_name,
- struct reservation_object *resv)
+struct dma_buf *dma_buf_export(struct dma_buf_export_info *exp_info)
{
struct dma_buf *dmabuf;
struct file *file;
size_t alloc_size = sizeof(struct dma_buf);
- if (!resv)
+ if (!exp_info->resv)
alloc_size += sizeof(struct reservation_object);
else
/* prevent &dma_buf[1] == dma_buf->resv */
alloc_size += 1;
- if (WARN_ON(!priv || !ops
- || !ops->map_dma_buf
- || !ops->unmap_dma_buf
- || !ops->release
- || !ops->kmap_atomic
- || !ops->kmap
- || !ops->mmap)) {
+ if (WARN_ON(!exp_info->priv
+ || !exp_info->ops
+ || !exp_info->ops->map_dma_buf
+ || !exp_info->ops->unmap_dma_buf
+ || !exp_info->ops->release
+ || !exp_info->ops->kmap_atomic
+ || !exp_info->ops->kmap
+ || !exp_info->ops->mmap)) {
return ERR_PTR(-EINVAL);
}
@@ -309,21 +310,22 @@ struct dma_buf *dma_buf_export_named(void *priv, const
struct dma_buf_ops *ops,
if (dmabuf == NULL)
return ERR_PTR(-ENOMEM);
- dmabuf->priv = priv;
- dmabuf->ops = ops;
- dmabuf->size = size;
- dmabuf->exp_name = exp_name;
+ dmabuf->priv = exp_info->priv;
+ dmabuf->ops = exp_info->ops;
+ dmabuf->size = exp_info->size;
+ dmabuf->exp_name = exp_info->exp_name;
init_waitqueue_head(&dmabuf->poll);
dmabuf->cb_excl.poll = dmabuf->cb_shared.poll = &dmabuf->poll;
dmabuf->cb_excl.active = dmabuf->cb_shared.active = 0;
- if (!resv) {
- resv = (struct reservation_object *)&dmabuf[1];
- reservation_object_init(resv);
+ if (!exp_info->resv) {
+ exp_info->resv = (struct reservation_object *)&dmabuf[1];
+ reservation_object_init(exp_info->resv);
}
- dmabuf->resv = resv;
+ dmabuf->resv
Re: [PATCH v3] dma-buf: cleanup dma_buf_export() to make it easily extensible
Em Wed, 28 Jan 2015 18:24:03 +0530
Sumit Semwal escreveu:
> +/**
> + * helper macro for exporters; zeros and fills in most common values
> + */
> +#define DEFINE_DMA_BUF_EXPORT_INFO(a)\
> + struct dma_buf_export_info a = { .exp_name = KBUILD_MODNAME }
> +
I suspect that this will let the other fields not initialized.
You likely need to do:
#define DEFINE_DMA_BUF_EXPORT_INFO(a) \
struct dma_buf_export_info a = {\
.exp_name = KBUILD_MODNAME; \
.fields = 0;\
...
}
Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] dma-buf: cleanup dma_buf_export() to make it easily extensible
Op 28-01-15 om 13:54 schreef Sumit Semwal:
> At present, dma_buf_export() takes a series of parameters, which
> makes it difficult to add any new parameters for exporters, if required.
>
> Make it simpler by moving all these parameters into a struct, and pass
> the struct * as parameter to dma_buf_export().
>
> While at it, unite dma_buf_export_named() with dma_buf_export(), and
> change all callers accordingly.
>
> Signed-off-by: Sumit Semwal
> ---
> v3: Daniel Thompson caught the C99 warning issue w/ using {0}; using
> {.exp_name = xxx} instead.
>
> v2: add macro to zero out local struct, and fill KBUILD_MODNAME by default
>
> drivers/dma-buf/dma-buf.c | 47
> +-
> drivers/gpu/drm/armada/armada_gem.c| 10 --
> drivers/gpu/drm/drm_prime.c| 12 ---
> drivers/gpu/drm/exynos/exynos_drm_dmabuf.c | 9 +++--
> drivers/gpu/drm/i915/i915_gem_dmabuf.c | 10 --
> drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c | 9 -
> drivers/gpu/drm/tegra/gem.c| 10 --
> drivers/gpu/drm/ttm/ttm_object.c | 9 +++--
> drivers/gpu/drm/udl/udl_dmabuf.c | 9 -
> drivers/media/v4l2-core/videobuf2-dma-contig.c | 8 -
> drivers/media/v4l2-core/videobuf2-dma-sg.c | 8 -
> drivers/media/v4l2-core/videobuf2-vmalloc.c| 8 -
> drivers/staging/android/ion/ion.c | 9 +++--
> include/linux/dma-buf.h| 34 +++
> 14 files changed, 142 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 5be225c2ba98..6d3df3dd9310 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -265,7 +265,7 @@ static inline int is_dma_buf_file(struct file *file)
> }
>
> /**
> - * dma_buf_export_named - Creates a new dma_buf, and associates an anon file
> + * dma_buf_export - Creates a new dma_buf, and associates an anon file
> * with this buffer, so it can be exported.
> * Also connect the allocator specific data and ops to the buffer.
> * Additionally, provide a name string for exporter; useful in debugging.
> @@ -277,31 +277,32 @@ static inline int is_dma_buf_file(struct file *file)
> * @exp_name:[in]name of the exporting module - useful for
> debugging.
> * @resv:[in]reservation-object, NULL to allocate default one.
> *
> + * All the above info comes from struct dma_buf_export_info.
> + *
> * Returns, on success, a newly created dma_buf object, which wraps the
> * supplied private data and operations for dma_buf_ops. On either missing
> * ops, or error in allocating struct dma_buf, will return negative error.
> *
> */
> -struct dma_buf *dma_buf_export_named(void *priv, const struct dma_buf_ops
> *ops,
> - size_t size, int flags, const char *exp_name,
> - struct reservation_object *resv)
> +struct dma_buf *dma_buf_export(struct dma_buf_export_info *exp_info)
This function should probably take a const struct dma_buf_export_info here..
Rest looks sane.
~Maarten
> {
> struct dma_buf *dmabuf;
> struct file *file;
> size_t alloc_size = sizeof(struct dma_buf);
> - if (!resv)
> + if (!exp_info->resv)
> alloc_size += sizeof(struct reservation_object);
> else
> /* prevent &dma_buf[1] == dma_buf->resv */
> alloc_size += 1;
>
> - if (WARN_ON(!priv || !ops
> - || !ops->map_dma_buf
> - || !ops->unmap_dma_buf
> - || !ops->release
> - || !ops->kmap_atomic
> - || !ops->kmap
> - || !ops->mmap)) {
> + if (WARN_ON(!exp_info->priv
> + || !exp_info->ops
> + || !exp_info->ops->map_dma_buf
> + || !exp_info->ops->unmap_dma_buf
> + || !exp_info->ops->release
> + || !exp_info->ops->kmap_atomic
> + || !exp_info->ops->kmap
> + || !exp_info->ops->mmap)) {
> return ERR_PTR(-EINVAL);
> }
>
> @@ -309,21 +310,22 @@ struct dma_buf *dma_buf_export_named(void *priv, const
> struct dma_buf_ops *ops,
> if (dmabuf == NULL)
> return ERR_PTR(-ENOMEM);
>
> - dmabuf->priv = priv;
> - dmabuf->ops = ops;
> - dmabuf->size = size;
> - dmabuf->exp_name = exp_name;
> + dmabuf->priv = exp_info->priv;
> + dmabuf->ops = exp_info->ops;
> + dmabuf->size = exp_info->size;
> + dmabuf->exp_name = exp_info->exp_name;
> init_waitqueue_head(&dmabuf->poll);
> dmabuf->cb_excl.poll = dmabuf->cb_shared.poll = &dmabuf->poll;
> dmabuf->cb_excl.active = dmabuf->cb_shared.active = 0;
>
> - if (!resv) {
> - resv = (struct reservation_ob
Re: [PATCH v3] dma-buf: cleanup dma_buf_export() to make it easily extensible
Hi Mauro,
On 28 January 2015 at 18:53, Mauro Carvalho Chehab
wrote:
> Em Wed, 28 Jan 2015 18:24:03 +0530
> Sumit Semwal escreveu:
>
>> +/**
>> + * helper macro for exporters; zeros and fills in most common values
>> + */
>> +#define DEFINE_DMA_BUF_EXPORT_INFO(a)\
>> + struct dma_buf_export_info a = { .exp_name = KBUILD_MODNAME }
>> +
>
> I suspect that this will let the other fields not initialized.
>
> You likely need to do:
>
> #define DEFINE_DMA_BUF_EXPORT_INFO(a) \
> struct dma_buf_export_info a = {\
> .exp_name = KBUILD_MODNAME; \
> .fields = 0;\
> ...
> }
I suspected the same, but Daniel kindly referred to the C99 standard,
which states:
" If there are fewer initializers in a brace-enclosed list than there
are elements or members
of an aggregate, or fewer characters in a string literal used to
initialize an array of known
size than there are elements in the array, the remainder of the
aggregate shall be
initialized implicitly the same as objects that have static storage duration."
So I think we're well covered there?
>
> Regards,
> Mauro
--
Thanks and regards,
Sumit Semwal
Kernel Team Lead - Linaro Mobile Group
Linaro.org │ Open source software for ARM SoCs
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
