Re: [PATCH] DRM: add drm gem CMA helper
Hi Sascha,
Thanks for the patch.
On Wednesday 27 June 2012 15:00:05 Sascha Hauer wrote:
Many embedded drm devices do not have a IOMMU and no dedicated
memory for graphics. These devices use CMA (Contiguous Memory
Allocator) backed graphics memory. This patch provides helper
functions to be able to share the code. The code technically does
not depend on CMA as the backend allocator, the name has been chosen
because cma makes for a nice, short but still descriptive function
prefix.
Signed-off-by: Sascha Hauer s.ha...@pengutronix.de
---
changes since v2:
- make drm_gem_cma_create_with_handle static
- use DIV_ROUND_UP(args-width * args-bpp, 8) to calculate the pitch
[snip]
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c
b/drivers/gpu/drm/drm_gem_cma_helper.c new file mode 100644
index 000..417f45e5
--- /dev/null
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -0,0 +1,250 @@
[snip]
+#include linux/mm.h
+#include linux/slab.h
+#include linux/mutex.h
+#include linux/export.h
+#include linux/dma-mapping.h
Nitpicking, I usually sort headers alphabetically, that helps locating
duplicates quickly. It's not mandatory though.
[snip]
+/*
+ * drm_gem_cma_dumb_create - (struct drm_driver)-dumb_create callback
+ * function
+ *
+ * This aligns the pitch and size arguments to the minimum required. wrap
+ * this into your own function if you need bigger alignment.
+ */
+int drm_gem_cma_dumb_create(struct drm_file *file_priv,
+ struct drm_device *dev, struct drm_mode_create_dumb *args)
+{
+ struct drm_gem_cma_object *cma_obj;
+
+ if (args-pitch args-width * DIV_ROUND_UP(args-bpp, 8))
+ args-pitch = args-width * DIV_ROUND_UP(args-bpp, 8);
It's nice to mention in the changelog that you fixed the problem, but it would
be even nicer to really fix it ;-)
+
+ if (args-size args-pitch * args-height)
+ args-size = args-pitch * args-height;
+
+ cma_obj = drm_gem_cma_create_with_handle(file_priv, dev,
+ args-size, args-handle);
+ if (IS_ERR(cma_obj))
+ return PTR_ERR(cma_obj);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(drm_gem_cma_dumb_create);
--
Regards,
Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] DRM: add drm gem CMA helper
On Wed, Jun 27, 2012 at 03:20:27PM +0200, Laurent Pinchart wrote:
Hi Sascha,
Thanks for the patch.
On Wednesday 27 June 2012 15:00:05 Sascha Hauer wrote:
Many embedded drm devices do not have a IOMMU and no dedicated
memory for graphics. These devices use CMA (Contiguous Memory
Allocator) backed graphics memory. This patch provides helper
functions to be able to share the code. The code technically does
not depend on CMA as the backend allocator, the name has been chosen
because cma makes for a nice, short but still descriptive function
prefix.
Signed-off-by: Sascha Hauer s.ha...@pengutronix.de
---
changes since v2:
- make drm_gem_cma_create_with_handle static
- use DIV_ROUND_UP(args-width * args-bpp, 8) to calculate the pitch
[snip]
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c
b/drivers/gpu/drm/drm_gem_cma_helper.c new file mode 100644
index 000..417f45e5
--- /dev/null
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -0,0 +1,250 @@
[snip]
+#include linux/mm.h
+#include linux/slab.h
+#include linux/mutex.h
+#include linux/export.h
+#include linux/dma-mapping.h
Nitpicking, I usually sort headers alphabetically, that helps locating
duplicates quickly. It's not mandatory though.
[snip]
+/*
+ * drm_gem_cma_dumb_create - (struct drm_driver)-dumb_create callback
+ * function
+ *
+ * This aligns the pitch and size arguments to the minimum required. wrap
+ * this into your own function if you need bigger alignment.
+ */
+int drm_gem_cma_dumb_create(struct drm_file *file_priv,
+ struct drm_device *dev, struct drm_mode_create_dumb *args)
+{
+ struct drm_gem_cma_object *cma_obj;
+
+ if (args-pitch args-width * DIV_ROUND_UP(args-bpp, 8))
+ args-pitch = args-width * DIV_ROUND_UP(args-bpp, 8);
It's nice to mention in the changelog that you fixed the problem, but it
would
be even nicer to really fix it ;-)
Damn ;)
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] DRM: add drm gem CMA helper
Hi Sascha,
On Wednesday 30 May 2012 18:28:12 Sascha Hauer wrote:
On Wed, May 30, 2012 at 05:40:13PM +0200, Laurent Pinchart wrote:
Hi Sascha,
Thank you for the patch. I've successfully tested the helper with the new
SH Mobile DRM driver. Just a couple of comments below in addition to
Lars' comments (this is not a full review, just details that caught my
attention when comparing the code with my implementation, and trying to
use it).
+/*
+ * drm_gem_cma_mmap - (struct file_operation)-mmap callback function
+ */
+int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
+{
+ int ret;
+
+ ret = drm_gem_mmap(filp, vma);
+ if (ret)
+ return ret;
+
+ vma-vm_flags = ~VM_PFNMAP;
+ vma-vm_flags |= VM_MIXEDMAP;
Why is this a mixed map ?
Honestly, I don't know. This is copied from the exynos driver. I don't
know much about these flags, so if you think something else is better
here than you're probably right ;)
I wouldn't claim to be an expert on this matter :-) As far as I know, PFNMAP
means that the mapping refers to physical memory with no struct page
associated with it (that could be I/O memory, DRAM reserved at boot time, ...
basically any memory outside of the system memory resources controlled by the
kernel page allocator). MIXEDMAP means that the mapping refers to memory that
contains both PFNMAP regions and non-PFNMAP regions. I would be surprised if
dma_alloc_writecombine() returned such a mix. On the other hand the memory it
returns might be PFNMAP or non-PFNMAP depending on the underneath allocator,
maybe that's why VM_MIXEDMAP was used.
I'd appreciate an expert's opinion on this :-)
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
My implementation maps the whole buffer in one go at mmap time, not page
by page at page fault time. Isn't that more efficient when mapping frame
buffer memory ?
I remember Alan has mentioned this in an earlier review. I'll update the
patch accordingly.
I will fix this and the other things you mentioned and repost.
--
Regards,
Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] DRM: add drm gem CMA helper
Hi Sascha,
Thank you for the patch. I've successfully tested the helper with the new SH
Mobile DRM driver. Just a couple of comments below in addition to Lars'
comments (this is not a full review, just details that caught my attention
when comparing the code with my implementation, and trying to use it).
On Tuesday 29 May 2012 16:10:27 Sascha Hauer wrote:
Many embedded drm devices do not have a IOMMU and no dedicated
memory for graphics. These devices use CMA (Contiguous Memory
Allocator) backed graphics memory. This patch provides helper
functions to be able to share the code.
Signed-off-by: Sascha Hauer s.ha...@pengutronix.de
---
Lars-Peter, please let me know if this fits your needs or if you are missing
something.
drivers/gpu/drm/Kconfig |6 +
drivers/gpu/drm/Makefile |1 +
drivers/gpu/drm/drm_gem_cma_helper.c | 321 +++
include/drm/drm_gem_cma_helper.h | 47 +
4 files changed, 375 insertions(+)
create mode 100644 drivers/gpu/drm/drm_gem_cma_helper.c
create mode 100644 include/drm/drm_gem_cma_helper.h
[snip]
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c
b/drivers/gpu/drm/drm_gem_cma_helper.c new file mode 100644
index 000..75534ff
--- /dev/null
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -0,0 +1,321 @@
[snip]
+/*
+ * drm_gem_cma_create - allocate an object with the given size
+ *
+ * returns a struct drm_gem_cma_obj* on success or ERR_PTR values
+ * on failure.
+ */
+struct drm_gem_cma_obj *drm_gem_cma_create(struct drm_device *drm,
+ unsigned int size)
+{
+ struct drm_gem_cma_obj *cma_obj;
+ struct drm_gem_object *gem_obj;
+ int ret;
+
+ size = roundup(size, PAGE_SIZE);
As PAGE_SIZE is guaranteed to be a power of two, you can use round_up, which
should be faster.
+
+ cma_obj = kzalloc(sizeof(*cma_obj), GFP_KERNEL);
+ if (!cma_obj)
+ return ERR_PTR(-ENOMEM);
+
+ cma_obj-vaddr = dma_alloc_writecombine(drm-dev, size,
+ (dma_addr_t *)cma_obj-paddr,
+ GFP_KERNEL | __GFP_NOWARN);
+ if (!cma_obj-vaddr) {
+ dev_err(drm-dev, failed to allocate buffer with size %d\n,
size);
+ ret = -ENOMEM;
+ goto err_dma_alloc;
+ }
+
+ gem_obj = cma_obj-base;
+
+ ret = drm_gem_object_init(drm, gem_obj, size);
+ if (ret)
+ goto err_obj_init;
+
+ ret = drm_gem_create_mmap_offset(gem_obj);
+ if (ret)
+ goto err_create_mmap_offset;
+
+ return cma_obj;
+
+err_create_mmap_offset:
+ drm_gem_object_release(gem_obj);
+
+err_obj_init:
+ drm_gem_cma_buf_destroy(drm, cma_obj);
+
+err_dma_alloc:
+ kfree(cma_obj);
+
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(drm_gem_cma_create);
[snip]
+/*
+ * drm_gem_cma_mmap - (struct file_operation)-mmap callback function
+ */
+int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
+{
+ int ret;
+
+ ret = drm_gem_mmap(filp, vma);
+ if (ret)
+ return ret;
+
+ vma-vm_flags = ~VM_PFNMAP;
+ vma-vm_flags |= VM_MIXEDMAP;
Why is this a mixed map ?
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
My implementation maps the whole buffer in one go at mmap time, not page by
page at page fault time. Isn't that more efficient when mapping frame buffer
memory ?
[snip]
diff --git a/include/drm/drm_gem_cma_helper.h
b/include/drm/drm_gem_cma_helper.h new file mode 100644
index 000..53b007c
--- /dev/null
+++ b/include/drm/drm_gem_cma_helper.h
@@ -0,0 +1,47 @@
+#ifndef __DRM_GEM_DMA_ALLOC_HELPER_H__
+#define __DRM_GEM_DMA_ALLOC_HELPER_H__
Should this be __DRM_GEM_CMA_HELPER_H__ ?
+
+struct drm_gem_cma_obj {
+ struct drm_gem_object base;
+ dma_addr_t paddr;
+ void __iomem *vaddr;
+};
All drm objects end with _object, would it be better to rename this to struct
drm_gem_cma_object ?
--
Regards,
Laurent Pinchart
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] DRM: add drm gem CMA helper
On Wed, May 30, 2012 at 05:40:13PM +0200, Laurent Pinchart wrote:
Hi Sascha,
Thank you for the patch. I've successfully tested the helper with the new SH
Mobile DRM driver. Just a couple of comments below in addition to Lars'
comments (this is not a full review, just details that caught my attention
when comparing the code with my implementation, and trying to use it).
+/*
+ * drm_gem_cma_mmap - (struct file_operation)-mmap callback function
+ */
+int drm_gem_cma_mmap(struct file *filp, struct vm_area_struct *vma)
+{
+ int ret;
+
+ ret = drm_gem_mmap(filp, vma);
+ if (ret)
+ return ret;
+
+ vma-vm_flags = ~VM_PFNMAP;
+ vma-vm_flags |= VM_MIXEDMAP;
Why is this a mixed map ?
Honestly, I don't know. This is copied from the exynos driver. I don't
know much about these flags, so if you think something else is better
here than you're probably right ;)
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(drm_gem_cma_mmap);
My implementation maps the whole buffer in one go at mmap time, not page by
page at page fault time. Isn't that more efficient when mapping frame buffer
memory ?
I remember Alan has mentioned this in an earlier review. I'll update
the patch accordingly.
I will fix this and the other things you mentioned and repost.
Thanks
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] DRM: add drm gem CMA helper
On 05/29/2012 04:10 PM, Sascha Hauer wrote:
Many embedded drm devices do not have a IOMMU and no dedicated
memory for graphics. These devices use CMA (Contiguous Memory
Allocator) backed graphics memory. This patch provides helper
functions to be able to share the code.
Signed-off-by: Sascha Hauer s.ha...@pengutronix.de
---
Lars-Peter, please let me know if this fits your needs or if you are missing
something.
Awesome :) The overall structure looks basically like what I had, so I can
just use these functions as drop-in replacement. I had some minor
differences in the implementation though. Comments inline.
drivers/gpu/drm/Kconfig |6 +
drivers/gpu/drm/Makefile |1 +
drivers/gpu/drm/drm_gem_cma_helper.c | 321
++
include/drm/drm_gem_cma_helper.h | 47 +
4 files changed, 375 insertions(+)
create mode 100644 drivers/gpu/drm/drm_gem_cma_helper.c
create mode 100644 include/drm/drm_gem_cma_helper.h
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index e354bc0..f62717e 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -53,6 +53,12 @@ config DRM_TTM
GPU memory types. Will be enabled automatically if a device driver
uses it.
+config DRM_GEM_CMA_HELPER
+ tristate
+ depends on DRM
+ help
+ Choose this if you need the GEM cma helper functions
This shouldn't have a help text as it should be selected by the driver and
not by the user. Also the 'depends on DRM' can go away, since it becomes
meaningless if the symbol is not user-selectable.
+
config DRM_TDFX
tristate 3dfx Banshee/Voodoo3+
depends on DRM PCI
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index c20da5b..9a0d98a 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -15,6 +15,7 @@ drm-y := drm_auth.o drm_buffer.o drm_bufs.o
drm_cache.o \
drm_trace_points.o drm_global.o drm_prime.o
drm-$(CONFIG_COMPAT) += drm_ioc32.o
+drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o
drm-usb-y := drm_usb.o
diff --git a/drivers/gpu/drm/drm_gem_cma_helper.c
b/drivers/gpu/drm/drm_gem_cma_helper.c
new file mode 100644
index 000..75534ff
--- /dev/null
+++ b/drivers/gpu/drm/drm_gem_cma_helper.c
@@ -0,0 +1,321 @@
[...]
+/*
+ * drm_gem_cma_create - allocate an object with the given size
+ *
+ * returns a struct drm_gem_cma_obj* on success or ERR_PTR values
+ * on failure.
+ */
+struct drm_gem_cma_obj *drm_gem_cma_create(struct drm_device *drm,
+ unsigned int size)
+{
+ struct drm_gem_cma_obj *cma_obj;
+ struct drm_gem_object *gem_obj;
+ int ret;
+
+ size = roundup(size, PAGE_SIZE);
+
+ cma_obj = kzalloc(sizeof(*cma_obj), GFP_KERNEL);
+ if (!cma_obj)
+ return ERR_PTR(-ENOMEM);
+
+ cma_obj-vaddr = dma_alloc_writecombine(drm-dev, size,
+ (dma_addr_t *)cma_obj-paddr,
The cast shouldn't be necessary.
? [...]
+}
+EXPORT_SYMBOL_GPL(drm_gem_cma_create);
[...]
+
+static int drm_gem_cma_mmap_buffer(struct file *filp,
+ struct vm_area_struct *vma)
+{
+ struct drm_gem_object *gem_obj = filp-private_data;
+ struct drm_gem_cma_obj *cma_obj = to_dma_alloc_gem_obj(gem_obj);
+ unsigned long pfn, vm_size;
+
+ vma-vm_flags |= VM_IO | VM_RESERVED;
+
+ vma-vm_page_prot = pgprot_writecombine(vma-vm_page_prot);
+ vma-vm_file = filp;
+
+ vm_size = vma-vm_end - vma-vm_start;
+
+ /* check if user-requested size is valid. */
+ if (vm_size gem_obj-size)
+ return -EINVAL;
+
+ /*
+ * get page frame number to physical memory to be mapped
+ * to user space.
+ */
+ pfn = cma_obj-paddr PAGE_SHIFT;
+
+ if (remap_pfn_range(vma, vma-vm_start, pfn, vm_size,
+ vma-vm_page_prot)) {
+ dev_err(gem_obj-dev-dev, failed to remap pfn range.\n);
+ return -EAGAIN;
+ }
+
+ return 0;
+}
+
+static const struct file_operations drm_gem_cma_fops = {
+ .mmap = drm_gem_cma_mmap_buffer,
+};
This and the function above seem to be unused. I think it's a relict from
the old Exynos code.
[...]
+/*
+ * drm_gem_cma_dumb_create - (struct drm_driver)-dumb_create callback
+ * function
+ *
+ * This aligns the pitch and size arguments to the minimum required. wrap
+ * this into your own function if you need bigger alignment.
+ */
+int drm_gem_cma_dumb_create(struct drm_file *file_priv,
+ struct drm_device *dev, struct drm_mode_create_dumb *args)
+{
+ struct drm_gem_cma_obj *cma_obj;
+
+ if (args-pitch args-width * args-bpp 3)
+ args-pitch = args-width * args-bpp 3;
I used DIV_ROUND_UP(args-bpp, 8) here. I think it makes more sense for the
generic case.
+
+ if (args-size args-pitch * args-height)
+
Re: [PATCH] DRM: add drm gem CMA helper
On 05/29/2012 04:46 PM, Lars-Peter Clausen wrote: On 05/29/2012 04:10 PM, Sascha Hauer wrote: Many embedded drm devices do not have a IOMMU and no dedicated memory for graphics. These devices use CMA (Contiguous Memory Allocator) backed graphics memory. This patch provides helper functions to be able to share the code. Signed-off-by: Sascha Hauer s.ha...@pengutronix.de --- Lars-Peter, please let me know if this fits your needs or if you are missing something. Awesome :) The overall structure looks basically like what I had, so I can just use these functions as drop-in replacement. I had some minor differences in the implementation though. Comments inline. Did some initial testing and it seems to work fine :) drivers/gpu/drm/Kconfig |6 + drivers/gpu/drm/Makefile |1 + drivers/gpu/drm/drm_gem_cma_helper.c | 321 ++ include/drm/drm_gem_cma_helper.h | 47 + 4 files changed, 375 insertions(+) create mode 100644 drivers/gpu/drm/drm_gem_cma_helper.c create mode 100644 include/drm/drm_gem_cma_helper.h diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index e354bc0..f62717e 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -53,6 +53,12 @@ config DRM_TTM GPU memory types. Will be enabled automatically if a device driver uses it. +config DRM_GEM_CMA_HELPER +tristate +depends on DRM +help + Choose this if you need the GEM cma helper functions This shouldn't have a help text as it should be selected by the driver and not by the user. Also the 'depends on DRM' can go away, since it becomes meaningless if the symbol is not user-selectable. Sorry, ignore that. I though it would appear in menuconfig if it had an help text. But it needs to have a title. The depends on can still go away though. + +#define to_dma_alloc_gem_obj(x) \ +container_of(x, struct drm_gem_cma_obj, base) This looks like it has been missed during some renaming. It should probably be to_drm_gem_cma_obj. And maybe make it an inline function. Thanks, - Lars ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] DRM: add drm gem CMA helper
Hi Lars,
Thanks for your quick comments.
On Tue, May 29, 2012 at 04:46:36PM +0200, Lars-Peter Clausen wrote:
On 05/29/2012 04:10 PM, Sascha Hauer wrote:
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index e354bc0..f62717e 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -53,6 +53,12 @@ config DRM_TTM
GPU memory types. Will be enabled automatically if a device driver
uses it.
+config DRM_GEM_CMA_HELPER
+ tristate
+ depends on DRM
+ help
+ Choose this if you need the GEM cma helper functions
This shouldn't have a help text as it should be selected by the driver and
not by the user. Also the 'depends on DRM' can go away, since it becomes
meaningless if the symbol is not user-selectable.
The other helpers also have a depends on DRM in them. You are right,
it's quite meaningless. The only advantage I can think of is that it
would produce a 'has unmet direct dependencies' if someone outside DRM
would select this. I'll drop it.
+
+static const struct file_operations drm_gem_cma_fops = {
+ .mmap = drm_gem_cma_mmap_buffer,
+};
This and the function above seem to be unused. I think it's a relict from
the old Exynos code.
Indeed. I wonder why my compiler hasn't warned me about this. Will
remove.
Do you think it makes sense to have generic vm_operations struct as well, I
think it will look the same for most drivers:
struct vm_operations_struct drm_gem_cma_vm_ops = {
.fault = drm_gem_cma_fault,
.open = drm_gem_vm_open,
.close = drm_gem_vm_close,
};
As we both can make use of this I'll add it. People who need something
else can still add their own vm_operations_struct.
I integrated your other comments, will repost tomorrow unless there are
more comments on this.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0|
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
