[PATCH 1/3] media: rkisp1: Cleanup error handling

linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed

* [PATCH 1/3] media: rkisp1: Cleanup error handling
       [not found] <20250522150944.400046-2-stefan.klug@ideasonboard.com>
@ 2025-05-22 15:08 ` Stefan Klug
  2025-05-22 15:42   ` Laurent Pinchart
  2025-05-22 15:08 ` [PATCH 2/3] media: rkisp1: Add RKISP1_CID_SUPPORTED_PARAMS_BLOCKS control Stefan Klug
  2025-05-22 15:08 ` [PATCH 3/3] media: rockchip: rkisp1: Add support for Wide Dynamic Range Stefan Klug
  2 siblings, 1 reply; 11+ messages in thread
From: Stefan Klug @ 2025-05-22 15:08 UTC (permalink / raw)
  To: linux-media, Dafna Hirschfeld, Laurent Pinchart,
	Mauro Carvalho Chehab, Heiko Stuebner
  Cc: Stefan Klug, linux-rockchip, linux-arm-kernel, linux-kernel

Do not call media_entity_cleanup() when media_entity_pads_init() fails.
As a drive-by fix handle an (very unlikely) error in
rkisp1_params_init_vb2_queue().

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 .../media/platform/rockchip/rkisp1/rkisp1-params.c    | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
index b28f4140c8a3..918eb06c7465 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
@@ -2763,7 +2763,9 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1)
 	vdev->queue = &node->buf_queue;
 	vdev->device_caps = V4L2_CAP_STREAMING | V4L2_CAP_META_OUTPUT;
 	vdev->vfl_dir = VFL_DIR_TX;
-	rkisp1_params_init_vb2_queue(vdev->queue, params);
+	ret = rkisp1_params_init_vb2_queue(vdev->queue, params);
+	if (ret)
+		goto err_mutex;
 
 	params->metafmt = &rkisp1_params_formats[RKISP1_PARAMS_FIXED];
 
@@ -2777,19 +2779,20 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1)
 	node->pad.flags = MEDIA_PAD_FL_SOURCE;
 	ret = media_entity_pads_init(&vdev->entity, 1, &node->pad);
 	if (ret)
-		goto error;
+		goto err_mutex;
 
 	ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
 	if (ret) {
 		dev_err(rkisp1->dev,
 			"failed to register %s, ret=%d\n", vdev->name, ret);
-		goto error;
+		goto err_media;
 	}
 
 	return 0;
 
-error:
+err_media:
 	media_entity_cleanup(&vdev->entity);
+err_mutex:
 	mutex_destroy(&node->vlock);
 	return ret;
 }
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/3] media: rkisp1: Add RKISP1_CID_SUPPORTED_PARAMS_BLOCKS control
       [not found] <20250522150944.400046-2-stefan.klug@ideasonboard.com>
  2025-05-22 15:08 ` [PATCH 1/3] media: rkisp1: Cleanup error handling Stefan Klug
@ 2025-05-22 15:08 ` Stefan Klug
  2025-05-22 15:56   ` Laurent Pinchart
  2025-05-23  2:19   ` Paul Elder
  2025-05-22 15:08 ` [PATCH 3/3] media: rockchip: rkisp1: Add support for Wide Dynamic Range Stefan Klug
  2 siblings, 2 replies; 11+ messages in thread
From: Stefan Klug @ 2025-05-22 15:08 UTC (permalink / raw)
  To: linux-media, Dafna Hirschfeld, Laurent Pinchart,
	Mauro Carvalho Chehab, Heiko Stuebner
  Cc: Stefan Klug, linux-rockchip, linux-arm-kernel, linux-kernel

Add a RKISP1_CID_SUPPORTED_PARAMS_BLOCKS V4L2 control to be able to
query the parameters blocks supported by the current kernel on the
current hardware from user space.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 .../platform/rockchip/rkisp1/rkisp1-common.h  |  2 +
 .../platform/rockchip/rkisp1/rkisp1-params.c  | 50 ++++++++++++++++++-
 include/uapi/linux/rkisp1-config.h            | 10 ++++
 include/uapi/linux/v4l2-controls.h            |  6 +++
 4 files changed, 67 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
index ca952fd0829b..5f187f9efc7b 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
@@ -415,6 +415,8 @@ struct rkisp1_params {
 	spinlock_t config_lock; /* locks the buffers list 'params' */
 	struct list_head params;
 
+	struct v4l2_ctrl_handler ctrls;
+
 	const struct v4l2_meta_format *metafmt;
 
 	enum v4l2_quantization quantization;
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
index 918eb06c7465..60c9b3c46593 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
@@ -2736,6 +2736,45 @@ static int rkisp1_params_init_vb2_queue(struct vb2_queue *q,
 	return vb2_queue_init(q);
 }
 
+static int rkisp1_ctrl_init(struct rkisp1_params *params)
+{
+	int ret;
+
+	v4l2_ctrl_handler_init(&params->ctrls, 1);
+
+	struct v4l2_ctrl_config ctrl_config = {
+		.id = RKISP1_CID_SUPPORTED_PARAMS_BLOCKS,
+		.name = "Supported Params Blocks",
+		.type = V4L2_CTRL_TYPE_BITMASK,
+		.flags = V4L2_CTRL_FLAG_READ_ONLY,
+	};
+
+	for (unsigned int i = 0; i < ARRAY_SIZE(rkisp1_ext_params_handlers); i++) {
+		const struct rkisp1_ext_params_handler *block_handler;
+
+		block_handler = &rkisp1_ext_params_handlers[i];
+		ctrl_config.max |= BIT(i);
+
+		if ((params->rkisp1->info->features & block_handler->features) !=
+		    block_handler->features)
+			continue;
+
+		ctrl_config.def |= BIT(i);
+	}
+
+	v4l2_ctrl_new_custom(&params->ctrls, &ctrl_config, NULL);
+
+	params->vnode.vdev.ctrl_handler = &params->ctrls;
+
+	if (params->ctrls.error) {
+		ret = params->ctrls.error;
+		v4l2_ctrl_handler_free(&params->ctrls);
+		return ret;
+	}
+
+	return 0;
+}
+
 int rkisp1_params_register(struct rkisp1_device *rkisp1)
 {
 	struct rkisp1_params *params = &rkisp1->params;
@@ -2776,10 +2815,16 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1)
 
 	video_set_drvdata(vdev, params);
 
+	ret = rkisp1_ctrl_init(params);
+	if (ret) {
+		dev_err(rkisp1->dev, "Control initialization error %d\n", ret);
+		goto err_mutex;
+	}
+
 	node->pad.flags = MEDIA_PAD_FL_SOURCE;
 	ret = media_entity_pads_init(&vdev->entity, 1, &node->pad);
 	if (ret)
-		goto err_mutex;
+		goto err_ctrl;
 
 	ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
 	if (ret) {
@@ -2792,6 +2837,8 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1)
 
 err_media:
 	media_entity_cleanup(&vdev->entity);
+err_ctrl:
+	v4l2_ctrl_handler_free(&params->ctrls);
 err_mutex:
 	mutex_destroy(&node->vlock);
 	return ret;
@@ -2808,5 +2855,6 @@ void rkisp1_params_unregister(struct rkisp1_device *rkisp1)
 
 	vb2_video_unregister_device(vdev);
 	media_entity_cleanup(&vdev->entity);
+	v4l2_ctrl_handler_free(&params->ctrls);
 	mutex_destroy(&node->vlock);
 }
diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
index 2d995f3c1ca3..4fc8f221d0c4 100644
--- a/include/uapi/linux/rkisp1-config.h
+++ b/include/uapi/linux/rkisp1-config.h
@@ -1086,6 +1086,9 @@ enum rkisp1_ext_params_block_type {
 #define RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE	(1U << 0)
 #define RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE	(1U << 1)
 
+/* A bitmask of parameters blocks supported on the current hardware. */
+#define RKISP1_CID_SUPPORTED_PARAMS_BLOCKS	(V4L2_CID_USER_RKISP1_BASE + 0x01)
+
 /**
  * struct rkisp1_ext_params_block_header - RkISP1 extensible parameters block
  *					   header
@@ -1520,6 +1523,13 @@ enum rksip1_ext_param_buffer_version {
  * V4L2 control. If such control is not available, userspace should assume only
  * RKISP1_EXT_PARAM_BUFFER_V1 is supported by the driver.
  *
+ * The read-only V4L2 control ``RKISP1_CID_SUPPORTED_PARAMS_BLOCKS`` can be used
+ * to query the blocks supported by the current hardware. It contains a bitmask
+ * where each bit represents the availability of the corresponding entry
+ * from the :c:type:`rkisp1_ext_params_block_type` enum. The max value of the
+ * control represents the blocks supported by the current kernel (independent of
+ * the current hardware).
+ *
  * For each ISP block that userspace wants to configure, a block-specific
  * structure is appended to the @data buffer, one after the other without gaps
  * in between nor overlaps. Userspace shall populate the @data_size field with
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 72e32814ea83..f836512e9deb 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -222,6 +222,12 @@ enum v4l2_colorfx {
  */
 #define V4L2_CID_USER_UVC_BASE			(V4L2_CID_USER_BASE + 0x11e0)
 
+/*
+ * The base for Rockchip ISP1 driver controls.
+ * We reserve 16 controls for this driver.
+ */
+#define V4L2_CID_USER_RKISP1_BASE		(V4L2_CID_USER_BASE + 0x1220)
+
 /* MPEG-class control IDs */
 /* The MPEG controls are applicable to all codec controls
  * and the 'MPEG' part of the define is historical */
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/3] media: rockchip: rkisp1: Add support for Wide Dynamic Range
       [not found] <20250522150944.400046-2-stefan.klug@ideasonboard.com>
  2025-05-22 15:08 ` [PATCH 1/3] media: rkisp1: Cleanup error handling Stefan Klug
  2025-05-22 15:08 ` [PATCH 2/3] media: rkisp1: Add RKISP1_CID_SUPPORTED_PARAMS_BLOCKS control Stefan Klug
@ 2025-05-22 15:08 ` Stefan Klug
  2 siblings, 0 replies; 11+ messages in thread
From: Stefan Klug @ 2025-05-22 15:08 UTC (permalink / raw)
  To: linux-media, Dafna Hirschfeld, Laurent Pinchart,
	Mauro Carvalho Chehab, Heiko Stuebner
  Cc: Stefan Klug, Jai Luthra, Paul Elder, linux-rockchip,
	linux-arm-kernel, linux-kernel

From: Jai Luthra <jai.luthra@ideasonboard.com>

RKISP supports a basic Wide Dynamic Range (WDR) module since the first
iteration (v1.0) of the ISP. Add support for enabling and configuring it
using extensible parameters.

Also, to ease programming, switch to using macro variables for defining
the tonemapping curve register addresses.

Signed-off-by: Jai Luthra <jai.luthra@ideasonboard.com>
Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
Tested-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 .../platform/rockchip/rkisp1/rkisp1-params.c  | 93 +++++++++++++++++
 .../platform/rockchip/rkisp1/rkisp1-regs.h    | 99 +++++--------------
 include/uapi/linux/rkisp1-config.h            | 92 +++++++++++++++++
 3 files changed, 210 insertions(+), 74 deletions(-)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
index 60c9b3c46593..bfd1e33938c5 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
  */
 
+#include <linux/bitfield.h>
 #include <linux/math.h>
 #include <linux/string.h>
 
@@ -60,6 +61,7 @@ union rkisp1_ext_params_config {
 	struct rkisp1_ext_params_afc_config afc;
 	struct rkisp1_ext_params_compand_bls_config compand_bls;
 	struct rkisp1_ext_params_compand_curve_config compand_curve;
+	struct rkisp1_ext_params_wdr_config wdr;
 };
 
 enum rkisp1_params_formats {
@@ -1348,6 +1350,73 @@ rkisp1_compand_compress_config(struct rkisp1_params *params,
 				       arg->x);
 }
 
+static void rkisp1_wdr_config(struct rkisp1_params *params,
+			      const struct rkisp1_cif_isp_wdr_config *arg)
+{
+	unsigned int i;
+	u32 value;
+
+	value = rkisp1_read(params->rkisp1, RKISP1_CIF_ISP_WDR_CTRL)
+	      & ~(RKISP1_CIF_ISP_WDR_USE_IREF |
+		  RKISP1_CIF_ISP_WDR_COLOR_SPACE_SELECT |
+		  RKISP1_CIF_ISP_WDR_CR_MAPPING_DISABLE |
+		  RKISP1_CIF_ISP_WDR_USE_Y9_8 |
+		  RKISP1_CIF_ISP_WDR_USE_RGB7_8 |
+		  RKISP1_CIF_ISP_WDR_DISABLE_TRANSIENT |
+		  RKISP1_CIF_ISP_WDR_RGB_FACTOR_MASK);
+
+	/* Colorspace and chrominance mapping */
+	if (arg->use_rgb_colorspace)
+		value |= RKISP1_CIF_ISP_WDR_COLOR_SPACE_SELECT;
+
+	if (!arg->use_rgb_colorspace && arg->bypass_chroma_mapping)
+		value |= RKISP1_CIF_ISP_WDR_CR_MAPPING_DISABLE;
+
+	/* Illumination reference */
+	if (arg->use_iref) {
+		value |= RKISP1_CIF_ISP_WDR_USE_IREF;
+
+		if (arg->iref_config.use_y9_8)
+			value |= RKISP1_CIF_ISP_WDR_USE_Y9_8;
+
+		if (arg->iref_config.use_rgb7_8)
+			value |= RKISP1_CIF_ISP_WDR_USE_RGB7_8;
+
+		if (arg->iref_config.disable_transient)
+			value |= RKISP1_CIF_ISP_WDR_DISABLE_TRANSIENT;
+
+		value |= FIELD_PREP(RKISP1_CIF_ISP_WDR_RGB_FACTOR_MASK,
+				    min(arg->iref_config.rgb_factor,
+					RKISP1_CIF_ISP_WDR_RGB_FACTOR_MAX));
+	}
+
+	rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_WDR_CTRL, value);
+
+	/* RGB and Luminance offsets */
+	value = FIELD_PREP(RKISP1_CIF_ISP_WDR_RGB_OFFSET_MASK,
+			   arg->rgb_offset)
+	      | FIELD_PREP(RKISP1_CIF_ISP_WDR_LUM_OFFSET_MASK,
+			   arg->luma_offset);
+	rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_WDR_OFFSET, value);
+
+	/* DeltaMin */
+	value = FIELD_PREP(RKISP1_CIF_ISP_WDR_DMIN_THRESH_MASK,
+			   arg->dmin_thresh)
+	      | FIELD_PREP(RKISP1_CIF_ISP_WDR_DMIN_STRENGTH_MASK,
+			   min(arg->dmin_strength,
+			       RKISP1_CIF_ISP_WDR_DMIN_STRENGTH_MAX));
+	rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_WDR_DELTAMIN, value);
+
+	/* Tone curve */
+	for (i = 0; i < RKISP1_CIF_ISP_WDR_CURVE_NUM_DY_REGS; i++)
+		rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_WDR_TONECURVE(i),
+			     arg->tone_curve.dY[i]);
+	for (i = 0; i < RKISP1_CIF_ISP_WDR_CURVE_NUM_COEFF; i++)
+		rkisp1_write(params->rkisp1, RKISP1_CIF_ISP_WDR_TONECURVE_YM(i),
+			     arg->tone_curve.ym[i] &
+				     RKISP1_CIF_ISP_WDR_TONE_CURVE_YM_MASK);
+}
+
 static void
 rkisp1_isp_isr_other_config(struct rkisp1_params *params,
 			    const struct rkisp1_params_cfg *new_params)
@@ -2005,6 +2074,25 @@ static void rkisp1_ext_params_compand_compress(struct rkisp1_params *params,
 				      RKISP1_CIF_ISP_COMPAND_CTRL_COMPRESS_ENABLE);
 }
 
+static void rkisp1_ext_params_wdr(struct rkisp1_params *params,
+				  const union rkisp1_ext_params_config *block)
+{
+	const struct rkisp1_ext_params_wdr_config *wdr = &block->wdr;
+
+	if (wdr->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE) {
+		rkisp1_param_clear_bits(params, RKISP1_CIF_ISP_WDR_CTRL,
+					RKISP1_CIF_ISP_WDR_CTRL_ENABLE);
+		return;
+	}
+
+	rkisp1_wdr_config(params, &wdr->config);
+
+	if ((wdr->header.flags & RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE) &&
+	    !(params->enabled_blocks & BIT(wdr->header.type)))
+		rkisp1_param_set_bits(params, RKISP1_CIF_ISP_WDR_CTRL,
+				      RKISP1_CIF_ISP_WDR_CTRL_ENABLE);
+}
+
 typedef void (*rkisp1_block_handler)(struct rkisp1_params *params,
 			     const union rkisp1_ext_params_config *config);
 
@@ -2118,6 +2206,11 @@ static const struct rkisp1_ext_params_handler {
 		.group		= RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
 		.features	= RKISP1_FEATURE_COMPAND,
 	},
+	[RKISP1_EXT_PARAMS_BLOCK_TYPE_WDR] = {
+		.size		= sizeof(struct rkisp1_ext_params_wdr_config),
+		.handler	= rkisp1_ext_params_wdr,
+		.group		= RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS,
+	},
 };
 
 static void rkisp1_ext_params_config(struct rkisp1_params *params,
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h
index bf0260600a19..5b9de1a2c624 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-regs.h
@@ -710,6 +710,27 @@
 #define RKISP1_CIF_ISP_COMPAND_CTRL_SOFT_RESET_FLAG	BIT(2)
 #define RKISP1_CIF_ISP_COMPAND_CTRL_BLS_ENABLE		BIT(3)
 
+/* WDR */
+/* ISP_WDR_CTRL */
+#define RKISP1_CIF_ISP_WDR_CTRL_ENABLE			BIT(0)
+#define RKISP1_CIF_ISP_WDR_COLOR_SPACE_SELECT		BIT(1)
+#define RKISP1_CIF_ISP_WDR_CR_MAPPING_DISABLE		BIT(2)
+#define RKISP1_CIF_ISP_WDR_USE_IREF			BIT(3)
+#define RKISP1_CIF_ISP_WDR_USE_Y9_8			BIT(4)
+#define RKISP1_CIF_ISP_WDR_USE_RGB7_8			BIT(5)
+#define RKISP1_CIF_ISP_WDR_DISABLE_TRANSIENT		BIT(6)
+#define RKISP1_CIF_ISP_WDR_RGB_FACTOR_MASK		GENMASK(11, 8)
+#define RKISP1_CIF_ISP_WDR_RGB_FACTOR_MAX		8U
+/* ISP_WDR_TONE_CURVE_YM */
+#define RKISP1_CIF_ISP_WDR_TONE_CURVE_YM_MASK		GENMASK(12, 0)
+/* ISP_WDR_OFFSET */
+#define RKISP1_CIF_ISP_WDR_RGB_OFFSET_MASK		GENMASK(11, 0)
+#define RKISP1_CIF_ISP_WDR_LUM_OFFSET_MASK		GENMASK(27, 16)
+/* ISP_WDR_DELTAMIN */
+#define RKISP1_CIF_ISP_WDR_DMIN_THRESH_MASK		GENMASK(11, 0)
+#define RKISP1_CIF_ISP_WDR_DMIN_STRENGTH_MASK		GENMASK(20, 16)
+#define RKISP1_CIF_ISP_WDR_DMIN_STRENGTH_MAX		16U
+
 /* =================================================================== */
 /*                            CIF Registers                            */
 /* =================================================================== */
@@ -1302,82 +1323,12 @@
 
 #define RKISP1_CIF_ISP_WDR_BASE			0x00002a00
 #define RKISP1_CIF_ISP_WDR_CTRL			(RKISP1_CIF_ISP_WDR_BASE + 0x00000000)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_1		(RKISP1_CIF_ISP_WDR_BASE + 0x00000004)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_2		(RKISP1_CIF_ISP_WDR_BASE + 0x00000008)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_3		(RKISP1_CIF_ISP_WDR_BASE + 0x0000000c)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_4		(RKISP1_CIF_ISP_WDR_BASE + 0x00000010)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_0	(RKISP1_CIF_ISP_WDR_BASE + 0x00000014)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_1	(RKISP1_CIF_ISP_WDR_BASE + 0x00000018)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_2	(RKISP1_CIF_ISP_WDR_BASE + 0x0000001c)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_3	(RKISP1_CIF_ISP_WDR_BASE + 0x00000020)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_4	(RKISP1_CIF_ISP_WDR_BASE + 0x00000024)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_5	(RKISP1_CIF_ISP_WDR_BASE + 0x00000028)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_6	(RKISP1_CIF_ISP_WDR_BASE + 0x0000002c)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_7	(RKISP1_CIF_ISP_WDR_BASE + 0x00000030)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_8	(RKISP1_CIF_ISP_WDR_BASE + 0x00000034)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_9	(RKISP1_CIF_ISP_WDR_BASE + 0x00000038)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_10	(RKISP1_CIF_ISP_WDR_BASE + 0x0000003c)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_11	(RKISP1_CIF_ISP_WDR_BASE + 0x00000040)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_12	(RKISP1_CIF_ISP_WDR_BASE + 0x00000044)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_13	(RKISP1_CIF_ISP_WDR_BASE + 0x00000048)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_14	(RKISP1_CIF_ISP_WDR_BASE + 0x0000004c)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_15	(RKISP1_CIF_ISP_WDR_BASE + 0x00000050)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_16	(RKISP1_CIF_ISP_WDR_BASE + 0x00000054)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_17	(RKISP1_CIF_ISP_WDR_BASE + 0x00000058)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_18	(RKISP1_CIF_ISP_WDR_BASE + 0x0000005c)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_19	(RKISP1_CIF_ISP_WDR_BASE + 0x00000060)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_20	(RKISP1_CIF_ISP_WDR_BASE + 0x00000064)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_21	(RKISP1_CIF_ISP_WDR_BASE + 0x00000068)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_22	(RKISP1_CIF_ISP_WDR_BASE + 0x0000006c)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_23	(RKISP1_CIF_ISP_WDR_BASE + 0x00000070)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_24	(RKISP1_CIF_ISP_WDR_BASE + 0x00000074)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_25	(RKISP1_CIF_ISP_WDR_BASE + 0x00000078)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_26	(RKISP1_CIF_ISP_WDR_BASE + 0x0000007c)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_27	(RKISP1_CIF_ISP_WDR_BASE + 0x00000080)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_28	(RKISP1_CIF_ISP_WDR_BASE + 0x00000084)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_29	(RKISP1_CIF_ISP_WDR_BASE + 0x00000088)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_30	(RKISP1_CIF_ISP_WDR_BASE + 0x0000008c)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_31	(RKISP1_CIF_ISP_WDR_BASE + 0x00000090)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_32	(RKISP1_CIF_ISP_WDR_BASE + 0x00000094)
+#define RKISP1_CIF_ISP_WDR_TONECURVE(n)		(RKISP1_CIF_ISP_WDR_BASE + 0x00000004 + (n) * 4)
+#define RKISP1_CIF_ISP_WDR_TONECURVE_YM(n)	(RKISP1_CIF_ISP_WDR_BASE + 0x00000014 + (n) * 4)
 #define RKISP1_CIF_ISP_WDR_OFFSET		(RKISP1_CIF_ISP_WDR_BASE + 0x00000098)
 #define RKISP1_CIF_ISP_WDR_DELTAMIN		(RKISP1_CIF_ISP_WDR_BASE + 0x0000009c)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_1_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x000000a0)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_2_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x000000a4)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_3_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x000000a8)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_4_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x000000ac)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_0_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x000000b0)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_1_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x000000b4)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_2_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x000000b8)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_3_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x000000bc)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_4_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x000000c0)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_5_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x000000c4)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_6_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x000000c8)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_7_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x000000cc)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_8_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x000000d0)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_9_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x000000d4)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_10_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x000000d8)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_11_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x000000dc)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_12_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x000000e0)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_13_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x000000e4)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_14_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x000000e8)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_15_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x000000ec)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_16_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x000000f0)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_17_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x000000f4)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_18_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x000000f8)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_19_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x000000fc)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_20_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x00000100)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_21_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x00000104)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_22_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x00000108)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_23_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x0000010c)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_24_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x00000110)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_25_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x00000114)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_26_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x00000118)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_27_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x0000011c)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_28_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x00000120)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_29_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x00000124)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_30_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x00000128)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_31_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x0000012c)
-#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_32_SHD	(RKISP1_CIF_ISP_WDR_BASE + 0x00000130)
+#define RKISP1_CIF_ISP_WDR_TONECURVE_SHD(n)	(RKISP1_CIF_ISP_WDR_BASE + 0x000000a0 + (n) * 4)
+#define RKISP1_CIF_ISP_WDR_TONECURVE_YM_SHD(n)	(RKISP1_CIF_ISP_WDR_BASE + 0x000000b0 + (n) * 4)
 
 #define RKISP1_CIF_ISP_HIST_BASE_V12		0x00002c00
 #define RKISP1_CIF_ISP_HIST_CTRL_V12		(RKISP1_CIF_ISP_HIST_BASE_V12 + 0x00000000)
diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
index 4fc8f221d0c4..a49a3420d9b5 100644
--- a/include/uapi/linux/rkisp1-config.h
+++ b/include/uapi/linux/rkisp1-config.h
@@ -169,6 +169,13 @@
  */
 #define RKISP1_CIF_ISP_COMPAND_NUM_POINTS	64
 
+/*
+ * Wide Dynamic Range
+ */
+#define RKISP1_CIF_ISP_WDR_CURVE_NUM_INTERV	32
+#define RKISP1_CIF_ISP_WDR_CURVE_NUM_COEFF	(RKISP1_CIF_ISP_WDR_CURVE_NUM_INTERV + 1)
+#define RKISP1_CIF_ISP_WDR_CURVE_NUM_DY_REGS	4
+
 /*
  * Measurement types
  */
@@ -889,6 +896,72 @@ struct rkisp1_cif_isp_compand_curve_config {
 	__u32 y[RKISP1_CIF_ISP_COMPAND_NUM_POINTS];
 };
 
+/**
+ * struct rkisp1_cif_isp_wdr_tone_curve - Tone mapping curve definition for WDR.
+ *
+ * @dY: the dYn increments for horizontal (input) axis of the tone curve.
+ *      each 3-bit dY value represents an increment of 2**(value+3).
+ *      dY[0] bits 0:2 is increment dY1, bit 3 unused
+ *      dY[0] bits 4:6 is increment dY2, bit 7 unused
+ *      ...
+ *      dY[0] bits 28:30 is increment dY8, bit 31 unused
+ *      ... and so on till dY[3] bits 28:30 is increment dY32, bit 31 unused.
+ * @ym: the Ym values for the vertical (output) axis of the tone curve.
+ *      each value is 13 bit.
+ */
+struct rkisp1_cif_isp_wdr_tone_curve {
+	__u32 dY[RKISP1_CIF_ISP_WDR_CURVE_NUM_DY_REGS];
+	__u16 ym[RKISP1_CIF_ISP_WDR_CURVE_NUM_COEFF];
+};
+
+/**
+ * struct rkisp1_cif_isp_wdr_iref_config - Illumination reference config for WDR.
+ *
+ * Use illumination reference value as described below, instead of only the
+ * luminance (Y) value for tone mapping and gain calculations:
+ * IRef = (rgb_factor * RGBMax_tr + (8 - rgb_factor) * Y)/8
+ *
+ * @rgb_factor: defines how much influence the RGBmax approach has in
+ *              comparison to Y (valid values are 0..8).
+ * @use_y9_8: use Y*9/8 for maximum value calculation along with the
+ *            default of R, G, B for noise reduction.
+ * @use_rgb7_8: decrease RGBMax by 7/8 for noise reduction.
+ * @disable_transient: disable transient calculation between Y and RGBY_max.
+ */
+struct rkisp1_cif_isp_wdr_iref_config {
+	__u8 rgb_factor;
+	__u8 use_y9_8;
+	__u8 use_rgb7_8;
+	__u8 disable_transient;
+};
+
+/**
+ * struct rkisp1_cif_isp_wdr_config - Configuration for wide dynamic range.
+ *
+ * @tone_curve: tone mapping curve.
+ * @iref_config: illumination reference configuration. (when use_iref is true)
+ * @rgb_offset: RGB offset value for RGB operation mode. (12 bits)
+ * @luma_offset: luminance offset value for RGB operation mode. (12 bits)
+ * @dmin_thresh: lower threshold for deltaMin value. (12 bits)
+ * @dmin_strength: strength factor for deltaMin. (valid range is 0x00..0x10)
+ * @use_rgb_colorspace: use RGB instead of luminance/chrominance colorspace.
+ * @bypass_chroma_mapping: disable chrominance mapping (only valid if
+ *                         use_rgb_colorspace = 0)
+ * @use_iref: use illumination reference instead of Y for tone mapping
+ *            and gain calculations.
+ */
+struct rkisp1_cif_isp_wdr_config {
+	struct rkisp1_cif_isp_wdr_tone_curve tone_curve;
+	struct rkisp1_cif_isp_wdr_iref_config iref_config;
+	__u16 rgb_offset;
+	__u16 luma_offset;
+	__u16 dmin_thresh;
+	__u8 dmin_strength;
+	__u8 use_rgb_colorspace;
+	__u8 bypass_chroma_mapping;
+	__u8 use_iref;
+};
+
 /*---------- PART2: Measurement Statistics ------------*/
 
 /**
@@ -1059,6 +1132,7 @@ struct rkisp1_stat_buffer {
  * @RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_BLS: BLS in the compand block
  * @RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_EXPAND: Companding expand curve
  * @RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_COMPRESS: Companding compress curve
+ * @RKISP1_EXT_PARAMS_BLOCK_TYPE_WDR: Wide dynamic range
  */
 enum rkisp1_ext_params_block_type {
 	RKISP1_EXT_PARAMS_BLOCK_TYPE_BLS,
@@ -1081,6 +1155,7 @@ enum rkisp1_ext_params_block_type {
 	RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_BLS,
 	RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_EXPAND,
 	RKISP1_EXT_PARAMS_BLOCK_TYPE_COMPAND_COMPRESS,
+	RKISP1_EXT_PARAMS_BLOCK_TYPE_WDR,
 };
 
 #define RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE	(1U << 0)
@@ -1463,6 +1538,23 @@ struct rkisp1_ext_params_compand_curve_config {
 	struct rkisp1_cif_isp_compand_curve_config config;
 } __attribute__((aligned(8)));
 
+/**
+ * struct rkisp1_ext_params_wdr_config - RkISP1 extensible params
+ *                                       Wide dynamic range config
+ *
+ * RkISP1 extensible parameters WDR block.
+ * Identified by :c:type:`RKISP1_EXT_PARAMS_BLOCK_TYPE_WDR`
+ *
+ * @header: The RkISP1 extensible parameters header, see
+ *	    :c:type:`rkisp1_ext_params_block_header`
+ * @config: WDR configuration, see
+ *	    :c:type:`rkisp1_cif_isp_wdr_config`
+ */
+struct rkisp1_ext_params_wdr_config {
+	struct rkisp1_ext_params_block_header header;
+	struct rkisp1_cif_isp_wdr_config config;
+} __attribute__((aligned(8)));
+
 /*
  * The rkisp1_ext_params_compand_curve_config structure is counted twice as it
  * is used for both the COMPAND_EXPAND and COMPAND_COMPRESS block types.
-- 
2.43.0



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] media: rkisp1: Cleanup error handling
  2025-05-22 15:08 ` [PATCH 1/3] media: rkisp1: Cleanup error handling Stefan Klug
@ 2025-05-22 15:42   ` Laurent Pinchart
  2025-05-22 16:30     ` Stefan Klug
  0 siblings, 1 reply; 11+ messages in thread
From: Laurent Pinchart @ 2025-05-22 15:42 UTC (permalink / raw)
  To: Stefan Klug
  Cc: linux-media, Dafna Hirschfeld, Mauro Carvalho Chehab,
	Heiko Stuebner, linux-rockchip, linux-arm-kernel, linux-kernel

Hi Stefan,

Thank you for the patch.

On Thu, May 22, 2025 at 05:08:38PM +0200, Stefan Klug wrote:
> Do not call media_entity_cleanup() when media_entity_pads_init() fails.

Why is it an issue ? The media_entity_cleanup() documentation clearly
states

 * Calling media_entity_cleanup() on a media_entity whose memory has been
 * zeroed but that has not been initialized with media_entity_pad_init() is
 * valid and is a no-op.

This is by design to simplify error handling in drivers.

> As a drive-by fix handle an (very unlikely) error in
> rkisp1_params_init_vb2_queue().
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  .../media/platform/rockchip/rkisp1/rkisp1-params.c    | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> index b28f4140c8a3..918eb06c7465 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> @@ -2763,7 +2763,9 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1)
>  	vdev->queue = &node->buf_queue;
>  	vdev->device_caps = V4L2_CAP_STREAMING | V4L2_CAP_META_OUTPUT;
>  	vdev->vfl_dir = VFL_DIR_TX;
> -	rkisp1_params_init_vb2_queue(vdev->queue, params);
> +	ret = rkisp1_params_init_vb2_queue(vdev->queue, params);
> +	if (ret)
> +		goto err_mutex;
>  
>  	params->metafmt = &rkisp1_params_formats[RKISP1_PARAMS_FIXED];
>  
> @@ -2777,19 +2779,20 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1)
>  	node->pad.flags = MEDIA_PAD_FL_SOURCE;
>  	ret = media_entity_pads_init(&vdev->entity, 1, &node->pad);
>  	if (ret)
> -		goto error;
> +		goto err_mutex;
>  
>  	ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
>  	if (ret) {
>  		dev_err(rkisp1->dev,
>  			"failed to register %s, ret=%d\n", vdev->name, ret);
> -		goto error;
> +		goto err_media;
>  	}
>  
>  	return 0;
>  
> -error:
> +err_media:
>  	media_entity_cleanup(&vdev->entity);
> +err_mutex:
>  	mutex_destroy(&node->vlock);
>  	return ret;
>  }

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] media: rkisp1: Add RKISP1_CID_SUPPORTED_PARAMS_BLOCKS control
  2025-05-22 15:08 ` [PATCH 2/3] media: rkisp1: Add RKISP1_CID_SUPPORTED_PARAMS_BLOCKS control Stefan Klug
@ 2025-05-22 15:56   ` Laurent Pinchart
  2025-05-22 16:36     ` Stefan Klug
  2025-05-23  2:19   ` Paul Elder
  1 sibling, 1 reply; 11+ messages in thread
From: Laurent Pinchart @ 2025-05-22 15:56 UTC (permalink / raw)
  To: Stefan Klug
  Cc: linux-media, Dafna Hirschfeld, Mauro Carvalho Chehab,
	Heiko Stuebner, linux-rockchip, linux-arm-kernel, linux-kernel,
	Hans Verkuil

Hi Stefan,

Thank you for the patch.

On Thu, May 22, 2025 at 05:08:39PM +0200, Stefan Klug wrote:
> Add a RKISP1_CID_SUPPORTED_PARAMS_BLOCKS V4L2 control to be able to
> query the parameters blocks supported by the current kernel on the
> current hardware from user space.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  .../platform/rockchip/rkisp1/rkisp1-common.h  |  2 +
>  .../platform/rockchip/rkisp1/rkisp1-params.c  | 50 ++++++++++++++++++-
>  include/uapi/linux/rkisp1-config.h            | 10 ++++
>  include/uapi/linux/v4l2-controls.h            |  6 +++
>  4 files changed, 67 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> index ca952fd0829b..5f187f9efc7b 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> @@ -415,6 +415,8 @@ struct rkisp1_params {
>  	spinlock_t config_lock; /* locks the buffers list 'params' */
>  	struct list_head params;
>  
> +	struct v4l2_ctrl_handler ctrls;
> +
>  	const struct v4l2_meta_format *metafmt;
>  
>  	enum v4l2_quantization quantization;
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> index 918eb06c7465..60c9b3c46593 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> @@ -2736,6 +2736,45 @@ static int rkisp1_params_init_vb2_queue(struct vb2_queue *q,
>  	return vb2_queue_init(q);
>  }
>  
> +static int rkisp1_ctrl_init(struct rkisp1_params *params)
> +{
> +	int ret;
> +
> +	v4l2_ctrl_handler_init(&params->ctrls, 1);
> +
> +	struct v4l2_ctrl_config ctrl_config = {
> +		.id = RKISP1_CID_SUPPORTED_PARAMS_BLOCKS,
> +		.name = "Supported Params Blocks",
> +		.type = V4L2_CTRL_TYPE_BITMASK,
> +		.flags = V4L2_CTRL_FLAG_READ_ONLY,
> +	};

	struct v4l2_ctrl_config ctrl_config = {
		.id = RKISP1_CID_SUPPORTED_PARAMS_BLOCKS,
		.name = "Supported Params Blocks",
		.type = V4L2_CTRL_TYPE_BITMASK,
		.flags = V4L2_CTRL_FLAG_READ_ONLY,
	};
	int ret;

	v4l2_ctrl_handler_init(&params->ctrls, 1);

Mixing code and variable declarations is still usually frown upon in the
kernel.

> +
> +	for (unsigned int i = 0; i < ARRAY_SIZE(rkisp1_ext_params_handlers); i++) {
> +		const struct rkisp1_ext_params_handler *block_handler;
> +
> +		block_handler = &rkisp1_ext_params_handlers[i];
> +		ctrl_config.max |= BIT(i);
> +
> +		if ((params->rkisp1->info->features & block_handler->features) !=
> +		    block_handler->features)
> +			continue;
> +
> +		ctrl_config.def |= BIT(i);
> +	}
> +
> +	v4l2_ctrl_new_custom(&params->ctrls, &ctrl_config, NULL);
> +
> +	params->vnode.vdev.ctrl_handler = &params->ctrls;
> +
> +	if (params->ctrls.error) {
> +		ret = params->ctrls.error;
> +		v4l2_ctrl_handler_free(&params->ctrls);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  int rkisp1_params_register(struct rkisp1_device *rkisp1)
>  {
>  	struct rkisp1_params *params = &rkisp1->params;
> @@ -2776,10 +2815,16 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1)
>  
>  	video_set_drvdata(vdev, params);
>  
> +	ret = rkisp1_ctrl_init(params);
> +	if (ret) {
> +		dev_err(rkisp1->dev, "Control initialization error %d\n", ret);
> +		goto err_mutex;
> +	}
> +
>  	node->pad.flags = MEDIA_PAD_FL_SOURCE;
>  	ret = media_entity_pads_init(&vdev->entity, 1, &node->pad);
>  	if (ret)
> -		goto err_mutex;
> +		goto err_ctrl;
>  
>  	ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
>  	if (ret) {
> @@ -2792,6 +2837,8 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1)
>  
>  err_media:
>  	media_entity_cleanup(&vdev->entity);
> +err_ctrl:
> +	v4l2_ctrl_handler_free(&params->ctrls);
>  err_mutex:
>  	mutex_destroy(&node->vlock);
>  	return ret;
> @@ -2808,5 +2855,6 @@ void rkisp1_params_unregister(struct rkisp1_device *rkisp1)
>  
>  	vb2_video_unregister_device(vdev);
>  	media_entity_cleanup(&vdev->entity);
> +	v4l2_ctrl_handler_free(&params->ctrls);
>  	mutex_destroy(&node->vlock);
>  }
> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> index 2d995f3c1ca3..4fc8f221d0c4 100644
> --- a/include/uapi/linux/rkisp1-config.h
> +++ b/include/uapi/linux/rkisp1-config.h
> @@ -1086,6 +1086,9 @@ enum rkisp1_ext_params_block_type {
>  #define RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE	(1U << 0)
>  #define RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE	(1U << 1)
>  
> +/* A bitmask of parameters blocks supported on the current hardware. */
> +#define RKISP1_CID_SUPPORTED_PARAMS_BLOCKS	(V4L2_CID_USER_RKISP1_BASE + 0x01)
> +
>  /**
>   * struct rkisp1_ext_params_block_header - RkISP1 extensible parameters block
>   *					   header
> @@ -1520,6 +1523,13 @@ enum rksip1_ext_param_buffer_version {
>   * V4L2 control. If such control is not available, userspace should assume only
>   * RKISP1_EXT_PARAM_BUFFER_V1 is supported by the driver.
>   *
> + * The read-only V4L2 control ``RKISP1_CID_SUPPORTED_PARAMS_BLOCKS`` can be used
> + * to query the blocks supported by the current hardware. It contains a bitmask

s/current hardware/device/

> + * where each bit represents the availability of the corresponding entry
> + * from the :c:type:`rkisp1_ext_params_block_type` enum. The max value of the
> + * control represents the blocks supported by the current kernel (independent of
> + * the current hardware).

 * from the :c:type:`rkisp1_ext_params_block_type` enum. The current and default
 * values of the control represents the blocks supported by the device instance,
 * while the maximum value represents the blocks supported by the kernel driver,
 * independently of the device instance.

I was going to say that the control should be documented in
Documentation/userspace-api/drivers/rkisp1.rst, but rkisp1-config.h is
pulled in the documentation tree by
Documentation/userspace-api/media/v4l/metafmt-rkisp1.rst, so I'm OK with
this. Hans, Mauro, are you fine as well with documenting the control
here ?

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

> + *
>   * For each ISP block that userspace wants to configure, a block-specific
>   * structure is appended to the @data buffer, one after the other without gaps
>   * in between nor overlaps. Userspace shall populate the @data_size field with
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 72e32814ea83..f836512e9deb 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -222,6 +222,12 @@ enum v4l2_colorfx {
>   */
>  #define V4L2_CID_USER_UVC_BASE			(V4L2_CID_USER_BASE + 0x11e0)
>  
> +/*
> + * The base for Rockchip ISP1 driver controls.
> + * We reserve 16 controls for this driver.
> + */
> +#define V4L2_CID_USER_RKISP1_BASE		(V4L2_CID_USER_BASE + 0x1220)
> +
>  /* MPEG-class control IDs */
>  /* The MPEG controls are applicable to all codec controls
>   * and the 'MPEG' part of the define is historical */

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] media: rkisp1: Cleanup error handling
  2025-05-22 15:42   ` Laurent Pinchart
@ 2025-05-22 16:30     ` Stefan Klug
  0 siblings, 0 replies; 11+ messages in thread
From: Stefan Klug @ 2025-05-22 16:30 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: linux-media, Dafna Hirschfeld, Mauro Carvalho Chehab,
	Heiko Stuebner, linux-rockchip, linux-arm-kernel, linux-kernel

Hi Laurent,

Thank you for the review.

Quoting Laurent Pinchart (2025-05-22 17:42:10)
> Hi Stefan,
> 
> Thank you for the patch.
> 
> On Thu, May 22, 2025 at 05:08:38PM +0200, Stefan Klug wrote:
> > Do not call media_entity_cleanup() when media_entity_pads_init() fails.
> 
> Why is it an issue ? The media_entity_cleanup() documentation clearly
> states
> 
>  * Calling media_entity_cleanup() on a media_entity whose memory has been
>  * zeroed but that has not been initialized with media_entity_pad_init() is
>  * valid and is a no-op.
> 
> This is by design to simplify error handling in drivers.

Oops. It was simple mechanical thing. I'll remove it in v2.

Regrads,
Stefan

> 
> > As a drive-by fix handle an (very unlikely) error in
> > rkisp1_params_init_vb2_queue().
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >  .../media/platform/rockchip/rkisp1/rkisp1-params.c    | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > index b28f4140c8a3..918eb06c7465 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > @@ -2763,7 +2763,9 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1)
> >       vdev->queue = &node->buf_queue;
> >       vdev->device_caps = V4L2_CAP_STREAMING | V4L2_CAP_META_OUTPUT;
> >       vdev->vfl_dir = VFL_DIR_TX;
> > -     rkisp1_params_init_vb2_queue(vdev->queue, params);
> > +     ret = rkisp1_params_init_vb2_queue(vdev->queue, params);
> > +     if (ret)
> > +             goto err_mutex;
> >  
> >       params->metafmt = &rkisp1_params_formats[RKISP1_PARAMS_FIXED];
> >  
> > @@ -2777,19 +2779,20 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1)
> >       node->pad.flags = MEDIA_PAD_FL_SOURCE;
> >       ret = media_entity_pads_init(&vdev->entity, 1, &node->pad);
> >       if (ret)
> > -             goto error;
> > +             goto err_mutex;
> >  
> >       ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
> >       if (ret) {
> >               dev_err(rkisp1->dev,
> >                       "failed to register %s, ret=%d\n", vdev->name, ret);
> > -             goto error;
> > +             goto err_media;
> >       }
> >  
> >       return 0;
> >  
> > -error:
> > +err_media:
> >       media_entity_cleanup(&vdev->entity);
> > +err_mutex:
> >       mutex_destroy(&node->vlock);
> >       return ret;
> >  }
> 
> -- 
> Regards,
> 
> Laurent Pinchart
>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] media: rkisp1: Add RKISP1_CID_SUPPORTED_PARAMS_BLOCKS control
  2025-05-22 15:56   ` Laurent Pinchart
@ 2025-05-22 16:36     ` Stefan Klug
  2025-05-22 16:41       ` Laurent Pinchart
  0 siblings, 1 reply; 11+ messages in thread
From: Stefan Klug @ 2025-05-22 16:36 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: linux-media, Dafna Hirschfeld, Mauro Carvalho Chehab,
	Heiko Stuebner, linux-rockchip, linux-arm-kernel, linux-kernel,
	Hans Verkuil

Hi Laurent,

Thank you for the review.

Quoting Laurent Pinchart (2025-05-22 17:56:41)
> Hi Stefan,
> 
> Thank you for the patch.
> 
> On Thu, May 22, 2025 at 05:08:39PM +0200, Stefan Klug wrote:
> > Add a RKISP1_CID_SUPPORTED_PARAMS_BLOCKS V4L2 control to be able to
> > query the parameters blocks supported by the current kernel on the
> > current hardware from user space.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >  .../platform/rockchip/rkisp1/rkisp1-common.h  |  2 +
> >  .../platform/rockchip/rkisp1/rkisp1-params.c  | 50 ++++++++++++++++++-
> >  include/uapi/linux/rkisp1-config.h            | 10 ++++
> >  include/uapi/linux/v4l2-controls.h            |  6 +++
> >  4 files changed, 67 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > index ca952fd0829b..5f187f9efc7b 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > @@ -415,6 +415,8 @@ struct rkisp1_params {
> >       spinlock_t config_lock; /* locks the buffers list 'params' */
> >       struct list_head params;
> >  
> > +     struct v4l2_ctrl_handler ctrls;
> > +
> >       const struct v4l2_meta_format *metafmt;
> >  
> >       enum v4l2_quantization quantization;
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > index 918eb06c7465..60c9b3c46593 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > @@ -2736,6 +2736,45 @@ static int rkisp1_params_init_vb2_queue(struct vb2_queue *q,
> >       return vb2_queue_init(q);
> >  }
> >  
> > +static int rkisp1_ctrl_init(struct rkisp1_params *params)
> > +{
> > +     int ret;
> > +
> > +     v4l2_ctrl_handler_init(&params->ctrls, 1);
> > +
> > +     struct v4l2_ctrl_config ctrl_config = {
> > +             .id = RKISP1_CID_SUPPORTED_PARAMS_BLOCKS,
> > +             .name = "Supported Params Blocks",
> > +             .type = V4L2_CTRL_TYPE_BITMASK,
> > +             .flags = V4L2_CTRL_FLAG_READ_ONLY,
> > +     };
> 
>         struct v4l2_ctrl_config ctrl_config = {
>                 .id = RKISP1_CID_SUPPORTED_PARAMS_BLOCKS,
>                 .name = "Supported Params Blocks",
>                 .type = V4L2_CTRL_TYPE_BITMASK,
>                 .flags = V4L2_CTRL_FLAG_READ_ONLY,
>         };
>         int ret;
> 
>         v4l2_ctrl_handler_init(&params->ctrls, 1);
> 
> Mixing code and variable declarations is still usually frown upon in the
> kernel.

I thought frown upon is not a no. And as this structure is not yet
complete and is modified afterwards it feels natural to me to put it
close to that place. But I can move it above the function. You decide.

> 
> > +
> > +     for (unsigned int i = 0; i < ARRAY_SIZE(rkisp1_ext_params_handlers); i++) {
> > +             const struct rkisp1_ext_params_handler *block_handler;
> > +
> > +             block_handler = &rkisp1_ext_params_handlers[i];
> > +             ctrl_config.max |= BIT(i);
> > +
> > +             if ((params->rkisp1->info->features & block_handler->features) !=
> > +                 block_handler->features)
> > +                     continue;
> > +
> > +             ctrl_config.def |= BIT(i);
> > +     }
> > +
> > +     v4l2_ctrl_new_custom(&params->ctrls, &ctrl_config, NULL);
> > +
> > +     params->vnode.vdev.ctrl_handler = &params->ctrls;
> > +
> > +     if (params->ctrls.error) {
> > +             ret = params->ctrls.error;
> > +             v4l2_ctrl_handler_free(&params->ctrls);
> > +             return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> >  int rkisp1_params_register(struct rkisp1_device *rkisp1)
> >  {
> >       struct rkisp1_params *params = &rkisp1->params;
> > @@ -2776,10 +2815,16 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1)
> >  
> >       video_set_drvdata(vdev, params);
> >  
> > +     ret = rkisp1_ctrl_init(params);
> > +     if (ret) {
> > +             dev_err(rkisp1->dev, "Control initialization error %d\n", ret);
> > +             goto err_mutex;
> > +     }
> > +
> >       node->pad.flags = MEDIA_PAD_FL_SOURCE;
> >       ret = media_entity_pads_init(&vdev->entity, 1, &node->pad);
> >       if (ret)
> > -             goto err_mutex;
> > +             goto err_ctrl;
> >  
> >       ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
> >       if (ret) {
> > @@ -2792,6 +2837,8 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1)
> >  
> >  err_media:
> >       media_entity_cleanup(&vdev->entity);
> > +err_ctrl:
> > +     v4l2_ctrl_handler_free(&params->ctrls);
> >  err_mutex:
> >       mutex_destroy(&node->vlock);
> >       return ret;
> > @@ -2808,5 +2855,6 @@ void rkisp1_params_unregister(struct rkisp1_device *rkisp1)
> >  
> >       vb2_video_unregister_device(vdev);
> >       media_entity_cleanup(&vdev->entity);
> > +     v4l2_ctrl_handler_free(&params->ctrls);
> >       mutex_destroy(&node->vlock);
> >  }
> > diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> > index 2d995f3c1ca3..4fc8f221d0c4 100644
> > --- a/include/uapi/linux/rkisp1-config.h
> > +++ b/include/uapi/linux/rkisp1-config.h
> > @@ -1086,6 +1086,9 @@ enum rkisp1_ext_params_block_type {
> >  #define RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE   (1U << 0)
> >  #define RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE    (1U << 1)
> >  
> > +/* A bitmask of parameters blocks supported on the current hardware. */
> > +#define RKISP1_CID_SUPPORTED_PARAMS_BLOCKS   (V4L2_CID_USER_RKISP1_BASE + 0x01)
> > +
> >  /**
> >   * struct rkisp1_ext_params_block_header - RkISP1 extensible parameters block
> >   *                                      header
> > @@ -1520,6 +1523,13 @@ enum rksip1_ext_param_buffer_version {
> >   * V4L2 control. If such control is not available, userspace should assume only
> >   * RKISP1_EXT_PARAM_BUFFER_V1 is supported by the driver.
> >   *
> > + * The read-only V4L2 control ``RKISP1_CID_SUPPORTED_PARAMS_BLOCKS`` can be used
> > + * to query the blocks supported by the current hardware. It contains a bitmask
> 
> s/current hardware/device/
> 
> > + * where each bit represents the availability of the corresponding entry
> > + * from the :c:type:`rkisp1_ext_params_block_type` enum. The max value of the
> > + * control represents the blocks supported by the current kernel (independent of
> > + * the current hardware).
> 
>  * from the :c:type:`rkisp1_ext_params_block_type` enum. The current and default
>  * values of the control represents the blocks supported by the device instance,
>  * while the maximum value represents the blocks supported by the kernel driver,
>  * independently of the device instance.
> 
> I was going to say that the control should be documented in
> Documentation/userspace-api/drivers/rkisp1.rst, but rkisp1-config.h is
> pulled in the documentation tree by
> Documentation/userspace-api/media/v4l/metafmt-rkisp1.rst, so I'm OK with
> this. Hans, Mauro, are you fine as well with documenting the control
> here ?

Looking at the docs, I realized that most people will read the already
existing docs. So creating a completely new file just for the single
control didn't feel good. As you like.

Regards,
Stefan

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > + *
> >   * For each ISP block that userspace wants to configure, a block-specific
> >   * structure is appended to the @data buffer, one after the other without gaps
> >   * in between nor overlaps. Userspace shall populate the @data_size field with
> > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > index 72e32814ea83..f836512e9deb 100644
> > --- a/include/uapi/linux/v4l2-controls.h
> > +++ b/include/uapi/linux/v4l2-controls.h
> > @@ -222,6 +222,12 @@ enum v4l2_colorfx {
> >   */
> >  #define V4L2_CID_USER_UVC_BASE                       (V4L2_CID_USER_BASE + 0x11e0)
> >  
> > +/*
> > + * The base for Rockchip ISP1 driver controls.
> > + * We reserve 16 controls for this driver.
> > + */
> > +#define V4L2_CID_USER_RKISP1_BASE            (V4L2_CID_USER_BASE + 0x1220)
> > +
> >  /* MPEG-class control IDs */
> >  /* The MPEG controls are applicable to all codec controls
> >   * and the 'MPEG' part of the define is historical */
> 
> -- 
> Regards,
> 
> Laurent Pinchart
>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] media: rkisp1: Add RKISP1_CID_SUPPORTED_PARAMS_BLOCKS control
  2025-05-22 16:36     ` Stefan Klug
@ 2025-05-22 16:41       ` Laurent Pinchart
  2025-05-22 17:10         ` Stefan Klug
  0 siblings, 1 reply; 11+ messages in thread
From: Laurent Pinchart @ 2025-05-22 16:41 UTC (permalink / raw)
  To: Stefan Klug
  Cc: linux-media, Dafna Hirschfeld, Mauro Carvalho Chehab,
	Heiko Stuebner, linux-rockchip, linux-arm-kernel, linux-kernel,
	Hans Verkuil

On Thu, May 22, 2025 at 06:36:06PM +0200, Stefan Klug wrote:
> Hi Laurent,
> 
> Thank you for the review.
> 
> Quoting Laurent Pinchart (2025-05-22 17:56:41)
> > Hi Stefan,
> > 
> > Thank you for the patch.
> > 
> > On Thu, May 22, 2025 at 05:08:39PM +0200, Stefan Klug wrote:
> > > Add a RKISP1_CID_SUPPORTED_PARAMS_BLOCKS V4L2 control to be able to
> > > query the parameters blocks supported by the current kernel on the
> > > current hardware from user space.
> > > 
> > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > ---
> > >  .../platform/rockchip/rkisp1/rkisp1-common.h  |  2 +
> > >  .../platform/rockchip/rkisp1/rkisp1-params.c  | 50 ++++++++++++++++++-
> > >  include/uapi/linux/rkisp1-config.h            | 10 ++++
> > >  include/uapi/linux/v4l2-controls.h            |  6 +++
> > >  4 files changed, 67 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > > index ca952fd0829b..5f187f9efc7b 100644
> > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > > @@ -415,6 +415,8 @@ struct rkisp1_params {
> > >       spinlock_t config_lock; /* locks the buffers list 'params' */
> > >       struct list_head params;
> > >  
> > > +     struct v4l2_ctrl_handler ctrls;
> > > +
> > >       const struct v4l2_meta_format *metafmt;
> > >  
> > >       enum v4l2_quantization quantization;
> > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > > index 918eb06c7465..60c9b3c46593 100644
> > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > > @@ -2736,6 +2736,45 @@ static int rkisp1_params_init_vb2_queue(struct vb2_queue *q,
> > >       return vb2_queue_init(q);
> > >  }
> > >  
> > > +static int rkisp1_ctrl_init(struct rkisp1_params *params)
> > > +{
> > > +     int ret;
> > > +
> > > +     v4l2_ctrl_handler_init(&params->ctrls, 1);
> > > +
> > > +     struct v4l2_ctrl_config ctrl_config = {
> > > +             .id = RKISP1_CID_SUPPORTED_PARAMS_BLOCKS,
> > > +             .name = "Supported Params Blocks",
> > > +             .type = V4L2_CTRL_TYPE_BITMASK,
> > > +             .flags = V4L2_CTRL_FLAG_READ_ONLY,
> > > +     };
> > 
> >         struct v4l2_ctrl_config ctrl_config = {
> >                 .id = RKISP1_CID_SUPPORTED_PARAMS_BLOCKS,
> >                 .name = "Supported Params Blocks",
> >                 .type = V4L2_CTRL_TYPE_BITMASK,
> >                 .flags = V4L2_CTRL_FLAG_READ_ONLY,
> >         };
> >         int ret;
> > 
> >         v4l2_ctrl_handler_init(&params->ctrls, 1);
> > 
> > Mixing code and variable declarations is still usually frown upon in the
> > kernel.
> 
> I thought frown upon is not a no. And as this structure is not yet
> complete and is modified afterwards it feels natural to me to put it
> close to that place. But I can move it above the function. You decide.

It obviously take someone to make the first move for things to change
:-) I however tend to favour consistency in coding style within a
driver.

> > > +
> > > +     for (unsigned int i = 0; i < ARRAY_SIZE(rkisp1_ext_params_handlers); i++) {
> > > +             const struct rkisp1_ext_params_handler *block_handler;
> > > +
> > > +             block_handler = &rkisp1_ext_params_handlers[i];
> > > +             ctrl_config.max |= BIT(i);
> > > +
> > > +             if ((params->rkisp1->info->features & block_handler->features) !=
> > > +                 block_handler->features)
> > > +                     continue;
> > > +
> > > +             ctrl_config.def |= BIT(i);
> > > +     }
> > > +
> > > +     v4l2_ctrl_new_custom(&params->ctrls, &ctrl_config, NULL);
> > > +
> > > +     params->vnode.vdev.ctrl_handler = &params->ctrls;
> > > +
> > > +     if (params->ctrls.error) {
> > > +             ret = params->ctrls.error;
> > > +             v4l2_ctrl_handler_free(&params->ctrls);
> > > +             return ret;
> > > +     }
> > > +
> > > +     return 0;
> > > +}
> > > +
> > >  int rkisp1_params_register(struct rkisp1_device *rkisp1)
> > >  {
> > >       struct rkisp1_params *params = &rkisp1->params;
> > > @@ -2776,10 +2815,16 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1)
> > >  
> > >       video_set_drvdata(vdev, params);
> > >  
> > > +     ret = rkisp1_ctrl_init(params);
> > > +     if (ret) {
> > > +             dev_err(rkisp1->dev, "Control initialization error %d\n", ret);
> > > +             goto err_mutex;
> > > +     }
> > > +
> > >       node->pad.flags = MEDIA_PAD_FL_SOURCE;
> > >       ret = media_entity_pads_init(&vdev->entity, 1, &node->pad);
> > >       if (ret)
> > > -             goto err_mutex;
> > > +             goto err_ctrl;
> > >  
> > >       ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
> > >       if (ret) {
> > > @@ -2792,6 +2837,8 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1)
> > >  
> > >  err_media:
> > >       media_entity_cleanup(&vdev->entity);
> > > +err_ctrl:
> > > +     v4l2_ctrl_handler_free(&params->ctrls);
> > >  err_mutex:
> > >       mutex_destroy(&node->vlock);
> > >       return ret;
> > > @@ -2808,5 +2855,6 @@ void rkisp1_params_unregister(struct rkisp1_device *rkisp1)
> > >  
> > >       vb2_video_unregister_device(vdev);
> > >       media_entity_cleanup(&vdev->entity);
> > > +     v4l2_ctrl_handler_free(&params->ctrls);
> > >       mutex_destroy(&node->vlock);
> > >  }
> > > diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> > > index 2d995f3c1ca3..4fc8f221d0c4 100644
> > > --- a/include/uapi/linux/rkisp1-config.h
> > > +++ b/include/uapi/linux/rkisp1-config.h
> > > @@ -1086,6 +1086,9 @@ enum rkisp1_ext_params_block_type {
> > >  #define RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE   (1U << 0)
> > >  #define RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE    (1U << 1)
> > >  
> > > +/* A bitmask of parameters blocks supported on the current hardware. */
> > > +#define RKISP1_CID_SUPPORTED_PARAMS_BLOCKS   (V4L2_CID_USER_RKISP1_BASE + 0x01)
> > > +
> > >  /**
> > >   * struct rkisp1_ext_params_block_header - RkISP1 extensible parameters block
> > >   *                                      header
> > > @@ -1520,6 +1523,13 @@ enum rksip1_ext_param_buffer_version {
> > >   * V4L2 control. If such control is not available, userspace should assume only
> > >   * RKISP1_EXT_PARAM_BUFFER_V1 is supported by the driver.
> > >   *
> > > + * The read-only V4L2 control ``RKISP1_CID_SUPPORTED_PARAMS_BLOCKS`` can be used
> > > + * to query the blocks supported by the current hardware. It contains a bitmask
> > 
> > s/current hardware/device/
> > 
> > > + * where each bit represents the availability of the corresponding entry
> > > + * from the :c:type:`rkisp1_ext_params_block_type` enum. The max value of the
> > > + * control represents the blocks supported by the current kernel (independent of
> > > + * the current hardware).
> > 
> >  * from the :c:type:`rkisp1_ext_params_block_type` enum. The current and default
> >  * values of the control represents the blocks supported by the device instance,
> >  * while the maximum value represents the blocks supported by the kernel driver,
> >  * independently of the device instance.
> > 
> > I was going to say that the control should be documented in
> > Documentation/userspace-api/drivers/rkisp1.rst, but rkisp1-config.h is
> > pulled in the documentation tree by
> > Documentation/userspace-api/media/v4l/metafmt-rkisp1.rst, so I'm OK with
> > this. Hans, Mauro, are you fine as well with documenting the control
> > here ?
> 
> Looking at the docs, I realized that most people will read the already
> existing docs. So creating a completely new file just for the single
> control didn't feel good. As you like.
> 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > 
> > > + *
> > >   * For each ISP block that userspace wants to configure, a block-specific
> > >   * structure is appended to the @data buffer, one after the other without gaps
> > >   * in between nor overlaps. Userspace shall populate the @data_size field with
> > > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > > index 72e32814ea83..f836512e9deb 100644
> > > --- a/include/uapi/linux/v4l2-controls.h
> > > +++ b/include/uapi/linux/v4l2-controls.h
> > > @@ -222,6 +222,12 @@ enum v4l2_colorfx {
> > >   */
> > >  #define V4L2_CID_USER_UVC_BASE                       (V4L2_CID_USER_BASE + 0x11e0)
> > >  
> > > +/*
> > > + * The base for Rockchip ISP1 driver controls.
> > > + * We reserve 16 controls for this driver.
> > > + */
> > > +#define V4L2_CID_USER_RKISP1_BASE            (V4L2_CID_USER_BASE + 0x1220)
> > > +
> > >  /* MPEG-class control IDs */
> > >  /* The MPEG controls are applicable to all codec controls
> > >   * and the 'MPEG' part of the define is historical */

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] media: rkisp1: Add RKISP1_CID_SUPPORTED_PARAMS_BLOCKS control
  2025-05-22 16:41       ` Laurent Pinchart
@ 2025-05-22 17:10         ` Stefan Klug
  2025-05-22 21:30           ` Laurent Pinchart
  0 siblings, 1 reply; 11+ messages in thread
From: Stefan Klug @ 2025-05-22 17:10 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: linux-media, Dafna Hirschfeld, Mauro Carvalho Chehab,
	Heiko Stuebner, linux-rockchip, linux-arm-kernel, linux-kernel,
	Hans Verkuil

Quoting Laurent Pinchart (2025-05-22 18:41:47)
> On Thu, May 22, 2025 at 06:36:06PM +0200, Stefan Klug wrote:
> > Hi Laurent,
> > 
> > Thank you for the review.
> > 
> > Quoting Laurent Pinchart (2025-05-22 17:56:41)
> > > Hi Stefan,
> > > 
> > > Thank you for the patch.
> > > 
> > > On Thu, May 22, 2025 at 05:08:39PM +0200, Stefan Klug wrote:
> > > > Add a RKISP1_CID_SUPPORTED_PARAMS_BLOCKS V4L2 control to be able to
> > > > query the parameters blocks supported by the current kernel on the
> > > > current hardware from user space.
> > > > 
> > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > > ---
> > > >  .../platform/rockchip/rkisp1/rkisp1-common.h  |  2 +
> > > >  .../platform/rockchip/rkisp1/rkisp1-params.c  | 50 ++++++++++++++++++-
> > > >  include/uapi/linux/rkisp1-config.h            | 10 ++++
> > > >  include/uapi/linux/v4l2-controls.h            |  6 +++
> > > >  4 files changed, 67 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > > > index ca952fd0829b..5f187f9efc7b 100644
> > > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > > > @@ -415,6 +415,8 @@ struct rkisp1_params {
> > > >       spinlock_t config_lock; /* locks the buffers list 'params' */
> > > >       struct list_head params;
> > > >  
> > > > +     struct v4l2_ctrl_handler ctrls;
> > > > +
> > > >       const struct v4l2_meta_format *metafmt;
> > > >  
> > > >       enum v4l2_quantization quantization;
> > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > > > index 918eb06c7465..60c9b3c46593 100644
> > > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > > > @@ -2736,6 +2736,45 @@ static int rkisp1_params_init_vb2_queue(struct vb2_queue *q,
> > > >       return vb2_queue_init(q);
> > > >  }
> > > >  
> > > > +static int rkisp1_ctrl_init(struct rkisp1_params *params)
> > > > +{
> > > > +     int ret;
> > > > +
> > > > +     v4l2_ctrl_handler_init(&params->ctrls, 1);
> > > > +
> > > > +     struct v4l2_ctrl_config ctrl_config = {
> > > > +             .id = RKISP1_CID_SUPPORTED_PARAMS_BLOCKS,
> > > > +             .name = "Supported Params Blocks",
> > > > +             .type = V4L2_CTRL_TYPE_BITMASK,
> > > > +             .flags = V4L2_CTRL_FLAG_READ_ONLY,
> > > > +     };
> > > 
> > >         struct v4l2_ctrl_config ctrl_config = {
> > >                 .id = RKISP1_CID_SUPPORTED_PARAMS_BLOCKS,
> > >                 .name = "Supported Params Blocks",
> > >                 .type = V4L2_CTRL_TYPE_BITMASK,
> > >                 .flags = V4L2_CTRL_FLAG_READ_ONLY,
> > >         };
> > >         int ret;
> > > 
> > >         v4l2_ctrl_handler_init(&params->ctrls, 1);
> > > 
> > > Mixing code and variable declarations is still usually frown upon in the
> > > kernel.
> > 
> > I thought frown upon is not a no. And as this structure is not yet
> > complete and is modified afterwards it feels natural to me to put it
> > close to that place. But I can move it above the function. You decide.
> 
> It obviously take someone to make the first move for things to change
> :-) I however tend to favour consistency in coding style within a
> driver.

Ahh, maybe I misread the comment. You meant to move it above ret. Not
outside the function. Right? That is easy to do. And I saw that pattern
in vivid-ctrls.c

Regards,
Stefan

> 
> > > > +
> > > > +     for (unsigned int i = 0; i < ARRAY_SIZE(rkisp1_ext_params_handlers); i++) {
> > > > +             const struct rkisp1_ext_params_handler *block_handler;
> > > > +
> > > > +             block_handler = &rkisp1_ext_params_handlers[i];
> > > > +             ctrl_config.max |= BIT(i);
> > > > +
> > > > +             if ((params->rkisp1->info->features & block_handler->features) !=
> > > > +                 block_handler->features)
> > > > +                     continue;
> > > > +
> > > > +             ctrl_config.def |= BIT(i);
> > > > +     }
> > > > +
> > > > +     v4l2_ctrl_new_custom(&params->ctrls, &ctrl_config, NULL);
> > > > +
> > > > +     params->vnode.vdev.ctrl_handler = &params->ctrls;
> > > > +
> > > > +     if (params->ctrls.error) {
> > > > +             ret = params->ctrls.error;
> > > > +             v4l2_ctrl_handler_free(&params->ctrls);
> > > > +             return ret;
> > > > +     }
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > >  int rkisp1_params_register(struct rkisp1_device *rkisp1)
> > > >  {
> > > >       struct rkisp1_params *params = &rkisp1->params;
> > > > @@ -2776,10 +2815,16 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1)
> > > >  
> > > >       video_set_drvdata(vdev, params);
> > > >  
> > > > +     ret = rkisp1_ctrl_init(params);
> > > > +     if (ret) {
> > > > +             dev_err(rkisp1->dev, "Control initialization error %d\n", ret);
> > > > +             goto err_mutex;
> > > > +     }
> > > > +
> > > >       node->pad.flags = MEDIA_PAD_FL_SOURCE;
> > > >       ret = media_entity_pads_init(&vdev->entity, 1, &node->pad);
> > > >       if (ret)
> > > > -             goto err_mutex;
> > > > +             goto err_ctrl;
> > > >  
> > > >       ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
> > > >       if (ret) {
> > > > @@ -2792,6 +2837,8 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1)
> > > >  
> > > >  err_media:
> > > >       media_entity_cleanup(&vdev->entity);
> > > > +err_ctrl:
> > > > +     v4l2_ctrl_handler_free(&params->ctrls);
> > > >  err_mutex:
> > > >       mutex_destroy(&node->vlock);
> > > >       return ret;
> > > > @@ -2808,5 +2855,6 @@ void rkisp1_params_unregister(struct rkisp1_device *rkisp1)
> > > >  
> > > >       vb2_video_unregister_device(vdev);
> > > >       media_entity_cleanup(&vdev->entity);
> > > > +     v4l2_ctrl_handler_free(&params->ctrls);
> > > >       mutex_destroy(&node->vlock);
> > > >  }
> > > > diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> > > > index 2d995f3c1ca3..4fc8f221d0c4 100644
> > > > --- a/include/uapi/linux/rkisp1-config.h
> > > > +++ b/include/uapi/linux/rkisp1-config.h
> > > > @@ -1086,6 +1086,9 @@ enum rkisp1_ext_params_block_type {
> > > >  #define RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE   (1U << 0)
> > > >  #define RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE    (1U << 1)
> > > >  
> > > > +/* A bitmask of parameters blocks supported on the current hardware. */
> > > > +#define RKISP1_CID_SUPPORTED_PARAMS_BLOCKS   (V4L2_CID_USER_RKISP1_BASE + 0x01)
> > > > +
> > > >  /**
> > > >   * struct rkisp1_ext_params_block_header - RkISP1 extensible parameters block
> > > >   *                                      header
> > > > @@ -1520,6 +1523,13 @@ enum rksip1_ext_param_buffer_version {
> > > >   * V4L2 control. If such control is not available, userspace should assume only
> > > >   * RKISP1_EXT_PARAM_BUFFER_V1 is supported by the driver.
> > > >   *
> > > > + * The read-only V4L2 control ``RKISP1_CID_SUPPORTED_PARAMS_BLOCKS`` can be used
> > > > + * to query the blocks supported by the current hardware. It contains a bitmask
> > > 
> > > s/current hardware/device/
> > > 
> > > > + * where each bit represents the availability of the corresponding entry
> > > > + * from the :c:type:`rkisp1_ext_params_block_type` enum. The max value of the
> > > > + * control represents the blocks supported by the current kernel (independent of
> > > > + * the current hardware).
> > > 
> > >  * from the :c:type:`rkisp1_ext_params_block_type` enum. The current and default
> > >  * values of the control represents the blocks supported by the device instance,
> > >  * while the maximum value represents the blocks supported by the kernel driver,
> > >  * independently of the device instance.
> > > 
> > > I was going to say that the control should be documented in
> > > Documentation/userspace-api/drivers/rkisp1.rst, but rkisp1-config.h is
> > > pulled in the documentation tree by
> > > Documentation/userspace-api/media/v4l/metafmt-rkisp1.rst, so I'm OK with
> > > this. Hans, Mauro, are you fine as well with documenting the control
> > > here ?
> > 
> > Looking at the docs, I realized that most people will read the already
> > existing docs. So creating a completely new file just for the single
> > control didn't feel good. As you like.
> > 
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > 
> > > > + *
> > > >   * For each ISP block that userspace wants to configure, a block-specific
> > > >   * structure is appended to the @data buffer, one after the other without gaps
> > > >   * in between nor overlaps. Userspace shall populate the @data_size field with
> > > > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > > > index 72e32814ea83..f836512e9deb 100644
> > > > --- a/include/uapi/linux/v4l2-controls.h
> > > > +++ b/include/uapi/linux/v4l2-controls.h
> > > > @@ -222,6 +222,12 @@ enum v4l2_colorfx {
> > > >   */
> > > >  #define V4L2_CID_USER_UVC_BASE                       (V4L2_CID_USER_BASE + 0x11e0)
> > > >  
> > > > +/*
> > > > + * The base for Rockchip ISP1 driver controls.
> > > > + * We reserve 16 controls for this driver.
> > > > + */
> > > > +#define V4L2_CID_USER_RKISP1_BASE            (V4L2_CID_USER_BASE + 0x1220)
> > > > +
> > > >  /* MPEG-class control IDs */
> > > >  /* The MPEG controls are applicable to all codec controls
> > > >   * and the 'MPEG' part of the define is historical */
> 
> -- 
> Regards,
> 
> Laurent Pinchart


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] media: rkisp1: Add RKISP1_CID_SUPPORTED_PARAMS_BLOCKS control
  2025-05-22 17:10         ` Stefan Klug
@ 2025-05-22 21:30           ` Laurent Pinchart
  0 siblings, 0 replies; 11+ messages in thread
From: Laurent Pinchart @ 2025-05-22 21:30 UTC (permalink / raw)
  To: Stefan Klug
  Cc: linux-media, Dafna Hirschfeld, Mauro Carvalho Chehab,
	Heiko Stuebner, linux-rockchip, linux-arm-kernel, linux-kernel,
	Hans Verkuil

On Thu, May 22, 2025 at 07:10:38PM +0200, Stefan Klug wrote:
> Quoting Laurent Pinchart (2025-05-22 18:41:47)
> > On Thu, May 22, 2025 at 06:36:06PM +0200, Stefan Klug wrote:
> > > Hi Laurent,
> > > 
> > > Thank you for the review.
> > > 
> > > Quoting Laurent Pinchart (2025-05-22 17:56:41)
> > > > Hi Stefan,
> > > > 
> > > > Thank you for the patch.
> > > > 
> > > > On Thu, May 22, 2025 at 05:08:39PM +0200, Stefan Klug wrote:
> > > > > Add a RKISP1_CID_SUPPORTED_PARAMS_BLOCKS V4L2 control to be able to
> > > > > query the parameters blocks supported by the current kernel on the
> > > > > current hardware from user space.
> > > > > 
> > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > > > ---
> > > > >  .../platform/rockchip/rkisp1/rkisp1-common.h  |  2 +
> > > > >  .../platform/rockchip/rkisp1/rkisp1-params.c  | 50 ++++++++++++++++++-
> > > > >  include/uapi/linux/rkisp1-config.h            | 10 ++++
> > > > >  include/uapi/linux/v4l2-controls.h            |  6 +++
> > > > >  4 files changed, 67 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > > > > index ca952fd0829b..5f187f9efc7b 100644
> > > > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > > > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > > > > @@ -415,6 +415,8 @@ struct rkisp1_params {
> > > > >       spinlock_t config_lock; /* locks the buffers list 'params' */
> > > > >       struct list_head params;
> > > > >  
> > > > > +     struct v4l2_ctrl_handler ctrls;
> > > > > +
> > > > >       const struct v4l2_meta_format *metafmt;
> > > > >  
> > > > >       enum v4l2_quantization quantization;
> > > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > > > > index 918eb06c7465..60c9b3c46593 100644
> > > > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > > > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > > > > @@ -2736,6 +2736,45 @@ static int rkisp1_params_init_vb2_queue(struct vb2_queue *q,
> > > > >       return vb2_queue_init(q);
> > > > >  }
> > > > >  
> > > > > +static int rkisp1_ctrl_init(struct rkisp1_params *params)
> > > > > +{
> > > > > +     int ret;
> > > > > +
> > > > > +     v4l2_ctrl_handler_init(&params->ctrls, 1);
> > > > > +
> > > > > +     struct v4l2_ctrl_config ctrl_config = {
> > > > > +             .id = RKISP1_CID_SUPPORTED_PARAMS_BLOCKS,
> > > > > +             .name = "Supported Params Blocks",
> > > > > +             .type = V4L2_CTRL_TYPE_BITMASK,
> > > > > +             .flags = V4L2_CTRL_FLAG_READ_ONLY,
> > > > > +     };
> > > > 
> > > >         struct v4l2_ctrl_config ctrl_config = {
> > > >                 .id = RKISP1_CID_SUPPORTED_PARAMS_BLOCKS,
> > > >                 .name = "Supported Params Blocks",
> > > >                 .type = V4L2_CTRL_TYPE_BITMASK,
> > > >                 .flags = V4L2_CTRL_FLAG_READ_ONLY,
> > > >         };
> > > >         int ret;
> > > > 
> > > >         v4l2_ctrl_handler_init(&params->ctrls, 1);
> > > > 
> > > > Mixing code and variable declarations is still usually frown upon in the
> > > > kernel.
> > > 
> > > I thought frown upon is not a no. And as this structure is not yet
> > > complete and is modified afterwards it feels natural to me to put it
> > > close to that place. But I can move it above the function. You decide.
> > 
> > It obviously take someone to make the first move for things to change
> > :-) I however tend to favour consistency in coding style within a
> > driver.
> 
> Ahh, maybe I misread the comment. You meant to move it above ret. Not
> outside the function. Right? That is easy to do. And I saw that pattern
> in vivid-ctrls.c

Yes, just above the ret.

static int rkisp1_ctrl_init(struct rkisp1_params *params)
{
	struct v4l2_ctrl_config ctrl_config = {
		.id = RKISP1_CID_SUPPORTED_PARAMS_BLOCKS,
		.name = "Supported Params Blocks",
		.type = V4L2_CTRL_TYPE_BITMASK,
		.flags = V4L2_CTRL_FLAG_READ_ONLY,
	};
	int ret;
 
	v4l2_ctrl_handler_init(&params->ctrls, 1);

	...

> > > > > +
> > > > > +     for (unsigned int i = 0; i < ARRAY_SIZE(rkisp1_ext_params_handlers); i++) {
> > > > > +             const struct rkisp1_ext_params_handler *block_handler;
> > > > > +
> > > > > +             block_handler = &rkisp1_ext_params_handlers[i];
> > > > > +             ctrl_config.max |= BIT(i);
> > > > > +
> > > > > +             if ((params->rkisp1->info->features & block_handler->features) !=
> > > > > +                 block_handler->features)
> > > > > +                     continue;
> > > > > +
> > > > > +             ctrl_config.def |= BIT(i);
> > > > > +     }
> > > > > +
> > > > > +     v4l2_ctrl_new_custom(&params->ctrls, &ctrl_config, NULL);
> > > > > +
> > > > > +     params->vnode.vdev.ctrl_handler = &params->ctrls;
> > > > > +
> > > > > +     if (params->ctrls.error) {
> > > > > +             ret = params->ctrls.error;
> > > > > +             v4l2_ctrl_handler_free(&params->ctrls);
> > > > > +             return ret;
> > > > > +     }
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > >  int rkisp1_params_register(struct rkisp1_device *rkisp1)
> > > > >  {
> > > > >       struct rkisp1_params *params = &rkisp1->params;
> > > > > @@ -2776,10 +2815,16 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1)
> > > > >  
> > > > >       video_set_drvdata(vdev, params);
> > > > >  
> > > > > +     ret = rkisp1_ctrl_init(params);
> > > > > +     if (ret) {
> > > > > +             dev_err(rkisp1->dev, "Control initialization error %d\n", ret);
> > > > > +             goto err_mutex;
> > > > > +     }
> > > > > +
> > > > >       node->pad.flags = MEDIA_PAD_FL_SOURCE;
> > > > >       ret = media_entity_pads_init(&vdev->entity, 1, &node->pad);
> > > > >       if (ret)
> > > > > -             goto err_mutex;
> > > > > +             goto err_ctrl;
> > > > >  
> > > > >       ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
> > > > >       if (ret) {
> > > > > @@ -2792,6 +2837,8 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1)
> > > > >  
> > > > >  err_media:
> > > > >       media_entity_cleanup(&vdev->entity);
> > > > > +err_ctrl:
> > > > > +     v4l2_ctrl_handler_free(&params->ctrls);
> > > > >  err_mutex:
> > > > >       mutex_destroy(&node->vlock);
> > > > >       return ret;
> > > > > @@ -2808,5 +2855,6 @@ void rkisp1_params_unregister(struct rkisp1_device *rkisp1)
> > > > >  
> > > > >       vb2_video_unregister_device(vdev);
> > > > >       media_entity_cleanup(&vdev->entity);
> > > > > +     v4l2_ctrl_handler_free(&params->ctrls);
> > > > >       mutex_destroy(&node->vlock);
> > > > >  }
> > > > > diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> > > > > index 2d995f3c1ca3..4fc8f221d0c4 100644
> > > > > --- a/include/uapi/linux/rkisp1-config.h
> > > > > +++ b/include/uapi/linux/rkisp1-config.h
> > > > > @@ -1086,6 +1086,9 @@ enum rkisp1_ext_params_block_type {
> > > > >  #define RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE   (1U << 0)
> > > > >  #define RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE    (1U << 1)
> > > > >  
> > > > > +/* A bitmask of parameters blocks supported on the current hardware. */
> > > > > +#define RKISP1_CID_SUPPORTED_PARAMS_BLOCKS   (V4L2_CID_USER_RKISP1_BASE + 0x01)
> > > > > +
> > > > >  /**
> > > > >   * struct rkisp1_ext_params_block_header - RkISP1 extensible parameters block
> > > > >   *                                      header
> > > > > @@ -1520,6 +1523,13 @@ enum rksip1_ext_param_buffer_version {
> > > > >   * V4L2 control. If such control is not available, userspace should assume only
> > > > >   * RKISP1_EXT_PARAM_BUFFER_V1 is supported by the driver.
> > > > >   *
> > > > > + * The read-only V4L2 control ``RKISP1_CID_SUPPORTED_PARAMS_BLOCKS`` can be used
> > > > > + * to query the blocks supported by the current hardware. It contains a bitmask
> > > > 
> > > > s/current hardware/device/
> > > > 
> > > > > + * where each bit represents the availability of the corresponding entry
> > > > > + * from the :c:type:`rkisp1_ext_params_block_type` enum. The max value of the
> > > > > + * control represents the blocks supported by the current kernel (independent of
> > > > > + * the current hardware).
> > > > 
> > > >  * from the :c:type:`rkisp1_ext_params_block_type` enum. The current and default
> > > >  * values of the control represents the blocks supported by the device instance,
> > > >  * while the maximum value represents the blocks supported by the kernel driver,
> > > >  * independently of the device instance.
> > > > 
> > > > I was going to say that the control should be documented in
> > > > Documentation/userspace-api/drivers/rkisp1.rst, but rkisp1-config.h is
> > > > pulled in the documentation tree by
> > > > Documentation/userspace-api/media/v4l/metafmt-rkisp1.rst, so I'm OK with
> > > > this. Hans, Mauro, are you fine as well with documenting the control
> > > > here ?
> > > 
> > > Looking at the docs, I realized that most people will read the already
> > > existing docs. So creating a completely new file just for the single
> > > control didn't feel good. As you like.
> > > 
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > 
> > > > > + *
> > > > >   * For each ISP block that userspace wants to configure, a block-specific
> > > > >   * structure is appended to the @data buffer, one after the other without gaps
> > > > >   * in between nor overlaps. Userspace shall populate the @data_size field with
> > > > > diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> > > > > index 72e32814ea83..f836512e9deb 100644
> > > > > --- a/include/uapi/linux/v4l2-controls.h
> > > > > +++ b/include/uapi/linux/v4l2-controls.h
> > > > > @@ -222,6 +222,12 @@ enum v4l2_colorfx {
> > > > >   */
> > > > >  #define V4L2_CID_USER_UVC_BASE                       (V4L2_CID_USER_BASE + 0x11e0)
> > > > >  
> > > > > +/*
> > > > > + * The base for Rockchip ISP1 driver controls.
> > > > > + * We reserve 16 controls for this driver.
> > > > > + */
> > > > > +#define V4L2_CID_USER_RKISP1_BASE            (V4L2_CID_USER_BASE + 0x1220)
> > > > > +
> > > > >  /* MPEG-class control IDs */
> > > > >  /* The MPEG controls are applicable to all codec controls
> > > > >   * and the 'MPEG' part of the define is historical */

-- 
Regards,

Laurent Pinchart


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] media: rkisp1: Add RKISP1_CID_SUPPORTED_PARAMS_BLOCKS control
  2025-05-22 15:08 ` [PATCH 2/3] media: rkisp1: Add RKISP1_CID_SUPPORTED_PARAMS_BLOCKS control Stefan Klug
  2025-05-22 15:56   ` Laurent Pinchart
@ 2025-05-23  2:19   ` Paul Elder
  1 sibling, 0 replies; 11+ messages in thread
From: Paul Elder @ 2025-05-23  2:19 UTC (permalink / raw)
  To: Dafna Hirschfeld, Heiko Stuebner, Laurent Pinchart,
	Mauro Carvalho Chehab, Stefan Klug, linux-media
  Cc: Stefan Klug, linux-rockchip, linux-arm-kernel, linux-kernel

Quoting Stefan Klug (2025-05-22 17:08:39)
> Add a RKISP1_CID_SUPPORTED_PARAMS_BLOCKS V4L2 control to be able to
> query the parameters blocks supported by the current kernel on the
> current hardware from user space.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  .../platform/rockchip/rkisp1/rkisp1-common.h  |  2 +
>  .../platform/rockchip/rkisp1/rkisp1-params.c  | 50 ++++++++++++++++++-
>  include/uapi/linux/rkisp1-config.h            | 10 ++++
>  include/uapi/linux/v4l2-controls.h            |  6 +++
>  4 files changed, 67 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> index ca952fd0829b..5f187f9efc7b 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> @@ -415,6 +415,8 @@ struct rkisp1_params {
>         spinlock_t config_lock; /* locks the buffers list 'params' */
>         struct list_head params;
>  
> +       struct v4l2_ctrl_handler ctrls;
> +
>         const struct v4l2_meta_format *metafmt;
>  
>         enum v4l2_quantization quantization;
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> index 918eb06c7465..60c9b3c46593 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> @@ -2736,6 +2736,45 @@ static int rkisp1_params_init_vb2_queue(struct vb2_queue *q,
>         return vb2_queue_init(q);
>  }
>  
> +static int rkisp1_ctrl_init(struct rkisp1_params *params)
> +{
> +       int ret;
> +
> +       v4l2_ctrl_handler_init(&params->ctrls, 1);
> +
> +       struct v4l2_ctrl_config ctrl_config = {
> +               .id = RKISP1_CID_SUPPORTED_PARAMS_BLOCKS,
> +               .name = "Supported Params Blocks",
> +               .type = V4L2_CTRL_TYPE_BITMASK,
> +               .flags = V4L2_CTRL_FLAG_READ_ONLY,
> +       };
> +
> +       for (unsigned int i = 0; i < ARRAY_SIZE(rkisp1_ext_params_handlers); i++) {
> +               const struct rkisp1_ext_params_handler *block_handler;
> +
> +               block_handler = &rkisp1_ext_params_handlers[i];
> +               ctrl_config.max |= BIT(i);
> +
> +               if ((params->rkisp1->info->features & block_handler->features) !=
> +                   block_handler->features)
> +                       continue;
> +
> +               ctrl_config.def |= BIT(i);
> +       }
> +
> +       v4l2_ctrl_new_custom(&params->ctrls, &ctrl_config, NULL);
> +
> +       params->vnode.vdev.ctrl_handler = &params->ctrls;
> +
> +       if (params->ctrls.error) {
> +               ret = params->ctrls.error;
> +               v4l2_ctrl_handler_free(&params->ctrls);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
>  int rkisp1_params_register(struct rkisp1_device *rkisp1)
>  {
>         struct rkisp1_params *params = &rkisp1->params;
> @@ -2776,10 +2815,16 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1)
>  
>         video_set_drvdata(vdev, params);
>  
> +       ret = rkisp1_ctrl_init(params);
> +       if (ret) {
> +               dev_err(rkisp1->dev, "Control initialization error %d\n", ret);
> +               goto err_mutex;
> +       }
> +
>         node->pad.flags = MEDIA_PAD_FL_SOURCE;
>         ret = media_entity_pads_init(&vdev->entity, 1, &node->pad);
>         if (ret)
> -               goto err_mutex;
> +               goto err_ctrl;
>  
>         ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
>         if (ret) {
> @@ -2792,6 +2837,8 @@ int rkisp1_params_register(struct rkisp1_device *rkisp1)
>  
>  err_media:
>         media_entity_cleanup(&vdev->entity);
> +err_ctrl:
> +       v4l2_ctrl_handler_free(&params->ctrls);
>  err_mutex:
>         mutex_destroy(&node->vlock);
>         return ret;
> @@ -2808,5 +2855,6 @@ void rkisp1_params_unregister(struct rkisp1_device *rkisp1)
>  
>         vb2_video_unregister_device(vdev);
>         media_entity_cleanup(&vdev->entity);
> +       v4l2_ctrl_handler_free(&params->ctrls);
>         mutex_destroy(&node->vlock);
>  }
> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> index 2d995f3c1ca3..4fc8f221d0c4 100644
> --- a/include/uapi/linux/rkisp1-config.h
> +++ b/include/uapi/linux/rkisp1-config.h
> @@ -1086,6 +1086,9 @@ enum rkisp1_ext_params_block_type {
>  #define RKISP1_EXT_PARAMS_FL_BLOCK_DISABLE     (1U << 0)
>  #define RKISP1_EXT_PARAMS_FL_BLOCK_ENABLE      (1U << 1)
>  
> +/* A bitmask of parameters blocks supported on the current hardware. */
> +#define RKISP1_CID_SUPPORTED_PARAMS_BLOCKS     (V4L2_CID_USER_RKISP1_BASE + 0x01)
> +
>  /**
>   * struct rkisp1_ext_params_block_header - RkISP1 extensible parameters block
>   *                                        header
> @@ -1520,6 +1523,13 @@ enum rksip1_ext_param_buffer_version {
>   * V4L2 control. If such control is not available, userspace should assume only
>   * RKISP1_EXT_PARAM_BUFFER_V1 is supported by the driver.
>   *
> + * The read-only V4L2 control ``RKISP1_CID_SUPPORTED_PARAMS_BLOCKS`` can be used
> + * to query the blocks supported by the current hardware. It contains a bitmask
> + * where each bit represents the availability of the corresponding entry
> + * from the :c:type:`rkisp1_ext_params_block_type` enum. The max value of the
> + * control represents the blocks supported by the current kernel (independent of
> + * the current hardware).
> + *
>   * For each ISP block that userspace wants to configure, a block-specific
>   * structure is appended to the @data buffer, one after the other without gaps
>   * in between nor overlaps. Userspace shall populate the @data_size field with
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 72e32814ea83..f836512e9deb 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -222,6 +222,12 @@ enum v4l2_colorfx {
>   */
>  #define V4L2_CID_USER_UVC_BASE                 (V4L2_CID_USER_BASE + 0x11e0)
>  
> +/*
> + * The base for Rockchip ISP1 driver controls.
> + * We reserve 16 controls for this driver.
> + */
> +#define V4L2_CID_USER_RKISP1_BASE              (V4L2_CID_USER_BASE + 0x1220)
> +
>  /* MPEG-class control IDs */
>  /* The MPEG controls are applicable to all codec controls
>   * and the 'MPEG' part of the define is historical */
> -- 
> 2.43.0
> 
>


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2025-05-23  2:22 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <20250522150944.400046-2-stefan.klug@ideasonboard.com>
2025-05-22 15:08 ` [PATCH 1/3] media: rkisp1: Cleanup error handling Stefan Klug
2025-05-22 15:42   ` Laurent Pinchart
2025-05-22 16:30     ` Stefan Klug
2025-05-22 15:08 ` [PATCH 2/3] media: rkisp1: Add RKISP1_CID_SUPPORTED_PARAMS_BLOCKS control Stefan Klug
2025-05-22 15:56   ` Laurent Pinchart
2025-05-22 16:36     ` Stefan Klug
2025-05-22 16:41       ` Laurent Pinchart
2025-05-22 17:10         ` Stefan Klug
2025-05-22 21:30           ` Laurent Pinchart
2025-05-23  2:19   ` Paul Elder
2025-05-22 15:08 ` [PATCH 3/3] media: rockchip: rkisp1: Add support for Wide Dynamic Range Stefan Klug

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).