[PATCH 0/8] v4l: vsp1: Partition phase developments

[PATCH 0/8] v4l: vsp1: Partition phase developments

[Kieran Bingham]

This series presents ongoing work with the scaler partition algorithm.

It is based upon the previous partition algorithm improvements submission [0]
This series has been pushed to a tag [1] for convenience in testing.

Patches 1-3, provide fixes and additions to the register definitions needed for
controlling the phases of the UDS.

Patches 4 and 5 rework the partition data configuration storage, opening the
path for Patch 6 to implement a new entity operation API. This new '.partition'
operation gives each entity an opportunity to adapt the partition data based on
its configuration.

A new helper function "vsp1_pipeline_propagate_partition()" is provided by the
vsp1_pipe to walk the pipeline in reverse, with each entity having the
opportunity to define it's input requirements to the predecessors.

Partition data is stored somewhat inefficiently in this series, whilst the
process is established and can be considered for improvement later.

Patch 7 begins the implementation of calculating the phase values in the UDS,
and applying them in the VI6_UDS_HPHASE register appropriately. Phase
calculations have been established from the partition algorithm pseudo code
provided by renesas, although the 'end phase' is always set as 0 in this code,
it is yet to be determined if this has an effect.

Finally Patch 8, begins to allow the UDS entity to perform extra overlap at the
partition borders to provide the filters with the required data to generate
clean transitions from one partition to the next.

[0] https://www.mail-archive.com/linux-renesas-soc@vger.kernel.org/msg08631.html
[1] https://git.kernel.org/pub/scm/linux/kernel/git/kbingham/rcar.git#vsp1/pa-phases-2017-02-10

Kieran Bingham (8):
  v4l: vsp1: Provide UDS register updates
  v4l: vsp1: Track the SRU entity in the pipeline
  v4l: vsp1: Correct image partition parameters
  v4l: vsp1: Move partition rectangles to struct
  v4l: vsp1: Operate on partition struct data directly
  v4l: vsp1: Allow entities to participate in the partition algorithm
  v4l: vsp1: Calculate UDS phase for partitions
  v4l: vsp1: Implement left edge partition algorithm overlap

 drivers/media/platform/vsp1/vsp1_entity.h |   8 +-
 drivers/media/platform/vsp1/vsp1_pipe.c   |  22 ++++-
 drivers/media/platform/vsp1/vsp1_pipe.h   |  49 +++++++-
 drivers/media/platform/vsp1/vsp1_regs.h   |  14 ++-
 drivers/media/platform/vsp1/vsp1_rpf.c    |  40 +++---
 drivers/media/platform/vsp1/vsp1_sru.c    |  29 +++++-
 drivers/media/platform/vsp1/vsp1_uds.c    | 144 ++++++++++++++++++++++-
 drivers/media/platform/vsp1/vsp1_video.c  |  82 ++++++++-----
 drivers/media/platform/vsp1/vsp1_wpf.c    |  34 +++--
 9 files changed, 364 insertions(+), 58 deletions(-)

base-commit: 0c3b6ad6a559391f367879fd4be6d2d85625bd5a
-- 
git-series 0.9.1

[PATCH 1/8] v4l: vsp1: Provide UDS register updates

[Kieran Bingham]

Provide register definitions required for UDS phase and partition
algorithm support

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_regs.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/media/platform/vsp1/vsp1_regs.h b/drivers/media/platform/vsp1/vsp1_regs.h
index 47b1dee044fb..1ad819680e2f 100644
--- a/drivers/media/platform/vsp1/vsp1_regs.h
+++ b/drivers/media/platform/vsp1/vsp1_regs.h
@@ -388,6 +388,7 @@
 #define VI6_UDS_CTRL_NE_RCR		(1 << 18)
 #define VI6_UDS_CTRL_NE_GY		(1 << 17)
 #define VI6_UDS_CTRL_NE_BCB		(1 << 16)
+#define VI6_UDS_CTRL_AMDSLH		(1 << 2)
 #define VI6_UDS_CTRL_TDIPC		(1 << 1)
 
 #define VI6_UDS_SCALE			0x2304
@@ -420,11 +421,24 @@
 #define VI6_UDS_PASS_BWIDTH_V_MASK	(0x7f << 0)
 #define VI6_UDS_PASS_BWIDTH_V_SHIFT	0
 
+#define VI6_UDS_HPHASE			0x2314
+#define VI6_UDS_HPHASE_HSTP_MASK	(0xfff << 16)
+#define VI6_UDS_HPHASE_HSTP_SHIFT	16
+#define VI6_UDS_HPHASE_HEDP_MASK	(0xfff << 0)
+#define VI6_UDS_HPHASE_HEDP_SHIFT	0
+
 #define VI6_UDS_IPC			0x2318
 #define VI6_UDS_IPC_FIELD		(1 << 27)
 #define VI6_UDS_IPC_VEDP_MASK		(0xfff << 0)
 #define VI6_UDS_IPC_VEDP_SHIFT		0
 
+#define VI6_UDS_HSZCLIP			0x231c
+#define VI6_UDS_HSZCLIP_HCEN		(1 << 28)
+#define VI6_UDS_HSZCLIP_HCL_OFST_MASK	(0x1ff << 16)
+#define VI6_UDS_HSZCLIP_HCL_OFST_SHIFT	16
+#define VI6_UDS_HSZCLIP_HCL_SIZE_MASK	(0x1fff << 0)
+#define VI6_UDS_HSZCLIP_HCL_SIZE_SHIFT	0
+
 #define VI6_UDS_CLIP_SIZE		0x2324
 #define VI6_UDS_CLIP_SIZE_HSIZE_MASK	(0x1fff << 16)
 #define VI6_UDS_CLIP_SIZE_HSIZE_SHIFT	16
-- 
git-series 0.9.1

[PATCH 2/8] v4l: vsp1: Track the SRU entity in the pipeline

[Kieran Bingham]

The UDS and other entities are already tracked directly through the
pipeline object. To follow the design pattern, and allow us to reference
the SRU convert the usage of 'sru_found'

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_pipe.h  |  2 ++
 drivers/media/platform/vsp1/vsp1_video.c | 11 ++++++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h b/drivers/media/platform/vsp1/vsp1_pipe.h
index bc419ef48d8d..5aa31143ce59 100644
--- a/drivers/media/platform/vsp1/vsp1_pipe.h
+++ b/drivers/media/platform/vsp1/vsp1_pipe.h
@@ -76,6 +76,7 @@ enum vsp1_pipeline_state {
  * @output: WPF at the output of the pipeline
  * @bru: BRU entity, if present
  * @lif: LIF entity, if present
+ * @sru: SRU entity, if present
  * @uds: UDS entity, if present
  * @uds_input: entity at the input of the UDS, if the UDS is present
  * @entities: list of entities in the pipeline
@@ -104,6 +105,7 @@ struct vsp1_pipeline {
 	struct vsp1_rwpf *output;
 	struct vsp1_entity *bru;
 	struct vsp1_entity *lif;
+	struct vsp1_entity *sru;
 	struct vsp1_entity *uds;
 	struct vsp1_entity *uds_input;
 
diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index e2f242e7f0fa..be9c860b1c04 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -473,7 +473,6 @@ static int vsp1_video_pipeline_build_branch(struct vsp1_pipeline *pipe,
 	struct vsp1_entity *entity;
 	struct media_pad *pad;
 	bool bru_found = false;
-	bool sru_found = false;
 	int ret;
 
 	ret = media_entity_enum_init(&ent_enum, &input->entity.vsp1->media_dev);
@@ -523,6 +522,12 @@ static int vsp1_video_pipeline_build_branch(struct vsp1_pipeline *pipe,
 		if (entity->type == VSP1_ENTITY_SRU) {
 			struct vsp1_sru *sru = to_sru(&entity->subdev);
 
+			/* SRU can't be chained. */
+			if (pipe->sru) {
+				ret = -EPIPE;
+				goto out;
+			}
+
 			/*
 			 * Gen3 partition algorithm restricts SRU double-scaled
 			 * resolution if it is connected after a UDS entity
@@ -530,7 +535,7 @@ static int vsp1_video_pipeline_build_branch(struct vsp1_pipeline *pipe,
 			if (vsp1->info->gen == 3 && pipe->uds)
 				sru->force_identity_mode = true;
 
-			sru_found = true;
+			pipe->sru = entity;
 		}
 
 		if (entity->type == VSP1_ENTITY_UDS) {
@@ -546,7 +551,7 @@ static int vsp1_video_pipeline_build_branch(struct vsp1_pipeline *pipe,
 			 * SRU on Gen3 will always engage the partition
 			 * algorithm
 			 */
-			if (vsp1->info->gen == 3 && sru_found) {
+			if (vsp1->info->gen == 3 && pipe->sru) {
 				ret = -EPIPE;
 				goto out;
 			}
-- 
git-series 0.9.1

[PATCH 3/8] v4l: vsp1: Correct image partition parameters

[Kieran Bingham]

The image partition algorithm operates on the image dimensions as input
into the WPF entity.

Correct this in the code, and document what defines the properties for
the algorithm in the section header

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_video.c | 12 ++++++++++--
 drivers/media/platform/vsp1/vsp1_wpf.c   |  4 ++--
 2 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index be9c860b1c04..4ade958a1c9e 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -176,6 +176,14 @@ static int __vsp1_video_try_format(struct vsp1_video *video,
 
 /* -----------------------------------------------------------------------------
  * VSP1 Partition Algorithm support
+ *
+ * VSP hardware can have restrictions on image width dependent on the hardware
+ * configuration of the pipeline. Adapting for these restrictions is implemented
+ * via the partition algorithm.
+ *
+ * The partition windows and sizes are based on the output size of the WPF
+ * before rotation, which is represented by the input parameters to the WPF
+ * entity in our pipeline.
  */
 
 /**
@@ -196,7 +204,7 @@ static struct v4l2_rect vsp1_video_partition(struct vsp1_pipeline *pipe,
 
 	format = vsp1_entity_get_pad_format(&pipe->output->entity,
 					    pipe->output->entity.config,
-					    RWPF_PAD_SOURCE);
+					    RWPF_PAD_SINK);
 
 	/* A single partition simply processes the output size in full. */
 	if (pipe->partitions <= 1) {
@@ -258,7 +266,7 @@ static void vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe)
 
 	format = vsp1_entity_get_pad_format(&pipe->output->entity,
 					    pipe->output->entity.config,
-					    RWPF_PAD_SOURCE);
+					    RWPF_PAD_SINK);
 	div_size = format->width;
 
 	/* Gen2 hardware doesn't require image partitioning. */
diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c b/drivers/media/platform/vsp1/vsp1_wpf.c
index 7c48f81cd5c1..ad67034e08e9 100644
--- a/drivers/media/platform/vsp1/vsp1_wpf.c
+++ b/drivers/media/platform/vsp1/vsp1_wpf.c
@@ -218,8 +218,8 @@ static void wpf_configure(struct vsp1_entity *entity,
 		const struct v4l2_pix_format_mplane *format = &wpf->format;
 		struct vsp1_rwpf_memory mem = wpf->mem;
 		unsigned int flip = wpf->flip.active;
-		unsigned int width = source_format->width;
-		unsigned int height = source_format->height;
+		unsigned int width = sink_format->width;
+		unsigned int height = sink_format->height;
 		unsigned int offset;
 
 		/*
-- 
git-series 0.9.1

[PATCH 4/8] v4l: vsp1: Move partition rectangles to struct

[Kieran Bingham]

As we develop the partition algorithm, we need to store more information
per partition to describe the phase and other parameters.

To keep this data together, further abstract the existing v4l2_rect
into a partition specific structure

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_pipe.h  | 12 ++++++++++--
 drivers/media/platform/vsp1/vsp1_rpf.c   |  4 ++--
 drivers/media/platform/vsp1/vsp1_uds.c   |  8 +++++---
 drivers/media/platform/vsp1/vsp1_video.c | 14 ++++++++++----
 drivers/media/platform/vsp1/vsp1_wpf.c   |  9 +++++----
 5 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h b/drivers/media/platform/vsp1/vsp1_pipe.h
index 5aa31143ce59..6494c4c75023 100644
--- a/drivers/media/platform/vsp1/vsp1_pipe.h
+++ b/drivers/media/platform/vsp1/vsp1_pipe.h
@@ -60,6 +60,14 @@ enum vsp1_pipeline_state {
 };
 
 /*
+ * struct vsp1_partition - A description of each partition slice performed by HW
+ * @dest: The position and dimension of this partition in the destination image
+ */
+struct vsp1_partition {
+	struct v4l2_rect dest;
+};
+
+/*
  * struct vsp1_pipeline - A VSP1 hardware pipeline
  * @pipe: the media pipeline
  * @irqlock: protects the pipeline state
@@ -114,8 +122,8 @@ struct vsp1_pipeline {
 	struct vsp1_dl_list *dl;
 
 	unsigned int partitions;
-	struct v4l2_rect partition;
-	struct v4l2_rect part_table[VSP1_PIPE_MAX_PARTITIONS];
+	struct vsp1_partition *partition;
+	struct vsp1_partition part_table[VSP1_PIPE_MAX_PARTITIONS];
 };
 
 void vsp1_pipeline_reset(struct vsp1_pipeline *pipe);
diff --git a/drivers/media/platform/vsp1/vsp1_rpf.c b/drivers/media/platform/vsp1/vsp1_rpf.c
index b2e34a800ffa..df380a237118 100644
--- a/drivers/media/platform/vsp1/vsp1_rpf.c
+++ b/drivers/media/platform/vsp1/vsp1_rpf.c
@@ -107,9 +107,9 @@ static void rpf_configure(struct vsp1_entity *entity,
 			output = vsp1_entity_get_pad_format(wpf, wpf->config,
 							    RWPF_PAD_SOURCE);
 
-			crop.width = pipe->partition.width * input_width
+			crop.width = pipe->partition->dest.width * input_width
 				   / output->width;
-			crop.left += pipe->partition.left * input_width
+			crop.left += pipe->partition->dest.left * input_width
 				   / output->width;
 		}
 
diff --git a/drivers/media/platform/vsp1/vsp1_uds.c b/drivers/media/platform/vsp1/vsp1_uds.c
index da8f89a31ea4..98c0836d6dcd 100644
--- a/drivers/media/platform/vsp1/vsp1_uds.c
+++ b/drivers/media/platform/vsp1/vsp1_uds.c
@@ -272,11 +272,13 @@ static void uds_configure(struct vsp1_entity *entity,
 	bool multitap;
 
 	if (params == VSP1_ENTITY_PARAMS_PARTITION) {
-		const struct v4l2_rect *clip = &pipe->partition;
+		struct vsp1_partition *partition = pipe->partition;
 
 		vsp1_uds_write(uds, dl, VI6_UDS_CLIP_SIZE,
-			       (clip->width << VI6_UDS_CLIP_SIZE_HSIZE_SHIFT) |
-			       (clip->height << VI6_UDS_CLIP_SIZE_VSIZE_SHIFT));
+			       (partition->dest.width
+					<< VI6_UDS_CLIP_SIZE_HSIZE_SHIFT) |
+			       (partition->dest.height
+					<< VI6_UDS_CLIP_SIZE_VSIZE_SHIFT));
 		return;
 	}
 
diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index 4ade958a1c9e..a978508a4993 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -271,8 +271,11 @@ static void vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe)
 
 	/* Gen2 hardware doesn't require image partitioning. */
 	if (vsp1->info->gen == 2) {
+		struct vsp1_partition *partition = &pipe->part_table[0];
+
 		pipe->partitions = 1;
-		pipe->part_table[0] = vsp1_video_partition(pipe, div_size, 0);
+		partition->dest = vsp1_video_partition(pipe, div_size, 0);
+
 		return;
 	}
 
@@ -288,8 +291,11 @@ static void vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe)
 
 	pipe->partitions = DIV_ROUND_UP(format->width, div_size);
 
-	for (i = 0; i < pipe->partitions; i++)
-		pipe->part_table[i] = vsp1_video_partition(pipe, div_size, i);
+	for (i = 0; i < pipe->partitions; i++) {
+		struct vsp1_partition *partition = &pipe->part_table[i];
+
+		partition->dest = vsp1_video_partition(pipe, div_size, i);
+	}
 }
 
 /* -----------------------------------------------------------------------------
@@ -373,7 +379,7 @@ static void vsp1_video_pipeline_run_partition(struct vsp1_pipeline *pipe,
 {
 	struct vsp1_entity *entity;
 
-	pipe->partition = pipe->part_table[partition_number];
+	pipe->partition = &pipe->part_table[partition_number];
 
 	list_for_each_entry(entity, &pipe->entities, list_pipe) {
 		if (entity->ops->configure)
diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c b/drivers/media/platform/vsp1/vsp1_wpf.c
index ad67034e08e9..bd4cd2807cc6 100644
--- a/drivers/media/platform/vsp1/vsp1_wpf.c
+++ b/drivers/media/platform/vsp1/vsp1_wpf.c
@@ -227,7 +227,7 @@ static void wpf_configure(struct vsp1_entity *entity,
 		 * multiple slices.
 		 */
 		if (pipe->partitions > 1)
-			width = pipe->partition.width;
+			width = pipe->partition->dest.width;
 
 		vsp1_wpf_write(wpf, dl, VI6_WPF_HSZCLIP, VI6_WPF_SZCLIP_EN |
 			       (0 << VI6_WPF_SZCLIP_OFST_SHIFT) |
@@ -255,10 +255,11 @@ static void wpf_configure(struct vsp1_entity *entity,
 			 * order the partitions correctly.
 			 */
 			if (flip & BIT(WPF_CTRL_HFLIP))
-				offset = format->width - pipe->partition.left
-					- pipe->partition.width;
+				offset = format->width
+					- pipe->partition->dest.left
+					- pipe->partition->dest.width;
 			else
-				offset = pipe->partition.left;
+				offset = pipe->partition->dest.left;
 
 			mem.addr[0] += offset * fmtinfo->bpp[0] / 8;
 			if (format->num_planes > 1) {
-- 
git-series 0.9.1

[PATCH 5/8] v4l: vsp1: Operate on partition struct data directly

[Kieran Bingham]

When generating the partition windows, operate directly on the partition
struct rather than copying and duplicating the processed data

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_video.c | 43 ++++++++++++-------------
 1 file changed, 21 insertions(+), 22 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index a978508a4993..5f1886bfad26 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -189,17 +189,17 @@ static int __vsp1_video_try_format(struct vsp1_video *video,
 /**
  * vsp1_video_partition - Calculate the active partition output window
  *
+ * @partition: The active partition data
  * @div_size: pre-determined maximum partition division size
  * @index: partition index
  *
- * Returns a v4l2_rect describing the partition window.
+ * Generates the output partitioning positions.
  */
-static struct v4l2_rect vsp1_video_partition(struct vsp1_pipeline *pipe,
-					     unsigned int div_size,
-					     unsigned int index)
+static void vsp1_video_partition(struct vsp1_pipeline *pipe,
+				 struct vsp1_partition *partition,
+				 unsigned int div_size, unsigned int index)
 {
 	const struct v4l2_mbus_framefmt *format;
-	struct v4l2_rect partition;
 	unsigned int modulus;
 
 	format = vsp1_entity_get_pad_format(&pipe->output->entity,
@@ -208,18 +208,19 @@ static struct v4l2_rect vsp1_video_partition(struct vsp1_pipeline *pipe,
 
 	/* A single partition simply processes the output size in full. */
 	if (pipe->partitions <= 1) {
-		partition.left = 0;
-		partition.top = 0;
-		partition.width = format->width;
-		partition.height = format->height;
-		return partition;
+		partition->dest.left = 0;
+		partition->dest.top = 0;
+		partition->dest.width = format->width;
+		partition->dest.height = format->height;
+
+		return;
 	}
 
 	/* Initialise the partition with sane starting conditions. */
-	partition.left = index * div_size;
-	partition.top = 0;
-	partition.width = div_size;
-	partition.height = format->height;
+	partition->dest.left = index * div_size;
+	partition->dest.top = 0;
+	partition->dest.width = div_size;
+	partition->dest.height = format->height;
 
 	modulus = format->width % div_size;
 
@@ -242,18 +243,16 @@ static struct v4l2_rect vsp1_video_partition(struct vsp1_pipeline *pipe,
 		if (modulus < div_size / 2) {
 			if (index == partitions - 1) {
 				/* Halve the penultimate partition. */
-				partition.width = div_size / 2;
+				partition->dest.width = div_size / 2;
 			} else if (index == partitions) {
 				/* Increase the final partition. */
-				partition.width = (div_size / 2) + modulus;
-				partition.left -= div_size / 2;
+				partition->dest.width = div_size / 2 + modulus;
+				partition->dest.left -= div_size / 2;
 			}
 		} else if (index == partitions) {
-			partition.width = modulus;
+			partition->dest.width = modulus;
 		}
 	}
-
-	return partition;
 }
 
 static void vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe)
@@ -274,7 +273,7 @@ static void vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe)
 		struct vsp1_partition *partition = &pipe->part_table[0];
 
 		pipe->partitions = 1;
-		partition->dest = vsp1_video_partition(pipe, div_size, 0);
+		vsp1_video_partition(pipe, partition, div_size, 0);
 
 		return;
 	}
@@ -294,7 +293,7 @@ static void vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe)
 	for (i = 0; i < pipe->partitions; i++) {
 		struct vsp1_partition *partition = &pipe->part_table[i];
 
-		partition->dest = vsp1_video_partition(pipe, div_size, i);
+		vsp1_video_partition(pipe, partition, div_size, i);
 	}
 }
 
-- 
git-series 0.9.1

[PATCH 6/8] v4l: vsp1: Allow entities to participate in the partition algorithm

[Kieran Bingham]

The configuration of the pipeline, and entities directly affects the
inputs required to each entity for the partition algorithm. Thus it
makes sense to involve those entities in the decision making process.

Extend the entity ops API to provide an optional '.partition' call. This
allows entities which may effect the partition window, to process based
on their configuration.

Entities implementing this operation must return their required input
parameters, which will be passed down the chain. This creates a process
whereby each entity describes what is required to satisfy the required
output to it's predecessor in the pipeline.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_entity.h |  8 ++++-
 drivers/media/platform/vsp1/vsp1_pipe.c   | 22 ++++++++++++-
 drivers/media/platform/vsp1/vsp1_pipe.h   | 35 +++++++++++++++++--
 drivers/media/platform/vsp1/vsp1_rpf.c    | 33 +++++++++---------
 drivers/media/platform/vsp1/vsp1_sru.c    | 29 +++++++++++++++-
 drivers/media/platform/vsp1/vsp1_uds.c    | 45 ++++++++++++++++++++++--
 drivers/media/platform/vsp1/vsp1_video.c  | 32 ++++++++++-------
 drivers/media/platform/vsp1/vsp1_wpf.c    | 29 +++++++++++----
 8 files changed, 195 insertions(+), 38 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_entity.h b/drivers/media/platform/vsp1/vsp1_entity.h
index 901146f807b9..08aa29dea13b 100644
--- a/drivers/media/platform/vsp1/vsp1_entity.h
+++ b/drivers/media/platform/vsp1/vsp1_entity.h
@@ -21,6 +21,7 @@
 struct vsp1_device;
 struct vsp1_dl_list;
 struct vsp1_pipeline;
+struct vsp1_partition;
 
 enum vsp1_entity_type {
 	VSP1_ENTITY_BRU,
@@ -79,12 +80,19 @@ struct vsp1_route {
  *		selection rectangles, ...)
  * @max_width:	Return the max supported width of data that the entity can
  *		process in a single operation.
+ * @partition:	Process the partition construction based on this entity's
+ *		configuration.
  */
 struct vsp1_entity_operations {
 	void (*destroy)(struct vsp1_entity *);
 	void (*configure)(struct vsp1_entity *, struct vsp1_pipeline *,
 			  struct vsp1_dl_list *, enum vsp1_entity_params);
 	unsigned int (*max_width)(struct vsp1_entity *, struct vsp1_pipeline *);
+	struct vsp1_partition_rect *(*partition)(struct vsp1_entity *,
+						 struct vsp1_pipeline *,
+						 struct vsp1_partition *,
+						 unsigned int,
+						 struct vsp1_partition_rect *);
 };
 
 struct vsp1_entity {
diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c b/drivers/media/platform/vsp1/vsp1_pipe.c
index 280ba0804699..16f2eada54d5 100644
--- a/drivers/media/platform/vsp1/vsp1_pipe.c
+++ b/drivers/media/platform/vsp1/vsp1_pipe.c
@@ -331,6 +331,28 @@ void vsp1_pipeline_propagate_alpha(struct vsp1_pipeline *pipe,
 	vsp1_uds_set_alpha(pipe->uds, dl, alpha);
 }
 
+/*
+ * Propagate the partition calculations through the pipeline
+ *
+ * Work backwards through the pipe, allowing each entity to update
+ * the partition parameters based on it's configuration, and the entity
+ * connected to it's source. Each entity must produce the partition
+ * required for the next entity in the routing.
+ */
+void vsp1_pipeline_propagate_partition(struct vsp1_pipeline *pipe,
+				       struct vsp1_partition *partition,
+				       unsigned int index,
+				       struct vsp1_partition_rect *rect)
+{
+	struct vsp1_entity *entity;
+
+	list_for_each_entry_reverse(entity, &pipe->entities, list_pipe) {
+		if (entity->ops->partition)
+			rect = entity->ops->partition(entity, pipe, partition,
+						      index, rect);
+	}
+}
+
 void vsp1_pipelines_suspend(struct vsp1_device *vsp1)
 {
 	unsigned long flags;
diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h b/drivers/media/platform/vsp1/vsp1_pipe.h
index 6494c4c75023..718ca0a5eca7 100644
--- a/drivers/media/platform/vsp1/vsp1_pipe.h
+++ b/drivers/media/platform/vsp1/vsp1_pipe.h
@@ -60,11 +60,32 @@ enum vsp1_pipeline_state {
 };
 
 /*
+ * struct vsp1_partition_rect
+ *
+ * replicates struct v4l2_rect, but with an offset to apply
+ */
+struct vsp1_partition_rect {
+	__s32   left;
+	__s32   top;
+	__u32   width;
+	__u32   height;
+	__u32   offset;
+};
+
+/*
  * struct vsp1_partition - A description of each partition slice performed by HW
- * @dest: The position and dimension of this partition in the destination image
+ * @rpf: The RPF partition window configuration
+ * @uds_sink: The UDS input partition window configuration
+ * @uds_source: The UDS output partition window configuration
+ * @sru: The SRU partition window configuration
+ * @wpf: The WPF partition window configuration
  */
 struct vsp1_partition {
-	struct v4l2_rect dest;
+	struct vsp1_partition_rect rpf;
+	struct vsp1_partition_rect uds_sink;
+	struct vsp1_partition_rect uds_source;
+	struct vsp1_partition_rect sru;
+	struct vsp1_partition_rect wpf;
 };
 
 /*
@@ -117,6 +138,11 @@ struct vsp1_pipeline {
 	struct vsp1_entity *uds;
 	struct vsp1_entity *uds_input;
 
+	/*
+	 * The order of this list should be representative of the order and
+	 * routing of the the pipeline, as it is assumed by the partition
+	 * algorithm that we can walk this list in sequence.
+	 */
 	struct list_head entities;
 
 	struct vsp1_dl_list *dl;
@@ -139,6 +165,11 @@ void vsp1_pipeline_frame_end(struct vsp1_pipeline *pipe);
 void vsp1_pipeline_propagate_alpha(struct vsp1_pipeline *pipe,
 				   struct vsp1_dl_list *dl, unsigned int alpha);
 
+void vsp1_pipeline_propagate_partition(struct vsp1_pipeline *pipe,
+				       struct vsp1_partition *partition,
+				       unsigned int index,
+				       struct vsp1_partition_rect *rect);
+
 void vsp1_pipelines_suspend(struct vsp1_device *vsp1);
 void vsp1_pipelines_resume(struct vsp1_device *vsp1);
 
diff --git a/drivers/media/platform/vsp1/vsp1_rpf.c b/drivers/media/platform/vsp1/vsp1_rpf.c
index df380a237118..94541ab4ca36 100644
--- a/drivers/media/platform/vsp1/vsp1_rpf.c
+++ b/drivers/media/platform/vsp1/vsp1_rpf.c
@@ -96,21 +96,8 @@ static void rpf_configure(struct vsp1_entity *entity,
 		 * 'width' need to be adjusted.
 		 */
 		if (pipe->partitions > 1) {
-			const struct v4l2_mbus_framefmt *output;
-			struct vsp1_entity *wpf = &pipe->output->entity;
-			unsigned int input_width = crop.width;
-
-			/*
-			 * Scale the partition window based on the configuration
-			 * of the pipeline.
-			 */
-			output = vsp1_entity_get_pad_format(wpf, wpf->config,
-							    RWPF_PAD_SOURCE);
-
-			crop.width = pipe->partition->dest.width * input_width
-				   / output->width;
-			crop.left += pipe->partition->dest.left * input_width
-				   / output->width;
+			crop.width = pipe->partition->rpf.width;
+			crop.left += pipe->partition->rpf.left;
 		}
 
 		vsp1_rpf_write(rpf, dl, VI6_RPF_SRC_BSIZE,
@@ -248,8 +235,24 @@ static void rpf_configure(struct vsp1_entity *entity,
 
 }
 
+/*
+ * Perform RPF specific calculations for the Partition Algorithm
+ */
+struct vsp1_partition_rect *rpf_partition(struct vsp1_entity *entity,
+					  struct vsp1_pipeline *pipe,
+					  struct vsp1_partition *partition,
+					  unsigned int partition_idx,
+					  struct vsp1_partition_rect *dest)
+{
+	/* Duplicate the target configuration to the RPF */
+	partition->rpf = *dest;
+
+	return &partition->rpf;
+}
+
 static const struct vsp1_entity_operations rpf_entity_ops = {
 	.configure = rpf_configure,
+	.partition = rpf_partition,
 };
 
 /* -----------------------------------------------------------------------------
diff --git a/drivers/media/platform/vsp1/vsp1_sru.c b/drivers/media/platform/vsp1/vsp1_sru.c
index 42a3ed6d9461..d474a112483f 100644
--- a/drivers/media/platform/vsp1/vsp1_sru.c
+++ b/drivers/media/platform/vsp1/vsp1_sru.c
@@ -18,6 +18,7 @@
 
 #include "vsp1.h"
 #include "vsp1_dl.h"
+#include "vsp1_pipe.h"
 #include "vsp1_sru.h"
 
 #define SRU_MIN_SIZE				4U
@@ -326,9 +327,37 @@ static unsigned int sru_max_width(struct vsp1_entity *entity,
 		return 256;
 }
 
+struct vsp1_partition_rect *sru_partition(struct vsp1_entity *entity,
+					  struct vsp1_pipeline *pipe,
+					  struct vsp1_partition *partition,
+					  unsigned int partition_idx,
+					  struct vsp1_partition_rect *dest)
+{
+	struct vsp1_sru *sru = to_sru(&entity->subdev);
+	struct v4l2_mbus_framefmt *input;
+	struct v4l2_mbus_framefmt *output;
+
+	input = vsp1_entity_get_pad_format(&sru->entity, sru->entity.config,
+					   SRU_PAD_SINK);
+	output = vsp1_entity_get_pad_format(&sru->entity, sru->entity.config,
+					    SRU_PAD_SOURCE);
+
+	/* Copy the destination target */
+	partition->sru = *dest;
+
+	/* Adapt if SRUx2 is enabled */
+	if (input->width != output->width) {
+		partition->sru.width /= 2;
+		partition->sru.left /= 2;
+	}
+
+	return &partition->sru;
+}
+
 static const struct vsp1_entity_operations sru_entity_ops = {
 	.configure = sru_configure,
 	.max_width = sru_max_width,
+	.partition = sru_partition,
 };
 
 /* -----------------------------------------------------------------------------
diff --git a/drivers/media/platform/vsp1/vsp1_uds.c b/drivers/media/platform/vsp1/vsp1_uds.c
index 98c0836d6dcd..b274cbc2428b 100644
--- a/drivers/media/platform/vsp1/vsp1_uds.c
+++ b/drivers/media/platform/vsp1/vsp1_uds.c
@@ -274,10 +274,18 @@ static void uds_configure(struct vsp1_entity *entity,
 	if (params == VSP1_ENTITY_PARAMS_PARTITION) {
 		struct vsp1_partition *partition = pipe->partition;
 
+		/* Input size clipping */
+		vsp1_uds_write(uds, dl, VI6_UDS_HSZCLIP, VI6_UDS_HSZCLIP_HCEN |
+			       (partition->uds_sink.offset
+					<< VI6_UDS_HSZCLIP_HCL_OFST_SHIFT) |
+			       (partition->uds_sink.width
+					<< VI6_UDS_HSZCLIP_HCL_SIZE_SHIFT));
+
+		/* Output size clipping */
 		vsp1_uds_write(uds, dl, VI6_UDS_CLIP_SIZE,
-			       (partition->dest.width
+			       (partition->uds_source.width
 					<< VI6_UDS_CLIP_SIZE_HSIZE_SHIFT) |
-			       (partition->dest.height
+			       (partition->uds_source.height
 					<< VI6_UDS_CLIP_SIZE_VSIZE_SHIFT));
 		return;
 	}
@@ -344,9 +352,42 @@ static unsigned int uds_max_width(struct vsp1_entity *entity,
 		return 2048;
 }
 
+/* -----------------------------------------------------------------------------
+ * Partition Algorithm Support
+ */
+
+/* Perform UDS specific calculations for the Partition Algorithm */
+struct vsp1_partition_rect *uds_partition(struct vsp1_entity *entity,
+					  struct vsp1_pipeline *pipe,
+					  struct vsp1_partition *partition,
+					  unsigned int partition_idx,
+					  struct vsp1_partition_rect *dest)
+{
+	struct vsp1_uds *uds = to_uds(&entity->subdev);
+	const struct v4l2_mbus_framefmt *output;
+	const struct v4l2_mbus_framefmt *input;
+
+	/* Initialise the partition state */
+	partition->uds_sink = *dest;
+	partition->uds_source = *dest;
+
+	input = vsp1_entity_get_pad_format(&uds->entity, uds->entity.config,
+					   UDS_PAD_SINK);
+	output = vsp1_entity_get_pad_format(&uds->entity, uds->entity.config,
+					    UDS_PAD_SOURCE);
+
+	partition->uds_sink.width = dest->width * input->width
+				  / output->width;
+	partition->uds_sink.left = dest->left * input->width
+				 / output->width;
+
+	return &partition->uds_sink;
+}
+
 static const struct vsp1_entity_operations uds_entity_ops = {
 	.configure = uds_configure,
 	.max_width = uds_max_width,
+	.partition = uds_partition,
 };
 
 /* -----------------------------------------------------------------------------
diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index 5f1886bfad26..99d1c69f9572 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -200,6 +200,7 @@ static void vsp1_video_partition(struct vsp1_pipeline *pipe,
 				 unsigned int div_size, unsigned int index)
 {
 	const struct v4l2_mbus_framefmt *format;
+	struct vsp1_partition_rect dest;
 	unsigned int modulus;
 
 	format = vsp1_entity_get_pad_format(&pipe->output->entity,
@@ -208,19 +209,24 @@ static void vsp1_video_partition(struct vsp1_pipeline *pipe,
 
 	/* A single partition simply processes the output size in full. */
 	if (pipe->partitions <= 1) {
-		partition->dest.left = 0;
-		partition->dest.top = 0;
-		partition->dest.width = format->width;
-		partition->dest.height = format->height;
+		dest.left = 0;
+		dest.top = 0;
+		dest.width = format->width;
+		dest.height = format->height;
+		dest.offset = 0;
+
+		vsp1_pipeline_propagate_partition(pipe, partition, index,
+						  &dest);
 
 		return;
 	}
 
 	/* Initialise the partition with sane starting conditions. */
-	partition->dest.left = index * div_size;
-	partition->dest.top = 0;
-	partition->dest.width = div_size;
-	partition->dest.height = format->height;
+	dest.left = index * div_size;
+	dest.top = 0;
+	dest.width = div_size;
+	dest.height = format->height;
+	dest.offset = 0;
 
 	modulus = format->width % div_size;
 
@@ -243,16 +249,18 @@ static void vsp1_video_partition(struct vsp1_pipeline *pipe,
 		if (modulus < div_size / 2) {
 			if (index == partitions - 1) {
 				/* Halve the penultimate partition. */
-				partition->dest.width = div_size / 2;
+				dest.width = div_size / 2;
 			} else if (index == partitions) {
 				/* Increase the final partition. */
-				partition->dest.width = div_size / 2 + modulus;
-				partition->dest.left -= div_size / 2;
+				dest.width = div_size / 2 + modulus;
+				dest.left -= div_size / 2;
 			}
 		} else if (index == partitions) {
-			partition->dest.width = modulus;
+			dest.width = modulus;
 		}
 	}
+
+	vsp1_pipeline_propagate_partition(pipe, partition, index, &dest);
 }
 
 static void vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe)
diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c b/drivers/media/platform/vsp1/vsp1_wpf.c
index bd4cd2807cc6..2f83efee5d78 100644
--- a/drivers/media/platform/vsp1/vsp1_wpf.c
+++ b/drivers/media/platform/vsp1/vsp1_wpf.c
@@ -220,17 +220,19 @@ static void wpf_configure(struct vsp1_entity *entity,
 		unsigned int flip = wpf->flip.active;
 		unsigned int width = sink_format->width;
 		unsigned int height = sink_format->height;
-		unsigned int offset;
+		unsigned int offset = 0;
 
 		/*
 		 * Cropping. The partition algorithm can split the image into
 		 * multiple slices.
 		 */
-		if (pipe->partitions > 1)
-			width = pipe->partition->dest.width;
+		if (pipe->partitions > 1) {
+			offset = pipe->partition->wpf.offset;
+			width = pipe->partition->wpf.width;
+		}
 
 		vsp1_wpf_write(wpf, dl, VI6_WPF_HSZCLIP, VI6_WPF_SZCLIP_EN |
-			       (0 << VI6_WPF_SZCLIP_OFST_SHIFT) |
+			       (offset << VI6_WPF_SZCLIP_OFST_SHIFT) |
 			       (width << VI6_WPF_SZCLIP_SIZE_SHIFT));
 		vsp1_wpf_write(wpf, dl, VI6_WPF_VSZCLIP, VI6_WPF_SZCLIP_EN |
 			       (0 << VI6_WPF_SZCLIP_OFST_SHIFT) |
@@ -256,10 +258,10 @@ static void wpf_configure(struct vsp1_entity *entity,
 			 */
 			if (flip & BIT(WPF_CTRL_HFLIP))
 				offset = format->width
-					- pipe->partition->dest.left
-					- pipe->partition->dest.width;
+					- pipe->partition->wpf.left
+					- pipe->partition->wpf.width;
 			else
-				offset = pipe->partition->dest.left;
+				offset = pipe->partition->wpf.left;
 
 			mem.addr[0] += offset * fmtinfo->bpp[0] / 8;
 			if (format->num_planes > 1) {
@@ -355,9 +357,22 @@ static void wpf_configure(struct vsp1_entity *entity,
 			   VI6_WFP_IRQ_ENB_DFEE);
 }
 
+struct vsp1_partition_rect *wpf_partition(struct vsp1_entity *entity,
+					  struct vsp1_pipeline *pipe,
+					  struct vsp1_partition *partition,
+					  unsigned int partition_idx,
+					  struct vsp1_partition_rect *dest)
+{
+	/* Initialise the WPF partition */
+	partition->wpf = *dest;
+
+	return &partition->wpf;
+}
+
 static const struct vsp1_entity_operations wpf_entity_ops = {
 	.destroy = vsp1_wpf_destroy,
 	.configure = wpf_configure,
+	.partition = wpf_partition,
 };
 
 /* -----------------------------------------------------------------------------
-- 
git-series 0.9.1

[PATCH 7/8] v4l: vsp1: Calculate UDS phase for partitions

[Kieran Bingham]

To improve image quality when scaling using the UDS we need to correctly
determine the start phase value for each partition window.

Provide helper functions for calculating the phase, and write this value
to the registers when used.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_pipe.h |  4 ++-
 drivers/media/platform/vsp1/vsp1_uds.c  | 64 +++++++++++++++++++++++++-
 2 files changed, 67 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h b/drivers/media/platform/vsp1/vsp1_pipe.h
index 718ca0a5eca7..0faa1c9f6184 100644
--- a/drivers/media/platform/vsp1/vsp1_pipe.h
+++ b/drivers/media/platform/vsp1/vsp1_pipe.h
@@ -79,6 +79,8 @@ struct vsp1_partition_rect {
  * @uds_source: The UDS output partition window configuration
  * @sru: The SRU partition window configuration
  * @wpf: The WPF partition window configuration
+ * @start_phase: The UDS start phase specific to this partition.
+ * @end_phase: The UDS end phase specific to this partition.
  */
 struct vsp1_partition {
 	struct vsp1_partition_rect rpf;
@@ -86,6 +88,8 @@ struct vsp1_partition {
 	struct vsp1_partition_rect uds_source;
 	struct vsp1_partition_rect sru;
 	struct vsp1_partition_rect wpf;
+	unsigned int start_phase;
+	unsigned int end_phase;
 };
 
 /*
diff --git a/drivers/media/platform/vsp1/vsp1_uds.c b/drivers/media/platform/vsp1/vsp1_uds.c
index b274cbc2428b..9c1fb7ef3c46 100644
--- a/drivers/media/platform/vsp1/vsp1_uds.c
+++ b/drivers/media/platform/vsp1/vsp1_uds.c
@@ -50,6 +50,46 @@ void vsp1_uds_set_alpha(struct vsp1_entity *entity, struct vsp1_dl_list *dl,
 		       alpha << VI6_UDS_ALPVAL_VAL0_SHIFT);
 }
 
+struct uds_phase {
+	unsigned int mp;
+	unsigned int prefilt_term;
+	unsigned int prefilt_outpos;
+	unsigned int residual;
+};
+
+/*
+ * TODO: Remove start_phase if possible:
+ * 'start_phase' as we use it should always be 0 I believe,
+ * Therefore this could be removed once confirmed
+ */
+static struct uds_phase uds_phase_calculation(int position, int start_phase,
+					      int ratio)
+{
+	struct uds_phase phase;
+	int alpha = ratio * position;
+
+	/* These must be adjusted if we ever set BLADV */
+	phase.mp = ratio / 4096;
+	phase.mp = phase.mp < 4 ? 1 : (phase.mp < 8 ? 2 : 4);
+
+	phase.prefilt_term = phase.mp * 4096;
+	phase.prefilt_outpos = (alpha - start_phase * phase.mp)
+			/ phase.prefilt_term;
+	phase.residual = (alpha - start_phase * phase.mp)
+			% phase.prefilt_term;
+
+	return phase;
+}
+
+static int uds_start_phase(int pos, int start_phase, int ratio)
+{
+	struct uds_phase phase;
+
+	phase = uds_phase_calculation(pos, start_phase, ratio);
+
+	return phase.residual ? (4096 - phase.residual / phase.mp) : 0;
+}
+
 /*
  * uds_output_size - Return the output size for an input size and scaling ratio
  * @input: input size in pixels
@@ -269,6 +309,7 @@ static void uds_configure(struct vsp1_entity *entity,
 	const struct v4l2_mbus_framefmt *input;
 	unsigned int hscale;
 	unsigned int vscale;
+	bool manual_phase = (pipe->partitions > 1);
 	bool multitap;
 
 	if (params == VSP1_ENTITY_PARAMS_PARTITION) {
@@ -287,6 +328,16 @@ static void uds_configure(struct vsp1_entity *entity,
 					<< VI6_UDS_CLIP_SIZE_HSIZE_SHIFT) |
 			       (partition->uds_source.height
 					<< VI6_UDS_CLIP_SIZE_VSIZE_SHIFT));
+
+		if (!manual_phase)
+			return;
+
+		vsp1_uds_write(uds, dl, VI6_UDS_HPHASE,
+			       (partition->start_phase
+					<< VI6_UDS_HPHASE_HSTP_SHIFT) |
+			       (partition->end_phase
+					<< VI6_UDS_HPHASE_HEDP_SHIFT));
+
 		return;
 	}
 
@@ -314,7 +365,8 @@ static void uds_configure(struct vsp1_entity *entity,
 
 	vsp1_uds_write(uds, dl, VI6_UDS_CTRL,
 		       (uds->scale_alpha ? VI6_UDS_CTRL_AON : 0) |
-		       (multitap ? VI6_UDS_CTRL_BC : 0));
+		       (multitap ? VI6_UDS_CTRL_BC : 0) |
+		       (manual_phase ? VI6_UDS_CTRL_AMDSLH : 0));
 
 	vsp1_uds_write(uds, dl, VI6_UDS_PASS_BWIDTH,
 		       (uds_passband_width(hscale)
@@ -366,6 +418,8 @@ struct vsp1_partition_rect *uds_partition(struct vsp1_entity *entity,
 	struct vsp1_uds *uds = to_uds(&entity->subdev);
 	const struct v4l2_mbus_framefmt *output;
 	const struct v4l2_mbus_framefmt *input;
+	unsigned int hscale;
+	unsigned int image_start_phase = 0;
 
 	/* Initialise the partition state */
 	partition->uds_sink = *dest;
@@ -376,11 +430,19 @@ struct vsp1_partition_rect *uds_partition(struct vsp1_entity *entity,
 	output = vsp1_entity_get_pad_format(&uds->entity, uds->entity.config,
 					    UDS_PAD_SOURCE);
 
+	hscale = uds_compute_ratio(input->width, output->width);
+
 	partition->uds_sink.width = dest->width * input->width
 				  / output->width;
 	partition->uds_sink.left = dest->left * input->width
 				 / output->width;
 
+	partition->start_phase = uds_start_phase(partition->uds_source.left,
+						 image_start_phase, hscale);
+
+	/* Renesas partition algorithm always sets end-phase as 0 */
+	partition->end_phase = 0;
+
 	return &partition->uds_sink;
 }
 
-- 
git-series 0.9.1

[PATCH 8/8] v4l: vsp1: Implement left edge partition algorithm overlap

[Kieran Bingham]

Increase the overlap on the left edge to allow a margin to provide
better image scaling

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_rpf.c |  7 +++++-
 drivers/media/platform/vsp1/vsp1_uds.c | 39 ++++++++++++++++++++++++---
 2 files changed, 42 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_rpf.c b/drivers/media/platform/vsp1/vsp1_rpf.c
index 94541ab4ca36..d08cfd944b7b 100644
--- a/drivers/media/platform/vsp1/vsp1_rpf.c
+++ b/drivers/media/platform/vsp1/vsp1_rpf.c
@@ -247,6 +247,13 @@ struct vsp1_partition_rect *rpf_partition(struct vsp1_entity *entity,
 	/* Duplicate the target configuration to the RPF */
 	partition->rpf = *dest;
 
+	/*
+	 * A partition offset, is a request for more input pixels, and a
+	 * declaration that the consumer will clip excess.
+	 */
+	partition->rpf.width += dest->offset;
+	partition->rpf.left -= dest->offset;
+
 	return &partition->rpf;
 }
 
diff --git a/drivers/media/platform/vsp1/vsp1_uds.c b/drivers/media/platform/vsp1/vsp1_uds.c
index 9c1fb7ef3c46..9ee476c8db59 100644
--- a/drivers/media/platform/vsp1/vsp1_uds.c
+++ b/drivers/media/platform/vsp1/vsp1_uds.c
@@ -81,6 +81,20 @@ static struct uds_phase uds_phase_calculation(int position, int start_phase,
 	return phase;
 }
 
+static int uds_left_src_pixel(int pos, int start_phase, int ratio)
+{
+	struct uds_phase phase;
+
+	phase = uds_phase_calculation(pos, start_phase, ratio);
+
+	/* Renesas guard against odd values in these scale ratios here ? */
+	if ((phase.mp == 2 && (phase.residual & 0x01)) ||
+	    (phase.mp == 4 && (phase.residual & 0x03)))
+		WARN_ON(1);
+
+	return phase.mp * (phase.prefilt_outpos + (phase.residual ? 1 : 0));
+}
+
 static int uds_start_phase(int pos, int start_phase, int ratio)
 {
 	struct uds_phase phase;
@@ -420,6 +434,8 @@ struct vsp1_partition_rect *uds_partition(struct vsp1_entity *entity,
 	const struct v4l2_mbus_framefmt *input;
 	unsigned int hscale;
 	unsigned int image_start_phase = 0;
+	unsigned int right_sink;
+	unsigned int margin;
 
 	/* Initialise the partition state */
 	partition->uds_sink = *dest;
@@ -432,10 +448,25 @@ struct vsp1_partition_rect *uds_partition(struct vsp1_entity *entity,
 
 	hscale = uds_compute_ratio(input->width, output->width);
 
-	partition->uds_sink.width = dest->width * input->width
-				  / output->width;
-	partition->uds_sink.left = dest->left * input->width
-				 / output->width;
+	/* Handle 'left' edge of the partitions */
+	if (partition_idx == 0) {
+		margin = 0;
+	} else {
+		margin = hscale < 0x200 ? 32 : /* 8 <  scale */
+			 hscale < 0x400 ? 16 : /* 4 <  scale <= 8 */
+			 hscale < 0x800 ?  8 : /* 2 <  scale <= 4 */
+					   4;  /* 1 <  scale <= 2, scale <= 1 */
+	}
+
+	partition->uds_sink.left = uds_left_src_pixel(dest->left,
+					image_start_phase, hscale);
+
+	partition->uds_sink.offset = margin;
+
+	right_sink = uds_left_src_pixel(dest->left + dest->width - 1,
+					image_start_phase, hscale);
+
+	partition->uds_sink.width = right_sink - partition->uds_sink.left + 1;
 
 	partition->start_phase = uds_start_phase(partition->uds_source.left,
 						 image_start_phase, hscale);
-- 
git-series 0.9.1

Re: [PATCH 1/8] v4l: vsp1: Provide UDS register updates

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Friday 10 Feb 2017 20:27:29 Kieran Bingham wrote:
> Provide register definitions required for UDS phase and partition
> algorithm support

I would mention here that those registers and bits are available on Gen3 only.

> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/vsp1/vsp1_regs.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_regs.h
> b/drivers/media/platform/vsp1/vsp1_regs.h index 47b1dee044fb..1ad819680e2f
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_regs.h
> +++ b/drivers/media/platform/vsp1/vsp1_regs.h
> @@ -388,6 +388,7 @@
>  #define VI6_UDS_CTRL_NE_RCR		(1 << 18)
>  #define VI6_UDS_CTRL_NE_GY		(1 << 17)
>  #define VI6_UDS_CTRL_NE_BCB		(1 << 16)
> +#define VI6_UDS_CTRL_AMDSLH		(1 << 2)
>  #define VI6_UDS_CTRL_TDIPC		(1 << 1)
> 
>  #define VI6_UDS_SCALE			0x2304
> @@ -420,11 +421,24 @@
>  #define VI6_UDS_PASS_BWIDTH_V_MASK	(0x7f << 0)
>  #define VI6_UDS_PASS_BWIDTH_V_SHIFT	0
> 
> +#define VI6_UDS_HPHASE			0x2314
> +#define VI6_UDS_HPHASE_HSTP_MASK	(0xfff << 16)
> +#define VI6_UDS_HPHASE_HSTP_SHIFT	16
> +#define VI6_UDS_HPHASE_HEDP_MASK	(0xfff << 0)
> +#define VI6_UDS_HPHASE_HEDP_SHIFT	0
> +
>  #define VI6_UDS_IPC			0x2318
>  #define VI6_UDS_IPC_FIELD		(1 << 27)
>  #define VI6_UDS_IPC_VEDP_MASK		(0xfff << 0)
>  #define VI6_UDS_IPC_VEDP_SHIFT		0
> 
> +#define VI6_UDS_HSZCLIP			0x231c
> +#define VI6_UDS_HSZCLIP_HCEN		(1 << 28)
> +#define VI6_UDS_HSZCLIP_HCL_OFST_MASK	(0x1ff << 16)

This field spans 8 bits, not 9.

With that fixed,

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

> +#define VI6_UDS_HSZCLIP_HCL_OFST_SHIFT	16
> +#define VI6_UDS_HSZCLIP_HCL_SIZE_MASK	(0x1fff << 0)
> +#define VI6_UDS_HSZCLIP_HCL_SIZE_SHIFT	0
> +
>  #define VI6_UDS_CLIP_SIZE		0x2324
>  #define VI6_UDS_CLIP_SIZE_HSIZE_MASK	(0x1fff << 16)
>  #define VI6_UDS_CLIP_SIZE_HSIZE_SHIFT	16

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/8] v4l: vsp1: Track the SRU entity in the pipeline

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Friday 10 Feb 2017 20:27:30 Kieran Bingham wrote:
> The UDS and other entities are already tracked directly through the
> pipeline object. To follow the design pattern, and allow us to reference
> the SRU convert the usage of 'sru_found'

I propose squashing this into "v4l: vsp1: Implement partition algorithm 
restrictions" as it hasn't been merged yet. Or, given that that patch requires 
more work, we can also reorder them to merge this series first.

> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/vsp1/vsp1_pipe.h  |  2 ++
>  drivers/media/platform/vsp1/vsp1_video.c | 11 ++++++++---
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h
> b/drivers/media/platform/vsp1/vsp1_pipe.h index bc419ef48d8d..5aa31143ce59
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_pipe.h
> +++ b/drivers/media/platform/vsp1/vsp1_pipe.h
> @@ -76,6 +76,7 @@ enum vsp1_pipeline_state {
>   * @output: WPF at the output of the pipeline
>   * @bru: BRU entity, if present
>   * @lif: LIF entity, if present
> + * @sru: SRU entity, if present
>   * @uds: UDS entity, if present
>   * @uds_input: entity at the input of the UDS, if the UDS is present
>   * @entities: list of entities in the pipeline
> @@ -104,6 +105,7 @@ struct vsp1_pipeline {
>  	struct vsp1_rwpf *output;
>  	struct vsp1_entity *bru;
>  	struct vsp1_entity *lif;
> +	struct vsp1_entity *sru;
>  	struct vsp1_entity *uds;
>  	struct vsp1_entity *uds_input;
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
> b/drivers/media/platform/vsp1/vsp1_video.c index e2f242e7f0fa..be9c860b1c04
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -473,7 +473,6 @@ static int vsp1_video_pipeline_build_branch(struct
> vsp1_pipeline *pipe, struct vsp1_entity *entity;
>  	struct media_pad *pad;
>  	bool bru_found = false;
> -	bool sru_found = false;
>  	int ret;
> 
>  	ret = media_entity_enum_init(&ent_enum, &input->entity.vsp1-
>media_dev);
> @@ -523,6 +522,12 @@ static int vsp1_video_pipeline_build_branch(struct
> vsp1_pipeline *pipe, if (entity->type == VSP1_ENTITY_SRU) {
>  			struct vsp1_sru *sru = to_sru(&entity->subdev);
> 
> +			/* SRU can't be chained. */
> +			if (pipe->sru) {
> +				ret = -EPIPE;
> +				goto out;
> +			}

You can drop this, there's at most one SRU per VSP instance.

> +
>  			/*
>  			 * Gen3 partition algorithm restricts SRU double-
scaled
>  			 * resolution if it is connected after a UDS entity
> @@ -530,7 +535,7 @@ static int vsp1_video_pipeline_build_branch(struct
> vsp1_pipeline *pipe, if (vsp1->info->gen == 3 && pipe->uds)
>  				sru->force_identity_mode = true;
> 
> -			sru_found = true;
> +			pipe->sru = entity;
>  		}
> 
>  		if (entity->type == VSP1_ENTITY_UDS) {
> @@ -546,7 +551,7 @@ static int vsp1_video_pipeline_build_branch(struct
> vsp1_pipeline *pipe, * SRU on Gen3 will always engage the partition
>  			 * algorithm
>  			 */
> -			if (vsp1->info->gen == 3 && sru_found) {
> +			if (vsp1->info->gen == 3 && pipe->sru) {
>  				ret = -EPIPE;
>  				goto out;
>  			}

You need to update vsp1_pipeline_reset() as well.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/8] v4l: vsp1: Correct image partition parameters

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Friday 10 Feb 2017 20:27:31 Kieran Bingham wrote:
> The image partition algorithm operates on the image dimensions as input
> into the WPF entity.
> 
> Correct this in the code, and document what defines the properties for
> the algorithm in the section header

Nitpicking, this sentence lacks a period.

> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

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

> ---
>  drivers/media/platform/vsp1/vsp1_video.c | 12 ++++++++++--
>  drivers/media/platform/vsp1/vsp1_wpf.c   |  4 ++--
>  2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
> b/drivers/media/platform/vsp1/vsp1_video.c index be9c860b1c04..4ade958a1c9e
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -176,6 +176,14 @@ static int __vsp1_video_try_format(struct vsp1_video
> *video,
> 
>  /*
> ---------------------------------------------------------------------------
> -- * VSP1 Partition Algorithm support
> + *
> + * VSP hardware can have restrictions on image width dependent on the
> hardware + * configuration of the pipeline. Adapting for these restrictions
> is implemented + * via the partition algorithm.
> + *
> + * The partition windows and sizes are based on the output size of the WPF
> + * before rotation, which is represented by the input parameters to the WPF
> + * entity in our pipeline.
>   */
> 
>  /**
> @@ -196,7 +204,7 @@ static struct v4l2_rect vsp1_video_partition(struct
> vsp1_pipeline *pipe,
> 
>  	format = vsp1_entity_get_pad_format(&pipe->output->entity,
>  					    pipe->output->entity.config,
> -					    RWPF_PAD_SOURCE);
> +					    RWPF_PAD_SINK);
> 
>  	/* A single partition simply processes the output size in full. */
>  	if (pipe->partitions <= 1) {
> @@ -258,7 +266,7 @@ static void vsp1_video_pipeline_setup_partitions(struct
> vsp1_pipeline *pipe)
> 
>  	format = vsp1_entity_get_pad_format(&pipe->output->entity,
>  					    pipe->output->entity.config,
> -					    RWPF_PAD_SOURCE);
> +					    RWPF_PAD_SINK);
>  	div_size = format->width;
> 
>  	/* Gen2 hardware doesn't require image partitioning. */
> diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c
> b/drivers/media/platform/vsp1/vsp1_wpf.c index 7c48f81cd5c1..ad67034e08e9
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_wpf.c
> +++ b/drivers/media/platform/vsp1/vsp1_wpf.c
> @@ -218,8 +218,8 @@ static void wpf_configure(struct vsp1_entity *entity,
>  		const struct v4l2_pix_format_mplane *format = &wpf->format;
>  		struct vsp1_rwpf_memory mem = wpf->mem;
>  		unsigned int flip = wpf->flip.active;
> -		unsigned int width = source_format->width;
> -		unsigned int height = source_format->height;
> +		unsigned int width = sink_format->width;
> +		unsigned int height = sink_format->height;
>  		unsigned int offset;
> 
>  		/*

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/8] v4l: vsp1: Move partition rectangles to struct

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Friday 10 Feb 2017 20:27:32 Kieran Bingham wrote:
> As we develop the partition algorithm, we need to store more information
> per partition to describe the phase and other parameters.
> 
> To keep this data together, further abstract the existing v4l2_rect
> into a partition specific structure
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/vsp1/vsp1_pipe.h  | 12 ++++++++++--
>  drivers/media/platform/vsp1/vsp1_rpf.c   |  4 ++--
>  drivers/media/platform/vsp1/vsp1_uds.c   |  8 +++++---
>  drivers/media/platform/vsp1/vsp1_video.c | 14 ++++++++++----
>  drivers/media/platform/vsp1/vsp1_wpf.c   |  9 +++++----
>  5 files changed, 32 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h
> b/drivers/media/platform/vsp1/vsp1_pipe.h index 5aa31143ce59..6494c4c75023
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_pipe.h
> +++ b/drivers/media/platform/vsp1/vsp1_pipe.h
> @@ -60,6 +60,14 @@ enum vsp1_pipeline_state {
>  };
> 
>  /*
> + * struct vsp1_partition - A description of each partition slice performed
> by HW
> + * @dest: The position and dimension of this partition in the destination
> image
> + */
> +struct vsp1_partition {
> +	struct v4l2_rect dest;

Given that we only partition the image horizontally, how about just storing 
the left and width values ?

Apart from that,

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

> +};
> +
> +/*
>   * struct vsp1_pipeline - A VSP1 hardware pipeline
>   * @pipe: the media pipeline
>   * @irqlock: protects the pipeline state
> @@ -114,8 +122,8 @@ struct vsp1_pipeline {
>  	struct vsp1_dl_list *dl;
> 
>  	unsigned int partitions;
> -	struct v4l2_rect partition;
> -	struct v4l2_rect part_table[VSP1_PIPE_MAX_PARTITIONS];
> +	struct vsp1_partition *partition;
> +	struct vsp1_partition part_table[VSP1_PIPE_MAX_PARTITIONS];
>  };
> 
>  void vsp1_pipeline_reset(struct vsp1_pipeline *pipe);
> diff --git a/drivers/media/platform/vsp1/vsp1_rpf.c
> b/drivers/media/platform/vsp1/vsp1_rpf.c index b2e34a800ffa..df380a237118
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_rpf.c
> +++ b/drivers/media/platform/vsp1/vsp1_rpf.c
> @@ -107,9 +107,9 @@ static void rpf_configure(struct vsp1_entity *entity,
>  			output = vsp1_entity_get_pad_format(wpf, wpf->config,
>  							    RWPF_PAD_SOURCE);
> 
> -			crop.width = pipe->partition.width * input_width
> +			crop.width = pipe->partition->dest.width * input_width
>  				   / output->width;
> -			crop.left += pipe->partition.left * input_width
> +			crop.left += pipe->partition->dest.left * input_width
>  				   / output->width;
>  		}
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_uds.c
> b/drivers/media/platform/vsp1/vsp1_uds.c index da8f89a31ea4..98c0836d6dcd
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_uds.c
> +++ b/drivers/media/platform/vsp1/vsp1_uds.c
> @@ -272,11 +272,13 @@ static void uds_configure(struct vsp1_entity *entity,
>  	bool multitap;
> 
>  	if (params == VSP1_ENTITY_PARAMS_PARTITION) {
> -		const struct v4l2_rect *clip = &pipe->partition;
> +		struct vsp1_partition *partition = pipe->partition;
> 
>  		vsp1_uds_write(uds, dl, VI6_UDS_CLIP_SIZE,
> -			       (clip->width << VI6_UDS_CLIP_SIZE_HSIZE_SHIFT) 
|
> -			       (clip->height << 
VI6_UDS_CLIP_SIZE_VSIZE_SHIFT));
> +			       (partition->dest.width
> +					<< VI6_UDS_CLIP_SIZE_HSIZE_SHIFT) |
> +			       (partition->dest.height
> +					<< VI6_UDS_CLIP_SIZE_VSIZE_SHIFT));
>  		return;
>  	}
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
> b/drivers/media/platform/vsp1/vsp1_video.c index 4ade958a1c9e..a978508a4993
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -271,8 +271,11 @@ static void vsp1_video_pipeline_setup_partitions(struct
> vsp1_pipeline *pipe)
> 
>  	/* Gen2 hardware doesn't require image partitioning. */
>  	if (vsp1->info->gen == 2) {
> +		struct vsp1_partition *partition = &pipe->part_table[0];
> +
>  		pipe->partitions = 1;
> -		pipe->part_table[0] = vsp1_video_partition(pipe, div_size, 0);
> +		partition->dest = vsp1_video_partition(pipe, div_size, 0);
> +
>  		return;
>  	}
> 
> @@ -288,8 +291,11 @@ static void vsp1_video_pipeline_setup_partitions(struct
> vsp1_pipeline *pipe)
> 
>  	pipe->partitions = DIV_ROUND_UP(format->width, div_size);
> 
> -	for (i = 0; i < pipe->partitions; i++)
> -		pipe->part_table[i] = vsp1_video_partition(pipe, div_size, i);
> +	for (i = 0; i < pipe->partitions; i++) {
> +		struct vsp1_partition *partition = &pipe->part_table[i];
> +
> +		partition->dest = vsp1_video_partition(pipe, div_size, i);
> +	}
>  }
> 
>  /*
> ---------------------------------------------------------------------------
> -- @@ -373,7 +379,7 @@ static void vsp1_video_pipeline_run_partition(struct
> vsp1_pipeline *pipe, {
>  	struct vsp1_entity *entity;
> 
> -	pipe->partition = pipe->part_table[partition_number];
> +	pipe->partition = &pipe->part_table[partition_number];
> 
>  	list_for_each_entry(entity, &pipe->entities, list_pipe) {
>  		if (entity->ops->configure)
> diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c
> b/drivers/media/platform/vsp1/vsp1_wpf.c index ad67034e08e9..bd4cd2807cc6
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_wpf.c
> +++ b/drivers/media/platform/vsp1/vsp1_wpf.c
> @@ -227,7 +227,7 @@ static void wpf_configure(struct vsp1_entity *entity,
>  		 * multiple slices.
>  		 */
>  		if (pipe->partitions > 1)
> -			width = pipe->partition.width;
> +			width = pipe->partition->dest.width;
> 
>  		vsp1_wpf_write(wpf, dl, VI6_WPF_HSZCLIP, VI6_WPF_SZCLIP_EN |
>  			       (0 << VI6_WPF_SZCLIP_OFST_SHIFT) |
> @@ -255,10 +255,11 @@ static void wpf_configure(struct vsp1_entity *entity,
>  			 * order the partitions correctly.
>  			 */
>  			if (flip & BIT(WPF_CTRL_HFLIP))
> -				offset = format->width - pipe->partition.left
> -					- pipe->partition.width;
> +				offset = format->width
> +					- pipe->partition->dest.left
> +					- pipe->partition->dest.width;
>  			else
> -				offset = pipe->partition.left;
> +				offset = pipe->partition->dest.left;
> 
>  			mem.addr[0] += offset * fmtinfo->bpp[0] / 8;
>  			if (format->num_planes > 1) {

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/8] v4l: vsp1: Operate on partition struct data directly

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Friday 10 Feb 2017 20:27:33 Kieran Bingham wrote:
> When generating the partition windows, operate directly on the partition
> struct rather than copying and duplicating the processed data
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

This looks good to me, but I'd squash it into the previous patch.

> ---
>  drivers/media/platform/vsp1/vsp1_video.c | 43 ++++++++++++-------------
>  1 file changed, 21 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
> b/drivers/media/platform/vsp1/vsp1_video.c index a978508a4993..5f1886bfad26
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -189,17 +189,17 @@ static int __vsp1_video_try_format(struct vsp1_video
> *video, /**
>   * vsp1_video_partition - Calculate the active partition output window
>   *
> + * @partition: The active partition data
>   * @div_size: pre-determined maximum partition division size
>   * @index: partition index
>   *
> - * Returns a v4l2_rect describing the partition window.
> + * Generates the output partitioning positions.
>   */
> -static struct v4l2_rect vsp1_video_partition(struct vsp1_pipeline *pipe,
> -					     unsigned int div_size,
> -					     unsigned int index)
> +static void vsp1_video_partition(struct vsp1_pipeline *pipe,
> +				 struct vsp1_partition *partition,
> +				 unsigned int div_size, unsigned int index)
>  {
>  	const struct v4l2_mbus_framefmt *format;
> -	struct v4l2_rect partition;
>  	unsigned int modulus;
> 
>  	format = vsp1_entity_get_pad_format(&pipe->output->entity,
> @@ -208,18 +208,19 @@ static struct v4l2_rect vsp1_video_partition(struct
> vsp1_pipeline *pipe,
> 
>  	/* A single partition simply processes the output size in full. */
>  	if (pipe->partitions <= 1) {
> -		partition.left = 0;
> -		partition.top = 0;
> -		partition.width = format->width;
> -		partition.height = format->height;
> -		return partition;
> +		partition->dest.left = 0;
> +		partition->dest.top = 0;
> +		partition->dest.width = format->width;
> +		partition->dest.height = format->height;
> +
> +		return;
>  	}
> 
>  	/* Initialise the partition with sane starting conditions. */
> -	partition.left = index * div_size;
> -	partition.top = 0;
> -	partition.width = div_size;
> -	partition.height = format->height;
> +	partition->dest.left = index * div_size;
> +	partition->dest.top = 0;
> +	partition->dest.width = div_size;
> +	partition->dest.height = format->height;
> 
>  	modulus = format->width % div_size;
> 
> @@ -242,18 +243,16 @@ static struct v4l2_rect vsp1_video_partition(struct
> vsp1_pipeline *pipe, if (modulus < div_size / 2) {
>  			if (index == partitions - 1) {
>  				/* Halve the penultimate partition. */
> -				partition.width = div_size / 2;
> +				partition->dest.width = div_size / 2;
>  			} else if (index == partitions) {
>  				/* Increase the final partition. */
> -				partition.width = (div_size / 2) + modulus;
> -				partition.left -= div_size / 2;
> +				partition->dest.width = div_size / 2 + 
modulus;
> +				partition->dest.left -= div_size / 2;
>  			}
>  		} else if (index == partitions) {
> -			partition.width = modulus;
> +			partition->dest.width = modulus;
>  		}
>  	}
> -
> -	return partition;
>  }
> 
>  static void vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline
> *pipe) @@ -274,7 +273,7 @@ static void
> vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe) struct
> vsp1_partition *partition = &pipe->part_table[0];
> 
>  		pipe->partitions = 1;
> -		partition->dest = vsp1_video_partition(pipe, div_size, 0);
> +		vsp1_video_partition(pipe, partition, div_size, 0);
> 
>  		return;
>  	}
> @@ -294,7 +293,7 @@ static void vsp1_video_pipeline_setup_partitions(struct
> vsp1_pipeline *pipe) for (i = 0; i < pipe->partitions; i++) {
>  		struct vsp1_partition *partition = &pipe->part_table[i];
> 
> -		partition->dest = vsp1_video_partition(pipe, div_size, i);
> +		vsp1_video_partition(pipe, partition, div_size, i);
>  	}
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/8] v4l: vsp1: Allow entities to participate in the partition algorithm

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Friday 10 Feb 2017 20:27:34 Kieran Bingham wrote:
> The configuration of the pipeline, and entities directly affects the
> inputs required to each entity for the partition algorithm. Thus it
> makes sense to involve those entities in the decision making process.
> 
> Extend the entity ops API to provide an optional '.partition' call. This
> allows entities which may effect the partition window, to process based
> on their configuration.
> 
> Entities implementing this operation must return their required input
> parameters, which will be passed down the chain. This creates a process
> whereby each entity describes what is required to satisfy the required
> output to it's predecessor in the pipeline.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/vsp1/vsp1_entity.h |  8 ++++-
>  drivers/media/platform/vsp1/vsp1_pipe.c   | 22 ++++++++++++-
>  drivers/media/platform/vsp1/vsp1_pipe.h   | 35 +++++++++++++++++--
>  drivers/media/platform/vsp1/vsp1_rpf.c    | 33 +++++++++---------
>  drivers/media/platform/vsp1/vsp1_sru.c    | 29 +++++++++++++++-
>  drivers/media/platform/vsp1/vsp1_uds.c    | 45 ++++++++++++++++++++++--
>  drivers/media/platform/vsp1/vsp1_video.c  | 32 ++++++++++-------
>  drivers/media/platform/vsp1/vsp1_wpf.c    | 29 +++++++++++----
>  8 files changed, 195 insertions(+), 38 deletions(-)

[snip]

> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c
> b/drivers/media/platform/vsp1/vsp1_pipe.c index 280ba0804699..16f2eada54d5
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_pipe.c
> +++ b/drivers/media/platform/vsp1/vsp1_pipe.c
> @@ -331,6 +331,28 @@ void vsp1_pipeline_propagate_alpha(struct vsp1_pipeline
> *pipe, vsp1_uds_set_alpha(pipe->uds, dl, alpha);
>  }
> 
> +/*
> + * Propagate the partition calculations through the pipeline
> + *
> + * Work backwards through the pipe, allowing each entity to update
> + * the partition parameters based on it's configuration, and the entity

s/it's/its/

> + * connected to it's source. Each entity must produce the partition

Ditto.

> + * required for the next entity in the routing.

Maybe "for the previous entity in the pipeline" ?

> + */
> +void vsp1_pipeline_propagate_partition(struct vsp1_pipeline *pipe,
> +				       struct vsp1_partition *partition,
> +				       unsigned int index,
> +				       struct vsp1_partition_rect *rect)
> +{
> +	struct vsp1_entity *entity;
> +
> +	list_for_each_entry_reverse(entity, &pipe->entities, list_pipe) {
> +		if (entity->ops->partition)
> +			rect = entity->ops->partition(entity, pipe, partition,
> +						      index, rect);

How about modifying the rect argument in place ? I think that would simplify 
the code.

> +	}
> +}
> +
>  void vsp1_pipelines_suspend(struct vsp1_device *vsp1)
>  {
>  	unsigned long flags;
> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h
> b/drivers/media/platform/vsp1/vsp1_pipe.h index 6494c4c75023..718ca0a5eca7
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_pipe.h
> +++ b/drivers/media/platform/vsp1/vsp1_pipe.h
> @@ -60,11 +60,32 @@ enum vsp1_pipeline_state {
>  };
> 
>  /*
> + * struct vsp1_partition_rect
> + *
> + * replicates struct v4l2_rect, but with an offset to apply
> + */
> +struct vsp1_partition_rect {

Let's name this vsp1_partition_window, as that's what it describes.

> +	__s32   left;
> +	__s32   top;
> +	__u32   width;
> +	__u32   height;
> +	__u32   offset;
> +};
> +
> +/*
>   * struct vsp1_partition - A description of each partition slice performed
> by HW
> - * @dest: The position and dimension of this partition in the destination
> image
> + * @rpf: The RPF partition window configuration
> + * @uds_sink: The UDS input partition window configuration
> + * @uds_source: The UDS output partition window configuration
> + * @sru: The SRU partition window configuration
> + * @wpf: The WPF partition window configuration
>   */
>  struct vsp1_partition {
> -	struct v4l2_rect dest;
> +	struct vsp1_partition_rect rpf;
> +	struct vsp1_partition_rect uds_sink;
> +	struct vsp1_partition_rect uds_source;
> +	struct vsp1_partition_rect sru;
> +	struct vsp1_partition_rect wpf;
>  };
> 
>  /*
> @@ -117,6 +138,11 @@ struct vsp1_pipeline {
>  	struct vsp1_entity *uds;
>  	struct vsp1_entity *uds_input;
> 
> +	/*
> +	 * The order of this list should be representative of the order and

I'd say it "must be identical to the order of the entities in the pipeline".

> +	 * routing of the the pipeline, as it is assumed by the partition
> +	 * algorithm that we can walk this list in sequence.
> +	 */
>  	struct list_head entities;
> 
>  	struct vsp1_dl_list *dl;
> @@ -139,6 +165,11 @@ void vsp1_pipeline_frame_end(struct vsp1_pipeline
> *pipe); void vsp1_pipeline_propagate_alpha(struct vsp1_pipeline *pipe,
>  				   struct vsp1_dl_list *dl, unsigned int 
alpha);
> 
> +void vsp1_pipeline_propagate_partition(struct vsp1_pipeline *pipe,
> +				       struct vsp1_partition *partition,
> +				       unsigned int index,
> +				       struct vsp1_partition_rect *rect);
> +
>  void vsp1_pipelines_suspend(struct vsp1_device *vsp1);
>  void vsp1_pipelines_resume(struct vsp1_device *vsp1);
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_rpf.c
> b/drivers/media/platform/vsp1/vsp1_rpf.c index df380a237118..94541ab4ca36
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_rpf.c
> +++ b/drivers/media/platform/vsp1/vsp1_rpf.c

[snip]

> +/*
> + * Perform RPF specific calculations for the Partition Algorithm

Is the "Partition Algorithm" such an almighty power that it requires caps ? 
:-) I think I'd drop the comment, the operation kerneldoc in vsp1_entity.h 
should be enough.

> + */
> +struct vsp1_partition_rect *rpf_partition(struct vsp1_entity *entity,

This function should be static. Same comment for the other modules.

> +					  struct vsp1_pipeline *pipe,
> +					  struct vsp1_partition *partition,
> +					  unsigned int partition_idx,
> +					  struct vsp1_partition_rect *dest)
> +{
> +	/* Duplicate the target configuration to the RPF */
> +	partition->rpf = *dest;
> +
> +	return &partition->rpf;
> +}

[snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/8] v4l: vsp1: Allow entities to participate in the partition algorithm

[Laurent Pinchart]

Hi Kieran,

A few more things.

On Tuesday 14 Feb 2017 00:51:10 Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Friday 10 Feb 2017 20:27:34 Kieran Bingham wrote:
> > The configuration of the pipeline, and entities directly affects the

s/,//

> > inputs required to each entity for the partition algorithm. Thus it
> > makes sense to involve those entities in the decision making process.
> > 
> > Extend the entity ops API to provide an optional '.partition' call. This

s/call/operation/

> > allows entities which may effect the partition window, to process based

s/which/that/
s/,//
s/process/proceed/ ?

> > on their configuration.
> > 
> > Entities implementing this operation must return their required input
> > parameters, which will be passed down the chain.

s/down the chain/up the pipeline/

> > This creates a process
> > whereby each entity describes what is required to satisfy the required
> > output to it's predecessor in the pipeline.

s/it's/its/

> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> > ---
> > 
> >  drivers/media/platform/vsp1/vsp1_entity.h |  8 ++++-
> >  drivers/media/platform/vsp1/vsp1_pipe.c   | 22 ++++++++++++-
> >  drivers/media/platform/vsp1/vsp1_pipe.h   | 35 +++++++++++++++++--
> >  drivers/media/platform/vsp1/vsp1_rpf.c    | 33 +++++++++---------
> >  drivers/media/platform/vsp1/vsp1_sru.c    | 29 +++++++++++++++-
> >  drivers/media/platform/vsp1/vsp1_uds.c    | 45 ++++++++++++++++++++++--
> >  drivers/media/platform/vsp1/vsp1_video.c  | 32 ++++++++++-------
> >  drivers/media/platform/vsp1/vsp1_wpf.c    | 29 +++++++++++----
> >  8 files changed, 195 insertions(+), 38 deletions(-)
> 
> [snip]
> 
> > diff --git a/drivers/media/platform/vsp1/vsp1_pipe.c
> > b/drivers/media/platform/vsp1/vsp1_pipe.c index 280ba0804699..16f2eada54d5
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_pipe.c
> > +++ b/drivers/media/platform/vsp1/vsp1_pipe.c
> > @@ -331,6 +331,28 @@ void vsp1_pipeline_propagate_alpha(struct
> > vsp1_pipeline *pipe, vsp1_uds_set_alpha(pipe->uds, dl, alpha);
> > 
> >  }
> > 
> > +/*
> > + * Propagate the partition calculations through the pipeline
> > + *
> > + * Work backwards through the pipe, allowing each entity to update
> > + * the partition parameters based on it's configuration, and the entity
> 
> s/it's/its/
> 
> > + * connected to it's source. Each entity must produce the partition
> 
> Ditto.
> 
> > + * required for the next entity in the routing.
> 
> Maybe "for the previous entity in the pipeline" ?
> 
> > + */
> > +void vsp1_pipeline_propagate_partition(struct vsp1_pipeline *pipe,
> > +				       struct vsp1_partition *partition,
> > +				       unsigned int index,
> > +				       struct vsp1_partition_rect *rect)
> > +{
> > +	struct vsp1_entity *entity;
> > +
> > +	list_for_each_entry_reverse(entity, &pipe->entities, list_pipe) {
> > +		if (entity->ops->partition)
> > +			rect = entity->ops->partition(entity, pipe, partition,
> > +						      index, rect);
> 
> How about modifying the rect argument in place ? I think that would simplify
> the code.
> 
> > +	}
> > +}
> > +
> > 
> >  void vsp1_pipelines_suspend(struct vsp1_device *vsp1)
> >  {
> >  
> >  	unsigned long flags;
> > 
> > diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h
> > b/drivers/media/platform/vsp1/vsp1_pipe.h index 6494c4c75023..718ca0a5eca7
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_pipe.h
> > +++ b/drivers/media/platform/vsp1/vsp1_pipe.h
> > @@ -60,11 +60,32 @@ enum vsp1_pipeline_state {
> > 
> >  };
> >  
> >  /*
> > 
> > + * struct vsp1_partition_rect
> > + *
> > + * replicates struct v4l2_rect, but with an offset to apply
> > + */
> > +struct vsp1_partition_rect {
> 
> Let's name this vsp1_partition_window, as that's what it describes.
> 
> > +	__s32   left;
> > +	__s32   top;
> > +	__u32   width;
> > +	__u32   height;
> > +	__u32   offset;

The offset isn't set to a value other than 0 in this patch, I would add it in 
patch 8/8.

> > +};
> > +
> > +/*
> > 
> >   * struct vsp1_partition - A description of each partition slice
> >   performed
> > 
> > by HW
> > - * @dest: The position and dimension of this partition in the destination
> > image
> > + * @rpf: The RPF partition window configuration
> > + * @uds_sink: The UDS input partition window configuration
> > + * @uds_source: The UDS output partition window configuration
> > + * @sru: The SRU partition window configuration
> > + * @wpf: The WPF partition window configuration
> > 
> >   */
> >  
> >  struct vsp1_partition {
> > 
> > -	struct v4l2_rect dest;
> > +	struct vsp1_partition_rect rpf;
> > +	struct vsp1_partition_rect uds_sink;
> > +	struct vsp1_partition_rect uds_source;
> > +	struct vsp1_partition_rect sru;
> > +	struct vsp1_partition_rect wpf;
> > 
> >  };
> >  
> >  /*
> > 
> > @@ -117,6 +138,11 @@ struct vsp1_pipeline {
> > 
> >  	struct vsp1_entity *uds;
> >  	struct vsp1_entity *uds_input;
> > 
> > +	/*
> > +	 * The order of this list should be representative of the order and
> 
> I'd say it "must be identical to the order of the entities in the pipeline".
> > +	 * routing of the the pipeline, as it is assumed by the partition
> > +	 * algorithm that we can walk this list in sequence.
> > +	 */
> > 
> >  	struct list_head entities;
> >  	
> >  	struct vsp1_dl_list *dl;
> > 
> > @@ -139,6 +165,11 @@ void vsp1_pipeline_frame_end(struct vsp1_pipeline
> > *pipe); void vsp1_pipeline_propagate_alpha(struct vsp1_pipeline *pipe,
> > 
> >  				   struct vsp1_dl_list *dl, unsigned int
> 
> alpha);
> 
> > +void vsp1_pipeline_propagate_partition(struct vsp1_pipeline *pipe,
> > +				       struct vsp1_partition *partition,
> > +				       unsigned int index,
> > +				       struct vsp1_partition_rect *rect);
> > +
> > 
> >  void vsp1_pipelines_suspend(struct vsp1_device *vsp1);
> >  void vsp1_pipelines_resume(struct vsp1_device *vsp1);
> > 
> > diff --git a/drivers/media/platform/vsp1/vsp1_rpf.c
> > b/drivers/media/platform/vsp1/vsp1_rpf.c index df380a237118..94541ab4ca36
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_rpf.c
> > +++ b/drivers/media/platform/vsp1/vsp1_rpf.c
> 
> [snip]
> 
> > +/*
> > + * Perform RPF specific calculations for the Partition Algorithm
> 
> Is the "Partition Algorithm" such an almighty power that it requires caps ?
> 
> :-) I think I'd drop the comment, the operation kerneldoc in vsp1_entity.h
> 
> should be enough.
> 
> > + */
> > +struct vsp1_partition_rect *rpf_partition(struct vsp1_entity *entity,
> 
> This function should be static. Same comment for the other modules.
> 
> > +					  struct vsp1_pipeline *pipe,
> > +					  struct vsp1_partition *partition,
> > +					  unsigned int partition_idx,
> > +					  struct vsp1_partition_rect *dest)
> > +{
> > +	/* Duplicate the target configuration to the RPF */
> > +	partition->rpf = *dest;
> > +
> > +	return &partition->rpf;
> > +}
> 
> [snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH 7/8] v4l: vsp1: Calculate UDS phase for partitions

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Friday 10 Feb 2017 20:27:35 Kieran Bingham wrote:
> To improve image quality when scaling using the UDS we need to correctly
> determine the start phase value for each partition window.

I think you mean partition, not partition window.

> Provide helper functions for calculating the phase, and write this value
> to the registers when used.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/vsp1/vsp1_pipe.h |  4 ++-
>  drivers/media/platform/vsp1/vsp1_uds.c  | 64 +++++++++++++++++++++++++-
>  2 files changed, 67 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h
> b/drivers/media/platform/vsp1/vsp1_pipe.h index 718ca0a5eca7..0faa1c9f6184
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_pipe.h
> +++ b/drivers/media/platform/vsp1/vsp1_pipe.h
> @@ -79,6 +79,8 @@ struct vsp1_partition_rect {
>   * @uds_source: The UDS output partition window configuration
>   * @sru: The SRU partition window configuration
>   * @wpf: The WPF partition window configuration
> + * @start_phase: The UDS start phase specific to this partition.

s/specific to this/for this/ ?

> + * @end_phase: The UDS end phase specific to this partition.

Nitpicking, the other lines don't end with a period, I wouldn't here either.

>   */
>  struct vsp1_partition {
>  	struct vsp1_partition_rect rpf;
> @@ -86,6 +88,8 @@ struct vsp1_partition {
>  	struct vsp1_partition_rect uds_source;
>  	struct vsp1_partition_rect sru;
>  	struct vsp1_partition_rect wpf;
> +	unsigned int start_phase;
> +	unsigned int end_phase;
>  };
> 
>  /*
> diff --git a/drivers/media/platform/vsp1/vsp1_uds.c
> b/drivers/media/platform/vsp1/vsp1_uds.c index b274cbc2428b..9c1fb7ef3c46
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_uds.c
> +++ b/drivers/media/platform/vsp1/vsp1_uds.c
> @@ -50,6 +50,46 @@ void vsp1_uds_set_alpha(struct vsp1_entity *entity,
> struct vsp1_dl_list *dl, alpha << VI6_UDS_ALPVAL_VAL0_SHIFT);
>  }
> 
> +struct uds_phase {
> +	unsigned int mp;
> +	unsigned int prefilt_term;
> +	unsigned int prefilt_outpos;
> +	unsigned int residual;
> +};
> +
> +/*
> + * TODO: Remove start_phase if possible:
> + * 'start_phase' as we use it should always be 0 I believe,
> + * Therefore this could be removed once confirmed
> + */
> +static struct uds_phase uds_phase_calculation(int position, int
> start_phase,
> +					      int ratio)
> +{
> +	struct uds_phase phase;
> +	int alpha = ratio * position;
> +
> +	/* These must be adjusted if we ever set BLADV */
> +	phase.mp = ratio / 4096;
> +	phase.mp = phase.mp < 4 ? 1 : (phase.mp < 8 ? 2 : 4);
> +
> +	phase.prefilt_term = phase.mp * 4096;
> +	phase.prefilt_outpos = (alpha - start_phase * phase.mp)
> +			/ phase.prefilt_term;
> +	phase.residual = (alpha - start_phase * phase.mp)
> +			% phase.prefilt_term;

This requires detailed documentation.

> +	return phase;
> +}
> +
> +static int uds_start_phase(int pos, int start_phase, int ratio)
> +{
> +	struct uds_phase phase;
> +
> +	phase = uds_phase_calculation(pos, start_phase, ratio);
> +
> +	return phase.residual ? (4096 - phase.residual / phase.mp) : 0;
> +}
> +
>  /*
>   * uds_output_size - Return the output size for an input size and scaling
> ratio * @input: input size in pixels
> @@ -269,6 +309,7 @@ static void uds_configure(struct vsp1_entity *entity,
>  	const struct v4l2_mbus_framefmt *input;
>  	unsigned int hscale;
>  	unsigned int vscale;
> +	bool manual_phase = (pipe->partitions > 1);
>  	bool multitap;
> 
>  	if (params == VSP1_ENTITY_PARAMS_PARTITION) {
> @@ -287,6 +328,16 @@ static void uds_configure(struct vsp1_entity *entity,
>  					<< VI6_UDS_CLIP_SIZE_HSIZE_SHIFT) |
>  			       (partition->uds_source.height
>  					<< VI6_UDS_CLIP_SIZE_VSIZE_SHIFT));
> +
> +		if (!manual_phase)
> +			return;
> +

I think you should write the register even when using a single partition, 
otherwise it will retain its last value, which could cause issues. The AMDSLH 
bit won't bit set so the HPHASE register should be ignored, but the datasheet 
recommends setting the HSTP and HEDP fields to 0 nonetheless.

> +		vsp1_uds_write(uds, dl, VI6_UDS_HPHASE,
> +			       (partition->start_phase
> +					<< VI6_UDS_HPHASE_HSTP_SHIFT) |
> +			       (partition->end_phase
> +					<< VI6_UDS_HPHASE_HEDP_SHIFT));
> +
>  		return;
>  	}
> 
> @@ -314,7 +365,8 @@ static void uds_configure(struct vsp1_entity *entity,
> 
>  	vsp1_uds_write(uds, dl, VI6_UDS_CTRL,
>  		       (uds->scale_alpha ? VI6_UDS_CTRL_AON : 0) |
> -		       (multitap ? VI6_UDS_CTRL_BC : 0));
> +		       (multitap ? VI6_UDS_CTRL_BC : 0) |
> +		       (manual_phase ? VI6_UDS_CTRL_AMDSLH : 0));

If you we write the HPHASE register unconditionally we can remove the 
manual_phase variable and use pipe->partitions > 1 directly here.

> 
>  	vsp1_uds_write(uds, dl, VI6_UDS_PASS_BWIDTH,
>  		       (uds_passband_width(hscale)
> @@ -366,6 +418,8 @@ struct vsp1_partition_rect *uds_partition(struct
> vsp1_entity *entity, struct vsp1_uds *uds = to_uds(&entity->subdev);
>  	const struct v4l2_mbus_framefmt *output;
>  	const struct v4l2_mbus_framefmt *input;
> +	unsigned int hscale;
> +	unsigned int image_start_phase = 0;

Maybe just 'start_phase' ?

> 
>  	/* Initialise the partition state */
>  	partition->uds_sink = *dest;
> @@ -376,11 +430,19 @@ struct vsp1_partition_rect *uds_partition(struct
> vsp1_entity *entity, output = vsp1_entity_get_pad_format(&uds->entity,
> uds->entity.config, UDS_PAD_SOURCE);
> 
> +	hscale = uds_compute_ratio(input->width, output->width);
> +
>  	partition->uds_sink.width = dest->width * input->width
>  				  / output->width;
>  	partition->uds_sink.left = dest->left * input->width
>  				 / output->width;
> 
> +	partition->start_phase = uds_start_phase(partition->uds_source.left,
> +						 image_start_phase, hscale);
> +
> +	/* Renesas partition algorithm always sets end-phase as 0 */

s/as 0/to 0/ ?

> +	partition->end_phase = 0;
> +
>  	return &partition->uds_sink;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH 8/8] v4l: vsp1: Implement left edge partition algorithm overlap

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Friday 10 Feb 2017 20:27:36 Kieran Bingham wrote:
> Increase the overlap on the left edge to allow a margin to provide
> better image scaling

-EIMPOSSIBLE_TO_REVIEW I'm afraid, we need more detailed documentation.

> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/vsp1/vsp1_rpf.c |  7 +++++-
>  drivers/media/platform/vsp1/vsp1_uds.c | 39 ++++++++++++++++++++++++---
>  2 files changed, 42 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_rpf.c
> b/drivers/media/platform/vsp1/vsp1_rpf.c index 94541ab4ca36..d08cfd944b7b
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_rpf.c
> +++ b/drivers/media/platform/vsp1/vsp1_rpf.c
> @@ -247,6 +247,13 @@ struct vsp1_partition_rect *rpf_partition(struct
> vsp1_entity *entity, /* Duplicate the target configuration to the RPF */
>  	partition->rpf = *dest;
> 
> +	/*
> +	 * A partition offset, is a request for more input pixels, and a
> +	 * declaration that the consumer will clip excess.
> +	 */
> +	partition->rpf.width += dest->offset;
> +	partition->rpf.left -= dest->offset;
> +
>  	return &partition->rpf;
>  }
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_uds.c
> b/drivers/media/platform/vsp1/vsp1_uds.c index 9c1fb7ef3c46..9ee476c8db59
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_uds.c
> +++ b/drivers/media/platform/vsp1/vsp1_uds.c
> @@ -81,6 +81,20 @@ static struct uds_phase uds_phase_calculation(int
> position, int start_phase, return phase;
>  }
> 
> +static int uds_left_src_pixel(int pos, int start_phase, int ratio)
> +{
> +	struct uds_phase phase;
> +
> +	phase = uds_phase_calculation(pos, start_phase, ratio);
> +
> +	/* Renesas guard against odd values in these scale ratios here ? */
> +	if ((phase.mp == 2 && (phase.residual & 0x01)) ||
> +	    (phase.mp == 4 && (phase.residual & 0x03)))
> +		WARN_ON(1);

That's harsh. Can it happen, or can we prove it can't happen ?

> +	return phase.mp * (phase.prefilt_outpos + (phase.residual ? 1 : 0));
> +}
> +
>  static int uds_start_phase(int pos, int start_phase, int ratio)
>  {
>  	struct uds_phase phase;
> @@ -420,6 +434,8 @@ struct vsp1_partition_rect *uds_partition(struct
> vsp1_entity *entity, const struct v4l2_mbus_framefmt *input;
>  	unsigned int hscale;
>  	unsigned int image_start_phase = 0;
> +	unsigned int right_sink;
> +	unsigned int margin;
> 
>  	/* Initialise the partition state */
>  	partition->uds_sink = *dest;
> @@ -432,10 +448,25 @@ struct vsp1_partition_rect *uds_partition(struct
> vsp1_entity *entity,
> 
>  	hscale = uds_compute_ratio(input->width, output->width);
> 
> -	partition->uds_sink.width = dest->width * input->width
> -				  / output->width;
> -	partition->uds_sink.left = dest->left * input->width
> -				 / output->width;
> +	/* Handle 'left' edge of the partitions */
> +	if (partition_idx == 0) {
> +		margin = 0;
> +	} else {
> +		margin = hscale < 0x200 ? 32 : /* 8 <  scale */
> +			 hscale < 0x400 ? 16 : /* 4 <  scale <= 8 */
> +			 hscale < 0x800 ?  8 : /* 2 <  scale <= 4 */
> +					   4;  /* 1 <  scale <= 2, scale <= 1 
*/
> +	}
> +
> +	partition->uds_sink.left = uds_left_src_pixel(dest->left,
> +					image_start_phase, hscale);
> +
> +	partition->uds_sink.offset = margin;
> +
> +	right_sink = uds_left_src_pixel(dest->left + dest->width - 1,
> +					image_start_phase, hscale);
> +
> +	partition->uds_sink.width = right_sink - partition->uds_sink.left + 1;
> 
>  	partition->start_phase = uds_start_phase(partition->uds_source.left,
>  						 image_start_phase, hscale);

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/8] v4l: vsp1: Partition phase developments

[Laurent Pinchart]

Hi Kieran,

Thank you for the patches.

On Friday 10 Feb 2017 20:27:28 Kieran Bingham wrote:
> This series presents ongoing work with the scaler partition algorithm.
> 
> It is based upon the previous partition algorithm improvements submission
> [0] This series has been pushed to a tag [1] for convenience in testing.
> 
> Patches 1-3, provide fixes and additions to the register definitions needed
> for controlling the phases of the UDS.
> 
> Patches 4 and 5 rework the partition data configuration storage, opening the
> path for Patch 6 to implement a new entity operation API. This new
> '.partition' operation gives each entity an opportunity to adapt the
> partition data based on its configuration.
> 
> A new helper function "vsp1_pipeline_propagate_partition()" is provided by
> the vsp1_pipe to walk the pipeline in reverse, with each entity having the
> opportunity to define it's input requirements to the predecessors.
> 
> Partition data is stored somewhat inefficiently in this series, whilst the
> process is established and can be considered for improvement later.
> 
> Patch 7 begins the implementation of calculating the phase values in the
> UDS, and applying them in the VI6_UDS_HPHASE register appropriately. Phase
> calculations have been established from the partition algorithm pseudo code
> provided by renesas, although the 'end phase' is always set as 0 in this
> code, it is yet to be determined if this has an effect.
> 
> Finally Patch 8, begins to allow the UDS entity to perform extra overlap at
> the partition borders to provide the filters with the required data to
> generate clean transitions from one partition to the next.

I've consolidated the two series, included all my review comments, and pushed 
the result to

	git://linuxtv.org/pinchartl/media.git vsp1/partition

The result is laid out as follows.

[PATCH 01/10] v4l: vsp1: Move vsp1_video_pipeline_setup_partitions() function
[PATCH 02/10] v4l: vsp1: Calculate partition sizes at stream start
[PATCH 03/10] v4l: vsp1: Remove redundant context variables
[PATCH 04/10] v4l: vsp1: Provide UDS register updates
[PATCH 05/10] v4l: vsp1: Correct image partition parameters
[PATCH 06/10] v4l: vsp1: Move partition rectangles to struct
[PATCH 07/10] v4l: vsp1: Allow entities to participate in the partition 
algorithm
[PATCH 08/10] v4l: vsp1: Calculate UDS phase for partitions
[PATCH 09/10] v4l: vsp1: Implement left edge partition algorithm overlap
[PATCH 10/10] v4l: vsp1: Implement partition algorithm restrictions

Patches 01/10 to 06/10 look good to me, although I'm wondering whether we 
should rename vsp1_pipeline::partitions to vsp1_pipeline::num_partitions and 
vsp1_pipeline::part_table to vsp1_pipeline::partitions.

Patch 07/10 is mostly fine, I'm just a bit annoyed by the explicit pipeline 
description in struct vsp1_partition. We've discussed that previously and 
decided not to bother for now, so let's keep it as-is and revisit it when the 
rest of the series will be ready.

Patches 08/10 and 09/10 require more documentation, I don't understand what 
they do. I've rebased them on top of the review comments but haven't tested 
them yet.

Patch 10/10 needs to be reworked as the force_identity_mode mechanism doesn't 
work as explained in my review. We might also want to relax the restrictions a 
bit, based on feedback we will receive from Renesas. I'm fine merging hard 
restrictions first and relaxing them later, but at least force_identity_mode 
needs to be fixed.

You will notice that the branch I've pushed doesn't contain the suspend/resume 
fixes yet, I'll work on that next.

> [0]
> https://www.mail-archive.com/linux-renesas-soc@vger.kernel.org/msg08631.htm
> l [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/kbingham/rcar.git#vsp1/pa-p
> hases-2017-02-10
> 
> Kieran Bingham (8):
>   v4l: vsp1: Provide UDS register updates
>   v4l: vsp1: Track the SRU entity in the pipeline
>   v4l: vsp1: Correct image partition parameters
>   v4l: vsp1: Move partition rectangles to struct
>   v4l: vsp1: Operate on partition struct data directly
>   v4l: vsp1: Allow entities to participate in the partition algorithm
>   v4l: vsp1: Calculate UDS phase for partitions
>   v4l: vsp1: Implement left edge partition algorithm overlap
> 
>  drivers/media/platform/vsp1/vsp1_entity.h |   8 +-
>  drivers/media/platform/vsp1/vsp1_pipe.c   |  22 ++++-
>  drivers/media/platform/vsp1/vsp1_pipe.h   |  49 +++++++-
>  drivers/media/platform/vsp1/vsp1_regs.h   |  14 ++-
>  drivers/media/platform/vsp1/vsp1_rpf.c    |  40 +++---
>  drivers/media/platform/vsp1/vsp1_sru.c    |  29 +++++-
>  drivers/media/platform/vsp1/vsp1_uds.c    | 144 ++++++++++++++++++++++-
>  drivers/media/platform/vsp1/vsp1_video.c  |  82 ++++++++-----
>  drivers/media/platform/vsp1/vsp1_wpf.c    |  34 +++--
>  9 files changed, 364 insertions(+), 58 deletions(-)
> 
> base-commit: 0c3b6ad6a559391f367879fd4be6d2d85625bd5a

-- 
Regards,

Laurent Pinchart