[PATCH v4 0/3] Fix the rkisp1 userspace API for later IP versions

[PATCH v4 0/3] Fix the rkisp1 userspace API for later IP versions

[Heiko Stuebner]

This NEEDs to go into 5.11 while we can still adapt the uapi
during its RC-cycle.


When looking into supporting the rkisp1 of the px30 I noticed that
some fields got bigger in the uapi, caused by bigger number of samples
for example for histograms or gamma values.

The rkisp1 was destaged with 5.11-rc1 so we have still time during
the 5.11 cycle to fix that without big hassles.

This was discussed previously in a mail [0] and a rfc series [1]
and this two-part series now splits out the important parts that
really should become part of a 5.11-rc and thus the final 5.11.

changes since v3:
- add patch fixing the original histogram size comment
- make comments in uapi more verbose (Hans)
- fix wording in admin guide (Hans)
- document version <-> soc in uapi as well (easier for people) (Dafna)

changes since v2:
- actually zero the correct sizes for u16 values (hist-bins)
  (kernel-test-robot)

changes since v1:
- drop duplicate isp_ver storage, hw_revision is enough (Dafna)
- document multiple maximum sizes in uapi (Hans)
- document usage of hw_revision field (Hans)
- zero fields transmitted to userspace before adding data (Hans)
- use _V10 field sizes when filling fields, as there is only v10 for now

changes since rfc:
- move rkisp1_version enum into uapo
- show version in media-api hw_revision
- introduce constants for versions and make max use the biggest

Heiko Stuebner (3):
  media: rockchip: rkisp1: fix comment about number of histogram
    sub-windows
  media: rockchip: rkisp1: carry ip version information
  media: rockchip: rkisp1: extend uapi array sizes

 Documentation/admin-guide/media/rkisp1.rst    | 16 ++++
 .../platform/rockchip/rkisp1/rkisp1-dev.c     | 21 +++---
 .../platform/rockchip/rkisp1/rkisp1-params.c  |  2 +-
 .../platform/rockchip/rkisp1/rkisp1-stats.c   | 12 ++-
 include/uapi/linux/rkisp1-config.h            | 74 +++++++++++++++++--
 5 files changed, 106 insertions(+), 19 deletions(-)

-- 
2.29.2

[PATCH v4 1/3] media: rockchip: rkisp1: fix comment about number of histogram sub-windows

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

The histogram data is divided into 16 sub-windows, not 25.
See RKISP1_CIF_ISP_HIST_BIN_N_MAX being defined as 16 for reference
and this also matches the datasheet.

Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
---
 include/uapi/linux/rkisp1-config.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
index 6e449e784260..ba443771cc0b 100644
--- a/include/uapi/linux/rkisp1-config.h
+++ b/include/uapi/linux/rkisp1-config.h
@@ -846,7 +846,7 @@ struct rkisp1_cif_isp_af_stat {
  *
  * @hist_bins: measured bin counters
  *
- * Measurement window divided into 25 sub-windows, set
+ * Measurement window divided into 16 sub-windows, set
  * with ISP_HIST_XXX
  */
 struct rkisp1_cif_isp_hist_stat {
-- 
2.29.2

[PATCH v4 2/3] media: rockchip: rkisp1: carry ip version information

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

The IP block evolved from its rk3288/rk3399 base and the vendor
designates them with a numerical version. rk3399 for example
is designated V10 probably meaning V1.0.

There doesn't seem to be an actual version register we could read that
information from, so allow the match_data to carry that information
for future differentiation.

Also carry that information in the hw_revision field of the media-
controller API, so that userspace also has access to that.

The added versions are:
- V10: at least rk3288 + rk3399
- V11: seemingly unused as of now, but probably appeared in some soc
- V12: at least rk3326 + px30
- V13: at least rk1808

Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 Documentation/admin-guide/media/rkisp1.rst    | 16 ++++++++++++++
 .../platform/rockchip/rkisp1/rkisp1-dev.c     | 21 +++++++++++--------
 include/uapi/linux/rkisp1-config.h            | 16 ++++++++++++++
 3 files changed, 44 insertions(+), 9 deletions(-)

diff --git a/Documentation/admin-guide/media/rkisp1.rst b/Documentation/admin-guide/media/rkisp1.rst
index 2267e4fb475e..ccf418713623 100644
--- a/Documentation/admin-guide/media/rkisp1.rst
+++ b/Documentation/admin-guide/media/rkisp1.rst
@@ -13,6 +13,22 @@ This file documents the driver for the Rockchip ISP1 that is part of RK3288
 and RK3399 SoCs. The driver is located under drivers/staging/media/rkisp1
 and uses the Media-Controller API.
 
+Revisions
+=========
+
+There exist multiple smaller revisions to this ISP that got introduced in
+later SoCs. Revisions can be found in the enum :c:type:`rkisp1_cif_isp_version`
+in the UAPI and the revision of the ISP inside the running SoC can be read
+in the field hw_revision of struct media_device_info as returned by
+ioctl MEDIA_IOC_DEVICE_INFO.
+
+Versions in use are:
+
+- RKISP1_V10: used at least in rk3288 and rk3399
+- RKISP1_V11: declared in the original vendor code, but not used
+- RKISP1_V12: used at least in rk3326 and px30
+- RKISP1_V13: used at least in rk1808
+
 Topology
 ========
 .. _rkisp1_topology_graph:
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
index 68da1eed753d..f7e9fd305548 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
@@ -104,6 +104,7 @@
 struct rkisp1_match_data {
 	const char * const *clks;
 	unsigned int size;
+	enum rkisp1_cif_isp_version isp_ver;
 };
 
 /* ----------------------------------------------------------------------------
@@ -411,15 +412,16 @@ static const char * const rk3399_isp_clks[] = {
 	"hclk",
 };
 
-static const struct rkisp1_match_data rk3399_isp_clk_data = {
+static const struct rkisp1_match_data rk3399_isp_match_data = {
 	.clks = rk3399_isp_clks,
 	.size = ARRAY_SIZE(rk3399_isp_clks),
+	.isp_ver = RKISP1_V10,
 };
 
 static const struct of_device_id rkisp1_of_match[] = {
 	{
 		.compatible = "rockchip,rk3399-cif-isp",
-		.data = &rk3399_isp_clk_data,
+		.data = &rk3399_isp_match_data,
 	},
 	{},
 };
@@ -457,15 +459,15 @@ static void rkisp1_debug_init(struct rkisp1_device *rkisp1)
 
 static int rkisp1_probe(struct platform_device *pdev)
 {
-	const struct rkisp1_match_data *clk_data;
+	const struct rkisp1_match_data *match_data;
 	struct device *dev = &pdev->dev;
 	struct rkisp1_device *rkisp1;
 	struct v4l2_device *v4l2_dev;
 	unsigned int i;
 	int ret, irq;
 
-	clk_data = of_device_get_match_data(&pdev->dev);
-	if (!clk_data)
+	match_data = of_device_get_match_data(&pdev->dev);
+	if (!match_data)
 		return -ENODEV;
 
 	rkisp1 = devm_kzalloc(dev, sizeof(*rkisp1), GFP_KERNEL);
@@ -494,15 +496,16 @@ static int rkisp1_probe(struct platform_device *pdev)
 
 	rkisp1->irq = irq;
 
-	for (i = 0; i < clk_data->size; i++)
-		rkisp1->clks[i].id = clk_data->clks[i];
-	ret = devm_clk_bulk_get(dev, clk_data->size, rkisp1->clks);
+	for (i = 0; i < match_data->size; i++)
+		rkisp1->clks[i].id = match_data->clks[i];
+	ret = devm_clk_bulk_get(dev, match_data->size, rkisp1->clks);
 	if (ret)
 		return ret;
-	rkisp1->clk_size = clk_data->size;
+	rkisp1->clk_size = match_data->size;
 
 	pm_runtime_enable(&pdev->dev);
 
+	rkisp1->media_dev.hw_revision = match_data->isp_ver;
 	strscpy(rkisp1->media_dev.model, RKISP1_DRIVER_NAME,
 		sizeof(rkisp1->media_dev.model));
 	rkisp1->media_dev.dev = &pdev->dev;
diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
index ba443771cc0b..9fecb6bc457d 100644
--- a/include/uapi/linux/rkisp1-config.h
+++ b/include/uapi/linux/rkisp1-config.h
@@ -124,6 +124,22 @@
 #define RKISP1_CIF_ISP_STAT_AFM           (1U << 2)
 #define RKISP1_CIF_ISP_STAT_HIST          (1U << 3)
 
+
+/**
+ * enum rkisp1_cif_isp_version - ISP variants
+ *
+ * @RKISP1_V10: used at least in rk3288 and rk3399
+ * @RKISP1_V11: declared in the original vendor code, but not used
+ * @RKISP1_V12: used at least in rk3326 and px30
+ * @RKISP1_V13: used at least in rk1808
+ */
+enum rkisp1_cif_isp_version {
+	RKISP1_V10 = 0,
+	RKISP1_V11,
+	RKISP1_V12,
+	RKISP1_V13,
+};
+
 enum rkisp1_cif_isp_histogram_mode {
 	RKISP1_CIF_ISP_HISTOGRAM_MODE_DISABLE,
 	RKISP1_CIF_ISP_HISTOGRAM_MODE_RGB_COMBINED,
-- 
2.29.2

[PATCH v4 3/3] media: rockchip: rkisp1: extend uapi array sizes

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>

Later variants of the rkisp1 block use more entries in some arrays:

RKISP1_CIF_ISP_AE_MEAN_MAX                 25 -> 81
RKISP1_CIF_ISP_HIST_BIN_N_MAX              16 -> 32
RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES       17 -> 34
RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28 -> 81

and we can still extend the uapi during the 5.11-rc cycle, so do that
now to be on the safe side.

V10 and V11 only need the smaller sizes, while V12 and V13 needed
the larger sizes.

When adding the bigger sizes make sure, values filled from hardware
values and transmitted to userspace don't leak kernel data by zeroing
them beforehand.

Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
Reviewed-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 .../platform/rockchip/rkisp1/rkisp1-params.c  |  2 +-
 .../platform/rockchip/rkisp1/rkisp1-stats.c   | 12 +++-
 include/uapi/linux/rkisp1-config.h            | 58 ++++++++++++++++---
 3 files changed, 62 insertions(+), 10 deletions(-)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
index 6af4d551ffb5..fa47fe2a02d0 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
@@ -391,7 +391,7 @@ static void rkisp1_goc_config(struct rkisp1_params *params,
 				RKISP1_CIF_ISP_CTRL_ISP_GAMMA_OUT_ENA);
 	rkisp1_write(params->rkisp1, arg->mode, RKISP1_CIF_ISP_GAMMA_OUT_MODE);
 
-	for (i = 0; i < RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES; i++)
+	for (i = 0; i < RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10; i++)
 		rkisp1_write(params->rkisp1, arg->gamma_y[i],
 			     RKISP1_CIF_ISP_GAMMA_OUT_Y_0 + i * 4);
 }
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
index 3ddab8fa8f2d..a26de388ca13 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
@@ -202,8 +202,12 @@ static void rkisp1_stats_get_aec_meas(struct rkisp1_stats *stats,
 	struct rkisp1_device *rkisp1 = stats->rkisp1;
 	unsigned int i;
 
+	/* the global max can be bigger than the version-specific one */
+	memset(pbuf->params.ae.exp_mean, 0, RKISP1_CIF_ISP_AE_MEAN_MAX *
+					    sizeof(*pbuf->params.ae.exp_mean));
+
 	pbuf->meas_type |= RKISP1_CIF_ISP_STAT_AUTOEXP;
-	for (i = 0; i < RKISP1_CIF_ISP_AE_MEAN_MAX; i++)
+	for (i = 0; i < RKISP1_CIF_ISP_AE_MEAN_MAX_V10; i++)
 		pbuf->params.ae.exp_mean[i] =
 			(u8)rkisp1_read(rkisp1,
 					RKISP1_CIF_ISP_EXP_MEAN_00 + i * 4);
@@ -232,8 +236,12 @@ static void rkisp1_stats_get_hst_meas(struct rkisp1_stats *stats,
 	struct rkisp1_device *rkisp1 = stats->rkisp1;
 	unsigned int i;
 
+	/* the global max can be bigger then the version-specific one */
+	memset(pbuf->params.hist.hist_bins, 0, RKISP1_CIF_ISP_HIST_BIN_N_MAX *
+					       sizeof(*pbuf->params.hist.hist_bins));
+
 	pbuf->meas_type |= RKISP1_CIF_ISP_STAT_HIST;
-	for (i = 0; i < RKISP1_CIF_ISP_HIST_BIN_N_MAX; i++)
+	for (i = 0; i < RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10; i++)
 		pbuf->params.hist.hist_bins[i] =
 			(u8)rkisp1_read(rkisp1,
 					RKISP1_CIF_ISP_HIST_BIN_0 + i * 4);
diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
index 9fecb6bc457d..e566069275cf 100644
--- a/include/uapi/linux/rkisp1-config.h
+++ b/include/uapi/linux/rkisp1-config.h
@@ -49,8 +49,14 @@
 #define RKISP1_CIF_ISP_CTK_COEFF_MAX            0x100
 #define RKISP1_CIF_ISP_CTK_OFFSET_MAX           0x800
 
-#define RKISP1_CIF_ISP_AE_MEAN_MAX              25
-#define RKISP1_CIF_ISP_HIST_BIN_N_MAX           16
+#define RKISP1_CIF_ISP_AE_MEAN_MAX_V10		25
+#define RKISP1_CIF_ISP_AE_MEAN_MAX_V12		81
+#define RKISP1_CIF_ISP_AE_MEAN_MAX		RKISP1_CIF_ISP_AE_MEAN_MAX_V12
+
+#define RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10	16
+#define RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12	32
+#define RKISP1_CIF_ISP_HIST_BIN_N_MAX		RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12
+
 #define RKISP1_CIF_ISP_AFM_MAX_WINDOWS          3
 #define RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE       17
 
@@ -86,7 +92,9 @@
  * Gamma out
  */
 /* Maximum number of color samples supported */
-#define RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES       17
+#define RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10   17
+#define RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12   34
+#define RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES       RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12
 
 /*
  * Lens shade correction
@@ -103,7 +111,9 @@
  * Histogram calculation
  */
 /* Last 3 values unused. */
-#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28
+#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10 28
+#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12 81
+#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE     RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12
 
 /*
  * Defect Pixel Cluster Correction
@@ -526,6 +536,15 @@ enum rkisp1_cif_isp_goc_mode {
  *
  * @mode: goc mode (from enum rkisp1_cif_isp_goc_mode)
  * @gamma_y: gamma out curve y-axis for all color components
+ *
+ * The number of entries of @gamma_y depends on the hardware revision
+ * as is reported by the hw_revision field of the struct media_device_info
+ * that is returned by ioctl MEDIA_IOC_DEVICE_INFO.
+ *
+ * Versions <= V11 have RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10
+ * entries, versions >= V12 have RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12
+ * entries. RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES is equal to the maximum
+ * of the two.
  */
 struct rkisp1_cif_isp_goc_config {
 	__u32 mode;
@@ -540,6 +559,15 @@ struct rkisp1_cif_isp_goc_config {
  *			  skipped
  * @meas_window: coordinates of the measure window
  * @hist_weight: weighting factor for sub-windows
+ *
+ * The number of entries of @hist_weight depends on the hardware revision
+ * as is reported by the hw_revision field of the struct media_device_info
+ * that is returned by ioctl MEDIA_IOC_DEVICE_INFO.
+ *
+ * Versions <= V11 have RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10
+ * entries, versions >= V12 have RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12
+ * entries. RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE is equal to the maximum
+ * of the two.
  */
 struct rkisp1_cif_isp_hst_config {
 	__u32 mode;
@@ -827,7 +855,15 @@ struct rkisp1_cif_isp_bls_meas_val {
  * @exp_mean: Mean luminance value of block xx
  * @bls_val:  BLS measured values
  *
- * Image is divided into 5x5 blocks.
+ * The number of entries of @exp_mean depends on the hardware revision
+ * as is reported by the hw_revision field of the struct media_device_info
+ * that is returned by ioctl MEDIA_IOC_DEVICE_INFO.
+ *
+ * Versions <= V11 have RKISP1_CIF_ISP_AE_MEAN_MAX_V10 entries,
+ * versions >= V12 have RKISP1_CIF_ISP_AE_MEAN_MAX_V12 entries.
+ * RKISP1_CIF_ISP_AE_MEAN_MAX is equal to the maximum of the two.
+ *
+ * Image is divided into 5x5 blocks on V10 and 9x9 blocks on V12.
  */
 struct rkisp1_cif_isp_ae_stat {
 	__u8 exp_mean[RKISP1_CIF_ISP_AE_MEAN_MAX];
@@ -862,8 +898,16 @@ struct rkisp1_cif_isp_af_stat {
  *
  * @hist_bins: measured bin counters
  *
- * Measurement window divided into 16 sub-windows, set
- * with ISP_HIST_XXX
+ * Measurement window divided into 16 sub-windows for V10/V10
+ * and 32 sub-windows for V12/V13, set with ISP_HIST_XXX
+ *
+ * The number of entries of @hist_bins depends on the hardware revision
+ * as is reported by the hw_revision field of the struct media_device_info
+ * that is returned by ioctl MEDIA_IOC_DEVICE_INFO.
+ *
+ * Versions <= V11 have RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10 entries,
+ * versions >= V12 have RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12 entries.
+ * RKISP1_CIF_ISP_HIST_BIN_N_MAX is equal to the maximum of the two.
  */
 struct rkisp1_cif_isp_hist_stat {
 	__u16 hist_bins[RKISP1_CIF_ISP_HIST_BIN_N_MAX];
-- 
2.29.2

Re: [PATCH v4 3/3] media: rockchip: rkisp1: extend uapi array sizes

[Dafna Hirschfeld]

Am 15.01.21 um 17:38 schrieb Heiko Stuebner:
> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> 
> Later variants of the rkisp1 block use more entries in some arrays:
> 
> RKISP1_CIF_ISP_AE_MEAN_MAX                 25 -> 81
> RKISP1_CIF_ISP_HIST_BIN_N_MAX              16 -> 32
> RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES       17 -> 34
> RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28 -> 81

I see you didn't change the value for that define.
The usage of it is a bit more complicated.
It is used in function rkisp1_hst_config.
Actually the real number of weight values are 25 (5x5) for rk3399,
the last 3 are not used. I think that in order to support both
5x5 and 9x9 the code in rkisp1-params.c should change. I'll
send a patch fixing it.

> 
> and we can still extend the uapi during the 5.11-rc cycle, so do that
> now to be on the safe side.
> 
> V10 and V11 only need the smaller sizes, while V12 and V13 needed
> the larger sizes.
> 
> When adding the bigger sizes make sure, values filled from hardware
> values and transmitted to userspace don't leak kernel data by zeroing
> them beforehand.
> 
> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
> Reviewed-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>   .../platform/rockchip/rkisp1/rkisp1-params.c  |  2 +-
>   .../platform/rockchip/rkisp1/rkisp1-stats.c   | 12 +++-
>   include/uapi/linux/rkisp1-config.h            | 58 ++++++++++++++++---
>   3 files changed, 62 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> index 6af4d551ffb5..fa47fe2a02d0 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> @@ -391,7 +391,7 @@ static void rkisp1_goc_config(struct rkisp1_params *params,
>   				RKISP1_CIF_ISP_CTRL_ISP_GAMMA_OUT_ENA);
>   	rkisp1_write(params->rkisp1, arg->mode, RKISP1_CIF_ISP_GAMMA_OUT_MODE);
>   
> -	for (i = 0; i < RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES; i++)
> +	for (i = 0; i < RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10; i++)
>   		rkisp1_write(params->rkisp1, arg->gamma_y[i],
>   			     RKISP1_CIF_ISP_GAMMA_OUT_Y_0 + i * 4);
>   }
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
> index 3ddab8fa8f2d..a26de388ca13 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
> @@ -202,8 +202,12 @@ static void rkisp1_stats_get_aec_meas(struct rkisp1_stats *stats,
>   	struct rkisp1_device *rkisp1 = stats->rkisp1;
>   	unsigned int i;
>   
> +	/* the global max can be bigger than the version-specific one */
> +	memset(pbuf->params.ae.exp_mean, 0, RKISP1_CIF_ISP_AE_MEAN_MAX *
> +					    sizeof(*pbuf->params.ae.exp_mean));
> +
>   	pbuf->meas_type |= RKISP1_CIF_ISP_STAT_AUTOEXP;
> -	for (i = 0; i < RKISP1_CIF_ISP_AE_MEAN_MAX; i++)
> +	for (i = 0; i < RKISP1_CIF_ISP_AE_MEAN_MAX_V10; i++)
>   		pbuf->params.ae.exp_mean[i] =
>   			(u8)rkisp1_read(rkisp1,
>   					RKISP1_CIF_ISP_EXP_MEAN_00 + i * 4);
> @@ -232,8 +236,12 @@ static void rkisp1_stats_get_hst_meas(struct rkisp1_stats *stats,
>   	struct rkisp1_device *rkisp1 = stats->rkisp1;
>   	unsigned int i;
>   
> +	/* the global max can be bigger then the version-specific one */
> +	memset(pbuf->params.hist.hist_bins, 0, RKISP1_CIF_ISP_HIST_BIN_N_MAX *
> +					       sizeof(*pbuf->params.hist.hist_bins));
> +
>   	pbuf->meas_type |= RKISP1_CIF_ISP_STAT_HIST;
> -	for (i = 0; i < RKISP1_CIF_ISP_HIST_BIN_N_MAX; i++)
> +	for (i = 0; i < RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10; i++)
>   		pbuf->params.hist.hist_bins[i] =
>   			(u8)rkisp1_read(rkisp1,
>   					RKISP1_CIF_ISP_HIST_BIN_0 + i * 4);
> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> index 9fecb6bc457d..e566069275cf 100644
> --- a/include/uapi/linux/rkisp1-config.h
> +++ b/include/uapi/linux/rkisp1-config.h
> @@ -49,8 +49,14 @@
>   #define RKISP1_CIF_ISP_CTK_COEFF_MAX            0x100
>   #define RKISP1_CIF_ISP_CTK_OFFSET_MAX           0x800
>   
> -#define RKISP1_CIF_ISP_AE_MEAN_MAX              25
> -#define RKISP1_CIF_ISP_HIST_BIN_N_MAX           16
> +#define RKISP1_CIF_ISP_AE_MEAN_MAX_V10		25
> +#define RKISP1_CIF_ISP_AE_MEAN_MAX_V12		81
> +#define RKISP1_CIF_ISP_AE_MEAN_MAX		RKISP1_CIF_ISP_AE_MEAN_MAX_V12
> +
> +#define RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10	16
> +#define RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12	32
> +#define RKISP1_CIF_ISP_HIST_BIN_N_MAX		RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12
> +
>   #define RKISP1_CIF_ISP_AFM_MAX_WINDOWS          3
>   #define RKISP1_CIF_ISP_DEGAMMA_CURVE_SIZE       17
>   
> @@ -86,7 +92,9 @@
>    * Gamma out
>    */
>   /* Maximum number of color samples supported */
> -#define RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES       17
> +#define RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10   17
> +#define RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12   34
> +#define RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES       RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12
>   
>   /*
>    * Lens shade correction
> @@ -103,7 +111,9 @@
>    * Histogram calculation
>    */
>   /* Last 3 values unused. */
> -#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28
> +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10 28
> +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12 81
> +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE     RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12
>   
>   /*
>    * Defect Pixel Cluster Correction
> @@ -526,6 +536,15 @@ enum rkisp1_cif_isp_goc_mode {
>    *
>    * @mode: goc mode (from enum rkisp1_cif_isp_goc_mode)
>    * @gamma_y: gamma out curve y-axis for all color components
> + *
> + * The number of entries of @gamma_y depends on the hardware revision
> + * as is reported by the hw_revision field of the struct media_device_info
> + * that is returned by ioctl MEDIA_IOC_DEVICE_INFO.
> + *
> + * Versions <= V11 have RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V10
> + * entries, versions >= V12 have RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES_V12
> + * entries. RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES is equal to the maximum
> + * of the two.
>    */
>   struct rkisp1_cif_isp_goc_config {
>   	__u32 mode;
> @@ -540,6 +559,15 @@ struct rkisp1_cif_isp_goc_config {
>    *			  skipped
>    * @meas_window: coordinates of the measure window
>    * @hist_weight: weighting factor for sub-windows
> + *
> + * The number of entries of @hist_weight depends on the hardware revision
> + * as is reported by the hw_revision field of the struct media_device_info
> + * that is returned by ioctl MEDIA_IOC_DEVICE_INFO.
> + *
> + * Versions <= V11 have RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10
> + * entries, versions >= V12 have RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12
> + * entries. RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE is equal to the maximum
> + * of the two.
>    */
>   struct rkisp1_cif_isp_hst_config {
>   	__u32 mode;
> @@ -827,7 +855,15 @@ struct rkisp1_cif_isp_bls_meas_val {
>    * @exp_mean: Mean luminance value of block xx
>    * @bls_val:  BLS measured values
>    *
> - * Image is divided into 5x5 blocks.
> + * The number of entries of @exp_mean depends on the hardware revision
> + * as is reported by the hw_revision field of the struct media_device_info
> + * that is returned by ioctl MEDIA_IOC_DEVICE_INFO.
> + *
> + * Versions <= V11 have RKISP1_CIF_ISP_AE_MEAN_MAX_V10 entries,
> + * versions >= V12 have RKISP1_CIF_ISP_AE_MEAN_MAX_V12 entries.
> + * RKISP1_CIF_ISP_AE_MEAN_MAX is equal to the maximum of the two.
> + *
> + * Image is divided into 5x5 blocks on V10 and 9x9 blocks on V12.
>    */
>   struct rkisp1_cif_isp_ae_stat {
>   	__u8 exp_mean[RKISP1_CIF_ISP_AE_MEAN_MAX];
> @@ -862,8 +898,16 @@ struct rkisp1_cif_isp_af_stat {
>    *
>    * @hist_bins: measured bin counters
>    *
> - * Measurement window divided into 16 sub-windows, set
> - * with ISP_HIST_XXX
> + * Measurement window divided into 16 sub-windows for V10/V10
> + * and 32 sub-windows for V12/V13, set with ISP_HIST_XXX

It is actually not windows but histogram bins. Could you change it to:
"The histogram values divided into 16 bins for V10/V11 and 32 bins
for V12/V13. It is configured within the struct rkisp1_cif_isp_hst_config.

Thanks,
Dafna


> + *
> + * The number of entries of @hist_bins depends on the hardware revision
> + * as is reported by the hw_revision field of the struct media_device_info
> + * that is returned by ioctl MEDIA_IOC_DEVICE_INFO.
> + *
> + * Versions <= V11 have RKISP1_CIF_ISP_HIST_BIN_N_MAX_V10 entries,
> + * versions >= V12 have RKISP1_CIF_ISP_HIST_BIN_N_MAX_V12 entries.
> + * RKISP1_CIF_ISP_HIST_BIN_N_MAX is equal to the maximum of the two.
>    */
>   struct rkisp1_cif_isp_hist_stat {
>   	__u16 hist_bins[RKISP1_CIF_ISP_HIST_BIN_N_MAX];
> 

Re: [PATCH v4 2/3] media: rockchip: rkisp1: carry ip version information

[Laurent Pinchart]

Hi Heiko,

Thank you for the patch.

On Fri, Jan 15, 2021 at 05:38:28PM +0100, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> 
> The IP block evolved from its rk3288/rk3399 base and the vendor
> designates them with a numerical version. rk3399 for example
> is designated V10 probably meaning V1.0.
> 
> There doesn't seem to be an actual version register we could read that
> information from, so allow the match_data to carry that information
> for future differentiation.
> 
> Also carry that information in the hw_revision field of the media-
> controller API, so that userspace also has access to that.
> 
> The added versions are:
> - V10: at least rk3288 + rk3399
> - V11: seemingly unused as of now, but probably appeared in some soc
> - V12: at least rk3326 + px30
> - V13: at least rk1808

In addition to changes to the ISP itself, are there changes in how it is
integrated in the system, in particular in how it is connected to CSI-2
receivers ? Do all the above SoCs have two ISP instances ?

> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  Documentation/admin-guide/media/rkisp1.rst    | 16 ++++++++++++++
>  .../platform/rockchip/rkisp1/rkisp1-dev.c     | 21 +++++++++++--------
>  include/uapi/linux/rkisp1-config.h            | 16 ++++++++++++++
>  3 files changed, 44 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/admin-guide/media/rkisp1.rst b/Documentation/admin-guide/media/rkisp1.rst
> index 2267e4fb475e..ccf418713623 100644
> --- a/Documentation/admin-guide/media/rkisp1.rst
> +++ b/Documentation/admin-guide/media/rkisp1.rst
> @@ -13,6 +13,22 @@ This file documents the driver for the Rockchip ISP1 that is part of RK3288
>  and RK3399 SoCs. The driver is located under drivers/staging/media/rkisp1
>  and uses the Media-Controller API.
>  
> +Revisions
> +=========
> +
> +There exist multiple smaller revisions to this ISP that got introduced in
> +later SoCs. Revisions can be found in the enum :c:type:`rkisp1_cif_isp_version`
> +in the UAPI and the revision of the ISP inside the running SoC can be read
> +in the field hw_revision of struct media_device_info as returned by
> +ioctl MEDIA_IOC_DEVICE_INFO.
> +
> +Versions in use are:
> +
> +- RKISP1_V10: used at least in rk3288 and rk3399
> +- RKISP1_V11: declared in the original vendor code, but not used
> +- RKISP1_V12: used at least in rk3326 and px30
> +- RKISP1_V13: used at least in rk1808
> +
>  Topology
>  ========
>  .. _rkisp1_topology_graph:
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> index 68da1eed753d..f7e9fd305548 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> @@ -104,6 +104,7 @@
>  struct rkisp1_match_data {
>  	const char * const *clks;
>  	unsigned int size;
> +	enum rkisp1_cif_isp_version isp_ver;
>  };
>  
>  /* ----------------------------------------------------------------------------
> @@ -411,15 +412,16 @@ static const char * const rk3399_isp_clks[] = {
>  	"hclk",
>  };
>  
> -static const struct rkisp1_match_data rk3399_isp_clk_data = {
> +static const struct rkisp1_match_data rk3399_isp_match_data = {
>  	.clks = rk3399_isp_clks,
>  	.size = ARRAY_SIZE(rk3399_isp_clks),
> +	.isp_ver = RKISP1_V10,
>  };
>  
>  static const struct of_device_id rkisp1_of_match[] = {
>  	{
>  		.compatible = "rockchip,rk3399-cif-isp",
> -		.data = &rk3399_isp_clk_data,
> +		.data = &rk3399_isp_match_data,
>  	},
>  	{},
>  };
> @@ -457,15 +459,15 @@ static void rkisp1_debug_init(struct rkisp1_device *rkisp1)
>  
>  static int rkisp1_probe(struct platform_device *pdev)
>  {
> -	const struct rkisp1_match_data *clk_data;
> +	const struct rkisp1_match_data *match_data;
>  	struct device *dev = &pdev->dev;
>  	struct rkisp1_device *rkisp1;
>  	struct v4l2_device *v4l2_dev;
>  	unsigned int i;
>  	int ret, irq;
>  
> -	clk_data = of_device_get_match_data(&pdev->dev);
> -	if (!clk_data)
> +	match_data = of_device_get_match_data(&pdev->dev);
> +	if (!match_data)
>  		return -ENODEV;
>  
>  	rkisp1 = devm_kzalloc(dev, sizeof(*rkisp1), GFP_KERNEL);
> @@ -494,15 +496,16 @@ static int rkisp1_probe(struct platform_device *pdev)
>  
>  	rkisp1->irq = irq;
>  
> -	for (i = 0; i < clk_data->size; i++)
> -		rkisp1->clks[i].id = clk_data->clks[i];
> -	ret = devm_clk_bulk_get(dev, clk_data->size, rkisp1->clks);
> +	for (i = 0; i < match_data->size; i++)
> +		rkisp1->clks[i].id = match_data->clks[i];
> +	ret = devm_clk_bulk_get(dev, match_data->size, rkisp1->clks);
>  	if (ret)
>  		return ret;
> -	rkisp1->clk_size = clk_data->size;
> +	rkisp1->clk_size = match_data->size;
>  
>  	pm_runtime_enable(&pdev->dev);
>  
> +	rkisp1->media_dev.hw_revision = match_data->isp_ver;
>  	strscpy(rkisp1->media_dev.model, RKISP1_DRIVER_NAME,
>  		sizeof(rkisp1->media_dev.model));
>  	rkisp1->media_dev.dev = &pdev->dev;
> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> index ba443771cc0b..9fecb6bc457d 100644
> --- a/include/uapi/linux/rkisp1-config.h
> +++ b/include/uapi/linux/rkisp1-config.h
> @@ -124,6 +124,22 @@
>  #define RKISP1_CIF_ISP_STAT_AFM           (1U << 2)
>  #define RKISP1_CIF_ISP_STAT_HIST          (1U << 3)
>  
> +
> +/**
> + * enum rkisp1_cif_isp_version - ISP variants
> + *
> + * @RKISP1_V10: used at least in rk3288 and rk3399
> + * @RKISP1_V11: declared in the original vendor code, but not used
> + * @RKISP1_V12: used at least in rk3326 and px30
> + * @RKISP1_V13: used at least in rk1808
> + */
> +enum rkisp1_cif_isp_version {
> +	RKISP1_V10 = 0,
> +	RKISP1_V11,
> +	RKISP1_V12,
> +	RKISP1_V13,
> +};
> +
>  enum rkisp1_cif_isp_histogram_mode {
>  	RKISP1_CIF_ISP_HISTOGRAM_MODE_DISABLE,
>  	RKISP1_CIF_ISP_HISTOGRAM_MODE_RGB_COMBINED,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 2/3] media: rockchip: rkisp1: carry ip version information

[Heiko Stübner]

Hi Laurent,

Am Freitag, 15. Januar 2021, 20:42:41 CET schrieb Laurent Pinchart:
> On Fri, Jan 15, 2021 at 05:38:28PM +0100, Heiko Stuebner wrote:
> > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > 
> > The IP block evolved from its rk3288/rk3399 base and the vendor
> > designates them with a numerical version. rk3399 for example
> > is designated V10 probably meaning V1.0.
> > 
> > There doesn't seem to be an actual version register we could read that
> > information from, so allow the match_data to carry that information
> > for future differentiation.
> > 
> > Also carry that information in the hw_revision field of the media-
> > controller API, so that userspace also has access to that.
> > 
> > The added versions are:
> > - V10: at least rk3288 + rk3399
> > - V11: seemingly unused as of now, but probably appeared in some soc
> > - V12: at least rk3326 + px30
> > - V13: at least rk1808
> 
> In addition to changes to the ISP itself, are there changes in how it is
> integrated in the system, in particular in how it is connected to CSI-2
> receivers ?

Hooking up my camera on the px30 was pretty straightforward.
As you can see in [0] the actual csi-phy block is different, but structurally
everything else stayed the same and the px30 series merely accounts
for some isp-specific changes, like the split interrupts and the V12-specific
parts.


> Do all the above SoCs have two ISP instances ?

Nope, I think only the rk3399 has two ISPs.

All others only seem to have one instance and maybe an additional more
simple video input processor. (like the rk3288 has both an ISP and something
called VIP - a simpler block)

Similar is the rk3326/px30 (same soc) which has an ISP as well as a VIP that
Maxime Chevallier is working on [1]

Heiko

[0] https://lore.kernel.org/linux-media/20210108193311.3423236-1-heiko@sntech.de/
[1] https://lore.kernel.org/linux-devicetree/20201229161724.511102-1-maxime.chevallier@bootlin.com/


> > Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> >  Documentation/admin-guide/media/rkisp1.rst    | 16 ++++++++++++++
> >  .../platform/rockchip/rkisp1/rkisp1-dev.c     | 21 +++++++++++--------
> >  include/uapi/linux/rkisp1-config.h            | 16 ++++++++++++++
> >  3 files changed, 44 insertions(+), 9 deletions(-)
> > 
> > diff --git a/Documentation/admin-guide/media/rkisp1.rst b/Documentation/admin-guide/media/rkisp1.rst
> > index 2267e4fb475e..ccf418713623 100644
> > --- a/Documentation/admin-guide/media/rkisp1.rst
> > +++ b/Documentation/admin-guide/media/rkisp1.rst
> > @@ -13,6 +13,22 @@ This file documents the driver for the Rockchip ISP1 that is part of RK3288
> >  and RK3399 SoCs. The driver is located under drivers/staging/media/rkisp1
> >  and uses the Media-Controller API.
> >  
> > +Revisions
> > +=========
> > +
> > +There exist multiple smaller revisions to this ISP that got introduced in
> > +later SoCs. Revisions can be found in the enum :c:type:`rkisp1_cif_isp_version`
> > +in the UAPI and the revision of the ISP inside the running SoC can be read
> > +in the field hw_revision of struct media_device_info as returned by
> > +ioctl MEDIA_IOC_DEVICE_INFO.
> > +
> > +Versions in use are:
> > +
> > +- RKISP1_V10: used at least in rk3288 and rk3399
> > +- RKISP1_V11: declared in the original vendor code, but not used
> > +- RKISP1_V12: used at least in rk3326 and px30
> > +- RKISP1_V13: used at least in rk1808
> > +
> >  Topology
> >  ========
> >  .. _rkisp1_topology_graph:
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > index 68da1eed753d..f7e9fd305548 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> > @@ -104,6 +104,7 @@
> >  struct rkisp1_match_data {
> >  	const char * const *clks;
> >  	unsigned int size;
> > +	enum rkisp1_cif_isp_version isp_ver;
> >  };
> >  
> >  /* ----------------------------------------------------------------------------
> > @@ -411,15 +412,16 @@ static const char * const rk3399_isp_clks[] = {
> >  	"hclk",
> >  };
> >  
> > -static const struct rkisp1_match_data rk3399_isp_clk_data = {
> > +static const struct rkisp1_match_data rk3399_isp_match_data = {
> >  	.clks = rk3399_isp_clks,
> >  	.size = ARRAY_SIZE(rk3399_isp_clks),
> > +	.isp_ver = RKISP1_V10,
> >  };
> >  
> >  static const struct of_device_id rkisp1_of_match[] = {
> >  	{
> >  		.compatible = "rockchip,rk3399-cif-isp",
> > -		.data = &rk3399_isp_clk_data,
> > +		.data = &rk3399_isp_match_data,
> >  	},
> >  	{},
> >  };
> > @@ -457,15 +459,15 @@ static void rkisp1_debug_init(struct rkisp1_device *rkisp1)
> >  
> >  static int rkisp1_probe(struct platform_device *pdev)
> >  {
> > -	const struct rkisp1_match_data *clk_data;
> > +	const struct rkisp1_match_data *match_data;
> >  	struct device *dev = &pdev->dev;
> >  	struct rkisp1_device *rkisp1;
> >  	struct v4l2_device *v4l2_dev;
> >  	unsigned int i;
> >  	int ret, irq;
> >  
> > -	clk_data = of_device_get_match_data(&pdev->dev);
> > -	if (!clk_data)
> > +	match_data = of_device_get_match_data(&pdev->dev);
> > +	if (!match_data)
> >  		return -ENODEV;
> >  
> >  	rkisp1 = devm_kzalloc(dev, sizeof(*rkisp1), GFP_KERNEL);
> > @@ -494,15 +496,16 @@ static int rkisp1_probe(struct platform_device *pdev)
> >  
> >  	rkisp1->irq = irq;
> >  
> > -	for (i = 0; i < clk_data->size; i++)
> > -		rkisp1->clks[i].id = clk_data->clks[i];
> > -	ret = devm_clk_bulk_get(dev, clk_data->size, rkisp1->clks);
> > +	for (i = 0; i < match_data->size; i++)
> > +		rkisp1->clks[i].id = match_data->clks[i];
> > +	ret = devm_clk_bulk_get(dev, match_data->size, rkisp1->clks);
> >  	if (ret)
> >  		return ret;
> > -	rkisp1->clk_size = clk_data->size;
> > +	rkisp1->clk_size = match_data->size;
> >  
> >  	pm_runtime_enable(&pdev->dev);
> >  
> > +	rkisp1->media_dev.hw_revision = match_data->isp_ver;
> >  	strscpy(rkisp1->media_dev.model, RKISP1_DRIVER_NAME,
> >  		sizeof(rkisp1->media_dev.model));
> >  	rkisp1->media_dev.dev = &pdev->dev;
> > diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> > index ba443771cc0b..9fecb6bc457d 100644
> > --- a/include/uapi/linux/rkisp1-config.h
> > +++ b/include/uapi/linux/rkisp1-config.h
> > @@ -124,6 +124,22 @@
> >  #define RKISP1_CIF_ISP_STAT_AFM           (1U << 2)
> >  #define RKISP1_CIF_ISP_STAT_HIST          (1U << 3)
> >  
> > +
> > +/**
> > + * enum rkisp1_cif_isp_version - ISP variants
> > + *
> > + * @RKISP1_V10: used at least in rk3288 and rk3399
> > + * @RKISP1_V11: declared in the original vendor code, but not used
> > + * @RKISP1_V12: used at least in rk3326 and px30
> > + * @RKISP1_V13: used at least in rk1808
> > + */
> > +enum rkisp1_cif_isp_version {
> > +	RKISP1_V10 = 0,
> > +	RKISP1_V11,
> > +	RKISP1_V12,
> > +	RKISP1_V13,
> > +};
> > +
> >  enum rkisp1_cif_isp_histogram_mode {
> >  	RKISP1_CIF_ISP_HISTOGRAM_MODE_DISABLE,
> >  	RKISP1_CIF_ISP_HISTOGRAM_MODE_RGB_COMBINED,
> 
> 

Re: [PATCH v4 3/3] media: rockchip: rkisp1: extend uapi array sizes

[Heiko Stübner]

Hi Dafna,

Am Freitag, 15. Januar 2021, 18:41:06 CET schrieb Dafna Hirschfeld:
> 
> Am 15.01.21 um 17:38 schrieb Heiko Stuebner:
> > From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> > 
> > Later variants of the rkisp1 block use more entries in some arrays:
> > 
> > RKISP1_CIF_ISP_AE_MEAN_MAX                 25 -> 81
> > RKISP1_CIF_ISP_HIST_BIN_N_MAX              16 -> 32
> > RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES       17 -> 34
> > RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28 -> 81
> 
> I see you didn't change the value for that define.

In the below patch I find

@@ -103,7 +111,9 @@
* Histogram calculation
*/
/* Last 3 values unused. */
-#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28
+#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10 28
+#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12 81
+#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE     RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12

so I'm not sure I understand what you mean except this.

> The usage of it is a bit more complicated.
> It is used in function rkisp1_hst_config.

Yeah, though the for-loop iterates over 4*7 entry values, so stays
below the RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10 in all cases.


> Actually the real number of weight values are 25 (5x5) for rk3399,
> the last 3 are not used. I think that in order to support both
> 5x5 and 9x9 the code in rkisp1-params.c should change. I'll
> send a patch fixing it.

If you look at my V12-patch [0] the weight handling is done different there
and from the registers, it looks like they exchanges that part of the isp.

[0] https://lore.kernel.org/linux-media/20210108193311.3423236-11-heiko@sntech.de/
void rkisp1_hst_config_v12() as a search term

[...]

> > @@ -862,8 +898,16 @@ struct rkisp1_cif_isp_af_stat {
> >    *
> >    * @hist_bins: measured bin counters
> >    *
> > - * Measurement window divided into 16 sub-windows, set
> > - * with ISP_HIST_XXX
> > + * Measurement window divided into 16 sub-windows for V10/V10
> > + * and 32 sub-windows for V12/V13, set with ISP_HIST_XXX
> 
> It is actually not windows but histogram bins. Could you change it to:
> "The histogram values divided into 16 bins for V10/V11 and 32 bins
> for V12/V13. It is configured within the struct rkisp1_cif_isp_hst_config.

I've changed this like your suggestions and will give a bit of time for
the stuff above. But I guess I can send a v5 some time tomorrow?


Thanks for your input
Heiko

Re: [PATCH v4 3/3] media: rockchip: rkisp1: extend uapi array sizes

[Dafna Hirschfeld]

Am 16.01.21 um 00:52 schrieb Heiko Stübner:
> Hi Dafna,
> 
> Am Freitag, 15. Januar 2021, 18:41:06 CET schrieb Dafna Hirschfeld:
>>
>> Am 15.01.21 um 17:38 schrieb Heiko Stuebner:
>>> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
>>>
>>> Later variants of the rkisp1 block use more entries in some arrays:
>>>
>>> RKISP1_CIF_ISP_AE_MEAN_MAX                 25 -> 81
>>> RKISP1_CIF_ISP_HIST_BIN_N_MAX              16 -> 32
>>> RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES       17 -> 34
>>> RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28 -> 81
>>
>> I see you didn't change the value for that define.
> 
> In the below patch I find
> 
> @@ -103,7 +111,9 @@
> * Histogram calculation
> */
> /* Last 3 values unused. */
> -#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28
> +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10 28
> +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12 81
> +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE     RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12
> 
> so I'm not sure I understand what you mean except this.

Opps, I somehow missed that.
But now since we have 81 entries, it makes no sense to
define it to 28 for V10 and documenting "Last 3 values unused" (see just above the definition).
We can set it just to 25, we have 56 (81-25) unused values anyway.

> 
>> The usage of it is a bit more complicated.
>> It is used in function rkisp1_hst_config.
> 
> Yeah, though the for-loop iterates over 4*7 entry values, so stays
> below the RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10 in all cases.
> 
> 
>> Actually the real number of weight values are 25 (5x5) for rk3399,
>> the last 3 are not used. I think that in order to support both
>> 5x5 and 9x9 the code in rkisp1-params.c should change. I'll
>> send a patch fixing it.
> 
> If you look at my V12-patch [0] the weight handling is done different there
> and from the registers, it looks like they exchanges that part of the isp.
> 
> [0] https://lore.kernel.org/linux-media/20210108193311.3423236-11-heiko@sntech.de/
> void rkisp1_hst_config_v12() as a search term
> 
> [...]

Right, there is no need to change the relevant code in rkisp1-param.c when setting the
RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE to 25.

Thanks,
Dafna

> 
>>> @@ -862,8 +898,16 @@ struct rkisp1_cif_isp_af_stat {
>>>     *
>>>     * @hist_bins: measured bin counters
>>>     *
>>> - * Measurement window divided into 16 sub-windows, set
>>> - * with ISP_HIST_XXX
>>> + * Measurement window divided into 16 sub-windows for V10/V10
>>> + * and 32 sub-windows for V12/V13, set with ISP_HIST_XXX
>>
>> It is actually not windows but histogram bins. Could you change it to:
>> "The histogram values divided into 16 bins for V10/V11 and 32 bins
>> for V12/V13. It is configured within the struct rkisp1_cif_isp_hst_config.
> 
> I've changed this like your suggestions and will give a bit of time for
> the stuff above. But I guess I can send a v5 some time tomorrow?
> 
> 
> Thanks for your input
> Heiko
> 
> 

Re: [PATCH v4 3/3] media: rockchip: rkisp1: extend uapi array sizes

[Heiko Stübner]

Hi Dafna,

Am Samstag, 16. Januar 2021, 10:05:39 CET schrieb Dafna Hirschfeld:
> Am 16.01.21 um 00:52 schrieb Heiko Stübner:
> > Am Freitag, 15. Januar 2021, 18:41:06 CET schrieb Dafna Hirschfeld:
> >> Am 15.01.21 um 17:38 schrieb Heiko Stuebner:
> >>> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> >>>
> >>> Later variants of the rkisp1 block use more entries in some arrays:
> >>>
> >>> RKISP1_CIF_ISP_AE_MEAN_MAX                 25 -> 81
> >>> RKISP1_CIF_ISP_HIST_BIN_N_MAX              16 -> 32
> >>> RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES       17 -> 34
> >>> RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28 -> 81
> >>
> >> I see you didn't change the value for that define.
> > 
> > In the below patch I find
> > 
> > @@ -103,7 +111,9 @@
> > * Histogram calculation
> > */
> > /* Last 3 values unused. */
> > -#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28
> > +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10 28
> > +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12 81
> > +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE     RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12
> > 
> > so I'm not sure I understand what you mean except this.
> 
> Opps, I somehow missed that.
> But now since we have 81 entries, it makes no sense to
> define it to 28 for V10 and documenting "Last 3 values unused" (see just above the definition).
> We can set it just to 25, we have 56 (81-25) unused values anyway.

The rkisp1_hst_config() function iterates over its regs-array and accesses
4 weight values in each step and writing them to the register:

static const u32 hist_weight_regs[] = {
	RKISP1_CIF_ISP_HIST_WEIGHT_00TO30_V10, ... weight0-3
	RKISP1_CIF_ISP_HIST_WEIGHT_40TO21_V10, ... weight4-7
	RKISP1_CIF_ISP_HIST_WEIGHT_31TO12_V10, ... weight8-11
	RKISP1_CIF_ISP_HIST_WEIGHT_22TO03_V10, ... weight12-15
	RKISP1_CIF_ISP_HIST_WEIGHT_13TO43_V10, ... weight16-19
	RKISP1_CIF_ISP_HIST_WEIGHT_04TO34_V10, ... weight20-23
	RKISP1_CIF_ISP_HIST_WEIGHT_44_V10, ... weight24-27
};

and yes the last step only uses 1 value to write to the register.
But I guess if we really want to reduce the max value, we should move
that last part into a separate write, so that we don't access parts we claim
to not access ;-)

> >> The usage of it is a bit more complicated.
> >> It is used in function rkisp1_hst_config.
> > 
> > Yeah, though the for-loop iterates over 4*7 entry values, so stays
> > below the RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10 in all cases.
> > 
> > 
> >> Actually the real number of weight values are 25 (5x5) for rk3399,
> >> the last 3 are not used. I think that in order to support both
> >> 5x5 and 9x9 the code in rkisp1-params.c should change. I'll
> >> send a patch fixing it.
> > 
> > If you look at my V12-patch [0] the weight handling is done different there
> > and from the registers, it looks like they exchanges that part of the isp.
> > 
> > [0] https://lore.kernel.org/linux-media/20210108193311.3423236-11-heiko@sntech.de/
> > void rkisp1_hst_config_v12() as a search term
> > 
> > [...]
> 
> Right, there is no need to change the relevant code in rkisp1-param.c when setting the
> RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE to 25.

ok, should we add a 4th patch fixing that and moving the last write out of the loop?


Heiko

> >>> @@ -862,8 +898,16 @@ struct rkisp1_cif_isp_af_stat {
> >>>     *
> >>>     * @hist_bins: measured bin counters
> >>>     *
> >>> - * Measurement window divided into 16 sub-windows, set
> >>> - * with ISP_HIST_XXX
> >>> + * Measurement window divided into 16 sub-windows for V10/V10
> >>> + * and 32 sub-windows for V12/V13, set with ISP_HIST_XXX
> >>
> >> It is actually not windows but histogram bins. Could you change it to:
> >> "The histogram values divided into 16 bins for V10/V11 and 32 bins
> >> for V12/V13. It is configured within the struct rkisp1_cif_isp_hst_config.
> > 
> > I've changed this like your suggestions and will give a bit of time for
> > the stuff above. But I guess I can send a v5 some time tomorrow?
> > 
> > 
> > Thanks for your input
> > Heiko
> > 
> > 
> 

Re: [PATCH v4 3/3] media: rockchip: rkisp1: extend uapi array sizes

[Dafna Hirschfeld]

Am 16.01.21 um 12:57 schrieb Heiko Stübner:
> Hi Dafna,
> 
> Am Samstag, 16. Januar 2021, 10:05:39 CET schrieb Dafna Hirschfeld:
>> Am 16.01.21 um 00:52 schrieb Heiko Stübner:
>>> Am Freitag, 15. Januar 2021, 18:41:06 CET schrieb Dafna Hirschfeld:
>>>> Am 15.01.21 um 17:38 schrieb Heiko Stuebner:
>>>>> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
>>>>>
>>>>> Later variants of the rkisp1 block use more entries in some arrays:
>>>>>
>>>>> RKISP1_CIF_ISP_AE_MEAN_MAX                 25 -> 81
>>>>> RKISP1_CIF_ISP_HIST_BIN_N_MAX              16 -> 32
>>>>> RKISP1_CIF_ISP_GAMMA_OUT_MAX_SAMPLES       17 -> 34
>>>>> RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28 -> 81
>>>>
>>>> I see you didn't change the value for that define.
>>>
>>> In the below patch I find
>>>
>>> @@ -103,7 +111,9 @@
>>> * Histogram calculation
>>> */
>>> /* Last 3 values unused. */
>>> -#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE 28
>>> +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10 28
>>> +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12 81
>>> +#define RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE     RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V12
>>>
>>> so I'm not sure I understand what you mean except this.
>>
>> Opps, I somehow missed that.
>> But now since we have 81 entries, it makes no sense to
>> define it to 28 for V10 and documenting "Last 3 values unused" (see just above the definition).
>> We can set it just to 25, we have 56 (81-25) unused values anyway.
> 
> The rkisp1_hst_config() function iterates over its regs-array and accesses
> 4 weight values in each step and writing them to the register:
> 
> static const u32 hist_weight_regs[] = {
> 	RKISP1_CIF_ISP_HIST_WEIGHT_00TO30_V10, ... weight0-3
> 	RKISP1_CIF_ISP_HIST_WEIGHT_40TO21_V10, ... weight4-7
> 	RKISP1_CIF_ISP_HIST_WEIGHT_31TO12_V10, ... weight8-11
> 	RKISP1_CIF_ISP_HIST_WEIGHT_22TO03_V10, ... weight12-15
> 	RKISP1_CIF_ISP_HIST_WEIGHT_13TO43_V10, ... weight16-19
> 	RKISP1_CIF_ISP_HIST_WEIGHT_04TO34_V10, ... weight20-23
> 	RKISP1_CIF_ISP_HIST_WEIGHT_44_V10, ... weight24-27
> };
> 
> and yes the last step only uses 1 value to write to the register.
> But I guess if we really want to reduce the max value, we should move
> that last part into a separate write, so that we don't access parts we claim
> to not access ;-)
> 
>>>> The usage of it is a bit more complicated.
>>>> It is used in function rkisp1_hst_config.
>>>
>>> Yeah, though the for-loop iterates over 4*7 entry values, so stays
>>> below the RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE_V10 in all cases.
>>>
>>>
>>>> Actually the real number of weight values are 25 (5x5) for rk3399,
>>>> the last 3 are not used. I think that in order to support both
>>>> 5x5 and 9x9 the code in rkisp1-params.c should change. I'll
>>>> send a patch fixing it.
>>>
>>> If you look at my V12-patch [0] the weight handling is done different there
>>> and from the registers, it looks like they exchanges that part of the isp.
>>>
>>> [0] https://lore.kernel.org/linux-media/20210108193311.3423236-11-heiko@sntech.de/
>>> void rkisp1_hst_config_v12() as a search term
>>>
>>> [...]
>>
>> Right, there is no need to change the relevant code in rkisp1-param.c when setting the
>> RKISP1_CIF_ISP_HISTOGRAM_WEIGHT_GRIDS_SIZE to 25.
> 
> ok, should we add a 4th patch fixing that and moving the last write out of the loop?

I think changing the related code in the rkisp1-params.c can be done later in a separate patch.
I think you can change it to 25 in the same patch that adds V12 support and write in the commit message
that because the array size is now 81, there is no point defining the grid size for V10 to 28
and in addition documenting that the last 3 values are not used.

Thanks,
Dafna

> 
> 
> Heiko
> 
>>>>> @@ -862,8 +898,16 @@ struct rkisp1_cif_isp_af_stat {
>>>>>      *
>>>>>      * @hist_bins: measured bin counters
>>>>>      *
>>>>> - * Measurement window divided into 16 sub-windows, set
>>>>> - * with ISP_HIST_XXX
>>>>> + * Measurement window divided into 16 sub-windows for V10/V10
>>>>> + * and 32 sub-windows for V12/V13, set with ISP_HIST_XXX
>>>>
>>>> It is actually not windows but histogram bins. Could you change it to:
>>>> "The histogram values divided into 16 bins for V10/V11 and 32 bins
>>>> for V12/V13. It is configured within the struct rkisp1_cif_isp_hst_config.
>>>
>>> I've changed this like your suggestions and will give a bit of time for
>>> the stuff above. But I guess I can send a v5 some time tomorrow?
>>>
>>>
>>> Thanks for your input
>>> Heiko
>>>
>>>
>>
> 
> 
> 
> 

Re: [PATCH v4 2/3] media: rockchip: rkisp1: carry ip version information

[Dafna Hirschfeld]

Am 15.01.21 um 21:29 schrieb Heiko Stübner:
> Hi Laurent,
> 
> Am Freitag, 15. Januar 2021, 20:42:41 CET schrieb Laurent Pinchart:
>> On Fri, Jan 15, 2021 at 05:38:28PM +0100, Heiko Stuebner wrote:
>>> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
>>>
>>> The IP block evolved from its rk3288/rk3399 base and the vendor
>>> designates them with a numerical version. rk3399 for example
>>> is designated V10 probably meaning V1.0.
>>>
>>> There doesn't seem to be an actual version register we could read that
>>> information from, so allow the match_data to carry that information
>>> for future differentiation.
>>>
>>> Also carry that information in the hw_revision field of the media-
>>> controller API, so that userspace also has access to that.
>>>
>>> The added versions are:
>>> - V10: at least rk3288 + rk3399
>>> - V11: seemingly unused as of now, but probably appeared in some soc
>>> - V12: at least rk3326 + px30
>>> - V13: at least rk1808
>>
>> In addition to changes to the ISP itself, are there changes in how it is
>> integrated in the system, in particular in how it is connected to CSI-2
>> receivers ?
> 
> Hooking up my camera on the px30 was pretty straightforward.
> As you can see in [0] the actual csi-phy block is different, but structurally
> everything else stayed the same and the px30 series merely accounts
> for some isp-specific changes, like the split interrupts and the V12-specific
> parts.
> 
> 
>> Do all the above SoCs have two ISP instances ?
> 
> Nope, I think only the rk3399 has two ISPs.
> 
> All others only seem to have one instance and maybe an additional more
> simple video input processor. (like the rk3288 has both an ISP and something
> called VIP - a simpler block)
> 
> Similar is the rk3326/px30 (same soc) which has an ISP as well as a VIP that
> Maxime Chevallier is working on [1]
> 
> Heiko
> 
> [0] https://lore.kernel.org/linux-media/20210108193311.3423236-1-heiko@sntech.de/
> [1] https://lore.kernel.org/linux-devicetree/20201229161724.511102-1-maxime.chevallier@bootlin.com/
> 
> 
>>> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
>>> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
>>> ---
>>>   Documentation/admin-guide/media/rkisp1.rst    | 16 ++++++++++++++
>>>   .../platform/rockchip/rkisp1/rkisp1-dev.c     | 21 +++++++++++--------
>>>   include/uapi/linux/rkisp1-config.h            | 16 ++++++++++++++
>>>   3 files changed, 44 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/Documentation/admin-guide/media/rkisp1.rst b/Documentation/admin-guide/media/rkisp1.rst
>>> index 2267e4fb475e..ccf418713623 100644
>>> --- a/Documentation/admin-guide/media/rkisp1.rst
>>> +++ b/Documentation/admin-guide/media/rkisp1.rst
>>> @@ -13,6 +13,22 @@ This file documents the driver for the Rockchip ISP1 that is part of RK3288
>>>   and RK3399 SoCs. The driver is located under drivers/staging/media/rkisp1
>>>   and uses the Media-Controller API.
>>>   
>>> +Revisions
>>> +=========
>>> +
>>> +There exist multiple smaller revisions to this ISP that got introduced in
>>> +later SoCs. Revisions can be found in the enum :c:type:`rkisp1_cif_isp_version`
>>> +in the UAPI and the revision of the ISP inside the running SoC can be read
>>> +in the field hw_revision of struct media_device_info as returned by
>>> +ioctl MEDIA_IOC_DEVICE_INFO.
>>> +
>>> +Versions in use are:
>>> +
>>> +- RKISP1_V10: used at least in rk3288 and rk3399
>>> +- RKISP1_V11: declared in the original vendor code, but not used
>>> +- RKISP1_V12: used at least in rk3326 and px30
>>> +- RKISP1_V13: used at least in rk1808
>>> +
>>>   Topology
>>>   ========
>>>   .. _rkisp1_topology_graph:
>>> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
>>> index 68da1eed753d..f7e9fd305548 100644
>>> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
>>> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
>>> @@ -104,6 +104,7 @@
>>>   struct rkisp1_match_data {
>>>   	const char * const *clks;
>>>   	unsigned int size;
>>> +	enum rkisp1_cif_isp_version isp_ver;
>>>   };
>>>   
>>>   /* ----------------------------------------------------------------------------
>>> @@ -411,15 +412,16 @@ static const char * const rk3399_isp_clks[] = {
>>>   	"hclk",
>>>   };
>>>   
>>> -static const struct rkisp1_match_data rk3399_isp_clk_data = {
>>> +static const struct rkisp1_match_data rk3399_isp_match_data = {
>>>   	.clks = rk3399_isp_clks,
>>>   	.size = ARRAY_SIZE(rk3399_isp_clks),
>>> +	.isp_ver = RKISP1_V10,
>>>   };
>>>   
>>>   static const struct of_device_id rkisp1_of_match[] = {
>>>   	{
>>>   		.compatible = "rockchip,rk3399-cif-isp",
>>> -		.data = &rk3399_isp_clk_data,
>>> +		.data = &rk3399_isp_match_data,
>>>   	},
>>>   	{},
>>>   };
>>> @@ -457,15 +459,15 @@ static void rkisp1_debug_init(struct rkisp1_device *rkisp1)
>>>   
>>>   static int rkisp1_probe(struct platform_device *pdev)
>>>   {
>>> -	const struct rkisp1_match_data *clk_data;
>>> +	const struct rkisp1_match_data *match_data;
>>>   	struct device *dev = &pdev->dev;
>>>   	struct rkisp1_device *rkisp1;
>>>   	struct v4l2_device *v4l2_dev;
>>>   	unsigned int i;
>>>   	int ret, irq;
>>>   
>>> -	clk_data = of_device_get_match_data(&pdev->dev);
>>> -	if (!clk_data)
>>> +	match_data = of_device_get_match_data(&pdev->dev);
>>> +	if (!match_data)
>>>   		return -ENODEV;
>>>   
>>>   	rkisp1 = devm_kzalloc(dev, sizeof(*rkisp1), GFP_KERNEL);
>>> @@ -494,15 +496,16 @@ static int rkisp1_probe(struct platform_device *pdev)
>>>   
>>>   	rkisp1->irq = irq;
>>>   
>>> -	for (i = 0; i < clk_data->size; i++)
>>> -		rkisp1->clks[i].id = clk_data->clks[i];
>>> -	ret = devm_clk_bulk_get(dev, clk_data->size, rkisp1->clks);
>>> +	for (i = 0; i < match_data->size; i++)
>>> +		rkisp1->clks[i].id = match_data->clks[i];
>>> +	ret = devm_clk_bulk_get(dev, match_data->size, rkisp1->clks);
>>>   	if (ret)
>>>   		return ret;
>>> -	rkisp1->clk_size = clk_data->size;
>>> +	rkisp1->clk_size = match_data->size;
>>>   
>>>   	pm_runtime_enable(&pdev->dev);
>>>   
>>> +	rkisp1->media_dev.hw_revision = match_data->isp_ver;
>>>   	strscpy(rkisp1->media_dev.model, RKISP1_DRIVER_NAME,
>>>   		sizeof(rkisp1->media_dev.model));
>>>   	rkisp1->media_dev.dev = &pdev->dev;
>>> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
>>> index ba443771cc0b..9fecb6bc457d 100644
>>> --- a/include/uapi/linux/rkisp1-config.h
>>> +++ b/include/uapi/linux/rkisp1-config.h
>>> @@ -124,6 +124,22 @@
>>>   #define RKISP1_CIF_ISP_STAT_AFM           (1U << 2)
>>>   #define RKISP1_CIF_ISP_STAT_HIST          (1U << 3)
>>>   
>>> +
>>> +/**
>>> + * enum rkisp1_cif_isp_version - ISP variants
>>> + *
>>> + * @RKISP1_V10: used at least in rk3288 and rk3399
>>> + * @RKISP1_V11: declared in the original vendor code, but not used
>>> + * @RKISP1_V12: used at least in rk3326 and px30
>>> + * @RKISP1_V13: used at least in rk1808
>>> + */
>>> +enum rkisp1_cif_isp_version {
>>> +	RKISP1_V10 = 0,

I wonder if it is useful to start the enum with:
RKISP1_V10 = 10,

So when printing the hw revision it always prints a positive number.

Thanks,
Dafna

>>> +	RKISP1_V11,
>>> +	RKISP1_V12,
>>> +	RKISP1_V13,
>>> +};
>>> +
>>>   enum rkisp1_cif_isp_histogram_mode {
>>>   	RKISP1_CIF_ISP_HISTOGRAM_MODE_DISABLE,
>>>   	RKISP1_CIF_ISP_HISTOGRAM_MODE_RGB_COMBINED,
>>
>>
> 
> 
> 
> 

Re: [PATCH v4 2/3] media: rockchip: rkisp1: carry ip version information

[Heiko Stübner]

Hi Dafna,

Am Montag, 18. Januar 2021, 10:19:41 CET schrieb Dafna Hirschfeld:
> Am 15.01.21 um 21:29 schrieb Heiko Stübner:
> > Am Freitag, 15. Januar 2021, 20:42:41 CET schrieb Laurent Pinchart:
> >> On Fri, Jan 15, 2021 at 05:38:28PM +0100, Heiko Stuebner wrote:
> >>> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> >>>
> >>> The IP block evolved from its rk3288/rk3399 base and the vendor
> >>> designates them with a numerical version. rk3399 for example
> >>> is designated V10 probably meaning V1.0.
> >>>
> >>> There doesn't seem to be an actual version register we could read that
> >>> information from, so allow the match_data to carry that information
> >>> for future differentiation.
> >>>
> >>> Also carry that information in the hw_revision field of the media-
> >>> controller API, so that userspace also has access to that.
> >>>
> >>> The added versions are:
> >>> - V10: at least rk3288 + rk3399
> >>> - V11: seemingly unused as of now, but probably appeared in some soc
> >>> - V12: at least rk3326 + px30
> >>> - V13: at least rk1808
> >>
> >> In addition to changes to the ISP itself, are there changes in how it is
> >> integrated in the system, in particular in how it is connected to CSI-2
> >> receivers ?
> > 
> > Hooking up my camera on the px30 was pretty straightforward.
> > As you can see in [0] the actual csi-phy block is different, but structurally
> > everything else stayed the same and the px30 series merely accounts
> > for some isp-specific changes, like the split interrupts and the V12-specific
> > parts.
> > 
> > 
> >> Do all the above SoCs have two ISP instances ?
> > 
> > Nope, I think only the rk3399 has two ISPs.
> > 
> > All others only seem to have one instance and maybe an additional more
> > simple video input processor. (like the rk3288 has both an ISP and something
> > called VIP - a simpler block)
> > 
> > Similar is the rk3326/px30 (same soc) which has an ISP as well as a VIP that
> > Maxime Chevallier is working on [1]
> > 
> > Heiko
> > 
> > [0] https://lore.kernel.org/linux-media/20210108193311.3423236-1-heiko@sntech.de/
> > [1] https://lore.kernel.org/linux-devicetree/20201229161724.511102-1-maxime.chevallier@bootlin.com/
> > 
> > 
> >>> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> >>> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
> >>> ---
> >>>   Documentation/admin-guide/media/rkisp1.rst    | 16 ++++++++++++++
> >>>   .../platform/rockchip/rkisp1/rkisp1-dev.c     | 21 +++++++++++--------
> >>>   include/uapi/linux/rkisp1-config.h            | 16 ++++++++++++++
> >>>   3 files changed, 44 insertions(+), 9 deletions(-)
> >>>
> >>> diff --git a/Documentation/admin-guide/media/rkisp1.rst b/Documentation/admin-guide/media/rkisp1.rst
> >>> index 2267e4fb475e..ccf418713623 100644
> >>> --- a/Documentation/admin-guide/media/rkisp1.rst
> >>> +++ b/Documentation/admin-guide/media/rkisp1.rst
> >>> @@ -13,6 +13,22 @@ This file documents the driver for the Rockchip ISP1 that is part of RK3288
> >>>   and RK3399 SoCs. The driver is located under drivers/staging/media/rkisp1
> >>>   and uses the Media-Controller API.
> >>>   
> >>> +Revisions
> >>> +=========
> >>> +
> >>> +There exist multiple smaller revisions to this ISP that got introduced in
> >>> +later SoCs. Revisions can be found in the enum :c:type:`rkisp1_cif_isp_version`
> >>> +in the UAPI and the revision of the ISP inside the running SoC can be read
> >>> +in the field hw_revision of struct media_device_info as returned by
> >>> +ioctl MEDIA_IOC_DEVICE_INFO.
> >>> +
> >>> +Versions in use are:
> >>> +
> >>> +- RKISP1_V10: used at least in rk3288 and rk3399
> >>> +- RKISP1_V11: declared in the original vendor code, but not used
> >>> +- RKISP1_V12: used at least in rk3326 and px30
> >>> +- RKISP1_V13: used at least in rk1808
> >>> +
> >>>   Topology
> >>>   ========
> >>>   .. _rkisp1_topology_graph:
> >>> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> >>> index 68da1eed753d..f7e9fd305548 100644
> >>> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> >>> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> >>> @@ -104,6 +104,7 @@
> >>>   struct rkisp1_match_data {
> >>>   	const char * const *clks;
> >>>   	unsigned int size;
> >>> +	enum rkisp1_cif_isp_version isp_ver;
> >>>   };
> >>>   
> >>>   /* ----------------------------------------------------------------------------
> >>> @@ -411,15 +412,16 @@ static const char * const rk3399_isp_clks[] = {
> >>>   	"hclk",
> >>>   };
> >>>   
> >>> -static const struct rkisp1_match_data rk3399_isp_clk_data = {
> >>> +static const struct rkisp1_match_data rk3399_isp_match_data = {
> >>>   	.clks = rk3399_isp_clks,
> >>>   	.size = ARRAY_SIZE(rk3399_isp_clks),
> >>> +	.isp_ver = RKISP1_V10,
> >>>   };
> >>>   
> >>>   static const struct of_device_id rkisp1_of_match[] = {
> >>>   	{
> >>>   		.compatible = "rockchip,rk3399-cif-isp",
> >>> -		.data = &rk3399_isp_clk_data,
> >>> +		.data = &rk3399_isp_match_data,
> >>>   	},
> >>>   	{},
> >>>   };
> >>> @@ -457,15 +459,15 @@ static void rkisp1_debug_init(struct rkisp1_device *rkisp1)
> >>>   
> >>>   static int rkisp1_probe(struct platform_device *pdev)
> >>>   {
> >>> -	const struct rkisp1_match_data *clk_data;
> >>> +	const struct rkisp1_match_data *match_data;
> >>>   	struct device *dev = &pdev->dev;
> >>>   	struct rkisp1_device *rkisp1;
> >>>   	struct v4l2_device *v4l2_dev;
> >>>   	unsigned int i;
> >>>   	int ret, irq;
> >>>   
> >>> -	clk_data = of_device_get_match_data(&pdev->dev);
> >>> -	if (!clk_data)
> >>> +	match_data = of_device_get_match_data(&pdev->dev);
> >>> +	if (!match_data)
> >>>   		return -ENODEV;
> >>>   
> >>>   	rkisp1 = devm_kzalloc(dev, sizeof(*rkisp1), GFP_KERNEL);
> >>> @@ -494,15 +496,16 @@ static int rkisp1_probe(struct platform_device *pdev)
> >>>   
> >>>   	rkisp1->irq = irq;
> >>>   
> >>> -	for (i = 0; i < clk_data->size; i++)
> >>> -		rkisp1->clks[i].id = clk_data->clks[i];
> >>> -	ret = devm_clk_bulk_get(dev, clk_data->size, rkisp1->clks);
> >>> +	for (i = 0; i < match_data->size; i++)
> >>> +		rkisp1->clks[i].id = match_data->clks[i];
> >>> +	ret = devm_clk_bulk_get(dev, match_data->size, rkisp1->clks);
> >>>   	if (ret)
> >>>   		return ret;
> >>> -	rkisp1->clk_size = clk_data->size;
> >>> +	rkisp1->clk_size = match_data->size;
> >>>   
> >>>   	pm_runtime_enable(&pdev->dev);
> >>>   
> >>> +	rkisp1->media_dev.hw_revision = match_data->isp_ver;
> >>>   	strscpy(rkisp1->media_dev.model, RKISP1_DRIVER_NAME,
> >>>   		sizeof(rkisp1->media_dev.model));
> >>>   	rkisp1->media_dev.dev = &pdev->dev;
> >>> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> >>> index ba443771cc0b..9fecb6bc457d 100644
> >>> --- a/include/uapi/linux/rkisp1-config.h
> >>> +++ b/include/uapi/linux/rkisp1-config.h
> >>> @@ -124,6 +124,22 @@
> >>>   #define RKISP1_CIF_ISP_STAT_AFM           (1U << 2)
> >>>   #define RKISP1_CIF_ISP_STAT_HIST          (1U << 3)
> >>>   
> >>> +
> >>> +/**
> >>> + * enum rkisp1_cif_isp_version - ISP variants
> >>> + *
> >>> + * @RKISP1_V10: used at least in rk3288 and rk3399
> >>> + * @RKISP1_V11: declared in the original vendor code, but not used
> >>> + * @RKISP1_V12: used at least in rk3326 and px30
> >>> + * @RKISP1_V13: used at least in rk1808
> >>> + */
> >>> +enum rkisp1_cif_isp_version {
> >>> +	RKISP1_V10 = 0,
> 
> I wonder if it is useful to start the enum with:
> RKISP1_V10 = 10,
> 
> So when printing the hw revision it always prints a positive number.

makes sense ... also if userspace prints the value somewhere, it's
readable without knowing the constants - will change with the v5
I'm preparing.

Heiko

Re: [PATCH v4 2/3] media: rockchip: rkisp1: carry ip version information

[Laurent Pinchart]

On Mon, Jan 18, 2021 at 10:21:40AM +0100, Heiko Stübner wrote:
> Am Montag, 18. Januar 2021, 10:19:41 CET schrieb Dafna Hirschfeld:
> > Am 15.01.21 um 21:29 schrieb Heiko Stübner:
> >> Am Freitag, 15. Januar 2021, 20:42:41 CET schrieb Laurent Pinchart:
> >>> On Fri, Jan 15, 2021 at 05:38:28PM +0100, Heiko Stuebner wrote:
> >>>> From: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> >>>>
> >>>> The IP block evolved from its rk3288/rk3399 base and the vendor
> >>>> designates them with a numerical version. rk3399 for example
> >>>> is designated V10 probably meaning V1.0.
> >>>>
> >>>> There doesn't seem to be an actual version register we could read that
> >>>> information from, so allow the match_data to carry that information
> >>>> for future differentiation.
> >>>>
> >>>> Also carry that information in the hw_revision field of the media-
> >>>> controller API, so that userspace also has access to that.
> >>>>
> >>>> The added versions are:
> >>>> - V10: at least rk3288 + rk3399
> >>>> - V11: seemingly unused as of now, but probably appeared in some soc
> >>>> - V12: at least rk3326 + px30
> >>>> - V13: at least rk1808
> >>>
> >>> In addition to changes to the ISP itself, are there changes in how it is
> >>> integrated in the system, in particular in how it is connected to CSI-2
> >>> receivers ?
> >> 
> >> Hooking up my camera on the px30 was pretty straightforward.
> >> As you can see in [0] the actual csi-phy block is different, but structurally
> >> everything else stayed the same and the px30 series merely accounts
> >> for some isp-specific changes, like the split interrupts and the V12-specific
> >> parts.
> >> 
> >>> Do all the above SoCs have two ISP instances ?
> >> 
> >> Nope, I think only the rk3399 has two ISPs.
> >> 
> >> All others only seem to have one instance and maybe an additional more
> >> simple video input processor. (like the rk3288 has both an ISP and something
> >> called VIP - a simpler block)
> >> 
> >> Similar is the rk3326/px30 (same soc) which has an ISP as well as a VIP that
> >> Maxime Chevallier is working on [1]
> >> 
> >> Heiko
> >> 
> >> [0] https://lore.kernel.org/linux-media/20210108193311.3423236-1-heiko@sntech.de/
> >> [1] https://lore.kernel.org/linux-devicetree/20201229161724.511102-1-maxime.chevallier@bootlin.com/
> >> 
> >>>> Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
> >>>> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
> >>>> ---
> >>>>   Documentation/admin-guide/media/rkisp1.rst    | 16 ++++++++++++++
> >>>>   .../platform/rockchip/rkisp1/rkisp1-dev.c     | 21 +++++++++++--------
> >>>>   include/uapi/linux/rkisp1-config.h            | 16 ++++++++++++++
> >>>>   3 files changed, 44 insertions(+), 9 deletions(-)
> >>>>
> >>>> diff --git a/Documentation/admin-guide/media/rkisp1.rst b/Documentation/admin-guide/media/rkisp1.rst
> >>>> index 2267e4fb475e..ccf418713623 100644
> >>>> --- a/Documentation/admin-guide/media/rkisp1.rst
> >>>> +++ b/Documentation/admin-guide/media/rkisp1.rst
> >>>> @@ -13,6 +13,22 @@ This file documents the driver for the Rockchip ISP1 that is part of RK3288
> >>>>   and RK3399 SoCs. The driver is located under drivers/staging/media/rkisp1
> >>>>   and uses the Media-Controller API.
> >>>>   
> >>>> +Revisions
> >>>> +=========
> >>>> +
> >>>> +There exist multiple smaller revisions to this ISP that got introduced in
> >>>> +later SoCs. Revisions can be found in the enum :c:type:`rkisp1_cif_isp_version`
> >>>> +in the UAPI and the revision of the ISP inside the running SoC can be read
> >>>> +in the field hw_revision of struct media_device_info as returned by
> >>>> +ioctl MEDIA_IOC_DEVICE_INFO.
> >>>> +
> >>>> +Versions in use are:
> >>>> +
> >>>> +- RKISP1_V10: used at least in rk3288 and rk3399
> >>>> +- RKISP1_V11: declared in the original vendor code, but not used
> >>>> +- RKISP1_V12: used at least in rk3326 and px30
> >>>> +- RKISP1_V13: used at least in rk1808
> >>>> +
> >>>>   Topology
> >>>>   ========
> >>>>   .. _rkisp1_topology_graph:
> >>>> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> >>>> index 68da1eed753d..f7e9fd305548 100644
> >>>> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> >>>> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> >>>> @@ -104,6 +104,7 @@
> >>>>   struct rkisp1_match_data {
> >>>>   	const char * const *clks;
> >>>>   	unsigned int size;
> >>>> +	enum rkisp1_cif_isp_version isp_ver;
> >>>>   };
> >>>>   
> >>>>   /* ----------------------------------------------------------------------------
> >>>> @@ -411,15 +412,16 @@ static const char * const rk3399_isp_clks[] = {
> >>>>   	"hclk",
> >>>>   };
> >>>>   
> >>>> -static const struct rkisp1_match_data rk3399_isp_clk_data = {
> >>>> +static const struct rkisp1_match_data rk3399_isp_match_data = {
> >>>>   	.clks = rk3399_isp_clks,
> >>>>   	.size = ARRAY_SIZE(rk3399_isp_clks),
> >>>> +	.isp_ver = RKISP1_V10,
> >>>>   };
> >>>>   
> >>>>   static const struct of_device_id rkisp1_of_match[] = {
> >>>>   	{
> >>>>   		.compatible = "rockchip,rk3399-cif-isp",
> >>>> -		.data = &rk3399_isp_clk_data,
> >>>> +		.data = &rk3399_isp_match_data,
> >>>>   	},
> >>>>   	{},
> >>>>   };
> >>>> @@ -457,15 +459,15 @@ static void rkisp1_debug_init(struct rkisp1_device *rkisp1)
> >>>>   
> >>>>   static int rkisp1_probe(struct platform_device *pdev)
> >>>>   {
> >>>> -	const struct rkisp1_match_data *clk_data;
> >>>> +	const struct rkisp1_match_data *match_data;
> >>>>   	struct device *dev = &pdev->dev;
> >>>>   	struct rkisp1_device *rkisp1;
> >>>>   	struct v4l2_device *v4l2_dev;
> >>>>   	unsigned int i;
> >>>>   	int ret, irq;
> >>>>   
> >>>> -	clk_data = of_device_get_match_data(&pdev->dev);
> >>>> -	if (!clk_data)
> >>>> +	match_data = of_device_get_match_data(&pdev->dev);
> >>>> +	if (!match_data)
> >>>>   		return -ENODEV;
> >>>>   
> >>>>   	rkisp1 = devm_kzalloc(dev, sizeof(*rkisp1), GFP_KERNEL);
> >>>> @@ -494,15 +496,16 @@ static int rkisp1_probe(struct platform_device *pdev)
> >>>>   
> >>>>   	rkisp1->irq = irq;
> >>>>   
> >>>> -	for (i = 0; i < clk_data->size; i++)
> >>>> -		rkisp1->clks[i].id = clk_data->clks[i];
> >>>> -	ret = devm_clk_bulk_get(dev, clk_data->size, rkisp1->clks);
> >>>> +	for (i = 0; i < match_data->size; i++)
> >>>> +		rkisp1->clks[i].id = match_data->clks[i];
> >>>> +	ret = devm_clk_bulk_get(dev, match_data->size, rkisp1->clks);
> >>>>   	if (ret)
> >>>>   		return ret;
> >>>> -	rkisp1->clk_size = clk_data->size;
> >>>> +	rkisp1->clk_size = match_data->size;
> >>>>   
> >>>>   	pm_runtime_enable(&pdev->dev);
> >>>>   
> >>>> +	rkisp1->media_dev.hw_revision = match_data->isp_ver;
> >>>>   	strscpy(rkisp1->media_dev.model, RKISP1_DRIVER_NAME,
> >>>>   		sizeof(rkisp1->media_dev.model));
> >>>>   	rkisp1->media_dev.dev = &pdev->dev;
> >>>> diff --git a/include/uapi/linux/rkisp1-config.h b/include/uapi/linux/rkisp1-config.h
> >>>> index ba443771cc0b..9fecb6bc457d 100644
> >>>> --- a/include/uapi/linux/rkisp1-config.h
> >>>> +++ b/include/uapi/linux/rkisp1-config.h
> >>>> @@ -124,6 +124,22 @@
> >>>>   #define RKISP1_CIF_ISP_STAT_AFM           (1U << 2)
> >>>>   #define RKISP1_CIF_ISP_STAT_HIST          (1U << 3)
> >>>>   
> >>>> +
> >>>> +/**
> >>>> + * enum rkisp1_cif_isp_version - ISP variants
> >>>> + *
> >>>> + * @RKISP1_V10: used at least in rk3288 and rk3399
> >>>> + * @RKISP1_V11: declared in the original vendor code, but not used
> >>>> + * @RKISP1_V12: used at least in rk3326 and px30
> >>>> + * @RKISP1_V13: used at least in rk1808
> >>>> + */
> >>>> +enum rkisp1_cif_isp_version {
> >>>> +	RKISP1_V10 = 0,
> > 
> > I wonder if it is useful to start the enum with:
> > RKISP1_V10 = 10,
> > 
> > So when printing the hw revision it always prints a positive number.
> 
> makes sense ... also if userspace prints the value somewhere, it's
> readable without knowing the constants - will change with the v5
> I'm preparing.

It's also useful for userspace to differenciate between 0 = RKISP1_V10
and 0 = older driver that doesn't set the version.

-- 
Regards,

Laurent Pinchart