diff mbox series

[3/5] media: i2c: imx335: Implement get selection API

Message ID 20231010005126.3425444-4-kieran.bingham@ideasonboard.com
State New
Headers
Series [1/5] media: dt-bindings: media: imx335: Add supply bindings |

Commit Message

Kieran Bingham Oct. 10, 2023, 12:51 a.m. UTC 
  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(+)

Comments

Umang Jain Oct. 10, 2023, 4:16 a.m. UTC | #1 
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,
>   };
Sakari Ailus Oct. 10, 2023, 6:14 a.m. UTC | #2 
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,
>  };
Sakari Ailus Oct. 11, 2023, 11:12 a.m. UTC | #3 
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 mbox series

Patch

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,
 };