Re: [PATCH 04/12] v4l: vb2-dma-contig: add setup of sglist for MMAP buffers
Hi Laurent, Tomasz, On 06/10/2012 11:28 PM, Laurent Pinchart wrote: Hi Tomasz, On Friday 08 June 2012 16:31:31 Tomasz Stanislawski wrote: Hi Laurent and Subash, I confirm the issue found by Subash. The function vb2_dc_kaddr_to_pages does fail for some occasions. The failures are rather strange like 'got 95 of 150 pages'. It took me some time to find the reason of the problem. I found that dma_alloc_coherent for iommu an ARM does use ioremap_page_range to map a buffer to the kernel space. The mapping is done by updating the page-table. The problem is that any process has a different first-level page-table. The ioremap_page_range updates only the table for init process. The PT present in current->mm shares a majority of entries of 1st-level PT at kernel range (above 0xc000) but *not all*. That is why vb2_dc_kaddr_to_pages worked for small buffers and occasionally failed for larger buffers. I found two ways to fix this problem. a) use&init_mm instead of current->mm while creating an artificial vma b) access the dma memory by calling *((volatile int *)kaddr) = 0; before calling follow_pfn This way a fault is generated and the PT is updated by copying entries from init_mm. What do you think about presented solutions? Just to be sure, this is a hack until dma_get_sgtable is available, and it won't make it to mainline, right ? In that case using init_mm seem easier. Although I agree adding a hack for timebeing, why not use the dma_get_sgtable() RFC itself to solve this in clean way? The hacks anyways cannot go into mainline when vb2 patches get merged. Regards, Subash -- 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 04/12] v4l: vb2-dma-contig: add setup of sglist for MMAP buffers
Hello Tomasz,
On 06/07/2012 06:06 AM, Laurent Pinchart wrote:
Hi Tomasz,
On Wednesday 06 June 2012 13:56:42 Tomasz Stanislawski wrote:
On 06/06/2012 10:06 AM, Laurent Pinchart wrote:
On Wednesday 23 May 2012 15:07:27 Tomasz Stanislawski wrote:
This patch adds the setup of sglist list for MMAP buffers.
It is needed for buffer exporting via DMABUF mechanism.
Signed-off-by: Tomasz Stanislawski
Signed-off-by: Kyungmin Park
---
drivers/media/video/videobuf2-dma-contig.c | 70 +-
1 file changed, 68 insertions(+), 2 deletions(-)
diff --git a/drivers/media/video/videobuf2-dma-contig.c
b/drivers/media/video/videobuf2-dma-contig.c index 52b4f59..ae656be
100644
--- a/drivers/media/video/videobuf2-dma-contig.c
+++ b/drivers/media/video/videobuf2-dma-contig.c
[snip]
+static int vb2_dc_kaddr_to_pages(unsigned long kaddr,
+ struct page **pages, unsigned int n_pages)
+{
+ unsigned int i;
+ unsigned long pfn;
+ struct vm_area_struct vma = {
+ .vm_flags = VM_IO | VM_PFNMAP,
+ .vm_mm = current->mm,
+ };
+
+ for (i = 0; i< n_pages; ++i, kaddr += PAGE_SIZE) {
The follow_pfn() kerneldoc mentions that it looks up a PFN for a user
address. The only users I've found in the kernel sources pass a user
address. Is it legal to use it for kernel addresses ?
It is not completely legal :). As I understand the mm code, the follow_pfn
works only for IO/PFN mappings. This is the typical case (every case?) of
mappings created by dma_alloc_coherent.
In order to make this function work for a kernel pointer, one has to create
an artificial VMA that has IO/PFN bits on.
This solution is a hack-around for dma_get_pages (aka dma_get_sgtable). This
way the dependency on dma_get_pages was broken giving a small hope of
merging vb2 exporting.
Marek prepared a patchset 'ARM: DMA-mapping: new extensions for buffer
sharing' that adds dma buffers with no kernel mappings and dma_get_sgtable
function.
However this patchset is still in a RFC state.
That's totally understood :-) I'm fine with keeping the hack for now until the
dma_get_sgtable() gets in a usable/mergeable state, please just mention it in
the code with something like
/* HACK: This is a temporary workaround until the dma_get_sgtable() function
becomes available. */
I have prepared a patch that removes vb2_dc_kaddr_to_pages and substitutes
it with dma_get_pages. It will become a part of vb2-exporter patches just
after dma_get_sgtable is merged (or at least acked by major maintainers).
The above function call (because of follow_pfn) doesn't succeed for all
the allocated pages. Hence I created a patch(attached) which is based on
[1] series. One can apply it for using your present patch-set in the
meantime.
Regards,
Subash
[1] http://www.spinics.net/lists/kernel/msg1343092.html
>From f9b2eace2eef0038a1830e5e6dd55f3bb6017e1a Mon Sep 17 00:00:00 2001
From: Subash Patel
Date: Thu, 7 Jun 2012 19:49:10 +0530
Subject: [PATCH] v4l2: vb2: use dma_get_sgtable
This is patch to remove vb2_dc_kaddr_to_pages() function with dma_get_sgtable()
in the patch set posted by Tomasz. It was observed that the former function
fails to get the pages, as follow_pfn() can fail for remapped kernel va provided
by the dma-mapping sub-system.
As Tomasz mentioned, the later call was temporary patch until dma-mapping author
finalizes the implementation of dma_get_sgtable(). One can use this patch to use
this vb2 patch-set for his/her work in the meantime.
Signed-off-by: Subash Patel
---
drivers/media/video/videobuf2-dma-contig.c | 53 ++-
1 files changed, 4 insertions(+), 49 deletions(-)
diff --git a/drivers/media/video/videobuf2-dma-contig.c b/drivers/media/video/videobuf2-dma-contig.c
index e8da7f1..1b9023a 100644
--- a/drivers/media/video/videobuf2-dma-contig.c
+++ b/drivers/media/video/videobuf2-dma-contig.c
@@ -143,32 +143,11 @@ static void vb2_dc_put(void *buf_priv)
kfree(buf);
}
-static int vb2_dc_kaddr_to_pages(unsigned long kaddr,
- struct page **pages, unsigned int n_pages)
-{
- unsigned int i;
- unsigned long pfn;
- struct vm_area_struct vma = {
- .vm_flags = VM_IO | VM_PFNMAP,
- .vm_mm = current->mm,
- };
-
- for (i = 0; i < n_pages; ++i, kaddr += PAGE_SIZE) {
- if (follow_pfn(&vma, kaddr, &pfn))
- break;
- pages[i] = pfn_to_page(pfn);
- }
-
- return i;
-}
-
static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size)
{
struct device *dev = alloc_ctx;
struct vb2_dc_buf *buf;
int ret = -ENOMEM;
- int n_pages;
- struct page **pages = NULL;
buf = kzalloc(sizeof *buf, GFP_KERNEL);
if (!buf)
@@ -183,35 +162,14 @@ static void *vb2_dc_alloc(void *alloc_ctx, unsigned long size)
WARN_ON((unsigned long)buf->vaddr & ~PAGE_MASK);
WARN_ON(buf->dma_addr & ~PAGE_MASK);
- n_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
+ ret = dma_get_sgtable(dev, &buf->sgt_base, buf->vad
Re: [PATCHv5 08/13] v4l: vb2-dma-contig: add support for scatterlist in userptr mode
Hello Tomasz, Laurent,
I have printed some logs during the dmabuf export and attach for the SGT
issue below. Please find it in the attachment. I hope it will be useful.
Regards,
Subash
On 05/08/2012 04:45 PM, Subash Patel wrote:
Hi Laurent,
On 05/08/2012 02:44 PM, Laurent Pinchart wrote:
Hi Subash,
On Monday 07 May 2012 20:08:25 Subash Patel wrote:
Hello Thomasz, Laurent,
I found an issue in the function vb2_dc_pages_to_sgt() below. I saw that
during the attach, the size of the SGT and size requested mis-matched
(by atleast 8k bytes). Hence I made a small correction to the code as
below. I could then attach the importer properly.
Thank you for the report.
Could you print the content of the sglist (number of chunks and size
of each
chunk) before and after your modifications, as well as the values of
n_pages,
offset and size ?
I will put back all the printk's and generate this. As of now, my setup
has changed and will do this when I get sometime.
On 04/20/2012 08:15 PM, Tomasz Stanislawski wrote:
[snip]
+static struct sg_table *vb2_dc_pages_to_sgt(struct page **pages,
+ unsigned int n_pages, unsigned long offset, unsigned long size)
+{
+ struct sg_table *sgt;
+ unsigned int chunks;
+ unsigned int i;
+ unsigned int cur_page;
+ int ret;
+ struct scatterlist *s;
+
+ sgt = kzalloc(sizeof *sgt, GFP_KERNEL);
+ if (!sgt)
+ return ERR_PTR(-ENOMEM);
+
+ /* compute number of chunks */
+ chunks = 1;
+ for (i = 1; i< n_pages; ++i)
+ if (pages[i] != pages[i - 1] + 1)
+ ++chunks;
+
+ ret = sg_alloc_table(sgt, chunks, GFP_KERNEL);
+ if (ret) {
+ kfree(sgt);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ /* merging chunks and putting them into the scatterlist */
+ cur_page = 0;
+ for_each_sg(sgt->sgl, s, sgt->orig_nents, i) {
+ unsigned long chunk_size;
+ unsigned int j;
size = PAGE_SIZE;
+
+ for (j = cur_page + 1; j< n_pages; ++j)
for (j = cur_page + 1; j< n_pages; ++j) {
+ if (pages[j] != pages[j - 1] + 1)
+ break;
size += PAGE
}
+
+ chunk_size = ((j - cur_page)<< PAGE_SHIFT) - offset;
+ sg_set_page(s, pages[cur_page], min(size, chunk_size), offset);
[DELETE] size -= chunk_size;
+ offset = 0;
+ cur_page = j;
+ }
+
+ return sgt;
+}
Regards,
Subash
[ 178.545000] vb2_dc_pages_to_sgt() sgt->orig_nents=2
[ 178.545000] vb2_dc_pages_to_sgt():83 cur_page=0
[ 178.55] vb2_dc_pages_to_sgt():84 offset=0
[ 178.555000] vb2_dc_pages_to_sgt():86 chunk_size=131072
[ 178.56] vb2_dc_pages_to_sgt():89 size=4294836224
[ 178.565000] vb2_dc_pages_to_sgt() sgt->orig_nents=2
[ 178.57] vb2_dc_pages_to_sgt():83 cur_page=32
[ 178.575000] vb2_dc_pages_to_sgt():84 offset=0
[ 178.58] vb2_dc_pages_to_sgt():86 chunk_size=262144
[ 178.585000] vb2_dc_pages_to_sgt():89 size=4294574080
[ 178.59] vb2_dc_pages_to_sgt() sgt->orig_nents=1
[ 178.595000] vb2_dc_pages_to_sgt():83 cur_page=0
[ 178.60] vb2_dc_pages_to_sgt():84 offset=0
[ 178.605000] vb2_dc_pages_to_sgt():86 chunk_size=8192
[ 178.61] vb2_dc_pages_to_sgt():89 size=4294959104
[ 178.625000] vb2_dc_pages_to_sgt() sgt->orig_nents=1
[ 178.625000] vb2_dc_pages_to_sgt():83 cur_page=0
[ 178.63] vb2_dc_pages_to_sgt():84 offset=0
[ 178.635000] vb2_dc_pages_to_sgt():86 chunk_size=131072
[ 178.64] vb2_dc_pages_to_sgt():89 size=4294836224
[ 178.645000] vb2_dc_pages_to_sgt() sgt->orig_nents=1
[ 178.65] vb2_dc_pages_to_sgt():83 cur_page=0
[ 178.655000] vb2_dc_pages_to_sgt():84 offset=0
[ 178.66] vb2_dc_pages_to_sgt():86 chunk_size=131072
[ 178.665000] vb2_dc_pages_to_sgt():89 size=4294836224
[ 178.67] vb2_dc_mmap: mapped dma addr 0x2006 at 0xb6e01000, size
131072
[ 178.67] vb2_dc_mmap: mapped dma addr 0x2008 at 0xb6de1000, size
131072
[ 178.68] vb2_dc_pages_to_sgt() sgt->orig_nents=2
[ 178.685000] vb2_dc_pages_to_sgt():83 cur_page=0
[ 178.69] vb2_dc_pages_to_sgt():84 offset=0
[ 178.695000] vb2_dc_pages_to_sgt():86 chunk_size=4096
[ 178.70] vb2_dc_pages_to_sgt():89 size=4294963200
[ 178.705000] vb2_dc_pages_to_sgt() sgt->orig_nents=2
[ 178.71] vb2_dc_pages_to_sgt():83 cur_page=1
[ 178.715000] vb2_dc_pages_to_sgt():84 offset=0
[ 178.715000] vb2_dc_pages_to_sgt():86 chunk_size=8192
[ 178.72] vb2_dc_pages_to_sgt():89 size=4294955008
[ 178.725000] vb2_dc_pages_to_sgt() sgt->orig_nents=1
[ 178.73] vb2_dc_pages_to_sgt():83 cur_page=0
[ 178.735000] vb2_dc_pages_to_sgt():84 offset=0
[ 178.74] vb2_dc_pages_to_sgt():86 chunk_size=8192
[ 178.745000] vb2_dc_pages_to_sgt():89 size=4294959104
[ 178.75] vb2_dc_pages_to_sgt() sgt->orig_nents=1
[ 178.755000] vb2_dc_pages_to_sgt():83 cur_page=0
[ 178.76] vb2_dc_pages_to_sgt():84 offset=0
[ 178.765000] vb2_dc_pages_to_sgt():86 chunk_size=131072
[ 178.77] vb2_dc_pages_to_sgt():89 size=4294836224
[ 178.78] vb2_dc_pages_to_sgt() sgt->orig_nents=2
[ 178.78] vb2_dc_pages_to_sgt():83 cur_page=0
[ 178.785000] vb2_dc_pages_to_sgt():84 offset=0
Re: [PATCHv5 08/13] v4l: vb2-dma-contig: add support for scatterlist in userptr mode
Hi Laurent,
On 05/08/2012 02:44 PM, Laurent Pinchart wrote:
Hi Subash,
On Monday 07 May 2012 20:08:25 Subash Patel wrote:
Hello Thomasz, Laurent,
I found an issue in the function vb2_dc_pages_to_sgt() below. I saw that
during the attach, the size of the SGT and size requested mis-matched
(by atleast 8k bytes). Hence I made a small correction to the code as
below. I could then attach the importer properly.
Thank you for the report.
Could you print the content of the sglist (number of chunks and size of each
chunk) before and after your modifications, as well as the values of n_pages,
offset and size ?
I will put back all the printk's and generate this. As of now, my setup
has changed and will do this when I get sometime.
On 04/20/2012 08:15 PM, Tomasz Stanislawski wrote:
[snip]
+static struct sg_table *vb2_dc_pages_to_sgt(struct page **pages,
+ unsigned int n_pages, unsigned long offset, unsigned long size)
+{
+ struct sg_table *sgt;
+ unsigned int chunks;
+ unsigned int i;
+ unsigned int cur_page;
+ int ret;
+ struct scatterlist *s;
+
+ sgt = kzalloc(sizeof *sgt, GFP_KERNEL);
+ if (!sgt)
+ return ERR_PTR(-ENOMEM);
+
+ /* compute number of chunks */
+ chunks = 1;
+ for (i = 1; i< n_pages; ++i)
+ if (pages[i] != pages[i - 1] + 1)
+ ++chunks;
+
+ ret = sg_alloc_table(sgt, chunks, GFP_KERNEL);
+ if (ret) {
+ kfree(sgt);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ /* merging chunks and putting them into the scatterlist */
+ cur_page = 0;
+ for_each_sg(sgt->sgl, s, sgt->orig_nents, i) {
+ unsigned long chunk_size;
+ unsigned int j;
size = PAGE_SIZE;
+
+ for (j = cur_page + 1; j< n_pages; ++j)
for (j = cur_page + 1; j< n_pages; ++j) {
+ if (pages[j] != pages[j - 1] + 1)
+ break;
size += PAGE
}
+
+ chunk_size = ((j - cur_page)<< PAGE_SHIFT) - offset;
+ sg_set_page(s, pages[cur_page], min(size, chunk_size), offset);
[DELETE] size -= chunk_size;
+ offset = 0;
+ cur_page = j;
+ }
+
+ return sgt;
+}
Regards,
Subash
--
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: [PATCHv5 08/13] v4l: vb2-dma-contig: add support for scatterlist in userptr mode
Hi Thomasz, I have extended the MFC-FIMC testcase posted by Kamil sometime ago to sanity test the UMM patches. This test is multi-threaded(further explanation for developers who may not have seen it yet), where thread one parses the encoded stream and feeds into the codec IP driver(aka MFC driver). Second thread will dequeue the buffer from MFC driver (DMA_BUF export) and queues it into a CSC IP(aka FIMC) driver(DMA_BUF import). Third thread dequeues the frame from FIMC driver and either pushes it into LCD driver for display or dumps to a flat file for analysis. MFC driver exports the fd's and FIMC driver imports and attaches it. During FIMC QBUF (thats when the attach and map happens), it is observed that in the function vb2_dc_map_dmabuf() call to vb2_dc_get_contiguous_size() fails. This is because contig_size < buf->size. contig_size is calculated from the SGT which we would have constructed in the function vb2_dc_pages_to_sgt(). Let me know if you need more details. Regards, Subash On 05/07/2012 08:41 PM, Tomasz Stanislawski wrote: Hi Subash, Could you provide a detailed description of a test case that causes a failure of vb2_dc_pages_to_sgt? Regards, Tomasz Stanislawski -- 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: [PATCHv5 08/13] v4l: vb2-dma-contig: add support for scatterlist in userptr mode
Hello Thomasz, Laurent,
I found an issue in the function vb2_dc_pages_to_sgt() below. I saw that
during the attach, the size of the SGT and size requested mis-matched
(by atleast 8k bytes). Hence I made a small correction to the code as
below. I could then attach the importer properly.
Regards,
Subash
On 04/20/2012 08:15 PM, Tomasz Stanislawski wrote:
From: Andrzej Pietrasiewicz
This patch introduces usage of dma_map_sg to map memory behind
a userspace pointer to a device as dma-contiguous mapping.
Signed-off-by: Andrzej Pietrasiewicz
Signed-off-by: Marek Szyprowski
[bugfixing]
Signed-off-by: Kamil Debski
[bugfixing]
Signed-off-by: Tomasz Stanislawski
[add sglist subroutines/code refactoring]
Signed-off-by: Kyungmin Park
---
drivers/media/video/videobuf2-dma-contig.c | 279 ++--
1 files changed, 262 insertions(+), 17 deletions(-)
diff --git a/drivers/media/video/videobuf2-dma-contig.c
b/drivers/media/video/videobuf2-dma-contig.c
index 476e536..9cbc8d4 100644
--- a/drivers/media/video/videobuf2-dma-contig.c
+++ b/drivers/media/video/videobuf2-dma-contig.c
@@ -11,6 +11,8 @@
*/
#include
+#include
+#include
#include
#include
@@ -22,6 +24,8 @@ struct vb2_dc_buf {
void*vaddr;
unsigned long size;
dma_addr_t dma_addr;
+ enum dma_data_direction dma_dir;
+ struct sg_table *dma_sgt;
/* MMAP related */
struct vb2_vmarea_handler handler;
@@ -32,6 +36,95 @@ struct vb2_dc_buf {
};
/*/
+/*scatterlist table functions*/
+/*/
+
+static struct sg_table *vb2_dc_pages_to_sgt(struct page **pages,
+ unsigned int n_pages, unsigned long offset, unsigned long size)
+{
+ struct sg_table *sgt;
+ unsigned int chunks;
+ unsigned int i;
+ unsigned int cur_page;
+ int ret;
+ struct scatterlist *s;
+
+ sgt = kzalloc(sizeof *sgt, GFP_KERNEL);
+ if (!sgt)
+ return ERR_PTR(-ENOMEM);
+
+ /* compute number of chunks */
+ chunks = 1;
+ for (i = 1; i< n_pages; ++i)
+ if (pages[i] != pages[i - 1] + 1)
+ ++chunks;
+
+ ret = sg_alloc_table(sgt, chunks, GFP_KERNEL);
+ if (ret) {
+ kfree(sgt);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ /* merging chunks and putting them into the scatterlist */
+ cur_page = 0;
+ for_each_sg(sgt->sgl, s, sgt->orig_nents, i) {
+ unsigned long chunk_size;
+ unsigned int j;
size = PAGE_SIZE;
+
+ for (j = cur_page + 1; j< n_pages; ++j)
for (j = cur_page + 1; j < n_pages; ++j) {
+ if (pages[j] != pages[j - 1] + 1)
+ break;
size += PAGE
}
+
+ chunk_size = ((j - cur_page)<< PAGE_SHIFT) - offset;
+ sg_set_page(s, pages[cur_page], min(size, chunk_size), offset);
[DELETE] size -= chunk_size;
+ offset = 0;
+ cur_page = j;
+ }
+
+ return sgt;
+}
+
+static void vb2_dc_release_sgtable(struct sg_table *sgt)
+{
+ sg_free_table(sgt);
+ kfree(sgt);
+}
+
+static void vb2_dc_sgt_foreach_page(struct sg_table *sgt,
+ void (*cb)(struct page *pg))
+{
+ struct scatterlist *s;
+ unsigned int i;
+
+ for_each_sg(sgt->sgl, s, sgt->nents, i) {
+ struct page *page = sg_page(s);
+ unsigned int n_pages = PAGE_ALIGN(s->offset + s->length)
+ >> PAGE_SHIFT;
+ unsigned int j;
+
+ for (j = 0; j< n_pages; ++j, ++page)
+ cb(page);
+ }
+}
+
+static unsigned long vb2_dc_get_contiguous_size(struct sg_table *sgt)
+{
+ struct scatterlist *s;
+ dma_addr_t expected = sg_dma_address(sgt->sgl);
+ unsigned int i;
+ unsigned long size = 0;
+
+ for_each_sg(sgt->sgl, s, sgt->nents, i) {
+ if (sg_dma_address(s) != expected)
+ break;
+ expected = sg_dma_address(s) + sg_dma_len(s);
+ size += sg_dma_len(s);
+ }
+ return size;
+}
+
+/*/
/* callbacks for all buffers */
/*/
@@ -116,42 +209,194 @@ static int vb2_dc_mmap(void *buf_priv, struct
vm_area_struct *vma)
/* callbacks for USERPTR buffers */
/*/
+static inline int vma_is_io(struct vm_area_struct *vma)
+{
+ return !!(vma->vm_flags& (VM_IO | VM_PFNMAP));
+}
+
+static struct vm_area_struct *vb2_dc_get_user_vma(
+ unsigned long start, unsign
Re: [Linaro-mm-sig] [PATCH 05/10] v4l: add buffer exporting via dmabuf
Hello Tomasz,
Sorry for a late reply. Please find my answers inline.
On 01/24/2012 05:36 PM, Tomasz Stanislawski wrote:
Hi,
On 01/24/2012 12:07 PM, Subash Patel wrote:
Hello Thomasz,
Instead of adding another IOCTL to query the file-descriptor in
user-space, why dont we extend the existing ones in v4l2/vb2?
When the memory type set is V4L2_MEMORY_DMABUF, call to VIDIOC_REQBUFS
/VIDIOC_QUERYBUF from driver can take/return the fd. We will need to add
another attribute to struct v4l2_requestbuffers for this.
struct v4l2_requestbuffers {
...
__u32 buf_fd;
};
The application requesting buffer would set it to -1, and will receive
the fd's when it calls the vb2_querybuf. In the same way, application
which is trying to attach to already allocated buffer will set this to
valid fd when it calls vidioc_reqbuf, and in vb2_reqbuf depending on the
memory type, this can be checked and used to attach with the dma_buf for
the respective buffer.
Ofcourse, this requires changes in vb2_reqbufs and vb2_querybuf similar
to what you did in vb2_expbufs.
Will there be any issues in such an approach?
I like your idea but IMO there are some issues which this approach.
The VIDIOC_REQBUF is used to create a collection of buffers (i.e. 5
frames). Every frame is a separate buffer therefore it should have
separate dmabuf file descriptor. This way you should add buffer index to
v4l2_request_buffers. Of course but one could reuse count field but
there is still problem with multiplane buffers (see below).
I agree.
Please note that dmabuf file could be create only for buffers with MMAP
memory type. The DMABUF memory type for VIDIOC_REQBUFS indicate that a
buffer is imported from DMABUF file descriptor. Similar way like content
of USERPTR buffers is taken from user pointer.
Therefore only MMAP buffers can be exported as DMABUF file descriptor.
I think for time being, mmap is best way we can share buffers between
IP's, like MM and GPU.
If VIDIOC_REQUBUF is used for buffer exporting, how to request a bunch
of buffers that are dedicated to obtain memory from DMABUF file
descriptors?
I am not sure if I understand this question. But what I meant is,
VIDIOC_REQBUF with memory type V4L2_MEMORY_DMABUF will ask the driver to
allocate MMAP type memory and create the dmabuf handles for those. In
the next call to VIDIOC_QUERYBUF (which you do on each of the buffers
infact), driver will return the dmabuf file descriptors to user space.
This can be used by the user space to mmap(when dmabuf supports) or pass
it onto another process to share the buffers.
The recipient user process can now pass these dmabuf file descriptors in
VIDIOC_REQBUF itself to its driver (say another v4l2 based). In the
driver, when the user space calls VIDIOC_QUERYBUF for those buffers, we
can call dmabuf's attach() to actually link to the buffer. Does it make
sense?
The second problem is VIDIOC_QUERYBUF. According to V4L2 spec, the
memory type field is read only. The driver returns the same memory type
as it was used in VIDIOC_REQBUFS. Therefore VIDIOC_QUERYBUF can only be
used for MMAP buffers to obtain memoffset. For DMABUF it may return
fd=-1 or most recently used file descriptor. As I remember, it was not
defined yet.
I was thinking why not the memory type move out from enum to an int? In
that case, we can send ORed types, like
V4L2_MEMORY_MMAP|V4L2_MEMORY_DMABUF etc. Does it help?
The third reason are multiplane buffers. This API was introduced if V4L2
buffer (IMO it should be called a frame) consist of multiple memory
buffers. Every plane can be mmapped separately. Therefore it should be
possible to export every plane as separate DMABUF descriptor.
Do we require to export every plane as separate dmabuf? I think dmabuf
should cover entire multi-plane buffer instead of individual planes. How
to calculate individual plane offsets should be the property of the
driver depending on its need for it.
After some research it was found that memoffset is good identifier of a
plane in a buffers. This id is also available in the userspace without
any API extensions.
VIDIOC_EXPBUF was used to export a buffer by id, similar way as mmap
uses this identifier to map a buffer to userspace. It seams to be the
simplest solution.
Ok. Then dont you think when dmabuf supports mmaping the buffers,
VIDIOC_REQBUF will be useless? VIDIOC_EXPBUF will provide that
functionality as well.
Fourth reason, VIDIOC_REQBUF means that the application request a
buffer. DMABUF framework is used to export existing buffers. Note that
memory may not be pinned to a buffer for some APIs like DRM. I think
that is should stated explicitly that application wants to export not
request a buffer. Using VIDIOC_REQBUF seams to be an API abuse.
I agree that memory may not be static like V4L2 in case of DRM. But this
is still an issue even with a new IOCTL :) VIDIOC_REQBUF might not be
abused as far I feel. It is extended to a) request buffers if not
alloc
Re: [Linaro-mm-sig] [PATCH 05/10] v4l: add buffer exporting via dmabuf
Hello Thomasz,
Instead of adding another IOCTL to query the file-descriptor in
user-space, why dont we extend the existing ones in v4l2/vb2?
When the memory type set is V4L2_MEMORY_DMABUF, call to VIDIOC_REQBUFS
/VIDIOC_QUERYBUF from driver can take/return the fd. We will need to add
another attribute to struct v4l2_requestbuffers for this.
struct v4l2_requestbuffers {
...
__u32 buf_fd;
};
The application requesting buffer would set it to -1, and will receive
the fd's when it calls the vb2_querybuf. In the same way, application
which is trying to attach to already allocated buffer will set this to
valid fd when it calls vidioc_reqbuf, and in vb2_reqbuf depending on the
memory type, this can be checked and used to attach with the dma_buf for
the respective buffer.
Ofcourse, this requires changes in vb2_reqbufs and vb2_querybuf similar
to what you did in vb2_expbufs.
Will there be any issues in such an approach?
Regards,
Subash
On 01/23/2012 07:21 PM, Tomasz Stanislawski wrote:
This patch adds extension to V4L2 api. It allow to export a mmap buffer as file
descriptor. New ioctl VIDIOC_EXPBUF is added. It takes a buffer offset used by
mmap and return a file descriptor on success.
Signed-off-by: Tomasz Stanislawski
Signed-off-by: Kyungmin Park
---
drivers/media/video/v4l2-compat-ioctl32.c |1 +
drivers/media/video/v4l2-ioctl.c | 11 +++
include/linux/videodev2.h |1 +
include/media/v4l2-ioctl.h|1 +
4 files changed, 14 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/v4l2-compat-ioctl32.c
b/drivers/media/video/v4l2-compat-ioctl32.c
index c68531b..0f18b5e 100644
--- a/drivers/media/video/v4l2-compat-ioctl32.c
+++ b/drivers/media/video/v4l2-compat-ioctl32.c
@@ -954,6 +954,7 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int
cmd, unsigned long arg)
case VIDIOC_S_FBUF32:
case VIDIOC_OVERLAY32:
case VIDIOC_QBUF32:
+ case VIDIOC_EXPBUF:
case VIDIOC_DQBUF32:
case VIDIOC_STREAMON32:
case VIDIOC_STREAMOFF32:
diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index e1da8fc..cb29e00 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -207,6 +207,7 @@ static const char *v4l2_ioctls[] = {
[_IOC_NR(VIDIOC_S_FBUF)] = "VIDIOC_S_FBUF",
[_IOC_NR(VIDIOC_OVERLAY)] = "VIDIOC_OVERLAY",
[_IOC_NR(VIDIOC_QBUF)] = "VIDIOC_QBUF",
+ [_IOC_NR(VIDIOC_EXPBUF)] = "VIDIOC_EXPBUF",
[_IOC_NR(VIDIOC_DQBUF)]= "VIDIOC_DQBUF",
[_IOC_NR(VIDIOC_STREAMON)] = "VIDIOC_STREAMON",
[_IOC_NR(VIDIOC_STREAMOFF)]= "VIDIOC_STREAMOFF",
@@ -932,6 +933,16 @@ static long __video_do_ioctl(struct file *file,
dbgbuf(cmd, vfd, p);
break;
}
+ case VIDIOC_EXPBUF:
+ {
+ unsigned int *p = arg;
+
+ if (!ops->vidioc_expbuf)
+ break;
+
+ ret = ops->vidioc_expbuf(file, fh, *p);
+ break;
+ }
case VIDIOC_DQBUF:
{
struct v4l2_buffer *p = arg;
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index 3c0ade1..448fbed 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -2183,6 +2183,7 @@ struct v4l2_create_buffers {
#define VIDIOC_S_FBUF _IOW('V', 11, struct v4l2_framebuffer)
#define VIDIOC_OVERLAY _IOW('V', 14, int)
#define VIDIOC_QBUF _IOWR('V', 15, struct v4l2_buffer)
+#define VIDIOC_EXPBUF _IOWR('V', 16, __u32)
#define VIDIOC_DQBUF _IOWR('V', 17, struct v4l2_buffer)
#define VIDIOC_STREAMON_IOW('V', 18, int)
#define VIDIOC_STREAMOFF _IOW('V', 19, int)
diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
index 4d1c74a..8201546 100644
--- a/include/media/v4l2-ioctl.h
+++ b/include/media/v4l2-ioctl.h
@@ -120,6 +120,7 @@ struct v4l2_ioctl_ops {
int (*vidioc_reqbufs) (struct file *file, void *fh, struct
v4l2_requestbuffers *b);
int (*vidioc_querybuf)(struct file *file, void *fh, struct v4l2_buffer
*b);
int (*vidioc_qbuf)(struct file *file, void *fh, struct v4l2_buffer
*b);
+ int (*vidioc_expbuf) (struct file *file, void *fh, __u32 offset);
int (*vidioc_dqbuf) (struct file *file, void *fh, struct v4l2_buffer
*b);
int (*vidioc_create_bufs)(struct file *file, void *fh, struct
v4l2_create_buffers *b);
--
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: [Linaro-mm-sig] [PATCH 8/9] ARM: integrate CMA with DMA-mapping subsystem
Hi Marek, As informed to you in private over IRC, below piece of code broke during booting EXYNOS4:SMDKV310 with ZONE_DMA enabled. On 10/06/2011 07:24 PM, Marek Szyprowski wrote: ... diff --git a/arch/arm/mm/init.c b/arch/arm/mm/init.c index fbdd12e..9c27fbd 100644 --- a/arch/arm/mm/init.c +++ b/arch/arm/mm/init.c @@ -21,6 +21,7 @@ #include #include #include +#include #include #include @@ -371,6 +372,13 @@ void __init arm_memblock_init(struct meminfo *mi, struct machine_desc *mdesc) if (mdesc->reserve) mdesc->reserve(); + /* reserve memory for DMA contigouos allocations */ +#ifdef CONFIG_ZONE_DMA + dma_contiguous_reserve(PHYS_OFFSET + mdesc->dma_zone_size - 1); +#else + dma_contiguous_reserve(0); +#endif + memblock_analyze(); memblock_dump_all(); } Regards, Subash -- 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 1/1] [media] MFC: Change MFC firmware binary name
Hello,
There is option in menu->"Device Drivers"->"Generic Driver
Options"->"External firmware blobs to build into the kernel binary".
I have used this many times instead of /lib/firmware mechanism. If
someone chooses to add firmware in that way, and gives different name,
then this code too can break. So I have proposed another way to solve
that. Have a look into this.
Regards,
Subash
On 09/30/2011 05:38 PM, Kamil Debski wrote:
Hi Sachin,
Thanks for the patch. I agree with you - MFC module could be used in other
SoCs as well.
From: Sachin Kamat [mailto:sachin.ka...@linaro.org]
Sent: 30 September 2011 12:56
This patches renames the MFC firmware binary to avoid SoC name in it.
Signed-off-by: Sachin Kamat
Acked-by: Kamil Debski
---
drivers/media/video/s5p-mfc/s5p_mfc_ctrl.c |4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/media/video/s5p-mfc/s5p_mfc_ctrl.c
b/drivers/media/video/s5p-mfc/s5p_mfc_ctrl.c
index 5f4da80..f2481a8 100644
--- a/drivers/media/video/s5p-mfc/s5p_mfc_ctrl.c
+++ b/drivers/media/video/s5p-mfc/s5p_mfc_ctrl.c
@@ -38,7 +38,7 @@ int s5p_mfc_alloc_and_load_firmware(struct s5p_mfc_dev
*dev)
* into kernel. */
mfc_debug_enter();
err = request_firmware((const struct firmware **)&fw_blob,
-"s5pc110-mfc.fw", dev->v4l2_dev.dev);
+"s5p-mfc.fw", dev->v4l2_dev.dev);
if (err != 0) {
mfc_err("Firmware is not present in the /lib/firmware directory
nor compiled in kernel\n");
return -EINVAL;
@@ -116,7 +116,7 @@ int s5p_mfc_reload_firmware(struct s5p_mfc_dev *dev)
* into kernel. */
mfc_debug_enter();
err = request_firmware((const struct firmware **)&fw_blob,
-"s5pc110-mfc.fw", dev->v4l2_dev.dev);
+"s5p-mfc.fw", dev->v4l2_dev.dev);
int s5p_mfc_alloc_and_load_firmware(struct s5p_mfc_dev *dev)
{
struct firmware *fw_blob;
size_t bank2_base_phys;
void *b_base;
int err;
/* default name */
char firmware_name[30] = "s5p-mfc.fw";
/* Firmare has to be present as a separate file or compiled
* into kernel. */
mfc_debug_enter();
#ifdef CONFIG_EXTRA_FIRMWARE
snprintf(firmware_name, sizeof(firmware_name), "%s",
CONFIG_EXTRA_FIRMWARE);
#endif
err = request_firmware((const struct firmware **)&fw_blob,
firmware_name, dev->v4l2_dev.dev);
if (err != 0) {
mfc_err("Firmware is not present in the /lib/firmware
directory nor compiled in kernel\n");
return -EINVAL;
}
if (err != 0) {
mfc_err("Firmware is not present in the /lib/firmware directory
nor compiled in kernel\n");
return -EINVAL;
--
1.7.4.1
Best regards,
--
Kamil Debski
Linux Platform Group
Samsung Poland R&D Center
--
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
--
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 1/3] v4l: Extend V4L2_CID_COLORFX control with AQUA effect
Hi Laurent,
I am not representing Sylwester :), But with a similar sensor I use,
Aqua means cool tone which is Cb/Cr manipulations.
Regards,
Subash
On 09/19/2011 04:38 AM, Laurent Pinchart wrote:
Hi Sylwester,
Thanks for the patch.
On Friday 16 September 2011 19:05:28 Sylwester Nawrocki wrote:
Add V4L2_COLORFX_AQUA image effect in the V4L2_CID_COLORFX menu.
What's the aqua effect ?
Signed-off-by: Sylwester Nawrocki
Signed-off-by: Kyungmin Park
---
Documentation/DocBook/media/v4l/controls.xml |5 +++--
include/linux/videodev2.h|1 +
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/Documentation/DocBook/media/v4l/controls.xml
b/Documentation/DocBook/media/v4l/controls.xml index 8516401..f3c6457
100644
--- a/Documentation/DocBook/media/v4l/controls.xml
+++ b/Documentation/DocBook/media/v4l/controls.xml
@@ -294,8 +294,9 @@ minimum value disables backlight compensation.
V4L2_COLORFX_SKETCH (5),
V4L2_COLORFX_SKY_BLUE (6),
V4L2_COLORFX_GRASS_GREEN (7),
-V4L2_COLORFX_SKIN_WHITEN (8) and
-V4L2_COLORFX_VIVID (9).
+V4L2_COLORFX_SKIN_WHITEN (8),
+V4L2_COLORFX_VIVID (9) and
+V4L2_COLORFX_AQUA (10).
V4L2_CID_ROTATE
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index fca24cc..5032226 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1144,6 +1144,7 @@ enum v4l2_colorfx {
V4L2_COLORFX_GRASS_GREEN = 7,
V4L2_COLORFX_SKIN_WHITEN = 8,
V4L2_COLORFX_VIVID = 9,
+ V4L2_COLORFX_AQUA = 10,
};
#define V4L2_CID_AUTOBRIGHTNESS (V4L2_CID_BASE+32)
#define V4L2_CID_BAND_STOP_FILTER (V4L2_CID_BASE+33)
--
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: [RFC] New class for low level sensors controls?
Hi Sakari, On 09/06/2011 05:52 PM, Sakari Ailus wrote: On Tue, Sep 06, 2011 at 01:41:11PM +0200, Laurent Pinchart wrote: Hi Sakari, On Tuesday 06 September 2011 13:36:53 Sakari Ailus wrote: Hi all, We are beginning to have raw bayer image sensor drivers in the mainline. Typically such sensors are not controlled by general purpose applications but e.g. require a camera control algorithm framework in user space. This needs to be implemented in libv4l for general purpose applications to work properly on this kind of hardware. These sensors expose controls such as - Per-component gain controls. Red, blue, green (blue) and green (red) gains. - Link frequency. The frequency of the data link from the sensor to the bridge. - Horizontal and vertical blanking. Other controls often found in bayer sensors are black level compensation and test pattern. Does all BAYER sensor allow the dark level compensation programming? I thought it must be auto dark level compensation, which is done by the sensor. The sensor detects the optical black value at start of each frame, and analog-to-digital conversion is shifted to compensate the dark level for that frame. Hence I am thinking if this should be a controllable feature. None of these controls are suitable for use of general purpose applications (let alone the end user!) but for the camera control algorithms. We have a control class called V4L2_CTRL_CLASS_CAMERA for camera controls. However, the controls in this class are relatively high level controls which are suitable for end user. The algorithms in the libv4l or a webcam could implement many of these controls whereas I see that only V4L2_CID_EXPOSURE_ABSOLUTE might be implemented by raw bayer sensors. My question is: would it make sense to create a new class of controls for the low level sensor controls in a similar fashion we have a control class for the flash controls? I think it would, but I'm not sure how we should name that class. V4L2_CTRL_CLASS_SENSOR is tempting, but many of the controls that will be found there (digital gains, black leverl compensation, test pattern, ...) can also be found in ISPs or other hardware blocks. I don't think ISPs typically implement test patterns. Do you know of any? I know atleast two sensors (ov5642 and s5k4bafx) which have inbuilt ISP, programmed through i2c. They both have test patten generator. I think RAW(BAYER) sensors themselves cannot generate a test pattern without some h/w entity to convert RGGB into color bars in RGB or YUV. Should we separate controls which clearly apply to sensors only from the rest? For sensors only: - Analog gain(s) - Horizontal and vertical blanking - Link frequency - Test pattern Where should the shutter operation be listed into? Also type (rolling, global) and method (manual, electronic) of shutter operation? The following can be implemented also on ISPs: - Per-component gains - Black level compensation Do we have more to add to the list? If we keep the two the same class, I could propose the following names: V4L2_CTRL_CLASS_LL_CAMERA (for low level camera) Instead of LL_CAMERA, wouldnt something like CAM_SENSOR_ARRAY would be more meaningful? We control the sensor array properties in this level. V4L2_CTRL_CLASS_SOURCE V4L2_CTRL_CLASS_IMAGE_SOURCE The last one would be a good name for the sensor control class, as far as I understand some are using tuners with the OMAP 3 ISP these days. For the another one, I propose V4L2_CTRL_CLASS_ISP. Better names are always welcome. :-) Regards, Subash -- 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: [ANN] Meeting minutes of the Cambourne meeting
Hi Sakari, I have a point with the pixel clock. During discussion we found that pixel clock get/set is required for user space to do fine control over the frame-rate etc. What if the user sets the pixel array clock which is above the system/if bus clock? Suppose we are setting the pixel clock (which user space sets) to higher rate at sensor array, but for some reason the bus cannot handle that rate (either low speed or loaded) or lower PCLK at say CSI2 interface is being set. Are we not going to loose data due to this? Also, there would be data validation overhead in driver on what is acceptable PCLK values for a particular sensor on an interface etc. I am still not favoring user space controlling this, and wish driver decides this for a given frame-rate requested by the user space :) Frame-rate resolution HSYNC VSYNC PCLK(array) PCLK (i/f bus) ... Let user space control only first two, and driver decide rest (PCLK can be different at different ISP h/w units though) Regards, Subash > Pixel clock and blanking > > > Preliminary conclusions: > > - Pixel clock(s) and blanking will be exported through controls on subdev > nodes. > - The pixel array pixel clock is needed by userspace. > - Hosts/bridges/ISPs need pixel clock and blanking information to validate > pipelines. > > Actions: > > - CSI2 and CCP2 bus frequencies will be selectable use integer menu controls. > (Sakari) > - Add an integer menu control type, replacing the name with a 64-bit integer. > (Sakari, Hans) > - Research which pixel clock(s) to expose based on the SMIA sensor. > (Sakari) > - Add two new internal subdev pad operations to get and set clocks and > blanking. > (Laurent, Sakari) -- 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 08/19] s5p-fimc: Add the media device driver
Hi Sylwester,
On 07/22/2011 02:29 PM, Sylwester Nawrocki wrote:
Hi Subash,
On 07/22/2011 08:26 AM, Subash Patel wrote:
Hi Sylwester,
On 07/04/2011 11:24 PM, Sylwester Nawrocki wrote:
The fimc media device driver is hooked onto "s5p-fimc-md" platform device.
Such a platform device need to be added in a board initialization code
and then camera sensors need to be specified as it's platform data.
The "s5p-fimc-md" device is a top level entity for all FIMC, mipi-csis
and sensor devices.
Signed-off-by: Sylwester Nawrocki
Signed-off-by: Kyungmin Park
---
drivers/media/video/Kconfig | 2 +-
drivers/media/video/s5p-fimc/Makefile | 2 +-
...
/* -*/
/* fimc-capture.c */
diff --git a/drivers/media/video/s5p-fimc/fimc-mdevice.c
b/drivers/media/video/s5p-fimc/fimc-mdevice.c
new file mode 100644
index 000..10c8d5d
--- /dev/null
+++ b/drivers/media/video/s5p-fimc/fimc-mdevice.c
@@ -0,0 +1,804 @@
...
+
+static int fimc_md_register_sensor_entities(struct fimc_md *fmd)
+{
+ struct s5p_platform_fimc *pdata = fmd->pdev->dev.platform_data;
+ struct fimc_dev *fd = NULL;
+ int num_clients, ret, i;
+
+ /*
+ * Runtime resume one of the FIMC entities to make sure
+ * the sclk_cam clocks are not globally disabled.
+ */
+ for (i = 0; !fd&& i< ARRAY_SIZE(fmd->fimc); i++)
+ if (fmd->fimc[i])
+ fd = fmd->fimc[i];
+ if (!fd)
+ return -ENXIO;
+ ret = pm_runtime_get_sync(&fd->pdev->dev);
+ if (ret< 0)
+ return ret;
+
+ WARN_ON(pdata->num_clients> ARRAY_SIZE(fmd->sensor));
+ num_clients = min_t(u32, pdata->num_clients, ARRAY_SIZE(fmd->sensor));
+
+ fmd->num_sensors = num_clients;
+ for (i = 0; i< num_clients; i++) {
+ fmd->sensor[i].pdata =&pdata->isp_info[i];
+ ret = __fimc_md_set_camclk(fmd,&fmd->sensor[i], true);
+ if (ret)
+ break;
+ fmd->sensor[i].subdev =
+ fimc_md_register_sensor(fmd,&fmd->sensor[i]);
There is an issue here. Function fimc_md_register_sensor(),
can return subdev, as also error codes when i2c_get_adapter()
or NULL when v4l2_i2c_new_subdev_board() fail. But we do not
invalidate, and assume the return value is valid subdev. It
will cause kernel NULL pointer exception later in fimc_md_create_links().
Thanks for letting know.
I remember fixing this issue in v2 of the patch set by making
fimc_md_register_sensor() return NULL on any error, also when
i2c_get_adapter() fails, rather than ERR_PTR value.
Do you really mean that there is a NULL or _invalid_ pointer
dereference in fimc_md_create_links() ?
An oops on a NULL subdev pointer in fmd->sensor[] array seems
impossible as there is a check at the beginning of the loop:
If you return NULL, then this check will block a crash. In my case, I
failed to get the i2c_adapter, and ENODEV was returned, which is not
NULL. Hence I pass through this check, and will crash in
s_info = v4l2_get_subdev_hostdata(sensor);
I dont have access to your new patch-set. But if you have returned NULL,
then thats should fix this.
+static int fimc_md_create_links(struct fimc_md *fmd)
+{
+ struct v4l2_subdev *sensor, *csis;
+ struct s5p_fimc_isp_info *pdata;
+ struct fimc_sensor_info *s_info;
+ struct media_entity *source;
+ int fimc_id = 0;
+ int i, pad;
+ int rc = 0;
+
+ for (i = 0; i< fmd->num_sensors; i++) {
+ if (fmd->sensor[i].subdev == NULL)
+ continue;
...
Sorry, I'll be able to only make more test of this on Monday.
--
Thanks,
Sylwester
Regards,
Subash
SISO-SLG
--
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 08/19] s5p-fimc: Add the media device driver
Hi Sylwester,
On 07/04/2011 11:24 PM, Sylwester Nawrocki wrote:
The fimc media device driver is hooked onto "s5p-fimc-md" platform device.
Such a platform device need to be added in a board initialization code
and then camera sensors need to be specified as it's platform data.
The "s5p-fimc-md" device is a top level entity for all FIMC, mipi-csis
and sensor devices.
Signed-off-by: Sylwester Nawrocki
Signed-off-by: Kyungmin Park
---
drivers/media/video/Kconfig |2 +-
drivers/media/video/s5p-fimc/Makefile |2 +-
...
/* -*/
/* fimc-capture.c */
diff --git a/drivers/media/video/s5p-fimc/fimc-mdevice.c
b/drivers/media/video/s5p-fimc/fimc-mdevice.c
new file mode 100644
index 000..10c8d5d
--- /dev/null
+++ b/drivers/media/video/s5p-fimc/fimc-mdevice.c
@@ -0,0 +1,804 @@
...
+
+static int fimc_md_register_sensor_entities(struct fimc_md *fmd)
+{
+ struct s5p_platform_fimc *pdata = fmd->pdev->dev.platform_data;
+ struct fimc_dev *fd = NULL;
+ int num_clients, ret, i;
+
+ /*
+* Runtime resume one of the FIMC entities to make sure
+* the sclk_cam clocks are not globally disabled.
+*/
+ for (i = 0; !fd&& i< ARRAY_SIZE(fmd->fimc); i++)
+ if (fmd->fimc[i])
+ fd = fmd->fimc[i];
+ if (!fd)
+ return -ENXIO;
+ ret = pm_runtime_get_sync(&fd->pdev->dev);
+ if (ret< 0)
+ return ret;
+
+ WARN_ON(pdata->num_clients> ARRAY_SIZE(fmd->sensor));
+ num_clients = min_t(u32, pdata->num_clients, ARRAY_SIZE(fmd->sensor));
+
+ fmd->num_sensors = num_clients;
+ for (i = 0; i< num_clients; i++) {
+ fmd->sensor[i].pdata =&pdata->isp_info[i];
+ ret = __fimc_md_set_camclk(fmd,&fmd->sensor[i], true);
+ if (ret)
+ break;
+ fmd->sensor[i].subdev =
+ fimc_md_register_sensor(fmd,&fmd->sensor[i]);
There is an issue here. Function fimc_md_register_sensor(), can return
subdev, as also error codes when i2c_get_adapter() or NULL when
v4l2_i2c_new_subdev_board() fail. But we do not invalidate, and assume
the return value is valid subdev. It will cause kernel NULL pointer
exception later in fimc_md_create_links().
+ ret = __fimc_md_set_camclk(fmd,&fmd->sensor[i], false);
+ if (ret)
+ break;
+ }
+ pm_runtime_put(&fd->pdev->dev);
+ return ret;
+}
+
+/*
+ * MIPI CSIS and FIMC platform devices registration.
+ */
Regards,
Subash
SISO-SLG
--
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 4/4 v9] MFC: Add MFC 5.1 V4L2 driver
Hi Kamil,
I went through the decoder flow (not codec specific yet) of your MFC
driver. Since I havent acquired/produced a v4l2 based test case, I didnt
check the functionality yet. Below are some of minor comments I found
during my review.
Regards,
Subash
SISO-SLG
On 06/14/2011 10:06 PM, Kamil Debski wrote:
...
diff --git a/drivers/media/video/s5p-mfc/s5p_mfc.c
b/drivers/media/video/s5p-mfc/s5p_mfc.c
new file mode 100644
...
+/* Open an MFC node */
+static int s5p_mfc_open(struct file *file)
+{
+ struct s5p_mfc_ctx *ctx = NULL;
+ struct s5p_mfc_dev *dev = video_drvdata(file);
+ struct vb2_queue *q;
+ unsigned long flags;
+ int ret = 0;
+
+ mfc_debug_enter();
+
+ dev->num_inst++; /* It is guarded by mfc_mutex in vfd */
+
+ /* Allocate memory for context */
+ ctx = kzalloc(sizeof *ctx, GFP_KERNEL);
+ if (!ctx) {
+ mfc_err("Not enough memory.\n");
+ ret = -ENOMEM;
+ goto err_alloc;
+ }
+
+ ret = v4l2_fh_init(&ctx->fh, video_devdata(file));
+ if (ret) {
+ mfc_err("Failed to init v4l2_fh\n");
+ goto err_fh;
+ }
+
+ file->private_data =&ctx->fh;
+ v4l2_fh_add(&ctx->fh);
+
+ ctx->dev = dev;
+ INIT_LIST_HEAD(&ctx->src_queue);
+ INIT_LIST_HEAD(&ctx->dst_queue);
+ ctx->src_queue_cnt = 0;
+ ctx->dst_queue_cnt = 0;
+ /* Get context number */
+ ctx->num = 0;
+ while (dev->ctx[ctx->num]) {
+ ctx->num++;
+ if (ctx->num>= MFC_NUM_CONTEXTS) {
+ mfc_err("Too many open contexts.\n");
+ ret = -EBUSY;
+ goto err_no_ctx;
+ }
+ }
+ /* Mark context as idle */
+ spin_lock_irqsave(&dev->condlock, flags);
+ clear_bit(ctx->num,&dev->ctx_work_bits);
+ spin_unlock_irqrestore(&dev->condlock, flags);
+ dev->ctx[ctx->num] = ctx;
+ if (s5p_mfc_get_node_type(file) == MFCNODE_DECODER) {
+ ctx->type = MFCINST_DECODER;
+ ctx->c_ops = get_dec_codec_ops();
+ /* Setup ctrl handler */
+ ret = s5p_mfc_dec_ctrls_setup(ctx);
+ if (ret) {
+ mfc_err("Failed to setup mfc controls\n");
+ goto err_ctrls_setup;
+ }
+
+
+ } else if (s5p_mfc_get_node_type(file) == MFCNODE_ENCODER) {
+ ctx->type = MFCINST_ENCODER;
+ ctx->c_ops = get_enc_codec_ops();
+ /* only for encoder */
+ INIT_LIST_HEAD(&ctx->ref_queue);
+ ctx->ref_queue_cnt = 0;
+
+ /* Setup ctrl handler */
+ ret = s5p_mfc_enc_ctrls_setup(ctx);
+ if (ret) {
+ mfc_err("Failed to setup mfc controls\n");
+ goto err_ctrls_setup;
+ }
+ } else {
+ ret = -ENOENT;
+ goto err_bad_node;
+ }
+
+ ctx->fh.ctrl_handler =&ctx->ctrl_handler;
+ ctx->inst_no = -1;
+ /* Load firmware if this is the first instance */
+ if (dev->num_inst == 1) {
+ dev->watchdog_timer.expires = jiffies +
+ msecs_to_jiffies(MFC_WATCHDOG_INTERVAL);
+ add_timer(&dev->watchdog_timer);
+
+ mfc_debug(2, "power on\n");
+ ret = s5p_mfc_power_on();
+ if (ret< 0) {
+ mfc_err("power on failed\n");
+ goto err_pwr_enable;
+ }
+
+ s5p_mfc_clock_on();
+
+ /* Load the FW */
+ ret = s5p_mfc_alloc_firmware(dev);
+ if (ret != 0)
+ goto err_alloc_fw;
+ ret = s5p_mfc_load_firmware(dev);
+ if (ret != 0)
+ goto err_load_fw;
+
+ /* Init the FW */
+ ret = s5p_mfc_init_hw(dev);
+ if (ret != 0)
+ goto err_init_hw;
+
+ s5p_mfc_clock_off();
+ }
+
+ /* Init videobuf2 queue for CAPTURE */
+ q =&ctx->vq_dst;
+ q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+ q->drv_priv =&ctx->fh;
+ if (s5p_mfc_get_node_type(file) == MFCNODE_DECODER) {
+ q->io_modes = VB2_MMAP;
+ q->ops = get_dec_queue_ops();
+ } else {
+ q->io_modes = VB2_MMAP | VB2_USERPTR;
+ q->ops = get_enc_queue_ops();
+ }
Node type is declared as one of MFCNODE_INVALID = -1,MFCNODE_DECODER =
0, MFCNODE_ENCODER = 1. Hence it would be good to check even for encoder
and return error for the invalid case.
+
+ q->mem_ops = (struct vb2_mem_ops *)&vb2_dma_contig_memops;
+ ret = vb2_queue_init(q);
+ if (ret) {
+ mfc_err("Failed to initialize videobuf2 queue(capture)\n");
+
Re: [PATCH 08/10] mm: cma: Contiguous Memory Allocator added
Hi Arnd, On 06/14/2011 09:33 PM, Arnd Bergmann wrote: On Tuesday 14 June 2011, Michal Nazarewicz wrote: On Tue, 14 Jun 2011 15:49:29 +0200, Arnd Bergmann wrote: Please explain the exact requirements that lead you to defining multiple contexts. Some devices may have access only to some banks of memory. Some devices may use different banks of memory for different purposes. For all I know, that is something that is only true for a few very special Samsung devices, and is completely unrelated of the need for contiguous allocations, so this approach becomes pointless as soon as the next generation of that chip grows an IOMMU, where we don't handle the special bank attributes. Also, the way I understood the situation for the Samsung SoC during the Budapest discussion, it's only a performance hack, not a functional requirement, unless you count '1080p playback' as a functional requirement. 1080p@30fps is indeed a functional requirement, as the IP has the capability to achieve it. This IP itself can act as more than one AXI master, and control more than one memory port(bank). So this is not a *performance hack* Also, if I recall, during the Budapest discussion (I was on irc), I recall that this requirement can become the information available to the actual allocator. Below is the summary point I could collect from summit notes: * May also need to specify more attributes (specific physical memory region) As per this point, the requirement (as above) must be attribute to the allocator, which is CMA in this case. Supporting contiguous allocation is a very useful goal and many people want this, but supporting a crazy one-off hardware design with lots of generic infrastructure is going a bit too far. If you can't be more specific than 'some devices may need this', I would suggest going forward without having multiple regions: * Remove the registration of specific addresses from the initial patch set (but keep the patch). * Add a heuristic plus command-line override to automatically come up with a reasonable location+size for *one* CMA area in the system. * Ship the patch to add support for multiple CMA areas with the BSP for the boards that need it (if any). * Wait for someone on a non-Samsung SoC to run into the same problem, then have /them/ get the final patch in. Even if you think you can convince enough people that having support for distinct predefined regions is a good idea, I would recommend splitting that out of the initial merge so we can have that discussion separately from the other issues. Arnd -- 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 Regards, Subash SISO-SLG -- 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: [Query] VIDIOC_QBUF and VIDIOC_STREAMON order
> Also, there might still be situations where being able to STREAMON > without buffers queued would be beneficial. For example, enabling the > device might be a slow/expensive operation and we might prefer to keep > it running even if we don't want any data at the moment. Even for > faster devices, being able to keep them on and periodically take a > snapshot would be faster without having to call STREAMON anyway... If we are speaking of devices like camera for phone/tabs, then this will have significant impact on the power consumption. > Suppose we forced QBUF to be done before STREAMON. This would work, > but what happens next? What should happen when we want to DQBUF the > last buffer? If the device couldn't start without any buffers queued, > can it continue streaming with all of them dequeued? I would guess > not. So we'd either have to deny DQBUF of the last buffer (which for > me personally is almost unacceptable) or have the last DQBUF > automatically cause a STREAMOFF. I think it depends on hardware's behavior on how it behaves without any buffers queued. Some hardwares do notify the overflow state (interrupting for each frame recieved) if there was no buffer queued. This will ensure even the last frame is DQUEUEd. Considering the scenario like preview/capture @ 30fps, if there is one frame loss too, it is acceptable. But that doesnt hold good for high-speed image capture. Regards, Subash On Tue, Mar 15, 2011 at 8:51 AM, Pawel Osciak wrote: > Hi, > > On Mon, Mar 14, 2011 at 03:49, Subash Patel wrote: >> VIDIOC_STREAMON expects buffers to be queued before hardware part of >> image/video pipe is enabled. From my experience of V4L2 user space, I >> have always QBUFfed before invoking the STREAMON. Below is the API >> specification which also speaks something same: >> > > Not exactly. It says that the API requires buffers to be queued for > output devices. It does not require any buffers to be queued for input > devices. Sylwester is right here. > > This feature of not having to queue input buffers before STREAMON > introduces problems to driver implementations and I am personally not > a big fan of it either. But I'm seeing some additional problems here. > Suppose we forced QBUF to be done before STREAMON. This would work, > but what happens next? What should happen when we want to DQBUF the > last buffer? If the device couldn't start without any buffers queued, > can it continue streaming with all of them dequeued? I would guess > not. So we'd either have to deny DQBUF of the last buffer (which for > me personally is almost unacceptable) or have the last DQBUF > automatically cause a STREAMOFF. So, for the latter, should > applications, after they get all the data they wanted, be made to > always have one more buffer queued as a "throwaway" buffer? This is > probably the only reasonable solution here, but the applications would > have to keep count of their queued buffers and be aware of this. > Also, there might still be situations where being able to STREAMON > without buffers queued would be beneficial. For example, enabling the > device might be a slow/expensive operation and we might prefer to keep > it running even if we don't want any data at the moment. Even for > faster devices, being able to keep them on and periodically take a > snapshot would be faster without having to call STREAMON anyway... > > -- > Best regards, > Pawel Osciak > -- 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: [Query] VIDIOC_QBUF and VIDIOC_STREAMON order
VIDIOC_STREAMON expects buffers to be queued before hardware part of image/video pipe is enabled. From my experience of V4L2 user space, I have always QBUFfed before invoking the STREAMON. Below is the API specification which also speaks something same: http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-streamon.html I think its better to return EINVAL if there are no queued buffers when VIDIOC_STREAMON is invoked. Regards, Subash On Mon, Mar 14, 2011 at 3:44 PM, Sylwester Nawrocki wrote: > Hello, > > As far as I know V4L2 applications are allowed to call VIDIOC_STREAMON before > queuing buffers with VIDIOC_QBUF. > > This leads to situation that a H/W is attempted to be enabled by the driver > when it has no any buffer ownership. > > Effectively actual activation of the data pipeline has to be deferred > until first buffer arrived in the driver. Which makes it difficult > to signal any errors to user during enabling the data pipeline. > > Is this allowed to force applications to queue some buffers before calling > STREAMON, by returning an error in vidioc_streamon from the driver, when > no buffers have been queued at this time? > > I suppose this could render some applications to stop working if this kind > of restriction is applied e.g. in camera capture driver. > > What the applications really expect? > > With the above I refer mostly to a snapshot mode where we have to be careful > not to lose any frame, as there could be only one.. > > > Please share you opinions. > > > Regards, > -- > Sylwester Nawrocki > Samsung Poland R&D Center > -- > 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 > -- 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
