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 m-kariche...@ti.com
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 hverk...@xs4all.nl
Signed-off-by: Muralidharan Karicheri m-kariche...@ti.com
---
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
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 m-kariche...@ti.com
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 hverk...@xs4all.nl
Signed-off-by: Muralidharan Karicheri m-kariche...@ti.com
---
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 @@ unlock_out:
return ret;
}
-
-static long vpfe_param_handler(struct file *file, void
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 m-kariche...@ti.com
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 hverk...@xs4all.nl
Signed-off-by: Muralidharan Karicheri m-kariche...@ti.com
---
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
[PATCH - v2 4/6] V4L - vpfe_capture - bug fixes and enhancements
From: Muralidharan Karicheri m-kariche...@ti.com
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 hverk...@xs4all.nl
Signed-off-by: Muralidharan Karicheri m-kariche...@ti.com
---
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 @@ unlock_out:
return ret;
}
-
-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 (vpfe_dev-started) {
- /* only allowed if streaming is not started */
- v4l2_err(vpfe_dev-v4l2_dev, device already started\n);
- return -EBUSY;
- }
-
- ret = mutex_lock_interruptible(vpfe_dev-lock);
- if (ret)
- return ret;
-
- 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 = ccdc_dev-hw_ops.set_params(param);
- if (ret) {
- v4l2_err(vpfe_dev-v4l2_dev,
- Error in setting parameters in CCDC\n);
- goto unlock_out;
-
