media: Add a cached version of the contiguous video buffers

PATCH Superseded Richard Röjfors 2 participants 9 messages Aug 3, 2010 – Aug 9, 2010 Media (V4L/DVB)

Share & tools

Richard Röjfors

author

2010-08-03 15:24 UTC #0 lore raw

PATCH v2 1/3 media: Add a cached version of the contiguous video buffers

This patch adds another init functions in the videobuf-dma-contig which is named _cached in the end. It creates a buffer factory which allocates buffers using kmalloc and the buffers are cached.

A sync callback is added to sync the buffers.

Most of the code is reused from the uncached version a bool is added to the memory struct to flag if the buffers are to be cached or not.

The reason is that I found the performance of the uncached buffers was too poor on an atom-based X86, so I added this cached version.

Signed-off-by Richard Röjfors <[email protected]>

---

drivers/media/video/videobuf-dma-contig.c +106 ~~-30~~

index 74730c6..a35f65a 100644
@@ -27,6 +27,7 @@ struct videobuf_dma_contig_memory {
 	u32 magic;
 	void *vaddr;
 	dma_addr_t dma_handle;
+	bool cached;
 	unsigned long size;
 	int is_userptr;
 };

@@ -38,6 +39,54 @@ struct videobuf_dma_contig_memory {
 		BUG();							    \
 	}
 
+static int __videobuf_dc_alloc(struct device *dev,
+	struct videobuf_dma_contig_memory *mem, unsigned long size)
+{
+	mem->size = size;
+	if (mem->cached) {
+		mem->vaddr = kmalloc(mem->size, GFP_KERNEL);
+		if (mem->vaddr) {
+			int err;
+
+			mem->dma_handle = dma_map_single(dev, mem->vaddr,
+				mem->size, DMA_FROM_DEVICE);
+			err = dma_mapping_error(dev, mem->dma_handle);
+			if (err) {
+				dev_err(dev, "dma_map_single failed\n");
+
+				kfree(mem->vaddr);
+				mem->vaddr = 0;
+				return err;
+			}
+		}
+	} else
+		mem->vaddr = dma_alloc_coherent(dev, mem->size,
+			&mem->dma_handle, GFP_KERNEL);
+
+	if (!mem->vaddr) {
+		dev_err(dev, "memory alloc size %ld failed\n",
+			mem->size);
+		return -ENOMEM;
+	}
+
+	dev_dbg(dev, "dma mapped data is at %p (%ld)\n", mem->vaddr, mem->size);
+
+	return 0;
+}
+
+static void __videobuf_dc_free(struct device *dev,
+	struct videobuf_dma_contig_memory *mem)
+{
+	if (mem->cached) {
+		dma_unmap_single(dev, mem->dma_handle, mem->size,
+			DMA_FROM_DEVICE);
+		kfree(mem->vaddr);
+	} else
+		dma_free_coherent(dev, mem->size, mem->vaddr, mem->dma_handle);
+
+	mem->vaddr = NULL;
+}
+
 static void
 videobuf_vm_open(struct vm_area_struct *vma)
 {

@@ -92,9 +141,7 @@ static void videobuf_vm_close(struct vm_area_struct *vma)
 				dev_dbg(q->dev, "buf[%d] freeing %p\n",
 					i, mem->vaddr);
 
-				dma_free_coherent(q->dev, mem->size,
-						  mem->vaddr, mem->dma_handle);
-				mem->vaddr = NULL;
+				__videobuf_dc_free(q->dev, mem);
 			}
 
 			q->bufs[i]->map   = NULL;

@@ -190,7 +237,7 @@ static int videobuf_dma_contig_user_get(struct videobuf_dma_contig_memory *mem,
 	return ret;
 }
 
-static struct videobuf_buffer *__videobuf_alloc(size_t size)
+static struct videobuf_buffer *__videobuf_alloc(size_t size, bool cached)
 {
 	struct videobuf_dma_contig_memory *mem;
 	struct videobuf_buffer *vb;

@@ -199,11 +246,22 @@ static struct videobuf_buffer *__videobuf_alloc(size_t size)
 	if (vb) {
 		mem = vb->priv = ((char *)vb) + size;
 		mem->magic = MAGIC_DC_MEM;
+		mem->cached = cached;
 	}
 
 	return vb;
 }
 
+static struct videobuf_buffer *__videobuf_alloc_uncached(size_t size)
+{
+	return __videobuf_alloc(size, false);
+}
+
+static struct videobuf_buffer *__videobuf_alloc_cached(size_t size)
+{
+	return __videobuf_alloc(size, true);
+}
+
 static void *__videobuf_to_vaddr(struct videobuf_buffer *buf)
 {
 	struct videobuf_dma_contig_memory *mem = buf->priv;

@@ -241,17 +299,8 @@ static int __videobuf_iolock(struct videobuf_queue *q,
 			return videobuf_dma_contig_user_get(mem, vb);
 
 		/* allocate memory for the read() method */
-		mem->size = PAGE_ALIGN(vb->size);
-		mem->vaddr = dma_alloc_coherent(q->dev, mem->size,
-						&mem->dma_handle, GFP_KERNEL);
-		if (!mem->vaddr) {
-			dev_err(q->dev, "dma_alloc_coherent %ld failed\n",
-					 mem->size);
+		if (__videobuf_dc_alloc(q->dev, mem, PAGE_ALIGN(vb->size)))
 			return -ENOMEM;
-		}
-
-		dev_dbg(q->dev, "dma_alloc_coherent data is at %p (%ld)\n",
-			mem->vaddr, mem->size);
 		break;
 	case V4L2_MEMORY_OVERLAY:
 	default:

@@ -263,6 +312,19 @@ static int __videobuf_iolock(struct videobuf_queue *q,
 	return 0;
 }
 
+static int __videobuf_sync(struct videobuf_queue *q,
+			   struct videobuf_buffer *buf)
+{
+	struct videobuf_dma_contig_memory *mem = buf->priv;
+	BUG_ON(!mem);
+	MAGIC_CHECK(mem->magic, MAGIC_DC_MEM);
+
+	dma_sync_single_for_cpu(q->dev, mem->dma_handle, mem->size,
+		DMA_FROM_DEVICE);
+
+	return 0;
+}
+
 static int __videobuf_mmap_mapper(struct videobuf_queue *q,
 				  struct videobuf_buffer *buf,
 				  struct vm_area_struct *vma)

@@ -290,30 +352,22 @@ static int __videobuf_mmap_mapper(struct videobuf_queue *q,
 	BUG_ON(!mem);
 	MAGIC_CHECK(mem->magic, MAGIC_DC_MEM);
 
-	mem->size = PAGE_ALIGN(buf->bsize);
-	mem->vaddr = dma_alloc_coherent(q->dev, mem->size,
-					&mem->dma_handle, GFP_KERNEL);
-	if (!mem->vaddr) {
-		dev_err(q->dev, "dma_alloc_coherent size %ld failed\n",
-			mem->size);
+	if (__videobuf_dc_alloc(q->dev, mem, PAGE_ALIGN(buf->bsize)))
 		goto error;
-	}
-	dev_dbg(q->dev, "dma_alloc_coherent data is at addr %p (size %ld)\n",
-		mem->vaddr, mem->size);
 
 	/* Try to remap memory */
 
 	size = vma->vm_end - vma->vm_start;
 	size = (size < mem->size) ? size : mem->size;
 
-	vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
+	if (!mem->cached)
+		vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
 	retval = remap_pfn_range(vma, vma->vm_start,
 				 mem->dma_handle >> PAGE_SHIFT,
 				 size, vma->vm_page_prot);
 	if (retval) {
 		dev_err(q->dev, "mmap: remap failed with error %d. ", retval);
-		dma_free_coherent(q->dev, mem->size,
-				  mem->vaddr, mem->dma_handle);
+		__videobuf_dc_free(q->dev, mem);
 		goto error;
 	}

@@ -338,8 +392,18 @@ error:
 static struct videobuf_qtype_ops qops = {
 	.magic        = MAGIC_QTYPE_OPS,
 
-	.alloc        = __videobuf_alloc,
+	.alloc        = __videobuf_alloc_uncached,
+	.iolock       = __videobuf_iolock,
+	.mmap_mapper  = __videobuf_mmap_mapper,
+	.vaddr        = __videobuf_to_vaddr,
+};
+
+static struct videobuf_qtype_ops qops_cached = {
+	.magic        = MAGIC_QTYPE_OPS,
+
+	.alloc        = __videobuf_alloc_cached,
 	.iolock       = __videobuf_iolock,
+	.sync         = __videobuf_sync,
 	.mmap_mapper  = __videobuf_mmap_mapper,
 	.vaddr        = __videobuf_to_vaddr,
 };

@@ -358,6 +422,20 @@ void videobuf_queue_dma_contig_init(struct videobuf_queue *q,
 }
 EXPORT_SYMBOL_GPL(videobuf_queue_dma_contig_init);
 
+void videobuf_queue_dma_contig_init_cached(struct videobuf_queue *q,
+				    const struct videobuf_queue_ops *ops,
+				    struct device *dev,
+				    spinlock_t *irqlock,
+				    enum v4l2_buf_type type,
+				    enum v4l2_field field,
+				    unsigned int msize,
+				    void *priv)
+{
+	videobuf_queue_core_init(q, ops, dev, irqlock, type, field, msize,
+				 priv, &qops_cached);
+}
+EXPORT_SYMBOL_GPL(videobuf_queue_dma_contig_init_cached);
+
 dma_addr_t videobuf_to_dma_contig(struct videobuf_buffer *buf)
 {
 	struct videobuf_dma_contig_memory *mem = buf->priv;

@@ -394,9 +472,7 @@ void videobuf_dma_contig_free(struct videobuf_queue *q,
 		return;
 	}
 
-	/* read() method */
-	dma_free_coherent(q->dev, mem->size, mem->vaddr, mem->dma_handle);
-	mem->vaddr = NULL;
+	__videobuf_dc_free(q->dev, mem);
 }
 EXPORT_SYMBOL_GPL(videobuf_dma_contig_free);

include/media/videobuf-dma-contig.h +9 -0

index ebaa9bc..43b94cd 100644
@@ -25,6 +25,15 @@ void videobuf_queue_dma_contig_init(struct videobuf_queue *q,
 				    unsigned int msize,
 				    void *priv);
 
+void videobuf_queue_dma_contig_init_cached(struct videobuf_queue *q,
+				    const struct videobuf_queue_ops *ops,
+				    struct device *dev,
+				    spinlock_t *irqlock,
+				    enum v4l2_buf_type type,
+				    enum v4l2_field field,
+				    unsigned int msize,
+				    void *priv);
+
 dma_addr_t videobuf_to_dma_contig(struct videobuf_buffer *buf);
 void videobuf_dma_contig_free(struct videobuf_queue *q,
 			      struct videobuf_buffer *buf);

Pawel Osciak

2010-08-04 07:57 UTC #1 lore raw

Hi Richard,

Quoted 5 lines

Richard Röjfors wrote:
This patch adds another init functions in the videobuf-dma-contig which is named _cached in the end. It creates a buffer factory which allocates buffers using kmalloc and the buffers are cached.

Before I review this in more detail, could you elaborate more on this? How large are your buffers, can kmalloc really allocate them for you? I am not convinced how this is supposed to work reliably, especially in a long-running systems.

Best regards
--
Pawel Osciak
Linux Platform Group
Samsung Poland R&D Center

Richard Röjfors

author

2010-08-04 09:40 UTC #2 lore raw

Quoted 8 lines · Pawel Osciak

Hi Richard,
Quoted 5 lines
Richard Röjfors wrote:
This patch adds another init functions in the videobuf-dma-contig which is named _cached in the end. It creates a buffer factory which allocates buffers using kmalloc and the buffers are cached.

Quoted 4 lines

Before I review this in more detail, could you elaborate more on this? How large are your buffers, can kmalloc really allocate them for you? I am not convinced how this is supposed to work reliably, especially in a long-running systems.

The buffers are normally 829440 bytes and yes kmalloc can allocate them. Normally userspace apps seem to request two buffers of this size.

How do you propose to allocate the buffers? They need to be contiguous and using uncached memory gave really bad performance.

--Richard

Pawel Osciak

2010-08-04 09:52 UTC #3 lore raw

Quoted 15 lines

Richard Röjfors <[email protected]> wrote:
Quoted 8 lines · Pawel Osciak
Hi Richard,
Quoted 5 lines
Richard Röjfors wrote:
This patch adds another init functions in the videobuf-dma-contig which is named _cached in the end. It creates a buffer factory which allocates buffers using kmalloc and the buffers are cached.
Quoted 4 lines
Before I review this in more detail, could you elaborate more on this? How large are your buffers, can kmalloc really allocate them for you? I am not convinced how this is supposed to work reliably, especially in a long-running systems.

Quoted 5 lines

The buffers are normally 829440 bytes and yes kmalloc can allocate them. Normally userspace apps seem to request two buffers of this size.
How do you propose to allocate the buffers? They need to be contiguous and using uncached memory gave really bad performance.

829440 bytes is a quite a lot and one can't reliably depend on kmalloc to be able to allocate such big chunks of contiguous memory. Were you testing this on a freshly rebooted system?

What you are asking for is actually a memory management holy grail, there is no ideal solution for contiguous memory allocation. There are and have been attempts at creating such an allocator, but there is no such thing in the kernel as of yet. One very recent proposal for a contiguous memory allocator can be found on this list, look for "The Contiguous Memory Allocator" topic from Jul, 26th.

One solution to have cached buffers is to use bootmem allocation and map those areas as cached manually.

Best regards
--
Pawel Osciak
Linux Platform Group
Samsung Poland R&D Center

Richard Röjfors

author

2010-08-04 10:03 UTC #4 lore raw

Quoted 7 lines · Pawel Osciak

Quoted 3 lines
How do you propose to allocate the buffers? They need to be contiguous and using uncached memory gave really bad performance.
829440 bytes is a quite a lot and one can't reliably depend on kmalloc to be able to allocate such big chunks of contiguous memory. Were you testing this on a freshly rebooted system?

The systems have been running for a while, but not days. I don't see why would dma_alloc_coherent work better than kmalloc?

I suppose bootmem could be used, or allocate the buffers at startup before memory gets fragmented.

--Richard

Pawel Osciak

2010-08-04 10:36 UTC #5 lore raw

Quoted 6 lines

Richard Röjfors <[email protected]> wrote:
Quoted 4 lines · Pawel Osciak
Quoted 3 lines
How do you propose to allocate the buffers? They need to be contiguous and using uncached memory gave really bad performance.

Quoted 7 lines

Quoted 3 lines
829440 bytes is a quite a lot and one can't reliably depend on kmalloc to be able to allocate such big chunks of contiguous memory. Were you testing this on a freshly rebooted system?
The systems have been running for a while, but not days. I don't see why would dma_alloc_coherent work better than kmalloc?

In principle it wouldn't. It's just it's much less intensively used and allocates from a special area. Not really a bullet-proof solution either though, I agree.

Best regards
--
Pawel Osciak
Linux Platform Group
Samsung Poland R&D Center

Richard Röjfors

author

2010-08-04 11:37 UTC #6 lore raw

Quoted 10 lines · Pawel Osciak

Quoted 10 lines
Richard Röjfors<[email protected]> wrote:
Quoted 7 lines · Pawel Osciak
Quoted 3 lines
How do you propose to allocate the buffers? They need to be contiguous and using uncached memory gave really bad performance.
829440 bytes is a quite a lot and one can't reliably depend on kmalloc to be able to allocate such big chunks of contiguous memory. Were you testing this on a freshly rebooted system?

Quoted 7 lines

Quoted 3 lines
The systems have been running for a while, but not days. I don't see why would dma_alloc_coherent work better than kmalloc?
In principle it wouldn't. It's just it's much less intensively used and allocates from a special area. Not really a bullet-proof solution either though, I agree.

So what is your proposal given the current situation? Using dma_alloc_noncoherent instead of kmalloc and use dma_cache_sync instead of dma_sync_single_for_cpu?

--Richard

Richard Röjfors

author

2010-08-05 20:25 UTC #7 lore raw

In reply to Pawel Osciak

Quoted 10 lines · Pawel Osciak

Quoted 10 lines
Richard Röjfors<[email protected]> wrote:
Quoted 7 lines · Pawel Osciak
Quoted 3 lines
How do you propose to allocate the buffers? They need to be contiguous and using uncached memory gave really bad performance.
829440 bytes is a quite a lot and one can't reliably depend on kmalloc to be able to allocate such big chunks of contiguous memory. Were you testing this on a freshly rebooted system?

Quoted 7 lines

Quoted 3 lines
The systems have been running for a while, but not days. I don't see why would dma_alloc_coherent work better than kmalloc?
In principle it wouldn't. It's just it's much less intensively used and allocates from a special area. Not really a bullet-proof solution either though, I agree.

So how do we move forward? I would like to see this kind of patch go in, it obviously makes our video driver useful.

I could change and verify the patch using dma_alloc_noncoherent instead of kmalloc. It would have the same "limitations" as todays' uncached buffers.

--Richard

Pawel Osciak

2010-08-09 08:52 UTC #8 lore raw

Quoted 20 lines

Richard Röjfors <[email protected]> wrote:
Quoted 10 lines · Pawel Osciak
Quoted 10 lines
Richard Röjfors<[email protected]> wrote:
Quoted 7 lines · Pawel Osciak
Quoted 3 lines
How do you propose to allocate the buffers? They need to be contiguous and using uncached memory gave really bad performance.
829440 bytes is a quite a lot and one can't reliably depend on kmalloc to be able to allocate such big chunks of contiguous memory. Were you testing this on a freshly rebooted system?
Quoted 7 lines
Quoted 3 lines
The systems have been running for a while, but not days. I don't see why would dma_alloc_coherent work better than kmalloc?
In principle it wouldn't. It's just it's much less intensively used and allocates from a special area. Not really a bullet-proof solution either though, I agree.

Quoted 5 lines

So how do we move forward? I would like to see this kind of patch go in, it obviously makes our video driver useful.
I could change and verify the patch using dma_alloc_noncoherent instead of kmalloc. It would have the same "limitations" as todays' uncached buffers.

By the way, I am only vaguely familiar with it, but isn't the memory always coherent on Atom?

Best regards
--
Pawel Osciak
Linux Platform Group
Samsung Poland R&D Center