RE: [PATCH - v2 4/6] V4L - vpfe_capture - bug fixes and enhancements
Hans, If you are okay with this patch, could you please merge this to your -hg tree and send a pull request to Mauro to merge to the linux-next tree? This depends on the previous patch set which is waiting for Mauro's merge. Murali Karicheri Software Design Engineer Texas Instruments Inc. Germantown, MD 20874 phone: 301-407-9583 email: m-kariche...@ti.com >-Original Message- >From: Karicheri, Muralidharan >Sent: Wednesday, December 23, 2009 10:26 AM >To: 'Hans Verkuil' >Cc: linux-media@vger.kernel.org; khil...@deeprootsystems.com; Hiremath, >Vaibhav; Nori, Sekhar; davinci-linux-open-sou...@linux.davincidsp.com >Subject: RE: [PATCH - v2 4/6] V4L - vpfe_capture - bug fixes and >enhancements > >Hans, > >The change is because of the void * type that we use. Since ccdc parameter >structures are different for different IPs, a constant type for this arg >is not possible. The ccdc driver needs the pointer to structure. But the >v4l2 core tries to copies 4 bytes of data from the void * pointed location >which is not what we want. I am sure this code will change once we have a >device node available for ccdc on which case, this ioctl will be handled by >the ccdc sub device node. The long term goal is to convert ccdc/isif >drivers >to sub device and pass this user ioctl to that sub device node. But >currently we don't have support for device nodes for sub devices. I think >that is needed for this conversion to be complete. > >>BTW, does this patch series rely on the patches in my v4l-dvb-davinci >tree? >>Or are these independent patches? > >Yes, this is dependent on my earlier patch. I had asked Mauro to merge that >patch to linux-next, but still waiting > >Murali Karicheri >Software Design Engineer >Texas Instruments Inc. >Germantown, MD 20874 >phone: 301-407-9583 >email: m-kariche...@ti.com > >>-Original Message- >>From: Hans Verkuil [mailto:hverk...@xs4all.nl] >>Sent: Wednesday, December 23, 2009 9:24 AM >>To: Karicheri, Muralidharan >>Cc: linux-media@vger.kernel.org; khil...@deeprootsystems.com; Hiremath, >>Vaibhav; Nori, Sekhar; davinci-linux-open-sou...@linux.davincidsp.com >>Subject: Re: [PATCH - v2 4/6] V4L - vpfe_capture - bug fixes and >>enhancements >> >>Hi Murali, >> >>Sorry for the long delay in reviewing this patch series. I've been very >>busy, >>first at work, and now for Christmas preparations (and occasionally I'd >>like >>to relax as well :-) ). >> >>I'm OK with the other patches in this series, but I do have a few comments >>on this one: I noticed that you added a wrapper function for video_ioctl2. >>I don't quite understand why. >> >>BTW, does this patch series rely on the patches in my v4l-dvb-davinci >tree? >>Or are these independent patches? Is it because the experimental >>VPFE_CMD_S/G_CCDC_RAW_PARAMS ioctls are used with different argument >>pointers? >>I mean, currently the arg is a void * instead of a properly typed argument. >> >>However, if it always uses the same type, then you should use that type in >>_IOR/_IOW and use video_ioctl2 directly so that the core framework will do >>the >>user-to-kernelspace conversion (and vv) for you. >> >>Regards, >> >> Hans >> >>On Saturday 19 December 2009 00:58:25 m-kariche...@ti.com wrote: >>> From: Muralidharan Karicheri >>> >>> Updated based on comments against v1 of the patch >>> >>> Added a experimental IOCTL, to read the CCDC parameters. >>> Default handler was not getting the original pointer from the core. >>> So a wrapper function added to handle the default handler properly. >>> Also fixed a bug in the probe() in the linux-next tree >>> >>> Reviewed-by: Hans Verkuil >>> Signed-off-by: Muralidharan Karicheri >>> --- >>> Applies to linux-next of v4l-dvb >>> drivers/media/video/davinci/vpfe_capture.c | 120 + >- >>-- >>> include/media/davinci/vpfe_capture.h | 12 ++- >>> 2 files changed, 81 insertions(+), 51 deletions(-) >>> >>> diff --git a/drivers/media/video/davinci/vpfe_capture.c >>b/drivers/media/video/davinci/vpfe_capture.c >>> index 9e3a531..99ffc62 100644 >>> --- a/drivers/media/video/davinci/vpfe_capture.c >>> +++ b/drivers/media/video/davinci/vpfe_capture.c >>> @@ -758,12 +758,83 @@ static unsigned int vpfe_poll(struct file *file, >>poll_table *wait) >>> return 0; >>> } >>> >>> +static long vpfe_param_handler(struct file *file, voi
RE: [PATCH - v2 4/6] V4L - vpfe_capture - bug fixes and enhancements
Hans,
The change is because of the void * type that we use. Since ccdc parameter
structures are different for different IPs, a constant type for this arg
is not possible. The ccdc driver needs the pointer to structure. But the
v4l2 core tries to copies 4 bytes of data from the void * pointed location
which is not what we want. I am sure this code will change once we have a
device node available for ccdc on which case, this ioctl will be handled by the
ccdc sub device node. The long term goal is to convert ccdc/isif drivers
to sub device and pass this user ioctl to that sub device node. But currently
we don't have support for device nodes for sub devices. I think
that is needed for this conversion to be complete.
>BTW, does this patch series rely on the patches in my v4l-dvb-davinci tree?
>Or are these independent patches?
Yes, this is dependent on my earlier patch. I had asked Mauro to merge that
patch to linux-next, but still waiting
Murali Karicheri
Software Design Engineer
Texas Instruments Inc.
Germantown, MD 20874
phone: 301-407-9583
email: m-kariche...@ti.com
>-Original Message-
>From: Hans Verkuil [mailto:hverk...@xs4all.nl]
>Sent: Wednesday, December 23, 2009 9:24 AM
>To: Karicheri, Muralidharan
>Cc: linux-media@vger.kernel.org; khil...@deeprootsystems.com; Hiremath,
>Vaibhav; Nori, Sekhar; davinci-linux-open-sou...@linux.davincidsp.com
>Subject: Re: [PATCH - v2 4/6] V4L - vpfe_capture - bug fixes and
>enhancements
>
>Hi Murali,
>
>Sorry for the long delay in reviewing this patch series. I've been very
>busy,
>first at work, and now for Christmas preparations (and occasionally I'd
>like
>to relax as well :-) ).
>
>I'm OK with the other patches in this series, but I do have a few comments
>on this one: I noticed that you added a wrapper function for video_ioctl2.
>I don't quite understand why.
>
>BTW, does this patch series rely on the patches in my v4l-dvb-davinci tree?
>Or are these independent patches? Is it because the experimental
>VPFE_CMD_S/G_CCDC_RAW_PARAMS ioctls are used with different argument
>pointers?
>I mean, currently the arg is a void * instead of a properly typed argument.
>
>However, if it always uses the same type, then you should use that type in
>_IOR/_IOW and use video_ioctl2 directly so that the core framework will do
>the
>user-to-kernelspace conversion (and vv) for you.
>
>Regards,
>
> Hans
>
>On Saturday 19 December 2009 00:58:25 m-kariche...@ti.com wrote:
>> From: Muralidharan Karicheri
>>
>> Updated based on comments against v1 of the patch
>>
>> Added a experimental IOCTL, to read the CCDC parameters.
>> Default handler was not getting the original pointer from the core.
>> So a wrapper function added to handle the default handler properly.
>> Also fixed a bug in the probe() in the linux-next tree
>>
>> Reviewed-by: Hans Verkuil
>> Signed-off-by: Muralidharan Karicheri
>> ---
>> Applies to linux-next of v4l-dvb
>> drivers/media/video/davinci/vpfe_capture.c | 120 +-
>--
>> include/media/davinci/vpfe_capture.h | 12 ++-
>> 2 files changed, 81 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/media/video/davinci/vpfe_capture.c
>b/drivers/media/video/davinci/vpfe_capture.c
>> index 9e3a531..99ffc62 100644
>> --- a/drivers/media/video/davinci/vpfe_capture.c
>> +++ b/drivers/media/video/davinci/vpfe_capture.c
>> @@ -758,12 +758,83 @@ static unsigned int vpfe_poll(struct file *file,
>poll_table *wait)
>> return 0;
>> }
>>
>> +static long vpfe_param_handler(struct file *file, void *priv,
>> +int cmd, void *param)
>> +{
>> +struct vpfe_device *vpfe_dev = video_drvdata(file);
>> +int ret = 0;
>> +
>> +v4l2_dbg(1, debug, &vpfe_dev->v4l2_dev, "vpfe_param_handler\n");
>> +
>> +if (NULL == param) {
>> +v4l2_dbg(1, debug, &vpfe_dev->v4l2_dev,
>> +"Invalid user ptr\n");
>> +return -EINVAL;
>> +}
>> +
>> +if (vpfe_dev->started) {
>> +/* only allowed if streaming is not started */
>> +v4l2_err(&vpfe_dev->v4l2_dev, "device already started\n");
>> +return -EBUSY;
>> +}
>> +
>> +switch (cmd) {
>> +case VPFE_CMD_S_CCDC_RAW_PARAMS:
>> +v4l2_warn(&vpfe_dev->v4l2_dev,
>> + "VPFE_CMD_S_CCDC_RAW_PARAMS: experimental ioctl\n");
>> +ret = mutex_lock_interruptible(&vpfe_dev->lock);
>> +if (ret)
>> +
Re: [PATCH - v2 4/6] V4L - vpfe_capture - bug fixes and enhancements
Hi Murali,
Sorry for the long delay in reviewing this patch series. I've been very busy,
first at work, and now for Christmas preparations (and occasionally I'd like
to relax as well :-) ).
I'm OK with the other patches in this series, but I do have a few comments
on this one: I noticed that you added a wrapper function for video_ioctl2.
I don't quite understand why.
BTW, does this patch series rely on the patches in my v4l-dvb-davinci tree?
Or are these independent patches? Is it because the experimental
VPFE_CMD_S/G_CCDC_RAW_PARAMS ioctls are used with different argument pointers?
I mean, currently the arg is a void * instead of a properly typed argument.
However, if it always uses the same type, then you should use that type in
_IOR/_IOW and use video_ioctl2 directly so that the core framework will do the
user-to-kernelspace conversion (and vv) for you.
Regards,
Hans
On Saturday 19 December 2009 00:58:25 m-kariche...@ti.com wrote:
> From: Muralidharan Karicheri
>
> Updated based on comments against v1 of the patch
>
> Added a experimental IOCTL, to read the CCDC parameters.
> Default handler was not getting the original pointer from the core.
> So a wrapper function added to handle the default handler properly.
> Also fixed a bug in the probe() in the linux-next tree
>
> Reviewed-by: Hans Verkuil
> Signed-off-by: Muralidharan Karicheri
> ---
> Applies to linux-next of v4l-dvb
> drivers/media/video/davinci/vpfe_capture.c | 120
> +---
> include/media/davinci/vpfe_capture.h | 12 ++-
> 2 files changed, 81 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/media/video/davinci/vpfe_capture.c
> b/drivers/media/video/davinci/vpfe_capture.c
> index 9e3a531..99ffc62 100644
> --- a/drivers/media/video/davinci/vpfe_capture.c
> +++ b/drivers/media/video/davinci/vpfe_capture.c
> @@ -758,12 +758,83 @@ static unsigned int vpfe_poll(struct file *file,
> poll_table *wait)
> return 0;
> }
>
> +static long vpfe_param_handler(struct file *file, void *priv,
> + int cmd, void *param)
> +{
> + struct vpfe_device *vpfe_dev = video_drvdata(file);
> + int ret = 0;
> +
> + v4l2_dbg(1, debug, &vpfe_dev->v4l2_dev, "vpfe_param_handler\n");
> +
> + if (NULL == param) {
> + v4l2_dbg(1, debug, &vpfe_dev->v4l2_dev,
> + "Invalid user ptr\n");
> + return -EINVAL;
> + }
> +
> + if (vpfe_dev->started) {
> + /* only allowed if streaming is not started */
> + v4l2_err(&vpfe_dev->v4l2_dev, "device already started\n");
> + return -EBUSY;
> + }
> +
> + switch (cmd) {
> + case VPFE_CMD_S_CCDC_RAW_PARAMS:
> + v4l2_warn(&vpfe_dev->v4l2_dev,
> + "VPFE_CMD_S_CCDC_RAW_PARAMS: experimental ioctl\n");
> + ret = mutex_lock_interruptible(&vpfe_dev->lock);
> + if (ret)
> + return ret;
> + ret = ccdc_dev->hw_ops.set_params(param);
> + if (ret) {
> + v4l2_dbg(1, debug, &vpfe_dev->v4l2_dev,
> + "Error in setting parameters in CCDC\n");
> + goto unlock_out;
> + }
> +
> + if (vpfe_get_ccdc_image_format(vpfe_dev, &vpfe_dev->fmt) < 0) {
> + v4l2_err(&vpfe_dev->v4l2_dev,
> + "Invalid image format at CCDC\n");
> + ret = -EINVAL;
> + }
> +unlock_out:
> + mutex_unlock(&vpfe_dev->lock);
> + break;
> + case VPFE_CMD_G_CCDC_RAW_PARAMS:
> + v4l2_warn(&vpfe_dev->v4l2_dev,
> + "VPFE_CMD_G_CCDC_RAW_PARAMS: experimental ioctl\n");
> + if (!ccdc_dev->hw_ops.get_params) {
> + ret = -EINVAL;
> + break;
> + }
> + ret = ccdc_dev->hw_ops.get_params(param);
> + if (ret) {
> + v4l2_dbg(1, debug, &vpfe_dev->v4l2_dev,
> + "Error in getting parameters from CCDC\n");
> + }
> + break;
> + default:
> + ret = -EINVAL;
> + break;
> + }
> + return ret;
> +}
> +
> +static long vpfe_ioctl(struct file *file, unsigned int cmd, unsigned long
> arg)
> +{
> + if (cmd == VPFE_CMD_S_CCDC_RAW_PARAMS ||
> + cmd == VPFE_CMD_G_CCDC_RAW_PARAMS)
> + return vpfe_param_handler(file, file->private_data, cmd,
> + (void *)arg);
> + return video_ioctl2(file, cmd, arg);
> +}
> +
> /* vpfe capture driver file operations */
> static const struct v4l2_file_operations vpfe_fops = {
> .owner = THIS_MODULE,
> .open = vpfe_open,
> .release = vpfe_release,
> - .unlocked_ioctl = video_ioctl2,
> + .unlocked_ioctl = vpfe_ioctl,
> .mmap = vpfe_mmap,
> .poll = vpfe_poll
> };
> @@ -1681,50 +1752,6 @@ u
