Hi Alexander
On Fri, 3 Feb 2023 at 07:35, Alexander Stein
<alexander.stein@xxxxxxxxxxxxxxx> wrote:
>
> Hi Dave,
>
> thanks for the patch.
>
> Am Dienstag, 31. Januar 2023, 20:20:16 CET schrieb Dave Stevenson:
> > 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). The V4L2_SEL_TGT_CROP region is therefore
> > adjusted appropriately.
> >
> > 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
> */
> > + 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;
> > + }
>
> Given the grab while streaming is on, it can't be changed while streaming. But
> then again the pm_runtime check above will prevent setting the registers while
> streaming is off.
But __v4l2_ctrl_handler_setup is called from imx290_start_streaming
after pm_runtime_resume_and_get, and so will programme the hardware.
Writing the flips from the set_ctrl even if they have been grabbed due
to changing the Bayer order is the normal approach with many other
drivers. See imx319, imx219, imx335, imx415, and probably others.
> > +
> > 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);
>
> Why is this grab necessary? While trying to remove these lines, I can flip the
> image while streaming.
IMX290 Datasheet section "Normal and inverted operation":
"One invalid frame is generated when reading immediately after the
readout direction is changed in order to switch the normal operation
and inversion of frames."
There is no synchronisation between CSI2 receiver and the subdev, so
no way to signal that corrupt frame.
Is it permitted for sources to knowingly deliver corrupt frames? My
understanding is not.
Dave
> Best regards,
> Alexander
>
> > 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);
> >
> > 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;
>
>
>
>
