Re: [PATCH v2 0/2] media: i2c: ov5640: Re-work MIPI startup sequence

[jacopo mondi <jacopo@jmondi.org>]

[-- Attachment #1: Type: text/plain, Size: 4874 bytes --]

Hi Steve,
   sorry for resurecting this.

On Mon, Jul 16, 2018 at 09:26:13AM -0700, Steve Longerbeam wrote:
>
>
> On 07/16/2018 01:29 AM, jacopo mondi wrote:
> >Hi Steve,
> >    thanks for keep testing it
> >
> >On Sat, Jul 14, 2018 at 01:02:32PM -0700, Steve Longerbeam wrote:
> >>
> >>On 07/14/2018 12:41 PM, Steve Longerbeam wrote:
> >>>Hi Jacopo,
> >>>
> >>>
> >>>On 07/14/2018 11:57 AM, Steve Longerbeam wrote:
> >>>>Hi Jacopo,
> >>>>
> >>>>Pardon the late reply, see below.
> >>>>
> >>>>On 07/11/2018 12:21 AM, jacopo mondi wrote:
> >>>>>Hi Steve,
> >>>>>
> >>>>>On Tue, Jul 10, 2018 at 02:10:54PM -0700, Steve Longerbeam wrote:
> >>>>>>Hi Jacopo,
> >>>>>>
> >>>>>>Sorry to report my testing on SabreSD has same result
> >>>>>>as last time. This series fixes the LP-11 timeout at stream
> >>>>>>on but captured images are still blank. I tried the 640x480
> >>>>>>mode with UYVY2X8. Here is the pad config:
> >>>>>This saddens me :(
> >>>>>
> >>>>>I'm capturing with the same format and sizes... this shouldn't be the
> >>>>>issue
> >>>>>
> >>>>>Could you confirm this matches what you have in your tree?
> >>>>>5dc2c80 media: ov5640: Fix timings setup code
> >>>>>b35e757 media: i2c: ov5640: Re-work MIPI startup sequence
> >>>>>3c4a737 media: ov5640: fix frame interval enumeration
> >>>>>41cb1c7 media: ov5640: adjust xclk_max
> >>>>>c3f3ba3 media: ov5640: add support of module orientation
> >>>>>ce85705 media: ov5640: add HFLIP/VFLIP controls support
> >>>>>8663341 media: ov5640: Program the visible resolution
> >>>>>476dec0 media: ov5640: Add horizontal and vertical totals
> >>>>>dba13a0 media: ov5640: Change horizontal and vertical resolutions name
> >>>>>8f57c2f media: ov5640: Init properly the SCLK dividers
> >>>>Yes, I have that commit sequence.
> >>>>
> >>>>FWIW, I can verify what Jagan Teki reported earlier, that the driver
> >>>>still
> >>>>works on the SabreSD platform at:
> >>>>
> >>>>dba13a0 media: ov5640: Change horizontal and vertical resolutions name
> >>>>
> >>>>and is broken at:
> >>>>
> >>>>476dec0 media: ov5640: Add horizontal and vertical totals
> >>>>
> >>>>with LP-11 timeout at the mipi csi-2 receiver:
> >>>>
> >>>>[   80.763189] imx6-mipi-csi2: LP-11 timeout, phy_state = 0x00000230
> >>>>[   80.769599] ipu1_csi1: pipeline start failed with -110
> >>>And I discovered the bug in 476dec0 "media: ov5640: Add horizontal and
> >>>vertical totals". The call to ov5640_set_timings() needs to be moved
> >>>before the
> >>>calls to ov5640_get_vts() and ov5640_get_hts(). But I see you have
> >>>discovered
> >>>that as well, and fixed in the second patch in your series.
> >>>
> >I'm sorry I'm not sur I'm following. Does this mean that with that bug
> >you are referring to up here fixed by my last patch you have capture
> >working?
>
> No, capture still not working for me on SabreSD, even after fixing
> the bug in 476dec0 "media: ov5640: Add horizontal and vertical totals",
> by either using your patchset, or by running version 476dec0 of ov5640.c
> with the call to ov5640_set_timings() moved to the correct places as
> described below.
>

I've been reported a bug on exposure handling that makes the first
captured frames all black. Both me and Hugues have tried to fix the
issue (him with a more complete series, but that's another topic).
See [1] and [2]

It might be possible that you're getting blank frames with this series
applied? I never seen them as I'm skipping the first frames when
capturing, but I've now tested and without the exposure fixes (either
[1] or [2]) I actually have blank frames.

If that's the case for you too (which I hope so much) would you be
available to test again this series with exposure fixes on top?
On my platform that actually makes all frames correct.

Thanks
   j

[1] [PATCH 0/2] media: ov5640: Fix set_timings and auto-exposure
[2] [PATCH v2 0/5] Fix OV5640 exposure & gain

> Steve
>
> >>But strangely, if I revert to 476dec0, and then move the call to
> >>ov5640_set_timings()
> >>to just after ov5640_load_regs() in ov5640_set_mode_exposure_calc() and
> >>ov5640_set_mode_direct(), the LP-11 timeouts are still present. So I can
> >>confirm
> >>this strangeness which you already pointed out below [1].
> >>
> >>
> >>>>>>>The version I'm sending here re-introduces some of the timings
> >>>>>>>parameters in the
> >>>>>>>initial configuration blob (not in the single mode ones), which
> >>>>>>>apparently has
> >>>>>>>to be at least initially programmed to allow the driver to later
> >>>>>>>program them
> >>>>>>>singularly in the 'set_timings()' function. Unfortunately I do not
> >>>>>>>have a real
> >>>>>>>rationale behind this which explains why it has to be done this
> >>>>>>>way :(
> >>>>>>>
> >>[1] here :)
> >>
> >>Steve
> >>
> >>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]