[PATCH 1/2] media: imx274: improve v4l2-compliance

[PATCH 1/2] media: imx274: improve v4l2-compliance

[Bob Veringa]

1) Fix incorrect return of get_fmt when fmt->which is set
to V4L2_SUBDEV_FORMAT_TRY. get_fmt always returns the active format,
but should return the pad configuration when V4L2_SUBDEV_FORMAT_TRY
is requested.

2) Fix set_fmt not functioning correctly when fmt->which is set to
V4L2_SUBDEV_FORMAT_TRY. When a trying a format the driver assumes that
try_crop is already set. If this is not the case it causes
unexpected behavior. Add a default value if crop value is not set.

3) Add support for enumerating media bus formats
via VIDIOC_SUBDEV_ENUM_MBUS_CODE.

4) Add  support for enumerating frame sizes
via VIDIOC_SUBDEV_ENUM_FRAME_SIZE.

5) Fix v4l2-compliance error 'VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL'
by implementing v4l2_subdev_core_ops struct with default functions.

Signed-off-by: Bob Veringa <bob.veringa@topic.nl>
Acked-by: Mike Looijmans <mike.looijmans@topic.nl>
---
 drivers/media/i2c/imx274.c | 72 +++++++++++++++++++++++++++++++++++---
 1 file changed, 68 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index d9849d34f39f..942407ed931b 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -28,6 +28,7 @@
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-subdev.h>
+#include <media/v4l2-event.h>
 
 /*
  * See "SHR, SVR Setting" in datasheet
@@ -72,6 +73,7 @@
 #define IMX274_MAX_FRAME_RATE			(120)
 #define IMX274_MIN_FRAME_RATE			(5)
 #define IMX274_DEF_FRAME_RATE			(60)
+#define IMX274_DEFAULT_MEDIA_FORMAT		MEDIA_BUS_FMT_SRGGB10_1X10
 
 /*
  * register SHR is limited to (SVR value + 1) x VMAX value - 4
@@ -1011,7 +1013,7 @@ static int __imx274_change_compose(struct stimx274 *imx274,
 				   u32 flags)
 {
 	struct device *dev = &imx274->client->dev;
-	const struct v4l2_rect *cur_crop;
+	struct v4l2_rect *cur_crop;
 	struct v4l2_mbus_framefmt *tgt_fmt;
 	unsigned int i;
 	const struct imx274_mode *best_mode = &imx274_modes[0];
@@ -1025,6 +1027,11 @@ static int __imx274_change_compose(struct stimx274 *imx274,
 		tgt_fmt = &imx274->format;
 	}
 
+	if (cur_crop->height == 0 || cur_crop->width == 0) {
+		cur_crop->height = IMX274_MAX_HEIGHT;
+		cur_crop->width = IMX274_MAX_WIDTH;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(imx274_modes); i++) {
 		u8 wratio = imx274_modes[i].wbin_ratio;
 		u8 hratio = imx274_modes[i].hbin_ratio;
@@ -1072,11 +1079,24 @@ static int imx274_get_fmt(struct v4l2_subdev *sd,
 			  struct v4l2_subdev_format *fmt)
 {
 	struct stimx274 *imx274 = to_imx274(sd);
+	int ret = 0;
 
 	mutex_lock(&imx274->lock);
-	fmt->format = imx274->format;
+
+	switch (fmt->which) {
+	case V4L2_SUBDEV_FORMAT_TRY:
+		fmt->format = *v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
+		break;
+	case V4L2_SUBDEV_FORMAT_ACTIVE:
+		fmt->format = imx274->format;
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
 	mutex_unlock(&imx274->lock);
-	return 0;
+	return ret;
 }
 
 /**
@@ -1274,6 +1294,41 @@ static int imx274_set_selection(struct v4l2_subdev *sd,
 	return -EINVAL;
 }
 
+static int imx274_enum_frame_size(struct v4l2_subdev *sd,
+				  struct v4l2_subdev_pad_config *cfg,
+				  struct v4l2_subdev_frame_size_enum *fse)
+{
+	__u32 width, height;
+
+	if (fse->index >= ARRAY_SIZE(imx274_modes))
+		return -EINVAL;
+
+	width = IMX274_MAX_WIDTH / imx274_modes[fse->index].wbin_ratio;
+	height = IMX274_MAX_HEIGHT / imx274_modes[fse->index].hbin_ratio;
+
+	fse->code = IMX274_DEFAULT_MEDIA_FORMAT;
+	fse->min_width = width;
+	fse->max_width = width;
+	fse->min_height = height;
+	fse->max_height = height;
+	return 0;
+}
+
+static int imx274_enum_mbus_code(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_pad_config *cfg,
+				 struct v4l2_subdev_mbus_code_enum *code)
+{
+	if (code->index > 0)
+		return -EINVAL;
+
+	if (code->pad != 0)
+		return -EINVAL;
+
+	code->code = IMX274_DEFAULT_MEDIA_FORMAT;
+
+	return 0;
+}
+
 static int imx274_apply_trimming(struct stimx274 *imx274)
 {
 	u32 h_start;
@@ -1903,11 +1958,19 @@ static int imx274_set_frame_interval(struct stimx274 *priv,
 	return err;
 }
 
+static const struct v4l2_subdev_core_ops imx274_core_ops = {
+	.log_status = v4l2_ctrl_subdev_log_status,
+	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
+	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
+};
+
 static const struct v4l2_subdev_pad_ops imx274_pad_ops = {
 	.get_fmt = imx274_get_fmt,
 	.set_fmt = imx274_set_fmt,
 	.get_selection = imx274_get_selection,
 	.set_selection = imx274_set_selection,
+	.enum_mbus_code = imx274_enum_mbus_code,
+	.enum_frame_size = imx274_enum_frame_size,
 };
 
 static const struct v4l2_subdev_video_ops imx274_video_ops = {
@@ -1917,6 +1980,7 @@ static const struct v4l2_subdev_video_ops imx274_video_ops = {
 };
 
 static const struct v4l2_subdev_ops imx274_subdev_ops = {
+	.core = &imx274_core_ops,
 	.pad = &imx274_pad_ops,
 	.video = &imx274_video_ops,
 };
@@ -1968,7 +2032,7 @@ static int imx274_probe(struct i2c_client *client)
 	imx274->format.width = imx274->crop.width / imx274->mode->wbin_ratio;
 	imx274->format.height = imx274->crop.height / imx274->mode->hbin_ratio;
 	imx274->format.field = V4L2_FIELD_NONE;
-	imx274->format.code = MEDIA_BUS_FMT_SRGGB10_1X10;
+	imx274->format.code = IMX274_DEFAULT_MEDIA_FORMAT;
 	imx274->format.colorspace = V4L2_COLORSPACE_SRGB;
 	imx274->frame_interval.numerator = 1;
 	imx274->frame_interval.denominator = IMX274_DEF_FRAME_RATE;
-- 
2.17.1


Met vriendelijke groet / kind regards,

Bob Veringa


TOPIC Embedded Systems B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 79
E: bob.Veringa@topic.nl
W: www.topic.nl

Please consider the environment before printing this e-mail

[PATCH 2/2] media: imx274: remove imx274_load_default

[Bob Veringa]

The function imx274_load_default no longer serves a purpose as it
does not configure the values since
commit ad97bc37426c1eec1464 ("media: i2c: imx274: Add IMX274
power on and off sequence')

All the values set in this function match the default value set when
registering the controls, except for exposure. The default value
of this is set to the minimal value for this field. The result of this 
is that the output video is black when exposure is not explicitly 
set by the user. By setting the default value when registering 
the control to the same value originally used in the function, 
this issue is avoided.

Signed-off-by: Bob Veringa <bob.veringa@topic.nl>
Acked-by: Mike Looijmans <mike.looijmans@topic.nl>
---
 drivers/media/i2c/imx274.c | 24 ++----------------------
 1 file changed, 2 insertions(+), 22 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 942407ed931b..9dc0bcb1e0df 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -1455,23 +1455,6 @@ static int imx274_s_frame_interval(struct v4l2_subdev *sd,
 	return ret;
 }
 
-/**
- * imx274_load_default - load default control values
- * @priv: Pointer to device structure
- *
- * Return: 0 on success, errors otherwise
- */
-static void imx274_load_default(struct stimx274 *priv)
-{
-	/* load default control values */
-	priv->frame_interval.numerator = 1;
-	priv->frame_interval.denominator = IMX274_DEF_FRAME_RATE;
-	priv->ctrls.exposure->val = 1000000 / IMX274_DEF_FRAME_RATE;
-	priv->ctrls.gain->val = IMX274_DEF_GAIN;
-	priv->ctrls.vflip->val = 0;
-	priv->ctrls.test_pattern->val = TEST_PATTERN_DISABLED;
-}
-
 /**
  * imx274_s_stream - It is used to start/stop the streaming.
  * @sd: V4L2 Sub device
@@ -2094,7 +2077,7 @@ static int imx274_probe(struct i2c_client *client)
 	imx274->ctrls.test_pattern = v4l2_ctrl_new_std_menu_items(
 		&imx274->ctrls.handler, &imx274_ctrl_ops,
 		V4L2_CID_TEST_PATTERN,
-		ARRAY_SIZE(tp_qmenu) - 1, 0, 0, tp_qmenu);
+		ARRAY_SIZE(tp_qmenu) - 1, 0, TEST_PATTERN_DISABLED, tp_qmenu);
 
 	imx274->ctrls.gain = v4l2_ctrl_new_std(
 		&imx274->ctrls.handler,
@@ -2108,7 +2091,7 @@ static int imx274_probe(struct i2c_client *client)
 		&imx274_ctrl_ops,
 		V4L2_CID_EXPOSURE, IMX274_MIN_EXPOSURE_TIME,
 		1000000 / IMX274_DEF_FRAME_RATE, 1,
-		IMX274_MIN_EXPOSURE_TIME);
+		1000000 / IMX274_DEF_FRAME_RATE);
 
 	imx274->ctrls.vflip = v4l2_ctrl_new_std(
 		&imx274->ctrls.handler,
@@ -2121,9 +2104,6 @@ static int imx274_probe(struct i2c_client *client)
 		goto err_ctrls;
 	}
 
-	/* load default control values */
-	imx274_load_default(imx274);
-
 	/* register subdevice */
 	ret = v4l2_async_register_subdev(sd);
 	if (ret < 0) {
-- 
2.17.1


Met vriendelijke groet / kind regards,

Bob Veringa


TOPIC Embedded Systems B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 79
E: bob.Veringa@topic.nl
W: www.topic.nl

Please consider the environment before printing this e-mail

Re: [PATCH 1/2] media: imx274: improve v4l2-compliance

[Sakari Ailus]

Hi Bob,

Thanks for the patch.

On Sat, Apr 24, 2021 at 01:14:58PM +0200, Bob Veringa wrote:
> 1) Fix incorrect return of get_fmt when fmt->which is set
> to V4L2_SUBDEV_FORMAT_TRY. get_fmt always returns the active format,
> but should return the pad configuration when V4L2_SUBDEV_FORMAT_TRY
> is requested.
> 
> 2) Fix set_fmt not functioning correctly when fmt->which is set to
> V4L2_SUBDEV_FORMAT_TRY. When a trying a format the driver assumes that
> try_crop is already set. If this is not the case it causes
> unexpected behavior. Add a default value if crop value is not set.
> 
> 3) Add support for enumerating media bus formats
> via VIDIOC_SUBDEV_ENUM_MBUS_CODE.
> 
> 4) Add  support for enumerating frame sizes
> via VIDIOC_SUBDEV_ENUM_FRAME_SIZE.
> 
> 5) Fix v4l2-compliance error 'VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: FAIL'
> by implementing v4l2_subdev_core_ops struct with default functions.
> 
> Signed-off-by: Bob Veringa <bob.veringa@topic.nl>
> Acked-by: Mike Looijmans <mike.looijmans@topic.nl>
> ---
>  drivers/media/i2c/imx274.c | 72 +++++++++++++++++++++++++++++++++++---
>  1 file changed, 68 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> index d9849d34f39f..942407ed931b 100644
> --- a/drivers/media/i2c/imx274.c
> +++ b/drivers/media/i2c/imx274.c
> @@ -28,6 +28,7 @@
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-subdev.h>
> +#include <media/v4l2-event.h>
>  
>  /*
>   * See "SHR, SVR Setting" in datasheet
> @@ -72,6 +73,7 @@
>  #define IMX274_MAX_FRAME_RATE			(120)
>  #define IMX274_MIN_FRAME_RATE			(5)
>  #define IMX274_DEF_FRAME_RATE			(60)
> +#define IMX274_DEFAULT_MEDIA_FORMAT		MEDIA_BUS_FMT_SRGGB10_1X10
>  
>  /*
>   * register SHR is limited to (SVR value + 1) x VMAX value - 4
> @@ -1011,7 +1013,7 @@ static int __imx274_change_compose(struct stimx274 *imx274,
>  				   u32 flags)
>  {
>  	struct device *dev = &imx274->client->dev;
> -	const struct v4l2_rect *cur_crop;
> +	struct v4l2_rect *cur_crop;
>  	struct v4l2_mbus_framefmt *tgt_fmt;
>  	unsigned int i;
>  	const struct imx274_mode *best_mode = &imx274_modes[0];
> @@ -1025,6 +1027,11 @@ static int __imx274_change_compose(struct stimx274 *imx274,
>  		tgt_fmt = &imx274->format;
>  	}
>  
> +	if (cur_crop->height == 0 || cur_crop->width == 0) {
> +		cur_crop->height = IMX274_MAX_HEIGHT;
> +		cur_crop->width = IMX274_MAX_WIDTH;

Shouldn't this be done in probe / init_cfg (to be added to subdev ops),
unconditionally?

The try format should also be initialised there. Both should be device
defaults, i.e. the same as those set in probe.

> +	}
> +
>  	for (i = 0; i < ARRAY_SIZE(imx274_modes); i++) {
>  		u8 wratio = imx274_modes[i].wbin_ratio;
>  		u8 hratio = imx274_modes[i].hbin_ratio;
> @@ -1072,11 +1079,24 @@ static int imx274_get_fmt(struct v4l2_subdev *sd,
>  			  struct v4l2_subdev_format *fmt)
>  {
>  	struct stimx274 *imx274 = to_imx274(sd);
> +	int ret = 0;
>  
>  	mutex_lock(&imx274->lock);
> -	fmt->format = imx274->format;
> +
> +	switch (fmt->which) {
> +	case V4L2_SUBDEV_FORMAT_TRY:
> +		fmt->format = *v4l2_subdev_get_try_format(sd, cfg, fmt->pad);
> +		break;
> +	case V4L2_SUBDEV_FORMAT_ACTIVE:
> +		fmt->format = imx274->format;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
>  	mutex_unlock(&imx274->lock);
> -	return 0;
> +	return ret;
>  }
>  
>  /**
> @@ -1274,6 +1294,41 @@ static int imx274_set_selection(struct v4l2_subdev *sd,
>  	return -EINVAL;
>  }
>  
> +static int imx274_enum_frame_size(struct v4l2_subdev *sd,
> +				  struct v4l2_subdev_pad_config *cfg,
> +				  struct v4l2_subdev_frame_size_enum *fse)
> +{
> +	__u32 width, height;

u32, please. __u32 is for the user space facing headers.

> +
> +	if (fse->index >= ARRAY_SIZE(imx274_modes))
> +		return -EINVAL;
> +
> +	width = IMX274_MAX_WIDTH / imx274_modes[fse->index].wbin_ratio;
> +	height = IMX274_MAX_HEIGHT / imx274_modes[fse->index].hbin_ratio;
> +
> +	fse->code = IMX274_DEFAULT_MEDIA_FORMAT;
> +	fse->min_width = width;
> +	fse->max_width = width;
> +	fse->min_height = height;
> +	fse->max_height = height;
> +	return 0;
> +}
> +
> +static int imx274_enum_mbus_code(struct v4l2_subdev *sd,
> +				 struct v4l2_subdev_pad_config *cfg,
> +				 struct v4l2_subdev_mbus_code_enum *code)
> +{
> +	if (code->index > 0)
> +		return -EINVAL;
> +
> +	if (code->pad != 0)
> +		return -EINVAL;

The caller has checked the pad field is valid, and with a single pad in the
entity no additional check is necessary.

> +
> +	code->code = IMX274_DEFAULT_MEDIA_FORMAT;
> +
> +	return 0;
> +}
> +
>  static int imx274_apply_trimming(struct stimx274 *imx274)
>  {
>  	u32 h_start;
> @@ -1903,11 +1958,19 @@ static int imx274_set_frame_interval(struct stimx274 *priv,
>  	return err;
>  }
>  
> +static const struct v4l2_subdev_core_ops imx274_core_ops = {
> +	.log_status = v4l2_ctrl_subdev_log_status,
> +	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
> +	.unsubscribe_event = v4l2_event_subdev_unsubscribe,
> +};
> +
>  static const struct v4l2_subdev_pad_ops imx274_pad_ops = {
>  	.get_fmt = imx274_get_fmt,
>  	.set_fmt = imx274_set_fmt,
>  	.get_selection = imx274_get_selection,
>  	.set_selection = imx274_set_selection,
> +	.enum_mbus_code = imx274_enum_mbus_code,
> +	.enum_frame_size = imx274_enum_frame_size,
>  };
>  
>  static const struct v4l2_subdev_video_ops imx274_video_ops = {
> @@ -1917,6 +1980,7 @@ static const struct v4l2_subdev_video_ops imx274_video_ops = {
>  };
>  
>  static const struct v4l2_subdev_ops imx274_subdev_ops = {
> +	.core = &imx274_core_ops,
>  	.pad = &imx274_pad_ops,
>  	.video = &imx274_video_ops,
>  };
> @@ -1968,7 +2032,7 @@ static int imx274_probe(struct i2c_client *client)
>  	imx274->format.width = imx274->crop.width / imx274->mode->wbin_ratio;
>  	imx274->format.height = imx274->crop.height / imx274->mode->hbin_ratio;
>  	imx274->format.field = V4L2_FIELD_NONE;
> -	imx274->format.code = MEDIA_BUS_FMT_SRGGB10_1X10;
> +	imx274->format.code = IMX274_DEFAULT_MEDIA_FORMAT;
>  	imx274->format.colorspace = V4L2_COLORSPACE_SRGB;
>  	imx274->frame_interval.numerator = 1;
>  	imx274->frame_interval.denominator = IMX274_DEF_FRAME_RATE;

-- 
Kind regards,

Sakari Ailus