[REVIEW PATCH 0/3] Add g_tvnorms video op

[REVIEW PATCH 0/3] Add g_tvnorms video op

[Hans Verkuil]

This patch series addresses a problem that was exposed by commit a5338190e.
The issue is that soc_camera implements s/g_std ioctls and just forwards
those to the subdev, whether or not the subdev actually implements them.

In addition, tvnorms is never set, so even if the subdev implements the
s/g_std the ENUMSTD ioctl will not report anything.

The solution is to add a g_tvnorms video op to v4l2_subdev (there was already
a g_tvnorms_output, so that fits nicely) and to let soc_camera call that so
the video_device tvnorms field is set correctly.

Before registering the video node it will check if tvnorms == 0 and disable
the STD ioctls if that's the case.

While this problem cropped up in soc_camera it is really a problem for any
generic bridge driver, so this is useful to have.

Note that it is untested. The plan is that Laurent tests and Guennadi pulls
it into his tree.

Regards,

	Hans

[REVIEW PATCH 1/3] v4l2-subdev.h: add g_tvnorms video op

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

While there was already a g_tvnorms_output video op, it's counterpart for
video capture was missing. Add it.

This is necessary for generic bridge drivers like soc-camera to set the
video_device tvnorms field correctly. Otherwise ENUMSTD cannot work.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 include/media/v4l2-subdev.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
index d67210a..787d078 100644
--- a/include/media/v4l2-subdev.h
+++ b/include/media/v4l2-subdev.h
@@ -264,8 +264,11 @@ struct v4l2_mbus_frame_desc {
    g_std_output: get current standard for video OUTPUT devices. This is ignored
 	by video input devices.
 
-   g_tvnorms_output: get v4l2_std_id with all standards supported by video
-	OUTPUT device. This is ignored by video input devices.
+   g_tvnorms: get v4l2_std_id with all standards supported by the video
+	CAPTURE device. This is ignored by video output devices.
+
+   g_tvnorms_output: get v4l2_std_id with all standards supported by the video
+	OUTPUT device. This is ignored by video capture devices.
 
    s_crystal_freq: sets the frequency of the crystal used to generate the
 	clocks in Hz. An extra flags field allows device specific configuration
@@ -308,6 +311,7 @@ struct v4l2_subdev_video_ops {
 	int (*s_std_output)(struct v4l2_subdev *sd, v4l2_std_id std);
 	int (*g_std_output)(struct v4l2_subdev *sd, v4l2_std_id *std);
 	int (*querystd)(struct v4l2_subdev *sd, v4l2_std_id *std);
+	int (*g_tvnorms)(struct v4l2_subdev *sd, v4l2_std_id *std);
 	int (*g_tvnorms_output)(struct v4l2_subdev *sd, v4l2_std_id *std);
 	int (*g_input_status)(struct v4l2_subdev *sd, u32 *status);
 	int (*s_stream)(struct v4l2_subdev *sd, int enable);
-- 
1.8.5.2

Re: [REVIEW PATCH 1/3] v4l2-subdev.h: add g_tvnorms video op

[Guennadi Liakhovetski]

Hi Hans,

Thanks for taking care about this problem. I'm not sure it would be ok for 
me to pull this specific patch via my tree, because it's for the V4L2 
core, and the other 2 patches in this series depend on this one. But 
anyway I've got a question to this patch:

On Mon, 17 Feb 2014, Hans Verkuil wrote:

> From: Hans Verkuil <hans.verkuil@cisco.com>
> 
> While there was already a g_tvnorms_output video op, it's counterpart for
> video capture was missing. Add it.
> 
> This is necessary for generic bridge drivers like soc-camera to set the
> video_device tvnorms field correctly. Otherwise ENUMSTD cannot work.
> 
> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  include/media/v4l2-subdev.h | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index d67210a..787d078 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -264,8 +264,11 @@ struct v4l2_mbus_frame_desc {
>     g_std_output: get current standard for video OUTPUT devices. This is ignored
>  	by video input devices.
>  
> -   g_tvnorms_output: get v4l2_std_id with all standards supported by video
> -	OUTPUT device. This is ignored by video input devices.
> +   g_tvnorms: get v4l2_std_id with all standards supported by the video
> +	CAPTURE device. This is ignored by video output devices.
> +
> +   g_tvnorms_output: get v4l2_std_id with all standards supported by the video
> +	OUTPUT device. This is ignored by video capture devices.

Why do we need two separate operations with the same functionality - one 
for capture and one for output? Can we have subdevices, that need to 
implement both? Besides, what about these two core ops:

	int (*g_std)(struct v4l2_subdev *sd, v4l2_std_id *norm);
	int (*s_std)(struct v4l2_subdev *sd, v4l2_std_id norm);

? Seems like a slightly different approach is needed? Or am I missing 
anything?

Thanks
Guennadi

>     s_crystal_freq: sets the frequency of the crystal used to generate the
>  	clocks in Hz. An extra flags field allows device specific configuration
> @@ -308,6 +311,7 @@ struct v4l2_subdev_video_ops {
>  	int (*s_std_output)(struct v4l2_subdev *sd, v4l2_std_id std);
>  	int (*g_std_output)(struct v4l2_subdev *sd, v4l2_std_id *std);
>  	int (*querystd)(struct v4l2_subdev *sd, v4l2_std_id *std);
> +	int (*g_tvnorms)(struct v4l2_subdev *sd, v4l2_std_id *std);
>  	int (*g_tvnorms_output)(struct v4l2_subdev *sd, v4l2_std_id *std);
>  	int (*g_input_status)(struct v4l2_subdev *sd, u32 *status);
>  	int (*s_stream)(struct v4l2_subdev *sd, int enable);
> -- 
> 1.8.5.2
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [REVIEW PATCH 1/3] v4l2-subdev.h: add g_tvnorms video op

[Hans Verkuil]

On 03/11/2014 12:23 AM, Guennadi Liakhovetski wrote:
> Hi Hans,
> 
> Thanks for taking care about this problem. I'm not sure it would be ok for 
> me to pull this specific patch via my tree, because it's for the V4L2 
> core, and the other 2 patches in this series depend on this one.

It's OK by me to pull this through your tree.

> But 
> anyway I've got a question to this patch:
> 
> On Mon, 17 Feb 2014, Hans Verkuil wrote:
> 
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> While there was already a g_tvnorms_output video op, it's counterpart for
>> video capture was missing. Add it.
>>
>> This is necessary for generic bridge drivers like soc-camera to set the
>> video_device tvnorms field correctly. Otherwise ENUMSTD cannot work.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  include/media/v4l2-subdev.h | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index d67210a..787d078 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -264,8 +264,11 @@ struct v4l2_mbus_frame_desc {
>>     g_std_output: get current standard for video OUTPUT devices. This is ignored
>>  	by video input devices.
>>  
>> -   g_tvnorms_output: get v4l2_std_id with all standards supported by video
>> -	OUTPUT device. This is ignored by video input devices.
>> +   g_tvnorms: get v4l2_std_id with all standards supported by the video
>> +	CAPTURE device. This is ignored by video output devices.
>> +
>> +   g_tvnorms_output: get v4l2_std_id with all standards supported by the video
>> +	OUTPUT device. This is ignored by video capture devices.
> 
> Why do we need two separate operations with the same functionality - one 
> for capture and one for output? Can we have subdevices, that need to 
> implement both? Besides, what about these two core ops:
> 
> 	int (*g_std)(struct v4l2_subdev *sd, v4l2_std_id *norm);
> 	int (*s_std)(struct v4l2_subdev *sd, v4l2_std_id norm);
> 
> ? Seems like a slightly different approach is needed? Or am I missing 
> anything?

There are drivers (ivtv) that have both capture and output subdevices. Each can
have its own standard. Such drivers use v4l2_device_call_all() to call the
same op for all subdevs so any subdev that can handle that op gets called.
So they use it to call the s_std op to change the capture standard and they
call s_std_output to change the output standard.

If you can't tell the difference between capture tvnorms and output tvnorms this
becomes tricky to handle. Just keep the two separate and there is no confusion.

Note that the video ops have the g/s_std_output ops. It's due to historical
reasons that g/s_std ended up in the core ops. They probably should be moved to
the video ops, but it's just not worth the effort.

Regards,

	Hans

> 
> Thanks
> Guennadi
> 
>>     s_crystal_freq: sets the frequency of the crystal used to generate the
>>  	clocks in Hz. An extra flags field allows device specific configuration
>> @@ -308,6 +311,7 @@ struct v4l2_subdev_video_ops {
>>  	int (*s_std_output)(struct v4l2_subdev *sd, v4l2_std_id std);
>>  	int (*g_std_output)(struct v4l2_subdev *sd, v4l2_std_id *std);
>>  	int (*querystd)(struct v4l2_subdev *sd, v4l2_std_id *std);
>> +	int (*g_tvnorms)(struct v4l2_subdev *sd, v4l2_std_id *std);
>>  	int (*g_tvnorms_output)(struct v4l2_subdev *sd, v4l2_std_id *std);
>>  	int (*g_input_status)(struct v4l2_subdev *sd, u32 *status);
>>  	int (*s_stream)(struct v4l2_subdev *sd, int enable);
>> -- 
>> 1.8.5.2
>>
> 
> ---
> 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
> 

Re: [REVIEW PATCH 1/3] v4l2-subdev.h: add g_tvnorms video op

[Laurent Pinchart]

Hi Hans,

On Tuesday 11 March 2014 00:32:48 Hans Verkuil wrote:
> On 03/11/2014 12:23 AM, Guennadi Liakhovetski wrote:
> > Hi Hans,
> > 
> > Thanks for taking care about this problem. I'm not sure it would be ok for
> > me to pull this specific patch via my tree, because it's for the V4L2
> > core, and the other 2 patches in this series depend on this one.
> 
> It's OK by me to pull this through your tree.
> 
> > But anyway I've got a question to this patch:
> > 
> > On Mon, 17 Feb 2014, Hans Verkuil wrote:
> >> From: Hans Verkuil <hans.verkuil@cisco.com>
> >> 
> >> While there was already a g_tvnorms_output video op, it's counterpart for
> >> video capture was missing. Add it.
> >> 
> >> This is necessary for generic bridge drivers like soc-camera to set the
> >> video_device tvnorms field correctly. Otherwise ENUMSTD cannot work.
> >> 
> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> >> ---
> >> 
> >>  include/media/v4l2-subdev.h | 8 ++++++--
> >>  1 file changed, 6 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> >> index d67210a..787d078 100644
> >> --- a/include/media/v4l2-subdev.h
> >> +++ b/include/media/v4l2-subdev.h
> >> @@ -264,8 +264,11 @@ struct v4l2_mbus_frame_desc {
> >> 
> >>     g_std_output: get current standard for video OUTPUT devices. This is
> >>     ignored by video input devices.
> >> 
> >> -   g_tvnorms_output: get v4l2_std_id with all standards supported by
> >> video
> >> -	OUTPUT device. This is ignored by video input devices.
> >> +   g_tvnorms: get v4l2_std_id with all standards supported by the video
> >> +	CAPTURE device. This is ignored by video output devices.
> >> +
> >> +   g_tvnorms_output: get v4l2_std_id with all standards supported by the
> >> video +	OUTPUT device. This is ignored by video capture devices.
> > 
> > Why do we need two separate operations with the same functionality - one
> > for capture and one for output? Can we have subdevices, that need to
> > 
> > implement both? Besides, what about these two core ops:
> > 	int (*g_std)(struct v4l2_subdev *sd, v4l2_std_id *norm);
> > 	int (*s_std)(struct v4l2_subdev *sd, v4l2_std_id norm);
> > 
> > ? Seems like a slightly different approach is needed? Or am I missing
> > anything?
> 
> There are drivers (ivtv) that have both capture and output subdevices. Each
> can have its own standard. Such drivers use v4l2_device_call_all() to call
> the same op for all subdevs so any subdev that can handle that op gets
> called. So they use it to call the s_std op to change the capture standard
> and they call s_std_output to change the output standard.
> 
> If you can't tell the difference between capture tvnorms and output tvnorms
> this becomes tricky to handle. Just keep the two separate and there is no
> confusion.
> 
> Note that the video ops have the g/s_std_output ops. It's due to historical
> reasons that g/s_std ended up in the core ops. They probably should be moved
> to the video ops, but it's just not worth the effort.

It's not such a big effort, I can easily cook up a patch. However, it looks 
like the s_std operation is implemented by the following subdev drivers that 
don't implement video ops:

tuner-core
tvaudio
sony-btf-mpx
vp27smpx
cx18-gpio

It probably doesn't make much sense to add video ops to all of those.

-- 
Regards,

Laurent Pinchart

Re: [REVIEW PATCH 1/3] v4l2-subdev.h: add g_tvnorms video op

[Hans Verkuil]

On 03/11/14 10:33, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Tuesday 11 March 2014 00:32:48 Hans Verkuil wrote:
>> On 03/11/2014 12:23 AM, Guennadi Liakhovetski wrote:
>>> Hi Hans,
>>>
>>> Thanks for taking care about this problem. I'm not sure it would be ok for
>>> me to pull this specific patch via my tree, because it's for the V4L2
>>> core, and the other 2 patches in this series depend on this one.
>>
>> It's OK by me to pull this through your tree.
>>
>>> But anyway I've got a question to this patch:
>>>
>>> On Mon, 17 Feb 2014, Hans Verkuil wrote:
>>>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>>>
>>>> While there was already a g_tvnorms_output video op, it's counterpart for
>>>> video capture was missing. Add it.
>>>>
>>>> This is necessary for generic bridge drivers like soc-camera to set the
>>>> video_device tvnorms field correctly. Otherwise ENUMSTD cannot work.
>>>>
>>>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>>>> ---
>>>>
>>>>  include/media/v4l2-subdev.h | 8 ++++++--
>>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>>>> index d67210a..787d078 100644
>>>> --- a/include/media/v4l2-subdev.h
>>>> +++ b/include/media/v4l2-subdev.h
>>>> @@ -264,8 +264,11 @@ struct v4l2_mbus_frame_desc {
>>>>
>>>>     g_std_output: get current standard for video OUTPUT devices. This is
>>>>     ignored by video input devices.
>>>>
>>>> -   g_tvnorms_output: get v4l2_std_id with all standards supported by
>>>> video
>>>> -	OUTPUT device. This is ignored by video input devices.
>>>> +   g_tvnorms: get v4l2_std_id with all standards supported by the video
>>>> +	CAPTURE device. This is ignored by video output devices.
>>>> +
>>>> +   g_tvnorms_output: get v4l2_std_id with all standards supported by the
>>>> video +	OUTPUT device. This is ignored by video capture devices.
>>>
>>> Why do we need two separate operations with the same functionality - one
>>> for capture and one for output? Can we have subdevices, that need to
>>>
>>> implement both? Besides, what about these two core ops:
>>> 	int (*g_std)(struct v4l2_subdev *sd, v4l2_std_id *norm);
>>> 	int (*s_std)(struct v4l2_subdev *sd, v4l2_std_id norm);
>>>
>>> ? Seems like a slightly different approach is needed? Or am I missing
>>> anything?
>>
>> There are drivers (ivtv) that have both capture and output subdevices. Each
>> can have its own standard. Such drivers use v4l2_device_call_all() to call
>> the same op for all subdevs so any subdev that can handle that op gets
>> called. So they use it to call the s_std op to change the capture standard
>> and they call s_std_output to change the output standard.
>>
>> If you can't tell the difference between capture tvnorms and output tvnorms
>> this becomes tricky to handle. Just keep the two separate and there is no
>> confusion.
>>
>> Note that the video ops have the g/s_std_output ops. It's due to historical
>> reasons that g/s_std ended up in the core ops. They probably should be moved
>> to the video ops, but it's just not worth the effort.
> 
> It's not such a big effort, I can easily cook up a patch. However, it looks 
> like the s_std operation is implemented by the following subdev drivers that 
> don't implement video ops:
> 
> tuner-core
> tvaudio
> sony-btf-mpx
> vp27smpx
> cx18-gpio
> 
> It probably doesn't make much sense to add video ops to all of those.

Well, if you move it to the video ops, then you have to. This was one of the
reasons I decided at the time to keep g/s_std in core since they are used by
drivers that otherwise do not need video ops. But in hindsight I always thought
is was a mistake and I was just a bit too lazy.

I really don't think it's worth the effort to change it all, but I won't
hold you back either :-)

Regards,

	Hans

[REVIEW PATCH 2/3] tw9910: add g_tvnorms video op

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Report to soc_camera which standards are supported by tw9910.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/i2c/soc_camera/tw9910.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/i2c/soc_camera/tw9910.c b/drivers/media/i2c/soc_camera/tw9910.c
index ab54628..02a51ff 100644
--- a/drivers/media/i2c/soc_camera/tw9910.c
+++ b/drivers/media/i2c/soc_camera/tw9910.c
@@ -872,6 +872,12 @@ static int tw9910_s_mbus_config(struct v4l2_subdev *sd,
 	return i2c_smbus_write_byte_data(client, OUTCTR1, val);
 }
 
+static int tw9910_g_tvnorms(struct v4l2_subdev *sd, v4l2_std_id *norm)
+{
+	*norm = V4L2_STD_NTSC | V4L2_STD_PAL;
+	return 0;
+}
+
 static struct v4l2_subdev_video_ops tw9910_subdev_video_ops = {
 	.s_stream	= tw9910_s_stream,
 	.g_mbus_fmt	= tw9910_g_fmt,
@@ -882,6 +888,7 @@ static struct v4l2_subdev_video_ops tw9910_subdev_video_ops = {
 	.enum_mbus_fmt	= tw9910_enum_fmt,
 	.g_mbus_config	= tw9910_g_mbus_config,
 	.s_mbus_config	= tw9910_s_mbus_config,
+	.g_tvnorms	= tw9910_g_tvnorms,
 };
 
 static struct v4l2_subdev_ops tw9910_subdev_ops = {
-- 
1.8.5.2

[REVIEW PATCH 3/3] soc_camera: disable STD ioctls if no tvnorms are set.

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

If the sub-device did not report any tvnorms, then disable the STD
ioctls.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/soc_camera/soc_camera.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
index 4b8c024..c8549bf 100644
--- a/drivers/media/platform/soc_camera/soc_camera.c
+++ b/drivers/media/platform/soc_camera/soc_camera.c
@@ -1277,6 +1277,8 @@ static int soc_camera_probe_finish(struct soc_camera_device *icd)
 	sd->grp_id = soc_camera_grp_id(icd);
 	v4l2_set_subdev_hostdata(sd, icd);
 
+	v4l2_subdev_call(sd, video, g_tvnorms, &icd->vdev->tvnorms);
+
 	ret = v4l2_ctrl_add_handler(&icd->ctrl_handler, sd->ctrl_handler, NULL);
 	if (ret < 0)
 		return ret;
@@ -1997,6 +1999,12 @@ static int soc_camera_video_start(struct soc_camera_device *icd)
 		return -ENODEV;
 
 	video_set_drvdata(icd->vdev, icd);
+	if (icd->vdev->tvnorms == 0) {
+		/* disable the STD API if there are no tvnorms defined */
+		v4l2_disable_ioctl(icd->vdev, VIDIOC_G_STD);
+		v4l2_disable_ioctl(icd->vdev, VIDIOC_S_STD);
+		v4l2_disable_ioctl(icd->vdev, VIDIOC_ENUMSTD);
+	}
 	ret = video_register_device(icd->vdev, VFL_TYPE_GRABBER, -1);
 	if (ret < 0) {
 		dev_err(icd->pdev, "video_register_device failed: %d\n", ret);
-- 
1.8.5.2

Re: [REVIEW PATCH 0/3] Add g_tvnorms video op

[Laurent Pinchart]

Hi Hans,

Thank you for the patches.

On Monday 17 February 2014 12:44:11 Hans Verkuil wrote:
> This patch series addresses a problem that was exposed by commit a5338190e.
> The issue is that soc_camera implements s/g_std ioctls and just forwards
> those to the subdev, whether or not the subdev actually implements them.
> 
> In addition, tvnorms is never set, so even if the subdev implements the
> s/g_std the ENUMSTD ioctl will not report anything.
> 
> The solution is to add a g_tvnorms video op to v4l2_subdev (there was
> already a g_tvnorms_output, so that fits nicely) and to let soc_camera call
> that so the video_device tvnorms field is set correctly.
> 
> Before registering the video node it will check if tvnorms == 0 and disable
> the STD ioctls if that's the case.
> 
> While this problem cropped up in soc_camera it is really a problem for any
> generic bridge driver, so this is useful to have.
> 
> Note that it is untested. The plan is that Laurent tests and Guennadi pulls
> it into his tree.

I've tested the series on v3.10 with the atmel-isi driver. Without the patches 
applied ENUMSTD returns -ENODATA, and with the patches applied it returns -
ENOTTY.

Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart