[PATCH] [media] tvp686x: Don't go past array

[PATCH] [media] tvp686x: Don't go past array

[Mauro Carvalho Chehab]

Depending on the compiler version, currently it produces the
following warnings:
	tw686x-video.c: In function 'tw686x_video_init':
	tw686x-video.c:65:543: warning: array subscript is above array bounds [-Warray-bounds]

This is actually bogus with the current code, as it currently
hardcodes the framerate to 30 frames/sec, however a potential
use after the array size could happen when the driver adds support
for setting the framerate. So, fix it.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
---
 drivers/media/pci/tw686x/tw686x-video.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c
index 118e9fac9f28..1ff59084ce08 100644
--- a/drivers/media/pci/tw686x/tw686x-video.c
+++ b/drivers/media/pci/tw686x/tw686x-video.c
@@ -61,8 +61,19 @@ static unsigned int tw686x_fields_map(v4l2_std_id std, unsigned int fps)
 		   8, 8, 9, 9, 10, 10, 11, 11, 12, 12, 13, 13, 14, 0, 0
 	};
 
-	unsigned int i =
-		(std & V4L2_STD_625_50) ? std_625_50[fps] : std_525_60[fps];
+	unsigned int i;
+
+	if (std & V4L2_STD_625_50) {
+		if (unlikely(i > ARRAY_SIZE(std_625_50)))
+			i = 14;		/* 25 fps */
+		else
+			i = std_625_50[fps];
+	} else {
+		if (unlikely(i > ARRAY_SIZE(std_525_60)))
+			i = 0;		/* 30 fps */
+		else
+			i = std_525_60[fps];
+	}
 
 	return map[i];
 }
-- 
2.5.5

Re: [PATCH] [media] tvp686x: Don't go past array

[Hans Verkuil]

On 04/23/2016 11:23 AM, Mauro Carvalho Chehab wrote:
> Depending on the compiler version, currently it produces the
> following warnings:
> 	tw686x-video.c: In function 'tw686x_video_init':
> 	tw686x-video.c:65:543: warning: array subscript is above array bounds [-Warray-bounds]

I posted two patches fixing this and another issue:

https://patchwork.linuxtv.org/patch/33942/
https://patchwork.linuxtv.org/patch/33943/

I noticed that I accidentally set them to 'Accepted', so that might be
why you didn't see them.

I was planning on making a pull request for these on Monday, but you can
also take them now since Ezequiel already Acked them.

Regards,

	Hans

> 
> This is actually bogus with the current code, as it currently
> hardcodes the framerate to 30 frames/sec, however a potential
> use after the array size could happen when the driver adds support
> for setting the framerate. So, fix it.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> ---
>  drivers/media/pci/tw686x/tw686x-video.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c
> index 118e9fac9f28..1ff59084ce08 100644
> --- a/drivers/media/pci/tw686x/tw686x-video.c
> +++ b/drivers/media/pci/tw686x/tw686x-video.c
> @@ -61,8 +61,19 @@ static unsigned int tw686x_fields_map(v4l2_std_id std, unsigned int fps)
>  		   8, 8, 9, 9, 10, 10, 11, 11, 12, 12, 13, 13, 14, 0, 0
>  	};
>  
> -	unsigned int i =
> -		(std & V4L2_STD_625_50) ? std_625_50[fps] : std_525_60[fps];
> +	unsigned int i;
> +
> +	if (std & V4L2_STD_625_50) {
> +		if (unlikely(i > ARRAY_SIZE(std_625_50)))
> +			i = 14;		/* 25 fps */
> +		else
> +			i = std_625_50[fps];
> +	} else {
> +		if (unlikely(i > ARRAY_SIZE(std_525_60)))
> +			i = 0;		/* 30 fps */
> +		else
> +			i = std_525_60[fps];
> +	}
>  
>  	return map[i];
>  }
> 

Re: [PATCH] [media] tvp686x: Don't go past array

[Hans Verkuil]

Since my patch exchanges the sparse warning with a smatch warning, it's
OK to take this one, with a few corrections:

Please update the subject line (it says tvp686x instead of tw686x).

On 04/23/2016 11:23 AM, Mauro Carvalho Chehab wrote:
> Depending on the compiler version, currently it produces the
> following warnings:
> 	tw686x-video.c: In function 'tw686x_video_init':
> 	tw686x-video.c:65:543: warning: array subscript is above array bounds [-Warray-bounds]
> 
> This is actually bogus with the current code, as it currently
> hardcodes the framerate to 30 frames/sec, however a potential
> use after the array size could happen when the driver adds support
> for setting the framerate. So, fix it.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> ---
>  drivers/media/pci/tw686x/tw686x-video.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c
> index 118e9fac9f28..1ff59084ce08 100644
> --- a/drivers/media/pci/tw686x/tw686x-video.c
> +++ b/drivers/media/pci/tw686x/tw686x-video.c
> @@ -61,8 +61,19 @@ static unsigned int tw686x_fields_map(v4l2_std_id std, unsigned int fps)
>  		   8, 8, 9, 9, 10, 10, 11, 11, 12, 12, 13, 13, 14, 0, 0
>  	};
>  
> -	unsigned int i =
> -		(std & V4L2_STD_625_50) ? std_625_50[fps] : std_525_60[fps];
> +	unsigned int i;
> +
> +	if (std & V4L2_STD_625_50) {

Please test against 525_60 since that is the recommended test.

> +		if (unlikely(i > ARRAY_SIZE(std_625_50)))

Please don't use 'unlikely'. It's pointless for code that is rarely used.

Actually, the code is wrong: i is uninitialized here.

It should be fps >= ARRAY_SIZE(std_625_50).

In fact, I'd write it like this:

		i = std_625_50[(fps >= ARRAY_SIZE(std_625_50) ? 24 : fps];

> +			i = 14;		/* 25 fps */
> +		else
> +			i = std_625_50[fps];
> +	} else {
> +		if (unlikely(i > ARRAY_SIZE(std_525_60)))
> +			i = 0;		/* 30 fps */
> +		else
> +			i = std_525_60[fps];
> +	}
>  
>  	return map[i];
>  }
> 

Regards,

	Hans

Re: [PATCH] [media] tvp686x: Don't go past array

[Mauro Carvalho Chehab]

Em Mon, 25 Apr 2016 13:36:57 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:

> Since my patch exchanges the sparse warning with a smatch warning, it's
> OK to take this one, with a few corrections:
> 
> Please update the subject line (it says tvp686x instead of tw686x).

Gah...

> 
> On 04/23/2016 11:23 AM, Mauro Carvalho Chehab wrote:
> > Depending on the compiler version, currently it produces the
> > following warnings:
> > 	tw686x-video.c: In function 'tw686x_video_init':
> > 	tw686x-video.c:65:543: warning: array subscript is above array bounds [-Warray-bounds]
> > 
> > This is actually bogus with the current code, as it currently
> > hardcodes the framerate to 30 frames/sec, however a potential
> > use after the array size could happen when the driver adds support
> > for setting the framerate. So, fix it.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
> > ---
> >  drivers/media/pci/tw686x/tw686x-video.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c
> > index 118e9fac9f28..1ff59084ce08 100644
> > --- a/drivers/media/pci/tw686x/tw686x-video.c
> > +++ b/drivers/media/pci/tw686x/tw686x-video.c
> > @@ -61,8 +61,19 @@ static unsigned int tw686x_fields_map(v4l2_std_id std, unsigned int fps)
> >  		   8, 8, 9, 9, 10, 10, 11, 11, 12, 12, 13, 13, 14, 0, 0
> >  	};
> >  
> > -	unsigned int i =
> > -		(std & V4L2_STD_625_50) ? std_625_50[fps] : std_525_60[fps];
> > +	unsigned int i;
> > +
> > +	if (std & V4L2_STD_625_50) {  
> 
> Please test against 525_60 since that is the recommended test.

Both ways should work, but I'm OK with such change.

> 
> > +		if (unlikely(i > ARRAY_SIZE(std_625_50)))  
> 
> Please don't use 'unlikely'. It's pointless for code that is rarely used.

OK.

> 
> Actually, the code is wrong: i is uninitialized here.
> 
> It should be fps >= ARRAY_SIZE(std_625_50).
> 
> In fact, I'd write it like this:
> 
> 		i = std_625_50[(fps >= ARRAY_SIZE(std_625_50) ? 24 : fps];

I really don't like the above, as it has an unexplained magic
number on it. Also, "24" is wrong there.

So, I would go to the following enclosed patch.

Ezequiel,

Btw, I'm not seeing support for fps != 25 (or 30 fps) on this driver.
As the device seems to support setting the fps, you should be adding
support on it for VIDIOC_S_PARM and VIDIOC_G_PARM.

On both ioctls, the driver should return the actual framerate used.
So, you'll need to add a code that would convert from the 15 possible
framerate converter register settings to v4l2_fract.

> 
> > +			i = 14;		/* 25 fps */
> > +		else
> > +			i = std_625_50[fps];
> > +	} else {
> > +		if (unlikely(i > ARRAY_SIZE(std_525_60)))
> > +			i = 0;		/* 30 fps */
> > +		else
> > +			i = std_525_60[fps];
> > +	}
> >  
> >  	return map[i];
> >  }
> >   
> 
> Regards,
> 
> 	Hans

Thanks,
Mauro

-

[media] tw686x: Don't go past array

Depending on the compiler version, currently it produces the
following warnings:
	tw686x-video.c: In function 'tw686x_video_init':
	tw686x-video.c:65:543: warning: array subscript is above array bounds [-Warray-bounds]

This is actually bogus with the current code, as it currently
hardcodes the framerate to 30 frames/sec, however a potential
use after the array size could happen when the driver adds support
for setting the framerate. So, fix it.

Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c
index 118e9fac9f28..9468fda69f3d 100644
--- a/drivers/media/pci/tw686x/tw686x-video.c
+++ b/drivers/media/pci/tw686x/tw686x-video.c
@@ -61,8 +61,17 @@ static unsigned int tw686x_fields_map(v4l2_std_id std, unsigned int fps)
 		   8, 8, 9, 9, 10, 10, 11, 11, 12, 12, 13, 13, 14, 0, 0
 	};
 
-	unsigned int i =
-		(std & V4L2_STD_625_50) ? std_625_50[fps] : std_525_60[fps];
+	unsigned int i;
+
+	if (std & V4L2_STD_525_60) {
+		if (fps > ARRAY_SIZE(std_525_60))
+			fps = 30;
+		i = std_525_60[fps];
+	} else {
+		if (fps > ARRAY_SIZE(std_625_50))
+			fps = 25;
+		i = std_625_50[fps];
+	}
 
 	return map[i];
 }

Re: [PATCH] [media] tvp686x: Don't go past array

[Hans Verkuil]

On 04/25/2016 02:40 PM, Mauro Carvalho Chehab wrote:
> Em Mon, 25 Apr 2016 13:36:57 +0200
> Hans Verkuil <hverkuil@xs4all.nl> escreveu:
> 
>> Since my patch exchanges the sparse warning with a smatch warning, it's
>> OK to take this one, with a few corrections:
>>
>> Please update the subject line (it says tvp686x instead of tw686x).
> 
> Gah...
> 
>>
>> On 04/23/2016 11:23 AM, Mauro Carvalho Chehab wrote:
>>> Depending on the compiler version, currently it produces the
>>> following warnings:
>>> 	tw686x-video.c: In function 'tw686x_video_init':
>>> 	tw686x-video.c:65:543: warning: array subscript is above array bounds [-Warray-bounds]
>>>
>>> This is actually bogus with the current code, as it currently
>>> hardcodes the framerate to 30 frames/sec, however a potential
>>> use after the array size could happen when the driver adds support
>>> for setting the framerate. So, fix it.
>>>
>>> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>>> ---
>>>  drivers/media/pci/tw686x/tw686x-video.c | 15 +++++++++++++--
>>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c
>>> index 118e9fac9f28..1ff59084ce08 100644
>>> --- a/drivers/media/pci/tw686x/tw686x-video.c
>>> +++ b/drivers/media/pci/tw686x/tw686x-video.c
>>> @@ -61,8 +61,19 @@ static unsigned int tw686x_fields_map(v4l2_std_id std, unsigned int fps)
>>>  		   8, 8, 9, 9, 10, 10, 11, 11, 12, 12, 13, 13, 14, 0, 0
>>>  	};
>>>  
>>> -	unsigned int i =
>>> -		(std & V4L2_STD_625_50) ? std_625_50[fps] : std_525_60[fps];
>>> +	unsigned int i;
>>> +
>>> +	if (std & V4L2_STD_625_50) {  
>>
>> Please test against 525_60 since that is the recommended test.
> 
> Both ways should work, but I'm OK with such change.
> 
>>
>>> +		if (unlikely(i > ARRAY_SIZE(std_625_50)))  
>>
>> Please don't use 'unlikely'. It's pointless for code that is rarely used.
> 
> OK.
> 
>>
>> Actually, the code is wrong: i is uninitialized here.
>>
>> It should be fps >= ARRAY_SIZE(std_625_50).
>>
>> In fact, I'd write it like this:
>>
>> 		i = std_625_50[(fps >= ARRAY_SIZE(std_625_50) ? 24 : fps];
> 
> I really don't like the above, as it has an unexplained magic
> number on it. Also, "24" is wrong there.
> 
> So, I would go to the following enclosed patch.

Looks good to me. Acked below. Amazing how many bugs one can make in one
simple patch...

> 
> Ezequiel,
> 
> Btw, I'm not seeing support for fps != 25 (or 30 fps) on this driver.
> As the device seems to support setting the fps, you should be adding
> support on it for VIDIOC_S_PARM and VIDIOC_G_PARM.
> 
> On both ioctls, the driver should return the actual framerate used.
> So, you'll need to add a code that would convert from the 15 possible
> framerate converter register settings to v4l2_fract.
> 
>>
>>> +			i = 14;		/* 25 fps */
>>> +		else
>>> +			i = std_625_50[fps];
>>> +	} else {
>>> +		if (unlikely(i > ARRAY_SIZE(std_525_60)))
>>> +			i = 0;		/* 30 fps */
>>> +		else
>>> +			i = std_525_60[fps];
>>> +	}
>>>  
>>>  	return map[i];
>>>  }
>>>   
>>
>> Regards,
>>
>> 	Hans
> 
> Thanks,
> Mauro
> 
> -
> 
> [media] tw686x: Don't go past array
> 
> Depending on the compiler version, currently it produces the
> following warnings:
> 	tw686x-video.c: In function 'tw686x_video_init':
> 	tw686x-video.c:65:543: warning: array subscript is above array bounds [-Warray-bounds]
> 
> This is actually bogus with the current code, as it currently
> hardcodes the framerate to 30 frames/sec, however a potential
> use after the array size could happen when the driver adds support
> for setting the framerate. So, fix it.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

> 
> diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c
> index 118e9fac9f28..9468fda69f3d 100644
> --- a/drivers/media/pci/tw686x/tw686x-video.c
> +++ b/drivers/media/pci/tw686x/tw686x-video.c
> @@ -61,8 +61,17 @@ static unsigned int tw686x_fields_map(v4l2_std_id std, unsigned int fps)
>  		   8, 8, 9, 9, 10, 10, 11, 11, 12, 12, 13, 13, 14, 0, 0
>  	};
>  
> -	unsigned int i =
> -		(std & V4L2_STD_625_50) ? std_625_50[fps] : std_525_60[fps];
> +	unsigned int i;
> +
> +	if (std & V4L2_STD_525_60) {
> +		if (fps > ARRAY_SIZE(std_525_60))
> +			fps = 30;
> +		i = std_525_60[fps];
> +	} else {
> +		if (fps > ARRAY_SIZE(std_625_50))
> +			fps = 25;
> +		i = std_625_50[fps];
> +	}
>  
>  	return map[i];
>  }
> 
> 
> --
> 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: [PATCH] [media] tvp686x: Don't go past array

[Mauro Carvalho Chehab]

Em Mon, 25 Apr 2016 14:42:31 +0200
Hans Verkuil <hverkuil@xs4all.nl> escreveu:


> > So, I would go to the following enclosed patch.  
> 
> Looks good to me. Acked below. Amazing how many bugs one can make in one
> simple patch...

Applied, thanks!

Yeah, simple patches are harder than complex ones ;)

> 
> > 
> > Ezequiel,
> > 
> > Btw, I'm not seeing support for fps != 25 (or 30 fps) on this driver.
> > As the device seems to support setting the fps, you should be adding
> > support on it for VIDIOC_S_PARM and VIDIOC_G_PARM.
> > 
> > On both ioctls, the driver should return the actual framerate used.
> > So, you'll need to add a code that would convert from the 15 possible
> > framerate converter register settings to v4l2_fract.
> >   
> >>  
> >>> +			i = 14;		/* 25 fps */
> >>> +		else
> >>> +			i = std_625_50[fps];
> >>> +	} else {
> >>> +		if (unlikely(i > ARRAY_SIZE(std_525_60)))
> >>> +			i = 0;		/* 30 fps */
> >>> +		else
> >>> +			i = std_525_60[fps];
> >>> +	}
> >>>  
> >>>  	return map[i];
> >>>  }
> >>>     
> >>
> >> Regards,
> >>
> >> 	Hans  
> > 
> > Thanks,
> > Mauro
> > 
> > -
> > 
> > [media] tw686x: Don't go past array
> > 
> > Depending on the compiler version, currently it produces the
> > following warnings:
> > 	tw686x-video.c: In function 'tw686x_video_init':
> > 	tw686x-video.c:65:543: warning: array subscript is above array bounds [-Warray-bounds]
> > 
> > This is actually bogus with the current code, as it currently
> > hardcodes the framerate to 30 frames/sec, however a potential
> > use after the array size could happen when the driver adds support
> > for setting the framerate. So, fix it.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>  
> 
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> > 
> > diff --git a/drivers/media/pci/tw686x/tw686x-video.c b/drivers/media/pci/tw686x/tw686x-video.c
> > index 118e9fac9f28..9468fda69f3d 100644
> > --- a/drivers/media/pci/tw686x/tw686x-video.c
> > +++ b/drivers/media/pci/tw686x/tw686x-video.c
> > @@ -61,8 +61,17 @@ static unsigned int tw686x_fields_map(v4l2_std_id std, unsigned int fps)
> >  		   8, 8, 9, 9, 10, 10, 11, 11, 12, 12, 13, 13, 14, 0, 0
> >  	};
> >  
> > -	unsigned int i =
> > -		(std & V4L2_STD_625_50) ? std_625_50[fps] : std_525_60[fps];
> > +	unsigned int i;
> > +
> > +	if (std & V4L2_STD_525_60) {
> > +		if (fps > ARRAY_SIZE(std_525_60))
> > +			fps = 30;
> > +		i = std_525_60[fps];
> > +	} else {
> > +		if (fps > ARRAY_SIZE(std_625_50))
> > +			fps = 25;
> > +		i = std_625_50[fps];
> > +	}
> >  
> >  	return map[i];
> >  }
> > 
> > 
> > --
> > 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
> >   
> 


-- 
Thanks,
Mauro

Re: [PATCH] [media] tvp686x: Don't go past array

[Ezequiel Garcia]

Hi Mauro, Hans:

Thanks for the fixes to this driver :-)

On 25 Apr 09:40 AM, Mauro Carvalho Chehab wrote:
> Ezequiel,
> 
> Btw, I'm not seeing support for fps != 25 (or 30 fps) on this driver.
> As the device seems to support setting the fps, you should be adding
> support on it for VIDIOC_S_PARM and VIDIOC_G_PARM.
> 
> On both ioctls, the driver should return the actual framerate used.
> So, you'll need to add a code that would convert from the 15 possible
> framerate converter register settings to v4l2_fract.
> 

OK, thanks for the information.

In fact, framerate support is implemented in the driver that is in
production.

Support for s_parm, g_parm was in the original submission [1]
but we removed it later [2] because we thought it was unused.
I can't see how we came to that conclusion, since it is actually
used to set the framerate!

Anyway, since we are discussing this, I would like to know if
having s_parm/g_parm is enough for framerate setting support.

When I implemented this a year ago, the v4l2src gstreamer plugin
seemed to require enum_frame_size and enum_frame_interval as well.
It didn't make much sense, but I ended up implementing them
to get it to work. Should that be required?

(To be honest, v4lsrc is quite picky regarding parameters,
so it wouldn't be that surprising if it needs some love).

Thanks!

[1] http://www.spinics.net/lists/linux-media/msg95953.html
[2] http://www.spinics.net/lists/linux-media/msg96503.html
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH] [media] tvp686x: Don't go past array

[Mauro Carvalho Chehab]

Em Mon, 25 Apr 2016 12:26:40 -0300
Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> escreveu:

> Hi Mauro, Hans:
> 
> Thanks for the fixes to this driver :-)
> 
> On 25 Apr 09:40 AM, Mauro Carvalho Chehab wrote:
> > Ezequiel,
> > 
> > Btw, I'm not seeing support for fps != 25 (or 30 fps) on this driver.
> > As the device seems to support setting the fps, you should be adding
> > support on it for VIDIOC_S_PARM and VIDIOC_G_PARM.
> > 
> > On both ioctls, the driver should return the actual framerate used.
> > So, you'll need to add a code that would convert from the 15 possible
> > framerate converter register settings to v4l2_fract.
> >   
> 
> OK, thanks for the information.
> 
> In fact, framerate support is implemented in the driver that is in
> production.
> 
> Support for s_parm, g_parm was in the original submission [1]
> but we removed it later [2] because we thought it was unused.

hmm... from [1], the support were provided by:

+static void tw686x_set_framerate(struct tw686x_video_channel *vc,
+				 unsigned int fps)
+{
+	unsigned int map;
+
+	if (vc->fps == fps)
+		return;
+
+	map = tw686x_fields_map(vc->video_standard, fps) << 1;
+	map |= map << 1;
+	if (map > 0)
+		map |= BIT(31);
+	reg_write(vc->dev, VIDEO_FIELD_CTRL[vc->ch], map);
+	vc->fps = fps;
+}

+static int tw686x_g_parm(struct file *file, void *priv,
+			 struct v4l2_streamparm *sp)
+{
+	struct tw686x_video_channel *vc = video_drvdata(file);
+	struct v4l2_captureparm *cp = &sp->parm.capture;
+
+	if (sp->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+		return -EINVAL;
+	sp->parm.capture.readbuffers = 3;
+
+	cp->capability = V4L2_CAP_TIMEPERFRAME;
+	cp->timeperframe.numerator = 1;
+	cp->timeperframe.denominator = vc->fps;
+	return 0;
+}

+static int tw686x_s_parm(struct file *file, void *priv,
+			 struct v4l2_streamparm *sp)
+{
+	struct tw686x_video_channel *vc = video_drvdata(file);
+	struct v4l2_captureparm *cp = &sp->parm.capture;
+	unsigned int denominator = cp->timeperframe.denominator;
+	unsigned int numerator = cp->timeperframe.numerator;
+	unsigned int fps;
+
+	if (vb2_is_busy(&vc->vidq))
+		return -EBUSY;
+
+	fps = (!numerator || !denominator) ? 0 : denominator / numerator;
+	if (vc->video_standard & V4L2_STD_625_50)
+		fps = (!fps || fps > 25) ? 25 : fps;
+	else
+		fps = (!fps || fps > 30) ? 30 : fps;
+	tw686x_set_framerate(vc, fps);
+
+	return tw686x_g_parm(file, priv, sp);
+}

Basically, s_parm just stores the fps received from the user and
g_parm just returns 1/fps. The only validation it does is to avoid
fps == 0 or fps > max_fps. This is not what the API docbook states.
It should, instead, return the actual framerate that it was
programmed on the hardware.

Looking at the code, it is not returning the actual framerate, as
the framerate seems to have only 15 possible values,
from 0 (meaning no temporal decimation - e. g. just use the
standard default) to 14:

static unsigned int tw686x_fields_map(v4l2_std_id std, unsigned int fps)
{
	static const unsigned int map[15] = {
		0x00000000, 0x00000001, 0x00004001, 0x00104001, 0x00404041,
		0x01041041, 0x01104411, 0x01111111, 0x04444445, 0x04511445,
		0x05145145, 0x05151515, 0x05515455, 0x05551555, 0x05555555
	};

	static const unsigned int std_625_50[26] = {
		0, 1, 1, 2,  3,  3,  4,  4,  5,  5,  6,  7,  7,
		   8, 8, 9, 10, 10, 11, 11, 12, 13, 13, 14, 14, 0
	};

	static const unsigned int std_525_60[31] = {
		0, 1, 1, 1, 2,  2,  3,  3,  4,  4,  5,  5,  6,  6, 7, 7,
		   8, 8, 9, 9, 10, 10, 11, 11, 12, 12, 13, 13, 14, 0, 0
	};

	unsigned int i;

	if (std & V4L2_STD_525_60) {
		if (fps > ARRAY_SIZE(std_525_60))
			fps = 30;
		i = std_525_60[fps];
	} else {
		if (fps > ARRAY_SIZE(std_625_50))
			fps = 25;
		i = std_625_50[fps];
	}

	return map[i];
}

>From the above, clearly it uses the same frame rate if "fps" var is
set to 1 to 2 (on 60Hz) and 1 to 3 (on 50 Hz).

What *I* suspect from the above is that the code setting a
temporal decimation register to zero (i = 0 -> map = 0x00000000)
in order to disable the temporal decimation;

And when "i" var is between 1 to 14, the temporal decimation block is
enabled and the real frame rate is given by:

	vc->real_fps = (fps * i) / 15

So, the driver should actually store the value of "i" and use it
when setting the framerate.

Btw, in the 60 Hz case, usually the fps is not 30 Hz, but,
instead, 30000/1001.

So, I guess the code at s_parm should be doing something like:

	if (std & V4L2_STD_525_60) {
		cp->timeperframe.numerator = 1001;
		cp->timeperframe.denominator = vc->real_fps * 1000;
	} else {
		cp->timeperframe.numerator = 1;
		cp->timeperframe.denominator = vc->real_fps;
	}


> I can't see how we came to that conclusion, since it is actually
> used to set the framerate!
> 
> Anyway, since we are discussing this, I would like to know if
> having s_parm/g_parm is enough for framerate setting support.

Yes.

> When I implemented this a year ago, the v4l2src gstreamer plugin
> seemed to require enum_frame_size and enum_frame_interval as well.
> It didn't make much sense, but I ended up implementing them
> to get it to work. Should that be required?
> 
> (To be honest, v4lsrc is quite picky regarding parameters,
> so it wouldn't be that surprising if it needs some love).

That sounds like a problem at v4l2src. You should talk with
gst developers if this is still an issue there.

I don't mind if you implement those two ioctls as well.

Regards,
Mauro