media: Add timberdale video-in driver

This patch adds the timberdale video-in driver.

The video IP of timberdale delivers the video data via DMA. The driver uses the DMA api to handle DMA transfers, and make use of the V4L2 videobuffers to handle buffers against user space. Due to some timing constraint it makes sense to do DMA into an intermediate buffer and then copy the data to vmalloc:ed buffers.

If available the driver uses an encoder to get/set the video standard

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

+#define TIMBLOGIW_DMA_BUFFER_SIZE (TIMBLOGIW_BYTES_PER_LINE * 576)

Quoted 12 lines · Richard Röjfors

This patch adds the timberdale video-in driver.

The video IP of timberdale delivers the video data via DMA. The driver uses the DMA api to handle DMA transfers, and make use of the V4L2 videobuffers to handle buffers against user space. Due to some timing constraint it makes sense to do DMA into an intermediate buffer and then copy the data to vmalloc:ed buffers.

If available the driver uses an encoder to get/set the video standard

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

+#define TIMBLOGIW_DMA_BUFFER_SIZE (TIMBLOGIW_BYTES_PER_LINE * 576)

...

drivers/media/video/timblogiw.c line 141

Quoted 13 lines

drivers/media/video/timblogiw.c new file +13 -0

+static int __timblogiw_alloc_dma(struct timblogiw_fh *fh, struct device *dev)
+{
+	dma_addr_t addr;
+	int err, i, pos;
+	int bytes_per_desc = TIMBLOGIW_LINES_PER_DESC *
+		timblogiw_bytes_per_line(fh->cur_norm);
+
+	fh->dma.cookie = -1;
+	fh->dma.dev = dev;
+
+	fh->dma.buf = kzalloc(TIMBLOGIW_DMA_BUFFER_SIZE, GFP_KERNEL);
+	if (!fh->dma.buf)
+		return -ENOMEM;

Why do you need a fixed DMA buffer size? Just allocate the buffer size dynamically at buffer_prepare callback.

Review

Quoted 3 lines

(quoted diff) +3 -0

+	videobuf_queue_vmalloc_init(&fh->vb_vidq,&timblogiw_video_qops,
+			NULL,&fh->queue_lock, V4L2_BUF_TYPE_VIDEO_CAPTURE,
+			V4L2_FIELD_NONE, sizeof(struct videobuf_buffer), fh);

You should be using, instead, videobuf_dma_sg or videobuf_cont, instead of using videobuf-vmalloc. This way, you'll avoid double buffering.

Quoted 35 lines · Mauro Carvalho Chehab

Quoted 12 lines · Richard Röjfors

This patch adds the timberdale video-in driver.

The video IP of timberdale delivers the video data via DMA. The driver uses the DMA api to handle DMA transfers, and make use of the V4L2 videobuffers to handle buffers against user space. Due to some timing constraint it makes sense to do DMA into an intermediate buffer and then copy the data to vmalloc:ed buffers.

If available the driver uses an encoder to get/set the video standard

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

+#define TIMBLOGIW_DMA_BUFFER_SIZE (TIMBLOGIW_BYTES_PER_LINE * 576)

...

Quoted 4 lines

+static int __timblogiw_alloc_dma(struct timblogiw_fh *fh, struct device *dev)

drivers/media/video/timblogiw.c new file +2 -0

+{
+    dma_addr_t addr;

drivers/media/video/timblogiw.c line 141

Quoted 10 lines

drivers/media/video/timblogiw.c new file +10 -0

+    int err, i, pos;
+    int bytes_per_desc = TIMBLOGIW_LINES_PER_DESC *
+        timblogiw_bytes_per_line(fh->cur_norm);
+
+    fh->dma.cookie = -1;
+    fh->dma.dev = dev;
+
+    fh->dma.buf = kzalloc(TIMBLOGIW_DMA_BUFFER_SIZE, GFP_KERNEL);
+    if (!fh->dma.buf)
+        return -ENOMEM;

Why do you need a fixed DMA buffer size? Just allocate the buffer size dynamically at
buffer_prepare callback.

Quoted 7 lines

Review

Quoted 3 lines

(quoted diff) +3 -0

+    videobuf_queue_vmalloc_init(&fh->vb_vidq,&timblogiw_video_qops,
+            NULL,&fh->queue_lock, V4L2_BUF_TYPE_VIDEO_CAPTURE,
+            V4L2_FIELD_NONE, sizeof(struct videobuf_buffer), fh);

You should be using, instead, videobuf_dma_sg or videobuf_cont, instead of
using videobuf-vmalloc. This way, you'll avoid double buffering.

1. dma_sg can not be used, the DMA engine requires the memory blocks to be aligned on a factor of bytes per line, so 4K pages wouldn't work.

2.
I tried using videobuf-dma-contig, but got poor performance. I can not really explain why, I though it's due to the fact that the contiguous buffer is allocated coherent -> no caching.
I saw both gstreamer and mplayer perform very badly.
The frame grabber requires the DMA transfer for a frame beeing started while the frame is decoded. When I tested using contigous buffers gstreamer sometimes was that slow that it sometimes missed to have a frame queued when a transfer was finished, so I got frame drops. Any other ideas of the poor performance? otherwise I would like to go for the double buffered solution.

Quoted 12 lines

2.
I tried using videobuf-dma-contig, but got poor performance. I can not really explain why, I though it's due to the fact that the contiguous buffer is allocated coherent -> no caching.
I saw both gstreamer and mplayer perform very badly.
The frame grabber requires the DMA transfer for a frame beeing started while the frame is decoded. When I tested using contigous buffers gstreamer sometimes was that slow that it sometimes missed to have a frame queued when a transfer was finished, so I got frame drops. Any other ideas of the poor performance? otherwise I would like to go for the double buffered solution.

The better is to fix videobuf-dma_contig to better work on your hardware. It makes sense to add a flag to allow specifying if it should use coherent or non-coherent memory for the dma buffer alloc/free calls.

Hi Mauro,

Quoted 13 lines · Mauro Carvalho Chehab

Quoted 12 lines

2.
I tried using videobuf-dma-contig, but got poor performance. I can not really explain why, I though it's due to the fact that the contiguous buffer is allocated coherent -> no caching.
I saw both gstreamer and mplayer perform very badly.
The frame grabber requires the DMA transfer for a frame beeing started while the frame is decoded. When I tested using contigous buffers gstreamer sometimes was that slow that it sometimes missed to have a frame queued when a transfer was finished, so I got frame drops. Any other ideas of the poor performance? otherwise I would like to go for the double buffered solution.

Quoted 4 lines

The better is to fix videobuf-dma_contig to better work on your hardware. It makes sense to add a flag to allow specifying if it should use coherent or non-coherent memory for the dma buffer alloc/free calls.

I have verified the performance issue and it goes back to non cached coherent buffers. I did an update to the videobuf-dma-contig. I did it by adding another init function, which calls videobuf_queue_core_init with another set of qops.
The other set uses the same functions as the uncached version, but a sync function is added, and the alloc_functions are different.

What do you think?

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

---

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

index dce4f3a..2fc923c 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(map->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 void *__videobuf_alloc(size_t size)
+static void *__videobuf_alloc(size_t size, bool cached)
  {
  	struct videobuf_dma_contig_memory *mem;
  	struct videobuf_buffer *vb;

@@ -199,11 +246,22 @@ static void *__videobuf_alloc(size_t size)
  	if (vb) {
  		mem = vb->priv = ((char *)vb) + size;
  		mem->magic = MAGIC_DC_MEM;
+		mem->cached = cached;
  	}
 
  	return vb;
  }
 
+static void *__videobuf_alloc_uncached(size_t size)
+{
+	return __videobuf_alloc(size, false);
+}
+
+static void *__videobuf_alloc_cached(size_t size)
+{
+	return __videobuf_alloc(size, true);
+}
+
  static void *__videobuf_to_vmalloc(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:

@@ -276,6 +325,20 @@ static int __videobuf_mmap_free(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 vm_area_struct *vma)
  {

@@ -321,30 +384,22 @@ static int __videobuf_mmap_mapper(struct videobuf_queue *q,
  	BUG_ON(!mem);
  	MAGIC_CHECK(mem->magic, MAGIC_DC_MEM);
 
-	mem->size = PAGE_ALIGN(q->bufs[first]->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(q->bufs[first]->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;
  	}
 

@@ -422,9 +477,22 @@ static int __videobuf_copy_stream(struct videobuf_queue *q,
  static struct videobuf_qtype_ops qops = {
  	.magic        = MAGIC_QTYPE_OPS,
 
-	.alloc        = __videobuf_alloc,
+	.alloc        = __videobuf_alloc_uncached,
+	.iolock       = __videobuf_iolock,
+	.mmap_free    = __videobuf_mmap_free,
+	.mmap_mapper  = __videobuf_mmap_mapper,
+	.video_copy_to_user = __videobuf_copy_to_user,
+	.copy_stream  = __videobuf_copy_stream,
+	.vmalloc      = __videobuf_to_vmalloc,
+};
+
+static struct videobuf_qtype_ops qops_cached = {
+	.magic        = MAGIC_QTYPE_OPS,
+
+	.alloc        = __videobuf_alloc_cached,
  	.iolock       = __videobuf_iolock,
  	.mmap_free    = __videobuf_mmap_free,
+	.sync         = __videobuf_sync,
  	.mmap_mapper  = __videobuf_mmap_mapper,
  	.video_copy_to_user = __videobuf_copy_to_user,
  	.copy_stream  = __videobuf_copy_stream,

@@ -445,6 +513,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;

@@ -481,9 +563,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 -1

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);
 
--

To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html