Re: [PATCH 12/16] media: i2c: ov9282: Make V4L2_CID_HBLANK r/w

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Jacopo.

On Thu, 6 Oct 2022 at 10:41, Jacopo Mondi <jacopo@xxxxxxxxxx> wrote:
>
> Hi Dave
>
> On Wed, Oct 05, 2022 at 04:28:05PM +0100, Dave Stevenson wrote:
> > There's no reason why HBLANK has to be read-only as it
> > only changes the TIMING_HTS register in the sensor.
> >
> > Remove the READ_ONLY flag, and add the relevant handling
> > for it.
> >
> > The minimum value also varies based on whether continuous clock
> > mode is being used or not, so allow hblank_min to depend on
> > that.
>
> Interesting, do you know why they're different and why the continous
> version is smaller ?

Dropping from HS to LP-11 and back to HS for non-continuous mode takes
a minimum amount of time, so the time per line on the MIPI bus is
increased. Whilst there is a FIFO between the pixel array and MIPI
block, having to expend that time on the MIPI side means that the FIFO
can overflow.

I haven't found this behaviour documented, but I was trying to work
out the difference between this driver and our downstream driver [1]
in max frame rate. The default value for TIMING_HTS of 0x2d8 (doubled
to 1456 pixels) didn't work with this mainline driver, and it used
0x2fd (1530 pixels). The CSI clock mode turned out to be the
difference.
The minimum HTS has therefore been left alone for non-continuous clock
mode, and the lower setting added for continuous clock.

  Dave

[1] https://github.com/raspberrypi/linux/blob/rpi-5.15.y/drivers/media/i2c/ov9281.c
based on https://github.com/rockchip-linux/kernel/blob/develop-4.4/drivers/media/i2c/ov9281.c

> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> > ---
> >  drivers/media/i2c/ov9282.c | 34 +++++++++++++++++++++-------------
> >  1 file changed, 21 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index 12cbe401fd78..8e86aa7e4b2a 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -22,6 +22,9 @@
> >  #define OV9282_MODE_STANDBY  0x00
> >  #define OV9282_MODE_STREAMING        0x01
> >
> > +#define OV9282_REG_TIMING_HTS        0x380c
> > +#define OV9282_TIMING_HTS_MAX        0x7fff
> > +
> >  /* Lines per frame */
> >  #define OV9282_REG_LPFR              0x380e
> >
> > @@ -99,7 +102,8 @@ struct ov9282_reg_list {
> >   * struct ov9282_mode - ov9282 sensor mode structure
> >   * @width: Frame width
> >   * @height: Frame height
> > - * @hblank: Horizontal blanking in lines
> > + * @hblank_min: Minimum horizontal blanking in lines for non-continuous[0] and
> > + *           continuous[1] clock modes
> >   * @vblank: Vertical blanking in lines
> >   * @vblank_min: Minimum vertical blanking in lines
> >   * @vblank_max: Maximum vertical blanking in lines
> > @@ -109,7 +113,7 @@ struct ov9282_reg_list {
> >  struct ov9282_mode {
> >       u32 width;
> >       u32 height;
> > -     u32 hblank;
> > +     u32 hblank_min[2];
> >       u32 vblank;
> >       u32 vblank_min;
> >       u32 vblank_max;
> > @@ -249,8 +253,6 @@ static const struct ov9282_reg mode_1280x720_regs[] = {
> >       {0x3809, 0x00},
> >       {0x380a, 0x02},
> >       {0x380b, 0xd0},
> > -     {0x380c, 0x02},
> > -     {0x380d, 0xfd},
> >       {0x3810, 0x00},
> >       {0x3811, 0x08},
> >       {0x3812, 0x00},
> > @@ -273,7 +275,7 @@ static const struct ov9282_mode supported_modes[] = {
> >       {
> >               .width = 1280,
> >               .height = 720,
> > -             .hblank = 250,
> > +             .hblank_min = { 250, 176 },
> >               .vblank = 1022,
> >               .vblank_min = 41,
> >               .vblank_max = 51540,
> > @@ -399,15 +401,17 @@ static int ov9282_write_regs(struct ov9282 *ov9282,
> >  static int ov9282_update_controls(struct ov9282 *ov9282,
> >                                 const struct ov9282_mode *mode)
> >  {
> > +     u32 hblank_min;
> >       int ret;
> >
> >       ret = __v4l2_ctrl_s_ctrl(ov9282->link_freq_ctrl, mode->link_freq_idx);
> >       if (ret)
> >               return ret;
> >
> > -     ret = __v4l2_ctrl_s_ctrl(ov9282->hblank_ctrl, mode->hblank);
> > -     if (ret)
> > -             return ret;
> > +     hblank_min = mode->hblank_min[ov9282->noncontinuous_clock ? 0 : 1];
> > +     ret =  __v4l2_ctrl_modify_range(ov9282->hblank_ctrl, hblank_min,
> > +                                     OV9282_TIMING_HTS_MAX - mode->width, 1,
> > +                                     hblank_min);
> >
> >       return __v4l2_ctrl_modify_range(ov9282->vblank_ctrl, mode->vblank_min,
> >                                       mode->vblank_max, 1, mode->vblank);
> > @@ -539,6 +543,10 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> >       case V4L2_CID_VFLIP:
> >               ret = ov9282_set_ctrl_vflip(ov9282, ctrl->val);
> >               break;
> > +     case V4L2_CID_HBLANK:
> > +             ret = ov9282_write_reg(ov9282, OV9282_REG_TIMING_HTS, 2,
> > +                                    (ctrl->val + ov9282->cur_mode->width) >> 1);
> > +             break;
> >       default:
> >               dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
> >               ret = -EINVAL;
> > @@ -1033,6 +1041,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> >       struct v4l2_ctrl_handler *ctrl_hdlr = &ov9282->ctrl_handler;
> >       const struct ov9282_mode *mode = ov9282->cur_mode;
> >       struct v4l2_fwnode_device_properties props;
> > +     u32 hblank_min;
> >       u32 lpfr;
> >       int ret;
> >
> > @@ -1091,14 +1100,13 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> >       if (ov9282->link_freq_ctrl)
> >               ov9282->link_freq_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> >
> > +     hblank_min = mode->hblank_min[ov9282->noncontinuous_clock ? 0 : 1];
> >       ov9282->hblank_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
> >                                               &ov9282_ctrl_ops,
> >                                               V4L2_CID_HBLANK,
> > -                                             OV9282_REG_MIN,
> > -                                             OV9282_REG_MAX,
> > -                                             1, mode->hblank);
> > -     if (ov9282->hblank_ctrl)
> > -             ov9282->hblank_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +                                             hblank_min,
> > +                                             OV9282_TIMING_HTS_MAX - mode->width,
> > +                                             1, hblank_min);
> >
> >       ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
> >       if (!ret) {
> > --
> > 2.34.1
> >



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux