Glad this is spurring a lot of conversation, and I’m happy to see this many
contributors too. I think we have all solved many of these problems (and the
many others) offline, and now it’s the hard part to try to glue them all
together. I decided to jump back in the mix with these patches because they
seemed that they would be the easiest to rebase and were obvious bug fixes. I
was hoping we could get this one applied and have a better starting place to
work from before we start what is a very necessary restructure of the mode
setting, but if everybody is ready to go now, we can just do a larger overhaul
and I can withdraw this patch from this series. I expect the larger overhaul to
warrant its own patch series.
I’m not going to comment yet on some of the proposed changes below, because I
think it is better if we just move the discussion to its own series.
Another note, I also agree that prior to changing the rest of mode setting, the
next step is to introduce maximes critical pll setting patch. It is required
for my platform to work at all (yes my platform has never worked with main
line) because my CSI2 receiver needs the PCLK PERIOD register to be set
properly. I actually have tweaked Maximes original patch so that it doesn’t
introduce a regression for MIPI platforms, but it will need to be tested on DVP
platforms. I’ll send it out RFC tonight, so hopefully that will get us closer
to a single patch that doesn’t break things in either mode.
IMO I would like to see this patch broken out of Maximes larger series, because
I think it is getting bogged down by the many other changes there.
With this many interested parties, someone would really be helping if they put
out a organized to do list that we could schedule out :)
> On Oct 16, 2018, at 5:15 AM, Laurent Pinchart
> <laurent.pinch...@ideasonboard.com> wrote:
>
> Hi Hugues,
>
>> On Monday, 15 October 2018 18:13:12 EEST Hugues FRUCHET wrote:
>> 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 ?
>
> Yes, that's pretty much the idea. The init sequence could be sent when
> powering the sensor on to save time at streamon. Everything else can be
> programmed at streamon time to simplify the implementation.
>
>>> On 10/10/2018 02:41 PM, Laurent Pinchart wrote:
>>>> 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 <s...@elite-embedded.com>
>>>>> ---
>>>>>
>>>>> 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:
>
> --
> Regards,
>
> Laurent Pinchart
>
>
>
