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@xxxxxxxxx>
>> ---
>> 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
