Hi Jacopo,
On 08/16/2018 12:10 PM, jacopo mondi wrote:
> Hi Hugues,
> thanks for the patch
>
> 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".
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).
Best regards,
Hugues.
>
> Thanks
> j
>
>>
>> Signed-off-by: Hugues Fruchet <hugues.fruchet@xxxxxx>
>> ---
>> drivers/media/i2c/ov5640.c | 8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>> index c110a6a..923cc30 100644
>> --- a/drivers/media/i2c/ov5640.c
>> +++ b/drivers/media/i2c/ov5640.c
>> @@ -225,6 +225,7 @@ struct ov5640_dev {
>> struct v4l2_mbus_framefmt fmt;
>>
>> const struct ov5640_mode_info *current_mode;
>> + const struct ov5640_mode_info *last_mode;
>> enum ov5640_frame_rate current_fr;
>> struct v4l2_fract frame_interval;
>>
>> @@ -1628,6 +1629,9 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
>> bool auto_exp = sensor->ctrls.auto_exp->val == V4L2_EXPOSURE_AUTO;
>> int ret;
>>
>> + if (!orig_mode)
>> + orig_mode = mode;
>> +
>> dn_mode = mode->dn_mode;
>> orig_dn_mode = orig_mode->dn_mode;
>>
>> @@ -1688,6 +1692,7 @@ static int ov5640_set_mode(struct ov5640_dev *sensor,
>> return ret;
>>
>> sensor->pending_mode_change = false;
>> + sensor->last_mode = mode;
>>
>> return 0;
>>
>> @@ -2551,7 +2556,8 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
>>
>> if (sensor->streaming == !enable) {
>> if (enable && sensor->pending_mode_change) {
>> - ret = ov5640_set_mode(sensor, sensor->current_mode);
>> + ret = ov5640_set_mode(sensor, sensor->last_mode);
>> +
>> if (ret)
>> goto out;
>>
>> --
>> 2.7.4
>>
