Hi Laurent,
On 09/10/2018 10:56 PM, Laurent Pinchart wrote:
> Hi Hugues,
>
> On Monday, 10 September 2018 18:14:45 EEST Hugues FRUCHET wrote:
>> On 09/07/2018 04:18 PM, Laurent Pinchart wrote:
>>> On Thursday, 16 August 2018 18:07:54 EEST Hugues FRUCHET wrote:
>>>> On 08/16/2018 12:10 PM, jacopo mondi wrote:
>>>>> On Mon, Aug 13, 2018 at 12:19:46PM +0200, Hugues Fruchet wrote:
>>>>>
>>>>>> Mode setting depends on last mode set, in particular
>>>>>> because of exposure calculation when downscale mode
>>>>>> change between subsampling and scaling.
>>>>>> At stream on the last mode was wrongly set to current mode,
>>>>>> so no change was detected and exposure calculation
>>>>>> was not made, fix this.
>>>>>
>>>>> I actually see a different issue here...
>>>>
>>>> Which problem do you have exactly, you got a VGA JPEG instead of a QVGA
>>>> YUYV ?
>>>>
>>>>> The issue I see here depends on the format programmed through
>>>>> set_fmt() never being applied when using the sensor with a media
>>>>> controller equipped device (in this case an i.MX6 board) through
>>>>> capture sessions, and the not properly calculated exposure you see may
>>>>> be a consequence of this.
>>>>>
>>>>> I'll try to write down what I see, with the help of some debug output.
>>>>>
>>>>> - At probe time mode 640x460@30 is programmed:
>>>>>
>>>>> [ 1.651216] ov5640_probe: Initial mode with id: 2
>>>>>
>>>>> - I set the format on the sensor's pad and it gets not applied but
>>>>> marked as pending as the sensor is powered off:
> >>>
>>>>> #media-ctl --set-v4l2 "'ov5640 2-003c':0[fmt:UYVY2X8/320x240
>>>>> field:none]"
>>>>> [ 65.611983] ov5640_set_fmt: NEW mode with id: 1 - PENDING
>>>>
>>>> So here sensor->current_mode is set to <1>;//QVGA
>>>> and sensor->pending_mode_change is set to true;
>>>>
>>>>> - I start streaming with yavta, and the sensor receives a power on;
>>>>> this causes the 'initial' format to be re-programmed and the
>>>>> pending change to be ignored:
>>>>>
>>>>> #yavta -c10 -n4 -f YUYV -s $320x240 -F"../frame-#.yuv" /dev/video4
>>>>>
>>>>> [ 69.395018] ov5640_set_power:1805 - on
>>>>> [ 69.431342] ov5640_restore_mode:1711
>>>>> [ 69.996882] ov5640_set_mode: Apply mode with id: 0
>>>>>
>>>>> The 'ov5640_set_mode()' call from 'ov5640_restore_mode()' clears
>>>>> the sensor->pending flag, discarding the newly requested format, for
>>>>> this reason, at s_stream() time, the pending flag is not set
>>>>> anymore.
>>>>
>>>> OK but before clearing sensor->pending_mode_change, set_mode() is
>>>> loading registers corresponding to sensor->current_mode:
>>>>
>>>> static int ov5640_set_mode(struct ov5640_dev *sensor,
>>>> const struct ov5640_mode_info *orig_mode)
>>>> {
>>>> ==> const struct ov5640_mode_info *mode = sensor->current_mode;
>>>> ...
>>>> ret = ov5640_set_mode_direct(sensor, mode, exposure);
>>>>
>>>> => so mode <1> is expected to be set now, so I don't understand your
>>>> trace:
>>>> "> [ 69.996882] ov5640_set_mode: Apply mode with id: 0"
>>>> Which variable do you trace that shows "0" ?
>>>>
>>>>> Are you using a media-controller system? I suspect in non-mc cases,
>>>>> the set_fmt is applied through a single power_on/power_off session, not
>>>>> causing the 'restore_mode()' issue. Is this the case for you or your
>>>>> issue is differnt?
>>>>>
>>>>> Edit:
>>>>> Mita-san tried to address the issue of the output pixel format not
>>>>> being restored when the image format was restored in
>>>>> 19ad26f9e6e1 ("media: ov5640: add missing output pixel format setting")
>>>>>
>>>>> I understand the issue he tried to fix, but shouldn't the pending
>>>>> format (if any) be applied instead of the initial one unconditionally?
>>>>
>>>> This is what does the ov5640_restore_mode(), set the current mode
>>>> (sensor->current_mode), that is done through this line:
>>>>
>>>> /* now restore the last capture mode */
>>>> ret = ov5640_set_mode(sensor, &ov5640_mode_init_data);
>>>>
>>>> => note that the comment above is weird, in fact it is the "current"
>>>> mode that is set.
>>>> => note also that the 2nd parameter is not the mode to be set but the
>>>> previously applied mode ! (ie loaded in ov5640 registers). This is used
>>>> to decide if we have to go to the "set_mode_exposure_calc" or
>>>> "set_mode_direct".
>>>>
>>>> the ov5640_restore_mode() also set the current pixel format
>>>> (sensor->fmt), that is done through this line:
>>>>
>>>> return ov5640_set_framefmt(sensor, &sensor->fmt);
>>>>
>>>> ==> This is what have fixed Mita-san, this line was missing previously,
>>>> leading to "mode registers" being loaded but not the "pixel format
>>>> registers".
>>>
>>> This seems overly complicated to me. Why do we have to set the mode at
>>> power on time at all, why can't we do it at stream on time only, and
>>> simplify all this logic ?
>>
>> I'm not the author of this driver, Steve do you know the origin and the
>> gain to do so ? Anyway, I would prefer that we stabilize currently existing
>> code before going to larger changes.
>
> I'm not opposed to that, but it's then pretty hard to review the patches, when
> they replace hard to read code with other hard to read code :-)
>
> Eventually we should really clean this up.
No problem to cleanup that code, I will be really happy to simplify that
stuff, but there are lot of stakeholders now so better to isolate that
exact change in a new serie and ask for a non-regression campaign.
>
>>>> PS: There are two other "set mode" related changes that are related to
>>>> this:
>>>> 1) 6949d864776e ("media: ov5640: do not change mode if format or
>>>> frame interval is unchanged")
>>>> => this is merged in media master, unfortunately I've introduced a
>>>> regression on "pixel format" side that I've fixed in this patchset :
>>>> 2)
>>>> https://www.mail-archive.com/linux-media@xxxxxxxxxxxxxxx/msg134413.html
>>>> Symptom was a noisy image when capturing QVGA YUV (in fact captured as
>>>> JPEG data).
>>>
>>> [snip]
>
BR Hugues.
