| Message ID | 20231010005126.3425444-4-kieran.bingham@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: Sony IMX335 improvements | expand |
Hi Kieran On 10/10/23 6:21 AM, Kieran Bingham wrote: > Support reporting of the Sensor Native and Active pixel array areas > through the Selection API. > > The implementation reports a single target crop only for the mode that > is presently exposed by the driver. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> LGTM, Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > drivers/media/i2c/imx335.c | 44 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c > index bf12b9b69fce..026777eb362e 100644 > --- a/drivers/media/i2c/imx335.c > +++ b/drivers/media/i2c/imx335.c > @@ -55,6 +55,14 @@ > #define IMX335_REG_MIN 0x00 > #define IMX335_REG_MAX 0xfffff > > +/* IMX335 native and active pixel array size. */ > +#define IMX335_NATIVE_WIDTH 2616U > +#define IMX335_NATIVE_HEIGHT 1964U > +#define IMX335_PIXEL_ARRAY_LEFT 12U > +#define IMX335_PIXEL_ARRAY_TOP 12U > +#define IMX335_PIXEL_ARRAY_WIDTH 2592U > +#define IMX335_PIXEL_ARRAY_HEIGHT 1944U > + > /** > * struct imx335_reg - imx335 sensor register > * @address: Register address > @@ -651,6 +659,41 @@ static int imx335_init_pad_cfg(struct v4l2_subdev *sd, > return imx335_set_pad_format(sd, sd_state, &fmt); > } > > +/** > + * imx335_get_selection() - Selection API > + * @sd: pointer to imx335 V4L2 sub-device structure > + * @sd_state: V4L2 sub-device configuration > + * @sel: V4L2 selection info > + * > + * Return: 0 if successful, error code otherwise. > + */ > +static int imx335_get_selection(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *sd_state, > + struct v4l2_subdev_selection *sel) > +{ > + switch (sel->target) { > + case V4L2_SEL_TGT_NATIVE_SIZE: > + sel->r.top = 0; > + sel->r.left = 0; > + sel->r.width = IMX335_NATIVE_WIDTH; > + sel->r.height = IMX335_NATIVE_HEIGHT; > + > + return 0; > + > + case V4L2_SEL_TGT_CROP: > + case V4L2_SEL_TGT_CROP_DEFAULT: > + case V4L2_SEL_TGT_CROP_BOUNDS: > + sel->r.top = IMX335_PIXEL_ARRAY_TOP; > + sel->r.left = IMX335_PIXEL_ARRAY_LEFT; > + sel->r.width = IMX335_PIXEL_ARRAY_WIDTH; > + sel->r.height = IMX335_PIXEL_ARRAY_HEIGHT; > + > + return 0; > + } > + > + return -EINVAL; > +} > + > /** > * imx335_start_streaming() - Start sensor stream > * @imx335: pointer to imx335 device > @@ -864,6 +907,7 @@ static const struct v4l2_subdev_pad_ops imx335_pad_ops = { > .init_cfg = imx335_init_pad_cfg, > .enum_mbus_code = imx335_enum_mbus_code, > .enum_frame_size = imx335_enum_frame_size, > + .get_selection = imx335_get_selection, > .get_fmt = imx335_get_pad_format, > .set_fmt = imx335_set_pad_format, > };
Hi Kieran, On Tue, Oct 10, 2023 at 01:51:24AM +0100, Kieran Bingham wrote: > Support reporting of the Sensor Native and Active pixel array areas > through the Selection API. > > The implementation reports a single target crop only for the mode that > is presently exposed by the driver. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Shouldn't you use the same callback for .set_selection? I guess this is somewhat grey area but doing so would be in line with how V4L2 API works in general. Cc Laurent. > --- > drivers/media/i2c/imx335.c | 44 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c > index bf12b9b69fce..026777eb362e 100644 > --- a/drivers/media/i2c/imx335.c > +++ b/drivers/media/i2c/imx335.c > @@ -55,6 +55,14 @@ > #define IMX335_REG_MIN 0x00 > #define IMX335_REG_MAX 0xfffff > > +/* IMX335 native and active pixel array size. */ > +#define IMX335_NATIVE_WIDTH 2616U > +#define IMX335_NATIVE_HEIGHT 1964U > +#define IMX335_PIXEL_ARRAY_LEFT 12U > +#define IMX335_PIXEL_ARRAY_TOP 12U > +#define IMX335_PIXEL_ARRAY_WIDTH 2592U > +#define IMX335_PIXEL_ARRAY_HEIGHT 1944U > + > /** > * struct imx335_reg - imx335 sensor register > * @address: Register address > @@ -651,6 +659,41 @@ static int imx335_init_pad_cfg(struct v4l2_subdev *sd, > return imx335_set_pad_format(sd, sd_state, &fmt); > } > > +/** > + * imx335_get_selection() - Selection API > + * @sd: pointer to imx335 V4L2 sub-device structure > + * @sd_state: V4L2 sub-device configuration > + * @sel: V4L2 selection info > + * > + * Return: 0 if successful, error code otherwise. > + */ > +static int imx335_get_selection(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *sd_state, > + struct v4l2_subdev_selection *sel) > +{ > + switch (sel->target) { > + case V4L2_SEL_TGT_NATIVE_SIZE: > + sel->r.top = 0; > + sel->r.left = 0; > + sel->r.width = IMX335_NATIVE_WIDTH; > + sel->r.height = IMX335_NATIVE_HEIGHT; > + > + return 0; > + > + case V4L2_SEL_TGT_CROP: > + case V4L2_SEL_TGT_CROP_DEFAULT: > + case V4L2_SEL_TGT_CROP_BOUNDS: > + sel->r.top = IMX335_PIXEL_ARRAY_TOP; > + sel->r.left = IMX335_PIXEL_ARRAY_LEFT; > + sel->r.width = IMX335_PIXEL_ARRAY_WIDTH; > + sel->r.height = IMX335_PIXEL_ARRAY_HEIGHT; > + > + return 0; > + } > + > + return -EINVAL; > +} > + > /** > * imx335_start_streaming() - Start sensor stream > * @imx335: pointer to imx335 device > @@ -864,6 +907,7 @@ static const struct v4l2_subdev_pad_ops imx335_pad_ops = { > .init_cfg = imx335_init_pad_cfg, > .enum_mbus_code = imx335_enum_mbus_code, > .enum_frame_size = imx335_enum_frame_size, > + .get_selection = imx335_get_selection, > .get_fmt = imx335_get_pad_format, > .set_fmt = imx335_set_pad_format, > };
Hi Kieran, On Wed, Oct 11, 2023 at 10:58:38AM +0100, Kieran Bingham wrote: > Quoting Sakari Ailus (2023-10-10 07:14:09) > > Hi Kieran, > > > > On Tue, Oct 10, 2023 at 01:51:24AM +0100, Kieran Bingham wrote: > > > Support reporting of the Sensor Native and Active pixel array areas > > > through the Selection API. > > > > > > The implementation reports a single target crop only for the mode that > > > is presently exposed by the driver. > > > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > Shouldn't you use the same callback for .set_selection? I guess this is > > somewhat grey area but doing so would be in line with how V4L2 API works in > > general. > > Hrm ... I didn't think it was needed as it's not possible to /set/ > anything. Similarly, VIDIOC_SUBDEV_S_FMT is available even if you can't change the format. > > I expect to change this once I add support for setting crops later > though. It was going to be something I'd add when it is used. > > Only the 'get_selection' call is necessary to make this camera operate > on both i.MX8MP and RPi5 platforms with libcamera, so that's what I've > done so far. My goal of this series was to bring the existing driver up > to a point that it can be used, before I start making new feature > additions. I don't have concerns with that, just that we implement the IOCTLs consitently. This has been discussed before but AFAIR without any firm conclusions. Additionally, some targets are settable while some won't be, and it may well depend on the driver. v4l2-compliance appears to be happy with G_SELECTION without S_SELECTION though. Also cc Hans.
diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c index bf12b9b69fce..026777eb362e 100644 --- a/drivers/media/i2c/imx335.c +++ b/drivers/media/i2c/imx335.c @@ -55,6 +55,14 @@ #define IMX335_REG_MIN 0x00 #define IMX335_REG_MAX 0xfffff +/* IMX335 native and active pixel array size. */ +#define IMX335_NATIVE_WIDTH 2616U +#define IMX335_NATIVE_HEIGHT 1964U +#define IMX335_PIXEL_ARRAY_LEFT 12U +#define IMX335_PIXEL_ARRAY_TOP 12U +#define IMX335_PIXEL_ARRAY_WIDTH 2592U +#define IMX335_PIXEL_ARRAY_HEIGHT 1944U + /** * struct imx335_reg - imx335 sensor register * @address: Register address @@ -651,6 +659,41 @@ static int imx335_init_pad_cfg(struct v4l2_subdev *sd, return imx335_set_pad_format(sd, sd_state, &fmt); } +/** + * imx335_get_selection() - Selection API + * @sd: pointer to imx335 V4L2 sub-device structure + * @sd_state: V4L2 sub-device configuration + * @sel: V4L2 selection info + * + * Return: 0 if successful, error code otherwise. + */ +static int imx335_get_selection(struct v4l2_subdev *sd, + struct v4l2_subdev_state *sd_state, + struct v4l2_subdev_selection *sel) +{ + switch (sel->target) { + case V4L2_SEL_TGT_NATIVE_SIZE: + sel->r.top = 0; + sel->r.left = 0; + sel->r.width = IMX335_NATIVE_WIDTH; + sel->r.height = IMX335_NATIVE_HEIGHT; + + return 0; + + case V4L2_SEL_TGT_CROP: + case V4L2_SEL_TGT_CROP_DEFAULT: + case V4L2_SEL_TGT_CROP_BOUNDS: + sel->r.top = IMX335_PIXEL_ARRAY_TOP; + sel->r.left = IMX335_PIXEL_ARRAY_LEFT; + sel->r.width = IMX335_PIXEL_ARRAY_WIDTH; + sel->r.height = IMX335_PIXEL_ARRAY_HEIGHT; + + return 0; + } + + return -EINVAL; +} + /** * imx335_start_streaming() - Start sensor stream * @imx335: pointer to imx335 device @@ -864,6 +907,7 @@ static const struct v4l2_subdev_pad_ops imx335_pad_ops = { .init_cfg = imx335_init_pad_cfg, .enum_mbus_code = imx335_enum_mbus_code, .enum_frame_size = imx335_enum_frame_size, + .get_selection = imx335_get_selection, .get_fmt = imx335_get_pad_format, .set_fmt = imx335_set_pad_format, };
Support reporting of the Sensor Native and Active pixel array areas through the Selection API. The implementation reports a single target crop only for the mode that is presently exposed by the driver. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- drivers/media/i2c/imx335.c | 44 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+)