[PATCH 0/3] carma-fpga: remove videobuf dependency

[PATCH 0/3] carma-fpga: remove videobuf dependency

[Hans Verkuil]

While checking which drivers were still abusing internal videobuf API
functions I came across these carma fpga misc drivers. These drivers
have absolutely nothing to do with videobuf or the media subsystem.

Drivers shouldn't use those low-level functions in the first place,
and in fact in the long run the videobuf API will be removed altogether.

So remove the videobuf dependency from these two drivers.

This has been compile tested (and that clearly hasn't been done for
carma-fpga-program.c recently).

Greg, is this something you want to take as misc driver maintainer?
That makes more sense than going through the media tree.

The first patch should probably go to 3.18.

I have no idea if anyone can test this with actual hardware. Ira, is
that something you can do?

Regards,

	Hans

[PATCH 1/3] carma-fpga-program.c: fix compile errors

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

drivers/misc/carma/carma-fpga-program.c: In function 'fpga_program_dma':
drivers/misc/carma/carma-fpga-program.c:529:2: error: expected ';' before 'if'
  if (ret) {
  ^
drivers/misc/carma/carma-fpga-program.c: In function 'fpga_read':
drivers/misc/carma/carma-fpga-program.c:752:45: error: 'ppos' undeclared (first use in this function)
  return simple_read_from_buffer(buf, count, ppos,
                                             ^
drivers/misc/carma/carma-fpga-program.c:752:45: note: each undeclared identifier is reported only once for each function it appears in
drivers/misc/carma/carma-fpga-program.c: In function 'fpga_llseek':
drivers/misc/carma/carma-fpga-program.c:765:27: error: 'file' undeclared (first use in this function)
  return fixed_size_llseek(file, offset, origin, priv->fw_size);
                           ^
drivers/misc/carma/carma-fpga-program.c:759:9: warning: unused variable 'newpos' [-Wunused-variable]
  loff_t newpos;
         ^
drivers/misc/carma/carma-fpga-program.c: In function 'fpga_read':
drivers/misc/carma/carma-fpga-program.c:754:1: warning: control reaches end of non-void function [-Wreturn-type]
 }
 ^
drivers/misc/carma/carma-fpga-program.c: In function 'fpga_llseek':
drivers/misc/carma/carma-fpga-program.c:766:1: warning: control reaches end of non-void function [-Wreturn-type]
 }
 ^
scripts/Makefile.build:263: recipe for target 'drivers/misc/carma/carma-fpga-program.o' failed

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/misc/carma/carma-fpga-program.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/carma/carma-fpga-program.c b/drivers/misc/carma/carma-fpga-program.c
index 339b252..eb8942b 100644
--- a/drivers/misc/carma/carma-fpga-program.c
+++ b/drivers/misc/carma/carma-fpga-program.c
@@ -525,7 +525,7 @@ static noinline int fpga_program_dma(struct fpga_dev *priv)
 		goto out_dma_unmap;
 	}
 
-	ret = fsl_dma_external_start(chan, 1)
+	ret = fsl_dma_external_start(chan, 1);
 	if (ret) {
 		dev_err(priv->dev, "DMA external control setup failed\n");
 		goto out_dma_unmap;
@@ -749,20 +749,19 @@ static ssize_t fpga_read(struct file *filp, char __user *buf, size_t count,
 			 loff_t *f_pos)
 {
 	struct fpga_dev *priv = filp->private_data;
-	return simple_read_from_buffer(buf, count, ppos,
+	return simple_read_from_buffer(buf, count, f_pos,
 				       priv->vb.vaddr, priv->bytes);
 }
 
 static loff_t fpga_llseek(struct file *filp, loff_t offset, int origin)
 {
 	struct fpga_dev *priv = filp->private_data;
-	loff_t newpos;
 
 	/* only read-only opens are allowed to seek */
 	if ((filp->f_flags & O_ACCMODE) != O_RDONLY)
 		return -EINVAL;
 
-	return fixed_size_llseek(file, offset, origin, priv->fw_size);
+	return fixed_size_llseek(filp, offset, origin, priv->fw_size);
 }
 
 static const struct file_operations fpga_fops = {
-- 
2.1.3

[PATCH 2/3] carma-fpga: drop videobuf dependency

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This driver abuses videobuf helper functions. This is a bad idea
because:

1) this driver is completely unrelated to media drivers
2) the videobuf API is deprecated and will be removed eventually

This patch replaces the videobuf functions with the normal DMA kernel
API.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/misc/carma/Kconfig      |  3 +-
 drivers/misc/carma/carma-fpga.c | 98 ++++++++++++++++++++++++++++++++++-------
 2 files changed, 82 insertions(+), 19 deletions(-)

diff --git a/drivers/misc/carma/Kconfig b/drivers/misc/carma/Kconfig
index c90370e..c6047fb 100644
--- a/drivers/misc/carma/Kconfig
+++ b/drivers/misc/carma/Kconfig
@@ -1,7 +1,6 @@
 config CARMA_FPGA
 	tristate "CARMA DATA-FPGA Access Driver"
-	depends on FSL_SOC && PPC_83xx && MEDIA_SUPPORT && HAS_DMA && FSL_DMA
-	select VIDEOBUF_DMA_SG
+	depends on FSL_SOC && PPC_83xx && HAS_DMA && FSL_DMA
 	default n
 	help
 	  Say Y here to include support for communicating with the data
diff --git a/drivers/misc/carma/carma-fpga.c b/drivers/misc/carma/carma-fpga.c
index 55e913b..5b12149 100644
--- a/drivers/misc/carma/carma-fpga.c
+++ b/drivers/misc/carma/carma-fpga.c
@@ -98,6 +98,7 @@
 #include <linux/seq_file.h>
 #include <linux/highmem.h>
 #include <linux/debugfs.h>
+#include <linux/vmalloc.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/poll.h>
@@ -105,8 +106,6 @@
 #include <linux/kref.h>
 #include <linux/io.h>
 
-#include <media/videobuf-dma-sg.h>
-
 /* system controller registers */
 #define SYS_IRQ_SOURCE_CTL	0x24
 #define SYS_IRQ_OUTPUT_EN	0x28
@@ -142,7 +141,10 @@ struct fpga_info {
 
 struct data_buf {
 	struct list_head entry;
-	struct videobuf_dmabuf vb;
+	void *vaddr;
+	struct scatterlist *sglist;
+	int sglen;
+	int nr_pages;
 	size_t size;
 };
 
@@ -207,6 +209,68 @@ static void fpga_device_release(struct kref *ref)
  * Data Buffer Allocation Helpers
  */
 
+static int carma_dma_init(struct data_buf *buf, int nr_pages)
+{
+	struct page *pg;
+	int i;
+
+	buf->vaddr = vmalloc_32(nr_pages << PAGE_SHIFT);
+	if (NULL == buf->vaddr) {
+		pr_debug("vmalloc_32(%d pages) failed\n", nr_pages);
+		return -ENOMEM;
+	}
+
+	pr_debug("vmalloc is at addr 0x%08lx, size=%d\n",
+				(unsigned long)buf->vaddr,
+				nr_pages << PAGE_SHIFT);
+
+	memset(buf->vaddr, 0, nr_pages << PAGE_SHIFT);
+	buf->nr_pages = nr_pages;
+
+	buf->sglist = vzalloc(buf->nr_pages * sizeof(*buf->sglist));
+	if (NULL == buf->sglist)
+		goto vzalloc_err;
+
+	sg_init_table(buf->sglist, buf->nr_pages);
+	for (i = 0; i < buf->nr_pages; i++) {
+		pg = vmalloc_to_page(buf->vaddr + i * PAGE_SIZE);
+		if (NULL == pg)
+			goto vmalloc_to_page_err;
+		sg_set_page(&buf->sglist[i], pg, PAGE_SIZE, 0);
+	}
+	return 0;
+
+vmalloc_to_page_err:
+	vfree(buf->sglist);
+	buf->sglist = NULL;
+vzalloc_err:
+	vfree(buf->vaddr);
+	buf->vaddr = NULL;
+	return -ENOMEM;
+}
+
+static int carma_dma_map(struct device *dev, struct data_buf *buf)
+{
+	buf->sglen = dma_map_sg(dev, buf->sglist,
+			buf->nr_pages, DMA_FROM_DEVICE);
+
+	if (0 == buf->sglen) {
+		pr_warn("%s: dma_map_sg failed\n", __func__);
+		return -ENOMEM;
+	}
+	return 0;
+}
+
+static int carma_dma_unmap(struct device *dev, struct data_buf *buf)
+{
+	if (!buf->sglen)
+		return 0;
+
+	dma_unmap_sg(dev, buf->sglist, buf->sglen, DMA_FROM_DEVICE);
+	buf->sglen = 0;
+	return 0;
+}
+
 /**
  * data_free_buffer() - free a single data buffer and all allocated memory
  * @buf: the buffer to free
@@ -221,7 +285,8 @@ static void data_free_buffer(struct data_buf *buf)
 		return;
 
 	/* free all memory */
-	videobuf_dma_free(&buf->vb);
+	vfree(buf->sglist);
+	vfree(buf->vaddr);
 	kfree(buf);
 }
 
@@ -230,7 +295,7 @@ static void data_free_buffer(struct data_buf *buf)
  * @bytes: the number of bytes required
  *
  * This allocates all space needed for a data buffer. It must be mapped before
- * use in a DMA transaction using videobuf_dma_map().
+ * use in a DMA transaction using carma_dma_map().
  *
  * Returns NULL on failure
  */
@@ -252,9 +317,8 @@ static struct data_buf *data_alloc_buffer(const size_t bytes)
 	INIT_LIST_HEAD(&buf->entry);
 	buf->size = bytes;
 
-	/* allocate the videobuf */
-	videobuf_dma_init(&buf->vb);
-	ret = videobuf_dma_init_kernel(&buf->vb, DMA_FROM_DEVICE, nr_pages);
+	/* allocate the buffer */
+	ret = carma_dma_init(buf, nr_pages);
 	if (ret)
 		goto out_free_buf;
 
@@ -285,13 +349,13 @@ static void data_free_buffers(struct fpga_device *priv)
 
 	list_for_each_entry_safe(buf, tmp, &priv->free, entry) {
 		list_del_init(&buf->entry);
-		videobuf_dma_unmap(priv->dev, &buf->vb);
+		carma_dma_unmap(priv->dev, buf);
 		data_free_buffer(buf);
 	}
 
 	list_for_each_entry_safe(buf, tmp, &priv->used, entry) {
 		list_del_init(&buf->entry);
-		videobuf_dma_unmap(priv->dev, &buf->vb);
+		carma_dma_unmap(priv->dev, buf);
 		data_free_buffer(buf);
 	}
 
@@ -330,7 +394,7 @@ static int data_alloc_buffers(struct fpga_device *priv)
 			break;
 
 		/* map it for DMA */
-		ret = videobuf_dma_map(priv->dev, &buf->vb);
+		ret = carma_dma_map(priv->dev, buf);
 		if (ret) {
 			data_free_buffer(buf);
 			break;
@@ -634,8 +698,8 @@ static int data_submit_dma(struct fpga_device *priv, struct data_buf *buf)
 	dma_addr_t dst, src;
 	unsigned long dma_flags = 0;
 
-	dst_sg = buf->vb.sglist;
-	dst_nents = buf->vb.sglen;
+	dst_sg = buf->sglist;
+	dst_nents = buf->sglen;
 
 	src_sg = priv->corl_table.sgl;
 	src_nents = priv->corl_nents;
@@ -1134,7 +1198,7 @@ static ssize_t data_read(struct file *filp, char __user *ubuf, size_t count,
 	spin_unlock_irq(&priv->lock);
 
 	/* Buffers are always mapped: unmap it */
-	videobuf_dma_unmap(priv->dev, &dbuf->vb);
+	carma_dma_unmap(priv->dev, dbuf);
 
 	/* save the buffer for later */
 	reader->buf = dbuf;
@@ -1143,7 +1207,7 @@ static ssize_t data_read(struct file *filp, char __user *ubuf, size_t count,
 have_buffer:
 	/* Get the number of bytes available */
 	avail = dbuf->size - reader->buf_start;
-	data = dbuf->vb.vaddr + reader->buf_start;
+	data = dbuf->vaddr + reader->buf_start;
 
 	/* Get the number of bytes we can transfer */
 	count = min(count, avail);
@@ -1171,7 +1235,7 @@ have_buffer:
 	 * If it fails, we pretend that the read never happed and return
 	 * -EFAULT to userspace. The read will be retried.
 	 */
-	ret = videobuf_dma_map(priv->dev, &dbuf->vb);
+	ret = carma_dma_map(priv->dev, dbuf);
 	if (ret) {
 		dev_err(priv->dev, "unable to remap buffer for DMA\n");
 		return -EFAULT;
@@ -1203,7 +1267,7 @@ out_unlock:
 	spin_unlock_irq(&priv->lock);
 
 	if (drop_buffer) {
-		videobuf_dma_unmap(priv->dev, &dbuf->vb);
+		carma_dma_unmap(priv->dev, dbuf);
 		data_free_buffer(dbuf);
 	}
 
-- 
2.1.3

[PATCH 3/3] carma-fpga-program: drop videobuf dependency

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This driver abuses videobuf helper functions. This is a bad idea
because:

1) this driver is completely unrelated to media drivers
2) the videobuf API is deprecated and will be removed eventually

This patch replaces the videobuf functions with the normal DMA kernel
API.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/misc/carma/Kconfig              |  3 +-
 drivers/misc/carma/carma-fpga-program.c | 97 +++++++++++++++++++++++++++------
 2 files changed, 81 insertions(+), 19 deletions(-)

diff --git a/drivers/misc/carma/Kconfig b/drivers/misc/carma/Kconfig
index c6047fb..295882b 100644
--- a/drivers/misc/carma/Kconfig
+++ b/drivers/misc/carma/Kconfig
@@ -8,8 +8,7 @@ config CARMA_FPGA
 
 config CARMA_FPGA_PROGRAM
 	tristate "CARMA DATA-FPGA Programmer"
-	depends on FSL_SOC && PPC_83xx && MEDIA_SUPPORT && HAS_DMA && FSL_DMA
-	select VIDEOBUF_DMA_SG
+	depends on FSL_SOC && PPC_83xx && HAS_DMA && FSL_DMA
 	default n
 	help
 	  Say Y here to include support for programming the data processing
diff --git a/drivers/misc/carma/carma-fpga-program.c b/drivers/misc/carma/carma-fpga-program.c
index eb8942b..a7496e3 100644
--- a/drivers/misc/carma/carma-fpga-program.c
+++ b/drivers/misc/carma/carma-fpga-program.c
@@ -19,6 +19,7 @@
 #include <linux/fsldma.h>
 #include <linux/interrupt.h>
 #include <linux/highmem.h>
+#include <linux/vmalloc.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
@@ -30,8 +31,6 @@
 #include <linux/fs.h>
 #include <linux/io.h>
 
-#include <media/videobuf-dma-sg.h>
-
 /* MPC8349EMDS specific get_immrbase() */
 #include <sysdev/fsl_soc.h>
 
@@ -67,14 +66,79 @@ struct fpga_dev {
 	/* FPGA Bitfile */
 	struct mutex lock;
 
-	struct videobuf_dmabuf vb;
-	bool vb_allocated;
+	void *vaddr;
+	struct scatterlist *sglist;
+	int sglen;
+	int nr_pages;
+	bool buf_allocated;
 
 	/* max size and written bytes */
 	size_t fw_size;
 	size_t bytes;
 };
 
+static int fpga_dma_init(struct fpga_dev *priv, int nr_pages)
+{
+	struct page *pg;
+	int i;
+
+	priv->vaddr = vmalloc_32(nr_pages << PAGE_SHIFT);
+	if (NULL == priv->vaddr) {
+		pr_debug("vmalloc_32(%d pages) failed\n", nr_pages);
+		return -ENOMEM;
+	}
+
+	pr_debug("vmalloc is at addr 0x%08lx, size=%d\n",
+				(unsigned long)priv->vaddr,
+				nr_pages << PAGE_SHIFT);
+
+	memset(priv->vaddr, 0, nr_pages << PAGE_SHIFT);
+	priv->nr_pages = nr_pages;
+
+	priv->sglist = vzalloc(priv->nr_pages * sizeof(*priv->sglist));
+	if (NULL == priv->sglist)
+		goto vzalloc_err;
+
+	sg_init_table(priv->sglist, priv->nr_pages);
+	for (i = 0; i < priv->nr_pages; i++) {
+		pg = vmalloc_to_page(priv->vaddr + i * PAGE_SIZE);
+		if (NULL == pg)
+			goto vmalloc_to_page_err;
+		sg_set_page(&priv->sglist[i], pg, PAGE_SIZE, 0);
+	}
+	return 0;
+
+vmalloc_to_page_err:
+	vfree(priv->sglist);
+	priv->sglist = NULL;
+vzalloc_err:
+	vfree(priv->vaddr);
+	priv->vaddr = NULL;
+	return -ENOMEM;
+}
+
+static int fpga_dma_map(struct fpga_dev *priv)
+{
+	priv->sglen = dma_map_sg(priv->dev, priv->sglist,
+			priv->nr_pages, DMA_TO_DEVICE);
+
+	if (0 == priv->sglen) {
+		pr_warn("%s: dma_map_sg failed\n", __func__);
+		return -ENOMEM;
+	}
+	return 0;
+}
+
+static int fpga_dma_unmap(struct fpga_dev *priv)
+{
+	if (!priv->sglen)
+		return 0;
+
+	dma_unmap_sg(priv->dev, priv->sglist, priv->sglen, DMA_TO_DEVICE);
+	priv->sglen = 0;
+	return 0;
+}
+
 /*
  * FPGA Bitfile Helpers
  */
@@ -87,8 +151,9 @@ struct fpga_dev {
  */
 static void fpga_drop_firmware_data(struct fpga_dev *priv)
 {
-	videobuf_dma_free(&priv->vb);
-	priv->vb_allocated = false;
+	vfree(priv->sglist);
+	vfree(priv->vaddr);
+	priv->buf_allocated = false;
 	priv->bytes = 0;
 }
 
@@ -427,7 +492,7 @@ static noinline int fpga_program_cpu(struct fpga_dev *priv)
 	dev_dbg(priv->dev, "enabled the controller\n");
 
 	/* Write each chunk of the FPGA bitfile to FPGA programmer */
-	ret = fpga_program_block(priv, priv->vb.vaddr, priv->bytes);
+	ret = fpga_program_block(priv, priv->vaddr, priv->bytes);
 	if (ret)
 		goto out_disable_controller;
 
@@ -463,7 +528,6 @@ out_disable_controller:
  */
 static noinline int fpga_program_dma(struct fpga_dev *priv)
 {
-	struct videobuf_dmabuf *vb = &priv->vb;
 	struct dma_chan *chan = priv->chan;
 	struct dma_async_tx_descriptor *tx;
 	size_t num_pages, len, avail = 0;
@@ -505,7 +569,7 @@ static noinline int fpga_program_dma(struct fpga_dev *priv)
 	}
 
 	/* Map the buffer for DMA */
-	ret = videobuf_dma_map(priv->dev, &priv->vb);
+	ret = fpga_dma_map(priv);
 	if (ret) {
 		dev_err(priv->dev, "Unable to map buffer for DMA\n");
 		goto out_free_table;
@@ -534,7 +598,7 @@ static noinline int fpga_program_dma(struct fpga_dev *priv)
 	/* setup and submit the DMA transaction */
 
 	tx = dmaengine_prep_dma_sg(chan, table.sgl, num_pages,
-			vb->sglist, vb->sglen, 0);
+			priv->sglist, priv->sglen, 0);
 	if (!tx) {
 		dev_err(priv->dev, "Unable to prep DMA transaction\n");
 		ret = -ENOMEM;
@@ -572,7 +636,7 @@ static noinline int fpga_program_dma(struct fpga_dev *priv)
 out_disable_controller:
 	fpga_programmer_disable(priv);
 out_dma_unmap:
-	videobuf_dma_unmap(priv->dev, vb);
+	fpga_dma_unmap(priv);
 out_free_table:
 	sg_free_table(&table);
 out_return:
@@ -702,12 +766,12 @@ static int fpga_open(struct inode *inode, struct file *filp)
 		priv->bytes = 0;
 
 	/* Check if we have already allocated a buffer */
-	if (priv->vb_allocated)
+	if (priv->buf_allocated)
 		return 0;
 
 	/* Allocate a buffer to hold enough data for the bitfile */
 	nr_pages = DIV_ROUND_UP(priv->fw_size, PAGE_SIZE);
-	ret = videobuf_dma_init_kernel(&priv->vb, DMA_TO_DEVICE, nr_pages);
+	ret = fpga_dma_init(priv, nr_pages);
 	if (ret) {
 		dev_err(priv->dev, "unable to allocate data buffer\n");
 		mutex_unlock(&priv->lock);
@@ -715,7 +779,7 @@ static int fpga_open(struct inode *inode, struct file *filp)
 		return ret;
 	}
 
-	priv->vb_allocated = true;
+	priv->buf_allocated = true;
 	return 0;
 }
 
@@ -738,7 +802,7 @@ static ssize_t fpga_write(struct file *filp, const char __user *buf,
 		return -ENOSPC;
 
 	count = min_t(size_t, priv->fw_size - priv->bytes, count);
-	if (copy_from_user(priv->vb.vaddr + priv->bytes, buf, count))
+	if (copy_from_user(priv->vaddr + priv->bytes, buf, count))
 		return -EFAULT;
 
 	priv->bytes += count;
@@ -750,7 +814,7 @@ static ssize_t fpga_read(struct file *filp, char __user *buf, size_t count,
 {
 	struct fpga_dev *priv = filp->private_data;
 	return simple_read_from_buffer(buf, count, f_pos,
-				       priv->vb.vaddr, priv->bytes);
+				       priv->vaddr, priv->bytes);
 }
 
 static loff_t fpga_llseek(struct file *filp, loff_t offset, int origin)
@@ -952,7 +1016,6 @@ static int fpga_of_probe(struct platform_device *op)
 	priv->dev = &op->dev;
 	mutex_init(&priv->lock);
 	init_completion(&priv->completion);
-	videobuf_dma_init(&priv->vb);
 
 	dev_set_drvdata(priv->dev, priv);
 	dma_cap_zero(mask);
-- 
2.1.3

Re: [PATCH 0/3] carma-fpga: remove videobuf dependency

[Hans Verkuil]

Hi Dave,

On 11/24/2014 06:45 PM, Ira Snyder wrote:
> 
> On Nov 23, 2014 6:31 AM, "Hans Verkuil" <hverkuil@xs4all.nl <mailto:hverkuil@xs4all.nl>> wrote:
>>
>> While checking which drivers were still abusing internal videobuf API
>> functions I came across these carma fpga misc drivers. These drivers
>> have absolutely nothing to do with videobuf or the media subsystem.
>>
>> Drivers shouldn't use those low-level functions in the first place,
>> and in fact in the long run the videobuf API will be removed altogether.
>>
>> So remove the videobuf dependency from these two drivers.
>>
>> This has been compile tested (and that clearly hasn't been done for
>> carma-fpga-program.c recently).
>>
>> Greg, is this something you want to take as misc driver maintainer?
>> That makes more sense than going through the media tree.
>>
>> The first patch should probably go to 3.18.
>>
>> I have no idea if anyone can test this with actual hardware. Ira, is
>> that something you can do?
>>
> 
> Hi Hans. My colleague Dave Hawkins (CC'd) is in charge of this hardware now. He can help to get this patch tested.

Any updates on this?

Regards,

	Hans