diff mbox

V4L: soc-camera: Add support for custom host mmap

Message ID 1299545691-917-1-git-send-email-saaguirre@ti.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Aguirre Rodriguez, Sergio Alberto March 8, 2011, 12:54 a.m. UTC 
  This helps redirect mmap calls to custom memory managers which
already have preallocated space to use by the device.

Otherwise, device might not support the allocation attempted
generically by videobuf.

Signed-off-by: Sergio Aguirre <saaguirre@ti.com>
---
 drivers/media/video/soc_camera.c |    7 ++++++-
 include/media/soc_camera.h       |    2 ++
 2 files changed, 8 insertions(+), 1 deletions(-)

Comments

Guennadi Liakhovetski March 8, 2011, 7:17 a.m. UTC | #1 
Hi Sergio

On Mon, 7 Mar 2011, Sergio Aguirre wrote:

> This helps redirect mmap calls to custom memory managers which
> already have preallocated space to use by the device.
> 
> Otherwise, device might not support the allocation attempted
> generically by videobuf.
> 
> Signed-off-by: Sergio Aguirre <saaguirre@ti.com>
> ---
>  drivers/media/video/soc_camera.c |    7 ++++++-
>  include/media/soc_camera.h       |    2 ++
>  2 files changed, 8 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
> index 59dc71d..d361ba0 100644
> --- a/drivers/media/video/soc_camera.c
> +++ b/drivers/media/video/soc_camera.c
> @@ -512,6 +512,7 @@ static ssize_t soc_camera_read(struct file *file, char __user *buf,
>  static int soc_camera_mmap(struct file *file, struct vm_area_struct *vma)
>  {
>  	struct soc_camera_device *icd = file->private_data;
> +	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);

This doesn't seem to be needed

>  	int err;
>  
>  	dev_dbg(&icd->dev, "mmap called, vma=0x%08lx\n", (unsigned long)vma);
> @@ -519,7 +520,11 @@ static int soc_camera_mmap(struct file *file, struct vm_area_struct *vma)
>  	if (icd->streamer != file)
>  		return -EBUSY;
>  
> -	err = videobuf_mmap_mapper(&icd->vb_vidq, vma);
> +	/* Check for an interface custom mmaper */

mmapper - double 'p'

> +	if (ici->ops->mmap)
> +		err = ici->ops->mmap(&icd->vb_vidq, icd, vma);
> +	else
> +		err = videobuf_mmap_mapper(&icd->vb_vidq, vma);

You're patching an old version of soc-camera. Please use a current one 
with support for videobuf2. Further, wouldn't it be possible for you to 
just replace the videobuf mmap_mapper() (videobuf2 q->mem_ops->mmap()) 
method? I am not sure how possible this is, maybe one of videobuf2 experts 
could help us? BTW, you really should be using the videobuf2 API.

>  
>  	dev_dbg(&icd->dev, "vma start=0x%08lx, size=%ld, ret=%d\n",
>  		(unsigned long)vma->vm_start,
> diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
> index de81370..11350c2 100644
> --- a/include/media/soc_camera.h
> +++ b/include/media/soc_camera.h
> @@ -87,6 +87,8 @@ struct soc_camera_host_ops {
>  	int (*set_ctrl)(struct soc_camera_device *, struct v4l2_control *);
>  	int (*get_parm)(struct soc_camera_device *, struct v4l2_streamparm *);
>  	int (*set_parm)(struct soc_camera_device *, struct v4l2_streamparm *);
> +	int (*mmap)(struct videobuf_queue *, struct soc_camera_device *,
> +		     struct vm_area_struct *);
>  	unsigned int (*poll)(struct file *, poll_table *);
>  	const struct v4l2_queryctrl *controls;
>  	int num_controls;
> -- 
> 1.7.1
> 

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aguirre Rodriguez, Sergio Alberto March 8, 2011, 1:18 p.m. UTC | #2 
Hi Guennadi,

On 03/08/2011 01:17 AM, Guennadi Liakhovetski wrote:
> Hi Sergio
>
> On Mon, 7 Mar 2011, Sergio Aguirre wrote:
>
>> This helps redirect mmap calls to custom memory managers which
>> already have preallocated space to use by the device.
>>
>> Otherwise, device might not support the allocation attempted
>> generically by videobuf.
>>
>> Signed-off-by: Sergio Aguirre<saaguirre@ti.com>
>> ---
>>   drivers/media/video/soc_camera.c |    7 ++++++-
>>   include/media/soc_camera.h       |    2 ++
>>   2 files changed, 8 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
>> index 59dc71d..d361ba0 100644
>> --- a/drivers/media/video/soc_camera.c
>> +++ b/drivers/media/video/soc_camera.c
>> @@ -512,6 +512,7 @@ static ssize_t soc_camera_read(struct file *file, char __user *buf,
>>   static int soc_camera_mmap(struct file *file, struct vm_area_struct *vma)
>>   {
>>   	struct soc_camera_device *icd = file->private_data;
>> +	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
>
> This doesn't seem to be needed

It's needed to call the custom mmaper.

ici->ops->mmap

Otherwise, how can I access the soc camera host ops?

>
>>   	int err;
>>
>>   	dev_dbg(&icd->dev, "mmap called, vma=0x%08lx\n", (unsigned long)vma);
>> @@ -519,7 +520,11 @@ static int soc_camera_mmap(struct file *file, struct vm_area_struct *vma)
>>   	if (icd->streamer != file)
>>   		return -EBUSY;
>>
>> -	err = videobuf_mmap_mapper(&icd->vb_vidq, vma);
>> +	/* Check for an interface custom mmaper */
>
> mmapper - double 'p'

Oops. Will fix.

>
>> +	if (ici->ops->mmap)
>> +		err = ici->ops->mmap(&icd->vb_vidq, icd, vma);
>> +	else
>> +		err = videobuf_mmap_mapper(&icd->vb_vidq, vma);
>
> You're patching an old version of soc-camera. Please use a current one
> with support for videobuf2. Further, wouldn't it be possible for you to
> just replace the videobuf mmap_mapper() (videobuf2 q->mem_ops->mmap())
> method? I am not sure how possible this is, maybe one of videobuf2 experts
> could help us? BTW, you really should be using the videobuf2 API.

I'm basing this patches on mainline, commit:

commit 214d93b02c4fe93638ad268613c9702a81ed9192
Merge: ad4a4a8 077f8ec
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Mon Mar 7 13:15:02 2011 -0800

     Merge branch 'omap-fixes-for-linus' of 
git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap-2.6

And i don't see videobuf2 there.

Should I rebase my patches on another tree?

Regards,
Sergio


>
>>
>>   	dev_dbg(&icd->dev, "vma start=0x%08lx, size=%ld, ret=%d\n",
>>   		(unsigned long)vma->vm_start,
>> diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
>> index de81370..11350c2 100644
>> --- a/include/media/soc_camera.h
>> +++ b/include/media/soc_camera.h
>> @@ -87,6 +87,8 @@ struct soc_camera_host_ops {
>>   	int (*set_ctrl)(struct soc_camera_device *, struct v4l2_control *);
>>   	int (*get_parm)(struct soc_camera_device *, struct v4l2_streamparm *);
>>   	int (*set_parm)(struct soc_camera_device *, struct v4l2_streamparm *);
>> +	int (*mmap)(struct videobuf_queue *, struct soc_camera_device *,
>> +		     struct vm_area_struct *);
>>   	unsigned int (*poll)(struct file *, poll_table *);
>>   	const struct v4l2_queryctrl *controls;
>>   	int num_controls;
>> --
>> 1.7.1
>>
>
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guennadi Liakhovetski March 8, 2011, 1:45 p.m. UTC | #3 
On Tue, 8 Mar 2011, Sergio Aguirre wrote:

> Hi Guennadi,
> 
> On 03/08/2011 01:17 AM, Guennadi Liakhovetski wrote:
> > Hi Sergio
> > 
> > On Mon, 7 Mar 2011, Sergio Aguirre wrote:
> > 
> > > This helps redirect mmap calls to custom memory managers which
> > > already have preallocated space to use by the device.
> > > 
> > > Otherwise, device might not support the allocation attempted
> > > generically by videobuf.
> > > 
> > > Signed-off-by: Sergio Aguirre<saaguirre@ti.com>
> > > ---
> > >   drivers/media/video/soc_camera.c |    7 ++++++-
> > >   include/media/soc_camera.h       |    2 ++
> > >   2 files changed, 8 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/media/video/soc_camera.c
> > > b/drivers/media/video/soc_camera.c
> > > index 59dc71d..d361ba0 100644
> > > --- a/drivers/media/video/soc_camera.c
> > > +++ b/drivers/media/video/soc_camera.c
> > > @@ -512,6 +512,7 @@ static ssize_t soc_camera_read(struct file *file, char
> > > __user *buf,
> > >   static int soc_camera_mmap(struct file *file, struct vm_area_struct
> > > *vma)
> > >   {
> > >   	struct soc_camera_device *icd = file->private_data;
> > > +	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
> > 
> > This doesn't seem to be needed
> 
> It's needed to call the custom mmaper.
> 
> ici->ops->mmap

Oops, sorry, diff context has confused me "@@ -512,6 +512,7 @@ static 
ssize_t soc_camera_read(";-)

> 
> Otherwise, how can I access the soc camera host ops?
> 
> > 
> > >   	int err;
> > > 
> > >   	dev_dbg(&icd->dev, "mmap called, vma=0x%08lx\n", (unsigned long)vma);
> > > @@ -519,7 +520,11 @@ static int soc_camera_mmap(struct file *file, struct
> > > vm_area_struct *vma)
> > >   	if (icd->streamer != file)
> > >   		return -EBUSY;
> > > 
> > > -	err = videobuf_mmap_mapper(&icd->vb_vidq, vma);
> > > +	/* Check for an interface custom mmaper */
> > 
> > mmapper - double 'p'
> 
> Oops. Will fix.
> 
> > 
> > > +	if (ici->ops->mmap)
> > > +		err = ici->ops->mmap(&icd->vb_vidq, icd, vma);
> > > +	else
> > > +		err = videobuf_mmap_mapper(&icd->vb_vidq, vma);
> > 
> > You're patching an old version of soc-camera. Please use a current one
> > with support for videobuf2. Further, wouldn't it be possible for you to
> > just replace the videobuf mmap_mapper() (videobuf2 q->mem_ops->mmap())
> > method? I am not sure how possible this is, maybe one of videobuf2 experts
> > could help us? BTW, you really should be using the videobuf2 API.
> 
> I'm basing this patches on mainline, commit:
> 
> commit 214d93b02c4fe93638ad268613c9702a81ed9192
> Merge: ad4a4a8 077f8ec
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date:   Mon Mar 7 13:15:02 2011 -0800
> 
>     Merge branch 'omap-fixes-for-linus' of
> git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap-2.6
> 
> And i don't see videobuf2 there.
> 
> Should I rebase my patches on another tree?

Yes, please use

git://linuxtv.org/media_tree.git	staging/for_v2.6.39

otherwise you can also base on linux-next if you like.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Aguirre Rodriguez, Sergio Alberto March 11, 2011, 4:47 p.m. UTC | #4 
On 03/08/2011 07:45 AM, Guennadi Liakhovetski wrote:
> On Tue, 8 Mar 2011, Sergio Aguirre wrote:
>
>> Hi Guennadi,
>>
>> On 03/08/2011 01:17 AM, Guennadi Liakhovetski wrote:
>>> Hi Sergio
>>>
>>> On Mon, 7 Mar 2011, Sergio Aguirre wrote:
>>>
>>>> This helps redirect mmap calls to custom memory managers which
>>>> already have preallocated space to use by the device.
>>>>
>>>> Otherwise, device might not support the allocation attempted
>>>> generically by videobuf.
>>>>
>>>> Signed-off-by: Sergio Aguirre<saaguirre@ti.com>
>>>> ---
>>>>    drivers/media/video/soc_camera.c |    7 ++++++-
>>>>    include/media/soc_camera.h       |    2 ++
>>>>    2 files changed, 8 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/drivers/media/video/soc_camera.c
>>>> b/drivers/media/video/soc_camera.c
>>>> index 59dc71d..d361ba0 100644
>>>> --- a/drivers/media/video/soc_camera.c
>>>> +++ b/drivers/media/video/soc_camera.c
>>>> @@ -512,6 +512,7 @@ static ssize_t soc_camera_read(struct file *file, char
>>>> __user *buf,
>>>>    static int soc_camera_mmap(struct file *file, struct vm_area_struct
>>>> *vma)
>>>>    {
>>>>    	struct soc_camera_device *icd = file->private_data;
>>>> +	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
>>>
>>> This doesn't seem to be needed
>>
>> It's needed to call the custom mmaper.
>>
>> ici->ops->mmap
>
> Oops, sorry, diff context has confused me "@@ -512,6 +512,7 @@ static
> ssize_t soc_camera_read(";-)

No worries. :)

>
>>
>> Otherwise, how can I access the soc camera host ops?
>>
>>>
>>>>    	int err;
>>>>
>>>>    	dev_dbg(&icd->dev, "mmap called, vma=0x%08lx\n", (unsigned long)vma);
>>>> @@ -519,7 +520,11 @@ static int soc_camera_mmap(struct file *file, struct
>>>> vm_area_struct *vma)
>>>>    	if (icd->streamer != file)
>>>>    		return -EBUSY;
>>>>
>>>> -	err = videobuf_mmap_mapper(&icd->vb_vidq, vma);
>>>> +	/* Check for an interface custom mmaper */
>>>
>>> mmapper - double 'p'
>>
>> Oops. Will fix.
>>
>>>
>>>> +	if (ici->ops->mmap)
>>>> +		err = ici->ops->mmap(&icd->vb_vidq, icd, vma);
>>>> +	else
>>>> +		err = videobuf_mmap_mapper(&icd->vb_vidq, vma);
>>>
>>> You're patching an old version of soc-camera. Please use a current one
>>> with support for videobuf2. Further, wouldn't it be possible for you to
>>> just replace the videobuf mmap_mapper() (videobuf2 q->mem_ops->mmap())
>>> method? I am not sure how possible this is, maybe one of videobuf2 experts
>>> could help us? BTW, you really should be using the videobuf2 API.
>>
>> I'm basing this patches on mainline, commit:
>>
>> commit 214d93b02c4fe93638ad268613c9702a81ed9192
>> Merge: ad4a4a8 077f8ec
>> Author: Linus Torvalds<torvalds@linux-foundation.org>
>> Date:   Mon Mar 7 13:15:02 2011 -0800
>>
>>      Merge branch 'omap-fixes-for-linus' of
>> git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap-2.6
>>
>> And i don't see videobuf2 there.
>>
>> Should I rebase my patches on another tree?
>
> Yes, please use
>
> git://linuxtv.org/media_tree.git	staging/for_v2.6.39

Ok, I'll do this later on. I'm currently developing my driver on an 
internal TI tree, based on 2.6.35.6 Android kernel, and rebasing just 
generic v4l2 changes to send for review.

I still need to try this driver out in mainline for the Pandaboard, and 
then I'll rebase everything on top of videobuf2 framework.

Bottom line: Holding this patch for now :)

Thanks for your time!

Sergio

>
> otherwise you can also base on linux-next if you like.
>
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guennadi Liakhovetski March 11, 2011, 4:51 p.m. UTC | #5 
On Tue, 8 Mar 2011, Sergio Aguirre wrote:

> Hi Guennadi,
> 
> On 03/08/2011 01:17 AM, Guennadi Liakhovetski wrote:
> > Hi Sergio
> > 
> > On Mon, 7 Mar 2011, Sergio Aguirre wrote:
> > 
> > > This helps redirect mmap calls to custom memory managers which
> > > already have preallocated space to use by the device.
> > > 
> > > Otherwise, device might not support the allocation attempted
> > > generically by videobuf.
> > > 
> > > Signed-off-by: Sergio Aguirre<saaguirre@ti.com>
> > > ---
> > >   drivers/media/video/soc_camera.c |    7 ++++++-
> > >   include/media/soc_camera.h       |    2 ++
> > >   2 files changed, 8 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/media/video/soc_camera.c
> > > b/drivers/media/video/soc_camera.c
> > > index 59dc71d..d361ba0 100644
> > > --- a/drivers/media/video/soc_camera.c
> > > +++ b/drivers/media/video/soc_camera.c

[snip]

> > > +	if (ici->ops->mmap)
> > > +		err = ici->ops->mmap(&icd->vb_vidq, icd, vma);
> > > +	else
> > > +		err = videobuf_mmap_mapper(&icd->vb_vidq, vma);
> > 
> > You're patching an old version of soc-camera. Please use a current one
> > with support for videobuf2. Further, wouldn't it be possible for you to
> > just replace the videobuf mmap_mapper() (videobuf2 q->mem_ops->mmap())
> > method? I am not sure how possible this is, maybe one of videobuf2 experts
> > could help us? BTW, you really should be using the videobuf2 API.

I looked a bit more at videobuf2. Wouldn't it satisfy your needs if you 
just provide an own struct vb2_mem_ops, copy all its fields from your 
required memory allocator, and only replace the .mmap method? Please, try, 
if this would work for you. Then you won't need any changes to 
soc_camera.c

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/media/video/soc_camera.c b/drivers/media/video/soc_camera.c
index 59dc71d..d361ba0 100644
--- a/drivers/media/video/soc_camera.c
+++ b/drivers/media/video/soc_camera.c
@@ -512,6 +512,7 @@  static ssize_t soc_camera_read(struct file *file, char __user *buf,
 static int soc_camera_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	struct soc_camera_device *icd = file->private_data;
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
 	int err;
 
 	dev_dbg(&icd->dev, "mmap called, vma=0x%08lx\n", (unsigned long)vma);
@@ -519,7 +520,11 @@  static int soc_camera_mmap(struct file *file, struct vm_area_struct *vma)
 	if (icd->streamer != file)
 		return -EBUSY;
 
-	err = videobuf_mmap_mapper(&icd->vb_vidq, vma);
+	/* Check for an interface custom mmaper */
+	if (ici->ops->mmap)
+		err = ici->ops->mmap(&icd->vb_vidq, icd, vma);
+	else
+		err = videobuf_mmap_mapper(&icd->vb_vidq, vma);
 
 	dev_dbg(&icd->dev, "vma start=0x%08lx, size=%ld, ret=%d\n",
 		(unsigned long)vma->vm_start,
diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
index de81370..11350c2 100644
--- a/include/media/soc_camera.h
+++ b/include/media/soc_camera.h
@@ -87,6 +87,8 @@  struct soc_camera_host_ops {
 	int (*set_ctrl)(struct soc_camera_device *, struct v4l2_control *);
 	int (*get_parm)(struct soc_camera_device *, struct v4l2_streamparm *);
 	int (*set_parm)(struct soc_camera_device *, struct v4l2_streamparm *);
+	int (*mmap)(struct videobuf_queue *, struct soc_camera_device *,
+		     struct vm_area_struct *);
 	unsigned int (*poll)(struct file *, poll_table *);
 	const struct v4l2_queryctrl *controls;
 	int num_controls;