Hi Laurent, Jacopo, Sam,
I'm also OK to change to a simpler alternative;
- drop the "restore" step
- send the whole init register sequence + mode changes + format changes
at streamon
is this what you have in mind Laurent ?
On 10/10/2018 02:41 PM, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wednesday, 10 October 2018 13:58:04 EEST jacopo mondi wrote:
>> Hi Sam,
>> thanks for the patch, I see the same issue you reported, but I
>> think this patch can be improved.
>>
>> (expanding the Cc list to all people involved in recent ov5640
>> developemts, not just for this patch, but for the whole series to look
>> at. Copying names from another series cover letter, hope it is
>> complete.)
>>
>> On Mon, Oct 08, 2018 at 11:47:59PM -0700, Sam Bobrowicz wrote:
>>> set_fmt was not properly triggering a mode change when
>>> a new mode was set that happened to have the same format
>>> as the previous mode (for example, when only changing the
>>> frame dimensions). Fix this.
>>>
>>> Signed-off-by: Sam Bobrowicz <sam@xxxxxxxxxxxxxxxxxx>
>>> ---
>>>
>>> drivers/media/i2c/ov5640.c | 8 ++++----
>>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>>> index eaefdb5..5031aab 100644
>>> --- a/drivers/media/i2c/ov5640.c
>>> +++ b/drivers/media/i2c/ov5640.c
>>> @@ -2045,12 +2045,12 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
>>>
>>> goto out;
>>>
>>> }
>>>
>>> - if (new_mode != sensor->current_mode) {
>>> +
>>> + if (new_mode != sensor->current_mode ||
>>> + mbus_fmt->code != sensor->fmt.code) {
>>> + sensor->fmt = *mbus_fmt;
>>>
>>> sensor->current_mode = new_mode;
>>> sensor->pending_mode_change = true;
>>>
>>> - }
>>> - if (mbus_fmt->code != sensor->fmt.code) {
>>> - sensor->fmt = *mbus_fmt;
>>>
>>> sensor->pending_fmt_change = true;
>>>
>>> }
>>
>> How I did reproduce the issue:
>>
>> # Set 1024x768 on ov5640 without changing the image format
>> # (default image size at startup is 640x480)
>> $ media-ctl --set-v4l2 "'ov5640 2-003c':0[fmt:UYVY2X8/1024x768 field:none]"
>> sensor->pending_mode_change = true; //verified this flag gets set
>>
>> # Start streaming, after having configured the whole pipeline to work
>> # with 1024x768
>> $ yavta -c10 -n4 -f UYVY -s 1024x768 /dev/video4
>> Unable to start streaming: Broken pipe (32).
>>
>> # Inspect which part of pipeline validation went wrong
>> # Turns out the sensor->fmt field is not updated, and when get_fmt()
>> # is called, the old one is returned.
>> $ media-ctl -e "ov5640 2-003c" -p
>> ...
>> [fmt:UYVY8_2X8/640x480@1/30 field:none colorspace:srgb xfer:srgb ycbcr:601
>> quantization:full-range] ^^^ ^^^
>>
>> So yes, sensor->fmt is not udapted as it should be when only image
>> resolution is changed.
>>
>> Although I still see value in having two separate flags for the
>> 'mode_change' (which in ov5640 lingo is resolution) and 'fmt_change' (which
>> in ov5640 lingo is the image format), and write their configuration to
>> registers only when they get actually changed.
>>
>> For this reasons I would like to propse the following patch which I
>> have tested by:
>> 1) changing resolution only
>> 2) changing format only
>> 3) change both
>>
>> What do you and others think?
>
> I think that the format setting code should be completely rewritten, it's
> pretty much unmaintainable as-is.
>
>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>> index eaefdb5..e392b9d 100644
>> --- a/drivers/media/i2c/ov5640.c
>> +++ b/drivers/media/i2c/ov5640.c
>> @@ -2020,6 +2020,7 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
>> struct ov5640_dev *sensor = to_ov5640_dev(sd);
>> const struct ov5640_mode_info *new_mode;
>> struct v4l2_mbus_framefmt *mbus_fmt = &format->format;
>> + struct v4l2_mbus_framefmt *fmt;
>> int ret;
>>
>> if (format->pad != 0)
>> @@ -2037,22 +2038,19 @@ static int ov5640_set_fmt(struct v4l2_subdev *sd,
>> if (ret)
>> goto out;
>>
>> - if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
>> - struct v4l2_mbus_framefmt *fmt =
>> - v4l2_subdev_get_try_format(sd, cfg, 0);
>> + if (format->which == V4L2_SUBDEV_FORMAT_TRY)
>> + fmt = v4l2_subdev_get_try_format(sd, cfg, 0);
>> + else
>> + fmt = &sensor->fmt;
>>
>> - *fmt = *mbus_fmt;
>> - goto out;
>> - }
>> + *fmt = *mbus_fmt;
>>
>> if (new_mode != sensor->current_mode) {
>> sensor->current_mode = new_mode;
>> sensor->pending_mode_change = true;
>> }
>> - if (mbus_fmt->code != sensor->fmt.code) {
>> - sensor->fmt = *mbus_fmt;
>> + if (mbus_fmt->code != sensor->fmt.code)
>> sensor->pending_fmt_change = true;
>> - }
>> out:
>> mutex_unlock(&sensor->lock);
>> return ret;
>>
>>> out:
>>> --
>>> 2.7.4
>
>
BR,
Hugues.
