[PATCH v2 0/7] media: i2c: imx219: Use subdev active state

[PATCH v2 0/7] media: i2c: imx219: Use subdev active state

[Jacopo Mondi]

Hello,
   this series ports the imx219 to use the subdev active state with great
simplification of format handling, locking and device initialization.

It resumes the work started by Jean-Michel but had to be ported to the new API
and applied on the most recent version of the sensor driver.

Tested with libcamera, by streaming in 1080p, VGA applying V/H flips to make
sure the mbus code permutation handling is still correct.

Thanks
   j

v1->v2:
- Two additional patches to fix colorspace handling a suggested by Laurent

Jacopo Mondi (5):
  media: i2c: imx219: Complete default format initialization
  media: i2c: imx219: Fix colorspace info
  media: i2c: imx219: Use subdev active state
  media: i2c: imx219: Simplify format assignment
  media: i2c: imx219: Simplify code handling in s_fmt

Jean-Michel Hautbois (2):
  media: i2c: imx219: Rename mbus codes array
  media: i2c: imx219: Switch from open to init_cfg

 drivers/media/i2c/imx219.c | 292 ++++++++++++-------------------------
 1 file changed, 93 insertions(+), 199 deletions(-)

--
2.40.1

[PATCH v2 1/7] media: i2c: imx219: Rename mbus codes array

[Jacopo Mondi]

From: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>

The imx219 is using the name "codes" for the mbus formats array. The
name is too generic and not easy to read and follow in the code. Change
it to imx219_mbus_formats.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Tommaso Merciai <tomm.merciai@gmail.com>
---
 drivers/media/i2c/imx219.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index d737d5e9a4a6..ac6b0e7a838d 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -345,7 +345,7 @@ static const char * const imx219_supply_name[] = {
  * - v flip
  * - h&v flips
  */
-static const u32 codes[] = {
+static const u32 imx219_mbus_formats[] = {
 	MEDIA_BUS_FMT_SRGGB10_1X10,
 	MEDIA_BUS_FMT_SGRBG10_1X10,
 	MEDIA_BUS_FMT_SGBRG10_1X10,
@@ -578,17 +578,17 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
 
 	lockdep_assert_held(&imx219->mutex);
 
-	for (i = 0; i < ARRAY_SIZE(codes); i++)
-		if (codes[i] == code)
+	for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
+		if (imx219_mbus_formats[i] == code)
 			break;
 
-	if (i >= ARRAY_SIZE(codes))
+	if (i >= ARRAY_SIZE(imx219_mbus_formats))
 		i = 0;
 
 	i = (i & ~3) | (imx219->vflip->val ? 2 : 0) |
 	    (imx219->hflip->val ? 1 : 0);
 
-	return codes[i];
+	return imx219_mbus_formats[i];
 }
 
 static void imx219_set_default_format(struct imx219 *imx219)
@@ -731,11 +731,11 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
 {
 	struct imx219 *imx219 = to_imx219(sd);
 
-	if (code->index >= (ARRAY_SIZE(codes) / 4))
+	if (code->index >= (ARRAY_SIZE(imx219_mbus_formats) / 4))
 		return -EINVAL;
 
 	mutex_lock(&imx219->mutex);
-	code->code = imx219_get_format_code(imx219, codes[code->index * 4]);
+	code->code = imx219_get_format_code(imx219, imx219_mbus_formats[code->index * 4]);
 	mutex_unlock(&imx219->mutex);
 
 	return 0;
@@ -831,14 +831,14 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 
 	mutex_lock(&imx219->mutex);
 
-	for (i = 0; i < ARRAY_SIZE(codes); i++)
-		if (codes[i] == fmt->format.code)
+	for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
+		if (imx219_mbus_formats[i] == fmt->format.code)
 			break;
-	if (i >= ARRAY_SIZE(codes))
+	if (i >= ARRAY_SIZE(imx219_mbus_formats))
 		i = 0;
 
 	/* Bayer order varies with flips */
-	fmt->format.code = imx219_get_format_code(imx219, codes[i]);
+	fmt->format.code = imx219_get_format_code(imx219, imx219_mbus_formats[i]);
 
 	mode = v4l2_find_nearest_size(supported_modes,
 				      ARRAY_SIZE(supported_modes),
-- 
2.40.1

[PATCH v2 2/7] media: i2c: imx219: Switch from open to init_cfg

[Jacopo Mondi]

From: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>

Use the init_cfg pad level operation instead of the internal subdev
open operation to set default formats on the pads.

Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/imx219.c | 63 +++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 32 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index ac6b0e7a838d..45b219321d98 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -608,34 +608,6 @@ static void imx219_set_default_format(struct imx219 *imx219)
 	fmt->field = V4L2_FIELD_NONE;
 }
 
-static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
-{
-	struct imx219 *imx219 = to_imx219(sd);
-	struct v4l2_mbus_framefmt *try_fmt =
-		v4l2_subdev_get_try_format(sd, fh->state, 0);
-	struct v4l2_rect *try_crop;
-
-	mutex_lock(&imx219->mutex);
-
-	/* Initialize try_fmt */
-	try_fmt->width = supported_modes[0].width;
-	try_fmt->height = supported_modes[0].height;
-	try_fmt->code = imx219_get_format_code(imx219,
-					       MEDIA_BUS_FMT_SRGGB10_1X10);
-	try_fmt->field = V4L2_FIELD_NONE;
-
-	/* Initialize try_crop rectangle. */
-	try_crop = v4l2_subdev_get_try_crop(sd, fh->state, 0);
-	try_crop->top = IMX219_PIXEL_ARRAY_TOP;
-	try_crop->left = IMX219_PIXEL_ARRAY_LEFT;
-	try_crop->width = IMX219_PIXEL_ARRAY_WIDTH;
-	try_crop->height = IMX219_PIXEL_ARRAY_HEIGHT;
-
-	mutex_unlock(&imx219->mutex);
-
-	return 0;
-}
-
 static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct imx219 *imx219 =
@@ -725,6 +697,36 @@ static const struct v4l2_ctrl_ops imx219_ctrl_ops = {
 	.s_ctrl = imx219_set_ctrl,
 };
 
+static int imx219_init_cfg(struct v4l2_subdev *sd,
+			   struct v4l2_subdev_state *state)
+{
+	struct imx219 *imx219 = to_imx219(sd);
+	struct v4l2_mbus_framefmt *format;
+	struct v4l2_rect *crop;
+
+	/* imx219_get_format_code() wants mutex locked. */
+	mutex_lock(&imx219->mutex);
+
+	/* Initialize try_fmt */
+	format = v4l2_subdev_get_pad_format(sd, state, 0);
+	format->width = supported_modes[0].width;
+	format->height = supported_modes[0].height;
+	format->code = imx219_get_format_code(imx219,
+					      MEDIA_BUS_FMT_SRGGB10_1X10);
+	format->field = V4L2_FIELD_NONE;
+
+	/* Initialize crop rectangle. */
+	crop = v4l2_subdev_get_pad_crop(sd, state, 0);
+	crop->top = IMX219_PIXEL_ARRAY_TOP;
+	crop->left = IMX219_PIXEL_ARRAY_LEFT;
+	crop->width = IMX219_PIXEL_ARRAY_WIDTH;
+	crop->height = IMX219_PIXEL_ARRAY_HEIGHT;
+
+	mutex_unlock(&imx219->mutex);
+
+	return 0;
+}
+
 static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *sd_state,
 				 struct v4l2_subdev_mbus_code_enum *code)
@@ -1235,6 +1237,7 @@ static const struct v4l2_subdev_video_ops imx219_video_ops = {
 };
 
 static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
+	.init_cfg = imx219_init_cfg,
 	.enum_mbus_code = imx219_enum_mbus_code,
 	.get_fmt = imx219_get_pad_format,
 	.set_fmt = imx219_set_pad_format,
@@ -1248,9 +1251,6 @@ static const struct v4l2_subdev_ops imx219_subdev_ops = {
 	.pad = &imx219_pad_ops,
 };
 
-static const struct v4l2_subdev_internal_ops imx219_internal_ops = {
-	.open = imx219_open,
-};
 
 static unsigned long imx219_get_pixel_rate(struct imx219 *imx219)
 {
@@ -1509,7 +1509,6 @@ static int imx219_probe(struct i2c_client *client)
 		goto error_power_off;
 
 	/* Initialize subdev */
-	imx219->sd.internal_ops = &imx219_internal_ops;
 	imx219->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
 			    V4L2_SUBDEV_FL_HAS_EVENTS;
 	imx219->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
-- 
2.40.1

[PATCH v2 3/7] media: i2c: imx219: Complete default format initialization

[Jacopo Mondi]

Complete the default format initialization in init_cfg() filling in
the fields for the colorspace configuration copied from
imx219_set_default_format().

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 drivers/media/i2c/imx219.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 45b219321d98..cd43a897391c 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -714,6 +714,12 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
 	format->code = imx219_get_format_code(imx219,
 					      MEDIA_BUS_FMT_SRGGB10_1X10);
 	format->field = V4L2_FIELD_NONE;
+	format->colorspace = V4L2_COLORSPACE_SRGB;
+	format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);
+	format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
+							     format->colorspace,
+							     format->ycbcr_enc);
+	format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(format->colorspace);
 
 	/* Initialize crop rectangle. */
 	crop = v4l2_subdev_get_pad_crop(sd, state, 0);
-- 
2.40.1

[PATCH v2 4/7] media: i2c: imx219: Fix colorspace info

[Jacopo Mondi]

The IMX219 is a RAW sensor. Fix the colorspace configuration by
using V4L2_COLORSPACE_RAW and adjust the quantization and transfer
function values. Drop ycbcr_enc as it doesn't apply to RAW sensors.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 drivers/media/i2c/imx219.c | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index cd43a897391c..6963e24e1bc2 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -597,15 +597,12 @@ static void imx219_set_default_format(struct imx219 *imx219)
 
 	fmt = &imx219->fmt;
 	fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
-	fmt->colorspace = V4L2_COLORSPACE_SRGB;
-	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
-	fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
-							  fmt->colorspace,
-							  fmt->ycbcr_enc);
-	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
+	fmt->colorspace = V4L2_COLORSPACE_RAW;
+	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
 	fmt->width = supported_modes[0].width;
 	fmt->height = supported_modes[0].height;
 	fmt->field = V4L2_FIELD_NONE;
+	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
 }
 
 static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
@@ -714,12 +711,10 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
 	format->code = imx219_get_format_code(imx219,
 					      MEDIA_BUS_FMT_SRGGB10_1X10);
 	format->field = V4L2_FIELD_NONE;
-	format->colorspace = V4L2_COLORSPACE_SRGB;
+	format->colorspace = V4L2_COLORSPACE_RAW;
 	format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);
-	format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
-							     format->colorspace,
-							     format->ycbcr_enc);
-	format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(format->colorspace);
+	format->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+	format->xfer_func = V4L2_XFER_FUNC_NONE;
 
 	/* Initialize crop rectangle. */
 	crop = v4l2_subdev_get_pad_crop(sd, state, 0);
@@ -775,12 +770,9 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
 
 static void imx219_reset_colorspace(struct v4l2_mbus_framefmt *fmt)
 {
-	fmt->colorspace = V4L2_COLORSPACE_SRGB;
-	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
-	fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
-							  fmt->colorspace,
-							  fmt->ycbcr_enc);
-	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
+	fmt->colorspace = V4L2_COLORSPACE_RAW;
+	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
 }
 
 static void imx219_update_pad_format(struct imx219 *imx219,
-- 
2.40.1

[PATCH v2 5/7] media: i2c: imx219: Use subdev active state

[Jacopo Mondi]

Port the imx219 sensor driver to use the subdev active state.

Move all the format configuration to the subdevice state and simplify
the format handling, locking and initialization.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/imx219.c | 179 ++++++++++---------------------------
 1 file changed, 48 insertions(+), 131 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 6963e24e1bc2..73e06583d651 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -460,8 +460,6 @@ struct imx219 {
 	struct v4l2_subdev sd;
 	struct media_pad pad;
 
-	struct v4l2_mbus_framefmt fmt;
-
 	struct clk *xclk; /* system clock to IMX219 */
 	u32 xclk_freq;
 
@@ -481,12 +479,6 @@ struct imx219 {
 	/* Current mode */
 	const struct imx219_mode *mode;
 
-	/*
-	 * Mutex for serialized access:
-	 * Protect sensor module set pad format and start/stop streaming safely.
-	 */
-	struct mutex mutex;
-
 	/* Streaming on/off */
 	bool streaming;
 
@@ -576,8 +568,6 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
 {
 	unsigned int i;
 
-	lockdep_assert_held(&imx219->mutex);
-
 	for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
 		if (imx219_mbus_formats[i] == code)
 			break;
@@ -591,20 +581,6 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
 	return imx219_mbus_formats[i];
 }
 
-static void imx219_set_default_format(struct imx219 *imx219)
-{
-	struct v4l2_mbus_framefmt *fmt;
-
-	fmt = &imx219->fmt;
-	fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
-	fmt->colorspace = V4L2_COLORSPACE_RAW;
-	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
-	fmt->width = supported_modes[0].width;
-	fmt->height = supported_modes[0].height;
-	fmt->field = V4L2_FIELD_NONE;
-	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
-}
-
 static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct imx219 *imx219 =
@@ -701,9 +677,6 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
 	struct v4l2_mbus_framefmt *format;
 	struct v4l2_rect *crop;
 
-	/* imx219_get_format_code() wants mutex locked. */
-	mutex_lock(&imx219->mutex);
-
 	/* Initialize try_fmt */
 	format = v4l2_subdev_get_pad_format(sd, state, 0);
 	format->width = supported_modes[0].width;
@@ -723,8 +696,6 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
 	crop->width = IMX219_PIXEL_ARRAY_WIDTH;
 	crop->height = IMX219_PIXEL_ARRAY_HEIGHT;
 
-	mutex_unlock(&imx219->mutex);
-
 	return 0;
 }
 
@@ -737,9 +708,7 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
 	if (code->index >= (ARRAY_SIZE(imx219_mbus_formats) / 4))
 		return -EINVAL;
 
-	mutex_lock(&imx219->mutex);
 	code->code = imx219_get_format_code(imx219, imx219_mbus_formats[code->index * 4]);
-	mutex_unlock(&imx219->mutex);
 
 	return 0;
 }
@@ -754,9 +723,7 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
 	if (fse->index >= ARRAY_SIZE(supported_modes))
 		return -EINVAL;
 
-	mutex_lock(&imx219->mutex);
 	code = imx219_get_format_code(imx219, fse->code);
-	mutex_unlock(&imx219->mutex);
 	if (fse->code != code)
 		return -EINVAL;
 
@@ -785,52 +752,16 @@ static void imx219_update_pad_format(struct imx219 *imx219,
 	imx219_reset_colorspace(&fmt->format);
 }
 
-static int __imx219_get_pad_format(struct imx219 *imx219,
-				   struct v4l2_subdev_state *sd_state,
-				   struct v4l2_subdev_format *fmt)
-{
-	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
-		struct v4l2_mbus_framefmt *try_fmt =
-			v4l2_subdev_get_try_format(&imx219->sd, sd_state,
-						   fmt->pad);
-		/* update the code which could change due to vflip or hflip: */
-		try_fmt->code = imx219_get_format_code(imx219, try_fmt->code);
-		fmt->format = *try_fmt;
-	} else {
-		imx219_update_pad_format(imx219, imx219->mode, fmt);
-		fmt->format.code = imx219_get_format_code(imx219,
-							  imx219->fmt.code);
-	}
-
-	return 0;
-}
-
-static int imx219_get_pad_format(struct v4l2_subdev *sd,
-				 struct v4l2_subdev_state *sd_state,
-				 struct v4l2_subdev_format *fmt)
-{
-	struct imx219 *imx219 = to_imx219(sd);
-	int ret;
-
-	mutex_lock(&imx219->mutex);
-	ret = __imx219_get_pad_format(imx219, sd_state, fmt);
-	mutex_unlock(&imx219->mutex);
-
-	return ret;
-}
-
 static int imx219_set_pad_format(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *sd_state,
 				 struct v4l2_subdev_format *fmt)
 {
 	struct imx219 *imx219 = to_imx219(sd);
 	const struct imx219_mode *mode;
-	struct v4l2_mbus_framefmt *framefmt;
 	int exposure_max, exposure_def, hblank;
+	struct v4l2_mbus_framefmt *format;
 	unsigned int i;
 
-	mutex_lock(&imx219->mutex);
-
 	for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
 		if (imx219_mbus_formats[i] == fmt->format.code)
 			break;
@@ -844,13 +775,14 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 				      ARRAY_SIZE(supported_modes),
 				      width, height,
 				      fmt->format.width, fmt->format.height);
+
 	imx219_update_pad_format(imx219, mode, fmt);
-	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
-		framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
-		*framefmt = fmt->format;
-	} else if (imx219->mode != mode ||
-		   imx219->fmt.code != fmt->format.code) {
-		imx219->fmt = fmt->format;
+	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
+
+	if (imx219->mode == mode && format->code == fmt->format.code)
+		return 0;
+
+	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
 		imx219->mode = mode;
 		/* Update limits and set FPS to default */
 		__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
@@ -876,14 +808,15 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 					 hblank);
 	}
 
-	mutex_unlock(&imx219->mutex);
+	*format = fmt->format;
 
 	return 0;
 }
 
-static int imx219_set_framefmt(struct imx219 *imx219)
+static int imx219_set_framefmt(struct imx219 *imx219,
+			       const struct v4l2_mbus_framefmt *format)
 {
-	switch (imx219->fmt.code) {
+	switch (format->code) {
 	case MEDIA_BUS_FMT_SRGGB8_1X8:
 	case MEDIA_BUS_FMT_SGRBG8_1X8:
 	case MEDIA_BUS_FMT_SGBRG8_1X8:
@@ -902,7 +835,8 @@ static int imx219_set_framefmt(struct imx219 *imx219)
 	return -EINVAL;
 }
 
-static int imx219_set_binning(struct imx219 *imx219)
+static int imx219_set_binning(struct imx219 *imx219,
+			      const struct v4l2_mbus_framefmt *format)
 {
 	if (!imx219->mode->binning) {
 		return imx219_write_reg(imx219, IMX219_REG_BINNING_MODE,
@@ -910,7 +844,7 @@ static int imx219_set_binning(struct imx219 *imx219)
 					IMX219_BINNING_NONE);
 	}
 
-	switch (imx219->fmt.code) {
+	switch (format->code) {
 	case MEDIA_BUS_FMT_SRGGB8_1X8:
 	case MEDIA_BUS_FMT_SGRBG8_1X8:
 	case MEDIA_BUS_FMT_SGBRG8_1X8:
@@ -931,34 +865,13 @@ static int imx219_set_binning(struct imx219 *imx219)
 	return -EINVAL;
 }
 
-static const struct v4l2_rect *
-__imx219_get_pad_crop(struct imx219 *imx219,
-		      struct v4l2_subdev_state *sd_state,
-		      unsigned int pad, enum v4l2_subdev_format_whence which)
-{
-	switch (which) {
-	case V4L2_SUBDEV_FORMAT_TRY:
-		return v4l2_subdev_get_try_crop(&imx219->sd, sd_state, pad);
-	case V4L2_SUBDEV_FORMAT_ACTIVE:
-		return &imx219->mode->crop;
-	}
-
-	return NULL;
-}
-
 static int imx219_get_selection(struct v4l2_subdev *sd,
 				struct v4l2_subdev_state *sd_state,
 				struct v4l2_subdev_selection *sel)
 {
 	switch (sel->target) {
 	case V4L2_SEL_TGT_CROP: {
-		struct imx219 *imx219 = to_imx219(sd);
-
-		mutex_lock(&imx219->mutex);
-		sel->r = *__imx219_get_pad_crop(imx219, sd_state, sel->pad,
-						sel->which);
-		mutex_unlock(&imx219->mutex);
-
+		sel->r = *v4l2_subdev_get_pad_crop(sd, sd_state, 0);
 		return 0;
 	}
 
@@ -990,9 +903,11 @@ static int imx219_configure_lanes(struct imx219 *imx219)
 				IMX219_CSI_2_LANE_MODE : IMX219_CSI_4_LANE_MODE);
 };
 
-static int imx219_start_streaming(struct imx219 *imx219)
+static int imx219_start_streaming(struct imx219 *imx219,
+				  struct v4l2_subdev_state *state)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
+	const struct v4l2_mbus_framefmt *format;
 	const struct imx219_reg_list *reg_list;
 	int ret;
 
@@ -1022,14 +937,15 @@ static int imx219_start_streaming(struct imx219 *imx219)
 		goto err_rpm_put;
 	}
 
-	ret = imx219_set_framefmt(imx219);
+	format = v4l2_subdev_get_pad_format(&imx219->sd, state, 0);
+	ret = imx219_set_framefmt(imx219, format);
 	if (ret) {
 		dev_err(&client->dev, "%s failed to set frame format: %d\n",
 			__func__, ret);
 		goto err_rpm_put;
 	}
 
-	ret = imx219_set_binning(imx219);
+	ret = imx219_set_binning(imx219, format);
 	if (ret) {
 		dev_err(&client->dev, "%s failed to set binning: %d\n",
 			__func__, ret);
@@ -1078,35 +994,30 @@ static void imx219_stop_streaming(struct imx219 *imx219)
 static int imx219_set_stream(struct v4l2_subdev *sd, int enable)
 {
 	struct imx219 *imx219 = to_imx219(sd);
+	struct v4l2_subdev_state *state;
 	int ret = 0;
 
-	mutex_lock(&imx219->mutex);
-	if (imx219->streaming == enable) {
-		mutex_unlock(&imx219->mutex);
-		return 0;
-	}
+	state = v4l2_subdev_lock_and_get_active_state(sd);
+
+	if (imx219->streaming == enable)
+		goto unlock;
 
 	if (enable) {
 		/*
 		 * Apply default & customized values
 		 * and then start streaming.
 		 */
-		ret = imx219_start_streaming(imx219);
+		ret = imx219_start_streaming(imx219, state);
 		if (ret)
-			goto err_unlock;
+			goto unlock;
 	} else {
 		imx219_stop_streaming(imx219);
 	}
 
 	imx219->streaming = enable;
 
-	mutex_unlock(&imx219->mutex);
-
-	return ret;
-
-err_unlock:
-	mutex_unlock(&imx219->mutex);
-
+unlock:
+	v4l2_subdev_unlock_state(state);
 	return ret;
 }
 
@@ -1171,10 +1082,13 @@ static int __maybe_unused imx219_resume(struct device *dev)
 {
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct imx219 *imx219 = to_imx219(sd);
+	struct v4l2_subdev_state *state;
 	int ret;
 
 	if (imx219->streaming) {
-		ret = imx219_start_streaming(imx219);
+		state = v4l2_subdev_lock_and_get_active_state(sd);
+		ret = imx219_start_streaming(imx219, state);
+		v4l2_subdev_unlock_state(state);
 		if (ret)
 			goto error;
 	}
@@ -1237,7 +1151,7 @@ static const struct v4l2_subdev_video_ops imx219_video_ops = {
 static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
 	.init_cfg = imx219_init_cfg,
 	.enum_mbus_code = imx219_enum_mbus_code,
-	.get_fmt = imx219_get_pad_format,
+	.get_fmt = v4l2_subdev_get_fmt,
 	.set_fmt = imx219_set_pad_format,
 	.get_selection = imx219_get_selection,
 	.enum_frame_size = imx219_enum_frame_size,
@@ -1270,9 +1184,6 @@ static int imx219_init_controls(struct imx219 *imx219)
 	if (ret)
 		return ret;
 
-	mutex_init(&imx219->mutex);
-	ctrl_hdlr->lock = &imx219->mutex;
-
 	/* By default, PIXEL_RATE is read only */
 	imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
 					       V4L2_CID_PIXEL_RATE,
@@ -1369,7 +1280,6 @@ static int imx219_init_controls(struct imx219 *imx219)
 
 error:
 	v4l2_ctrl_handler_free(ctrl_hdlr);
-	mutex_destroy(&imx219->mutex);
 
 	return ret;
 }
@@ -1377,7 +1287,6 @@ static int imx219_init_controls(struct imx219 *imx219)
 static void imx219_free_controls(struct imx219 *imx219)
 {
 	v4l2_ctrl_handler_free(imx219->sd.ctrl_handler);
-	mutex_destroy(&imx219->mutex);
 }
 
 static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
@@ -1514,19 +1423,23 @@ static int imx219_probe(struct i2c_client *client)
 	/* Initialize source pad */
 	imx219->pad.flags = MEDIA_PAD_FL_SOURCE;
 
-	/* Initialize default format */
-	imx219_set_default_format(imx219);
-
 	ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
 	if (ret) {
 		dev_err(dev, "failed to init entity pads: %d\n", ret);
 		goto error_handler_free;
 	}
 
+	imx219->sd.state_lock = imx219->ctrl_handler.lock;
+	ret = v4l2_subdev_init_finalize(&imx219->sd);
+	if (ret < 0) {
+		dev_err(dev, "subdev init error: %d\n", ret);
+		goto error_media_entity;
+	}
+
 	ret = v4l2_async_register_subdev_sensor(&imx219->sd);
 	if (ret < 0) {
 		dev_err(dev, "failed to register sensor sub-device: %d\n", ret);
-		goto error_media_entity;
+		goto error_subdev_cleanup;
 	}
 
 	/* Enable runtime PM and turn off the device */
@@ -1536,6 +1449,9 @@ static int imx219_probe(struct i2c_client *client)
 
 	return 0;
 
+error_subdev_cleanup:
+	v4l2_subdev_cleanup(&imx219->sd);
+
 error_media_entity:
 	media_entity_cleanup(&imx219->sd.entity);
 
@@ -1554,6 +1470,7 @@ static void imx219_remove(struct i2c_client *client)
 	struct imx219 *imx219 = to_imx219(sd);
 
 	v4l2_async_unregister_subdev(sd);
+	v4l2_subdev_cleanup(sd);
 	media_entity_cleanup(&sd->entity);
 	imx219_free_controls(imx219);
 
-- 
2.40.1

[PATCH v2 6/7] media: i2c: imx219: Simplify format assignment

[Jacopo Mondi]

The set_fmt and init_cfg functions both fills a v4l2_mbus_framefmt
instance, passing in the mode and the media bus code. While set_fmt
uses function helpers, init_cfg open-codes the assignments.

Simplify the format initialization by moving it to a common helper.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/imx219.c | 48 ++++++++++++++------------------------
 1 file changed, 18 insertions(+), 30 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 73e06583d651..4f214f10846c 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -670,6 +670,20 @@ static const struct v4l2_ctrl_ops imx219_ctrl_ops = {
 	.s_ctrl = imx219_set_ctrl,
 };
 
+static void imx219_update_pad_format(struct imx219 *imx219,
+				     const struct imx219_mode *mode,
+				     struct v4l2_mbus_framefmt *fmt, u32 code)
+{
+	/* Bayer order varies with flips */
+	fmt->code = imx219_get_format_code(imx219, code);
+	fmt->width = mode->width;
+	fmt->height = mode->height;
+	fmt->field = V4L2_FIELD_NONE;
+	fmt->colorspace = V4L2_COLORSPACE_RAW;
+	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
+	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
+}
+
 static int imx219_init_cfg(struct v4l2_subdev *sd,
 			   struct v4l2_subdev_state *state)
 {
@@ -679,15 +693,8 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
 
 	/* Initialize try_fmt */
 	format = v4l2_subdev_get_pad_format(sd, state, 0);
-	format->width = supported_modes[0].width;
-	format->height = supported_modes[0].height;
-	format->code = imx219_get_format_code(imx219,
-					      MEDIA_BUS_FMT_SRGGB10_1X10);
-	format->field = V4L2_FIELD_NONE;
-	format->colorspace = V4L2_COLORSPACE_RAW;
-	format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);
-	format->quantization = V4L2_QUANTIZATION_FULL_RANGE;
-	format->xfer_func = V4L2_XFER_FUNC_NONE;
+	imx219_update_pad_format(imx219, &supported_modes[0], format,
+				 MEDIA_BUS_FMT_SRGGB10_1X10);
 
 	/* Initialize crop rectangle. */
 	crop = v4l2_subdev_get_pad_crop(sd, state, 0);
@@ -735,23 +742,6 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static void imx219_reset_colorspace(struct v4l2_mbus_framefmt *fmt)
-{
-	fmt->colorspace = V4L2_COLORSPACE_RAW;
-	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
-	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
-}
-
-static void imx219_update_pad_format(struct imx219 *imx219,
-				     const struct imx219_mode *mode,
-				     struct v4l2_subdev_format *fmt)
-{
-	fmt->format.width = mode->width;
-	fmt->format.height = mode->height;
-	fmt->format.field = V4L2_FIELD_NONE;
-	imx219_reset_colorspace(&fmt->format);
-}
-
 static int imx219_set_pad_format(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *sd_state,
 				 struct v4l2_subdev_format *fmt)
@@ -768,15 +758,13 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 	if (i >= ARRAY_SIZE(imx219_mbus_formats))
 		i = 0;
 
-	/* Bayer order varies with flips */
-	fmt->format.code = imx219_get_format_code(imx219, imx219_mbus_formats[i]);
-
 	mode = v4l2_find_nearest_size(supported_modes,
 				      ARRAY_SIZE(supported_modes),
 				      width, height,
 				      fmt->format.width, fmt->format.height);
 
-	imx219_update_pad_format(imx219, mode, fmt);
+	imx219_update_pad_format(imx219, mode, &fmt->format,
+				 imx219_mbus_formats[i]);
 	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
 
 	if (imx219->mode == mode && format->code == fmt->format.code)
-- 
2.40.1

[PATCH v2 7/7] media: i2c: imx219: Simplify code handling in s_fmt

[Jacopo Mondi]

The imx219_set_pad_format() function adjusts the media bus code provided
through the v4l2_subdev_format parameter to a media bus code known
to be supported by the sensor.

The same exact operation is performed by the imx219_get_format_code()
function which called by imx219_update_pad_format(), which is in the
imx219_set_pad_format() call path.

Remove the duplicated operation and simplify imx219_set_pad_format().

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/imx219.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 4f214f10846c..a1136fdfbed2 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -750,21 +750,13 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 	const struct imx219_mode *mode;
 	int exposure_max, exposure_def, hblank;
 	struct v4l2_mbus_framefmt *format;
-	unsigned int i;
-
-	for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
-		if (imx219_mbus_formats[i] == fmt->format.code)
-			break;
-	if (i >= ARRAY_SIZE(imx219_mbus_formats))
-		i = 0;
 
 	mode = v4l2_find_nearest_size(supported_modes,
 				      ARRAY_SIZE(supported_modes),
 				      width, height,
 				      fmt->format.width, fmt->format.height);
 
-	imx219_update_pad_format(imx219, mode, &fmt->format,
-				 imx219_mbus_formats[i]);
+	imx219_update_pad_format(imx219, mode, &fmt->format, fmt->format.code);
 	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
 
 	if (imx219->mode == mode && format->code == fmt->format.code)
-- 
2.40.1

Re: [PATCH v2 1/7] media: i2c: imx219: Rename mbus codes array

[Umang Jain]

Hi Jacopo,

On 7/10/23 9:21 PM, Jacopo Mondi wrote:
> From: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>
> The imx219 is using the name "codes" for the mbus formats array. The
> name is too generic and not easy to read and follow in the code. Change
> it to imx219_mbus_formats.
>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Reviewed-by: Tommaso Merciai <tomm.merciai@gmail.com>

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

> ---
>   drivers/media/i2c/imx219.c | 22 +++++++++++-----------
>   1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index d737d5e9a4a6..ac6b0e7a838d 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -345,7 +345,7 @@ static const char * const imx219_supply_name[] = {
>    * - v flip
>    * - h&v flips
>    */
> -static const u32 codes[] = {
> +static const u32 imx219_mbus_formats[] = {
>   	MEDIA_BUS_FMT_SRGGB10_1X10,
>   	MEDIA_BUS_FMT_SGRBG10_1X10,
>   	MEDIA_BUS_FMT_SGBRG10_1X10,
> @@ -578,17 +578,17 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
>   
>   	lockdep_assert_held(&imx219->mutex);
>   
> -	for (i = 0; i < ARRAY_SIZE(codes); i++)
> -		if (codes[i] == code)
> +	for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
> +		if (imx219_mbus_formats[i] == code)
>   			break;
>   
> -	if (i >= ARRAY_SIZE(codes))
> +	if (i >= ARRAY_SIZE(imx219_mbus_formats))
>   		i = 0;
>   
>   	i = (i & ~3) | (imx219->vflip->val ? 2 : 0) |
>   	    (imx219->hflip->val ? 1 : 0);
>   
> -	return codes[i];
> +	return imx219_mbus_formats[i];
>   }
>   
>   static void imx219_set_default_format(struct imx219 *imx219)
> @@ -731,11 +731,11 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
>   {
>   	struct imx219 *imx219 = to_imx219(sd);
>   
> -	if (code->index >= (ARRAY_SIZE(codes) / 4))
> +	if (code->index >= (ARRAY_SIZE(imx219_mbus_formats) / 4))
>   		return -EINVAL;
>   
>   	mutex_lock(&imx219->mutex);
> -	code->code = imx219_get_format_code(imx219, codes[code->index * 4]);
> +	code->code = imx219_get_format_code(imx219, imx219_mbus_formats[code->index * 4]);
>   	mutex_unlock(&imx219->mutex);
>   
>   	return 0;
> @@ -831,14 +831,14 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>   
>   	mutex_lock(&imx219->mutex);
>   
> -	for (i = 0; i < ARRAY_SIZE(codes); i++)
> -		if (codes[i] == fmt->format.code)
> +	for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
> +		if (imx219_mbus_formats[i] == fmt->format.code)
>   			break;
> -	if (i >= ARRAY_SIZE(codes))
> +	if (i >= ARRAY_SIZE(imx219_mbus_formats))
>   		i = 0;
>   
>   	/* Bayer order varies with flips */
> -	fmt->format.code = imx219_get_format_code(imx219, codes[i]);
> +	fmt->format.code = imx219_get_format_code(imx219, imx219_mbus_formats[i]);
>   
>   	mode = v4l2_find_nearest_size(supported_modes,
>   				      ARRAY_SIZE(supported_modes),

Re: [PATCH v2 2/7] media: i2c: imx219: Switch from open to init_cfg

[Umang Jain]

Hi Jacopo,

On 7/10/23 9:21 PM, Jacopo Mondi wrote:
> From: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
>
> Use the init_cfg pad level operation instead of the internal subdev
> open operation to set default formats on the pads.
>
> Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   drivers/media/i2c/imx219.c | 63 +++++++++++++++++++-------------------
>   1 file changed, 31 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index ac6b0e7a838d..45b219321d98 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -608,34 +608,6 @@ static void imx219_set_default_format(struct imx219 *imx219)
>   	fmt->field = V4L2_FIELD_NONE;
>   }
>   
> -static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> -{
> -	struct imx219 *imx219 = to_imx219(sd);
> -	struct v4l2_mbus_framefmt *try_fmt =
> -		v4l2_subdev_get_try_format(sd, fh->state, 0);
> -	struct v4l2_rect *try_crop;
> -
> -	mutex_lock(&imx219->mutex);
> -
> -	/* Initialize try_fmt */
> -	try_fmt->width = supported_modes[0].width;
> -	try_fmt->height = supported_modes[0].height;
> -	try_fmt->code = imx219_get_format_code(imx219,
> -					       MEDIA_BUS_FMT_SRGGB10_1X10);
> -	try_fmt->field = V4L2_FIELD_NONE;
> -
> -	/* Initialize try_crop rectangle. */
> -	try_crop = v4l2_subdev_get_try_crop(sd, fh->state, 0);
> -	try_crop->top = IMX219_PIXEL_ARRAY_TOP;
> -	try_crop->left = IMX219_PIXEL_ARRAY_LEFT;
> -	try_crop->width = IMX219_PIXEL_ARRAY_WIDTH;
> -	try_crop->height = IMX219_PIXEL_ARRAY_HEIGHT;
> -
> -	mutex_unlock(&imx219->mutex);
> -
> -	return 0;
> -}
> -
>   static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
>   {
>   	struct imx219 *imx219 =
> @@ -725,6 +697,36 @@ static const struct v4l2_ctrl_ops imx219_ctrl_ops = {
>   	.s_ctrl = imx219_set_ctrl,
>   };
>   
> +static int imx219_init_cfg(struct v4l2_subdev *sd,
> +			   struct v4l2_subdev_state *state)
> +{
> +	struct imx219 *imx219 = to_imx219(sd);
> +	struct v4l2_mbus_framefmt *format;
> +	struct v4l2_rect *crop;
> +
> +	/* imx219_get_format_code() wants mutex locked. */
> +	mutex_lock(&imx219->mutex);
> +
> +	/* Initialize try_fmt */
> +	format = v4l2_subdev_get_pad_format(sd, state, 0);
> +	format->width = supported_modes[0].width;
> +	format->height = supported_modes[0].height;
> +	format->code = imx219_get_format_code(imx219,
> +					      MEDIA_BUS_FMT_SRGGB10_1X10);
> +	format->field = V4L2_FIELD_NONE;
> +
> +	/* Initialize crop rectangle. */
> +	crop = v4l2_subdev_get_pad_crop(sd, state, 0);
> +	crop->top = IMX219_PIXEL_ARRAY_TOP;
> +	crop->left = IMX219_PIXEL_ARRAY_LEFT;
> +	crop->width = IMX219_PIXEL_ARRAY_WIDTH;
> +	crop->height = IMX219_PIXEL_ARRAY_HEIGHT;
> +
> +	mutex_unlock(&imx219->mutex);
> +
> +	return 0;
> +}
> +
>   static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
>   				 struct v4l2_subdev_state *sd_state,
>   				 struct v4l2_subdev_mbus_code_enum *code)
> @@ -1235,6 +1237,7 @@ static const struct v4l2_subdev_video_ops imx219_video_ops = {
>   };
>   
>   static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
> +	.init_cfg = imx219_init_cfg,
>   	.enum_mbus_code = imx219_enum_mbus_code,
>   	.get_fmt = imx219_get_pad_format,
>   	.set_fmt = imx219_set_pad_format,
> @@ -1248,9 +1251,6 @@ static const struct v4l2_subdev_ops imx219_subdev_ops = {
>   	.pad = &imx219_pad_ops,
>   };
>   
> -static const struct v4l2_subdev_internal_ops imx219_internal_ops = {
> -	.open = imx219_open,
> -};
>   

nit: maybe this extra line needs deletion?

with that,

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

>   static unsigned long imx219_get_pixel_rate(struct imx219 *imx219)
>   {
> @@ -1509,7 +1509,6 @@ static int imx219_probe(struct i2c_client *client)
>   		goto error_power_off;
>   
>   	/* Initialize subdev */
> -	imx219->sd.internal_ops = &imx219_internal_ops;
>   	imx219->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
>   			    V4L2_SUBDEV_FL_HAS_EVENTS;
>   	imx219->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;

Re: [PATCH v2 3/7] media: i2c: imx219: Complete default format initialization

[Umang Jain]

Hi Jacopo,

On 7/10/23 9:21 PM, Jacopo Mondi wrote:
> Complete the default format initialization in init_cfg() filling in
> the fields for the colorspace configuration copied from
> imx219_set_default_format().
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

> ---
>   drivers/media/i2c/imx219.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 45b219321d98..cd43a897391c 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -714,6 +714,12 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
>   	format->code = imx219_get_format_code(imx219,
>   					      MEDIA_BUS_FMT_SRGGB10_1X10);
>   	format->field = V4L2_FIELD_NONE;
> +	format->colorspace = V4L2_COLORSPACE_SRGB;
> +	format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);
> +	format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> +							     format->colorspace,
> +							     format->ycbcr_enc);
> +	format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(format->colorspace);
>   
>   	/* Initialize crop rectangle. */
>   	crop = v4l2_subdev_get_pad_crop(sd, state, 0);

Re: [PATCH v2 4/7] media: i2c: imx219: Fix colorspace info

[Umang Jain]

Hi Jaocpo,

On 7/10/23 9:22 PM, Jacopo Mondi wrote:
> The IMX219 is a RAW sensor. Fix the colorspace configuration by
> using V4L2_COLORSPACE_RAW and adjust the quantization and transfer
> function values. Drop ycbcr_enc as it doesn't apply to RAW sensors.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>   drivers/media/i2c/imx219.c | 26 +++++++++-----------------
>   1 file changed, 9 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index cd43a897391c..6963e24e1bc2 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -597,15 +597,12 @@ static void imx219_set_default_format(struct imx219 *imx219)
>   
>   	fmt = &imx219->fmt;
>   	fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
> -	fmt->colorspace = V4L2_COLORSPACE_SRGB;
> -	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> -	fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> -							  fmt->colorspace,
> -							  fmt->ycbcr_enc);
> -	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> +	fmt->colorspace = V4L2_COLORSPACE_RAW;
> +	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>   	fmt->width = supported_modes[0].width;
>   	fmt->height = supported_modes[0].height;
>   	fmt->field = V4L2_FIELD_NONE;
> +	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
>   }
>   
>   static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> @@ -714,12 +711,10 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
>   	format->code = imx219_get_format_code(imx219,
>   					      MEDIA_BUS_FMT_SRGGB10_1X10);
>   	format->field = V4L2_FIELD_NONE;
> -	format->colorspace = V4L2_COLORSPACE_SRGB;
> +	format->colorspace = V4L2_COLORSPACE_RAW;
>   	format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);

forgot to remove ycbcr_enc from here ?

> -	format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> -							     format->colorspace,
> -							     format->ycbcr_enc);
> -	format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(format->colorspace);
> +	format->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +	format->xfer_func = V4L2_XFER_FUNC_NONE;
>   
>   	/* Initialize crop rectangle. */
>   	crop = v4l2_subdev_get_pad_crop(sd, state, 0);
> @@ -775,12 +770,9 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
>   
>   static void imx219_reset_colorspace(struct v4l2_mbus_framefmt *fmt)
>   {
> -	fmt->colorspace = V4L2_COLORSPACE_SRGB;
> -	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> -	fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> -							  fmt->colorspace,
> -							  fmt->ycbcr_enc);
> -	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> +	fmt->colorspace = V4L2_COLORSPACE_RAW;
> +	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
>   }
>   
>   static void imx219_update_pad_format(struct imx219 *imx219,

Re: [PATCH v2 6/7] media: i2c: imx219: Simplify format assignment

[Umang Jain]

Hi Jacopo,

On 7/10/23 9:22 PM, Jacopo Mondi wrote:
> The set_fmt and init_cfg functions both fills a v4l2_mbus_framefmt
> instance, passing in the mode and the media bus code. While set_fmt
> uses function helpers, init_cfg open-codes the assignments.
>
> Simplify the format initialization by moving it to a common helper.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

> ---
>   drivers/media/i2c/imx219.c | 48 ++++++++++++++------------------------
>   1 file changed, 18 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 73e06583d651..4f214f10846c 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -670,6 +670,20 @@ static const struct v4l2_ctrl_ops imx219_ctrl_ops = {
>   	.s_ctrl = imx219_set_ctrl,
>   };
>   
> +static void imx219_update_pad_format(struct imx219 *imx219,
> +				     const struct imx219_mode *mode,
> +				     struct v4l2_mbus_framefmt *fmt, u32 code)
> +{
> +	/* Bayer order varies with flips */
> +	fmt->code = imx219_get_format_code(imx219, code);
> +	fmt->width = mode->width;
> +	fmt->height = mode->height;
> +	fmt->field = V4L2_FIELD_NONE;
> +	fmt->colorspace = V4L2_COLORSPACE_RAW;
> +	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
> +}
> +
>   static int imx219_init_cfg(struct v4l2_subdev *sd,
>   			   struct v4l2_subdev_state *state)
>   {
> @@ -679,15 +693,8 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
>   
>   	/* Initialize try_fmt */
>   	format = v4l2_subdev_get_pad_format(sd, state, 0);
> -	format->width = supported_modes[0].width;
> -	format->height = supported_modes[0].height;
> -	format->code = imx219_get_format_code(imx219,
> -					      MEDIA_BUS_FMT_SRGGB10_1X10);
> -	format->field = V4L2_FIELD_NONE;
> -	format->colorspace = V4L2_COLORSPACE_RAW;
> -	format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);
> -	format->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> -	format->xfer_func = V4L2_XFER_FUNC_NONE;
> +	imx219_update_pad_format(imx219, &supported_modes[0], format,
> +				 MEDIA_BUS_FMT_SRGGB10_1X10);
>   
>   	/* Initialize crop rectangle. */
>   	crop = v4l2_subdev_get_pad_crop(sd, state, 0);
> @@ -735,23 +742,6 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
>   	return 0;
>   }
>   
> -static void imx219_reset_colorspace(struct v4l2_mbus_framefmt *fmt)
> -{
> -	fmt->colorspace = V4L2_COLORSPACE_RAW;
> -	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> -	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
> -}
> -
> -static void imx219_update_pad_format(struct imx219 *imx219,
> -				     const struct imx219_mode *mode,
> -				     struct v4l2_subdev_format *fmt)
> -{
> -	fmt->format.width = mode->width;
> -	fmt->format.height = mode->height;
> -	fmt->format.field = V4L2_FIELD_NONE;
> -	imx219_reset_colorspace(&fmt->format);
> -}
> -
>   static int imx219_set_pad_format(struct v4l2_subdev *sd,
>   				 struct v4l2_subdev_state *sd_state,
>   				 struct v4l2_subdev_format *fmt)
> @@ -768,15 +758,13 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>   	if (i >= ARRAY_SIZE(imx219_mbus_formats))
>   		i = 0;
>   
> -	/* Bayer order varies with flips */
> -	fmt->format.code = imx219_get_format_code(imx219, imx219_mbus_formats[i]);
> -
>   	mode = v4l2_find_nearest_size(supported_modes,
>   				      ARRAY_SIZE(supported_modes),
>   				      width, height,
>   				      fmt->format.width, fmt->format.height);
>   
> -	imx219_update_pad_format(imx219, mode, fmt);
> +	imx219_update_pad_format(imx219, mode, &fmt->format,
> +				 imx219_mbus_formats[i]);
>   	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
>   
>   	if (imx219->mode == mode && format->code == fmt->format.code)

Re: [PATCH v2 3/7] media: i2c: imx219: Complete default format initialization

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Mon, Jul 10, 2023 at 05:51:59PM +0200, Jacopo Mondi wrote:
> Complete the default format initialization in init_cfg() filling in
> the fields for the colorspace configuration copied from
> imx219_set_default_format().
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  drivers/media/i2c/imx219.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 45b219321d98..cd43a897391c 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -714,6 +714,12 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
>  	format->code = imx219_get_format_code(imx219,
>  					      MEDIA_BUS_FMT_SRGGB10_1X10);
>  	format->field = V4L2_FIELD_NONE;
> +	format->colorspace = V4L2_COLORSPACE_SRGB;
> +	format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);
> +	format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> +							     format->colorspace,
> +							     format->ycbcr_enc);
> +	format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(format->colorspace);

There are multiple ways to initialize color space, and for raw sensors I
think I'd have a preference for being explicit:

        format->colorspace = V4L2_COLORSPACE_SRGB;
        format->ycbcr_enc = V4L2_YCBCR_ENC_601;
        format->quantization = V4L2_QUANTIZATION_FULL_RANGE;
        format->xfer_func = V4L2_XFER_FUNC_NONE;

Furthermore, I think V4L2_COLORSPACE_RAW would be better.

Granted, this doesn't match imx219_set_default_format(), but you remove
that function later in the series.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	/* Initialize crop rectangle. */
>  	crop = v4l2_subdev_get_pad_crop(sd, state, 0);

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 4/7] media: i2c: imx219: Fix colorspace info

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.


On Mon, Jul 10, 2023 at 05:52:00PM +0200, Jacopo Mondi wrote:
> The IMX219 is a RAW sensor. Fix the colorspace configuration by
> using V4L2_COLORSPACE_RAW and adjust the quantization and transfer
> function values. Drop ycbcr_enc as it doesn't apply to RAW sensors.

So this addresses part of my comments for 3/7, nice :-)

> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  drivers/media/i2c/imx219.c | 26 +++++++++-----------------
>  1 file changed, 9 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index cd43a897391c..6963e24e1bc2 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -597,15 +597,12 @@ static void imx219_set_default_format(struct imx219 *imx219)
>  
>  	fmt = &imx219->fmt;
>  	fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
> -	fmt->colorspace = V4L2_COLORSPACE_SRGB;
> -	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> -	fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> -							  fmt->colorspace,
> -							  fmt->ycbcr_enc);
> -	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> +	fmt->colorspace = V4L2_COLORSPACE_RAW;
> +	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>  	fmt->width = supported_modes[0].width;
>  	fmt->height = supported_modes[0].height;
>  	fmt->field = V4L2_FIELD_NONE;
> +	fmt->xfer_func = V4L2_XFER_FUNC_NONE;

Any reason to not group xfer_func with colorspace and quantization ?

>  }
>  
>  static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> @@ -714,12 +711,10 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
>  	format->code = imx219_get_format_code(imx219,
>  					      MEDIA_BUS_FMT_SRGGB10_1X10);
>  	format->field = V4L2_FIELD_NONE;
> -	format->colorspace = V4L2_COLORSPACE_SRGB;
> +	format->colorspace = V4L2_COLORSPACE_RAW;
>  	format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);

You're keeping ycbcr_enc here while you've dropped it in the two other
locations. Was that on purpose ?

While the encoding doesn't apply to raw formats, I think it should still
be set explicitly, or it will end up having a random value.

> -	format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> -							     format->colorspace,
> -							     format->ycbcr_enc);
> -	format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(format->colorspace);
> +	format->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +	format->xfer_func = V4L2_XFER_FUNC_NONE;
>  
>  	/* Initialize crop rectangle. */
>  	crop = v4l2_subdev_get_pad_crop(sd, state, 0);
> @@ -775,12 +770,9 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
>  
>  static void imx219_reset_colorspace(struct v4l2_mbus_framefmt *fmt)
>  {
> -	fmt->colorspace = V4L2_COLORSPACE_SRGB;
> -	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> -	fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> -							  fmt->colorspace,
> -							  fmt->ycbcr_enc);
> -	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> +	fmt->colorspace = V4L2_COLORSPACE_RAW;
> +	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
>  }
>  
>  static void imx219_update_pad_format(struct imx219 *imx219,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 5/7] media: i2c: imx219: Use subdev active state

[Umang Jain]

Hi Jacopo ,

On 7/10/23 9:22 PM, Jacopo Mondi wrote:
> Port the imx219 sensor driver to use the subdev active state.
>
> Move all the format configuration to the subdevice state and simplify
> the format handling, locking and initialization.
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   drivers/media/i2c/imx219.c | 179 ++++++++++---------------------------
>   1 file changed, 48 insertions(+), 131 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 6963e24e1bc2..73e06583d651 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -460,8 +460,6 @@ struct imx219 {
>   	struct v4l2_subdev sd;
>   	struct media_pad pad;
>   
> -	struct v4l2_mbus_framefmt fmt;
> -
>   	struct clk *xclk; /* system clock to IMX219 */
>   	u32 xclk_freq;
>   
> @@ -481,12 +479,6 @@ struct imx219 {
>   	/* Current mode */
>   	const struct imx219_mode *mode;
>   
> -	/*
> -	 * Mutex for serialized access:
> -	 * Protect sensor module set pad format and start/stop streaming safely.
> -	 */
> -	struct mutex mutex;
> -
>   	/* Streaming on/off */
>   	bool streaming;
>   
> @@ -576,8 +568,6 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
>   {
>   	unsigned int i;
>   
> -	lockdep_assert_held(&imx219->mutex);
> -
>   	for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
>   		if (imx219_mbus_formats[i] == code)
>   			break;
> @@ -591,20 +581,6 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
>   	return imx219_mbus_formats[i];
>   }
>   
> -static void imx219_set_default_format(struct imx219 *imx219)
> -{
> -	struct v4l2_mbus_framefmt *fmt;
> -
> -	fmt = &imx219->fmt;
> -	fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
> -	fmt->colorspace = V4L2_COLORSPACE_RAW;
> -	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> -	fmt->width = supported_modes[0].width;
> -	fmt->height = supported_modes[0].height;
> -	fmt->field = V4L2_FIELD_NONE;
> -	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
> -}
> -
>   static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
>   {
>   	struct imx219 *imx219 =
> @@ -701,9 +677,6 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
>   	struct v4l2_mbus_framefmt *format;
>   	struct v4l2_rect *crop;
>   
> -	/* imx219_get_format_code() wants mutex locked. */
> -	mutex_lock(&imx219->mutex);
> -
>   	/* Initialize try_fmt */
>   	format = v4l2_subdev_get_pad_format(sd, state, 0);
>   	format->width = supported_modes[0].width;
> @@ -723,8 +696,6 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
>   	crop->width = IMX219_PIXEL_ARRAY_WIDTH;
>   	crop->height = IMX219_PIXEL_ARRAY_HEIGHT;
>   
> -	mutex_unlock(&imx219->mutex);
> -
>   	return 0;
>   }
>   
> @@ -737,9 +708,7 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
>   	if (code->index >= (ARRAY_SIZE(imx219_mbus_formats) / 4))
>   		return -EINVAL;
>   
> -	mutex_lock(&imx219->mutex);
>   	code->code = imx219_get_format_code(imx219, imx219_mbus_formats[code->index * 4]);
> -	mutex_unlock(&imx219->mutex);
>   
>   	return 0;
>   }
> @@ -754,9 +723,7 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
>   	if (fse->index >= ARRAY_SIZE(supported_modes))
>   		return -EINVAL;
>   
> -	mutex_lock(&imx219->mutex);
>   	code = imx219_get_format_code(imx219, fse->code);
> -	mutex_unlock(&imx219->mutex);
>   	if (fse->code != code)
>   		return -EINVAL;
>   
> @@ -785,52 +752,16 @@ static void imx219_update_pad_format(struct imx219 *imx219,
>   	imx219_reset_colorspace(&fmt->format);
>   }
>   
> -static int __imx219_get_pad_format(struct imx219 *imx219,
> -				   struct v4l2_subdev_state *sd_state,
> -				   struct v4l2_subdev_format *fmt)
> -{
> -	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		struct v4l2_mbus_framefmt *try_fmt =
> -			v4l2_subdev_get_try_format(&imx219->sd, sd_state,
> -						   fmt->pad);
> -		/* update the code which could change due to vflip or hflip: */
> -		try_fmt->code = imx219_get_format_code(imx219, try_fmt->code);
> -		fmt->format = *try_fmt;
> -	} else {
> -		imx219_update_pad_format(imx219, imx219->mode, fmt);
> -		fmt->format.code = imx219_get_format_code(imx219,
> -							  imx219->fmt.code);
> -	}
> -
> -	return 0;
> -}
> -
> -static int imx219_get_pad_format(struct v4l2_subdev *sd,
> -				 struct v4l2_subdev_state *sd_state,
> -				 struct v4l2_subdev_format *fmt)
> -{
> -	struct imx219 *imx219 = to_imx219(sd);
> -	int ret;
> -
> -	mutex_lock(&imx219->mutex);
> -	ret = __imx219_get_pad_format(imx219, sd_state, fmt);
> -	mutex_unlock(&imx219->mutex);
> -
> -	return ret;
> -}
> -
>   static int imx219_set_pad_format(struct v4l2_subdev *sd,
>   				 struct v4l2_subdev_state *sd_state,
>   				 struct v4l2_subdev_format *fmt)
>   {
>   	struct imx219 *imx219 = to_imx219(sd);
>   	const struct imx219_mode *mode;
> -	struct v4l2_mbus_framefmt *framefmt;
>   	int exposure_max, exposure_def, hblank;
> +	struct v4l2_mbus_framefmt *format;
>   	unsigned int i;
>   
> -	mutex_lock(&imx219->mutex);
> -
>   	for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
>   		if (imx219_mbus_formats[i] == fmt->format.code)
>   			break;
> @@ -844,13 +775,14 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>   				      ARRAY_SIZE(supported_modes),
>   				      width, height,
>   				      fmt->format.width, fmt->format.height);
> +
>   	imx219_update_pad_format(imx219, mode, fmt);
> -	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> -		framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
> -		*framefmt = fmt->format;
> -	} else if (imx219->mode != mode ||
> -		   imx219->fmt.code != fmt->format.code) {
> -		imx219->fmt = fmt->format;
> +	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> +
> +	if (imx219->mode == mode && format->code == fmt->format.code)
> +		return 0;
> +
> +	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>   		imx219->mode = mode;
>   		/* Update limits and set FPS to default */
>   		__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> @@ -876,14 +808,15 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>   					 hblank);
>   	}
>   
> -	mutex_unlock(&imx219->mutex);
> +	*format = fmt->format;
>   
>   	return 0;
>   }
>   
> -static int imx219_set_framefmt(struct imx219 *imx219)
> +static int imx219_set_framefmt(struct imx219 *imx219,
> +			       const struct v4l2_mbus_framefmt *format)
>   {
> -	switch (imx219->fmt.code) {
> +	switch (format->code) {
>   	case MEDIA_BUS_FMT_SRGGB8_1X8:
>   	case MEDIA_BUS_FMT_SGRBG8_1X8:
>   	case MEDIA_BUS_FMT_SGBRG8_1X8:
> @@ -902,7 +835,8 @@ static int imx219_set_framefmt(struct imx219 *imx219)
>   	return -EINVAL;
>   }
>   
> -static int imx219_set_binning(struct imx219 *imx219)
> +static int imx219_set_binning(struct imx219 *imx219,
> +			      const struct v4l2_mbus_framefmt *format)
>   {
>   	if (!imx219->mode->binning) {
>   		return imx219_write_reg(imx219, IMX219_REG_BINNING_MODE,
> @@ -910,7 +844,7 @@ static int imx219_set_binning(struct imx219 *imx219)
>   					IMX219_BINNING_NONE);
>   	}
>   
> -	switch (imx219->fmt.code) {
> +	switch (format->code) {
>   	case MEDIA_BUS_FMT_SRGGB8_1X8:
>   	case MEDIA_BUS_FMT_SGRBG8_1X8:
>   	case MEDIA_BUS_FMT_SGBRG8_1X8:
> @@ -931,34 +865,13 @@ static int imx219_set_binning(struct imx219 *imx219)
>   	return -EINVAL;
>   }
>   
> -static const struct v4l2_rect *
> -__imx219_get_pad_crop(struct imx219 *imx219,
> -		      struct v4l2_subdev_state *sd_state,
> -		      unsigned int pad, enum v4l2_subdev_format_whence which)
> -{
> -	switch (which) {
> -	case V4L2_SUBDEV_FORMAT_TRY:
> -		return v4l2_subdev_get_try_crop(&imx219->sd, sd_state, pad);
> -	case V4L2_SUBDEV_FORMAT_ACTIVE:
> -		return &imx219->mode->crop;
> -	}
> -
> -	return NULL;
> -}
> -
>   static int imx219_get_selection(struct v4l2_subdev *sd,
>   				struct v4l2_subdev_state *sd_state,
>   				struct v4l2_subdev_selection *sel)
>   {
>   	switch (sel->target) {
>   	case V4L2_SEL_TGT_CROP: {
> -		struct imx219 *imx219 = to_imx219(sd);
> -
> -		mutex_lock(&imx219->mutex);
> -		sel->r = *__imx219_get_pad_crop(imx219, sd_state, sel->pad,
> -						sel->which);
> -		mutex_unlock(&imx219->mutex);
> -
> +		sel->r = *v4l2_subdev_get_pad_crop(sd, sd_state, 0);
>   		return 0;
>   	}
>   
> @@ -990,9 +903,11 @@ static int imx219_configure_lanes(struct imx219 *imx219)
>   				IMX219_CSI_2_LANE_MODE : IMX219_CSI_4_LANE_MODE);
>   };
>   
> -static int imx219_start_streaming(struct imx219 *imx219)
> +static int imx219_start_streaming(struct imx219 *imx219,
> +				  struct v4l2_subdev_state *state)
>   {
>   	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> +	const struct v4l2_mbus_framefmt *format;
>   	const struct imx219_reg_list *reg_list;
>   	int ret;
>   
> @@ -1022,14 +937,15 @@ static int imx219_start_streaming(struct imx219 *imx219)
>   		goto err_rpm_put;
>   	}
>   
> -	ret = imx219_set_framefmt(imx219);
> +	format = v4l2_subdev_get_pad_format(&imx219->sd, state, 0);
> +	ret = imx219_set_framefmt(imx219, format);
>   	if (ret) {
>   		dev_err(&client->dev, "%s failed to set frame format: %d\n",
>   			__func__, ret);
>   		goto err_rpm_put;
>   	}
>   
> -	ret = imx219_set_binning(imx219);
> +	ret = imx219_set_binning(imx219, format);
>   	if (ret) {
>   		dev_err(&client->dev, "%s failed to set binning: %d\n",
>   			__func__, ret);
> @@ -1078,35 +994,30 @@ static void imx219_stop_streaming(struct imx219 *imx219)
>   static int imx219_set_stream(struct v4l2_subdev *sd, int enable)
>   {
>   	struct imx219 *imx219 = to_imx219(sd);
> +	struct v4l2_subdev_state *state;
>   	int ret = 0;
>   
> -	mutex_lock(&imx219->mutex);
> -	if (imx219->streaming == enable) {
> -		mutex_unlock(&imx219->mutex);
> -		return 0;
> -	}
> +	state = v4l2_subdev_lock_and_get_active_state(sd);
> +
> +	if (imx219->streaming == enable)
> +		goto unlock;
>   
>   	if (enable) {
>   		/*
>   		 * Apply default & customized values
>   		 * and then start streaming.
>   		 */
> -		ret = imx219_start_streaming(imx219);
> +		ret = imx219_start_streaming(imx219, state);
>   		if (ret)
> -			goto err_unlock;
> +			goto unlock;
>   	} else {
>   		imx219_stop_streaming(imx219);
>   	}
>   
>   	imx219->streaming = enable;
>   
> -	mutex_unlock(&imx219->mutex);
> -
> -	return ret;
> -
> -err_unlock:
> -	mutex_unlock(&imx219->mutex);
> -
> +unlock:
> +	v4l2_subdev_unlock_state(state);
>   	return ret;
>   }
>   
> @@ -1171,10 +1082,13 @@ static int __maybe_unused imx219_resume(struct device *dev)
>   {
>   	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>   	struct imx219 *imx219 = to_imx219(sd);
> +	struct v4l2_subdev_state *state;
>   	int ret;
>   
>   	if (imx219->streaming) {
> -		ret = imx219_start_streaming(imx219);
> +		state = v4l2_subdev_lock_and_get_active_state(sd);
> +		ret = imx219_start_streaming(imx219, state);
> +		v4l2_subdev_unlock_state(state);
>   		if (ret)
>   			goto error;
>   	}
> @@ -1237,7 +1151,7 @@ static const struct v4l2_subdev_video_ops imx219_video_ops = {
>   static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
>   	.init_cfg = imx219_init_cfg,
>   	.enum_mbus_code = imx219_enum_mbus_code,
> -	.get_fmt = imx219_get_pad_format,
> +	.get_fmt = v4l2_subdev_get_fmt,
>   	.set_fmt = imx219_set_pad_format,
>   	.get_selection = imx219_get_selection,
>   	.enum_frame_size = imx219_enum_frame_size,
> @@ -1270,9 +1184,6 @@ static int imx219_init_controls(struct imx219 *imx219)
>   	if (ret)
>   		return ret;
>   
> -	mutex_init(&imx219->mutex);
> -	ctrl_hdlr->lock = &imx219->mutex;
> -
>   	/* By default, PIXEL_RATE is read only */
>   	imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
>   					       V4L2_CID_PIXEL_RATE,
> @@ -1369,7 +1280,6 @@ static int imx219_init_controls(struct imx219 *imx219)
>   
>   error:
>   	v4l2_ctrl_handler_free(ctrl_hdlr);
> -	mutex_destroy(&imx219->mutex);
>   
>   	return ret;
>   }
> @@ -1377,7 +1287,6 @@ static int imx219_init_controls(struct imx219 *imx219)
>   static void imx219_free_controls(struct imx219 *imx219)
>   {
>   	v4l2_ctrl_handler_free(imx219->sd.ctrl_handler);
> -	mutex_destroy(&imx219->mutex);
>   }
>   
>   static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
> @@ -1514,19 +1423,23 @@ static int imx219_probe(struct i2c_client *client)
>   	/* Initialize source pad */
>   	imx219->pad.flags = MEDIA_PAD_FL_SOURCE;
>   
> -	/* Initialize default format */
> -	imx219_set_default_format(imx219);
> -
>   	ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
>   	if (ret) {
>   		dev_err(dev, "failed to init entity pads: %d\n", ret);
>   		goto error_handler_free;
>   	}
>   
> +	imx219->sd.state_lock = imx219->ctrl_handler.lock;
> +	ret = v4l2_subdev_init_finalize(&imx219->sd);
> +	if (ret < 0) {
> +		dev_err(dev, "subdev init error: %d\n", ret);

maybe dev_err_probe ? other than that,

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

> +		goto error_media_entity;
> +	}
> +
>   	ret = v4l2_async_register_subdev_sensor(&imx219->sd);
>   	if (ret < 0) {
>   		dev_err(dev, "failed to register sensor sub-device: %d\n", ret);
> -		goto error_media_entity;
> +		goto error_subdev_cleanup;
>   	}
>   
>   	/* Enable runtime PM and turn off the device */
> @@ -1536,6 +1449,9 @@ static int imx219_probe(struct i2c_client *client)
>   
>   	return 0;
>   
> +error_subdev_cleanup:
> +	v4l2_subdev_cleanup(&imx219->sd);
> +
>   error_media_entity:
>   	media_entity_cleanup(&imx219->sd.entity);
>   
> @@ -1554,6 +1470,7 @@ static void imx219_remove(struct i2c_client *client)
>   	struct imx219 *imx219 = to_imx219(sd);
>   
>   	v4l2_async_unregister_subdev(sd);
> +	v4l2_subdev_cleanup(sd);
>   	media_entity_cleanup(&sd->entity);
>   	imx219_free_controls(imx219);
>   

Re: [PATCH v2 7/7] media: i2c: imx219: Simplify code handling in s_fmt

[Umang Jain]

Hi Jacopo

On 7/10/23 9:22 PM, Jacopo Mondi wrote:
> The imx219_set_pad_format() function adjusts the media bus code provided
> through the v4l2_subdev_format parameter to a media bus code known
> to be supported by the sensor.
>
> The same exact operation is performed by the imx219_get_format_code()
> function which called by imx219_update_pad_format(), which is in the
> imx219_set_pad_format() call path.
>
> Remove the duplicated operation and simplify imx219_set_pad_format().
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

> ---
>   drivers/media/i2c/imx219.c | 10 +---------
>   1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 4f214f10846c..a1136fdfbed2 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -750,21 +750,13 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>   	const struct imx219_mode *mode;
>   	int exposure_max, exposure_def, hblank;
>   	struct v4l2_mbus_framefmt *format;
> -	unsigned int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
> -		if (imx219_mbus_formats[i] == fmt->format.code)
> -			break;
> -	if (i >= ARRAY_SIZE(imx219_mbus_formats))
> -		i = 0;
>   
>   	mode = v4l2_find_nearest_size(supported_modes,
>   				      ARRAY_SIZE(supported_modes),
>   				      width, height,
>   				      fmt->format.width, fmt->format.height);
>   
> -	imx219_update_pad_format(imx219, mode, &fmt->format,
> -				 imx219_mbus_formats[i]);
> +	imx219_update_pad_format(imx219, mode, &fmt->format, fmt->format.code);
>   	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
>   
>   	if (imx219->mode == mode && format->code == fmt->format.code)

Re: [PATCH v2 2/7] media: i2c: imx219: Switch from open to init_cfg

[Jacopo Mondi]

Hi Umang

On Sat, Jul 29, 2023 at 07:38:32PM +0530, Umang Jain wrote:
> Hi Jacopo,
>
> On 7/10/23 9:21 PM, Jacopo Mondi wrote:
> > From: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> >
> > Use the init_cfg pad level operation instead of the internal subdev
> > open operation to set default formats on the pads.
> >
> > Signed-off-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >   drivers/media/i2c/imx219.c | 63 +++++++++++++++++++-------------------
> >   1 file changed, 31 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index ac6b0e7a838d..45b219321d98 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -608,34 +608,6 @@ static void imx219_set_default_format(struct imx219 *imx219)
> >   	fmt->field = V4L2_FIELD_NONE;
> >   }
> > -static int imx219_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> > -{
> > -	struct imx219 *imx219 = to_imx219(sd);
> > -	struct v4l2_mbus_framefmt *try_fmt =
> > -		v4l2_subdev_get_try_format(sd, fh->state, 0);
> > -	struct v4l2_rect *try_crop;
> > -
> > -	mutex_lock(&imx219->mutex);
> > -
> > -	/* Initialize try_fmt */
> > -	try_fmt->width = supported_modes[0].width;
> > -	try_fmt->height = supported_modes[0].height;
> > -	try_fmt->code = imx219_get_format_code(imx219,
> > -					       MEDIA_BUS_FMT_SRGGB10_1X10);
> > -	try_fmt->field = V4L2_FIELD_NONE;
> > -
> > -	/* Initialize try_crop rectangle. */
> > -	try_crop = v4l2_subdev_get_try_crop(sd, fh->state, 0);
> > -	try_crop->top = IMX219_PIXEL_ARRAY_TOP;
> > -	try_crop->left = IMX219_PIXEL_ARRAY_LEFT;
> > -	try_crop->width = IMX219_PIXEL_ARRAY_WIDTH;
> > -	try_crop->height = IMX219_PIXEL_ARRAY_HEIGHT;
> > -
> > -	mutex_unlock(&imx219->mutex);
> > -
> > -	return 0;
> > -}
> > -
> >   static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> >   {
> >   	struct imx219 *imx219 =
> > @@ -725,6 +697,36 @@ static const struct v4l2_ctrl_ops imx219_ctrl_ops = {
> >   	.s_ctrl = imx219_set_ctrl,
> >   };
> > +static int imx219_init_cfg(struct v4l2_subdev *sd,
> > +			   struct v4l2_subdev_state *state)
> > +{
> > +	struct imx219 *imx219 = to_imx219(sd);
> > +	struct v4l2_mbus_framefmt *format;
> > +	struct v4l2_rect *crop;
> > +
> > +	/* imx219_get_format_code() wants mutex locked. */
> > +	mutex_lock(&imx219->mutex);
> > +
> > +	/* Initialize try_fmt */
> > +	format = v4l2_subdev_get_pad_format(sd, state, 0);
> > +	format->width = supported_modes[0].width;
> > +	format->height = supported_modes[0].height;
> > +	format->code = imx219_get_format_code(imx219,
> > +					      MEDIA_BUS_FMT_SRGGB10_1X10);
> > +	format->field = V4L2_FIELD_NONE;
> > +
> > +	/* Initialize crop rectangle. */
> > +	crop = v4l2_subdev_get_pad_crop(sd, state, 0);
> > +	crop->top = IMX219_PIXEL_ARRAY_TOP;
> > +	crop->left = IMX219_PIXEL_ARRAY_LEFT;
> > +	crop->width = IMX219_PIXEL_ARRAY_WIDTH;
> > +	crop->height = IMX219_PIXEL_ARRAY_HEIGHT;
> > +
> > +	mutex_unlock(&imx219->mutex);
> > +
> > +	return 0;
> > +}
> > +
> >   static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
> >   				 struct v4l2_subdev_state *sd_state,
> >   				 struct v4l2_subdev_mbus_code_enum *code)
> > @@ -1235,6 +1237,7 @@ static const struct v4l2_subdev_video_ops imx219_video_ops = {
> >   };
> >   static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
> > +	.init_cfg = imx219_init_cfg,
> >   	.enum_mbus_code = imx219_enum_mbus_code,
> >   	.get_fmt = imx219_get_pad_format,
> >   	.set_fmt = imx219_set_pad_format,
> > @@ -1248,9 +1251,6 @@ static const struct v4l2_subdev_ops imx219_subdev_ops = {
> >   	.pad = &imx219_pad_ops,
> >   };
> > -static const struct v4l2_subdev_internal_ops imx219_internal_ops = {
> > -	.open = imx219_open,
> > -};
>
> nit: maybe this extra line needs deletion?

Ah yes indeed!

Thanks and sorry for missing it!

>
> with that,
>
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
>
> >   static unsigned long imx219_get_pixel_rate(struct imx219 *imx219)
> >   {
> > @@ -1509,7 +1509,6 @@ static int imx219_probe(struct i2c_client *client)
> >   		goto error_power_off;
> >   	/* Initialize subdev */
> > -	imx219->sd.internal_ops = &imx219_internal_ops;
> >   	imx219->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> >   			    V4L2_SUBDEV_FL_HAS_EVENTS;
> >   	imx219->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>

Re: [PATCH v2 4/7] media: i2c: imx219: Fix colorspace info

[Jacopo Mondi]

Hi Lauren, Umang

On Sat, Jul 29, 2023 at 08:13:00PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
>
> On Mon, Jul 10, 2023 at 05:52:00PM +0200, Jacopo Mondi wrote:
> > The IMX219 is a RAW sensor. Fix the colorspace configuration by
> > using V4L2_COLORSPACE_RAW and adjust the quantization and transfer
> > function values. Drop ycbcr_enc as it doesn't apply to RAW sensors.
>
> So this addresses part of my comments for 3/7, nice :-)
>
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  drivers/media/i2c/imx219.c | 26 +++++++++-----------------
> >  1 file changed, 9 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index cd43a897391c..6963e24e1bc2 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -597,15 +597,12 @@ static void imx219_set_default_format(struct imx219 *imx219)
> >
> >  	fmt = &imx219->fmt;
> >  	fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
> > -	fmt->colorspace = V4L2_COLORSPACE_SRGB;
> > -	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> > -	fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> > -							  fmt->colorspace,
> > -							  fmt->ycbcr_enc);
> > -	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> > +	fmt->colorspace = V4L2_COLORSPACE_RAW;
> > +	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> >  	fmt->width = supported_modes[0].width;
> >  	fmt->height = supported_modes[0].height;
> >  	fmt->field = V4L2_FIELD_NONE;
> > +	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
>
> Any reason to not group xfer_func with colorspace and quantization ?
>
> >  }
> >
> >  static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> > @@ -714,12 +711,10 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
> >  	format->code = imx219_get_format_code(imx219,
> >  					      MEDIA_BUS_FMT_SRGGB10_1X10);
> >  	format->field = V4L2_FIELD_NONE;
> > -	format->colorspace = V4L2_COLORSPACE_SRGB;
> > +	format->colorspace = V4L2_COLORSPACE_RAW;
> >  	format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);
>
> You're keeping ycbcr_enc here while you've dropped it in the two other
> locations. Was that on purpose ?

Umang had the same comment, and I clearly forgot to remove this one

>
> While the encoding doesn't apply to raw formats, I think it should still
> be set explicitly, or it will end up having a random value.
>

Should I use V4L2_YCBCR_ENC_DEFAULT even if for RAW sensors it doesn't
make much sense ?

Thanks
   j

> > -	format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> > -							     format->colorspace,
> > -							     format->ycbcr_enc);
> > -	format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(format->colorspace);
> > +	format->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > +	format->xfer_func = V4L2_XFER_FUNC_NONE;
> >
> >  	/* Initialize crop rectangle. */
> >  	crop = v4l2_subdev_get_pad_crop(sd, state, 0);
> > @@ -775,12 +770,9 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
> >
> >  static void imx219_reset_colorspace(struct v4l2_mbus_framefmt *fmt)
> >  {
> > -	fmt->colorspace = V4L2_COLORSPACE_SRGB;
> > -	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> > -	fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> > -							  fmt->colorspace,
> > -							  fmt->ycbcr_enc);
> > -	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> > +	fmt->colorspace = V4L2_COLORSPACE_RAW;
> > +	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > +	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
> >  }
> >
> >  static void imx219_update_pad_format(struct imx219 *imx219,
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v2 5/7] media: i2c: imx219: Use subdev active state

[Jacopo Mondi]

Hi Umang

On Mon, Jul 31, 2023 at 02:56:06AM +0530, Umang Jain wrote:
> Hi Jacopo ,
>
> On 7/10/23 9:22 PM, Jacopo Mondi wrote:
> > Port the imx219 sensor driver to use the subdev active state.
> >
> > Move all the format configuration to the subdevice state and simplify
> > the format handling, locking and initialization.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >   drivers/media/i2c/imx219.c | 179 ++++++++++---------------------------
> >   1 file changed, 48 insertions(+), 131 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index 6963e24e1bc2..73e06583d651 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -460,8 +460,6 @@ struct imx219 {
> >   	struct v4l2_subdev sd;
> >   	struct media_pad pad;
> > -	struct v4l2_mbus_framefmt fmt;
> > -
> >   	struct clk *xclk; /* system clock to IMX219 */
> >   	u32 xclk_freq;
> > @@ -481,12 +479,6 @@ struct imx219 {
> >   	/* Current mode */
> >   	const struct imx219_mode *mode;
> > -	/*
> > -	 * Mutex for serialized access:
> > -	 * Protect sensor module set pad format and start/stop streaming safely.
> > -	 */
> > -	struct mutex mutex;
> > -
> >   	/* Streaming on/off */
> >   	bool streaming;
> > @@ -576,8 +568,6 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
> >   {
> >   	unsigned int i;
> > -	lockdep_assert_held(&imx219->mutex);
> > -
> >   	for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
> >   		if (imx219_mbus_formats[i] == code)
> >   			break;
> > @@ -591,20 +581,6 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
> >   	return imx219_mbus_formats[i];
> >   }
> > -static void imx219_set_default_format(struct imx219 *imx219)
> > -{
> > -	struct v4l2_mbus_framefmt *fmt;
> > -
> > -	fmt = &imx219->fmt;
> > -	fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
> > -	fmt->colorspace = V4L2_COLORSPACE_RAW;
> > -	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > -	fmt->width = supported_modes[0].width;
> > -	fmt->height = supported_modes[0].height;
> > -	fmt->field = V4L2_FIELD_NONE;
> > -	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
> > -}
> > -
> >   static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> >   {
> >   	struct imx219 *imx219 =
> > @@ -701,9 +677,6 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
> >   	struct v4l2_mbus_framefmt *format;
> >   	struct v4l2_rect *crop;
> > -	/* imx219_get_format_code() wants mutex locked. */
> > -	mutex_lock(&imx219->mutex);
> > -
> >   	/* Initialize try_fmt */
> >   	format = v4l2_subdev_get_pad_format(sd, state, 0);
> >   	format->width = supported_modes[0].width;
> > @@ -723,8 +696,6 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
> >   	crop->width = IMX219_PIXEL_ARRAY_WIDTH;
> >   	crop->height = IMX219_PIXEL_ARRAY_HEIGHT;
> > -	mutex_unlock(&imx219->mutex);
> > -
> >   	return 0;
> >   }
> > @@ -737,9 +708,7 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
> >   	if (code->index >= (ARRAY_SIZE(imx219_mbus_formats) / 4))
> >   		return -EINVAL;
> > -	mutex_lock(&imx219->mutex);
> >   	code->code = imx219_get_format_code(imx219, imx219_mbus_formats[code->index * 4]);
> > -	mutex_unlock(&imx219->mutex);
> >   	return 0;
> >   }
> > @@ -754,9 +723,7 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
> >   	if (fse->index >= ARRAY_SIZE(supported_modes))
> >   		return -EINVAL;
> > -	mutex_lock(&imx219->mutex);
> >   	code = imx219_get_format_code(imx219, fse->code);
> > -	mutex_unlock(&imx219->mutex);
> >   	if (fse->code != code)
> >   		return -EINVAL;
> > @@ -785,52 +752,16 @@ static void imx219_update_pad_format(struct imx219 *imx219,
> >   	imx219_reset_colorspace(&fmt->format);
> >   }
> > -static int __imx219_get_pad_format(struct imx219 *imx219,
> > -				   struct v4l2_subdev_state *sd_state,
> > -				   struct v4l2_subdev_format *fmt)
> > -{
> > -	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > -		struct v4l2_mbus_framefmt *try_fmt =
> > -			v4l2_subdev_get_try_format(&imx219->sd, sd_state,
> > -						   fmt->pad);
> > -		/* update the code which could change due to vflip or hflip: */
> > -		try_fmt->code = imx219_get_format_code(imx219, try_fmt->code);
> > -		fmt->format = *try_fmt;
> > -	} else {
> > -		imx219_update_pad_format(imx219, imx219->mode, fmt);
> > -		fmt->format.code = imx219_get_format_code(imx219,
> > -							  imx219->fmt.code);
> > -	}
> > -
> > -	return 0;
> > -}
> > -
> > -static int imx219_get_pad_format(struct v4l2_subdev *sd,
> > -				 struct v4l2_subdev_state *sd_state,
> > -				 struct v4l2_subdev_format *fmt)
> > -{
> > -	struct imx219 *imx219 = to_imx219(sd);
> > -	int ret;
> > -
> > -	mutex_lock(&imx219->mutex);
> > -	ret = __imx219_get_pad_format(imx219, sd_state, fmt);
> > -	mutex_unlock(&imx219->mutex);
> > -
> > -	return ret;
> > -}
> > -
> >   static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >   				 struct v4l2_subdev_state *sd_state,
> >   				 struct v4l2_subdev_format *fmt)
> >   {
> >   	struct imx219 *imx219 = to_imx219(sd);
> >   	const struct imx219_mode *mode;
> > -	struct v4l2_mbus_framefmt *framefmt;
> >   	int exposure_max, exposure_def, hblank;
> > +	struct v4l2_mbus_framefmt *format;
> >   	unsigned int i;
> > -	mutex_lock(&imx219->mutex);
> > -
> >   	for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
> >   		if (imx219_mbus_formats[i] == fmt->format.code)
> >   			break;
> > @@ -844,13 +775,14 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >   				      ARRAY_SIZE(supported_modes),
> >   				      width, height,
> >   				      fmt->format.width, fmt->format.height);
> > +
> >   	imx219_update_pad_format(imx219, mode, fmt);
> > -	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> > -		framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
> > -		*framefmt = fmt->format;
> > -	} else if (imx219->mode != mode ||
> > -		   imx219->fmt.code != fmt->format.code) {
> > -		imx219->fmt = fmt->format;
> > +	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> > +
> > +	if (imx219->mode == mode && format->code == fmt->format.code)
> > +		return 0;
> > +
> > +	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> >   		imx219->mode = mode;
> >   		/* Update limits and set FPS to default */
> >   		__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
> > @@ -876,14 +808,15 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >   					 hblank);
> >   	}
> > -	mutex_unlock(&imx219->mutex);
> > +	*format = fmt->format;
> >   	return 0;
> >   }
> > -static int imx219_set_framefmt(struct imx219 *imx219)
> > +static int imx219_set_framefmt(struct imx219 *imx219,
> > +			       const struct v4l2_mbus_framefmt *format)
> >   {
> > -	switch (imx219->fmt.code) {
> > +	switch (format->code) {
> >   	case MEDIA_BUS_FMT_SRGGB8_1X8:
> >   	case MEDIA_BUS_FMT_SGRBG8_1X8:
> >   	case MEDIA_BUS_FMT_SGBRG8_1X8:
> > @@ -902,7 +835,8 @@ static int imx219_set_framefmt(struct imx219 *imx219)
> >   	return -EINVAL;
> >   }
> > -static int imx219_set_binning(struct imx219 *imx219)
> > +static int imx219_set_binning(struct imx219 *imx219,
> > +			      const struct v4l2_mbus_framefmt *format)
> >   {
> >   	if (!imx219->mode->binning) {
> >   		return imx219_write_reg(imx219, IMX219_REG_BINNING_MODE,
> > @@ -910,7 +844,7 @@ static int imx219_set_binning(struct imx219 *imx219)
> >   					IMX219_BINNING_NONE);
> >   	}
> > -	switch (imx219->fmt.code) {
> > +	switch (format->code) {
> >   	case MEDIA_BUS_FMT_SRGGB8_1X8:
> >   	case MEDIA_BUS_FMT_SGRBG8_1X8:
> >   	case MEDIA_BUS_FMT_SGBRG8_1X8:
> > @@ -931,34 +865,13 @@ static int imx219_set_binning(struct imx219 *imx219)
> >   	return -EINVAL;
> >   }
> > -static const struct v4l2_rect *
> > -__imx219_get_pad_crop(struct imx219 *imx219,
> > -		      struct v4l2_subdev_state *sd_state,
> > -		      unsigned int pad, enum v4l2_subdev_format_whence which)
> > -{
> > -	switch (which) {
> > -	case V4L2_SUBDEV_FORMAT_TRY:
> > -		return v4l2_subdev_get_try_crop(&imx219->sd, sd_state, pad);
> > -	case V4L2_SUBDEV_FORMAT_ACTIVE:
> > -		return &imx219->mode->crop;
> > -	}
> > -
> > -	return NULL;
> > -}
> > -
> >   static int imx219_get_selection(struct v4l2_subdev *sd,
> >   				struct v4l2_subdev_state *sd_state,
> >   				struct v4l2_subdev_selection *sel)
> >   {
> >   	switch (sel->target) {
> >   	case V4L2_SEL_TGT_CROP: {
> > -		struct imx219 *imx219 = to_imx219(sd);
> > -
> > -		mutex_lock(&imx219->mutex);
> > -		sel->r = *__imx219_get_pad_crop(imx219, sd_state, sel->pad,
> > -						sel->which);
> > -		mutex_unlock(&imx219->mutex);
> > -
> > +		sel->r = *v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> >   		return 0;
> >   	}
> > @@ -990,9 +903,11 @@ static int imx219_configure_lanes(struct imx219 *imx219)
> >   				IMX219_CSI_2_LANE_MODE : IMX219_CSI_4_LANE_MODE);
> >   };
> > -static int imx219_start_streaming(struct imx219 *imx219)
> > +static int imx219_start_streaming(struct imx219 *imx219,
> > +				  struct v4l2_subdev_state *state)
> >   {
> >   	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
> > +	const struct v4l2_mbus_framefmt *format;
> >   	const struct imx219_reg_list *reg_list;
> >   	int ret;
> > @@ -1022,14 +937,15 @@ static int imx219_start_streaming(struct imx219 *imx219)
> >   		goto err_rpm_put;
> >   	}
> > -	ret = imx219_set_framefmt(imx219);
> > +	format = v4l2_subdev_get_pad_format(&imx219->sd, state, 0);
> > +	ret = imx219_set_framefmt(imx219, format);
> >   	if (ret) {
> >   		dev_err(&client->dev, "%s failed to set frame format: %d\n",
> >   			__func__, ret);
> >   		goto err_rpm_put;
> >   	}
> > -	ret = imx219_set_binning(imx219);
> > +	ret = imx219_set_binning(imx219, format);
> >   	if (ret) {
> >   		dev_err(&client->dev, "%s failed to set binning: %d\n",
> >   			__func__, ret);
> > @@ -1078,35 +994,30 @@ static void imx219_stop_streaming(struct imx219 *imx219)
> >   static int imx219_set_stream(struct v4l2_subdev *sd, int enable)
> >   {
> >   	struct imx219 *imx219 = to_imx219(sd);
> > +	struct v4l2_subdev_state *state;
> >   	int ret = 0;
> > -	mutex_lock(&imx219->mutex);
> > -	if (imx219->streaming == enable) {
> > -		mutex_unlock(&imx219->mutex);
> > -		return 0;
> > -	}
> > +	state = v4l2_subdev_lock_and_get_active_state(sd);
> > +
> > +	if (imx219->streaming == enable)
> > +		goto unlock;
> >   	if (enable) {
> >   		/*
> >   		 * Apply default & customized values
> >   		 * and then start streaming.
> >   		 */
> > -		ret = imx219_start_streaming(imx219);
> > +		ret = imx219_start_streaming(imx219, state);
> >   		if (ret)
> > -			goto err_unlock;
> > +			goto unlock;
> >   	} else {
> >   		imx219_stop_streaming(imx219);
> >   	}
> >   	imx219->streaming = enable;
> > -	mutex_unlock(&imx219->mutex);
> > -
> > -	return ret;
> > -
> > -err_unlock:
> > -	mutex_unlock(&imx219->mutex);
> > -
> > +unlock:
> > +	v4l2_subdev_unlock_state(state);
> >   	return ret;
> >   }
> > @@ -1171,10 +1082,13 @@ static int __maybe_unused imx219_resume(struct device *dev)
> >   {
> >   	struct v4l2_subdev *sd = dev_get_drvdata(dev);
> >   	struct imx219 *imx219 = to_imx219(sd);
> > +	struct v4l2_subdev_state *state;
> >   	int ret;
> >   	if (imx219->streaming) {
> > -		ret = imx219_start_streaming(imx219);
> > +		state = v4l2_subdev_lock_and_get_active_state(sd);
> > +		ret = imx219_start_streaming(imx219, state);
> > +		v4l2_subdev_unlock_state(state);
> >   		if (ret)
> >   			goto error;
> >   	}
> > @@ -1237,7 +1151,7 @@ static const struct v4l2_subdev_video_ops imx219_video_ops = {
> >   static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
> >   	.init_cfg = imx219_init_cfg,
> >   	.enum_mbus_code = imx219_enum_mbus_code,
> > -	.get_fmt = imx219_get_pad_format,
> > +	.get_fmt = v4l2_subdev_get_fmt,
> >   	.set_fmt = imx219_set_pad_format,
> >   	.get_selection = imx219_get_selection,
> >   	.enum_frame_size = imx219_enum_frame_size,
> > @@ -1270,9 +1184,6 @@ static int imx219_init_controls(struct imx219 *imx219)
> >   	if (ret)
> >   		return ret;
> > -	mutex_init(&imx219->mutex);
> > -	ctrl_hdlr->lock = &imx219->mutex;
> > -
> >   	/* By default, PIXEL_RATE is read only */
> >   	imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
> >   					       V4L2_CID_PIXEL_RATE,
> > @@ -1369,7 +1280,6 @@ static int imx219_init_controls(struct imx219 *imx219)
> >   error:
> >   	v4l2_ctrl_handler_free(ctrl_hdlr);
> > -	mutex_destroy(&imx219->mutex);
> >   	return ret;
> >   }
> > @@ -1377,7 +1287,6 @@ static int imx219_init_controls(struct imx219 *imx219)
> >   static void imx219_free_controls(struct imx219 *imx219)
> >   {
> >   	v4l2_ctrl_handler_free(imx219->sd.ctrl_handler);
> > -	mutex_destroy(&imx219->mutex);
> >   }
> >   static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
> > @@ -1514,19 +1423,23 @@ static int imx219_probe(struct i2c_client *client)
> >   	/* Initialize source pad */
> >   	imx219->pad.flags = MEDIA_PAD_FL_SOURCE;
> > -	/* Initialize default format */
> > -	imx219_set_default_format(imx219);
> > -
> >   	ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
> >   	if (ret) {
> >   		dev_err(dev, "failed to init entity pads: %d\n", ret);
> >   		goto error_handler_free;
> >   	}
> > +	imx219->sd.state_lock = imx219->ctrl_handler.lock;
> > +	ret = v4l2_subdev_init_finalize(&imx219->sd);
> > +	if (ret < 0) {
> > +		dev_err(dev, "subdev init error: %d\n", ret);
>
> maybe dev_err_probe ? other than that,

Is there anything that might cause a deferred probe in the
v4l2_subdev_init_finalize() call path ? I can only see a call to
the init_cfg() operation where one could potentially hit a deferred
probe, but this driver's implementation doesn't (and other shouldn't
anyway).

>
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
>
> > +		goto error_media_entity;
> > +	}
> > +
> >   	ret = v4l2_async_register_subdev_sensor(&imx219->sd);
> >   	if (ret < 0) {
> >   		dev_err(dev, "failed to register sensor sub-device: %d\n", ret);
> > -		goto error_media_entity;
> > +		goto error_subdev_cleanup;
> >   	}
> >   	/* Enable runtime PM and turn off the device */
> > @@ -1536,6 +1449,9 @@ static int imx219_probe(struct i2c_client *client)
> >   	return 0;
> > +error_subdev_cleanup:
> > +	v4l2_subdev_cleanup(&imx219->sd);
> > +
> >   error_media_entity:
> >   	media_entity_cleanup(&imx219->sd.entity);
> > @@ -1554,6 +1470,7 @@ static void imx219_remove(struct i2c_client *client)
> >   	struct imx219 *imx219 = to_imx219(sd);
> >   	v4l2_async_unregister_subdev(sd);
> > +	v4l2_subdev_cleanup(sd);
> >   	media_entity_cleanup(&sd->entity);
> >   	imx219_free_controls(imx219);
>

Re: [PATCH v2 4/7] media: i2c: imx219: Fix colorspace info

[Laurent Pinchart]

On Mon, Jul 31, 2023 at 09:32:08AM +0200, Jacopo Mondi wrote:
> On Sat, Jul 29, 2023 at 08:13:00PM +0300, Laurent Pinchart wrote:
> > On Mon, Jul 10, 2023 at 05:52:00PM +0200, Jacopo Mondi wrote:
> > > The IMX219 is a RAW sensor. Fix the colorspace configuration by
> > > using V4L2_COLORSPACE_RAW and adjust the quantization and transfer
> > > function values. Drop ycbcr_enc as it doesn't apply to RAW sensors.
> >
> > So this addresses part of my comments for 3/7, nice :-)
> >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > ---
> > >  drivers/media/i2c/imx219.c | 26 +++++++++-----------------
> > >  1 file changed, 9 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > index cd43a897391c..6963e24e1bc2 100644
> > > --- a/drivers/media/i2c/imx219.c
> > > +++ b/drivers/media/i2c/imx219.c
> > > @@ -597,15 +597,12 @@ static void imx219_set_default_format(struct imx219 *imx219)
> > >
> > >  	fmt = &imx219->fmt;
> > >  	fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
> > > -	fmt->colorspace = V4L2_COLORSPACE_SRGB;
> > > -	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> > > -	fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> > > -							  fmt->colorspace,
> > > -							  fmt->ycbcr_enc);
> > > -	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> > > +	fmt->colorspace = V4L2_COLORSPACE_RAW;
> > > +	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > >  	fmt->width = supported_modes[0].width;
> > >  	fmt->height = supported_modes[0].height;
> > >  	fmt->field = V4L2_FIELD_NONE;
> > > +	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
> >
> > Any reason to not group xfer_func with colorspace and quantization ?
> >
> > >  }
> > >
> > >  static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
> > > @@ -714,12 +711,10 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
> > >  	format->code = imx219_get_format_code(imx219,
> > >  					      MEDIA_BUS_FMT_SRGGB10_1X10);
> > >  	format->field = V4L2_FIELD_NONE;
> > > -	format->colorspace = V4L2_COLORSPACE_SRGB;
> > > +	format->colorspace = V4L2_COLORSPACE_RAW;
> > >  	format->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(format->colorspace);
> >
> > You're keeping ycbcr_enc here while you've dropped it in the two other
> > locations. Was that on purpose ?
> 
> Umang had the same comment, and I clearly forgot to remove this one
> 
> > While the encoding doesn't apply to raw formats, I think it should still
> > be set explicitly, or it will end up having a random value.
> 
> Should I use V4L2_YCBCR_ENC_DEFAULT even if for RAW sensors it doesn't
> make much sense ?

For the transfer function we have V4L2_XFER_FUNC_NONE, but for the
encoding we have no similar value. I recall that V4L2_YCBCR_ENC_DEFAULT
is meant for applications only, and that drivers must always return an
appropriate non-default value, but I can't find at the moment where that
is documented. It may have been from a discussion with Hans, and
v4l2-compliance may enforce it, I haven't checked.

We could add a new V4L2_YCBCR_ENC_NONE value, allow
V4L2_YCBCR_ENC_DEFAULT for this use case, or return V4L2_YCBCR_ENC_601
as done by other drivers (this is the value returned by
V4L2_MAP_YCBCR_ENC_DEFAULT() for a RAW colorspace).

> > > -	format->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> > > -							     format->colorspace,
> > > -							     format->ycbcr_enc);
> > > -	format->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(format->colorspace);
> > > +	format->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > > +	format->xfer_func = V4L2_XFER_FUNC_NONE;
> > >
> > >  	/* Initialize crop rectangle. */
> > >  	crop = v4l2_subdev_get_pad_crop(sd, state, 0);
> > > @@ -775,12 +770,9 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
> > >
> > >  static void imx219_reset_colorspace(struct v4l2_mbus_framefmt *fmt)
> > >  {
> > > -	fmt->colorspace = V4L2_COLORSPACE_SRGB;
> > > -	fmt->ycbcr_enc = V4L2_MAP_YCBCR_ENC_DEFAULT(fmt->colorspace);
> > > -	fmt->quantization = V4L2_MAP_QUANTIZATION_DEFAULT(true,
> > > -							  fmt->colorspace,
> > > -							  fmt->ycbcr_enc);
> > > -	fmt->xfer_func = V4L2_MAP_XFER_FUNC_DEFAULT(fmt->colorspace);
> > > +	fmt->colorspace = V4L2_COLORSPACE_RAW;
> > > +	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> > > +	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
> > >  }
> > >
> > >  static void imx219_update_pad_format(struct imx219 *imx219,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 5/7] media: i2c: imx219: Use subdev active state

[Umang Jain]

Hi Jacopo.

On 7/31/23 1:05 PM, Jacopo Mondi wrote:
> Hi Umang
>
> On Mon, Jul 31, 2023 at 02:56:06AM +0530, Umang Jain wrote:
>> Hi Jacopo ,
>>
>> On 7/10/23 9:22 PM, Jacopo Mondi wrote:
>>> Port the imx219 sensor driver to use the subdev active state.
>>>
>>> Move all the format configuration to the subdevice state and simplify
>>> the format handling, locking and initialization.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> ---
>>>    drivers/media/i2c/imx219.c | 179 ++++++++++---------------------------
>>>    1 file changed, 48 insertions(+), 131 deletions(-)
>>>
>>> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
>>> index 6963e24e1bc2..73e06583d651 100644
>>> --- a/drivers/media/i2c/imx219.c
>>> +++ b/drivers/media/i2c/imx219.c
>>> @@ -460,8 +460,6 @@ struct imx219 {
>>>    	struct v4l2_subdev sd;
>>>    	struct media_pad pad;
>>> -	struct v4l2_mbus_framefmt fmt;
>>> -
>>>    	struct clk *xclk; /* system clock to IMX219 */
>>>    	u32 xclk_freq;
>>> @@ -481,12 +479,6 @@ struct imx219 {
>>>    	/* Current mode */
>>>    	const struct imx219_mode *mode;
>>> -	/*
>>> -	 * Mutex for serialized access:
>>> -	 * Protect sensor module set pad format and start/stop streaming safely.
>>> -	 */
>>> -	struct mutex mutex;
>>> -
>>>    	/* Streaming on/off */
>>>    	bool streaming;
>>> @@ -576,8 +568,6 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
>>>    {
>>>    	unsigned int i;
>>> -	lockdep_assert_held(&imx219->mutex);
>>> -
>>>    	for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
>>>    		if (imx219_mbus_formats[i] == code)
>>>    			break;
>>> @@ -591,20 +581,6 @@ static u32 imx219_get_format_code(struct imx219 *imx219, u32 code)
>>>    	return imx219_mbus_formats[i];
>>>    }
>>> -static void imx219_set_default_format(struct imx219 *imx219)
>>> -{
>>> -	struct v4l2_mbus_framefmt *fmt;
>>> -
>>> -	fmt = &imx219->fmt;
>>> -	fmt->code = MEDIA_BUS_FMT_SRGGB10_1X10;
>>> -	fmt->colorspace = V4L2_COLORSPACE_RAW;
>>> -	fmt->quantization = V4L2_QUANTIZATION_FULL_RANGE;
>>> -	fmt->width = supported_modes[0].width;
>>> -	fmt->height = supported_modes[0].height;
>>> -	fmt->field = V4L2_FIELD_NONE;
>>> -	fmt->xfer_func = V4L2_XFER_FUNC_NONE;
>>> -}
>>> -
>>>    static int imx219_set_ctrl(struct v4l2_ctrl *ctrl)
>>>    {
>>>    	struct imx219 *imx219 =
>>> @@ -701,9 +677,6 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
>>>    	struct v4l2_mbus_framefmt *format;
>>>    	struct v4l2_rect *crop;
>>> -	/* imx219_get_format_code() wants mutex locked. */
>>> -	mutex_lock(&imx219->mutex);
>>> -
>>>    	/* Initialize try_fmt */
>>>    	format = v4l2_subdev_get_pad_format(sd, state, 0);
>>>    	format->width = supported_modes[0].width;
>>> @@ -723,8 +696,6 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
>>>    	crop->width = IMX219_PIXEL_ARRAY_WIDTH;
>>>    	crop->height = IMX219_PIXEL_ARRAY_HEIGHT;
>>> -	mutex_unlock(&imx219->mutex);
>>> -
>>>    	return 0;
>>>    }
>>> @@ -737,9 +708,7 @@ static int imx219_enum_mbus_code(struct v4l2_subdev *sd,
>>>    	if (code->index >= (ARRAY_SIZE(imx219_mbus_formats) / 4))
>>>    		return -EINVAL;
>>> -	mutex_lock(&imx219->mutex);
>>>    	code->code = imx219_get_format_code(imx219, imx219_mbus_formats[code->index * 4]);
>>> -	mutex_unlock(&imx219->mutex);
>>>    	return 0;
>>>    }
>>> @@ -754,9 +723,7 @@ static int imx219_enum_frame_size(struct v4l2_subdev *sd,
>>>    	if (fse->index >= ARRAY_SIZE(supported_modes))
>>>    		return -EINVAL;
>>> -	mutex_lock(&imx219->mutex);
>>>    	code = imx219_get_format_code(imx219, fse->code);
>>> -	mutex_unlock(&imx219->mutex);
>>>    	if (fse->code != code)
>>>    		return -EINVAL;
>>> @@ -785,52 +752,16 @@ static void imx219_update_pad_format(struct imx219 *imx219,
>>>    	imx219_reset_colorspace(&fmt->format);
>>>    }
>>> -static int __imx219_get_pad_format(struct imx219 *imx219,
>>> -				   struct v4l2_subdev_state *sd_state,
>>> -				   struct v4l2_subdev_format *fmt)
>>> -{
>>> -	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
>>> -		struct v4l2_mbus_framefmt *try_fmt =
>>> -			v4l2_subdev_get_try_format(&imx219->sd, sd_state,
>>> -						   fmt->pad);
>>> -		/* update the code which could change due to vflip or hflip: */
>>> -		try_fmt->code = imx219_get_format_code(imx219, try_fmt->code);
>>> -		fmt->format = *try_fmt;
>>> -	} else {
>>> -		imx219_update_pad_format(imx219, imx219->mode, fmt);
>>> -		fmt->format.code = imx219_get_format_code(imx219,
>>> -							  imx219->fmt.code);
>>> -	}
>>> -
>>> -	return 0;
>>> -}
>>> -
>>> -static int imx219_get_pad_format(struct v4l2_subdev *sd,
>>> -				 struct v4l2_subdev_state *sd_state,
>>> -				 struct v4l2_subdev_format *fmt)
>>> -{
>>> -	struct imx219 *imx219 = to_imx219(sd);
>>> -	int ret;
>>> -
>>> -	mutex_lock(&imx219->mutex);
>>> -	ret = __imx219_get_pad_format(imx219, sd_state, fmt);
>>> -	mutex_unlock(&imx219->mutex);
>>> -
>>> -	return ret;
>>> -}
>>> -
>>>    static int imx219_set_pad_format(struct v4l2_subdev *sd,
>>>    				 struct v4l2_subdev_state *sd_state,
>>>    				 struct v4l2_subdev_format *fmt)
>>>    {
>>>    	struct imx219 *imx219 = to_imx219(sd);
>>>    	const struct imx219_mode *mode;
>>> -	struct v4l2_mbus_framefmt *framefmt;
>>>    	int exposure_max, exposure_def, hblank;
>>> +	struct v4l2_mbus_framefmt *format;
>>>    	unsigned int i;
>>> -	mutex_lock(&imx219->mutex);
>>> -
>>>    	for (i = 0; i < ARRAY_SIZE(imx219_mbus_formats); i++)
>>>    		if (imx219_mbus_formats[i] == fmt->format.code)
>>>    			break;
>>> @@ -844,13 +775,14 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>>>    				      ARRAY_SIZE(supported_modes),
>>>    				      width, height,
>>>    				      fmt->format.width, fmt->format.height);
>>> +
>>>    	imx219_update_pad_format(imx219, mode, fmt);
>>> -	if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
>>> -		framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
>>> -		*framefmt = fmt->format;
>>> -	} else if (imx219->mode != mode ||
>>> -		   imx219->fmt.code != fmt->format.code) {
>>> -		imx219->fmt = fmt->format;
>>> +	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
>>> +
>>> +	if (imx219->mode == mode && format->code == fmt->format.code)
>>> +		return 0;
>>> +
>>> +	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>>>    		imx219->mode = mode;
>>>    		/* Update limits and set FPS to default */
>>>    		__v4l2_ctrl_modify_range(imx219->vblank, IMX219_VBLANK_MIN,
>>> @@ -876,14 +808,15 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>>>    					 hblank);
>>>    	}
>>> -	mutex_unlock(&imx219->mutex);
>>> +	*format = fmt->format;
>>>    	return 0;
>>>    }
>>> -static int imx219_set_framefmt(struct imx219 *imx219)
>>> +static int imx219_set_framefmt(struct imx219 *imx219,
>>> +			       const struct v4l2_mbus_framefmt *format)
>>>    {
>>> -	switch (imx219->fmt.code) {
>>> +	switch (format->code) {
>>>    	case MEDIA_BUS_FMT_SRGGB8_1X8:
>>>    	case MEDIA_BUS_FMT_SGRBG8_1X8:
>>>    	case MEDIA_BUS_FMT_SGBRG8_1X8:
>>> @@ -902,7 +835,8 @@ static int imx219_set_framefmt(struct imx219 *imx219)
>>>    	return -EINVAL;
>>>    }
>>> -static int imx219_set_binning(struct imx219 *imx219)
>>> +static int imx219_set_binning(struct imx219 *imx219,
>>> +			      const struct v4l2_mbus_framefmt *format)
>>>    {
>>>    	if (!imx219->mode->binning) {
>>>    		return imx219_write_reg(imx219, IMX219_REG_BINNING_MODE,
>>> @@ -910,7 +844,7 @@ static int imx219_set_binning(struct imx219 *imx219)
>>>    					IMX219_BINNING_NONE);
>>>    	}
>>> -	switch (imx219->fmt.code) {
>>> +	switch (format->code) {
>>>    	case MEDIA_BUS_FMT_SRGGB8_1X8:
>>>    	case MEDIA_BUS_FMT_SGRBG8_1X8:
>>>    	case MEDIA_BUS_FMT_SGBRG8_1X8:
>>> @@ -931,34 +865,13 @@ static int imx219_set_binning(struct imx219 *imx219)
>>>    	return -EINVAL;
>>>    }
>>> -static const struct v4l2_rect *
>>> -__imx219_get_pad_crop(struct imx219 *imx219,
>>> -		      struct v4l2_subdev_state *sd_state,
>>> -		      unsigned int pad, enum v4l2_subdev_format_whence which)
>>> -{
>>> -	switch (which) {
>>> -	case V4L2_SUBDEV_FORMAT_TRY:
>>> -		return v4l2_subdev_get_try_crop(&imx219->sd, sd_state, pad);
>>> -	case V4L2_SUBDEV_FORMAT_ACTIVE:
>>> -		return &imx219->mode->crop;
>>> -	}
>>> -
>>> -	return NULL;
>>> -}
>>> -
>>>    static int imx219_get_selection(struct v4l2_subdev *sd,
>>>    				struct v4l2_subdev_state *sd_state,
>>>    				struct v4l2_subdev_selection *sel)
>>>    {
>>>    	switch (sel->target) {
>>>    	case V4L2_SEL_TGT_CROP: {
>>> -		struct imx219 *imx219 = to_imx219(sd);
>>> -
>>> -		mutex_lock(&imx219->mutex);
>>> -		sel->r = *__imx219_get_pad_crop(imx219, sd_state, sel->pad,
>>> -						sel->which);
>>> -		mutex_unlock(&imx219->mutex);
>>> -
>>> +		sel->r = *v4l2_subdev_get_pad_crop(sd, sd_state, 0);
>>>    		return 0;
>>>    	}
>>> @@ -990,9 +903,11 @@ static int imx219_configure_lanes(struct imx219 *imx219)
>>>    				IMX219_CSI_2_LANE_MODE : IMX219_CSI_4_LANE_MODE);
>>>    };
>>> -static int imx219_start_streaming(struct imx219 *imx219)
>>> +static int imx219_start_streaming(struct imx219 *imx219,
>>> +				  struct v4l2_subdev_state *state)
>>>    {
>>>    	struct i2c_client *client = v4l2_get_subdevdata(&imx219->sd);
>>> +	const struct v4l2_mbus_framefmt *format;
>>>    	const struct imx219_reg_list *reg_list;
>>>    	int ret;
>>> @@ -1022,14 +937,15 @@ static int imx219_start_streaming(struct imx219 *imx219)
>>>    		goto err_rpm_put;
>>>    	}
>>> -	ret = imx219_set_framefmt(imx219);
>>> +	format = v4l2_subdev_get_pad_format(&imx219->sd, state, 0);
>>> +	ret = imx219_set_framefmt(imx219, format);
>>>    	if (ret) {
>>>    		dev_err(&client->dev, "%s failed to set frame format: %d\n",
>>>    			__func__, ret);
>>>    		goto err_rpm_put;
>>>    	}
>>> -	ret = imx219_set_binning(imx219);
>>> +	ret = imx219_set_binning(imx219, format);
>>>    	if (ret) {
>>>    		dev_err(&client->dev, "%s failed to set binning: %d\n",
>>>    			__func__, ret);
>>> @@ -1078,35 +994,30 @@ static void imx219_stop_streaming(struct imx219 *imx219)
>>>    static int imx219_set_stream(struct v4l2_subdev *sd, int enable)
>>>    {
>>>    	struct imx219 *imx219 = to_imx219(sd);
>>> +	struct v4l2_subdev_state *state;
>>>    	int ret = 0;
>>> -	mutex_lock(&imx219->mutex);
>>> -	if (imx219->streaming == enable) {
>>> -		mutex_unlock(&imx219->mutex);
>>> -		return 0;
>>> -	}
>>> +	state = v4l2_subdev_lock_and_get_active_state(sd);
>>> +
>>> +	if (imx219->streaming == enable)
>>> +		goto unlock;
>>>    	if (enable) {
>>>    		/*
>>>    		 * Apply default & customized values
>>>    		 * and then start streaming.
>>>    		 */
>>> -		ret = imx219_start_streaming(imx219);
>>> +		ret = imx219_start_streaming(imx219, state);
>>>    		if (ret)
>>> -			goto err_unlock;
>>> +			goto unlock;
>>>    	} else {
>>>    		imx219_stop_streaming(imx219);
>>>    	}
>>>    	imx219->streaming = enable;
>>> -	mutex_unlock(&imx219->mutex);
>>> -
>>> -	return ret;
>>> -
>>> -err_unlock:
>>> -	mutex_unlock(&imx219->mutex);
>>> -
>>> +unlock:
>>> +	v4l2_subdev_unlock_state(state);
>>>    	return ret;
>>>    }
>>> @@ -1171,10 +1082,13 @@ static int __maybe_unused imx219_resume(struct device *dev)
>>>    {
>>>    	struct v4l2_subdev *sd = dev_get_drvdata(dev);
>>>    	struct imx219 *imx219 = to_imx219(sd);
>>> +	struct v4l2_subdev_state *state;
>>>    	int ret;
>>>    	if (imx219->streaming) {
>>> -		ret = imx219_start_streaming(imx219);
>>> +		state = v4l2_subdev_lock_and_get_active_state(sd);
>>> +		ret = imx219_start_streaming(imx219, state);
>>> +		v4l2_subdev_unlock_state(state);
>>>    		if (ret)
>>>    			goto error;
>>>    	}
>>> @@ -1237,7 +1151,7 @@ static const struct v4l2_subdev_video_ops imx219_video_ops = {
>>>    static const struct v4l2_subdev_pad_ops imx219_pad_ops = {
>>>    	.init_cfg = imx219_init_cfg,
>>>    	.enum_mbus_code = imx219_enum_mbus_code,
>>> -	.get_fmt = imx219_get_pad_format,
>>> +	.get_fmt = v4l2_subdev_get_fmt,
>>>    	.set_fmt = imx219_set_pad_format,
>>>    	.get_selection = imx219_get_selection,
>>>    	.enum_frame_size = imx219_enum_frame_size,
>>> @@ -1270,9 +1184,6 @@ static int imx219_init_controls(struct imx219 *imx219)
>>>    	if (ret)
>>>    		return ret;
>>> -	mutex_init(&imx219->mutex);
>>> -	ctrl_hdlr->lock = &imx219->mutex;
>>> -
>>>    	/* By default, PIXEL_RATE is read only */
>>>    	imx219->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, &imx219_ctrl_ops,
>>>    					       V4L2_CID_PIXEL_RATE,
>>> @@ -1369,7 +1280,6 @@ static int imx219_init_controls(struct imx219 *imx219)
>>>    error:
>>>    	v4l2_ctrl_handler_free(ctrl_hdlr);
>>> -	mutex_destroy(&imx219->mutex);
>>>    	return ret;
>>>    }
>>> @@ -1377,7 +1287,6 @@ static int imx219_init_controls(struct imx219 *imx219)
>>>    static void imx219_free_controls(struct imx219 *imx219)
>>>    {
>>>    	v4l2_ctrl_handler_free(imx219->sd.ctrl_handler);
>>> -	mutex_destroy(&imx219->mutex);
>>>    }
>>>    static int imx219_check_hwcfg(struct device *dev, struct imx219 *imx219)
>>> @@ -1514,19 +1423,23 @@ static int imx219_probe(struct i2c_client *client)
>>>    	/* Initialize source pad */
>>>    	imx219->pad.flags = MEDIA_PAD_FL_SOURCE;
>>> -	/* Initialize default format */
>>> -	imx219_set_default_format(imx219);
>>> -
>>>    	ret = media_entity_pads_init(&imx219->sd.entity, 1, &imx219->pad);
>>>    	if (ret) {
>>>    		dev_err(dev, "failed to init entity pads: %d\n", ret);
>>>    		goto error_handler_free;
>>>    	}
>>> +	imx219->sd.state_lock = imx219->ctrl_handler.lock;
>>> +	ret = v4l2_subdev_init_finalize(&imx219->sd);
>>> +	if (ret < 0) {
>>> +		dev_err(dev, "subdev init error: %d\n", ret);
>> maybe dev_err_probe ? other than that,
> Is there anything that might cause a deferred probe in the
> v4l2_subdev_init_finalize() call path ? I can only see a call to
> the init_cfg() operation where one could potentially hit a deferred
> probe, but this driver's implementation doesn't (and other shouldn't
> anyway).
>
You are correct!  I don't see any op causing deffered probe too.
Sorry for the noise!
>> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
>>
>>> +		goto error_media_entity;
>>> +	}
>>> +
>>>    	ret = v4l2_async_register_subdev_sensor(&imx219->sd);
>>>    	if (ret < 0) {
>>>    		dev_err(dev, "failed to register sensor sub-device: %d\n", ret);
>>> -		goto error_media_entity;
>>> +		goto error_subdev_cleanup;
>>>    	}
>>>    	/* Enable runtime PM and turn off the device */
>>> @@ -1536,6 +1449,9 @@ static int imx219_probe(struct i2c_client *client)
>>>    	return 0;
>>> +error_subdev_cleanup:
>>> +	v4l2_subdev_cleanup(&imx219->sd);
>>> +
>>>    error_media_entity:
>>>    	media_entity_cleanup(&imx219->sd.entity);
>>> @@ -1554,6 +1470,7 @@ static void imx219_remove(struct i2c_client *client)
>>>    	struct imx219 *imx219 = to_imx219(sd);
>>>    	v4l2_async_unregister_subdev(sd);
>>> +	v4l2_subdev_cleanup(sd);
>>>    	media_entity_cleanup(&sd->entity);
>>>    	imx219_free_controls(imx219);