diff mbox series

[v2,15/30] media: ov5647: Add support for get_selection()

Message ID 20201104103622.595908-16-jacopo@jmondi.org
State Superseded
Headers show
Series media: ov5647: Support RaspberryPi Camera Module v1 | expand

Commit Message

Jacopo Mondi Nov. 4, 2020, 10:36 a.m. UTC 
From: Dave Stevenson <dave.stevenson@raspberrypi.com>

Support the get_selection() pad operation to report the device
full pixel array size, the currently applied analogue crop rectangle and
the active pixel array dimensions.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov5647.c | 93 ++++++++++++++++++++++++++++----------
 1 file changed, 70 insertions(+), 23 deletions(-)

Comments

Hans Verkuil Nov. 6, 2020, 9:42 a.m. UTC | #1 
On 04/11/2020 11:36, Jacopo Mondi wrote:
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> 
> Support the get_selection() pad operation to report the device
> full pixel array size, the currently applied analogue crop rectangle and
> the active pixel array dimensions.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/i2c/ov5647.c | 93 ++++++++++++++++++++++++++++----------
>  1 file changed, 70 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
> index d41c11afe1216..65fcd86dcba96 100644
> --- a/drivers/media/i2c/ov5647.c
> +++ b/drivers/media/i2c/ov5647.c
> @@ -59,25 +59,14 @@
>  #define VAL_TERM 0xfe
>  #define REG_DLY  0xffff
>  
> -#define OV5647_ROW_START		0x01
> -#define OV5647_ROW_START_MIN		0
> -#define OV5647_ROW_START_MAX		2004
> -#define OV5647_ROW_START_DEF		54
> -
> -#define OV5647_COLUMN_START		0x02
> -#define OV5647_COLUMN_START_MIN		0
> -#define OV5647_COLUMN_START_MAX		2750
> -#define OV5647_COLUMN_START_DEF		16
> -
> -#define OV5647_WINDOW_HEIGHT		0x03
> -#define OV5647_WINDOW_HEIGHT_MIN	2
> -#define OV5647_WINDOW_HEIGHT_MAX	2006
> -#define OV5647_WINDOW_HEIGHT_DEF	1944
> -
> -#define OV5647_WINDOW_WIDTH		0x04
> -#define OV5647_WINDOW_WIDTH_MIN		2
> -#define OV5647_WINDOW_WIDTH_MAX		2752
> -#define OV5647_WINDOW_WIDTH_DEF		2592
> +/* OV5647 native and active pixel array size */
> +#define OV5647_NATIVE_WIDTH		2624U
> +#define OV5647_NATIVE_HEIGHT		1956U
> +
> +#define OV5647_PIXEL_ARRAY_LEFT		16U
> +#define OV5647_PIXEL_ARRAY_TOP		16U
> +#define OV5647_PIXEL_ARRAY_WIDTH	2592U
> +#define OV5647_PIXEL_ARRAY_HEIGHT	1944U
>  
>  struct regval_list {
>  	u16 addr;
> @@ -86,6 +75,7 @@ struct regval_list {
>  
>  struct ov5647_mode {
>  	struct v4l2_mbus_framefmt	format;
> +	struct v4l2_rect		crop;
>  	const struct regval_list	*reg_list;
>  	unsigned int			num_regs;
>  };
> @@ -224,6 +214,12 @@ static const struct ov5647_mode ov5647_8bit_modes[] = {
>  			.width		= 640,
>  			.height		= 480
>  		},
> +		.crop = {
> +			.left		= 0,
> +			.top		= 0,
> +			.width		= 1280,
> +			.height		= 960,
> +		},
>  		.reg_list	= ov5647_640x480,
>  		.num_regs	= ARRAY_SIZE(ov5647_640x480)
>  	},
> @@ -509,6 +505,20 @@ static const struct v4l2_subdev_core_ops ov5647_subdev_core_ops = {
>  #endif
>  };
>  
> +static const struct v4l2_rect *
> +__ov5647_get_pad_crop(struct ov5647 *ov5647, struct v4l2_subdev_pad_config *cfg,
> +		      unsigned int pad, enum v4l2_subdev_format_whence which)
> +{
> +	switch (which) {
> +	case V4L2_SUBDEV_FORMAT_TRY:
> +		return v4l2_subdev_get_try_crop(&ov5647->sd, cfg, pad);
> +	case V4L2_SUBDEV_FORMAT_ACTIVE:
> +		return &ov5647->mode->crop;
> +	}
> +
> +	return NULL;
> +}
> +
>  static int ov5647_s_stream(struct v4l2_subdev *sd, int enable)
>  {
>  	struct ov5647 *sensor = to_sensor(sd);
> @@ -579,11 +589,48 @@ static int ov5647_set_get_fmt(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static int ov5647_get_selection(struct v4l2_subdev *sd,
> +				struct v4l2_subdev_pad_config *cfg,
> +				struct v4l2_subdev_selection *sel)
> +{
> +	switch (sel->target) {
> +	case V4L2_SEL_TGT_CROP: {
> +		struct ov5647 *sensor = to_sensor(sd);
> +
> +		mutex_lock(&sensor->lock);
> +		sel->r = *__ov5647_get_pad_crop(sensor, cfg, sel->pad,
> +						sel->which);
> +		mutex_unlock(&sensor->lock);
> +
> +		return 0;
> +	}
> +
> +	case V4L2_SEL_TGT_NATIVE_SIZE:
> +		sel->r.top = 0;
> +		sel->r.left = 0;
> +		sel->r.width = OV5647_NATIVE_WIDTH;
> +		sel->r.height = OV5647_NATIVE_HEIGHT;
> +
> +		return 0;
> +
> +	case V4L2_SEL_TGT_CROP_DEFAULT:
> +		sel->r.top = OV5647_PIXEL_ARRAY_TOP;
> +		sel->r.left = OV5647_PIXEL_ARRAY_LEFT;
> +		sel->r.width = OV5647_PIXEL_ARRAY_WIDTH;
> +		sel->r.height = OV5647_PIXEL_ARRAY_HEIGHT;
> +
> +		return 0;

You're missing V4L2_SEL_TGT_CROP_BOUNDS. In order to support VIDIOC_CROPCAP
it is required that g_selection supports both V4L2_SEL_TGT_CROP_DEFAULT and
V4L2_SEL_TGT_CROP_BOUNDS.

So this is the reason why v4l2-compliance fails that test.

I'm improving the comment in that test to make it easier to pinpoint the
reason for the fail.

Regards,

	Hans

> +	}
> +
> +	return -EINVAL;
> +}
> +
>  static const struct v4l2_subdev_pad_ops ov5647_subdev_pad_ops = {
>  	.enum_mbus_code		= ov5647_enum_mbus_code,
>  	.enum_frame_size	= ov5647_enum_frame_size,
>  	.set_fmt		= ov5647_set_get_fmt,
>  	.get_fmt		= ov5647_set_get_fmt,
> +	.get_selection		= ov5647_get_selection,
>  };
>  
>  static const struct v4l2_subdev_ops ov5647_subdev_ops = {
> @@ -628,10 +675,10 @@ static int ov5647_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
>  	struct v4l2_mbus_framefmt *format = v4l2_subdev_get_try_format(sd, fh->pad, 0);
>  	struct v4l2_rect *crop = v4l2_subdev_get_try_crop(sd, fh->pad, 0);
>  
> -	crop->left = OV5647_COLUMN_START_DEF;
> -	crop->top = OV5647_ROW_START_DEF;
> -	crop->width = OV5647_WINDOW_WIDTH_DEF;
> -	crop->height = OV5647_WINDOW_HEIGHT_DEF;
> +	crop->left = OV5647_PIXEL_ARRAY_LEFT;
> +	crop->top = OV5647_PIXEL_ARRAY_TOP;
> +	crop->width = OV5647_PIXEL_ARRAY_WIDTH;
> +	crop->height = OV5647_PIXEL_ARRAY_HEIGHT;
>  
>  	*format = OV5647_DEFAULT_FORMAT;
>  
>
diff mbox series

Patch

diff --git a/drivers/media/i2c/ov5647.c b/drivers/media/i2c/ov5647.c
index d41c11afe1216..65fcd86dcba96 100644
--- a/drivers/media/i2c/ov5647.c
+++ b/drivers/media/i2c/ov5647.c
@@ -59,25 +59,14 @@ 
 #define VAL_TERM 0xfe
 #define REG_DLY  0xffff
 
-#define OV5647_ROW_START		0x01
-#define OV5647_ROW_START_MIN		0
-#define OV5647_ROW_START_MAX		2004
-#define OV5647_ROW_START_DEF		54
-
-#define OV5647_COLUMN_START		0x02
-#define OV5647_COLUMN_START_MIN		0
-#define OV5647_COLUMN_START_MAX		2750
-#define OV5647_COLUMN_START_DEF		16
-
-#define OV5647_WINDOW_HEIGHT		0x03
-#define OV5647_WINDOW_HEIGHT_MIN	2
-#define OV5647_WINDOW_HEIGHT_MAX	2006
-#define OV5647_WINDOW_HEIGHT_DEF	1944
-
-#define OV5647_WINDOW_WIDTH		0x04
-#define OV5647_WINDOW_WIDTH_MIN		2
-#define OV5647_WINDOW_WIDTH_MAX		2752
-#define OV5647_WINDOW_WIDTH_DEF		2592
+/* OV5647 native and active pixel array size */
+#define OV5647_NATIVE_WIDTH		2624U
+#define OV5647_NATIVE_HEIGHT		1956U
+
+#define OV5647_PIXEL_ARRAY_LEFT		16U
+#define OV5647_PIXEL_ARRAY_TOP		16U
+#define OV5647_PIXEL_ARRAY_WIDTH	2592U
+#define OV5647_PIXEL_ARRAY_HEIGHT	1944U
 
 struct regval_list {
 	u16 addr;
@@ -86,6 +75,7 @@  struct regval_list {
 
 struct ov5647_mode {
 	struct v4l2_mbus_framefmt	format;
+	struct v4l2_rect		crop;
 	const struct regval_list	*reg_list;
 	unsigned int			num_regs;
 };
@@ -224,6 +214,12 @@  static const struct ov5647_mode ov5647_8bit_modes[] = {
 			.width		= 640,
 			.height		= 480
 		},
+		.crop = {
+			.left		= 0,
+			.top		= 0,
+			.width		= 1280,
+			.height		= 960,
+		},
 		.reg_list	= ov5647_640x480,
 		.num_regs	= ARRAY_SIZE(ov5647_640x480)
 	},
@@ -509,6 +505,20 @@  static const struct v4l2_subdev_core_ops ov5647_subdev_core_ops = {
 #endif
 };
 
+static const struct v4l2_rect *
+__ov5647_get_pad_crop(struct ov5647 *ov5647, struct v4l2_subdev_pad_config *cfg,
+		      unsigned int pad, enum v4l2_subdev_format_whence which)
+{
+	switch (which) {
+	case V4L2_SUBDEV_FORMAT_TRY:
+		return v4l2_subdev_get_try_crop(&ov5647->sd, cfg, pad);
+	case V4L2_SUBDEV_FORMAT_ACTIVE:
+		return &ov5647->mode->crop;
+	}
+
+	return NULL;
+}
+
 static int ov5647_s_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct ov5647 *sensor = to_sensor(sd);
@@ -579,11 +589,48 @@  static int ov5647_set_get_fmt(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int ov5647_get_selection(struct v4l2_subdev *sd,
+				struct v4l2_subdev_pad_config *cfg,
+				struct v4l2_subdev_selection *sel)
+{
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP: {
+		struct ov5647 *sensor = to_sensor(sd);
+
+		mutex_lock(&sensor->lock);
+		sel->r = *__ov5647_get_pad_crop(sensor, cfg, sel->pad,
+						sel->which);
+		mutex_unlock(&sensor->lock);
+
+		return 0;
+	}
+
+	case V4L2_SEL_TGT_NATIVE_SIZE:
+		sel->r.top = 0;
+		sel->r.left = 0;
+		sel->r.width = OV5647_NATIVE_WIDTH;
+		sel->r.height = OV5647_NATIVE_HEIGHT;
+
+		return 0;
+
+	case V4L2_SEL_TGT_CROP_DEFAULT:
+		sel->r.top = OV5647_PIXEL_ARRAY_TOP;
+		sel->r.left = OV5647_PIXEL_ARRAY_LEFT;
+		sel->r.width = OV5647_PIXEL_ARRAY_WIDTH;
+		sel->r.height = OV5647_PIXEL_ARRAY_HEIGHT;
+
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
 static const struct v4l2_subdev_pad_ops ov5647_subdev_pad_ops = {
 	.enum_mbus_code		= ov5647_enum_mbus_code,
 	.enum_frame_size	= ov5647_enum_frame_size,
 	.set_fmt		= ov5647_set_get_fmt,
 	.get_fmt		= ov5647_set_get_fmt,
+	.get_selection		= ov5647_get_selection,
 };
 
 static const struct v4l2_subdev_ops ov5647_subdev_ops = {
@@ -628,10 +675,10 @@  static int ov5647_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
 	struct v4l2_mbus_framefmt *format = v4l2_subdev_get_try_format(sd, fh->pad, 0);
 	struct v4l2_rect *crop = v4l2_subdev_get_try_crop(sd, fh->pad, 0);
 
-	crop->left = OV5647_COLUMN_START_DEF;
-	crop->top = OV5647_ROW_START_DEF;
-	crop->width = OV5647_WINDOW_WIDTH_DEF;
-	crop->height = OV5647_WINDOW_HEIGHT_DEF;
+	crop->left = OV5647_PIXEL_ARRAY_LEFT;
+	crop->top = OV5647_PIXEL_ARRAY_TOP;
+	crop->width = OV5647_PIXEL_ARRAY_WIDTH;
+	crop->height = OV5647_PIXEL_ARRAY_HEIGHT;
 
 	*format = OV5647_DEFAULT_FORMAT;