[1/4] V4L2: Added New V4L2 CIDs VIDIOC_S/G_COLOR_SPACE_CONV
Commit Message
From: Vaibhav Hiremath <hvaibhav@ti.com>
Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
---
drivers/media/video/v4l2-ioctl.c | 19 +++++++++++++++++++
include/linux/videodev2.h | 14 ++++++++++++++
include/media/v4l2-ioctl.h | 4 ++++
3 files changed, 37 insertions(+), 0 deletions(-)
Comments
Hi,
On Friday 16 October 2009 12:19:20 hvaibhav@ti.com wrote:
> From: Vaibhav Hiremath <hvaibhav@ti.com>
>
>
> Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
> ---
> drivers/media/video/v4l2-ioctl.c | 19 +++++++++++++++++++
> include/linux/videodev2.h | 14 ++++++++++++++
> include/media/v4l2-ioctl.h | 4 ++++
> 3 files changed, 37 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/media/video/v4l2-ioctl.c
> b/drivers/media/video/v4l2-ioctl.c index 30cc334..d3140e0 100644
> --- a/drivers/media/video/v4l2-ioctl.c
> +++ b/drivers/media/video/v4l2-ioctl.c
> @@ -284,6 +284,8 @@ static const char *v4l2_ioctls[] = {
> [_IOC_NR(VIDIOC_DBG_G_CHIP_IDENT)] = "VIDIOC_DBG_G_CHIP_IDENT",
> [_IOC_NR(VIDIOC_S_HW_FREQ_SEEK)] = "VIDIOC_S_HW_FREQ_SEEK",
> #endif
> + [_IOC_NR(VIDIOC_S_COLOR_SPACE_CONV)] = "VIDIOC_S_COLOR_SPACE_CONV",
> + [_IOC_NR(VIDIOC_G_COLOR_SPACE_CONV)] = "VIDIOC_G_COLOR_SPACE_CONV",
> };
> #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
>
This should go through a control, not an ioctl. Strings control have recently
been introduced, it should be fairly easy to create binary controls for such
cases.
On Friday 16 October 2009 14:26:52 Laurent Pinchart wrote:
> Hi,
>
> On Friday 16 October 2009 12:19:20 hvaibhav@ti.com wrote:
> > From: Vaibhav Hiremath <hvaibhav@ti.com>
> >
> >
> > Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
> > ---
> > drivers/media/video/v4l2-ioctl.c | 19 +++++++++++++++++++
> > include/linux/videodev2.h | 14 ++++++++++++++
> > include/media/v4l2-ioctl.h | 4 ++++
> > 3 files changed, 37 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/media/video/v4l2-ioctl.c
> > b/drivers/media/video/v4l2-ioctl.c index 30cc334..d3140e0 100644
> > --- a/drivers/media/video/v4l2-ioctl.c
> > +++ b/drivers/media/video/v4l2-ioctl.c
> > @@ -284,6 +284,8 @@ static const char *v4l2_ioctls[] = {
> > [_IOC_NR(VIDIOC_DBG_G_CHIP_IDENT)] = "VIDIOC_DBG_G_CHIP_IDENT",
> > [_IOC_NR(VIDIOC_S_HW_FREQ_SEEK)] = "VIDIOC_S_HW_FREQ_SEEK",
> > #endif
> > + [_IOC_NR(VIDIOC_S_COLOR_SPACE_CONV)] = "VIDIOC_S_COLOR_SPACE_CONV",
> > + [_IOC_NR(VIDIOC_G_COLOR_SPACE_CONV)] = "VIDIOC_G_COLOR_SPACE_CONV",
> > };
> > #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
>
> This should go through a control, not an ioctl. Strings control have
> recently been introduced, it should be fairly easy to create binary
> controls for such cases.
I'm not sure whether this should be seen as a control. That feels like an
abuse of the control framework to me.
Actually, shouldn't this be something for a subdev node? I.e. an omap2/3
specific ioctl?
It might be good to refresh our memory of how this is supposed to be used.
Regards,
Hans
On Friday 16 October 2009 22:26:53 Hans Verkuil wrote:
> On Friday 16 October 2009 14:26:52 Laurent Pinchart wrote:
> > Hi,
> >
> > On Friday 16 October 2009 12:19:20 hvaibhav@ti.com wrote:
> > > From: Vaibhav Hiremath <hvaibhav@ti.com>
> > >
> > >
> > > Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
> > > ---
> > > drivers/media/video/v4l2-ioctl.c | 19 +++++++++++++++++++
> > > include/linux/videodev2.h | 14 ++++++++++++++
> > > include/media/v4l2-ioctl.h | 4 ++++
> > > 3 files changed, 37 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/drivers/media/video/v4l2-ioctl.c
> > > b/drivers/media/video/v4l2-ioctl.c index 30cc334..d3140e0 100644
> > > --- a/drivers/media/video/v4l2-ioctl.c
> > > +++ b/drivers/media/video/v4l2-ioctl.c
> > > @@ -284,6 +284,8 @@ static const char *v4l2_ioctls[] = {
> > > [_IOC_NR(VIDIOC_DBG_G_CHIP_IDENT)] = "VIDIOC_DBG_G_CHIP_IDENT",
> > > [_IOC_NR(VIDIOC_S_HW_FREQ_SEEK)] = "VIDIOC_S_HW_FREQ_SEEK",
> > > #endif
> > > + [_IOC_NR(VIDIOC_S_COLOR_SPACE_CONV)] = "VIDIOC_S_COLOR_SPACE_CONV",
> > > + [_IOC_NR(VIDIOC_G_COLOR_SPACE_CONV)] = "VIDIOC_G_COLOR_SPACE_CONV",
> > > };
> > > #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
> >
> > This should go through a control, not an ioctl. Strings control have
> > recently been introduced, it should be fairly easy to create binary
> > controls for such cases.
>
> I'm not sure whether this should be seen as a control. That feels like an
> abuse of the control framework to me.
>
> Actually, shouldn't this be something for a subdev node? I.e. an omap2/3
> specific ioctl?
I would see it as a subdev control. Even though this is a complex control that
uses a matrix instead of a simple integer value, I believe this kind of use
cases qualify for the control API. They're really controls, i.e. values that
apply to a block in the video pipeline to tune its behavior.
> It might be good to refresh our memory of how this is supposed to be used.
> -----Original Message-----
> From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com]
> Sent: Friday, October 16, 2009 5:57 PM
> To: Hiremath, Vaibhav
> Cc: linux-media@vger.kernel.org
> Subject: Re: [PATCH 1/4] V4L2: Added New V4L2 CIDs
> VIDIOC_S/G_COLOR_SPACE_CONV
>
> Hi,
>
> On Friday 16 October 2009 12:19:20 hvaibhav@ti.com wrote:
> > From: Vaibhav Hiremath <hvaibhav@ti.com>
> >
> >
> > Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
> > ---
> > drivers/media/video/v4l2-ioctl.c | 19 +++++++++++++++++++
> > include/linux/videodev2.h | 14 ++++++++++++++
> > include/media/v4l2-ioctl.h | 4 ++++
> > 3 files changed, 37 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/media/video/v4l2-ioctl.c
> > b/drivers/media/video/v4l2-ioctl.c index 30cc334..d3140e0 100644
> > --- a/drivers/media/video/v4l2-ioctl.c
> > +++ b/drivers/media/video/v4l2-ioctl.c
> > @@ -284,6 +284,8 @@ static const char *v4l2_ioctls[] = {
> > [_IOC_NR(VIDIOC_DBG_G_CHIP_IDENT)] =
> "VIDIOC_DBG_G_CHIP_IDENT",
> > [_IOC_NR(VIDIOC_S_HW_FREQ_SEEK)] = "VIDIOC_S_HW_FREQ_SEEK",
> > #endif
> > + [_IOC_NR(VIDIOC_S_COLOR_SPACE_CONV)] =
> "VIDIOC_S_COLOR_SPACE_CONV",
> > + [_IOC_NR(VIDIOC_G_COLOR_SPACE_CONV)] =
> "VIDIOC_G_COLOR_SPACE_CONV",
> > };
> > #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
> >
>
> This should go through a control, not an ioctl. Strings control have
> recently
> been introduced, it should be fairly easy to create binary controls
> for such
> cases.
>
[Hiremath, Vaibhav] I am really not sure how we can fit this into string control.
Atleast from OMAP3 point of view we need nine 11 bit signed coeff. We can not use string control here but we can leverage same mechanism.
I can have __s32 * in v4l2_ext_control which will point to array of nine 11 bit coeff.
Again there is control over full or limited range conversion.
Thanks,
Vaibhav
> --
> Regards,
>
> Laurent Pinchart
--
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
On Tuesday 20 October 2009 14:22:03 Laurent Pinchart wrote:
> On Friday 16 October 2009 22:26:53 Hans Verkuil wrote:
> > On Friday 16 October 2009 14:26:52 Laurent Pinchart wrote:
> > > Hi,
> > >
> > > On Friday 16 October 2009 12:19:20 hvaibhav@ti.com wrote:
> > > > From: Vaibhav Hiremath <hvaibhav@ti.com>
> > > >
> > > >
> > > > Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
> > > > ---
> > > > drivers/media/video/v4l2-ioctl.c | 19 +++++++++++++++++++
> > > > include/linux/videodev2.h | 14 ++++++++++++++
> > > > include/media/v4l2-ioctl.h | 4 ++++
> > > > 3 files changed, 37 insertions(+), 0 deletions(-)
> > > >
> > > > diff --git a/drivers/media/video/v4l2-ioctl.c
> > > > b/drivers/media/video/v4l2-ioctl.c index 30cc334..d3140e0 100644
> > > > --- a/drivers/media/video/v4l2-ioctl.c
> > > > +++ b/drivers/media/video/v4l2-ioctl.c
> > > > @@ -284,6 +284,8 @@ static const char *v4l2_ioctls[] = {
> > > > [_IOC_NR(VIDIOC_DBG_G_CHIP_IDENT)] = "VIDIOC_DBG_G_CHIP_IDENT",
> > > > [_IOC_NR(VIDIOC_S_HW_FREQ_SEEK)] = "VIDIOC_S_HW_FREQ_SEEK",
> > > > #endif
> > > > + [_IOC_NR(VIDIOC_S_COLOR_SPACE_CONV)] = "VIDIOC_S_COLOR_SPACE_CONV",
> > > > + [_IOC_NR(VIDIOC_G_COLOR_SPACE_CONV)] = "VIDIOC_G_COLOR_SPACE_CONV",
> > > > };
> > > > #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
> > >
> > > This should go through a control, not an ioctl. Strings control have
> > > recently been introduced, it should be fairly easy to create binary
> > > controls for such cases.
> >
> > I'm not sure whether this should be seen as a control. That feels like an
> > abuse of the control framework to me.
> >
> > Actually, shouldn't this be something for a subdev node? I.e. an omap2/3
> > specific ioctl?
>
> I would see it as a subdev control. Even though this is a complex control that
> uses a matrix instead of a simple integer value, I believe this kind of use
> cases qualify for the control API. They're really controls, i.e. values that
> apply to a block in the video pipeline to tune its behavior.
You have a good point here. It is similar in behavior to e.g. a brightness
control.
Regards,
Hans
>
> > It might be good to refresh our memory of how this is supposed to be used.
>
@@ -284,6 +284,8 @@ static const char *v4l2_ioctls[] = {
[_IOC_NR(VIDIOC_DBG_G_CHIP_IDENT)] = "VIDIOC_DBG_G_CHIP_IDENT",
[_IOC_NR(VIDIOC_S_HW_FREQ_SEEK)] = "VIDIOC_S_HW_FREQ_SEEK",
#endif
+ [_IOC_NR(VIDIOC_S_COLOR_SPACE_CONV)] = "VIDIOC_S_COLOR_SPACE_CONV",
+ [_IOC_NR(VIDIOC_G_COLOR_SPACE_CONV)] = "VIDIOC_G_COLOR_SPACE_CONV",
};
#define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
@@ -1795,6 +1797,23 @@ static long __video_do_ioctl(struct file *file,
break;
}
+ /*---------------Color space conversion------------------------------*/
+ case VIDIOC_S_COLOR_SPACE_CONV:
+ {
+ struct v4l2_color_space_conv *p = arg;
+ if (!ops->vidioc_s_color_space_conv)
+ break;
+ ret = ops->vidioc_s_color_space_conv(file, fh, p);
+ break;
+ }
+ case VIDIOC_G_COLOR_SPACE_CONV:
+ {
+ struct v4l2_color_space_conv *p = arg;
+ if (!ops->vidioc_g_color_space_conv)
+ break;
+ ret = ops->vidioc_g_color_space_conv(file, fh, p);
+ break;
+ }
default:
{
if (!ops->vidioc_default)
@@ -1281,6 +1281,18 @@ struct v4l2_rds_data {
#define V4L2_RDS_BLOCK_ERROR 0x80
/*
+ * Color conversion
+ * User needs to pass pointer to color conversion matrix
+ * defined by hardware
+ */
+struct v4l2_color_space_conv {
+ __s32 coefficients[3][3];
+ __s32 const_factor;
+ __s32 input_offs[3];
+ __s32 output_offs[3];
+};
+
+/*
* A U D I O
*/
struct v4l2_audio {
@@ -1619,6 +1631,8 @@ struct v4l2_dbg_chip_ident {
#endif
#define VIDIOC_S_HW_FREQ_SEEK _IOW('V', 82, struct v4l2_hw_freq_seek)
+#define VIDIOC_S_COLOR_SPACE_CONV _IOW('V', 83, struct v4l2_color_space_conv)
+#define VIDIOC_G_COLOR_SPACE_CONV _IOR('V', 84, struct v4l2_color_space_conv)
/* Reminder: when adding new ioctls please add support for them to
drivers/media/video/v4l2-compat-ioctl32.c as well! */
@@ -242,6 +242,10 @@ struct v4l2_ioctl_ops {
/* For other private ioctls */
long (*vidioc_default) (struct file *file, void *fh,
int cmd, void *arg);
+ int (*vidioc_s_color_space_conv) (struct file *file, void *fh,
+ struct v4l2_color_space_conv *a);
+ int (*vidioc_g_color_space_conv) (struct file *file, void *fh,
+ struct v4l2_color_space_conv *a);
};