| Message ID | 20230131192016.3476937-12-dave.stevenson@raspberrypi.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | imx290: Minor fixes, support for alternate INCK, and more ctrls | expand |
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 ? > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > --- > 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. > + 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. > + } > + > 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; >
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@raspberrypi.com> > --- > 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. > + > 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. 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;
Hi Alexander On Fri, 3 Feb 2023 at 07:35, Alexander Stein <alexander.stein@ew.tq-group.com> 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@raspberrypi.com> > > --- > > 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; > > > >
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@ew.tq-group.com> 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@raspberrypi.com> > > > --- > > > > > > 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. > 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. 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;
On Fri, 3 Feb 2023 at 08:33, Alexander Stein <alexander.stein@ew.tq-group.com> 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@ew.tq-group.com> 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@raspberrypi.com> > > > > --- > > > > > > > > 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; > > > >
Hi Laurent On Thu, 2 Feb 2023 at 22:10, Laurent Pinchart <laurent.pinchart@ideasonboard.com> 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@raspberrypi.com> > > --- > > 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
Hi Dave, On Fri, Feb 03, 2023 at 10:21:28AM +0000, Dave Stevenson wrote: > On Thu, 2 Feb 2023 at 22:10, Laurent Pinchart wrote: > > 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. I wonder if anybody would ever notice. > 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. Thank you for the information. Is there any sand left to burry my head as well ? > > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com> > > > --- > > > 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? Ah yes, it can be done in regmap. > 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. Works for me too. > > > + 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 :-) With a bit of luck it will only be one extra function call at init time and all will be fine :-) > > > + } > > > + > > > 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; > > >
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; + } + 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); 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;
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@raspberrypi.com> --- drivers/media/i2c/imx290.c | 37 ++++++++++++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-)