On Fri, 3 Feb 2023 at 08:33, Alexander Stein
<alexander.stein@xxxxxxxxxxxxxxx> wrote:
>
> Am Freitag, 3. Februar 2023, 08:57:37 CET schrieb Dave Stevenson:
> > 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.
>
> That's right. I just noticed libcamera (libcamersrc in a gst-pipeline
> actrually) resets flipping to default when starting... so this behaviour is
> understandable, although somewhat unexpected.
Libcamera has controls for flips, and those will always adopt either
defaults or the user requested flips. If libcamera updating VBLANK to
control the frame rate or EXPOSURE isn't unexpected, why would it
updating FLIPs be unexpected?
Either libcamera is in control of the sensor, or the user is through
v4l2-ctl. A half-and-half doesn't work. (Been there, done that, whilst
trying to get HBLANK implemented in some drivers)
> > 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.
>
> What I meant is that it's not possible to change flipping from userspace while
> being grabbed.
>
> > > > +
> > > >
> > > > 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.
>
> I see. I was not aware of this, maybe due to this there should be a comment
> why changing flips during stream should not be done. Current comment is
> totally misleading.
Largely the same text in all the other drivers, even when it would
change the Bayer order. It's describing the result of the grab rather
than the reasoning, but I'll update it.
Dave
> Best regards
> Alexande
>
> > 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;
>
>
>
>
