Thanks for the review, Sakari and Tomasz.
-----Original Message-----
From: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
Sent: Tuesday, January 23, 2024 6:16 PM
To: Tomasz Figa <tfiga@xxxxxxxxxxxx>
Cc: Chen, Jason Z <jason.z.chen@xxxxxxxxx>; bingbu.cao@xxxxxxxxxxxxxxx; linux-media@xxxxxxxxxxxxxxx; Yeh, Andy <andy.yeh@xxxxxxxxx>; Zhang, Qingwu <qingwu.zhang@xxxxxxxxx>; senozhatsky@xxxxxxxxxxxx
Subject: Re: [PATCH v3] media: ov08x40: Reduce start streaming time
Hi Tomasz,
On Tue, Jan 23, 2024 at 07:03:49PM +0900, Tomasz Figa wrote:
> On Tue, Jan 23, 2024 at 6:37 PM Chen, Jason Z <jason.z.chen@xxxxxxxxx> wrote:
> >
> > From: Jason Chen <jason.z.chen@xxxxxxxxx>
> >
> > Because video duration involves calculating the streaming time, and
> > i2c communication incurs too many XTALK register settings every 4
> > bytes with i2c START and STOP.
> >
> > So we have opted switch to the i2c burst method.
> > This method involves writing the XTALK registers in the order of the
> > register block.
> >
> > The start streaming time can be reduced from around 400ms to 150ms
> >
> > Signed-off-by: Jason Chen <jason.z.chen@xxxxxxxxx>
> > ---
> > drivers/media/i2c/ov08x40.c | 1207
> > ++---------------------------------
> > 1 file changed, 55 insertions(+), 1152 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov08x40.c
> > b/drivers/media/i2c/ov08x40.c index ddcb4b6848b..7b09f405fc5 100644
> > --- a/drivers/media/i2c/ov08x40.c
> > +++ b/drivers/media/i2c/ov08x40.c
> [snip]
> > +static int ov08x40_burst_fill_regs(struct ov08x40 *ov08x, u16 first_reg,
> > + u16 last_reg, u8 val) {
> > + struct i2c_client *client = v4l2_get_subdevdata(&ov08x->sd);
> > + struct i2c_msg msgs;
> > + __be16 reg_addr_be = cpu_to_be16(first_reg);
> > + size_t i, num_regs;
> > + int ret;
> > +
> > + num_regs = last_reg - first_reg + 1;
> > + msgs.addr = client->addr;
> > + msgs.flags = 0;
> > + msgs.len = 2 + num_regs;
> > + msgs.buf = kmalloc(msgs.len, GFP_KERNEL);
> > + if (!msgs.buf)
> > + return -ENOMEM;
> > +
> > + /* Set the register address to the first two bytes of the buffer */
> > + memcpy(msgs.buf, ®_addr_be, 2);
>
> nit: Wouldn't the code be much simpler to follow if rewritten as below?
>
> msgs.buf[0] = first_reg >> 8;
> msgs.buf[1] = first_reg & 0xff;
>Or... just put_unaligned_be16()?
Done
>
> > + for (i = 0; i < num_regs; ++i)
> > + msgs.buf[2 + i] = val;
> > +
> > + ret = i2c_transfer(client->adapter, &msgs, 1);
> > +
> > + kfree(msgs.buf);
> > +
> > + if (ret != 1) {
> > + dev_err(&client->dev, "Failed %d regs transferred: %d\n", ret);
> > + return -EIO;
> > + }
> > +
> > + dev_dbg(&client->dev, "I2C burst transfer succeeded\n");
> > +
> > + return 0;
> > +}
> > +
> > /* Write registers up to 4 at a time */ static int
> > ov08x40_write_reg(struct ov08x40 *ov08x,
> > u16 reg, u32 len, u32 __val) @@ -2936,6
> > +1826,19 @@ static int ov08x40_start_streaming(struct ov08x40 *ov08x)
> > return ret;
> > }
> >
> > + /* Use i2c burst to write register on full size registers */
> > + if (ov08x->cur_mode->exposure_shift == 1) {
> > + ret = ov08x40_burst_fill_regs(ov08x, OV08X40_REG_XTALK_FIRST_A,
> > + OV08X40_REG_XTALK_LAST_A, 0x75);
> > + ret = ov08x40_burst_fill_regs(ov08x, OV08X40_REG_XTALK_FIRST_B,
> > + OV08X40_REG_XTALK_LAST_B, 0x75);
> > + }
>
> Hmm, if we only need to set these if exposure_shift is 1, don't we
> need to somehow "unset" them if the mode exposure_shift != 1?
>I recall the driver powers the sensor off every time streaming is stopped (without pm_runtime_put_sync(), so it's not reliable)... that would be nice to address as well, but outside the scope of this patch.
No need, because when starting streaming with the binning mode, it will flush with the `mode_1928x1208_regs`.
This is just for full-size register settings.
I have checked the i2c functionality as well, and it does work to set the register well.
--
Regards,
Sakari Ailus
