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 > >> > >> >
signature.asc
Description: PGP signature
