imx274: fix frame interval handling
diff mbox series

Message ID 9f73f763-3b1f-bf18-0b4e-b69cfa22620b@xs4all.nl
State New
Headers show
Series
  • imx274: fix frame interval handling
Related show

Commit Message

Hans Verkuil July 2, 2020, 1:52 p.m. UTC
1) the numerator and/or denominator might be 0, in that case
   fall back to the default frame interval. This is per the spec
   and this caused a v4l2-compliance failure.

2) the updated frame interval wasn't returned in the s_frame_interval
   subdev op.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/i2c/imx274.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

Comments

Luca Ceresoli July 3, 2020, 8:46 a.m. UTC | #1
Hi Hans,

[Cc: ing the imx274 maintainer]

On 02/07/20 15:52, Hans Verkuil wrote:
> 1) the numerator and/or denominator might be 0, in that case
>    fall back to the default frame interval. This is per the spec
>    and this caused a v4l2-compliance failure.
> 
> 2) the updated frame interval wasn't returned in the s_frame_interval
>    subdev op.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/i2c/imx274.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> index 6011cec5e351..a9304b98f7af 100644
> --- a/drivers/media/i2c/imx274.c
> +++ b/drivers/media/i2c/imx274.c
> @@ -1235,6 +1235,8 @@ static int imx274_s_frame_interval(struct v4l2_subdev *sd,
>  	ret = imx274_set_frame_interval(imx274, fi->interval);
> 
>  	if (!ret) {
> +		fi->interval = imx274->frame_interval;
> +
>  		/*
>  		 * exposure time range is decided by frame interval
>  		 * need to update it after frame interval changes
> @@ -1730,9 +1732,9 @@ static int imx274_set_frame_interval(struct stimx274 *priv,
>  		__func__, frame_interval.numerator,
>  		frame_interval.denominator);
> 
> -	if (frame_interval.numerator == 0) {
> -		err = -EINVAL;
> -		goto fail;
> +	if (frame_interval.numerator == 0 || frame_interval.denominator) {

Missing '== 0'?

Please excuse my noobness, but I'm unable to understand which code path
would lead to calling imx274_set_frame_interval() with a zero here. I'm
assuming the v4l2 framework won't call imx274_s_frame_interval() with
numerator or denominator set to zero.
Hans Verkuil July 3, 2020, 9:12 a.m. UTC | #2
On 03/07/2020 10:46, Luca Ceresoli wrote:
> Hi Hans,
> 
> [Cc: ing the imx274 maintainer]
> 
> On 02/07/20 15:52, Hans Verkuil wrote:
>> 1) the numerator and/or denominator might be 0, in that case
>>    fall back to the default frame interval. This is per the spec
>>    and this caused a v4l2-compliance failure.
>>
>> 2) the updated frame interval wasn't returned in the s_frame_interval
>>    subdev op.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>>  drivers/media/i2c/imx274.c | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
>> index 6011cec5e351..a9304b98f7af 100644
>> --- a/drivers/media/i2c/imx274.c
>> +++ b/drivers/media/i2c/imx274.c
>> @@ -1235,6 +1235,8 @@ static int imx274_s_frame_interval(struct v4l2_subdev *sd,
>>  	ret = imx274_set_frame_interval(imx274, fi->interval);
>>
>>  	if (!ret) {
>> +		fi->interval = imx274->frame_interval;
>> +
>>  		/*
>>  		 * exposure time range is decided by frame interval
>>  		 * need to update it after frame interval changes
>> @@ -1730,9 +1732,9 @@ static int imx274_set_frame_interval(struct stimx274 *priv,
>>  		__func__, frame_interval.numerator,
>>  		frame_interval.denominator);
>>
>> -	if (frame_interval.numerator == 0) {
>> -		err = -EINVAL;
>> -		goto fail;
>> +	if (frame_interval.numerator == 0 || frame_interval.denominator) {
> 
> Missing '== 0'?

Oops!

> 
> Please excuse my noobness, but I'm unable to understand which code path
> would lead to calling imx274_set_frame_interval() with a zero here. I'm
> assuming the v4l2 framework won't call imx274_s_frame_interval() with
> numerator or denominator set to zero.
> 

A bridge driver that is called with VIDIOC_S_PARM will just pass on the new
interval to the sensor. And that interval can have either numerator and/or
denominator set to 0 in which case the sensor driver should set the frame
interval to a suitable default value (as per the spec). The bridge driver
doesn't know what a suitable default value is, so this has to be done in the
sensor driver.

I found this when running v4l2-compliance with the imx274 and the upcoming tegra
video input driver.

Regards,

	Hans
Luca Ceresoli July 3, 2020, 9:50 a.m. UTC | #3
Hi Hans,

On 03/07/20 11:12, Hans Verkuil wrote:
> On 03/07/2020 10:46, Luca Ceresoli wrote:
>> Hi Hans,
>>
>> [Cc: ing the imx274 maintainer]
>>
>> On 02/07/20 15:52, Hans Verkuil wrote:
>>> 1) the numerator and/or denominator might be 0, in that case
>>>    fall back to the default frame interval. This is per the spec
>>>    and this caused a v4l2-compliance failure.
>>>
>>> 2) the updated frame interval wasn't returned in the s_frame_interval
>>>    subdev op.
>>>
>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>> ---
>>>  drivers/media/i2c/imx274.c | 8 +++++---
>>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
>>> index 6011cec5e351..a9304b98f7af 100644
>>> --- a/drivers/media/i2c/imx274.c
>>> +++ b/drivers/media/i2c/imx274.c
>>> @@ -1235,6 +1235,8 @@ static int imx274_s_frame_interval(struct v4l2_subdev *sd,
>>>  	ret = imx274_set_frame_interval(imx274, fi->interval);
>>>
>>>  	if (!ret) {
>>> +		fi->interval = imx274->frame_interval;
>>> +
>>>  		/*
>>>  		 * exposure time range is decided by frame interval
>>>  		 * need to update it after frame interval changes
>>> @@ -1730,9 +1732,9 @@ static int imx274_set_frame_interval(struct stimx274 *priv,
>>>  		__func__, frame_interval.numerator,
>>>  		frame_interval.denominator);
>>>
>>> -	if (frame_interval.numerator == 0) {
>>> -		err = -EINVAL;
>>> -		goto fail;
>>> +	if (frame_interval.numerator == 0 || frame_interval.denominator) {
>>
>> Missing '== 0'?
> 
> Oops!
> 
>>
>> Please excuse my noobness, but I'm unable to understand which code path
>> would lead to calling imx274_set_frame_interval() with a zero here. I'm
>> assuming the v4l2 framework won't call imx274_s_frame_interval() with
>> numerator or denominator set to zero.
>>
> 
> A bridge driver that is called with VIDIOC_S_PARM will just pass on the new
> interval to the sensor. And that interval can have either numerator and/or
> denominator set to 0 in which case the sensor driver should set the frame
> interval to a suitable default value (as per the spec). The bridge driver
> doesn't know what a suitable default value is, so this has to be done in the
> sensor driver.

Thanks for the explanation Hans. I was assuming the framework would
check sanity when passing calls between drivers, but now I checked the
v4l2_subdev_call() and it clearly does just pass the call through.

Patch
diff mbox series

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 6011cec5e351..a9304b98f7af 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -1235,6 +1235,8 @@  static int imx274_s_frame_interval(struct v4l2_subdev *sd,
 	ret = imx274_set_frame_interval(imx274, fi->interval);

 	if (!ret) {
+		fi->interval = imx274->frame_interval;
+
 		/*
 		 * exposure time range is decided by frame interval
 		 * need to update it after frame interval changes
@@ -1730,9 +1732,9 @@  static int imx274_set_frame_interval(struct stimx274 *priv,
 		__func__, frame_interval.numerator,
 		frame_interval.denominator);

-	if (frame_interval.numerator == 0) {
-		err = -EINVAL;
-		goto fail;
+	if (frame_interval.numerator == 0 || frame_interval.denominator) {
+		frame_interval.denominator = IMX274_DEF_FRAME_RATE;
+		frame_interval.numerator = 1;
 	}

 	req_frame_rate = (u32)(frame_interval.denominator