| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CA+V-a8vsX9Ei8dS+fJQe9jqcFXmvfeVb==-HhDkXFnajQYT6tg@mail.gmail.com>
Date: Tue, 21 Dec 2021 15:11:52 +0000
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Eugen Hristev <Eugen.Hristev@...rochip.com>
Cc: Hugues Fruchet <hugues.fruchet@...com>,
"Lad, Prabhakar" <prabhakar.mahadev-lad.rj@...renesas.com>,
Jacopo Mondi <jacopo+renesas@...ndi.org>,
Sakari Ailus <sakari.ailus@...ux.intel.com>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Steve Longerbeam <slongerbeam@...il.com>,
Paul <paul.kocialkowski@...tlin.com>,
linux-media <linux-media@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
Biju Das <biju.das.jz@...renesas.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>
Subject: Re: [PATCH v4 1/6] media: i2c: ov5640: Remain in power down for DVP
mode unless streaming
Hi Eugen,
On Tue, Dec 21, 2021 at 3:02 PM <Eugen.Hristev@...rochip.com> wrote:
>
> On 12/21/21 4:47 PM, Lad, Prabhakar wrote:
> > Hi,
> >
> > Sorry for the delay.
> >
> > On Tue, Dec 21, 2021 at 7:37 AM <Eugen.Hristev@...rochip.com> wrote:
> >>
> >> On 6/29/21 1:47 PM, Eugen Hristev - M18282 wrote:
> >>> On 9/9/20 7:16 PM, Hugues FRUCHET wrote:
> >>>> Hi Prabhakar,
> >>>>
> >>>> As discussed separately I would prefer to better understand issue before
> >>>> going to this patch.
> >>>> Nevertheless I have some remarks in code in case we'll need it at the end.
> >>>>
> >>>> On 9/4/20 10:18 PM, Lad Prabhakar wrote:
> >>>>> Keep the sensor in software power down mode and wake up only in
> >>>>> ov5640_set_stream_dvp() callback.
> >>>>>
> >>>>> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> >>>>> Reviewed-by: Biju Das <biju.das.jz@...renesas.com>
> >>>>> Tested-by: Jacopo Mondi <jacopo@...ndi.org>
> >>>>> ---
> >>>>> drivers/media/i2c/ov5640.c | 19 ++++++++++++++++---
> >>>>> 1 file changed, 16 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> >>>>> index 2fe4a7ac0592..880fde73a030 100644
> >>>>> --- a/drivers/media/i2c/ov5640.c
> >>>>> +++ b/drivers/media/i2c/ov5640.c
> >>>>> @@ -34,6 +34,8 @@
> >>>>> #define OV5640_REG_SYS_RESET02 0x3002
> >>>>> #define OV5640_REG_SYS_CLOCK_ENABLE02 0x3006
> >>>>> #define OV5640_REG_SYS_CTRL0 0x3008
> >>>>> +#define OV5640_REG_SYS_CTRL0_SW_PWDN 0x42
> >>>>> +#define OV5640_REG_SYS_CTRL0_SW_PWUP 0x02
> >>>>
> >>>> For the time being this section was only referring to registers
> >>>> addresses and bit details was explained into a comment right before
> >>>> affectation, see OV5640_REG_IO_MIPI_CTRL00 for example.
> >>>>
> >>>>> #define OV5640_REG_CHIP_ID 0x300a
> >>>>> #define OV5640_REG_IO_MIPI_CTRL00 0x300e
> >>>>> #define OV5640_REG_PAD_OUTPUT_ENABLE01 0x3017
> >>>>> @@ -1120,6 +1122,12 @@ static int ov5640_load_regs(struct ov5640_dev *sensor,
> >>>>> val = regs->val;
> >>>>> mask = regs->mask;
> >>>>>
> >>>>> + /* remain in power down mode for DVP */
> >>>>> + if (regs->reg_addr == OV5640_REG_SYS_CTRL0 &&
> >>>>> + val == OV5640_REG_SYS_CTRL0_SW_PWUP &&
> >>>>> + sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> >>>>> + continue;
> >>>>> +
> >>>>
> >>>> I understand that more or less register OV5640_REG_SYS_CTRL0 (0x3008)
> >>>> has been partially removed from big init sequence: for power-up part,
> >>>> but power-dwn remains at very beginning of sequence.
> >>>> We should completely remove 0x3008 from init sequence, including
> >>>> power-dwn, and introduce a new function ov5640_sw_powerdown(on/off) that
> >>>> should be called at the right place instead.
> >>>>
> >>>>
> >>>>> if (mask)
> >>>>> ret = ov5640_mod_reg(sensor, reg_addr, mask, val);
> >>>>> else
> >>>>> @@ -1297,9 +1305,14 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
> >>>>> * PAD OUTPUT ENABLE 02
> >>>>> * - [7:2]: D[5:0] output enable
> >>>>> */
> >>>>> - return ov5640_write_reg(sensor,
> >>>>> - OV5640_REG_PAD_OUTPUT_ENABLE02,
> >>>>> - on ? 0xfc : 0);
> >>>>> + ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02,
> >>>>> + on ? 0xfc : 0);
> >>>>> + if (ret)
> >>>>> + return ret;
> >>>>> +
> >>>>> + return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> >>>>> + OV5640_REG_SYS_CTRL0_SW_PWUP :
> >>>>> + OV5640_REG_SYS_CTRL0_SW_PWDN);
> >>>>> }
> >>>>>
> >>>>> static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> >>>>>
> >>>>
> >>>>
> >>>> BR,
> >>>> Hugues.
> >>>>
> >>>
> >>>
> >>> Hello everyone,
> >>>
> >>> When I updated driver in my tree with this patch, I noticed that my
> >>> setup using the ATMEL isc platform + ov5640 in parallel bus mode stopped
> >>> working.
> >>>
> >>> It looks like the culprit is this patch.
> >>> I am not sure which is the best way to fix it.
> >>> It looks like initially things work fine when probing the driver, but
> >>> when we want to start streaming at a later point, the register SYS_CTRL0
> >>> cannot be written anymore.
> >>> Here is an output of the log:
> >>>
> >>> --- Opening /dev/video0...
> >>> Trying source module v4l2...
> >>> /dev/video0 opened.
> >>> No input was specified, using the first.
> >>> ov5640 1-003c: ov5640_write_reg: error: reg=3008, val=2
> >>> atmel-sama5d2-isc f0008000.isc: stream on failed in subdev -121
> >>> Error starting stream.
> >>> VIDIOC_STREAMON: Remote I/O error
> >>> Unable to use mmap. Using read instead.
> >>> Unable to use read.
> >>>
> >>> I am using a simple application to read from /dev/video0 (which is
> >>> registered by the atmel-isc once the sensor probed)
> >>>
> >>> I have a feeling that something is wrong related to power, but I cannot
> >>> tell for sure.
> >>>
> >>> Reverting this patch makes it work fine again.
> >>>
> >>> Help is appreciated,
> >>> Thanks,
> >>> Eugen
> >>>
> >>
> >>
> >> Gentle ping.
> >>
> >> I would like to send a revert patch if nobody cares about this
> >> driver/patch except me.
> >>
> > I powered up the Renesas RZ/G1H connected with an ov5640 sensor and
> > was able to capture the video data [0] using the yavta application.
> > I'm fine with reverting the patch too as this works fine as well.
>
> Hi Prabhakar,
>
> Thanks for trying this out.
> ov5640 works in both parallel or CSI2 mode. Looking at the board, it
> looks a parallel connection but I am not 100%, you tested using parallel
> interface ?
>
I have tested it in parallel interface mode as RZ/G1H supports parallel capture
> >
> > Just some suggestions:
> > - What is the application you are trying to use for capturing?
>
> I was using a simple app that reads from /dev/video0, it's called
> fswebcam. I can try more apps if it's needed.
>
could you give it a shot with yavta please.
> > - Have you tried reducing the i2c clock speed?
>
> I did not, but without this patch, there is no problem whatsoever, and I
> have been using this sensor since around 4.9 kernel version.
>
Agreed, I'm thinking aloud just to narrow things.
> > - Can you try and do a dummy write for some other register in
> > ov5640_set_stream_dvp() to see if i2c writes are failing for all regs?
>
> I can try
>
Thank you.
> > - Can you add ov5640_read_reg() in ov5640_write_reg() when
> > i2c_transfer() fails to check if read too is failing.
> >
> > [0] https://paste.debian.net/1224317/
>
> You seem to be able to capture successfully, I have a feeling that in my
> case the sensor is somehow not powered up when the capture is being
> requested.
> Could you share a snippet of your device tree node for the sensor so I
> can have a look ?
>
[0] contains the bridge node and [1] contains the sensor node.
[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/r8a7742-iwg21d-q7-dbcm-ca.dts?h=v5.16-rc6#n309
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/r8a7742-iwg21d-q7-dbcm-ov5640-single.dtsi?h=v5.16-rc6
Cheers,
Prabhakar
Powered by blists - more mailing lists