Hi Laurent
On Thu, 2 Feb 2023 at 22:10, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Dave,
>
> Thank you for the patch.
>
> On Tue, Jan 31, 2023 at 07:20:16PM +0000, Dave Stevenson wrote:
> > The sensor supports H & V flips, so add the relevant hooks for
> > V4L2_CID_HFLIP and V4L2_CID_VFLIP to configure them.
> >
> > Note that the Bayer order is maintained as the readout area
> > shifts by 1 pixel in the appropriate direction (note the
> > comment about the top margin being 8 pixels whilst the bottom
> > margin is 9).
>
> That's why ! Now it makes sense to me.
>
> > The V4L2_SEL_TGT_CROP region is therefore
> > adjusted appropriately.
>
> I'm not sure I like when sensors try to be clever... Out of curiosity,
> do you know if this automatic shift of the crop rectangle can be
> disabled ?
Not as far as I'm aware.
Most of the OnSemi sensors I've looked at also preserve the Bayer
order on flips, and the datasheets say they're doing the same thing of
shifting by one pixel. I did query if it could be disabled, and they
said no.
I have a suspicion that the situation here is actually worse, but
haven't had a chance to test experimentally.
The datasheet settings for all-pixel mode gives [X|Y]_OUT_SIZE as
1948x1097, but the driver sets them to 1920x1080 (1308x729 for the
1280x720 mode). Which pixels get dropped due to that reduction. My
expectation is that it'll be the right side and bottom edge, not a
centre crop as is currently assumed by the driver in get_selection. If
you flip that, then it'll be the top and left edges that get cropped
off.
If this is the case, then we'll have to switch to using window
cropping mode to request the desired crop. Some people may be happy
with this as it'll give them the information required to configure
VGA@129fps and CIF@205fps modes as mentioned in the datasheet. For now
I'm burying my head in the sand.
> > Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> > ---
> > drivers/media/i2c/imx290.c | 37 ++++++++++++++++++++++++++++++++++---
> > 1 file changed, 34 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
> > index 7f6746f74040..d2b7534f2c51 100644
> > --- a/drivers/media/i2c/imx290.c
> > +++ b/drivers/media/i2c/imx290.c
> > @@ -227,6 +227,8 @@ struct imx290 {
> > struct v4l2_ctrl *hblank;
> > struct v4l2_ctrl *vblank;
> > struct v4l2_ctrl *exposure;
> > + struct v4l2_ctrl *hflip;
> > + struct v4l2_ctrl *vflip;
> > };
> >
> > static inline struct imx290 *to_imx290(struct v4l2_subdev *_sd)
> > @@ -801,6 +803,24 @@ static int imx290_set_ctrl(struct v4l2_ctrl *ctrl)
> > NULL);
> > break;
> >
> > + case V4L2_CID_HFLIP:
> > + case V4L2_CID_VFLIP:
> > + {
> > + u32 reg;
> > +
> > + /* WINMODE is in bits [6:4], so need to read-modify-write */
>
> You could also cache the value of the register in struct imx290 to avoid
> the read.
We're already using regmap, so cache it there instead of locally?
Or alternatively move it out of the mode register array and into the
struct imx290_mode, then we can use imx290->current_mode->reg_ctrl_07
here. There's no need to set it from imx290_start_streaming as that
calls __v4l2_ctrl_handler_setup which will come through here. That
seems cleanest to me.
> > + ret = imx290_read(imx290, IMX290_CTRL_07, ®);
> > + if (ret)
> > + break;
> > + reg &= ~(IMX290_HREVERSE | IMX290_VREVERSE);
> > + if (imx290->hflip->val)
> > + reg |= IMX290_HREVERSE;
> > + if (imx290->vflip->val)
> > + reg |= IMX290_VREVERSE;
> > + ret = imx290_write(imx290, IMX290_CTRL_07, reg, NULL);
> > + break;
>
> As you always write those two controls together, they should be put in a
> cluster to have a single call to imx290_set_ctrl() when both are set in
> the same VIDIOC_S_EXT_CTRLS call.
Ah ctrl clusters - another can of worms :-)
Dave
> > + }
> > +
> > default:
> > ret = -EINVAL;
> > break;
> > @@ -853,7 +873,7 @@ static int imx290_ctrl_init(struct imx290 *imx290)
> > if (ret < 0)
> > return ret;
> >
> > - v4l2_ctrl_handler_init(&imx290->ctrls, 9);
> > + v4l2_ctrl_handler_init(&imx290->ctrls, 11);
> >
> > /*
> > * The sensor has an analog gain and a digital gain, both controlled
> > @@ -909,6 +929,11 @@ static int imx290_ctrl_init(struct imx290 *imx290)
> > imx290->vblank = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
> > V4L2_CID_VBLANK, 1, 1, 1, 1);
> >
> > + imx290->hflip = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
> > + V4L2_CID_HFLIP, 0, 1, 1, 0);
> > + imx290->vflip = v4l2_ctrl_new_std(&imx290->ctrls, &imx290_ctrl_ops,
> > + V4L2_CID_VFLIP, 0, 1, 1, 0);
> > +
> > v4l2_ctrl_new_fwnode_properties(&imx290->ctrls, &imx290_ctrl_ops,
> > &props);
> >
> > @@ -1030,6 +1055,9 @@ static int imx290_set_stream(struct v4l2_subdev *sd, int enable)
> > pm_runtime_put_autosuspend(imx290->dev);
> > }
> >
> > + /* vflip and hflip cannot change during streaming */
> > + __v4l2_ctrl_grab(imx290->vflip, enable);
> > + __v4l2_ctrl_grab(imx290->hflip, enable);
>
> A blank line would be nice.
>
> > unlock:
> > v4l2_subdev_unlock_state(state);
> > return ret;
> > @@ -1115,6 +1143,7 @@ static int imx290_get_selection(struct v4l2_subdev *sd,
> > struct v4l2_subdev_state *sd_state,
> > struct v4l2_subdev_selection *sel)
> > {
> > + struct imx290 *imx290 = to_imx290(sd);
> > struct v4l2_mbus_framefmt *format;
> >
> > switch (sel->target) {
> > @@ -1122,9 +1151,11 @@ static int imx290_get_selection(struct v4l2_subdev *sd,
> > format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> >
>
> A comment to explain here why the crop rectangle is adjusted would be
> nice.
>
> > sel->r.top = IMX920_PIXEL_ARRAY_MARGIN_TOP
> > - + (IMX290_PIXEL_ARRAY_RECORDING_HEIGHT - format->height) / 2;
> > + + (IMX290_PIXEL_ARRAY_RECORDING_HEIGHT - format->height) / 2
> > + + imx290->vflip->val;
> > sel->r.left = IMX920_PIXEL_ARRAY_MARGIN_LEFT
> > - + (IMX290_PIXEL_ARRAY_RECORDING_WIDTH - format->width) / 2;
> > + + (IMX290_PIXEL_ARRAY_RECORDING_WIDTH - format->width) / 2
> > + + imx290->hflip->val;
> > sel->r.width = format->width;
> > sel->r.height = format->height;
> >
>
> --
> Regards,
>
> Laurent Pinchart
