[PATCH v1 0/9] media: vimc improvements

[PATCH v1 0/9] media: vimc improvements

[Laurent Pinchart]

Hello,

This patch series cleans up and improves the vimc driver, with the end
goal of converting it to the V4L2 subdev active state API. The goal of
this exercise is to make the API used by a virtual test driver, to
increase test coverage.

The series starts with 4 random cleanups, to avoid unnecessary
iterations (1/9), constify structures (2/9 and 3/9) and rename a
weirdly-named enum (4/9). Patch 5/9 then centralizes the subdev
internal_ops initialization to prepare for the switch to the active
state API. The remaining patches (6/9 to 9/9) convert the vimc entities
to the new API one by one.

The result has been tested using the libcamera unit tests, which make
extensive use of the vimc driver, as well as with v4l2-compliance. The
latter reports 4 errors, but they occur already with the latest stage
master branch.

Laurent Pinchart (9):
  media: vimc: Don't iterate over single pad
  media: vimc: Constify vimc_ent_type structures
  media: vimc: Constify the ent_config array
  media: vimc: scaler: Rename vic_sca_pad enum to vimc_scaler_pad
  media: vimc: Centralize subdev internal_ops initialization
  media: vimc: Initialize subdev active state
  media: vimc: sensor: Use subdev active state
  media: vimc: debayer: Use subdev active state
  media: vimc: scaler: Use subdev active state

 .../media/test-drivers/vimc/vimc-capture.c    |   2 +-
 drivers/media/test-drivers/vimc/vimc-common.c |  25 ++-
 drivers/media/test-drivers/vimc/vimc-common.h |  14 +-
 drivers/media/test-drivers/vimc/vimc-core.c   |   2 +-
 .../media/test-drivers/vimc/vimc-debayer.c    | 197 ++++++++----------
 drivers/media/test-drivers/vimc/vimc-lens.c   |   5 +-
 drivers/media/test-drivers/vimc/vimc-scaler.c | 134 +++++-------
 drivers/media/test-drivers/vimc/vimc-sensor.c | 125 +++++------
 8 files changed, 236 insertions(+), 268 deletions(-)


base-commit: e42a204f0519a2540f1507ac2798be2aeaa76bee
-- 
Regards,

Laurent Pinchart

[PATCH v1 1/9] media: vimc: Don't iterate over single pad

[Laurent Pinchart]

The .init_state() operations of the debayer and sensor entities iterate
over the entity's pads. In practice, the iteration covers a single pad
only. Access the pad directly and remove the loops.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/test-drivers/vimc/vimc-debayer.c |  9 +++------
 drivers/media/test-drivers/vimc/vimc-sensor.c  | 10 +++-------
 2 files changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/media/test-drivers/vimc/vimc-debayer.c b/drivers/media/test-drivers/vimc/vimc-debayer.c
index d72ed086e00b..e1bf6db73050 100644
--- a/drivers/media/test-drivers/vimc/vimc-debayer.c
+++ b/drivers/media/test-drivers/vimc/vimc-debayer.c
@@ -155,16 +155,13 @@ static int vimc_debayer_init_state(struct v4l2_subdev *sd,
 {
 	struct vimc_debayer_device *vdebayer = v4l2_get_subdevdata(sd);
 	struct v4l2_mbus_framefmt *mf;
-	unsigned int i;
 
 	mf = v4l2_subdev_state_get_format(sd_state, 0);
 	*mf = sink_fmt_default;
 
-	for (i = 1; i < sd->entity.num_pads; i++) {
-		mf = v4l2_subdev_state_get_format(sd_state, i);
-		*mf = sink_fmt_default;
-		mf->code = vdebayer->src_code;
-	}
+	mf = v4l2_subdev_state_get_format(sd_state, 1);
+	*mf = sink_fmt_default;
+	mf->code = vdebayer->src_code;
 
 	return 0;
 }
diff --git a/drivers/media/test-drivers/vimc/vimc-sensor.c b/drivers/media/test-drivers/vimc/vimc-sensor.c
index 5e34b1aed95e..b535b3ffecff 100644
--- a/drivers/media/test-drivers/vimc/vimc-sensor.c
+++ b/drivers/media/test-drivers/vimc/vimc-sensor.c
@@ -44,14 +44,10 @@ static const struct v4l2_mbus_framefmt fmt_default = {
 static int vimc_sensor_init_state(struct v4l2_subdev *sd,
 				  struct v4l2_subdev_state *sd_state)
 {
-	unsigned int i;
+	struct v4l2_mbus_framefmt *mf;
 
-	for (i = 0; i < sd->entity.num_pads; i++) {
-		struct v4l2_mbus_framefmt *mf;
-
-		mf = v4l2_subdev_state_get_format(sd_state, i);
-		*mf = fmt_default;
-	}
+	mf = v4l2_subdev_state_get_format(sd_state, 0);
+	*mf = fmt_default;
 
 	return 0;
 }
-- 
Regards,

Laurent Pinchart

[PATCH v1 2/9] media: vimc: Constify vimc_ent_type structures

[Laurent Pinchart]

The vimc_ent_type structure contains static pointers to functions, and
no other information that need to be modified after initialization. Make
them const to avoid the risk of arbitrary code execution following an
overflow that would overwrite the structure's contents.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/test-drivers/vimc/vimc-capture.c |  2 +-
 drivers/media/test-drivers/vimc/vimc-common.h  | 12 ++++++------
 drivers/media/test-drivers/vimc/vimc-debayer.c |  2 +-
 drivers/media/test-drivers/vimc/vimc-lens.c    |  2 +-
 drivers/media/test-drivers/vimc/vimc-scaler.c  |  2 +-
 drivers/media/test-drivers/vimc/vimc-sensor.c  |  2 +-
 6 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c b/drivers/media/test-drivers/vimc/vimc-capture.c
index ba7550b8ba7e..89506ae00901 100644
--- a/drivers/media/test-drivers/vimc/vimc-capture.c
+++ b/drivers/media/test-drivers/vimc/vimc-capture.c
@@ -494,7 +494,7 @@ static struct vimc_ent_device *vimc_capture_add(struct vimc_device *vimc,
 	return ERR_PTR(ret);
 }
 
-struct vimc_ent_type vimc_capture_type = {
+const struct vimc_ent_type vimc_capture_type = {
 	.add = vimc_capture_add,
 	.unregister = vimc_capture_unregister,
 	.release = vimc_capture_release
diff --git a/drivers/media/test-drivers/vimc/vimc-common.h b/drivers/media/test-drivers/vimc/vimc-common.h
index 7641a101a728..6a76717e0384 100644
--- a/drivers/media/test-drivers/vimc/vimc-common.h
+++ b/drivers/media/test-drivers/vimc/vimc-common.h
@@ -156,7 +156,7 @@ struct vimc_ent_type {
  */
 struct vimc_ent_config {
 	const char *name;
-	struct vimc_ent_type *type;
+	const struct vimc_ent_type *type;
 };
 
 /**
@@ -167,11 +167,11 @@ struct vimc_ent_config {
  */
 bool vimc_is_source(struct media_entity *ent);
 
-extern struct vimc_ent_type vimc_sensor_type;
-extern struct vimc_ent_type vimc_debayer_type;
-extern struct vimc_ent_type vimc_scaler_type;
-extern struct vimc_ent_type vimc_capture_type;
-extern struct vimc_ent_type vimc_lens_type;
+extern const struct vimc_ent_type vimc_sensor_type;
+extern const struct vimc_ent_type vimc_debayer_type;
+extern const struct vimc_ent_type vimc_scaler_type;
+extern const struct vimc_ent_type vimc_capture_type;
+extern const struct vimc_ent_type vimc_lens_type;
 
 /**
  * vimc_pix_map_by_index - get vimc_pix_map struct by its index
diff --git a/drivers/media/test-drivers/vimc/vimc-debayer.c b/drivers/media/test-drivers/vimc/vimc-debayer.c
index e1bf6db73050..e2f12a7fb58f 100644
--- a/drivers/media/test-drivers/vimc/vimc-debayer.c
+++ b/drivers/media/test-drivers/vimc/vimc-debayer.c
@@ -622,7 +622,7 @@ static struct vimc_ent_device *vimc_debayer_add(struct vimc_device *vimc,
 	return ERR_PTR(ret);
 }
 
-struct vimc_ent_type vimc_debayer_type = {
+const struct vimc_ent_type vimc_debayer_type = {
 	.add = vimc_debayer_add,
 	.release = vimc_debayer_release
 };
diff --git a/drivers/media/test-drivers/vimc/vimc-lens.c b/drivers/media/test-drivers/vimc/vimc-lens.c
index 3ce7f4b4d2cc..e7d78fa8ccdb 100644
--- a/drivers/media/test-drivers/vimc/vimc-lens.c
+++ b/drivers/media/test-drivers/vimc/vimc-lens.c
@@ -96,7 +96,7 @@ static void vimc_lens_release(struct vimc_ent_device *ved)
 	kfree(vlens);
 }
 
-struct vimc_ent_type vimc_lens_type = {
+const struct vimc_ent_type vimc_lens_type = {
 	.add = vimc_lens_add,
 	.release = vimc_lens_release
 };
diff --git a/drivers/media/test-drivers/vimc/vimc-scaler.c b/drivers/media/test-drivers/vimc/vimc-scaler.c
index afe13d6af321..3e32cfb79c64 100644
--- a/drivers/media/test-drivers/vimc/vimc-scaler.c
+++ b/drivers/media/test-drivers/vimc/vimc-scaler.c
@@ -442,7 +442,7 @@ static struct vimc_ent_device *vimc_scaler_add(struct vimc_device *vimc,
 	return &vscaler->ved;
 }
 
-struct vimc_ent_type vimc_scaler_type = {
+const struct vimc_ent_type vimc_scaler_type = {
 	.add = vimc_scaler_add,
 	.release = vimc_scaler_release
 };
diff --git a/drivers/media/test-drivers/vimc/vimc-sensor.c b/drivers/media/test-drivers/vimc/vimc-sensor.c
index b535b3ffecff..11df18332865 100644
--- a/drivers/media/test-drivers/vimc/vimc-sensor.c
+++ b/drivers/media/test-drivers/vimc/vimc-sensor.c
@@ -448,7 +448,7 @@ static struct vimc_ent_device *vimc_sensor_add(struct vimc_device *vimc,
 	return ERR_PTR(ret);
 }
 
-struct vimc_ent_type vimc_sensor_type = {
+const struct vimc_ent_type vimc_sensor_type = {
 	.add = vimc_sensor_add,
 	.release = vimc_sensor_release
 };
-- 
Regards,

Laurent Pinchart

[PATCH v1 3/9] media: vimc: Constify the ent_config array

[Laurent Pinchart]

The ent_config array contains data that is never modified. Make it
const.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/test-drivers/vimc/vimc-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/test-drivers/vimc/vimc-core.c b/drivers/media/test-drivers/vimc/vimc-core.c
index af127476e920..2083c60e34d6 100644
--- a/drivers/media/test-drivers/vimc/vimc-core.c
+++ b/drivers/media/test-drivers/vimc/vimc-core.c
@@ -81,7 +81,7 @@ struct vimc_pipeline_config {
  * Topology Configuration
  */
 
-static struct vimc_ent_config ent_config[] = {
+static const struct vimc_ent_config ent_config[] = {
 	[SENSOR_A] = {
 		.name = "Sensor A",
 		.type = &vimc_sensor_type
-- 
Regards,

Laurent Pinchart

[PATCH v1 4/9] media: vimc: scaler: Rename vic_sca_pad enum to vimc_scaler_pad

[Laurent Pinchart]

The vic_sca_pad enum's name has been shortened to the extreme for no
good reason. Rename it to vimc_scaler_pad.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/test-drivers/vimc/vimc-scaler.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/test-drivers/vimc/vimc-scaler.c b/drivers/media/test-drivers/vimc/vimc-scaler.c
index 3e32cfb79c64..013cd84f82be 100644
--- a/drivers/media/test-drivers/vimc/vimc-scaler.c
+++ b/drivers/media/test-drivers/vimc/vimc-scaler.c
@@ -15,7 +15,7 @@
 #include "vimc-common.h"
 
 /* Pad identifier */
-enum vic_sca_pad {
+enum vimc_scaler_pad {
 	VIMC_SCALER_SINK = 0,
 	VIMC_SCALER_SRC = 1,
 };
-- 
Regards,

Laurent Pinchart

[PATCH v1 5/9] media: vimc: Centralize subdev internal_ops initialization

[Laurent Pinchart]

Initialize the subdev internal_ops field in the vimc_ent_sd_register()
function. This handles the internal ops the same way as the subdev ops,
and prepares for moving to the V4L2 subdev active state API.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/test-drivers/vimc/vimc-common.c  | 2 ++
 drivers/media/test-drivers/vimc/vimc-common.h  | 2 ++
 drivers/media/test-drivers/vimc/vimc-debayer.c | 5 ++---
 drivers/media/test-drivers/vimc/vimc-lens.c    | 2 +-
 drivers/media/test-drivers/vimc/vimc-scaler.c  | 5 ++---
 drivers/media/test-drivers/vimc/vimc-sensor.c  | 4 +---
 6 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/media/test-drivers/vimc/vimc-common.c b/drivers/media/test-drivers/vimc/vimc-common.c
index 2e72974e35b4..3da2271215c6 100644
--- a/drivers/media/test-drivers/vimc/vimc-common.c
+++ b/drivers/media/test-drivers/vimc/vimc-common.c
@@ -358,6 +358,7 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
 			 u32 function,
 			 u16 num_pads,
 			 struct media_pad *pads,
+			 const struct v4l2_subdev_internal_ops *int_ops,
 			 const struct v4l2_subdev_ops *sd_ops)
 {
 	int ret;
@@ -367,6 +368,7 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
 
 	/* Initialize the subdev */
 	v4l2_subdev_init(sd, sd_ops);
+	sd->internal_ops = int_ops;
 	sd->entity.function = function;
 	sd->entity.ops = &vimc_ent_sd_mops;
 	sd->owner = THIS_MODULE;
diff --git a/drivers/media/test-drivers/vimc/vimc-common.h b/drivers/media/test-drivers/vimc/vimc-common.h
index 6a76717e0384..7a45a2117748 100644
--- a/drivers/media/test-drivers/vimc/vimc-common.h
+++ b/drivers/media/test-drivers/vimc/vimc-common.h
@@ -215,6 +215,7 @@ const struct vimc_pix_map *vimc_pix_map_by_pixelformat(u32 pixelformat);
  * @num_pads:	number of pads to initialize
  * @pads:	the array of pads of the entity, the caller should set the
  *		flags of the pads
+ * @int_ops:	pointer to &struct v4l2_subdev_internal_ops.
  * @sd_ops:	pointer to &struct v4l2_subdev_ops.
  *
  * Helper function initialize and register the struct vimc_ent_device and struct
@@ -227,6 +228,7 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
 			 u32 function,
 			 u16 num_pads,
 			 struct media_pad *pads,
+			 const struct v4l2_subdev_internal_ops *int_ops,
 			 const struct v4l2_subdev_ops *sd_ops);
 
 /**
diff --git a/drivers/media/test-drivers/vimc/vimc-debayer.c b/drivers/media/test-drivers/vimc/vimc-debayer.c
index e2f12a7fb58f..d4ca57b3672d 100644
--- a/drivers/media/test-drivers/vimc/vimc-debayer.c
+++ b/drivers/media/test-drivers/vimc/vimc-debayer.c
@@ -591,12 +591,11 @@ static struct vimc_ent_device *vimc_debayer_add(struct vimc_device *vimc,
 	ret = vimc_ent_sd_register(&vdebayer->ved, &vdebayer->sd, v4l2_dev,
 				   vcfg_name,
 				   MEDIA_ENT_F_PROC_VIDEO_PIXEL_ENC_CONV, 2,
-				   vdebayer->pads, &vimc_debayer_ops);
+				   vdebayer->pads, &vimc_debayer_internal_ops,
+				   &vimc_debayer_ops);
 	if (ret)
 		goto err_free_hdl;
 
-	vdebayer->sd.internal_ops = &vimc_debayer_internal_ops;
-
 	vdebayer->ved.process_frame = vimc_debayer_process_frame;
 	vdebayer->ved.dev = vimc->mdev.dev;
 	vdebayer->mean_win_size = vimc_debayer_ctrl_mean_win_size.def;
diff --git a/drivers/media/test-drivers/vimc/vimc-lens.c b/drivers/media/test-drivers/vimc/vimc-lens.c
index e7d78fa8ccdb..42c58a3e3f1b 100644
--- a/drivers/media/test-drivers/vimc/vimc-lens.c
+++ b/drivers/media/test-drivers/vimc/vimc-lens.c
@@ -72,7 +72,7 @@ static struct vimc_ent_device *vimc_lens_add(struct vimc_device *vimc,
 
 	ret = vimc_ent_sd_register(&vlens->ved, &vlens->sd, v4l2_dev,
 				   vcfg_name, MEDIA_ENT_F_LENS, 0,
-				   NULL, &vimc_lens_ops);
+				   NULL, NULL, &vimc_lens_ops);
 	if (ret)
 		goto err_free_hdl;
 
diff --git a/drivers/media/test-drivers/vimc/vimc-scaler.c b/drivers/media/test-drivers/vimc/vimc-scaler.c
index 013cd84f82be..4f9c44a663e1 100644
--- a/drivers/media/test-drivers/vimc/vimc-scaler.c
+++ b/drivers/media/test-drivers/vimc/vimc-scaler.c
@@ -421,14 +421,13 @@ static struct vimc_ent_device *vimc_scaler_add(struct vimc_device *vimc,
 	ret = vimc_ent_sd_register(&vscaler->ved, &vscaler->sd, v4l2_dev,
 				   vcfg_name,
 				   MEDIA_ENT_F_PROC_VIDEO_SCALER, 2,
-				   vscaler->pads, &vimc_scaler_ops);
+				   vscaler->pads, &vimc_scaler_internal_ops,
+				   &vimc_scaler_ops);
 	if (ret) {
 		kfree(vscaler);
 		return ERR_PTR(ret);
 	}
 
-	vscaler->sd.internal_ops = &vimc_scaler_internal_ops;
-
 	vscaler->ved.process_frame = vimc_scaler_process_frame;
 	vscaler->ved.dev = vimc->mdev.dev;
 
diff --git a/drivers/media/test-drivers/vimc/vimc-sensor.c b/drivers/media/test-drivers/vimc/vimc-sensor.c
index 11df18332865..5c31d9435cdd 100644
--- a/drivers/media/test-drivers/vimc/vimc-sensor.c
+++ b/drivers/media/test-drivers/vimc/vimc-sensor.c
@@ -424,12 +424,10 @@ static struct vimc_ent_device *vimc_sensor_add(struct vimc_device *vimc,
 	ret = vimc_ent_sd_register(&vsensor->ved, &vsensor->sd, v4l2_dev,
 				   vcfg_name,
 				   MEDIA_ENT_F_CAM_SENSOR, 1, &vsensor->pad,
-				   &vimc_sensor_ops);
+				   &vimc_sensor_internal_ops, &vimc_sensor_ops);
 	if (ret)
 		goto err_free_tpg;
 
-	vsensor->sd.internal_ops = &vimc_sensor_internal_ops;
-
 	vsensor->ved.process_frame = vimc_sensor_process_frame;
 	vsensor->ved.dev = vimc->mdev.dev;
 
-- 
Regards,

Laurent Pinchart

[PATCH v1 6/9] media: vimc: Initialize subdev active state

[Laurent Pinchart]

Finalize subdev initialization for all subdevs that provide a
.init_state() operation. This creates an active state for all those
subdevs, which subsequent patches will use to simplify the
implementation of individual vimc entities.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/test-drivers/vimc/vimc-common.c | 23 ++++++++++++++++++-
 .../media/test-drivers/vimc/vimc-debayer.c    |  1 +
 drivers/media/test-drivers/vimc/vimc-lens.c   |  1 +
 drivers/media/test-drivers/vimc/vimc-scaler.c |  1 +
 drivers/media/test-drivers/vimc/vimc-sensor.c |  1 +
 5 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/media/test-drivers/vimc/vimc-common.c b/drivers/media/test-drivers/vimc/vimc-common.c
index 3da2271215c6..4f4fcb26e236 100644
--- a/drivers/media/test-drivers/vimc/vimc-common.c
+++ b/drivers/media/test-drivers/vimc/vimc-common.c
@@ -8,6 +8,8 @@
 #include <linux/init.h>
 #include <linux/module.h>
 
+#include <media/v4l2-ctrls.h>
+
 #include "vimc-common.h"
 
 /*
@@ -385,17 +387,36 @@ int vimc_ent_sd_register(struct vimc_ent_device *ved,
 	if (ret)
 		return ret;
 
+	/*
+	 * Finalize the subdev initialization if it supports active states. Use
+	 * the control handler lock as the state lock if available.
+	 */
+	if (int_ops && int_ops->init_state) {
+		if (sd->ctrl_handler)
+			sd->state_lock = sd->ctrl_handler->lock;
+
+		ret = v4l2_subdev_init_finalize(sd);
+		if (ret) {
+			dev_err(v4l2_dev->dev,
+				"%s: subdev initialization failed (err=%d)\n",
+				name, ret);
+			goto err_clean_m_ent;
+		}
+	}
+
 	/* Register the subdev with the v4l2 and the media framework */
 	ret = v4l2_device_register_subdev(v4l2_dev, sd);
 	if (ret) {
 		dev_err(v4l2_dev->dev,
 			"%s: subdev register failed (err=%d)\n",
 			name, ret);
-		goto err_clean_m_ent;
+		goto err_clean_sd;
 	}
 
 	return 0;
 
+err_clean_sd:
+	v4l2_subdev_cleanup(sd);
 err_clean_m_ent:
 	media_entity_cleanup(&sd->entity);
 	return ret;
diff --git a/drivers/media/test-drivers/vimc/vimc-debayer.c b/drivers/media/test-drivers/vimc/vimc-debayer.c
index d4ca57b3672d..9f8a31fb0695 100644
--- a/drivers/media/test-drivers/vimc/vimc-debayer.c
+++ b/drivers/media/test-drivers/vimc/vimc-debayer.c
@@ -540,6 +540,7 @@ static void vimc_debayer_release(struct vimc_ent_device *ved)
 		container_of(ved, struct vimc_debayer_device, ved);
 
 	v4l2_ctrl_handler_free(&vdebayer->hdl);
+	v4l2_subdev_cleanup(&vdebayer->sd);
 	media_entity_cleanup(vdebayer->ved.ent);
 	kfree(vdebayer);
 }
diff --git a/drivers/media/test-drivers/vimc/vimc-lens.c b/drivers/media/test-drivers/vimc/vimc-lens.c
index 42c58a3e3f1b..96399057a2b5 100644
--- a/drivers/media/test-drivers/vimc/vimc-lens.c
+++ b/drivers/media/test-drivers/vimc/vimc-lens.c
@@ -92,6 +92,7 @@ static void vimc_lens_release(struct vimc_ent_device *ved)
 		container_of(ved, struct vimc_lens_device, ved);
 
 	v4l2_ctrl_handler_free(&vlens->hdl);
+	v4l2_subdev_cleanup(&vlens->sd);
 	media_entity_cleanup(vlens->ved.ent);
 	kfree(vlens);
 }
diff --git a/drivers/media/test-drivers/vimc/vimc-scaler.c b/drivers/media/test-drivers/vimc/vimc-scaler.c
index 4f9c44a663e1..f8639f5b4d0c 100644
--- a/drivers/media/test-drivers/vimc/vimc-scaler.c
+++ b/drivers/media/test-drivers/vimc/vimc-scaler.c
@@ -398,6 +398,7 @@ static void vimc_scaler_release(struct vimc_ent_device *ved)
 	struct vimc_scaler_device *vscaler =
 		container_of(ved, struct vimc_scaler_device, ved);
 
+	v4l2_subdev_cleanup(&vscaler->sd);
 	media_entity_cleanup(vscaler->ved.ent);
 	kfree(vscaler);
 }
diff --git a/drivers/media/test-drivers/vimc/vimc-sensor.c b/drivers/media/test-drivers/vimc/vimc-sensor.c
index 5c31d9435cdd..9eb24c4791bf 100644
--- a/drivers/media/test-drivers/vimc/vimc-sensor.c
+++ b/drivers/media/test-drivers/vimc/vimc-sensor.c
@@ -340,6 +340,7 @@ static void vimc_sensor_release(struct vimc_ent_device *ved)
 
 	v4l2_ctrl_handler_free(&vsensor->hdl);
 	tpg_free(&vsensor->tpg);
+	v4l2_subdev_cleanup(&vsensor->sd);
 	media_entity_cleanup(vsensor->ved.ent);
 	kfree(vsensor);
 }
-- 
Regards,

Laurent Pinchart

[PATCH v1 7/9] media: vimc: sensor: Use subdev active state

[Laurent Pinchart]

Store the active formats and crop rectangle in the subdevice active
state. This simplifies implementation of the format and selection
accessors, and allows using the v4l2_subdev_get_fmt() helper to
implement the .get_fmt() operation.

The active configuration that is used in the .process_frame() handler is
still stored in the vimc_sensor_device structure. The driver could
instead access the active state in the .process_frame() handler, but the
required locking could interfere with the real time constraints of the
frame processing. This data would be stored in registers in the
.s_stream() handler for real hardware, storing it in dedicated storage
thus mimics a real driver. To differentiate them from the rest of the
device private data, move the corresponding fields to a sub-structure of
vimc_sensor_device named hw.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/test-drivers/vimc/vimc-sensor.c | 108 ++++++++----------
 1 file changed, 50 insertions(+), 58 deletions(-)

diff --git a/drivers/media/test-drivers/vimc/vimc-sensor.c b/drivers/media/test-drivers/vimc/vimc-sensor.c
index 9eb24c4791bf..027767777763 100644
--- a/drivers/media/test-drivers/vimc/vimc-sensor.c
+++ b/drivers/media/test-drivers/vimc/vimc-sensor.c
@@ -24,13 +24,20 @@ struct vimc_sensor_device {
 	struct vimc_ent_device ved;
 	struct v4l2_subdev sd;
 	struct tpg_data tpg;
-	u8 *frame;
-	enum vimc_sensor_osd_mode osd_value;
-	u64 start_stream_ts;
-	/* The active format */
-	struct v4l2_mbus_framefmt mbus_format;
 	struct v4l2_ctrl_handler hdl;
 	struct media_pad pad;
+
+	u8 *frame;
+
+	/*
+	 * Virtual "hardware" configuration, filled when the stream starts or
+	 * when controls are set.
+	 */
+	struct {
+		struct v4l2_area size;
+		enum vimc_sensor_osd_mode osd_value;
+		u64 start_stream_ts;
+	} hw;
 };
 
 static const struct v4l2_mbus_framefmt fmt_default = {
@@ -88,36 +95,22 @@ static int vimc_sensor_enum_frame_size(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static int vimc_sensor_get_fmt(struct v4l2_subdev *sd,
-			       struct v4l2_subdev_state *sd_state,
-			       struct v4l2_subdev_format *fmt)
+static void vimc_sensor_tpg_s_format(struct vimc_sensor_device *vsensor,
+				     const struct v4l2_mbus_framefmt *format)
 {
-	struct vimc_sensor_device *vsensor =
-				container_of(sd, struct vimc_sensor_device, sd);
+	const struct vimc_pix_map *vpix = vimc_pix_map_by_code(format->code);
 
-	fmt->format = fmt->which == V4L2_SUBDEV_FORMAT_TRY ?
-		      *v4l2_subdev_state_get_format(sd_state, fmt->pad) :
-		      vsensor->mbus_format;
-
-	return 0;
-}
-
-static void vimc_sensor_tpg_s_format(struct vimc_sensor_device *vsensor)
-{
-	const struct vimc_pix_map *vpix =
-				vimc_pix_map_by_code(vsensor->mbus_format.code);
-
-	tpg_reset_source(&vsensor->tpg, vsensor->mbus_format.width,
-			 vsensor->mbus_format.height, vsensor->mbus_format.field);
-	tpg_s_bytesperline(&vsensor->tpg, 0, vsensor->mbus_format.width * vpix->bpp);
-	tpg_s_buf_height(&vsensor->tpg, vsensor->mbus_format.height);
+	tpg_reset_source(&vsensor->tpg, format->width, format->height,
+			 format->field);
+	tpg_s_bytesperline(&vsensor->tpg, 0, format->width * vpix->bpp);
+	tpg_s_buf_height(&vsensor->tpg, format->height);
 	tpg_s_fourcc(&vsensor->tpg, vpix->pixelformat);
 	/* TODO: add support for V4L2_FIELD_ALTERNATE */
-	tpg_s_field(&vsensor->tpg, vsensor->mbus_format.field, false);
-	tpg_s_colorspace(&vsensor->tpg, vsensor->mbus_format.colorspace);
-	tpg_s_ycbcr_enc(&vsensor->tpg, vsensor->mbus_format.ycbcr_enc);
-	tpg_s_quantization(&vsensor->tpg, vsensor->mbus_format.quantization);
-	tpg_s_xfer_func(&vsensor->tpg, vsensor->mbus_format.xfer_func);
+	tpg_s_field(&vsensor->tpg, format->field, false);
+	tpg_s_colorspace(&vsensor->tpg, format->colorspace);
+	tpg_s_ycbcr_enc(&vsensor->tpg, format->ycbcr_enc);
+	tpg_s_quantization(&vsensor->tpg, format->quantization);
+	tpg_s_xfer_func(&vsensor->tpg, format->xfer_func);
 }
 
 static void vimc_sensor_adjust_fmt(struct v4l2_mbus_framefmt *fmt)
@@ -148,15 +141,11 @@ static int vimc_sensor_set_fmt(struct v4l2_subdev *sd,
 	struct vimc_sensor_device *vsensor = v4l2_get_subdevdata(sd);
 	struct v4l2_mbus_framefmt *mf;
 
-	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
-		/* Do not change the format while stream is on */
-		if (vsensor->frame)
-			return -EBUSY;
+	/* Do not change the format while stream is on */
+	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE && vsensor->frame)
+		return -EBUSY;
 
-		mf = &vsensor->mbus_format;
-	} else {
-		mf = v4l2_subdev_state_get_format(sd_state, fmt->pad);
-	}
+	mf = v4l2_subdev_state_get_format(sd_state, fmt->pad);
 
 	/* Set the new format */
 	vimc_sensor_adjust_fmt(&fmt->format);
@@ -181,7 +170,7 @@ static int vimc_sensor_set_fmt(struct v4l2_subdev *sd,
 static const struct v4l2_subdev_pad_ops vimc_sensor_pad_ops = {
 	.enum_mbus_code		= vimc_sensor_enum_mbus_code,
 	.enum_frame_size	= vimc_sensor_enum_frame_size,
-	.get_fmt		= vimc_sensor_get_fmt,
+	.get_fmt		= v4l2_subdev_get_fmt,
 	.set_fmt		= vimc_sensor_set_fmt,
 };
 
@@ -198,7 +187,7 @@ static void *vimc_sensor_process_frame(struct vimc_ent_device *ved,
 
 	tpg_fill_plane_buffer(&vsensor->tpg, 0, 0, vsensor->frame);
 	tpg_calc_text_basep(&vsensor->tpg, basep, 0, vsensor->frame);
-	switch (vsensor->osd_value) {
+	switch (vsensor->hw.osd_value) {
 	case VIMC_SENSOR_OSD_SHOW_ALL: {
 		const char *order = tpg_g_color_order(&vsensor->tpg);
 
@@ -212,15 +201,14 @@ static void *vimc_sensor_process_frame(struct vimc_ent_device *ved,
 			 vsensor->tpg.hue);
 		tpg_gen_text(&vsensor->tpg, basep, line++ * line_height, 16, str);
 		snprintf(str, sizeof(str), "sensor size: %dx%d",
-			 vsensor->mbus_format.width,
-			 vsensor->mbus_format.height);
+			 vsensor->hw.size.width, vsensor->hw.size.height);
 		tpg_gen_text(&vsensor->tpg, basep, line++ * line_height, 16, str);
 		fallthrough;
 	}
 	case VIMC_SENSOR_OSD_SHOW_COUNTERS: {
 		unsigned int ms;
 
-		ms = div_u64(ktime_get_ns() - vsensor->start_stream_ts, 1000000);
+		ms = div_u64(ktime_get_ns() - vsensor->hw.start_stream_ts, 1000000);
 		snprintf(str, sizeof(str), "%02d:%02d:%02d:%03d",
 			 (ms / (60 * 60 * 1000)) % 24,
 			 (ms / (60 * 1000)) % 60,
@@ -243,15 +231,25 @@ static int vimc_sensor_s_stream(struct v4l2_subdev *sd, int enable)
 				container_of(sd, struct vimc_sensor_device, sd);
 
 	if (enable) {
+		const struct v4l2_mbus_framefmt *format;
+		struct v4l2_subdev_state *state;
 		const struct vimc_pix_map *vpix;
 		unsigned int frame_size;
 
-		vsensor->start_stream_ts = ktime_get_ns();
+		state = v4l2_subdev_lock_and_get_active_state(sd);
+		format = v4l2_subdev_state_get_format(state, 0);
 
-		/* Calculate the frame size */
-		vpix = vimc_pix_map_by_code(vsensor->mbus_format.code);
-		frame_size = vsensor->mbus_format.width * vpix->bpp *
-			     vsensor->mbus_format.height;
+		/* Configure the test pattern generator. */
+		vimc_sensor_tpg_s_format(vsensor, format);
+
+		/* Calculate the frame size. */
+		vpix = vimc_pix_map_by_code(format->code);
+		frame_size = format->width * vpix->bpp * format->height;
+
+		vsensor->hw.size.width = format->width;
+		vsensor->hw.size.height = format->height;
+
+		v4l2_subdev_unlock_state(state);
 
 		/*
 		 * Allocate the frame buffer. Use vmalloc to be able to
@@ -261,9 +259,7 @@ static int vimc_sensor_s_stream(struct v4l2_subdev *sd, int enable)
 		if (!vsensor->frame)
 			return -ENOMEM;
 
-		/* configure the test pattern generator */
-		vimc_sensor_tpg_s_format(vsensor);
-
+		vsensor->hw.start_stream_ts = ktime_get_ns();
 	} else {
 
 		vfree(vsensor->frame);
@@ -321,7 +317,7 @@ static int vimc_sensor_s_ctrl(struct v4l2_ctrl *ctrl)
 		tpg_s_saturation(&vsensor->tpg, ctrl->val);
 		break;
 	case VIMC_CID_OSD_TEXT_MODE:
-		vsensor->osd_value = ctrl->val;
+		vsensor->hw.osd_value = ctrl->val;
 		break;
 	default:
 		return -EINVAL;
@@ -414,8 +410,7 @@ static struct vimc_ent_device *vimc_sensor_add(struct vimc_device *vimc,
 	}
 
 	/* Initialize the test pattern generator */
-	tpg_init(&vsensor->tpg, vsensor->mbus_format.width,
-		 vsensor->mbus_format.height);
+	tpg_init(&vsensor->tpg, fmt_default.width, fmt_default.height);
 	ret = tpg_alloc(&vsensor->tpg, VIMC_FRAME_MAX_WIDTH);
 	if (ret)
 		goto err_free_hdl;
@@ -432,9 +427,6 @@ static struct vimc_ent_device *vimc_sensor_add(struct vimc_device *vimc,
 	vsensor->ved.process_frame = vimc_sensor_process_frame;
 	vsensor->ved.dev = vimc->mdev.dev;
 
-	/* Initialize the frame format */
-	vsensor->mbus_format = fmt_default;
-
 	return &vsensor->ved;
 
 err_free_tpg:
-- 
Regards,

Laurent Pinchart

[PATCH v1 8/9] media: vimc: debayer: Use subdev active state

[Laurent Pinchart]

Store the active formats and crop rectangle in the subdevice active
state. This simplifies implementation of the format and selection
accessors, and allows using the v4l2_subdev_get_fmt() helper to
implement the .get_fmt() operation.

The active configuration that is used in the .process_frame() handler is
still stored in the vimc_debayer_device structure. The driver could
instead access the active state in the .process_frame() handler, but the
required locking could interfere with the real time constraints of the
frame processing. This data would be stored in registers in the
.s_stream() handler for real hardware, storing it in dedicated storage
thus mimics a real driver. To differentiate them from the rest of the
device private data, move the corresponding fields to a sub-structure of
vimc_debayer_device named hw.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../media/test-drivers/vimc/vimc-debayer.c    | 182 ++++++++----------
 1 file changed, 83 insertions(+), 99 deletions(-)

diff --git a/drivers/media/test-drivers/vimc/vimc-debayer.c b/drivers/media/test-drivers/vimc/vimc-debayer.c
index 9f8a31fb0695..bbb7c7a86df0 100644
--- a/drivers/media/test-drivers/vimc/vimc-debayer.c
+++ b/drivers/media/test-drivers/vimc/vimc-debayer.c
@@ -15,6 +15,9 @@
 
 #include "vimc-common.h"
 
+/* TODO: Add support for more output formats, we only support RGB888 for now. */
+#define VIMC_DEBAYER_SOURCE_MBUS_FMT	MEDIA_BUS_FMT_RGB888_1X24
+
 enum vimc_debayer_rgb_colors {
 	VIMC_DEBAYER_RED = 0,
 	VIMC_DEBAYER_GREEN = 1,
@@ -29,19 +32,26 @@ struct vimc_debayer_pix_map {
 struct vimc_debayer_device {
 	struct vimc_ent_device ved;
 	struct v4l2_subdev sd;
-	/* The active format */
-	struct v4l2_mbus_framefmt sink_fmt;
-	u32 src_code;
+	struct v4l2_ctrl_handler hdl;
+	struct media_pad pads[2];
+
+	u8 *src_frame;
+
 	void (*set_rgb_src)(struct vimc_debayer_device *vdebayer,
 			    unsigned int lin, unsigned int col,
 			    unsigned int rgb[3]);
-	/* Values calculated when the stream starts */
-	u8 *src_frame;
-	const struct vimc_debayer_pix_map *sink_pix_map;
-	unsigned int sink_bpp;
-	unsigned int mean_win_size;
-	struct v4l2_ctrl_handler hdl;
-	struct media_pad pads[2];
+
+	/*
+	 * Virtual "hardware" configuration, filled when the stream starts or
+	 * when controls are set.
+	 */
+	struct {
+		const struct vimc_debayer_pix_map *sink_pix_map;
+		unsigned int sink_bpp;
+		struct v4l2_area size;
+		unsigned int mean_win_size;
+		u32 src_code;
+	} hw;
 };
 
 static const struct v4l2_mbus_framefmt sink_fmt_default = {
@@ -153,7 +163,6 @@ static bool vimc_debayer_src_code_is_valid(u32 code)
 static int vimc_debayer_init_state(struct v4l2_subdev *sd,
 				   struct v4l2_subdev_state *sd_state)
 {
-	struct vimc_debayer_device *vdebayer = v4l2_get_subdevdata(sd);
 	struct v4l2_mbus_framefmt *mf;
 
 	mf = v4l2_subdev_state_get_format(sd_state, 0);
@@ -161,7 +170,7 @@ static int vimc_debayer_init_state(struct v4l2_subdev *sd,
 
 	mf = v4l2_subdev_state_get_format(sd_state, 1);
 	*mf = sink_fmt_default;
-	mf->code = vdebayer->src_code;
+	mf->code = VIMC_DEBAYER_SOURCE_MBUS_FMT;
 
 	return 0;
 }
@@ -210,24 +219,6 @@ static int vimc_debayer_enum_frame_size(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static int vimc_debayer_get_fmt(struct v4l2_subdev *sd,
-				struct v4l2_subdev_state *sd_state,
-				struct v4l2_subdev_format *fmt)
-{
-	struct vimc_debayer_device *vdebayer = v4l2_get_subdevdata(sd);
-
-	/* Get the current sink format */
-	fmt->format = fmt->which == V4L2_SUBDEV_FORMAT_TRY ?
-		      *v4l2_subdev_state_get_format(sd_state, 0) :
-		      vdebayer->sink_fmt;
-
-	/* Set the right code for the source pad */
-	if (VIMC_IS_SRC(fmt->pad))
-		fmt->format.code = vdebayer->src_code;
-
-	return 0;
-}
-
 static void vimc_debayer_adjust_sink_fmt(struct v4l2_mbus_framefmt *fmt)
 {
 	const struct vimc_debayer_pix_map *vpix;
@@ -253,52 +244,42 @@ static int vimc_debayer_set_fmt(struct v4l2_subdev *sd,
 				struct v4l2_subdev_format *fmt)
 {
 	struct vimc_debayer_device *vdebayer = v4l2_get_subdevdata(sd);
-	struct v4l2_mbus_framefmt *sink_fmt;
-	u32 *src_code;
+	struct v4l2_mbus_framefmt *format;
 
-	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
-		/* Do not change the format while stream is on */
-		if (vdebayer->src_frame)
-			return -EBUSY;
-
-		sink_fmt = &vdebayer->sink_fmt;
-		src_code = &vdebayer->src_code;
-	} else {
-		sink_fmt = v4l2_subdev_state_get_format(sd_state, 0);
-		src_code = &v4l2_subdev_state_get_format(sd_state, 1)->code;
-	}
+	/* Do not change the format while stream is on. */
+	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE && vdebayer->src_frame)
+		return -EBUSY;
 
 	/*
-	 * Do not change the format of the source pad,
-	 * it is propagated from the sink
+	 * Do not change the format of the source pad, it is propagated from
+	 * the sink.
 	 */
-	if (VIMC_IS_SRC(fmt->pad)) {
-		u32 code = fmt->format.code;
+	if (VIMC_IS_SRC(fmt->pad))
+		return v4l2_subdev_get_fmt(sd, sd_state, fmt);
 
-		fmt->format = *sink_fmt;
+	/* Set the new format in the sink pad. */
+	vimc_debayer_adjust_sink_fmt(&fmt->format);
 
-		if (vimc_debayer_src_code_is_valid(code))
-			*src_code = code;
+	format = v4l2_subdev_state_get_format(sd_state, 0);
 
-		fmt->format.code = *src_code;
-	} else {
-		/* Set the new format in the sink pad */
-		vimc_debayer_adjust_sink_fmt(&fmt->format);
+	dev_dbg(vdebayer->ved.dev, "%s: sink format update: "
+		"old:%dx%d (0x%x, %d, %d, %d, %d) "
+		"new:%dx%d (0x%x, %d, %d, %d, %d)\n", vdebayer->sd.name,
+		/* old */
+		format->width, format->height, format->code,
+		format->colorspace, format->quantization,
+		format->xfer_func, format->ycbcr_enc,
+		/* new */
+		fmt->format.width, fmt->format.height, fmt->format.code,
+		fmt->format.colorspace,	fmt->format.quantization,
+		fmt->format.xfer_func, fmt->format.ycbcr_enc);
 
-		dev_dbg(vdebayer->ved.dev, "%s: sink format update: "
-			"old:%dx%d (0x%x, %d, %d, %d, %d) "
-			"new:%dx%d (0x%x, %d, %d, %d, %d)\n", vdebayer->sd.name,
-			/* old */
-			sink_fmt->width, sink_fmt->height, sink_fmt->code,
-			sink_fmt->colorspace, sink_fmt->quantization,
-			sink_fmt->xfer_func, sink_fmt->ycbcr_enc,
-			/* new */
-			fmt->format.width, fmt->format.height, fmt->format.code,
-			fmt->format.colorspace,	fmt->format.quantization,
-			fmt->format.xfer_func, fmt->format.ycbcr_enc);
+	*format = fmt->format;
 
-		*sink_fmt = fmt->format;
-	}
+	/* Propagate the format to the source pad. */
+	format = v4l2_subdev_state_get_format(sd_state, 1);
+	*format = fmt->format;
+	format->code = VIMC_DEBAYER_SOURCE_MBUS_FMT;
 
 	return 0;
 }
@@ -306,7 +287,7 @@ static int vimc_debayer_set_fmt(struct v4l2_subdev *sd,
 static const struct v4l2_subdev_pad_ops vimc_debayer_pad_ops = {
 	.enum_mbus_code		= vimc_debayer_enum_mbus_code,
 	.enum_frame_size	= vimc_debayer_enum_frame_size,
-	.get_fmt		= vimc_debayer_get_fmt,
+	.get_fmt		= v4l2_subdev_get_fmt,
 	.set_fmt		= vimc_debayer_set_fmt,
 };
 
@@ -318,8 +299,8 @@ static void vimc_debayer_process_rgb_frame(struct vimc_debayer_device *vdebayer,
 	const struct vimc_pix_map *vpix;
 	unsigned int i, index;
 
-	vpix = vimc_pix_map_by_code(vdebayer->src_code);
-	index = VIMC_FRAME_INDEX(lin, col, vdebayer->sink_fmt.width, 3);
+	vpix = vimc_pix_map_by_code(vdebayer->hw.src_code);
+	index = VIMC_FRAME_INDEX(lin, col, vdebayer->hw.size.width, 3);
 	for (i = 0; i < 3; i++) {
 		switch (vpix->pixelformat) {
 		case V4L2_PIX_FMT_RGB24:
@@ -337,24 +318,37 @@ static int vimc_debayer_s_stream(struct v4l2_subdev *sd, int enable)
 	struct vimc_debayer_device *vdebayer = v4l2_get_subdevdata(sd);
 
 	if (enable) {
+		const struct v4l2_mbus_framefmt *sink_fmt;
+		const struct v4l2_mbus_framefmt *src_fmt;
+		struct v4l2_subdev_state *state;
 		const struct vimc_pix_map *vpix;
 		unsigned int frame_size;
 
 		if (vdebayer->src_frame)
 			return 0;
 
+		state = v4l2_subdev_lock_and_get_active_state(sd);
+		sink_fmt = v4l2_subdev_state_get_format(state, 0);
+		src_fmt = v4l2_subdev_state_get_format(state, 1);
+
 		/* Calculate the frame size of the source pad */
-		vpix = vimc_pix_map_by_code(vdebayer->src_code);
-		frame_size = vdebayer->sink_fmt.width * vdebayer->sink_fmt.height *
-				vpix->bpp;
+		vpix = vimc_pix_map_by_code(src_fmt->code);
+		frame_size = src_fmt->width * src_fmt->height * vpix->bpp;
 
 		/* Save the bytes per pixel of the sink */
-		vpix = vimc_pix_map_by_code(vdebayer->sink_fmt.code);
-		vdebayer->sink_bpp = vpix->bpp;
+		vpix = vimc_pix_map_by_code(sink_fmt->code);
+		vdebayer->hw.sink_bpp = vpix->bpp;
 
 		/* Get the corresponding pixel map from the table */
-		vdebayer->sink_pix_map =
-			vimc_debayer_pix_map_by_code(vdebayer->sink_fmt.code);
+		vdebayer->hw.sink_pix_map =
+			vimc_debayer_pix_map_by_code(sink_fmt->code);
+
+		vdebayer->hw.size.width = sink_fmt->width;
+		vdebayer->hw.size.height = sink_fmt->height;
+
+		vdebayer->hw.src_code = src_fmt->code;
+
+		v4l2_subdev_unlock_state(state);
 
 		/*
 		 * Allocate the frame buffer. Use vmalloc to be able to
@@ -363,7 +357,6 @@ static int vimc_debayer_s_stream(struct v4l2_subdev *sd, int enable)
 		vdebayer->src_frame = vmalloc(frame_size);
 		if (!vdebayer->src_frame)
 			return -ENOMEM;
-
 	} else {
 		if (!vdebayer->src_frame)
 			return 0;
@@ -424,13 +417,13 @@ static void vimc_debayer_calc_rgb_sink(struct vimc_debayer_device *vdebayer,
 	 * the top left corner of the mean window (considering the current
 	 * pixel as the center)
 	 */
-	seek = vdebayer->mean_win_size / 2;
+	seek = vdebayer->hw.mean_win_size / 2;
 
 	/* Sum the values of the colors in the mean window */
 
 	dev_dbg(vdebayer->ved.dev,
 		"deb: %s: --- Calc pixel %dx%d, window mean %d, seek %d ---\n",
-		vdebayer->sd.name, lin, col, vdebayer->sink_fmt.height, seek);
+		vdebayer->sd.name, lin, col, vdebayer->hw.size.height, seek);
 
 	/*
 	 * Iterate through all the lines in the mean window, start
@@ -439,7 +432,7 @@ static void vimc_debayer_calc_rgb_sink(struct vimc_debayer_device *vdebayer,
 	 * frame
 	 */
 	for (wlin = seek > lin ? 0 : lin - seek;
-	     wlin < lin + seek + 1 && wlin < vdebayer->sink_fmt.height;
+	     wlin < lin + seek + 1 && wlin < vdebayer->hw.size.height;
 	     wlin++) {
 
 		/*
@@ -449,17 +442,17 @@ static void vimc_debayer_calc_rgb_sink(struct vimc_debayer_device *vdebayer,
 		 * frame
 		 */
 		for (wcol = seek > col ? 0 : col - seek;
-		     wcol < col + seek + 1 && wcol < vdebayer->sink_fmt.width;
+		     wcol < col + seek + 1 && wcol < vdebayer->hw.size.width;
 		     wcol++) {
 			enum vimc_debayer_rgb_colors color;
 			unsigned int index;
 
 			/* Check which color this pixel is */
-			color = vdebayer->sink_pix_map->order[wlin % 2][wcol % 2];
+			color = vdebayer->hw.sink_pix_map->order[wlin % 2][wcol % 2];
 
 			index = VIMC_FRAME_INDEX(wlin, wcol,
-						 vdebayer->sink_fmt.width,
-						 vdebayer->sink_bpp);
+						 vdebayer->hw.size.width,
+						 vdebayer->hw.sink_bpp);
 
 			dev_dbg(vdebayer->ved.dev,
 				"deb: %s: RGB CALC: frame index %d, win pos %dx%d, color %d\n",
@@ -468,7 +461,7 @@ static void vimc_debayer_calc_rgb_sink(struct vimc_debayer_device *vdebayer,
 			/* Get its value */
 			rgb[color] = rgb[color] +
 				vimc_debayer_get_val(&frame[index],
-						     vdebayer->sink_bpp);
+						     vdebayer->hw.sink_bpp);
 
 			/* Save how many values we already added */
 			n_rgb[color]++;
@@ -506,8 +499,8 @@ static void *vimc_debayer_process_frame(struct vimc_ent_device *ved,
 	if (!vdebayer->src_frame)
 		return ERR_PTR(-EINVAL);
 
-	for (i = 0; i < vdebayer->sink_fmt.height; i++)
-		for (j = 0; j < vdebayer->sink_fmt.width; j++) {
+	for (i = 0; i < vdebayer->hw.size.height; i++)
+		for (j = 0; j < vdebayer->hw.size.width; j++) {
 			vimc_debayer_calc_rgb_sink(vdebayer, sink_frame, i, j, rgb);
 			vdebayer->set_rgb_src(vdebayer, i, j, rgb);
 		}
@@ -522,7 +515,7 @@ static int vimc_debayer_s_ctrl(struct v4l2_ctrl *ctrl)
 
 	switch (ctrl->id) {
 	case VIMC_CID_MEAN_WIN_SIZE:
-		vdebayer->mean_win_size = ctrl->val;
+		vdebayer->hw.mean_win_size = ctrl->val;
 		break;
 	default:
 		return -EINVAL;
@@ -599,17 +592,8 @@ static struct vimc_ent_device *vimc_debayer_add(struct vimc_device *vimc,
 
 	vdebayer->ved.process_frame = vimc_debayer_process_frame;
 	vdebayer->ved.dev = vimc->mdev.dev;
-	vdebayer->mean_win_size = vimc_debayer_ctrl_mean_win_size.def;
+	vdebayer->hw.mean_win_size = vimc_debayer_ctrl_mean_win_size.def;
 
-	/* Initialize the frame format */
-	vdebayer->sink_fmt = sink_fmt_default;
-	/*
-	 * TODO: Add support for more output formats, we only support
-	 * RGB888 for now
-	 * NOTE: the src format is always the same as the sink, except
-	 * for the code
-	 */
-	vdebayer->src_code = MEDIA_BUS_FMT_RGB888_1X24;
 	vdebayer->set_rgb_src = vimc_debayer_process_rgb_frame;
 
 	return &vdebayer->ved;
-- 
Regards,

Laurent Pinchart

[PATCH v1 9/9] media: vimc: scaler: Use subdev active state

[Laurent Pinchart]

Store the active formats and crop rectangle in the subdevice active
state. This simplifies implementation of the format and selection
accessors, and allows using the v4l2_subdev_get_fmt() helper to
implement the .get_fmt() operation.

The active configuration that is used in the .process_frame() handler is
still stored in the vimc_scaler_device structure. The driver could
instead access the active state in the .process_frame() handler, but the
required locking could interfere with the real time constraints of the
frame processing. This data would be stored in registers in the
.s_stream() handler for real hardware, storing it in dedicated storage
thus mimics a real driver. To differentiate them from the rest of the
device private data, move the corresponding fields to a sub-structure of
vimc_scaler_device named hw.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/test-drivers/vimc/vimc-scaler.c | 124 +++++++-----------
 1 file changed, 49 insertions(+), 75 deletions(-)

diff --git a/drivers/media/test-drivers/vimc/vimc-scaler.c b/drivers/media/test-drivers/vimc/vimc-scaler.c
index f8639f5b4d0c..47d0d63865a0 100644
--- a/drivers/media/test-drivers/vimc/vimc-scaler.c
+++ b/drivers/media/test-drivers/vimc/vimc-scaler.c
@@ -26,13 +26,20 @@ enum vimc_scaler_pad {
 struct vimc_scaler_device {
 	struct vimc_ent_device ved;
 	struct v4l2_subdev sd;
-	struct v4l2_rect crop_rect;
-	/* Frame format for both sink and src pad */
-	struct v4l2_mbus_framefmt fmt[2];
-	/* Values calculated when the stream starts */
-	u8 *src_frame;
-	unsigned int bpp;
 	struct media_pad pads[2];
+
+	u8 *src_frame;
+
+	/*
+	 * Virtual "hardware" configuration, filled when the stream starts or
+	 * when controls are set.
+	 */
+	struct {
+		struct v4l2_mbus_framefmt sink_fmt;
+		struct v4l2_mbus_framefmt src_fmt;
+		struct v4l2_rect sink_crop;
+		unsigned int bpp;
+	} hw;
 };
 
 static const struct v4l2_mbus_framefmt fmt_default = {
@@ -132,39 +139,6 @@ static int vimc_scaler_enum_frame_size(struct v4l2_subdev *sd,
 	return 0;
 }
 
-static struct v4l2_mbus_framefmt *
-vimc_scaler_pad_format(struct vimc_scaler_device *vscaler,
-		    struct v4l2_subdev_state *sd_state, u32 pad,
-		    enum v4l2_subdev_format_whence which)
-{
-	if (which == V4L2_SUBDEV_FORMAT_TRY)
-		return v4l2_subdev_state_get_format(sd_state, pad);
-	else
-		return &vscaler->fmt[pad];
-}
-
-static struct v4l2_rect *
-vimc_scaler_pad_crop(struct vimc_scaler_device *vscaler,
-		  struct v4l2_subdev_state *sd_state,
-		  enum v4l2_subdev_format_whence which)
-{
-	if (which == V4L2_SUBDEV_FORMAT_TRY)
-		return v4l2_subdev_state_get_crop(sd_state, VIMC_SCALER_SINK);
-	else
-		return &vscaler->crop_rect;
-}
-
-static int vimc_scaler_get_fmt(struct v4l2_subdev *sd,
-			    struct v4l2_subdev_state *sd_state,
-			    struct v4l2_subdev_format *format)
-{
-	struct vimc_scaler_device *vscaler = v4l2_get_subdevdata(sd);
-
-	format->format = *vimc_scaler_pad_format(vscaler, sd_state, format->pad,
-					      format->which);
-	return 0;
-}
-
 static int vimc_scaler_set_fmt(struct v4l2_subdev *sd,
 			    struct v4l2_subdev_state *sd_state,
 			    struct v4l2_subdev_format *format)
@@ -176,7 +150,7 @@ static int vimc_scaler_set_fmt(struct v4l2_subdev *sd,
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE && vscaler->src_frame)
 		return -EBUSY;
 
-	fmt = vimc_scaler_pad_format(vscaler, sd_state, format->pad, format->which);
+	fmt = v4l2_subdev_state_get_format(sd_state, format->pad);
 
 	/*
 	 * The media bus code and colorspace can only be changed on the sink
@@ -214,14 +188,13 @@ static int vimc_scaler_set_fmt(struct v4l2_subdev *sd,
 		struct v4l2_mbus_framefmt *src_fmt;
 		struct v4l2_rect *crop;
 
-		crop = vimc_scaler_pad_crop(vscaler, sd_state, format->which);
+		crop = v4l2_subdev_state_get_crop(sd_state, VIMC_SCALER_SINK);
 		crop->width = fmt->width;
 		crop->height = fmt->height;
 		crop->top = 0;
 		crop->left = 0;
 
-		src_fmt = vimc_scaler_pad_format(vscaler, sd_state, VIMC_SCALER_SRC,
-					      format->which);
+		src_fmt = v4l2_subdev_state_get_format(sd_state, VIMC_SCALER_SRC);
 		*src_fmt = *fmt;
 	}
 
@@ -234,7 +207,6 @@ static int vimc_scaler_get_selection(struct v4l2_subdev *sd,
 				  struct v4l2_subdev_state *sd_state,
 				  struct v4l2_subdev_selection *sel)
 {
-	struct vimc_scaler_device *vscaler = v4l2_get_subdevdata(sd);
 	struct v4l2_mbus_framefmt *sink_fmt;
 
 	if (VIMC_IS_SRC(sel->pad))
@@ -242,11 +214,10 @@ static int vimc_scaler_get_selection(struct v4l2_subdev *sd,
 
 	switch (sel->target) {
 	case V4L2_SEL_TGT_CROP:
-		sel->r = *vimc_scaler_pad_crop(vscaler, sd_state, sel->which);
+		sel->r = *v4l2_subdev_state_get_crop(sd_state, VIMC_SCALER_SINK);
 		break;
 	case V4L2_SEL_TGT_CROP_BOUNDS:
-		sink_fmt = vimc_scaler_pad_format(vscaler, sd_state, VIMC_SCALER_SINK,
-					       sel->which);
+		sink_fmt = v4l2_subdev_state_get_format(sd_state, VIMC_SCALER_SINK);
 		sel->r = vimc_scaler_get_crop_bound_sink(sink_fmt);
 		break;
 	default:
@@ -282,9 +253,8 @@ static int vimc_scaler_set_selection(struct v4l2_subdev *sd,
 	if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE && vscaler->src_frame)
 		return -EBUSY;
 
-	crop_rect = vimc_scaler_pad_crop(vscaler, sd_state, sel->which);
-	sink_fmt = vimc_scaler_pad_format(vscaler, sd_state, VIMC_SCALER_SINK,
-				       sel->which);
+	crop_rect = v4l2_subdev_state_get_crop(sd_state, VIMC_SCALER_SINK);
+	sink_fmt = v4l2_subdev_state_get_format(sd_state, VIMC_SCALER_SINK);
 	vimc_scaler_adjust_sink_crop(&sel->r, sink_fmt);
 	*crop_rect = sel->r;
 
@@ -294,7 +264,7 @@ static int vimc_scaler_set_selection(struct v4l2_subdev *sd,
 static const struct v4l2_subdev_pad_ops vimc_scaler_pad_ops = {
 	.enum_mbus_code		= vimc_scaler_enum_mbus_code,
 	.enum_frame_size	= vimc_scaler_enum_frame_size,
-	.get_fmt		= vimc_scaler_get_fmt,
+	.get_fmt		= v4l2_subdev_get_fmt,
 	.set_fmt		= vimc_scaler_set_fmt,
 	.get_selection		= vimc_scaler_get_selection,
 	.set_selection		= vimc_scaler_set_selection,
@@ -305,27 +275,38 @@ static int vimc_scaler_s_stream(struct v4l2_subdev *sd, int enable)
 	struct vimc_scaler_device *vscaler = v4l2_get_subdevdata(sd);
 
 	if (enable) {
-		const struct vimc_pix_map *vpix;
+		struct v4l2_subdev_state *state;
+		const struct v4l2_mbus_framefmt *format;
+		const struct v4l2_rect *rect;
 		unsigned int frame_size;
 
 		if (vscaler->src_frame)
 			return 0;
 
-		/* Save the bytes per pixel of the sink */
-		vpix = vimc_pix_map_by_code(vscaler->fmt[VIMC_SCALER_SINK].code);
-		vscaler->bpp = vpix->bpp;
+		state = v4l2_subdev_lock_and_get_active_state(sd);
 
-		/* Calculate the frame size of the source pad */
-		frame_size = vscaler->fmt[VIMC_SCALER_SRC].width
-			   * vscaler->fmt[VIMC_SCALER_SRC].height * vscaler->bpp;
+		/* Save the bytes per pixel of the sink. */
+		format = v4l2_subdev_state_get_format(state, VIMC_SCALER_SINK);
+		vscaler->hw.sink_fmt = *format;
+		vscaler->hw.bpp = vimc_pix_map_by_code(format->code)->bpp;
 
-		/* Allocate the frame buffer. Use vmalloc to be able to
-		 * allocate a large amount of memory
+		/* Calculate the frame size of the source pad. */
+		format = v4l2_subdev_state_get_format(state, VIMC_SCALER_SRC);
+		vscaler->hw.src_fmt = *format;
+		frame_size = format->width * format->height * vscaler->hw.bpp;
+
+		rect = v4l2_subdev_state_get_crop(state, VIMC_SCALER_SINK);
+		vscaler->hw.sink_crop = *rect;
+
+		v4l2_subdev_unlock_state(state);
+
+		/*
+		 * Allocate the frame buffer. Use vmalloc to be able to allocate
+		 * a large amount of memory.
 		 */
 		vscaler->src_frame = vmalloc(frame_size);
 		if (!vscaler->src_frame)
 			return -ENOMEM;
-
 	} else {
 		if (!vscaler->src_frame)
 			return 0;
@@ -353,9 +334,9 @@ static const struct v4l2_subdev_internal_ops vimc_scaler_internal_ops = {
 static void vimc_scaler_fill_src_frame(const struct vimc_scaler_device *const vscaler,
 				    const u8 *const sink_frame)
 {
-	const struct v4l2_mbus_framefmt *src_fmt = &vscaler->fmt[VIMC_SCALER_SRC];
-	const struct v4l2_rect *r = &vscaler->crop_rect;
-	unsigned int snk_width = vscaler->fmt[VIMC_SCALER_SINK].width;
+	const struct v4l2_mbus_framefmt *sink_fmt = &vscaler->hw.sink_fmt;
+	const struct v4l2_mbus_framefmt *src_fmt = &vscaler->hw.src_fmt;
+	const struct v4l2_rect *r = &vscaler->hw.sink_crop;
 	unsigned int src_x, src_y;
 	u8 *walker = vscaler->src_frame;
 
@@ -364,16 +345,16 @@ static void vimc_scaler_fill_src_frame(const struct vimc_scaler_device *const vs
 		unsigned int snk_y, y_offset;
 
 		snk_y = (src_y * r->height) / src_fmt->height + r->top;
-		y_offset = snk_y * snk_width * vscaler->bpp;
+		y_offset = snk_y * sink_fmt->width * vscaler->hw.bpp;
 
 		for (src_x = 0; src_x < src_fmt->width; src_x++) {
 			unsigned int snk_x, x_offset, index;
 
 			snk_x = (src_x * r->width) / src_fmt->width + r->left;
-			x_offset = snk_x * vscaler->bpp;
+			x_offset = snk_x * vscaler->hw.bpp;
 			index = y_offset + x_offset;
-			memcpy(walker, &sink_frame[index], vscaler->bpp);
-			walker += vscaler->bpp;
+			memcpy(walker, &sink_frame[index], vscaler->hw.bpp);
+			walker += vscaler->hw.bpp;
 		}
 	}
 }
@@ -432,13 +413,6 @@ static struct vimc_ent_device *vimc_scaler_add(struct vimc_device *vimc,
 	vscaler->ved.process_frame = vimc_scaler_process_frame;
 	vscaler->ved.dev = vimc->mdev.dev;
 
-	/* Initialize the frame format */
-	vscaler->fmt[VIMC_SCALER_SINK] = fmt_default;
-	vscaler->fmt[VIMC_SCALER_SRC] = fmt_default;
-
-	/* Initialize the crop selection */
-	vscaler->crop_rect = crop_rect_default;
-
 	return &vscaler->ved;
 }
 
-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 0/9] media: vimc improvements

[Laurent Pinchart]

Hello,

On Thu, Apr 25, 2024 at 02:57:32AM +0300, Laurent Pinchart wrote:
> Hello,
> 
> This patch series cleans up and improves the vimc driver, with the end
> goal of converting it to the V4L2 subdev active state API. The goal of
> this exercise is to make the API used by a virtual test driver, to
> increase test coverage.
> 
> The series starts with 4 random cleanups, to avoid unnecessary
> iterations (1/9), constify structures (2/9 and 3/9) and rename a
> weirdly-named enum (4/9). Patch 5/9 then centralizes the subdev
> internal_ops initialization to prepare for the switch to the active
> state API. The remaining patches (6/9 to 9/9) convert the vimc entities
> to the new API one by one.
> 
> The result has been tested using the libcamera unit tests, which make
> extensive use of the vimc driver, as well as with v4l2-compliance. The
> latter reports 4 errors, but they occur already with the latest stage
> master branch.

Any comment about this patch series ? Shuah, do you plan to give it a
look ? If you don't have much time for vimc these days that's OK, I can
work with Kieran to get the patches reviewed.

On a related note, I would like to modify the MC pipeline of the vimc
driver to make it look more like a real ISP. This would help with unit
testing and compliance testing in libcamera. I briefly discussed this
with Hans on IRC who had no objection. If anyone has any issue or
concern with this plan, I would appreciate if you could let me know.

> Laurent Pinchart (9):
>   media: vimc: Don't iterate over single pad
>   media: vimc: Constify vimc_ent_type structures
>   media: vimc: Constify the ent_config array
>   media: vimc: scaler: Rename vic_sca_pad enum to vimc_scaler_pad
>   media: vimc: Centralize subdev internal_ops initialization
>   media: vimc: Initialize subdev active state
>   media: vimc: sensor: Use subdev active state
>   media: vimc: debayer: Use subdev active state
>   media: vimc: scaler: Use subdev active state
> 
>  .../media/test-drivers/vimc/vimc-capture.c    |   2 +-
>  drivers/media/test-drivers/vimc/vimc-common.c |  25 ++-
>  drivers/media/test-drivers/vimc/vimc-common.h |  14 +-
>  drivers/media/test-drivers/vimc/vimc-core.c   |   2 +-
>  .../media/test-drivers/vimc/vimc-debayer.c    | 197 ++++++++----------
>  drivers/media/test-drivers/vimc/vimc-lens.c   |   5 +-
>  drivers/media/test-drivers/vimc/vimc-scaler.c | 134 +++++-------
>  drivers/media/test-drivers/vimc/vimc-sensor.c | 125 +++++------
>  8 files changed, 236 insertions(+), 268 deletions(-)
> 
> 
> base-commit: e42a204f0519a2540f1507ac2798be2aeaa76bee

-- 
Regards,

Laurent Pinchart