[PATCHv2 0/2] v4l: vsp1: Add HGT support

[PATCHv2 0/2] v4l: vsp1: Add HGT support

[Niklas Söderlund]

Hi,

This series add support for the VSP1 2-D histogram engine HGT.

It's based on top of Laurent Pinchart tree at 
git://linuxtv.org/pinchartl/media.git hgo. And depends on Laurents patch 
'[PATCH] v4l: vsp1: Move subdev operations from HGO to common histogram 
code'.

It is tested on Koelsch using a modified vsp-tests suite package,
modifications can be found at https://git.ragnatech.se/vsp-tests hgt.

* Changes since v1
- Rebased on top of Laurents patch which made all subdev operations 
  common for HGO and HGT. This removed a lot of code that is now shared.
- Removed the Hue area configuration for the histogram pixel format.  
  These values are set by userspace so it already knows them.
- Updated pixel format documentation after input from Laurent.
- Better aligned the code to the existing VSP code base.
- Simplify check that hue areas are valid for the hardware.
- Fixed spelling.

Niklas Söderlund (2):
  v4l: Define a pixel format for the R-Car VSP1 2-D histogram engine
  v4l: vsp1: Add HGT support

 Documentation/media/uapi/v4l/meta-formats.rst      |   1 +
 .../media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst        | 122 ++++++++++++
 drivers/media/platform/vsp1/Makefile               |   2 +-
 drivers/media/platform/vsp1/vsp1.h                 |   3 +
 drivers/media/platform/vsp1/vsp1_drv.c             |  33 +++-
 drivers/media/platform/vsp1/vsp1_entity.c          |  33 +++-
 drivers/media/platform/vsp1/vsp1_entity.h          |   1 +
 drivers/media/platform/vsp1/vsp1_hgt.c             | 217 +++++++++++++++++++++
 drivers/media/platform/vsp1/vsp1_hgt.h             |  42 ++++
 drivers/media/platform/vsp1/vsp1_pipe.c            |  16 ++
 drivers/media/platform/vsp1/vsp1_pipe.h            |   2 +
 drivers/media/platform/vsp1/vsp1_regs.h            |   9 +
 drivers/media/platform/vsp1/vsp1_video.c           |  10 +-
 drivers/media/v4l2-core/v4l2-ioctl.c               |   1 +
 include/uapi/linux/videodev2.h                     |   3 +-
 15 files changed, 478 insertions(+), 17 deletions(-)
 create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst
 create mode 100644 drivers/media/platform/vsp1/vsp1_hgt.c
 create mode 100644 drivers/media/platform/vsp1/vsp1_hgt.h

-- 
2.9.3

[PATCHv2 1/2] v4l: Define a pixel format for the R-Car VSP1 2-D histogram engine

[Niklas Söderlund]

The format is used on the R-Car VSP1 video queues that carry
2-D histogram statistics data.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 Documentation/media/uapi/v4l/meta-formats.rst      |   1 +
 .../media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst        | 122 +++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-ioctl.c               |   1 +
 include/uapi/linux/videodev2.h                     |   3 +-
 4 files changed, 126 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst

diff --git a/Documentation/media/uapi/v4l/meta-formats.rst b/Documentation/media/uapi/v4l/meta-formats.rst
index 05ab91e..01e24e3 100644
--- a/Documentation/media/uapi/v4l/meta-formats.rst
+++ b/Documentation/media/uapi/v4l/meta-formats.rst
@@ -13,3 +13,4 @@ These formats are used for the :ref:`metadata` interface only.
     :maxdepth: 1
 
     pixfmt-meta-vsp1-hgo
+    pixfmt-meta-vsp1-hgt
diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst b/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst
new file mode 100644
index 0000000..0393148
--- /dev/null
+++ b/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst
@@ -0,0 +1,122 @@
+.. -*- coding: utf-8; mode: rst -*-
+
+.. _v4l2-meta-fmt-vsp1-hgt:
+
+*******************************
+V4L2_META_FMT_VSP1_HGT ('VSPT')
+*******************************
+
+*man V4L2_META_FMT_VSP1_HGT(2)*
+
+Renesas R-Car VSP1 2-D Histogram Data
+
+
+Description
+===========
+
+This format describes histogram data generated by the Renesas R-Car VSP1
+2-D Histogram (HGT) engine.
+
+The VSP1 HGT is a histogram computation engine that operates on HSV
+data. It operates on a possibly cropped and subsampled input image and
+computes the sum, maximum and minimum of the S component as well as a
+weighted frequency histogram based on the H and S components.
+
+The histogram is a matrix of 6 Hue and 32 Saturation buckets, 192 in
+total. Each HSV value is added to one or more buckets with a weight
+between 1 and 16 depending on the Hue areas configuration. Finding the
+corresponding buckets is done by inspecting the H and S value independently.
+
+The Saturation position **n** (0 - 31) of the bucket in the matrix is
+found by the expression:
+
+    n = S / 8
+
+The Hue position **m** (0 - 5) of the bucket in the matrix depends on
+how the HGT Hue areas are configured. There are 6 user configurable Hue
+Areas which can be configured to cover overlapping Hue values:
+
+::
+
+         Area 0       Area 1       Area 2       Area 3       Area 4       Area 5
+        ________     ________     ________     ________     ________     ________
+   \   /|      |\   /|      |\   /|      |\   /|      |\   /|      |\   /|      |\   /
+    \ / |      | \ / |      | \ / |      | \ / |      | \ / |      | \ / |      | \ /
+     X  |      |  X  |      |  X  |      |  X  |      |  X  |      |  X  |      |  X
+    / \ |      | / \ |      | / \ |      | / \ |      | / \ |      | / \ |      | / \
+   /   \|      |/   \|      |/   \|      |/   \|      |/   \|      |/   \|      |/   \
+  5U   0L      0U   1L      1U   2L      2U   3L      3U   4L      4U   5L      5U   0L
+        <0..............................Hue Value............................255>
+
+When two consecutive areas don't overlap (n+1L is equal to nU) the boundary
+value is considered as part of the lower area.
+
+Pixels with a hue value included in the centre of an area (between nL and nU
+included) are are attributed to that single area and given a weight of 16.
+Pixels with a hue value included in the overlapping region between two areas
+(between n+1L and nU excluded) are attributed to both areas and given a weight
+for each of these areas proportional to their position along the diagonal
+lines (rounded down)."
+
+The Hue area setup must match one of the following constrains:
+
+::
+
+    0L <= 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U
+
+::
+
+    0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U <= 0L
+
+**Byte Order.**
+All data is stored in memory in little endian format. Each cell in the tables
+contains one byte.
+
+.. flat-table:: VSP1 HGT Data - (776 bytes)
+    :header-rows:  2
+    :stub-columns: 0
+
+    * - Offset
+      - :cspan:`4` Memory
+    * -
+      - [31:24]
+      - [23:16]
+      - [15:8]
+      - [7:0]
+    * - 0
+      - -
+      - S max [7:0]
+      - -
+      - S min [7:0]
+    * - 4
+      - :cspan:`4` S sum [31:0]
+    * - 8
+      - :cspan:`4` Histogram bucket (m=0, n=0) [31:0]
+    * - 12
+      - :cspan:`4` Histogram bucket (m=0, n=1) [31:0]
+    * -
+      - :cspan:`4` ...
+    * - 132
+      - :cspan:`4` Histogram bucket (m=0, n=31) [31:0]
+    * - 136
+      - :cspan:`4` Histogram bucket (m=1, n=0) [31:0]
+    * -
+      - :cspan:`4` ...
+    * - 264
+      - :cspan:`4` Histogram bucket (m=2, n=0) [31:0]
+    * -
+      - :cspan:`4` ...
+    * - 392
+      - :cspan:`4` Histogram bucket (m=3, n=0) [31:0]
+    * -
+      - :cspan:`4` ...
+    * - 520
+      - :cspan:`4` Histogram bucket (m=4, n=0) [31:0]
+    * -
+      - :cspan:`4` ...
+    * - 648
+      - :cspan:`4` Histogram bucket (m=5, n=0) [31:0]
+    * -
+      - :cspan:`4` ...
+    * - 772
+      - :cspan:`4` Histogram bucket (m=5, n=31) [31:0]
diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c b/drivers/media/v4l2-core/v4l2-ioctl.c
index b7f7d5f..f459c4f 100644
--- a/drivers/media/v4l2-core/v4l2-ioctl.c
+++ b/drivers/media/v4l2-core/v4l2-ioctl.c
@@ -1259,6 +1259,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
 	case V4L2_SDR_FMT_CS14LE:	descr = "Complex S14LE"; break;
 	case V4L2_SDR_FMT_RU12LE:	descr = "Real U12LE"; break;
 	case V4L2_META_FMT_VSP1_HGO:	descr = "R-Car VSP1 1-D Histogram"; break;
+	case V4L2_META_FMT_VSP1_HGT:	descr = "R-Car VSP1 2-D Histogram"; break;
 
 	default:
 		/* Compressed formats */
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 1dbe52a..c8c046c 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -638,7 +638,8 @@ struct v4l2_pix_format {
 #define V4L2_SDR_FMT_RU12LE       v4l2_fourcc('R', 'U', '1', '2') /* real u12le */
 
 /* Meta-data formats */
-#define V4L2_META_FMT_VSP1_HGO    v4l2_fourcc('V', 'S', 'P', 'H') /* R-Car VSP1 Histogram */
+#define V4L2_META_FMT_VSP1_HGO    v4l2_fourcc('V', 'S', 'P', 'H') /* R-Car VSP1 1-D Histogram */
+#define V4L2_META_FMT_VSP1_HGT    v4l2_fourcc('V', 'S', 'P', 'T') /* R-Car VSP1 2-D Histogram */
 
 /* priv field value to indicates that subsequent fields are valid. */
 #define V4L2_PIX_FMT_PRIV_MAGIC		0xfeedcafe
-- 
2.9.3

[PATCHv2 2/2] v4l: vsp1: Add HGT support

[Niklas Söderlund]

The HGT is a Histogram Generator Two-Dimensions. It computes a weighted
frequency histograms for hue and saturation areas over a configurable
region of the image with optional subsampling.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/vsp1/Makefile      |   2 +-
 drivers/media/platform/vsp1/vsp1.h        |   3 +
 drivers/media/platform/vsp1/vsp1_drv.c    |  33 ++++-
 drivers/media/platform/vsp1/vsp1_entity.c |  33 +++--
 drivers/media/platform/vsp1/vsp1_entity.h |   1 +
 drivers/media/platform/vsp1/vsp1_hgt.c    | 217 ++++++++++++++++++++++++++++++
 drivers/media/platform/vsp1/vsp1_hgt.h    |  42 ++++++
 drivers/media/platform/vsp1/vsp1_pipe.c   |  16 +++
 drivers/media/platform/vsp1/vsp1_pipe.h   |   2 +
 drivers/media/platform/vsp1/vsp1_regs.h   |   9 ++
 drivers/media/platform/vsp1/vsp1_video.c  |  10 +-
 11 files changed, 352 insertions(+), 16 deletions(-)
 create mode 100644 drivers/media/platform/vsp1/vsp1_hgt.c
 create mode 100644 drivers/media/platform/vsp1/vsp1_hgt.h

diff --git a/drivers/media/platform/vsp1/Makefile b/drivers/media/platform/vsp1/Makefile
index 8ab6a06..a33afc3 100644
--- a/drivers/media/platform/vsp1/Makefile
+++ b/drivers/media/platform/vsp1/Makefile
@@ -3,7 +3,7 @@ vsp1-y					+= vsp1_dl.o vsp1_drm.o vsp1_video.o
 vsp1-y					+= vsp1_rpf.o vsp1_rwpf.o vsp1_wpf.o
 vsp1-y					+= vsp1_clu.o vsp1_hsit.o vsp1_lut.o
 vsp1-y					+= vsp1_bru.o vsp1_sru.o vsp1_uds.o
-vsp1-y					+= vsp1_hgo.o vsp1_histo.o
+vsp1-y					+= vsp1_hgo.o vsp1_hgt.o vsp1_histo.o
 vsp1-y					+= vsp1_lif.o
 
 obj-$(CONFIG_VIDEO_RENESAS_VSP1)	+= vsp1.o
diff --git a/drivers/media/platform/vsp1/vsp1.h b/drivers/media/platform/vsp1/vsp1.h
index 9dce3ea..012ce40 100644
--- a/drivers/media/platform/vsp1/vsp1.h
+++ b/drivers/media/platform/vsp1/vsp1.h
@@ -33,6 +33,7 @@ struct vsp1_platform_data;
 struct vsp1_bru;
 struct vsp1_clu;
 struct vsp1_hgo;
+struct vsp1_hgt;
 struct vsp1_hsit;
 struct vsp1_lif;
 struct vsp1_lut;
@@ -52,6 +53,7 @@ struct vsp1_uds;
 #define VSP1_HAS_WPF_VFLIP	(1 << 5)
 #define VSP1_HAS_WPF_HFLIP	(1 << 6)
 #define VSP1_HAS_HGO		(1 << 7)
+#define VSP1_HAS_HGT		(1 << 8)
 
 struct vsp1_device_info {
 	u32 version;
@@ -74,6 +76,7 @@ struct vsp1_device {
 	struct vsp1_bru *bru;
 	struct vsp1_clu *clu;
 	struct vsp1_hgo *hgo;
+	struct vsp1_hgt *hgt;
 	struct vsp1_hsit *hsi;
 	struct vsp1_hsit *hst;
 	struct vsp1_lif *lif;
diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c
index 9ea4244..df97b57 100644
--- a/drivers/media/platform/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/vsp1/vsp1_drv.c
@@ -31,6 +31,7 @@
 #include "vsp1_dl.h"
 #include "vsp1_drm.h"
 #include "vsp1_hgo.h"
+#include "vsp1_hgt.h"
 #include "vsp1_hsit.h"
 #include "vsp1_lif.h"
 #include "vsp1_lut.h"
@@ -107,6 +108,7 @@ static int vsp1_create_sink_links(struct vsp1_device *vsp1,
 			continue;
 
 		if (source->type == VSP1_ENTITY_HGO ||
+		    source->type == VSP1_ENTITY_HGT ||
 		    source->type == VSP1_ENTITY_LIF ||
 		    source->type == VSP1_ENTITY_WPF)
 			continue;
@@ -160,6 +162,16 @@ static int vsp1_uapi_create_links(struct vsp1_device *vsp1)
 			return ret;
 	}
 
+	if (vsp1->hgt) {
+		ret = media_create_pad_link(&vsp1->hgt->histo.entity.subdev.entity,
+					    HISTO_PAD_SOURCE,
+					    &vsp1->hgt->histo.video.entity, 0,
+					    MEDIA_LNK_FL_ENABLED |
+					    MEDIA_LNK_FL_IMMUTABLE);
+		if (ret < 0)
+			return ret;
+	}
+
 	if (vsp1->lif) {
 		ret = media_create_pad_link(&vsp1->wpf[0]->entity.subdev.entity,
 					    RWPF_PAD_SOURCE,
@@ -301,6 +313,17 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
 			      &vsp1->entities);
 	}
 
+	if (vsp1->info->features & VSP1_HAS_HGT && vsp1->info->uapi) {
+		vsp1->hgt = vsp1_hgt_create(vsp1);
+		if (IS_ERR(vsp1->hgt)) {
+			ret = PTR_ERR(vsp1->hgt);
+			goto done;
+		}
+
+		list_add_tail(&vsp1->hgt->histo.entity.list_dev,
+			      &vsp1->entities);
+	}
+
 	/* The LIF is only supported when used in conjunction with the DU, in
 	 * which case the userspace API is disabled. If the userspace API is
 	 * enabled skip the LIF, even when present.
@@ -584,7 +607,8 @@ static const struct vsp1_device_info vsp1_device_infos[] = {
 		.version = VI6_IP_VERSION_MODEL_VSPS_H2,
 		.gen = 2,
 		.features = VSP1_HAS_BRU | VSP1_HAS_CLU | VSP1_HAS_HGO
-			  | VSP1_HAS_LUT | VSP1_HAS_SRU | VSP1_HAS_WPF_VFLIP,
+			  | VSP1_HAS_HGT | VSP1_HAS_LUT | VSP1_HAS_SRU
+			  | VSP1_HAS_WPF_VFLIP,
 		.rpf_count = 5,
 		.uds_count = 3,
 		.wpf_count = 4,
@@ -613,7 +637,8 @@ static const struct vsp1_device_info vsp1_device_infos[] = {
 		.version = VI6_IP_VERSION_MODEL_VSPS_M2,
 		.gen = 2,
 		.features = VSP1_HAS_BRU | VSP1_HAS_CLU | VSP1_HAS_HGO
-			  | VSP1_HAS_LUT | VSP1_HAS_SRU | VSP1_HAS_WPF_VFLIP,
+			  | VSP1_HAS_HGT | VSP1_HAS_LUT | VSP1_HAS_SRU
+			  | VSP1_HAS_WPF_VFLIP,
 		.rpf_count = 5,
 		.uds_count = 1,
 		.wpf_count = 4,
@@ -622,8 +647,8 @@ static const struct vsp1_device_info vsp1_device_infos[] = {
 	}, {
 		.version = VI6_IP_VERSION_MODEL_VSPI_GEN3,
 		.gen = 3,
-		.features = VSP1_HAS_CLU | VSP1_HAS_HGO | VSP1_HAS_LUT
-			  | VSP1_HAS_SRU | VSP1_HAS_WPF_HFLIP
+		.features = VSP1_HAS_CLU | VSP1_HAS_HGO | VSP1_HAS_HGT
+			  | VSP1_HAS_LUT | VSP1_HAS_SRU | VSP1_HAS_WPF_HFLIP
 			  | VSP1_HAS_WPF_VFLIP,
 		.rpf_count = 1,
 		.uds_count = 1,
diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c
index a6b7d87..0a0c46e 100644
--- a/drivers/media/platform/vsp1/vsp1_entity.c
+++ b/drivers/media/platform/vsp1/vsp1_entity.c
@@ -49,6 +49,18 @@ void vsp1_entity_route_setup(struct vsp1_entity *entity,
 
 		vsp1_dl_list_write(dl, VI6_DPR_HGO_SMPPT, smppt);
 		return;
+	} else if (entity->type == VSP1_ENTITY_HGT) {
+		u32 smppt;
+
+		/* The HGT is a special case, its routing is configured on the
+		 * sink pad.
+		 */
+		source = media_entity_to_vsp1_entity(entity->sources[0]);
+		smppt = (pipe->output->entity.index << VI6_DPR_SMPPT_TGW_SHIFT)
+		      | (source->route->output << VI6_DPR_SMPPT_PT_SHIFT);
+
+		vsp1_dl_list_write(dl, VI6_DPR_HGT_SMPPT, smppt);
+		return;
 	}
 
 	source = entity;
@@ -302,9 +314,10 @@ static int vsp1_entity_link_setup_source(const struct media_pad *source_pad,
 			= media_entity_to_vsp1_entity(sink_pad->entity);
 
 		/* Fan-out is limited to one for the normal data path plus an
-		 * optional HGO. We ignore the HGO here.
+		 * optional HGO or HGT. We ignore the HGO and HGT here.
 		 */
-		if (sink->type != VSP1_ENTITY_HGO) {
+		if (sink->type != VSP1_ENTITY_HGO &&
+		    sink->type != VSP1_ENTITY_HGT) {
 			if (source->sink)
 				return -EBUSY;
 			source->sink = sink_pad->entity;
@@ -357,10 +370,10 @@ int vsp1_entity_link_setup(struct media_entity *entity,
  * links originating or terminating at that pad until an enabled link is found.
  *
  * Our link setup implementation guarantees that the output fan-out will not be
- * higher than one for the data pipelines, except for the link to the HGO that
- * can be enabled in addition to a regular data link. When traversing outgoing
- * links this function ignores HGO entities and should thus be used in place of
- * the generic media_entity_remote_pad() function when traversing data
+ * higher than one for the data pipelines, except for the link to the HGO or HGT
+ * that can be enabled in addition to a regular data link. When traversing
+ * outgoing links this function ignores HGO entities and should thus be used in
+ * place of the generic media_entity_remote_pad() function when traversing data
  * pipelines.
  *
  * Return a pointer to the pad at the remote end of the first found enabled
@@ -376,19 +389,20 @@ struct media_pad *vsp1_entity_remote_pad(struct media_pad *pad)
 		if (!(link->flags & MEDIA_LNK_FL_ENABLED))
 			continue;
 
-		/* If we're the sink the source will never be an HGO. */
+		/* If we're the sink the source will never be an HGO or HGT. */
 		if (link->sink == pad)
 			return link->source;
 
 		if (link->source != pad)
 			continue;
 
-		/* If the sink isn't a subdevice it can't be an HGO. */
+		/* If the sink isn't a subdevice it can't be an HGO or HGT. */
 		if (!is_media_entity_v4l2_subdev(link->sink->entity))
 			return link->sink;
 
 		entity = media_entity_to_vsp1_entity(link->sink->entity);
-		if (entity->type != VSP1_ENTITY_HGO)
+		if (entity->type != VSP1_ENTITY_HGO &&
+		    entity->type != VSP1_ENTITY_HGT)
 			return link->sink;
 	}
 
@@ -423,6 +437,7 @@ static const struct vsp1_route vsp1_routes[] = {
 	    VI6_DPR_NODE_BRU_IN(4) }, VI6_DPR_NODE_BRU_OUT },
 	VSP1_ENTITY_ROUTE(CLU),
 	{ VSP1_ENTITY_HGO, 0, 0, { 0, }, 0 },
+	{ VSP1_ENTITY_HGT, 0, 0, { 0, }, 0 },
 	VSP1_ENTITY_ROUTE(HSI),
 	VSP1_ENTITY_ROUTE(HST),
 	{ VSP1_ENTITY_LIF, 0, 0, { VI6_DPR_NODE_LIF, }, VI6_DPR_NODE_LIF },
diff --git a/drivers/media/platform/vsp1/vsp1_entity.h b/drivers/media/platform/vsp1/vsp1_entity.h
index 4fc2cd1..0682400 100644
--- a/drivers/media/platform/vsp1/vsp1_entity.h
+++ b/drivers/media/platform/vsp1/vsp1_entity.h
@@ -26,6 +26,7 @@ enum vsp1_entity_type {
 	VSP1_ENTITY_BRU,
 	VSP1_ENTITY_CLU,
 	VSP1_ENTITY_HGO,
+	VSP1_ENTITY_HGT,
 	VSP1_ENTITY_HSI,
 	VSP1_ENTITY_HST,
 	VSP1_ENTITY_LIF,
diff --git a/drivers/media/platform/vsp1/vsp1_hgt.c b/drivers/media/platform/vsp1/vsp1_hgt.c
new file mode 100644
index 0000000..4e3f762
--- /dev/null
+++ b/drivers/media/platform/vsp1/vsp1_hgt.c
@@ -0,0 +1,217 @@
+/*
+ * vsp1_hgt.c  --  R-Car VSP1 Histogram Generator 2D
+ *
+ * Copyright (C) 2016 Renesas Electronics Corporation
+ *
+ * Contact: Niklas Söderlund (niklas.soderlund@ragnatech.se)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/device.h>
+#include <linux/gfp.h>
+
+#include <media/v4l2-subdev.h>
+#include <media/videobuf2-vmalloc.h>
+
+#include "vsp1.h"
+#include "vsp1_dl.h"
+#include "vsp1_hgt.h"
+
+#define HGT_DATA_SIZE				((2 +  6 * 32) * 4)
+
+/* -----------------------------------------------------------------------------
+ * Device Access
+ */
+
+static inline u32 vsp1_hgt_read(struct vsp1_hgt *hgt, u32 reg)
+{
+	return vsp1_read(hgt->histo.entity.vsp1, reg);
+}
+
+static inline void vsp1_hgt_write(struct vsp1_hgt *hgt, struct vsp1_dl_list *dl,
+				  u32 reg, u32 data)
+{
+	vsp1_dl_list_write(dl, reg, data);
+}
+
+/* -----------------------------------------------------------------------------
+ * Frame End Handler
+ */
+
+void vsp1_hgt_frame_end(struct vsp1_entity *entity)
+{
+	struct vsp1_hgt *hgt = to_hgt(&entity->subdev);
+	struct vsp1_histogram_buffer *buf;
+	unsigned int m;
+	unsigned int n;
+	u32 *data;
+
+	buf = vsp1_histogram_buffer_get(&hgt->histo);
+	if (!buf)
+		return;
+
+	data = buf->addr;
+
+	*data++ = vsp1_hgt_read(hgt, VI6_HGT_MAXMIN);
+	*data++ = vsp1_hgt_read(hgt, VI6_HGT_SUM);
+
+	for (m = 0; m < 6; ++m)
+		for (n = 0; n < 32; ++n)
+			*data++ = vsp1_hgt_read(hgt, VI6_HGT_HISTO(m, n));
+
+	vsp1_histogram_buffer_complete(&hgt->histo, buf, HGT_DATA_SIZE);
+}
+
+/* -----------------------------------------------------------------------------
+ * Controls
+ */
+
+#define V4L2_CID_VSP1_HGT_HUE_AREAS	(V4L2_CID_USER_BASE | 0x1001)
+
+static int hgt_hue_areas_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct vsp1_hgt *hgt = container_of(ctrl->handler, struct vsp1_hgt,
+					    ctrls);
+	u8 *value = ctrl->p_new.p_u8;
+	unsigned int i;
+	bool ok = true;
+
+	/*
+	 * Make sure values meet one of two possible hardware constrains
+	 * 0L <= 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U
+	 * 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U <= 0L
+	 */
+
+	if ((value[0] > value[1]) && (value[11] > value[0]))
+		ok = false;
+	for (i = 1; i < (HGT_NUM_HUE_AREAS * 2) - 1; ++i)
+		if (value[i] > value[i+1])
+			ok = false;
+
+	/* Values do not match hardware, adjust to valid settings. */
+	if (!ok) {
+		for (i = 0; i < (HGT_NUM_HUE_AREAS * 2) - 1; ++i) {
+			if (value[i] > value[i+1])
+				value[i] = value[i+1];
+		}
+	}
+
+	memcpy(hgt->hue_areas, ctrl->p_new.p_u8, sizeof(hgt->hue_areas));
+
+	return 0;
+}
+
+static const struct v4l2_ctrl_ops hgt_hue_areas_ctrl_ops = {
+	.s_ctrl = hgt_hue_areas_s_ctrl,
+};
+
+static const struct v4l2_ctrl_config hgt_hue_areas = {
+	.ops = &hgt_hue_areas_ctrl_ops,
+	.id = V4L2_CID_VSP1_HGT_HUE_AREAS,
+	.name = "Boundary Values for Hue Area",
+	.type = V4L2_CTRL_TYPE_U8,
+	.min = 0,
+	.max = 255,
+	.def = 0,
+	.step = 1,
+	.dims = { 12 },
+};
+
+/* -----------------------------------------------------------------------------
+ * VSP1 Entity Operations
+ */
+
+static void hgt_configure(struct vsp1_entity *entity,
+			  struct vsp1_pipeline *pipe,
+			  struct vsp1_dl_list *dl, bool full)
+{
+	struct vsp1_hgt *hgt = to_hgt(&entity->subdev);
+	struct v4l2_rect *compose;
+	struct v4l2_rect *crop;
+	unsigned int hratio;
+	unsigned int vratio;
+	u8 lower;
+	u8 upper;
+	unsigned int i;
+
+	if (!full)
+		return;
+
+	crop = vsp1_entity_get_pad_selection(entity, entity->config,
+					     HISTO_PAD_SINK, V4L2_SEL_TGT_CROP);
+	compose = vsp1_entity_get_pad_selection(entity, entity->config,
+						HISTO_PAD_SINK,
+						V4L2_SEL_TGT_COMPOSE);
+
+	vsp1_hgt_write(hgt, dl, VI6_HGT_REGRST, VI6_HGT_REGRST_RCLEA);
+
+	vsp1_hgt_write(hgt, dl, VI6_HGT_OFFSET,
+		       (crop->left << VI6_HGT_OFFSET_HOFFSET_SHIFT) |
+		       (crop->top << VI6_HGT_OFFSET_VOFFSET_SHIFT));
+	vsp1_hgt_write(hgt, dl, VI6_HGT_SIZE,
+		       (crop->width << VI6_HGT_SIZE_HSIZE_SHIFT) |
+		       (crop->height << VI6_HGT_SIZE_VSIZE_SHIFT));
+
+	mutex_lock(hgt->ctrls.lock);
+	for (i = 0; i < HGT_NUM_HUE_AREAS; ++i) {
+		lower = hgt->hue_areas[i*2 + 0];
+		upper = hgt->hue_areas[i*2 + 1];
+		vsp1_hgt_write(hgt, dl, VI6_HGT_HUE_AREA(i),
+			       (lower << VI6_HGT_HUE_AREA_LOWER_SHIFT) |
+			       (upper << VI6_HGT_HUE_AREA_UPPER_SHIFT));
+	}
+	mutex_unlock(hgt->ctrls.lock);
+
+	hratio = crop->width * 2 / compose->width / 3;
+	vratio = crop->height * 2 / compose->height / 3;
+	vsp1_hgt_write(hgt, dl, VI6_HGT_MODE,
+		       (hratio << VI6_HGT_MODE_HRATIO_SHIFT) |
+		       (vratio << VI6_HGT_MODE_VRATIO_SHIFT));
+}
+
+static const struct vsp1_entity_operations hgt_entity_ops = {
+	.configure = hgt_configure,
+	.destroy = vsp1_histogram_destroy,
+};
+
+/* -----------------------------------------------------------------------------
+ * Initialization and Cleanup
+ */
+
+static const unsigned int hgt_mbus_formats[] = {
+	MEDIA_BUS_FMT_AHSV8888_1X32,
+};
+
+struct vsp1_hgt *vsp1_hgt_create(struct vsp1_device *vsp1)
+{
+	struct vsp1_hgt *hgt;
+	int ret;
+
+	hgt = devm_kzalloc(vsp1->dev, sizeof(*hgt), GFP_KERNEL);
+	if (hgt == NULL)
+		return ERR_PTR(-ENOMEM);
+
+	/* Initialize the control handler. */
+	v4l2_ctrl_handler_init(&hgt->ctrls, 1);
+	v4l2_ctrl_new_custom(&hgt->ctrls, &hgt_hue_areas, NULL);
+
+	hgt->histo.entity.subdev.ctrl_handler = &hgt->ctrls;
+
+	/* Initialize the video device and queue for statistics data. */
+	ret = vsp1_histogram_init(vsp1, &hgt->histo, VSP1_ENTITY_HGT, "hgt",
+				  &hgt_entity_ops, hgt_mbus_formats,
+				  ARRAY_SIZE(hgt_mbus_formats),
+				  HGT_DATA_SIZE, V4L2_META_FMT_VSP1_HGT);
+	if (ret < 0) {
+		vsp1_entity_destroy(&hgt->histo.entity);
+		return ERR_PTR(ret);
+	}
+
+	v4l2_ctrl_handler_setup(&hgt->ctrls);
+
+	return hgt;
+}
diff --git a/drivers/media/platform/vsp1/vsp1_hgt.h b/drivers/media/platform/vsp1/vsp1_hgt.h
new file mode 100644
index 0000000..83f2e13
--- /dev/null
+++ b/drivers/media/platform/vsp1/vsp1_hgt.h
@@ -0,0 +1,42 @@
+/*
+ * vsp1_hgt.h  --  R-Car VSP1 Histogram Generator 2D
+ *
+ * Copyright (C) 2016 Renesas Electronics Corporation
+ *
+ * Contact: Niklas Söderlund (niklas.soderlund@ragnatech.se)
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#ifndef __VSP1_HGT_H__
+#define __VSP1_HGT_H__
+
+#include <media/media-entity.h>
+#include <media/v4l2-ctrls.h>
+#include <media/v4l2-subdev.h>
+
+#include "vsp1_histo.h"
+
+struct vsp1_device;
+
+#define HGT_NUM_HUE_AREAS			6
+
+struct vsp1_hgt {
+	struct vsp1_histogram histo;
+
+	struct v4l2_ctrl_handler ctrls;
+
+	u8 hue_areas[HGT_NUM_HUE_AREAS * 2];
+};
+
+static inline struct vsp1_hgt *to_hgt(struct v4l2_subdev *subdev)
+{
+	return container_of(subdev, struct vsp1_hgt, histo.entity.subdev);
+}
+
+struct vsp1_hgt *vsp1_hgt_create(struct vsp1_device *vsp1);
+void vsp1_hgt_frame_end(struct vsp1_entity *hgt);
+
+#endif /* __VSP1_HGT_H__ */
diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c b/drivers/media/platform/vsp1/vsp1_pipe.c
index 0dd7c16..052a603 100644
--- a/drivers/media/platform/vsp1/vsp1_pipe.c
+++ b/drivers/media/platform/vsp1/vsp1_pipe.c
@@ -24,6 +24,7 @@
 #include "vsp1_dl.h"
 #include "vsp1_entity.h"
 #include "vsp1_hgo.h"
+#include "vsp1_hgt.h"
 #include "vsp1_pipe.h"
 #include "vsp1_rwpf.h"
 #include "vsp1_uds.h"
@@ -191,12 +192,19 @@ void vsp1_pipeline_reset(struct vsp1_pipeline *pipe)
 		hgo->histo.pipe = NULL;
 	}
 
+	if (pipe->hgt) {
+		struct vsp1_hgt *hgt = to_hgt(&pipe->hgt->subdev);
+
+		hgt->histo.pipe = NULL;
+	}
+
 	INIT_LIST_HEAD(&pipe->entities);
 	pipe->state = VSP1_PIPELINE_STOPPED;
 	pipe->buffers_ready = 0;
 	pipe->num_inputs = 0;
 	pipe->bru = NULL;
 	pipe->hgo = NULL;
+	pipe->hgt = NULL;
 	pipe->lif = NULL;
 	pipe->uds = NULL;
 }
@@ -278,6 +286,11 @@ int vsp1_pipeline_stop(struct vsp1_pipeline *pipe)
 			   (7 << VI6_DPR_SMPPT_TGW_SHIFT) |
 			   (VI6_DPR_NODE_UNUSED << VI6_DPR_SMPPT_PT_SHIFT));
 
+	if (pipe->hgt)
+		vsp1_write(vsp1, VI6_DPR_HGT_SMPPT,
+			   (7 << VI6_DPR_SMPPT_TGW_SHIFT) |
+			   (VI6_DPR_NODE_UNUSED << VI6_DPR_SMPPT_PT_SHIFT));
+
 	v4l2_subdev_call(&pipe->output->entity.subdev, video, s_stream, 0);
 
 	return ret;
@@ -304,6 +317,9 @@ void vsp1_pipeline_frame_end(struct vsp1_pipeline *pipe)
 	if (pipe->hgo)
 		vsp1_hgo_frame_end(pipe->hgo);
 
+	if (pipe->hgt)
+		vsp1_hgt_frame_end(pipe->hgt);
+
 	if (pipe->frame_end)
 		pipe->frame_end(pipe);
 
diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h b/drivers/media/platform/vsp1/vsp1_pipe.h
index bd42eff..740bde2 100644
--- a/drivers/media/platform/vsp1/vsp1_pipe.h
+++ b/drivers/media/platform/vsp1/vsp1_pipe.h
@@ -73,6 +73,7 @@ enum vsp1_pipeline_state {
  * @output: WPF at the output of the pipeline
  * @bru: BRU entity, if present
  * @hgo: HGO entity, if present
+ * @hgt: HGT entity, if present
  * @lif: LIF entity, if present
  * @uds: UDS entity, if present
  * @uds_input: entity at the input of the UDS, if the UDS is present
@@ -99,6 +100,7 @@ struct vsp1_pipeline {
 	struct vsp1_rwpf *output;
 	struct vsp1_entity *bru;
 	struct vsp1_entity *hgo;
+	struct vsp1_entity *hgt;
 	struct vsp1_entity *lif;
 	struct vsp1_entity *uds;
 	struct vsp1_entity *uds_input;
diff --git a/drivers/media/platform/vsp1/vsp1_regs.h b/drivers/media/platform/vsp1/vsp1_regs.h
index d821348..d1d9af9 100644
--- a/drivers/media/platform/vsp1/vsp1_regs.h
+++ b/drivers/media/platform/vsp1/vsp1_regs.h
@@ -628,9 +628,17 @@
  */
 
 #define VI6_HGT_OFFSET			0x3400
+#define VI6_HGT_OFFSET_HOFFSET_SHIFT	16
+#define VI6_HGT_OFFSET_VOFFSET_SHIFT	0
 #define VI6_HGT_SIZE			0x3404
+#define VI6_HGT_SIZE_HSIZE_SHIFT	16
+#define VI6_HGT_SIZE_VSIZE_SHIFT	0
 #define VI6_HGT_MODE			0x3408
+#define VI6_HGT_MODE_HRATIO_SHIFT	2
+#define VI6_HGT_MODE_VRATIO_SHIFT	0
 #define VI6_HGT_HUE_AREA(n)		(0x340c + (n) * 4)
+#define VI6_HGT_HUE_AREA_LOWER_SHIFT	16
+#define VI6_HGT_HUE_AREA_UPPER_SHIFT	0
 #define VI6_HGT_LB_TH			0x3424
 #define VI6_HGT_LBn_H(n)		(0x3438 + (n) * 8)
 #define VI6_HGT_LBn_V(n)		(0x342c + (n) * 8)
@@ -639,6 +647,7 @@
 #define VI6_HGT_SUM			0x3754
 #define VI6_HGT_LB_DET			0x3758
 #define VI6_HGT_REGRST			0x37fc
+#define VI6_HGT_REGRST_RCLEA		(1 << 0)
 
 /* -----------------------------------------------------------------------------
  * LIF Control Registers
diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index 75e6e6c..7215e08 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -32,6 +32,7 @@
 #include "vsp1_dl.h"
 #include "vsp1_entity.h"
 #include "vsp1_hgo.h"
+#include "vsp1_hgt.h"
 #include "vsp1_pipe.h"
 #include "vsp1_rwpf.h"
 #include "vsp1_uds.h"
@@ -326,7 +327,7 @@ static int vsp1_video_pipeline_build_branch(struct vsp1_pipeline *pipe,
 	if (ret < 0)
 		return ret;
 
-	/* The main data path doesn't include the HGO, use
+	/* The main data path doesn't include the HGO or HGT, use
 	 * vsp1_entity_remote_pad() to traverse the graph.
 	 */
 
@@ -382,7 +383,7 @@ static int vsp1_video_pipeline_build_branch(struct vsp1_pipeline *pipe,
 					: &input->entity;
 		}
 
-		/* Follow the source link, ignoring any HGO. */
+		/* Follow the source link, ignoring any HGO or HGT. */
 		pad = &entity->pads[entity->source_pad];
 		pad = vsp1_entity_remote_pad(pad);
 	}
@@ -444,6 +445,11 @@ static int vsp1_video_pipeline_build(struct vsp1_pipeline *pipe,
 
 			pipe->hgo = e;
 			hgo->histo.pipe = pipe;
+		} else if (e->type == VSP1_ENTITY_HGT) {
+			struct vsp1_hgt *hgt = to_hgt(subdev);
+
+			pipe->hgt = e;
+			hgt->histo.pipe = pipe;
 		}
 	}
 
-- 
2.9.3

Re: [PATCHv2 1/2] v4l: Define a pixel format for the R-Car VSP1 2-D histogram engine

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 06 Sep 2016 16:38:55 Niklas Söderlund wrote:
> The format is used on the R-Car VSP1 video queues that carry
> 2-D histogram statistics data.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  Documentation/media/uapi/v4l/meta-formats.rst      |   1 +
>  .../media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst        | 122 ++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-ioctl.c               |   1 +
>  include/uapi/linux/videodev2.h                     |   3 +-
>  4 files changed, 126 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst
> 
> diff --git a/Documentation/media/uapi/v4l/meta-formats.rst
> b/Documentation/media/uapi/v4l/meta-formats.rst index 05ab91e..01e24e3
> 100644
> --- a/Documentation/media/uapi/v4l/meta-formats.rst
> +++ b/Documentation/media/uapi/v4l/meta-formats.rst
> @@ -13,3 +13,4 @@ These formats are used for the :ref:`metadata` interface
> only.
>      :maxdepth: 1
> 
>      pixfmt-meta-vsp1-hgo
> +    pixfmt-meta-vsp1-hgt
> diff --git a/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst
> b/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst new file mode
> 100644
> index 0000000..0393148
> --- /dev/null
> +++ b/Documentation/media/uapi/v4l/pixfmt-meta-vsp1-hgt.rst
> @@ -0,0 +1,122 @@
> +.. -*- coding: utf-8; mode: rst -*-
> +
> +.. _v4l2-meta-fmt-vsp1-hgt:
> +
> +*******************************
> +V4L2_META_FMT_VSP1_HGT ('VSPT')
> +*******************************
> +
> +*man V4L2_META_FMT_VSP1_HGT(2)*
> +
> +Renesas R-Car VSP1 2-D Histogram Data
> +
> +
> +Description
> +===========
> +
> +This format describes histogram data generated by the Renesas R-Car VSP1
> +2-D Histogram (HGT) engine.
> +
> +The VSP1 HGT is a histogram computation engine that operates on HSV
> +data. It operates on a possibly cropped and subsampled input image and
> +computes the sum, maximum and minimum of the S component as well as a
> +weighted frequency histogram based on the H and S components.
> +
> +The histogram is a matrix of 6 Hue and 32 Saturation buckets, 192 in
> +total. Each HSV value is added to one or more buckets with a weight
> +between 1 and 16 depending on the Hue areas configuration. Finding the
> +corresponding buckets is done by inspecting the H and S value
> independently. +
> +The Saturation position **n** (0 - 31) of the bucket in the matrix is
> +found by the expression:
> +
> +    n = S / 8
> +
> +The Hue position **m** (0 - 5) of the bucket in the matrix depends on
> +how the HGT Hue areas are configured. There are 6 user configurable Hue
> +Areas which can be configured to cover overlapping Hue values:
> +
> +::
> +
> +         Area 0       Area 1       Area 2       Area 3       Area 4      
> Area 5 +        ________     ________     ________     ________    
> ________     ________ +   \   /|      |\   /|      |\   /|      |\   /|    
>  |\   /|      |\   /|      |\   / +    \ / |      | \ / |      | \ / |     
> | \ / |      | \ / |      | \ / |      | \ / +     X  |      |  X  |      |
>  X  |      |  X  |      |  X  |      |  X  |      |  X +    / \ |      | /
> \ |      | / \ |      | / \ |      | / \ |      | / \ |      | / \ +   /  
> \|      |/   \|      |/   \|      |/   \|      |/   \|      |/   \|      |/
>   \ +  5U   0L      0U   1L      1U   2L      2U   3L      3U   4L      4U 
>  5L      5U   0L +        <0..............................Hue
> Value............................255> +
> +When two consecutive areas don't overlap (n+1L is equal to nU) the boundary
> +value is considered as part of the lower area.
> +
> +Pixels with a hue value included in the centre of an area (between nL and
> nU +included) are are attributed to that single area and given a weight of

s/are are/are/

> 16. +Pixels with a hue value included in the overlapping region between two
> areas +(between n+1L and nU excluded) are attributed to both areas and
> given a weight +for each of these areas proportional to their position
> along the diagonal +lines (rounded down)."

s/"//

Apart from that,

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

and applied to my tree with the above fixes.

> +
> +The Hue area setup must match one of the following constrains:
> +
> +::
> +
> +    0L <= 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U
> +
> +::
> +
> +    0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U <= 0L
> +
> +**Byte Order.**
> +All data is stored in memory in little endian format. Each cell in the
> tables +contains one byte.
> +
> +.. flat-table:: VSP1 HGT Data - (776 bytes)
> +    :header-rows:  2
> +    :stub-columns: 0
> +
> +    * - Offset
> +      - :cspan:`4` Memory
> +    * -
> +      - [31:24]
> +      - [23:16]
> +      - [15:8]
> +      - [7:0]
> +    * - 0
> +      - -
> +      - S max [7:0]
> +      - -
> +      - S min [7:0]
> +    * - 4
> +      - :cspan:`4` S sum [31:0]
> +    * - 8
> +      - :cspan:`4` Histogram bucket (m=0, n=0) [31:0]
> +    * - 12
> +      - :cspan:`4` Histogram bucket (m=0, n=1) [31:0]
> +    * -
> +      - :cspan:`4` ...
> +    * - 132
> +      - :cspan:`4` Histogram bucket (m=0, n=31) [31:0]
> +    * - 136
> +      - :cspan:`4` Histogram bucket (m=1, n=0) [31:0]
> +    * -
> +      - :cspan:`4` ...
> +    * - 264
> +      - :cspan:`4` Histogram bucket (m=2, n=0) [31:0]
> +    * -
> +      - :cspan:`4` ...
> +    * - 392
> +      - :cspan:`4` Histogram bucket (m=3, n=0) [31:0]
> +    * -
> +      - :cspan:`4` ...
> +    * - 520
> +      - :cspan:`4` Histogram bucket (m=4, n=0) [31:0]
> +    * -
> +      - :cspan:`4` ...
> +    * - 648
> +      - :cspan:`4` Histogram bucket (m=5, n=0) [31:0]
> +    * -
> +      - :cspan:`4` ...
> +    * - 772
> +      - :cspan:`4` Histogram bucket (m=5, n=31) [31:0]
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> b/drivers/media/v4l2-core/v4l2-ioctl.c index b7f7d5f..f459c4f 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -1259,6 +1259,7 @@ static void v4l_fill_fmtdesc(struct v4l2_fmtdesc *fmt)
> case V4L2_SDR_FMT_CS14LE:	descr = "Complex S14LE"; break;
>  	case V4L2_SDR_FMT_RU12LE:	descr = "Real U12LE"; break;
>  	case V4L2_META_FMT_VSP1_HGO:	descr = "R-Car VSP1 1-D Histogram"; 
break;
> +	case V4L2_META_FMT_VSP1_HGT:	descr = "R-Car VSP1 2-D Histogram"; 
break;
> 
>  	default:
>  		/* Compressed formats */
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 1dbe52a..c8c046c 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -638,7 +638,8 @@ struct v4l2_pix_format {
>  #define V4L2_SDR_FMT_RU12LE       v4l2_fourcc('R', 'U', '1', '2') /* real
> u12le */
> 
>  /* Meta-data formats */
> -#define V4L2_META_FMT_VSP1_HGO    v4l2_fourcc('V', 'S', 'P', 'H') /* R-Car
> VSP1 Histogram */ +#define V4L2_META_FMT_VSP1_HGO    v4l2_fourcc('V', 'S',
> 'P', 'H') /* R-Car VSP1 1-D Histogram */ +#define V4L2_META_FMT_VSP1_HGT   
> v4l2_fourcc('V', 'S', 'P', 'T') /* R-Car VSP1 2-D Histogram */
> 
>  /* priv field value to indicates that subsequent fields are valid. */
>  #define V4L2_PIX_FMT_PRIV_MAGIC		0xfeedcafe

-- 
Regards,

Laurent Pinchart

Re: [PATCHv2 2/2] v4l: vsp1: Add HGT support

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Tuesday 06 Sep 2016 16:38:56 Niklas Söderlund wrote:
> The HGT is a Histogram Generator Two-Dimensions. It computes a weighted
> frequency histograms for hue and saturation areas over a configurable
> region of the image with optional subsampling.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/vsp1/Makefile      |   2 +-
>  drivers/media/platform/vsp1/vsp1.h        |   3 +
>  drivers/media/platform/vsp1/vsp1_drv.c    |  33 ++++-
>  drivers/media/platform/vsp1/vsp1_entity.c |  33 +++--
>  drivers/media/platform/vsp1/vsp1_entity.h |   1 +
>  drivers/media/platform/vsp1/vsp1_hgt.c    | 217 +++++++++++++++++++++++++++
>  drivers/media/platform/vsp1/vsp1_hgt.h    |  42 ++++++
>  drivers/media/platform/vsp1/vsp1_pipe.c   |  16 +++
>  drivers/media/platform/vsp1/vsp1_pipe.h   |   2 +
>  drivers/media/platform/vsp1/vsp1_regs.h   |   9 ++
>  drivers/media/platform/vsp1/vsp1_video.c  |  10 +-
>  11 files changed, 352 insertions(+), 16 deletions(-)
>  create mode 100644 drivers/media/platform/vsp1/vsp1_hgt.c
>  create mode 100644 drivers/media/platform/vsp1/vsp1_hgt.h

[snip]

> diff --git a/drivers/media/platform/vsp1/vsp1_hgt.c
> b/drivers/media/platform/vsp1/vsp1_hgt.c new file mode 100644
> index 0000000..4e3f762
> --- /dev/null
> +++ b/drivers/media/platform/vsp1/vsp1_hgt.c

[snip]

> +/* ------------------------------------------------------------------------
> + * Controls
> + */
> +
> +#define V4L2_CID_VSP1_HGT_HUE_AREAS	(V4L2_CID_USER_BASE | 0x1001)
> +
> +static int hgt_hue_areas_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct vsp1_hgt *hgt = container_of(ctrl->handler, struct vsp1_hgt,
> +					    ctrls);
> +	u8 *value = ctrl->p_new.p_u8;

Nitpicking, I'd call the variable values.

> +	unsigned int i;
> +	bool ok = true;
> +
> +	/*
> +	 * Make sure values meet one of two possible hardware constrains

s/constrains/constraints./

> +	 * 0L <= 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 
5U
> +	 * 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U <= 
0L
> +	 */
> +
> +	if ((value[0] > value[1]) && (value[11] > value[0]))
> +		ok = false;
> +	for (i = 1; i < (HGT_NUM_HUE_AREAS * 2) - 1; ++i)
> +		if (value[i] > value[i+1])
> +			ok = false;
> +
> +	/* Values do not match hardware, adjust to valid settings. */
> +	if (!ok) {
> +		for (i = 0; i < (HGT_NUM_HUE_AREAS * 2) - 1; ++i) {
> +			if (value[i] > value[i+1])
> +				value[i] = value[i+1];
> +		}
> +	}

I'm afraid this won't work. Let's assume value[0] = 100, value[1] = 50, 
value[2] = 25. The loop will unroll to

	if (value[0] /* 100 */ > value[1] /* 50 */)
		value[0] = value[1] /* 50 */;
	if (value[1] /* 50 */ > value[2] /* 25 */)
		value[1] = value[2] /* 25 */;

You will end up with value[0] = 50, value[1] = 25, value[2] = 25, which 
doesn't match the hardware constraints.

How about the following, which tests and fixes the values in a single 
operation ?

static int hgt_hue_areas_s_ctrl(struct v4l2_ctrl *ctrl)
{
	struct vsp1_hgt *hgt = container_of(ctrl->handler, struct vsp1_hgt,
					    ctrls);
	u8 *values = ctrl->p_new.p_u8;
	unsigned int i;
        
	/*
	 * Adjust the values if they don't meet the hardware constraints:
	 *
	 * 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U
	 */ 
	for (i = 1; i < (HGT_NUM_HUE_AREAS * 2) - 1; ++i) {
		if (values[i] > values[i+1])
			values[i+1] = values[i];
	}
        
	/* 0L <= 0U or 5U <= 0L */
	if (values[0] > values[1] && values[11] > values[0])
		values[0] = values[1];
        
	memcpy(hgt->hue_areas, ctrl->p_new.p_u8, sizeof(hgt->hue_areas));

	return 0;
}

I'm also beginning to wonder whether it wouldn't make sense to return -EINVAL 
when the values don't match the constraints instead of trying to fix them.

> +	memcpy(hgt->hue_areas, ctrl->p_new.p_u8, sizeof(hgt->hue_areas));
> +
> +	return 0;
> +}

[snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCHv2 2/2] v4l: vsp1: Add HGT support

[Niklas Söderlund]

Hi Laurent,

Thanks for your review.

On 2016-09-06 22:59:22 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Tuesday 06 Sep 2016 16:38:56 Niklas Söderlund wrote:
> > The HGT is a Histogram Generator Two-Dimensions. It computes a weighted
> > frequency histograms for hue and saturation areas over a configurable
> > region of the image with optional subsampling.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/vsp1/Makefile      |   2 +-
> >  drivers/media/platform/vsp1/vsp1.h        |   3 +
> >  drivers/media/platform/vsp1/vsp1_drv.c    |  33 ++++-
> >  drivers/media/platform/vsp1/vsp1_entity.c |  33 +++--
> >  drivers/media/platform/vsp1/vsp1_entity.h |   1 +
> >  drivers/media/platform/vsp1/vsp1_hgt.c    | 217 +++++++++++++++++++++++++++
> >  drivers/media/platform/vsp1/vsp1_hgt.h    |  42 ++++++
> >  drivers/media/platform/vsp1/vsp1_pipe.c   |  16 +++
> >  drivers/media/platform/vsp1/vsp1_pipe.h   |   2 +
> >  drivers/media/platform/vsp1/vsp1_regs.h   |   9 ++
> >  drivers/media/platform/vsp1/vsp1_video.c  |  10 +-
> >  11 files changed, 352 insertions(+), 16 deletions(-)
> >  create mode 100644 drivers/media/platform/vsp1/vsp1_hgt.c
> >  create mode 100644 drivers/media/platform/vsp1/vsp1_hgt.h
> 
> [snip]
> 
> > diff --git a/drivers/media/platform/vsp1/vsp1_hgt.c
> > b/drivers/media/platform/vsp1/vsp1_hgt.c new file mode 100644
> > index 0000000..4e3f762
> > --- /dev/null
> > +++ b/drivers/media/platform/vsp1/vsp1_hgt.c
> 
> [snip]
> 
> > +/* ------------------------------------------------------------------------
> > + * Controls
> > + */
> > +
> > +#define V4L2_CID_VSP1_HGT_HUE_AREAS	(V4L2_CID_USER_BASE | 0x1001)
> > +
> > +static int hgt_hue_areas_s_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +	struct vsp1_hgt *hgt = container_of(ctrl->handler, struct vsp1_hgt,
> > +					    ctrls);
> > +	u8 *value = ctrl->p_new.p_u8;
> 
> Nitpicking, I'd call the variable values.
> 
> > +	unsigned int i;
> > +	bool ok = true;
> > +
> > +	/*
> > +	 * Make sure values meet one of two possible hardware constrains
> 
> s/constrains/constraints./
> 
> > +	 * 0L <= 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 
> 5U
> > +	 * 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U <= 
> 0L
> > +	 */
> > +
> > +	if ((value[0] > value[1]) && (value[11] > value[0]))
> > +		ok = false;
> > +	for (i = 1; i < (HGT_NUM_HUE_AREAS * 2) - 1; ++i)
> > +		if (value[i] > value[i+1])
> > +			ok = false;
> > +
> > +	/* Values do not match hardware, adjust to valid settings. */
> > +	if (!ok) {
> > +		for (i = 0; i < (HGT_NUM_HUE_AREAS * 2) - 1; ++i) {
> > +			if (value[i] > value[i+1])
> > +				value[i] = value[i+1];
> > +		}
> > +	}
> 
> I'm afraid this won't work. Let's assume value[0] = 100, value[1] = 50, 
> value[2] = 25. The loop will unroll to
> 
> 	if (value[0] /* 100 */ > value[1] /* 50 */)
> 		value[0] = value[1] /* 50 */;
> 	if (value[1] /* 50 */ > value[2] /* 25 */)
> 		value[1] = value[2] /* 25 */;
> 
> You will end up with value[0] = 50, value[1] = 25, value[2] = 25, which 
> doesn't match the hardware constraints.
> 
> How about the following, which tests and fixes the values in a single 
> operation ?
> 
> static int hgt_hue_areas_s_ctrl(struct v4l2_ctrl *ctrl)
> {
> 	struct vsp1_hgt *hgt = container_of(ctrl->handler, struct vsp1_hgt,
> 					    ctrls);
> 	u8 *values = ctrl->p_new.p_u8;
> 	unsigned int i;
>         
> 	/*
> 	 * Adjust the values if they don't meet the hardware constraints:
> 	 *
> 	 * 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U
> 	 */ 
> 	for (i = 1; i < (HGT_NUM_HUE_AREAS * 2) - 1; ++i) {
> 		if (values[i] > values[i+1])
> 			values[i+1] = values[i];
> 	}
>         
> 	/* 0L <= 0U or 5U <= 0L */
> 	if (values[0] > values[1] && values[11] > values[0])
> 		values[0] = values[1];
>         
> 	memcpy(hgt->hue_areas, ctrl->p_new.p_u8, sizeof(hgt->hue_areas));
> 
> 	return 0;
> }
> 
> I'm also beginning to wonder whether it wouldn't make sense to return -EINVAL 
> when the values don't match the constraints instead of trying to fix them.

I'm fine with either solution. I looked at a few other drivers and it 
seems the most common way is to correct the control value. But maybe in 
this case it's better to just return -EINVAL.

Let me know what you think and I will make it so and send a v3.

> 
> > +	memcpy(hgt->hue_areas, ctrl->p_new.p_u8, sizeof(hgt->hue_areas));
> > +
> > +	return 0;
> > +}
> 
> [snip]
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCHv2 2/2] v4l: vsp1: Add HGT support

[Niklas Söderlund]

Hi Laurent,

Thanks for your review.

On 2016-09-06 22:59:22 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Tuesday 06 Sep 2016 16:38:56 Niklas S�derlund wrote:
> > The HGT is a Histogram Generator Two-Dimensions. It computes a weighted
> > frequency histograms for hue and saturation areas over a configurable
> > region of the image with optional subsampling.
> > 
> > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/vsp1/Makefile      |   2 +-
> >  drivers/media/platform/vsp1/vsp1.h        |   3 +
> >  drivers/media/platform/vsp1/vsp1_drv.c    |  33 ++++-
> >  drivers/media/platform/vsp1/vsp1_entity.c |  33 +++--
> >  drivers/media/platform/vsp1/vsp1_entity.h |   1 +
> >  drivers/media/platform/vsp1/vsp1_hgt.c    | 217 +++++++++++++++++++++++++++
> >  drivers/media/platform/vsp1/vsp1_hgt.h    |  42 ++++++
> >  drivers/media/platform/vsp1/vsp1_pipe.c   |  16 +++
> >  drivers/media/platform/vsp1/vsp1_pipe.h   |   2 +
> >  drivers/media/platform/vsp1/vsp1_regs.h   |   9 ++
> >  drivers/media/platform/vsp1/vsp1_video.c  |  10 +-
> >  11 files changed, 352 insertions(+), 16 deletions(-)
> >  create mode 100644 drivers/media/platform/vsp1/vsp1_hgt.c
> >  create mode 100644 drivers/media/platform/vsp1/vsp1_hgt.h
> 
> [snip]
> 
> > diff --git a/drivers/media/platform/vsp1/vsp1_hgt.c
> > b/drivers/media/platform/vsp1/vsp1_hgt.c new file mode 100644
> > index 0000000..4e3f762
> > --- /dev/null
> > +++ b/drivers/media/platform/vsp1/vsp1_hgt.c
> 
> [snip]
> 
> > +/* ------------------------------------------------------------------------
> > + * Controls
> > + */
> > +
> > +#define V4L2_CID_VSP1_HGT_HUE_AREAS	(V4L2_CID_USER_BASE | 0x1001)
> > +
> > +static int hgt_hue_areas_s_ctrl(struct v4l2_ctrl *ctrl)
> > +{
> > +	struct vsp1_hgt *hgt = container_of(ctrl->handler, struct vsp1_hgt,
> > +					    ctrls);
> > +	u8 *value = ctrl->p_new.p_u8;
> 
> Nitpicking, I'd call the variable values.
> 
> > +	unsigned int i;
> > +	bool ok = true;
> > +
> > +	/*
> > +	 * Make sure values meet one of two possible hardware constrains
> 
> s/constrains/constraints./
> 
> > +	 * 0L <= 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 
> 5U
> > +	 * 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U <= 
> 0L
> > +	 */
> > +
> > +	if ((value[0] > value[1]) && (value[11] > value[0]))
> > +		ok = false;
> > +	for (i = 1; i < (HGT_NUM_HUE_AREAS * 2) - 1; ++i)
> > +		if (value[i] > value[i+1])
> > +			ok = false;
> > +
> > +	/* Values do not match hardware, adjust to valid settings. */
> > +	if (!ok) {
> > +		for (i = 0; i < (HGT_NUM_HUE_AREAS * 2) - 1; ++i) {
> > +			if (value[i] > value[i+1])
> > +				value[i] = value[i+1];
> > +		}
> > +	}
> 
> I'm afraid this won't work. Let's assume value[0] = 100, value[1] = 50, 
> value[2] = 25. The loop will unroll to
> 
> 	if (value[0] /* 100 */ > value[1] /* 50 */)
> 		value[0] = value[1] /* 50 */;
> 	if (value[1] /* 50 */ > value[2] /* 25 */)
> 		value[1] = value[2] /* 25 */;
> 
> You will end up with value[0] = 50, value[1] = 25, value[2] = 25, which 
> doesn't match the hardware constraints.
> 
> How about the following, which tests and fixes the values in a single 
> operation ?
> 
> static int hgt_hue_areas_s_ctrl(struct v4l2_ctrl *ctrl)
> {
> 	struct vsp1_hgt *hgt = container_of(ctrl->handler, struct vsp1_hgt,
> 					    ctrls);
> 	u8 *values = ctrl->p_new.p_u8;
> 	unsigned int i;
>         
> 	/*
> 	 * Adjust the values if they don't meet the hardware constraints:
> 	 *
> 	 * 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U
> 	 */ 
> 	for (i = 1; i < (HGT_NUM_HUE_AREAS * 2) - 1; ++i) {
> 		if (values[i] > values[i+1])
> 			values[i+1] = values[i];
> 	}
>         
> 	/* 0L <= 0U or 5U <= 0L */
> 	if (values[0] > values[1] && values[11] > values[0])
> 		values[0] = values[1];
>         
> 	memcpy(hgt->hue_areas, ctrl->p_new.p_u8, sizeof(hgt->hue_areas));
> 
> 	return 0;
> }
> 
> I'm also beginning to wonder whether it wouldn't make sense to return -EINVAL 
> when the values don't match the constraints instead of trying to fix them.

I'm fine with either solution. I looked at a few other drivers and it 
seems the most common way is to correct the control value. But maybe in 
this case it's better to just return -EINVAL.

Let me know what you think and I will make it so and send a v3.

> 
> > +	memcpy(hgt->hue_areas, ctrl->p_new.p_u8, sizeof(hgt->hue_areas));
> > +
> > +	return 0;
> > +}
> 
> [snip]
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCHv2 2/2] v4l: vsp1: Add HGT support

[Laurent Pinchart]

Hi Niklas,

On Wednesday 07 Sep 2016 12:05:26 Niklas Söderlund wrote:
> On 2016-09-06 22:59:22 +0300, Laurent Pinchart wrote:
> > On Tuesday 06 Sep 2016 16:38:56 Niklas Söderlund wrote:
> >> The HGT is a Histogram Generator Two-Dimensions. It computes a weighted
> >> frequency histograms for hue and saturation areas over a configurable
> >> region of the image with optional subsampling.
> >> 
> >> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >> ---
> >> 
> >>  drivers/media/platform/vsp1/Makefile      |   2 +-
> >>  drivers/media/platform/vsp1/vsp1.h        |   3 +
> >>  drivers/media/platform/vsp1/vsp1_drv.c    |  33 ++++-
> >>  drivers/media/platform/vsp1/vsp1_entity.c |  33 +++--
> >>  drivers/media/platform/vsp1/vsp1_entity.h |   1 +
> >>  drivers/media/platform/vsp1/vsp1_hgt.c    | 217 +++++++++++++++++++++++
> >>  drivers/media/platform/vsp1/vsp1_hgt.h    |  42 ++++++
> >>  drivers/media/platform/vsp1/vsp1_pipe.c   |  16 +++
> >>  drivers/media/platform/vsp1/vsp1_pipe.h   |   2 +
> >>  drivers/media/platform/vsp1/vsp1_regs.h   |   9 ++
> >>  drivers/media/platform/vsp1/vsp1_video.c  |  10 +-
> >>  11 files changed, 352 insertions(+), 16 deletions(-)
> >>  create mode 100644 drivers/media/platform/vsp1/vsp1_hgt.c
> >>  create mode 100644 drivers/media/platform/vsp1/vsp1_hgt.h
> > 
> > [snip]
> > 
> >> diff --git a/drivers/media/platform/vsp1/vsp1_hgt.c
> >> b/drivers/media/platform/vsp1/vsp1_hgt.c new file mode 100644
> >> index 0000000..4e3f762
> >> --- /dev/null
> >> +++ b/drivers/media/platform/vsp1/vsp1_hgt.c
> > 
> > [snip]
> > 
> >> +/* ---------------------------------------------------------------------
> >> + * Controls
> >> + */
> >> +
> >> +#define V4L2_CID_VSP1_HGT_HUE_AREAS	(V4L2_CID_USER_BASE | 0x1001)
> >> +
> >> +static int hgt_hue_areas_s_ctrl(struct v4l2_ctrl *ctrl)
> >> +{
> >> +	struct vsp1_hgt *hgt = container_of(ctrl->handler, struct vsp1_hgt,
> >> +					    ctrls);
> >> +	u8 *value = ctrl->p_new.p_u8;
> > 
> > Nitpicking, I'd call the variable values.
> > 
> >> +	unsigned int i;
> >> +	bool ok = true;
> >> +
> >> +	/*
> >> +	 * Make sure values meet one of two possible hardware constrains
> > 
> > s/constrains/constraints./
> > 
> >> +	 * 0L <= 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <=
> >> 5U
> >> +	 * 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U <=
> >> 0L
> >> +	 */
> >> +
> >> +	if ((value[0] > value[1]) && (value[11] > value[0]))
> >> +		ok = false;
> >> +	for (i = 1; i < (HGT_NUM_HUE_AREAS * 2) - 1; ++i)
> >> +		if (value[i] > value[i+1])
> >> +			ok = false;
> >> +
> >> +	/* Values do not match hardware, adjust to valid settings. */
> >> +	if (!ok) {
> >> +		for (i = 0; i < (HGT_NUM_HUE_AREAS * 2) - 1; ++i) {
> >> +			if (value[i] > value[i+1])
> >> +				value[i] = value[i+1];
> >> +		}
> >> +	}
> > 
> > I'm afraid this won't work. Let's assume value[0] = 100, value[1] = 50,
> > value[2] = 25. The loop will unroll to
> > 
> > 	if (value[0] /* 100 */ > value[1] /* 50 */)
> > 		value[0] = value[1] /* 50 */;
> > 	
> > 	if (value[1] /* 50 */ > value[2] /* 25 */)
> > 		value[1] = value[2] /* 25 */;
> > 
> > You will end up with value[0] = 50, value[1] = 25, value[2] = 25, which
> > doesn't match the hardware constraints.
> > 
> > How about the following, which tests and fixes the values in a single
> > operation ?
> > 
> > static int hgt_hue_areas_s_ctrl(struct v4l2_ctrl *ctrl)
> > {
> > 	struct vsp1_hgt *hgt = container_of(ctrl->handler, struct vsp1_hgt,
> > 					    ctrls);
> > 	u8 *values = ctrl->p_new.p_u8;
> > 	unsigned int i;
> > 	
> > 	/*
> > 	 * Adjust the values if they don't meet the hardware constraints:
> > 	 *
> > 	 * 0U <= 1L <= 1U <= 2L <= 2U <= 3L <= 3U <= 4L <= 4U <= 5L <= 5U
> > 	 */
> > 	for (i = 1; i < (HGT_NUM_HUE_AREAS * 2) - 1; ++i) {
> > 		if (values[i] > values[i+1])
> > 			values[i+1] = values[i];
> > 	}
> > 	
> > 	/* 0L <= 0U or 5U <= 0L */
> > 	if (values[0] > values[1] && values[11] > values[0])
> > 		values[0] = values[1];
> > 	
> > 	memcpy(hgt->hue_areas, ctrl->p_new.p_u8, sizeof(hgt->hue_areas));
> > 	
> > 	return 0;
> > }
> > 
> > I'm also beginning to wonder whether it wouldn't make sense to return
> > -EINVAL when the values don't match the constraints instead of trying to
> > fix them.
>
> I'm fine with either solution. I looked at a few other drivers and it
> seems the most common way is to correct the control value. But maybe in
> this case it's better to just return -EINVAL.
> 
> Let me know what you think and I will make it so and send a v3.

Given that fixed values would result in a different histogram that would 
likely be unusable by a userspace application that doesn't expect the control 
values to be changed (and if it did, it should set correct values to start 
with), I think -EINVAL is better. As Hans pointed out on IRC, this should be 
implemented in the .try_ctrl() handler.
 
> > > +	memcpy(hgt->hue_areas, ctrl->p_new.p_u8, sizeof(hgt->hue_areas));
> > > +
> > > +	return 0;
> > > +}
> > 
> > [snip]

-- 
Regards,

Laurent Pinchart