[PATCH 0/4] vsp1 partition algorithm improvements

[PATCH 0/4] vsp1 partition algorithm improvements

[Kieran Bingham]

Some updates and initial improvements for the VSP1 partition algorithm that
remove redundant processing and variables, reducing the processing done in
interrupt context slightly.

Patch 1 brings in some protection against invalid pipeline configurations that
are not supported by the partition algorithm on Gen3 hardware.

Patches 2,3 and 4 clean up the calculation of the partition sizes such that they
are only calculated once at streamon - and the partition windows are stored in
the vsp1_pipeline object.

Kieran Bingham (4):
  v4l: vsp1: Implement partition algorithm restrictions
  v4l: vsp1: Move vsp1_video_pipeline_setup_partitions() function
  v4l: vsp1: Calculate partition sizes at stream start.
  v4l: vsp1: Remove redundant context variables

 drivers/media/platform/vsp1/vsp1_pipe.h  |  10 ++-
 drivers/media/platform/vsp1/vsp1_sru.c   |   7 +-
 drivers/media/platform/vsp1/vsp1_sru.h   |   1 +
 drivers/media/platform/vsp1/vsp1_video.c | 124 +++++++++++++++++++------------
 4 files changed, 89 insertions(+), 53 deletions(-)

-- 
2.7.4

[PATCH 1/4] v4l: vsp1: Implement partition algorithm restrictions

[Kieran Bingham]

The partition algorithm introduced to support scaling, and rotation on
Gen3 hardware has some restrictions on pipeline configuration.

The UDS must not be connected after the SRU in a pipeline, and whilst an
SRU can be connected before the UDS, it can only do so in identity mode.

On Gen3 hardware, the use of an SRU will always engage the partition
algorithm, therefore we must always ensure the restrictions are met on
Gen3 hardware utilising an SRU in the pipeline.

A pipeline with an SRU connected after the UDS will disable any scaling
features of the SRU.

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_sru.c   |  7 +++++--
 drivers/media/platform/vsp1/vsp1_sru.h   |  1 +
 drivers/media/platform/vsp1/vsp1_video.c | 29 ++++++++++++++++++++++++++++-
 3 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_sru.c b/drivers/media/platform/vsp1/vsp1_sru.c
index b4e568a3b4ed..42a3ed6d9461 100644
--- a/drivers/media/platform/vsp1/vsp1_sru.c
+++ b/drivers/media/platform/vsp1/vsp1_sru.c
@@ -152,7 +152,8 @@ static int sru_enum_frame_size(struct v4l2_subdev *subdev,
 		fse->min_width = format->width;
 		fse->min_height = format->height;
 		if (format->width <= SRU_MAX_SIZE / 2 &&
-		    format->height <= SRU_MAX_SIZE / 2) {
+		    format->height <= SRU_MAX_SIZE / 2 &&
+		    sru->force_identity_mode == false) {
 			fse->max_width = format->width * 2;
 			fse->max_height = format->height * 2;
 		} else {
@@ -203,7 +204,8 @@ static void sru_try_format(struct vsp1_sru *sru,
 
 		if (fmt->width <= SRU_MAX_SIZE / 2 &&
 		    fmt->height <= SRU_MAX_SIZE / 2 &&
-		    output_area > input_area * 9 / 4) {
+		    output_area > input_area * 9 / 4 &&
+		    sru->force_identity_mode == false) {
 			fmt->width = format->width * 2;
 			fmt->height = format->height * 2;
 		} else {
@@ -355,6 +357,7 @@ struct vsp1_sru *vsp1_sru_create(struct vsp1_device *vsp1)
 	v4l2_ctrl_new_custom(&sru->ctrls, &sru_intensity_control, NULL);
 
 	sru->intensity = 1;
+	sru->force_identity_mode = false;
 
 	sru->entity.subdev.ctrl_handler = &sru->ctrls;
 
diff --git a/drivers/media/platform/vsp1/vsp1_sru.h b/drivers/media/platform/vsp1/vsp1_sru.h
index 85e241457af2..f8652c04268e 100644
--- a/drivers/media/platform/vsp1/vsp1_sru.h
+++ b/drivers/media/platform/vsp1/vsp1_sru.h
@@ -30,6 +30,7 @@ struct vsp1_sru {
 	struct v4l2_ctrl_handler ctrls;
 
 	unsigned int intensity;
+	bool force_identity_mode;
 };
 
 static inline struct vsp1_sru *to_sru(struct v4l2_subdev *subdev)
diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index f19d879ce5ee..d1d3413c6fdf 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -35,6 +35,7 @@
 #include "vsp1_hgt.h"
 #include "vsp1_pipe.h"
 #include "vsp1_rwpf.h"
+#include "vsp1_sru.h"
 #include "vsp1_uds.h"
 #include "vsp1_video.h"
 
@@ -458,10 +459,12 @@ static int vsp1_video_pipeline_build_branch(struct vsp1_pipeline *pipe,
 					    struct vsp1_rwpf *input,
 					    struct vsp1_rwpf *output)
 {
+	struct vsp1_device *vsp1 = output->entity.vsp1;
 	struct media_entity_enum ent_enum;
 	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);
@@ -512,13 +515,37 @@ static int vsp1_video_pipeline_build_branch(struct vsp1_pipeline *pipe,
 			goto out;
 		}
 
-		/* UDS can't be chained. */
+		if (entity->type == VSP1_ENTITY_SRU) {
+			struct vsp1_sru *sru = to_sru(&entity->subdev);
+
+			/*
+			 * Gen3 partition algorithm restricts SRU double-scaled
+			 * resolution if it is connected after a UDS entity
+			 */
+			if (vsp1->info->gen == 3 && pipe->uds)
+				sru->force_identity_mode = true;
+
+			sru_found = true;
+		}
+
 		if (entity->type == VSP1_ENTITY_UDS) {
+			/* UDS can't be chained. */
 			if (pipe->uds) {
 				ret = -EPIPE;
 				goto out;
 			}
 
+			/*
+			 * On Gen3 hardware using the partition algorithm, the
+			 * UDS must not be connected after the SRU. Using the
+			 * SRU on Gen3 will always engage the partition
+			 * algorithm
+			 */
+			if (vsp1->info->gen == 3 && sru_found) {
+				ret = -EPIPE;
+				goto out;
+			}
+
 			pipe->uds = entity;
 			pipe->uds_input = bru_found ? pipe->bru
 					: &input->entity;
-- 
2.7.4

Re: [PATCH 1/4] v4l: vsp1: Implement partition algorithm restrictions

[Laurent Pinchart]

Hi Kieran,

(CC'ing Morimoto-san)

Thank you for the patch.

On Friday 04 Nov 2016 18:19:27 Kieran Bingham wrote:
> The partition algorithm introduced to support scaling, and rotation on

s/,// if I'm not mistaken.

> Gen3 hardware has some restrictions on pipeline configuration.

Strictly speaking this shouldn't be a limitation of the partition algorithm, 
but a limitation of the Gen3 hardware. The limitations relate to image 
partitioning as partitions are needed when using the UDS or SRU in a pipeline, 
but as far as I understand, that's the whole extent of the relationship 
between the two.

> The UDS must not be connected after the SRU in a pipeline, and whilst an
> SRU can be connected before the UDS, it can only do so in identity mode.

I assume you mean after instead of before.

I've tested the SRU-UDS and UDS-SRU pipelines with all combinations for 
downscaling, identity and upscaling for the UDS and identity and upscaling for 
the UDS. On Gen2 everything works correctly.

On Gen3 the results are as follows:

Testing SRU-UDS scaling 768x576 - 768x576 - 640x480 in RGB24: fail
Testing SRU-UDS scaling 768x576 - 768x576 - 768x576 in RGB24: pass
Testing SRU-UDS scaling 768x576 - 768x576 - 1024x768 in RGB24: pass
Testing SRU-UDS scaling 768x576 - 1536x1152 - 1280x960 in RGB24: pass
Testing SRU-UDS scaling 768x576 - 1536x1152 - 1536x1152 in RGB24: pass
Testing SRU-UDS scaling 768x576 - 1536x1152 - 2048x1536 in RGB24: pass
Testing UDS-SRU scaling 768x576 - 640x480 - 640x480 in RGB24: pass
Testing UDS-SRU scaling 768x576 - 640x480 - 1280x960 in RGB24: pass
Testing UDS-SRU scaling 768x576 - 768x576 - 768x576 in RGB24: pass
Testing UDS-SRU scaling 768x576 - 768x576 - 1536x1152 in RGB24: pass
Testing UDS-SRU scaling 768x576 - 1024x768 - 1024x768 in RGB24: pass
Testing UDS-SRU scaling 768x576 - 1024x768 - 2048x1536 in RGB24: pass
Testing SRU-UDS scaling 768x576 - 768x576 - 640x480 in YUV444M: fail
Testing SRU-UDS scaling 768x576 - 768x576 - 768x576 in YUV444M: pass
Testing SRU-UDS scaling 768x576 - 768x576 - 1024x768 in YUV444M: pass
Testing SRU-UDS scaling 768x576 - 1536x1152 - 1280x960 in YUV444M: pass
Testing SRU-UDS scaling 768x576 - 1536x1152 - 1536x1152 in YUV444M: pass
Testing SRU-UDS scaling 768x576 - 1536x1152 - 2048x1536 in YUV444M: hangs
Testing UDS-SRU scaling 768x576 - 640x480 - 640x480 in YUV444M: pass
Testing UDS-SRU scaling 768x576 - 640x480 - 1280x960 in YUV444M: fail
Testing UDS-SRU scaling 768x576 - 768x576 - 768x576 in YUV444M: pass
Testing UDS-SRU scaling 768x576 - 768x576 - 1536x1152 in YUV444M: pass
Testing UDS-SRU scaling 768x576 - 1024x768 - 1024x768 in YUV444M: pass
Testing UDS-SRU scaling 768x576 - 1024x768 - 2048x1536 in YUV444M: hangs

Two of the tests hang the VSP completely, for a reason I don't understand yet. 
Among the three tests that fail, the "UDS-SRU scaling 768x576 - 640x480 - 
1280x960 in YUV444M" tests is a false positives due to differences in scaling 
algorithms between the VSP and gen-image.

The only two tests that really fail are thus "SRU-UDS scaling 768x576 - 
768x576 - 640x480" in RGB24 and YUV444M.

The "20151116_VSP-IMG_Image_Partition_Information.pptx" document mentions the 
following restrictions:

- double-scaled super resolution and UDS cannot be used at same time.
- UDS cannot be connected after SRU.

However, from the above tests it looks like the hardware can live with more 
relaxed restrictions than the ones implemented here. I haven't tested all UDS 
scaling ratios, and certainly not under all memory bus load conditions, I 
might thus be too optimistic. Morimoto-san, would it be possible to get more 
information about this from the hardware team, to check whether the above two 
restrictions need to be honoured, or whether they come from an older hardware 
version ?

> On Gen3 hardware, the use of an SRU will always engage the partition
> algorithm, therefore we must always ensure the restrictions are met on
> Gen3 hardware utilising an SRU in the pipeline.
> 
> A pipeline with an SRU connected after the UDS will disable any scaling
> features of the SRU.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/vsp1/vsp1_sru.c   |  7 +++++--
>  drivers/media/platform/vsp1/vsp1_sru.h   |  1 +
>  drivers/media/platform/vsp1/vsp1_video.c | 29 ++++++++++++++++++++++++++++-
> 3 files changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_sru.c
> b/drivers/media/platform/vsp1/vsp1_sru.c index b4e568a3b4ed..42a3ed6d9461
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_sru.c
> +++ b/drivers/media/platform/vsp1/vsp1_sru.c
> @@ -152,7 +152,8 @@ static int sru_enum_frame_size(struct v4l2_subdev
> *subdev, fse->min_width = format->width;
>  		fse->min_height = format->height;
>  		if (format->width <= SRU_MAX_SIZE / 2 &&
> -		    format->height <= SRU_MAX_SIZE / 2) {
> +		    format->height <= SRU_MAX_SIZE / 2 &&
> +		    sru->force_identity_mode == false) {

This won't work, as force_identity_mode is set when starting the pipeline, 
while sru_enum_frame_size() and sru_try_format() are called before, when 
configuring the pipeline. I see no other option than failing pipeline 
validation instead of trying to report the problem to the user when setting 
formats (but feel free to prove me wrong).

>  			fse->max_width = format->width * 2;
>  			fse->max_height = format->height * 2;
>  		} else {
> @@ -203,7 +204,8 @@ static void sru_try_format(struct vsp1_sru *sru,
> 
>  		if (fmt->width <= SRU_MAX_SIZE / 2 &&
>  		    fmt->height <= SRU_MAX_SIZE / 2 &&
> -		    output_area > input_area * 9 / 4) {
> +		    output_area > input_area * 9 / 4 &&
> +		    sru->force_identity_mode == false) {
>  			fmt->width = format->width * 2;
>  			fmt->height = format->height * 2;
>  		} else {
> @@ -355,6 +357,7 @@ struct vsp1_sru *vsp1_sru_create(struct vsp1_device
> *vsp1) v4l2_ctrl_new_custom(&sru->ctrls, &sru_intensity_control, NULL);
> 
>  	sru->intensity = 1;
> +	sru->force_identity_mode = false;
> 
>  	sru->entity.subdev.ctrl_handler = &sru->ctrls;
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_sru.h
> b/drivers/media/platform/vsp1/vsp1_sru.h index 85e241457af2..f8652c04268e
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_sru.h
> +++ b/drivers/media/platform/vsp1/vsp1_sru.h
> @@ -30,6 +30,7 @@ struct vsp1_sru {
>  	struct v4l2_ctrl_handler ctrls;
> 
>  	unsigned int intensity;
> +	bool force_identity_mode;
>  };
> 
>  static inline struct vsp1_sru *to_sru(struct v4l2_subdev *subdev)
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
> b/drivers/media/platform/vsp1/vsp1_video.c index f19d879ce5ee..d1d3413c6fdf
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -35,6 +35,7 @@
>  #include "vsp1_hgt.h"
>  #include "vsp1_pipe.h"
>  #include "vsp1_rwpf.h"
> +#include "vsp1_sru.h"
>  #include "vsp1_uds.h"
>  #include "vsp1_video.h"
> 
> @@ -458,10 +459,12 @@ static int vsp1_video_pipeline_build_branch(struct
> vsp1_pipeline *pipe, struct vsp1_rwpf *input,
>  					    struct vsp1_rwpf *output)
>  {
> +	struct vsp1_device *vsp1 = output->entity.vsp1;
>  	struct media_entity_enum ent_enum;
>  	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);
> @@ -512,13 +515,37 @@ static int vsp1_video_pipeline_build_branch(struct
> vsp1_pipeline *pipe, goto out;
>  		}
> 
> -		/* UDS can't be chained. */
> +		if (entity->type == VSP1_ENTITY_SRU) {
> +			struct vsp1_sru *sru = to_sru(&entity->subdev);
> +
> +			/*
> +			 * Gen3 partition algorithm restricts SRU double-
scaled
> +			 * resolution if it is connected after a UDS entity
> +			 */
> +			if (vsp1->info->gen == 3 && pipe->uds)
> +				sru->force_identity_mode = true;

force_identity_mode is never reset to false.

> +
> +			sru_found = true;
> +		}
> +
>  		if (entity->type == VSP1_ENTITY_UDS) {
> +			/* UDS can't be chained. */
>  			if (pipe->uds) {
>  				ret = -EPIPE;
>  				goto out;
>  			}
> 
> +			/*
> +			 * On Gen3 hardware using the partition algorithm, the
> +			 * UDS must not be connected after the SRU. Using the
> +			 * SRU on Gen3 will always engage the partition
> +			 * algorithm
> +			 */
> +			if (vsp1->info->gen == 3 && sru_found) {
> +				ret = -EPIPE;
> +				goto out;
> +			}
> +
>  			pipe->uds = entity;
>  			pipe->uds_input = bru_found ? pipe->bru
>  					: &input->entity;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/4] v4l: vsp1: Implement partition algorithm restrictions

[Kuninori Morimoto]

Hi Laurent, Kieran

Quick response

> Testing SRU-UDS scaling 768x576 - 768x576 - 640x480 in RGB24: fail
> Testing SRU-UDS scaling 768x576 - 768x576 - 768x576 in RGB24: pass
> Testing SRU-UDS scaling 768x576 - 768x576 - 1024x768 in RGB24: pass
> Testing SRU-UDS scaling 768x576 - 1536x1152 - 1280x960 in RGB24: pass
> Testing SRU-UDS scaling 768x576 - 1536x1152 - 1536x1152 in RGB24: pass
> Testing SRU-UDS scaling 768x576 - 1536x1152 - 2048x1536 in RGB24: pass
> Testing UDS-SRU scaling 768x576 - 640x480 - 640x480 in RGB24: pass
> Testing UDS-SRU scaling 768x576 - 640x480 - 1280x960 in RGB24: pass
> Testing UDS-SRU scaling 768x576 - 768x576 - 768x576 in RGB24: pass
> Testing UDS-SRU scaling 768x576 - 768x576 - 1536x1152 in RGB24: pass
> Testing UDS-SRU scaling 768x576 - 1024x768 - 1024x768 in RGB24: pass
> Testing UDS-SRU scaling 768x576 - 1024x768 - 2048x1536 in RGB24: pass
> Testing SRU-UDS scaling 768x576 - 768x576 - 640x480 in YUV444M: fail
> Testing SRU-UDS scaling 768x576 - 768x576 - 768x576 in YUV444M: pass
> Testing SRU-UDS scaling 768x576 - 768x576 - 1024x768 in YUV444M: pass
> Testing SRU-UDS scaling 768x576 - 1536x1152 - 1280x960 in YUV444M: pass
> Testing SRU-UDS scaling 768x576 - 1536x1152 - 1536x1152 in YUV444M: pass
> Testing SRU-UDS scaling 768x576 - 1536x1152 - 2048x1536 in YUV444M: hangs
> Testing UDS-SRU scaling 768x576 - 640x480 - 640x480 in YUV444M: pass
> Testing UDS-SRU scaling 768x576 - 640x480 - 1280x960 in YUV444M: fail
> Testing UDS-SRU scaling 768x576 - 768x576 - 768x576 in YUV444M: pass
> Testing UDS-SRU scaling 768x576 - 768x576 - 1536x1152 in YUV444M: pass
> Testing UDS-SRU scaling 768x576 - 1024x768 - 1024x768 in YUV444M: pass
> Testing UDS-SRU scaling 768x576 - 1024x768 - 2048x1536 in YUV444M: hangs
(snip)
> However, from the above tests it looks like the hardware can live with more 
> relaxed restrictions than the ones implemented here. I haven't tested all UDS 
> scaling ratios, and certainly not under all memory bus load conditions, I 
> might thus be too optimistic. Morimoto-san, would it be possible to get more 
> information about this from the hardware team, to check whether the above two 
> restrictions need to be honoured, or whether they come from an older hardware 
> version ?

I asked it to HW team.
Please wait

Best regards
---
Kuninori Morimoto

Re: [PATCH 1/4] v4l: vsp1: Implement partition algorithm restrictions

[Kuninori Morimoto]

Hi Laurent, Kieran

> > Testing SRU-UDS scaling 768x576 - 768x576 - 640x480 in RGB24: fail
> > Testing SRU-UDS scaling 768x576 - 768x576 - 768x576 in RGB24: pass
> > Testing SRU-UDS scaling 768x576 - 768x576 - 1024x768 in RGB24: pass
> > Testing SRU-UDS scaling 768x576 - 1536x1152 - 1280x960 in RGB24: pass
> > Testing SRU-UDS scaling 768x576 - 1536x1152 - 1536x1152 in RGB24: pass
> > Testing SRU-UDS scaling 768x576 - 1536x1152 - 2048x1536 in RGB24: pass
> > Testing UDS-SRU scaling 768x576 - 640x480 - 640x480 in RGB24: pass
> > Testing UDS-SRU scaling 768x576 - 640x480 - 1280x960 in RGB24: pass
> > Testing UDS-SRU scaling 768x576 - 768x576 - 768x576 in RGB24: pass
> > Testing UDS-SRU scaling 768x576 - 768x576 - 1536x1152 in RGB24: pass
> > Testing UDS-SRU scaling 768x576 - 1024x768 - 1024x768 in RGB24: pass
> > Testing UDS-SRU scaling 768x576 - 1024x768 - 2048x1536 in RGB24: pass
> > Testing SRU-UDS scaling 768x576 - 768x576 - 640x480 in YUV444M: fail
> > Testing SRU-UDS scaling 768x576 - 768x576 - 768x576 in YUV444M: pass
> > Testing SRU-UDS scaling 768x576 - 768x576 - 1024x768 in YUV444M: pass
> > Testing SRU-UDS scaling 768x576 - 1536x1152 - 1280x960 in YUV444M: pass
> > Testing SRU-UDS scaling 768x576 - 1536x1152 - 1536x1152 in YUV444M: pass
> > Testing SRU-UDS scaling 768x576 - 1536x1152 - 2048x1536 in YUV444M: hangs
> > Testing UDS-SRU scaling 768x576 - 640x480 - 640x480 in YUV444M: pass
> > Testing UDS-SRU scaling 768x576 - 640x480 - 1280x960 in YUV444M: fail
> > Testing UDS-SRU scaling 768x576 - 768x576 - 768x576 in YUV444M: pass
> > Testing UDS-SRU scaling 768x576 - 768x576 - 1536x1152 in YUV444M: pass
> > Testing UDS-SRU scaling 768x576 - 1024x768 - 1024x768 in YUV444M: pass
> > Testing UDS-SRU scaling 768x576 - 1024x768 - 2048x1536 in YUV444M: hangs
> (snip)
> > However, from the above tests it looks like the hardware can live with more 
> > relaxed restrictions than the ones implemented here. I haven't tested all UDS 
> > scaling ratios, and certainly not under all memory bus load conditions, I 
> > might thus be too optimistic. Morimoto-san, would it be possible to get more 
> > information about this from the hardware team, to check whether the above two 
> > restrictions need to be honoured, or whether they come from an older hardware 
> > version ?
> 
> I asked it to HW team.
> Please wait

We still not yet get clear answer from HW team.
It is still researching

Best regards
---
Kuninori Morimoto

Re: [PATCH 1/4] v4l: vsp1: Implement partition algorithm restrictions

[Kuninori Morimoto]

Hi Laurent, Kieran

> > > Testing SRU-UDS scaling 768x576 - 768x576 - 640x480 in RGB24: fail
> > > Testing SRU-UDS scaling 768x576 - 768x576 - 768x576 in RGB24: pass
> > > Testing SRU-UDS scaling 768x576 - 768x576 - 1024x768 in RGB24: pass
> > > Testing SRU-UDS scaling 768x576 - 1536x1152 - 1280x960 in RGB24: pass
> > > Testing SRU-UDS scaling 768x576 - 1536x1152 - 1536x1152 in RGB24: pass
> > > Testing SRU-UDS scaling 768x576 - 1536x1152 - 2048x1536 in RGB24: pass
> > > Testing UDS-SRU scaling 768x576 - 640x480 - 640x480 in RGB24: pass
> > > Testing UDS-SRU scaling 768x576 - 640x480 - 1280x960 in RGB24: pass
> > > Testing UDS-SRU scaling 768x576 - 768x576 - 768x576 in RGB24: pass
> > > Testing UDS-SRU scaling 768x576 - 768x576 - 1536x1152 in RGB24: pass
> > > Testing UDS-SRU scaling 768x576 - 1024x768 - 1024x768 in RGB24: pass
> > > Testing UDS-SRU scaling 768x576 - 1024x768 - 2048x1536 in RGB24: pass
> > > Testing SRU-UDS scaling 768x576 - 768x576 - 640x480 in YUV444M: fail
> > > Testing SRU-UDS scaling 768x576 - 768x576 - 768x576 in YUV444M: pass
> > > Testing SRU-UDS scaling 768x576 - 768x576 - 1024x768 in YUV444M: pass
> > > Testing SRU-UDS scaling 768x576 - 1536x1152 - 1280x960 in YUV444M: pass
> > > Testing SRU-UDS scaling 768x576 - 1536x1152 - 1536x1152 in YUV444M: pass
> > > Testing SRU-UDS scaling 768x576 - 1536x1152 - 2048x1536 in YUV444M: hangs
> > > Testing UDS-SRU scaling 768x576 - 640x480 - 640x480 in YUV444M: pass
> > > Testing UDS-SRU scaling 768x576 - 640x480 - 1280x960 in YUV444M: fail
> > > Testing UDS-SRU scaling 768x576 - 768x576 - 768x576 in YUV444M: pass
> > > Testing UDS-SRU scaling 768x576 - 768x576 - 1536x1152 in YUV444M: pass
> > > Testing UDS-SRU scaling 768x576 - 1024x768 - 1024x768 in YUV444M: pass
> > > Testing UDS-SRU scaling 768x576 - 1024x768 - 2048x1536 in YUV444M: hangs
> > (snip)
> > > However, from the above tests it looks like the hardware can live with more 
> > > relaxed restrictions than the ones implemented here. I haven't tested all UDS 
> > > scaling ratios, and certainly not under all memory bus load conditions, I 
> > > might thus be too optimistic. Morimoto-san, would it be possible to get more 
> > > information about this from the hardware team, to check whether the above two 
> > > restrictions need to be honoured, or whether they come from an older hardware 
> > > version ?
> > 
> > I asked it to HW team.
> > Please wait

I'm still waiting from HW team's response, but can you check
"32.3.7 Image partition for VSPI processing" on v0.53 datasheet ?
(v0.53 is for ES2.0, but this chapter should be same for ES1.x / ES2.0)
You may / may not find something from here

Best regards
---
Kuninori Morimoto

Re: [PATCH 1/4] v4l: vsp1: Implement partition algorithm restrictions

[Laurent Pinchart]

Hi Morimoto-san,

On Monday 06 Mar 2017 06:17:47 Kuninori Morimoto wrote:
> Hi Laurent, Kieran
> 
> >>> Testing SRU-UDS scaling 768x576 - 768x576 - 640x480 in RGB24: fail
> >>> Testing SRU-UDS scaling 768x576 - 768x576 - 768x576 in RGB24: pass
> >>> Testing SRU-UDS scaling 768x576 - 768x576 - 1024x768 in RGB24: pass
> >>> Testing SRU-UDS scaling 768x576 - 1536x1152 - 1280x960 in RGB24: pass
> >>> Testing SRU-UDS scaling 768x576 - 1536x1152 - 1536x1152 in RGB24: pass
> >>> Testing SRU-UDS scaling 768x576 - 1536x1152 - 2048x1536 in RGB24: pass
> >>> Testing UDS-SRU scaling 768x576 - 640x480 - 640x480 in RGB24: pass
> >>> Testing UDS-SRU scaling 768x576 - 640x480 - 1280x960 in RGB24: pass
> >>> Testing UDS-SRU scaling 768x576 - 768x576 - 768x576 in RGB24: pass
> >>> Testing UDS-SRU scaling 768x576 - 768x576 - 1536x1152 in RGB24: pass
> >>> Testing UDS-SRU scaling 768x576 - 1024x768 - 1024x768 in RGB24: pass
> >>> Testing UDS-SRU scaling 768x576 - 1024x768 - 2048x1536 in RGB24: pass
> >>> Testing SRU-UDS scaling 768x576 - 768x576 - 640x480 in YUV444M: fail
> >>> Testing SRU-UDS scaling 768x576 - 768x576 - 768x576 in YUV444M: pass
> >>> Testing SRU-UDS scaling 768x576 - 768x576 - 1024x768 in YUV444M: pass
> >>> Testing SRU-UDS scaling 768x576 - 1536x1152 - 1280x960 in YUV444M: pass
> >>> Testing SRU-UDS scaling 768x576 - 1536x1152 - 1536x1152 in YUV444M: pass
> >>> Testing SRU-UDS scaling 768x576 - 1536x1152 - 2048x1536 in YUV444M:hangs
> >>> Testing UDS-SRU scaling 768x576 - 640x480 - 640x480 in YUV444M: pass
> >>> Testing UDS-SRU scaling 768x576 - 640x480 - 1280x960 in YUV444M: fail
> >>> Testing UDS-SRU scaling 768x576 - 768x576 - 768x576 in YUV444M: pass
> >>> Testing UDS-SRU scaling 768x576 - 768x576 - 1536x1152 in YUV444M: pass
> >>> Testing UDS-SRU scaling 768x576 - 1024x768 - 1024x768 in YUV444M: pass
> >>> Testing UDS-SRU scaling 768x576 - 1024x768 - 2048x1536 in YUV444M:hangs
> >> 
> >> (snip)
> >> 
> >>> However, from the above tests it looks like the hardware can live with
> >>> more relaxed restrictions than the ones implemented here. I haven't
> >>> tested all UDS scaling ratios, and certainly not under all memory bus
> >>> load conditions, I might thus be too optimistic. Morimoto-san, would it
> >>> be possible to get more information about this from the hardware team,
> >>> to check whether the above two restrictions need to be honoured, or
> >> whether they come from an older hardware version ?
> >> 
> >> I asked it to HW team.
> >> Please wait
> 
> I'm still waiting from HW team's response, but can you check
> "32.3.7 Image partition for VSPI processing" on v0.53 datasheet ?
> (v0.53 is for ES2.0, but this chapter should be same for ES1.x / ES2.0)
> You may / may not find something from here

That's very detailed, good job of the documentation writers ! Please thank 
them for me if you know who they are :-)

I'm sure we will find useful information there. Kieran, could you please have 
a look when you'll be back at the end of this week, and list the points that 
you think we don't address correctly yet ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/4] v4l: vsp1: Implement partition algorithm restrictions

[Kieran Bingham]

Hi Laurent, Morimoto-san,

On 06/03/17 15:16, Laurent Pinchart wrote:
> Hi Morimoto-san,
> 
> On Monday 06 Mar 2017 06:17:47 Kuninori Morimoto wrote:
>> Hi Laurent, Kieran
>>
>>>>
>>>> I asked it to HW team.
>>>> Please wait
>>
>> I'm still waiting from HW team's response, but can you check
>> "32.3.7 Image partition for VSPI processing" on v0.53 datasheet ?
>> (v0.53 is for ES2.0, but this chapter should be same for ES1.x / ES2.0)
>> You may / may not find something from here
> 
> That's very detailed, good job of the documentation writers ! Please thank 
> them for me if you know who they are :-)
> 
> I'm sure we will find useful information there. Kieran, could you please have 
> a look when you'll be back at the end of this week, and list the points that 
> you think we don't address correctly yet ?
> 

No, I'm afraid I can not. :-D

I have
 R-Car-Gen3-rev0.51e.pdf
and
 R-Car-Gen3-rev0.52E.pdf

Neither of these files has a section
"32.3.7 Image partition for VSPI processing"

If I find a link to a new version of the datasheet in my inbox then I will
certainly consider changing my decision ;-)

--
Regards

Kieran

Re: [PATCH 1/4] v4l: vsp1: Implement partition algorithm restrictions

[Kieran Bingham]

Hi Laurent,

On 13/02/17 19:17, Laurent Pinchart wrote:
> Hi Kieran,
> 
> (CC'ing Morimoto-san)
> 
> Thank you for the patch.
> 
> On Friday 04 Nov 2016 18:19:27 Kieran Bingham wrote:
>> The partition algorithm introduced to support scaling, and rotation on
> 
> s/,// if I'm not mistaken.
> 
>> Gen3 hardware has some restrictions on pipeline configuration.
> 
> Strictly speaking this shouldn't be a limitation of the partition algorithm, 
> but a limitation of the Gen3 hardware. The limitations relate to image 
> partitioning as partitions are needed when using the UDS or SRU in a pipeline, 
> but as far as I understand, that's the whole extent of the relationship 
> between the two.

How about swapping those two sentences over:

Gen3 hardware has some restrictions on pipeline configuration when using the
partition algorithm.


>> The UDS must not be connected after the SRU in a pipeline, and whilst an
>> SRU can be connected before the UDS, it can only do so in identity mode.
> 
> I assume you mean after instead of before.

Indeed - I'm rather contradicting myself in the above sentence otherwise :)


> I've tested the SRU-UDS and UDS-SRU pipelines with all combinations for 
> downscaling, identity and upscaling for the UDS and identity and upscaling for 
> the UDS. On Gen2 everything works correctly.
> 
> On Gen3 the results are as follows:
> 
> Testing SRU-UDS scaling 768x576 - 768x576 - 640x480 in RGB24: fail
> Testing SRU-UDS scaling 768x576 - 768x576 - 768x576 in RGB24: pass
> Testing SRU-UDS scaling 768x576 - 768x576 - 1024x768 in RGB24: pass
> Testing SRU-UDS scaling 768x576 - 1536x1152 - 1280x960 in RGB24: pass
> Testing SRU-UDS scaling 768x576 - 1536x1152 - 1536x1152 in RGB24: pass
> Testing SRU-UDS scaling 768x576 - 1536x1152 - 2048x1536 in RGB24: pass
> Testing UDS-SRU scaling 768x576 - 640x480 - 640x480 in RGB24: pass
> Testing UDS-SRU scaling 768x576 - 640x480 - 1280x960 in RGB24: pass
> Testing UDS-SRU scaling 768x576 - 768x576 - 768x576 in RGB24: pass
> Testing UDS-SRU scaling 768x576 - 768x576 - 1536x1152 in RGB24: pass
> Testing UDS-SRU scaling 768x576 - 1024x768 - 1024x768 in RGB24: pass
> Testing UDS-SRU scaling 768x576 - 1024x768 - 2048x1536 in RGB24: pass
> Testing SRU-UDS scaling 768x576 - 768x576 - 640x480 in YUV444M: fail
> Testing SRU-UDS scaling 768x576 - 768x576 - 768x576 in YUV444M: pass
> Testing SRU-UDS scaling 768x576 - 768x576 - 1024x768 in YUV444M: pass
> Testing SRU-UDS scaling 768x576 - 1536x1152 - 1280x960 in YUV444M: pass
> Testing SRU-UDS scaling 768x576 - 1536x1152 - 1536x1152 in YUV444M: pass
> Testing SRU-UDS scaling 768x576 - 1536x1152 - 2048x1536 in YUV444M: hangs
> Testing UDS-SRU scaling 768x576 - 640x480 - 640x480 in YUV444M: pass
> Testing UDS-SRU scaling 768x576 - 640x480 - 1280x960 in YUV444M: fail
> Testing UDS-SRU scaling 768x576 - 768x576 - 768x576 in YUV444M: pass
> Testing UDS-SRU scaling 768x576 - 768x576 - 1536x1152 in YUV444M: pass
> Testing UDS-SRU scaling 768x576 - 1024x768 - 1024x768 in YUV444M: pass
> Testing UDS-SRU scaling 768x576 - 1024x768 - 2048x1536 in YUV444M: hangs

<longshot>
 Do you still have the patch to vsp-tests for all these extra tests/checks?
</longshot>

> Two of the tests hang the VSP completely, for a reason I don't understand yet. 
> Among the three tests that fail, the "UDS-SRU scaling 768x576 - 640x480 - 
> 1280x960 in YUV444M" tests is a false positives due to differences in scaling 
> algorithms between the VSP and gen-image.

Did the YUV444M tests hang repeatedly on the same test? I have seen 'random'
hangs on seemingly correct pipelines with YUV444M ... But reset/repeat test -
and it will work again. Most distressing, but perhaps might need deeper
investigation with the hardware.



> The only two tests that really fail are thus "SRU-UDS scaling 768x576 - 
> 768x576 - 640x480" in RGB24 and YUV444M.
> 
> The "20151116_VSP-IMG_Image_Partition_Information.pptx" document mentions the 
> following restrictions:
> 
> - double-scaled super resolution and UDS cannot be used at same time.
> - UDS cannot be connected after SRU.
> 
> However, from the above tests it looks like the hardware can live with more 
> relaxed restrictions than the ones implemented here. I haven't tested all UDS 
> scaling ratios, and certainly not under all memory bus load conditions, I 
> might thus be too optimistic. Morimoto-san, would it be possible to get more 
> information about this from the hardware team, to check whether the above two 
> restrictions need to be honoured, or whether they come from an older hardware 
> version ?


Well, the updated documentation is certainly more descriptive than the first set
I saw when I started this. So I'll try to look at how I can update in this regards.

My theory would have been that the SRU / UDS can be used in combination, as long
as the input/output requirements are correctly met for both cells, whilst taking
into consideration the scaling up or down that they perform.

Thus I had thought that perhaps this restriction could be relaxed in the code
once we have 'negotiation' of the partition between the entities ... however
your testing shows SRUx1 -> UDS as the fail case, which I would have expected to
be an 'easy pass'

Perhaps later we might investigate if it can be worked around in software, but
as the documentation states it is unsupported, and we have (even if it's just
one) failure case matching that statement - perhaps it's best to provide the
limitation for now.

Morimoto, was there any further feedback from the hardware engineers on this
topic? (other than the updated documentation/datasheet)


>> On Gen3 hardware, the use of an SRU will always engage the partition
>> algorithm, therefore we must always ensure the restrictions are met on
>> Gen3 hardware utilising an SRU in the pipeline.
>>
>> A pipeline with an SRU connected after the UDS will disable any scaling
>> features of the SRU.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>> ---
>>  drivers/media/platform/vsp1/vsp1_sru.c   |  7 +++++--
>>  drivers/media/platform/vsp1/vsp1_sru.h   |  1 +
>>  drivers/media/platform/vsp1/vsp1_video.c | 29 ++++++++++++++++++++++++++++-
>> 3 files changed, 34 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/platform/vsp1/vsp1_sru.c
>> b/drivers/media/platform/vsp1/vsp1_sru.c index b4e568a3b4ed..42a3ed6d9461
>> 100644
>> --- a/drivers/media/platform/vsp1/vsp1_sru.c
>> +++ b/drivers/media/platform/vsp1/vsp1_sru.c
>> @@ -152,7 +152,8 @@ static int sru_enum_frame_size(struct v4l2_subdev
>> *subdev, fse->min_width = format->width;
>>  		fse->min_height = format->height;
>>  		if (format->width <= SRU_MAX_SIZE / 2 &&
>> -		    format->height <= SRU_MAX_SIZE / 2) {
>> +		    format->height <= SRU_MAX_SIZE / 2 &&
>> +		    sru->force_identity_mode == false) {
> 
> This won't work, as force_identity_mode is set when starting the pipeline, 
> while sru_enum_frame_size() and sru_try_format() are called before, when 
> configuring the pipeline. I see no other option than failing pipeline 
> validation instead of trying to report the problem to the user when setting 
> formats (but feel free to prove me wrong).

Good spot.

I think you're right. Of course the links are already defined at this point, so
perhaps we could parse the media-device graph - but that seems a bit ugly/overkill.



> 
>>  			fse->max_width = format->width * 2;
>>  			fse->max_height = format->height * 2;
>>  		} else {
>> @@ -203,7 +204,8 @@ static void sru_try_format(struct vsp1_sru *sru,
>>
>>  		if (fmt->width <= SRU_MAX_SIZE / 2 &&
>>  		    fmt->height <= SRU_MAX_SIZE / 2 &&
>> -		    output_area > input_area * 9 / 4) {
>> +		    output_area > input_area * 9 / 4 &&
>> +		    sru->force_identity_mode == false) {
>>  			fmt->width = format->width * 2;
>>  			fmt->height = format->height * 2;
>>  		} else {
>> @@ -355,6 +357,7 @@ struct vsp1_sru *vsp1_sru_create(struct vsp1_device
>> *vsp1) v4l2_ctrl_new_custom(&sru->ctrls, &sru_intensity_control, NULL);
>>
>>  	sru->intensity = 1;
>> +	sru->force_identity_mode = false;
>>
>>  	sru->entity.subdev.ctrl_handler = &sru->ctrls;
>>
>> diff --git a/drivers/media/platform/vsp1/vsp1_sru.h
>> b/drivers/media/platform/vsp1/vsp1_sru.h index 85e241457af2..f8652c04268e
>> 100644
>> --- a/drivers/media/platform/vsp1/vsp1_sru.h
>> +++ b/drivers/media/platform/vsp1/vsp1_sru.h
>> @@ -30,6 +30,7 @@ struct vsp1_sru {
>>  	struct v4l2_ctrl_handler ctrls;
>>
>>  	unsigned int intensity;
>> +	bool force_identity_mode;
>>  };
>>
>>  static inline struct vsp1_sru *to_sru(struct v4l2_subdev *subdev)
>> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
>> b/drivers/media/platform/vsp1/vsp1_video.c index f19d879ce5ee..d1d3413c6fdf
>> 100644
>> --- a/drivers/media/platform/vsp1/vsp1_video.c
>> +++ b/drivers/media/platform/vsp1/vsp1_video.c
>> @@ -35,6 +35,7 @@
>>  #include "vsp1_hgt.h"
>>  #include "vsp1_pipe.h"
>>  #include "vsp1_rwpf.h"
>> +#include "vsp1_sru.h"
>>  #include "vsp1_uds.h"
>>  #include "vsp1_video.h"
>>
>> @@ -458,10 +459,12 @@ static int vsp1_video_pipeline_build_branch(struct
>> vsp1_pipeline *pipe, struct vsp1_rwpf *input,
>>  					    struct vsp1_rwpf *output)
>>  {
>> +	struct vsp1_device *vsp1 = output->entity.vsp1;
>>  	struct media_entity_enum ent_enum;
>>  	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);
>> @@ -512,13 +515,37 @@ static int vsp1_video_pipeline_build_branch(struct
>> vsp1_pipeline *pipe, goto out;
>>  		}
>>
>> -		/* UDS can't be chained. */
>> +		if (entity->type == VSP1_ENTITY_SRU) {
>> +			struct vsp1_sru *sru = to_sru(&entity->subdev);
>> +
>> +			/*
>> +			 * Gen3 partition algorithm restricts SRU double-
> scaled
>> +			 * resolution if it is connected after a UDS entity
>> +			 */
>> +			if (vsp1->info->gen == 3 && pipe->uds)
>> +				sru->force_identity_mode = true;
> 
> force_identity_mode is never reset to false.

it is in vsp1_sru_create(),
Aha, but vsp1_sru_create() is when the entities are created - and the pipeline
could be reconfigured/relinked.

I guess I should reset it at the beginning of the pipeline_build operation.


> 
>> +
>> +			sru_found = true;
>> +		}
>> +
>>  		if (entity->type == VSP1_ENTITY_UDS) {
>> +			/* UDS can't be chained. */
>>  			if (pipe->uds) {
>>  				ret = -EPIPE;
>>  				goto out;
>>  			}
>>
>> +			/*
>> +			 * On Gen3 hardware using the partition algorithm, the
>> +			 * UDS must not be connected after the SRU. Using the
>> +			 * SRU on Gen3 will always engage the partition
>> +			 * algorithm
>> +			 */
>> +			if (vsp1->info->gen == 3 && sru_found) {
>> +				ret = -EPIPE;
>> +				goto out;
>> +			}
>> +
>>  			pipe->uds = entity;
>>  			pipe->uds_input = bru_found ? pipe->bru
>>  					: &input->entity;

--
Kieran

[PATCH 2/4] v4l: vsp1: Move vsp1_video_pipeline_setup_partitions() function

[Kieran Bingham]

Separate the code change from the function move so that code changes can
be clearly identified. This commit has no functional change.

The partition algorithm functions will be changed, and
vsp1_video_partition() will call vsp1_video_pipeline_setup_partitions().
To prepare for that, move the function without any code change.

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

diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index d1d3413c6fdf..6d43c02bbc56 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -175,43 +175,6 @@ static int __vsp1_video_try_format(struct vsp1_video *video,
  * VSP1 Partition Algorithm support
  */
 
-static void vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe)
-{
-	struct vsp1_device *vsp1 = pipe->output->entity.vsp1;
-	const struct v4l2_mbus_framefmt *format;
-	struct vsp1_entity *entity;
-	unsigned int div_size;
-
-	/*
-	 * Partitions are computed on the size before rotation, use the format
-	 * at the WPF sink.
-	 */
-	format = vsp1_entity_get_pad_format(&pipe->output->entity,
-					    pipe->output->entity.config,
-					    RWPF_PAD_SINK);
-	div_size = format->width;
-
-	/* Gen2 hardware doesn't require image partitioning. */
-	if (vsp1->info->gen == 2) {
-		pipe->div_size = div_size;
-		pipe->partitions = 1;
-		return;
-	}
-
-	list_for_each_entry(entity, &pipe->entities, list_pipe) {
-		unsigned int entity_max = VSP1_VIDEO_MAX_WIDTH;
-
-		if (entity->ops->max_width) {
-			entity_max = entity->ops->max_width(entity, pipe);
-			if (entity_max)
-				div_size = min(div_size, entity_max);
-		}
-	}
-
-	pipe->div_size = div_size;
-	pipe->partitions = DIV_ROUND_UP(format->width, div_size);
-}
-
 /**
  * vsp1_video_partition - Calculate the active partition output window
  *
@@ -286,6 +249,43 @@ static struct v4l2_rect vsp1_video_partition(struct vsp1_pipeline *pipe,
 	return partition;
 }
 
+static void vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe)
+{
+	struct vsp1_device *vsp1 = pipe->output->entity.vsp1;
+	const struct v4l2_mbus_framefmt *format;
+	struct vsp1_entity *entity;
+	unsigned int div_size;
+
+	/*
+	 * Partitions are computed on the size before rotation, use the format
+	 * at the WPF sink.
+	 */
+	format = vsp1_entity_get_pad_format(&pipe->output->entity,
+					    pipe->output->entity.config,
+					    RWPF_PAD_SINK);
+	div_size = format->width;
+
+	/* Gen2 hardware doesn't require image partitioning. */
+	if (vsp1->info->gen == 2) {
+		pipe->div_size = div_size;
+		pipe->partitions = 1;
+		return;
+	}
+
+	list_for_each_entry(entity, &pipe->entities, list_pipe) {
+		unsigned int entity_max = VSP1_VIDEO_MAX_WIDTH;
+
+		if (entity->ops->max_width) {
+			entity_max = entity->ops->max_width(entity, pipe);
+			if (entity_max)
+				div_size = min(div_size, entity_max);
+		}
+	}
+
+	pipe->div_size = div_size;
+	pipe->partitions = DIV_ROUND_UP(format->width, div_size);
+}
+
 /* -----------------------------------------------------------------------------
  * Pipeline Management
  */
-- 
2.7.4

Re: [PATCH 2/4] v4l: vsp1: Move vsp1_video_pipeline_setup_partitions() function

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Friday 04 Nov 2016 18:19:28 Kieran Bingham wrote:
> Separate the code change from the function move so that code changes can
> be clearly identified. This commit has no functional change.
> 
> The partition algorithm functions will be changed, and
> vsp1_video_partition() will call vsp1_video_pipeline_setup_partitions().
> To prepare for that, move the function without any code change.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

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

(assuming I don't conclude in my review of patches 3/4 and 4/4 that this isn't 
needed :-))

> ---
>  drivers/media/platform/vsp1/vsp1_video.c | 74 ++++++++++++++---------------
>  1 file changed, 37 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
> b/drivers/media/platform/vsp1/vsp1_video.c index d1d3413c6fdf..6d43c02bbc56
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -175,43 +175,6 @@ static int __vsp1_video_try_format(struct vsp1_video
> *video, * VSP1 Partition Algorithm support
>   */
> 
> -static void vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline
> *pipe) -{
> -	struct vsp1_device *vsp1 = pipe->output->entity.vsp1;
> -	const struct v4l2_mbus_framefmt *format;
> -	struct vsp1_entity *entity;
> -	unsigned int div_size;
> -
> -	/*
> -	 * Partitions are computed on the size before rotation, use the format
> -	 * at the WPF sink.
> -	 */
> -	format = vsp1_entity_get_pad_format(&pipe->output->entity,
> -					    pipe->output->entity.config,
> -					    RWPF_PAD_SINK);
> -	div_size = format->width;
> -
> -	/* Gen2 hardware doesn't require image partitioning. */
> -	if (vsp1->info->gen == 2) {
> -		pipe->div_size = div_size;
> -		pipe->partitions = 1;
> -		return;
> -	}
> -
> -	list_for_each_entry(entity, &pipe->entities, list_pipe) {
> -		unsigned int entity_max = VSP1_VIDEO_MAX_WIDTH;
> -
> -		if (entity->ops->max_width) {
> -			entity_max = entity->ops->max_width(entity, pipe);
> -			if (entity_max)
> -				div_size = min(div_size, entity_max);
> -		}
> -	}
> -
> -	pipe->div_size = div_size;
> -	pipe->partitions = DIV_ROUND_UP(format->width, div_size);
> -}
> -
>  /**
>   * vsp1_video_partition - Calculate the active partition output window
>   *
> @@ -286,6 +249,43 @@ static struct v4l2_rect vsp1_video_partition(struct
> vsp1_pipeline *pipe, return partition;
>  }
> 
> +static void vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline
> *pipe) +{
> +	struct vsp1_device *vsp1 = pipe->output->entity.vsp1;
> +	const struct v4l2_mbus_framefmt *format;
> +	struct vsp1_entity *entity;
> +	unsigned int div_size;
> +
> +	/*
> +	 * Partitions are computed on the size before rotation, use the format
> +	 * at the WPF sink.
> +	 */
> +	format = vsp1_entity_get_pad_format(&pipe->output->entity,
> +					    pipe->output->entity.config,
> +					    RWPF_PAD_SINK);
> +	div_size = format->width;
> +
> +	/* Gen2 hardware doesn't require image partitioning. */
> +	if (vsp1->info->gen == 2) {
> +		pipe->div_size = div_size;
> +		pipe->partitions = 1;
> +		return;
> +	}
> +
> +	list_for_each_entry(entity, &pipe->entities, list_pipe) {
> +		unsigned int entity_max = VSP1_VIDEO_MAX_WIDTH;
> +
> +		if (entity->ops->max_width) {
> +			entity_max = entity->ops->max_width(entity, pipe);
> +			if (entity_max)
> +				div_size = min(div_size, entity_max);
> +		}
> +	}
> +
> +	pipe->div_size = div_size;
> +	pipe->partitions = DIV_ROUND_UP(format->width, div_size);
> +}
> +
>  /*
> ---------------------------------------------------------------------------
> -- * Pipeline Management
>   */

-- 
Regards,

Laurent Pinchart

[PATCH 3/4] v4l: vsp1: Calculate partition sizes at stream start.

[Kieran Bingham]

Previously the active window and partition sizes for each partition is
calculated for each partition every frame. This data is constant and
only needs to be calculated once at the start of the stream.

Extend the vsp1_pipe object to store the maximum number of partitions
possible and pre-calculate the partition sizes into this table.

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

diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h b/drivers/media/platform/vsp1/vsp1_pipe.h
index f181949824c9..3af96c4ea244 100644
--- a/drivers/media/platform/vsp1/vsp1_pipe.h
+++ b/drivers/media/platform/vsp1/vsp1_pipe.h
@@ -20,6 +20,9 @@
 
 #include <media/media-entity.h>
 
+/* Max Video Width / Min Partition Size = 8190/128 */
+#define VSP1_PIPE_MAX_PARTITIONS 64
+
 struct vsp1_dl_list;
 struct vsp1_rwpf;
 
@@ -81,7 +84,9 @@ enum vsp1_pipeline_state {
  * @dl: display list associated with the pipeline
  * @div_size: The maximum allowed partition size for the pipeline
  * @partitions: The number of partitions used to process one frame
+ * @partition: The current partition for configuration to process
  * @current_partition: The partition number currently being configured
+ * @part_table: The pre-calculated partitions used by the pipeline
  */
 struct vsp1_pipeline {
 	struct media_pipeline pipe;
@@ -116,6 +121,7 @@ struct vsp1_pipeline {
 	unsigned int partitions;
 	struct v4l2_rect partition;
 	unsigned int current_partition;
+	struct v4l2_rect part_table[VSP1_PIPE_MAX_PARTITIONS];
 };
 
 void vsp1_pipeline_reset(struct vsp1_pipeline *pipe);
diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index 6d43c02bbc56..c4a8c30df108 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -255,6 +255,7 @@ static void vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe)
 	const struct v4l2_mbus_framefmt *format;
 	struct vsp1_entity *entity;
 	unsigned int div_size;
+	int i;
 
 	/*
 	 * Partitions are computed on the size before rotation, use the format
@@ -269,6 +270,7 @@ static void vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe)
 	if (vsp1->info->gen == 2) {
 		pipe->div_size = div_size;
 		pipe->partitions = 1;
+		pipe->part_table[0] = vsp1_video_partition(pipe, div_size, 0);
 		return;
 	}
 
@@ -284,6 +286,9 @@ static void vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe)
 
 	pipe->div_size = div_size;
 	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);
 }
 
 /* -----------------------------------------------------------------------------
@@ -355,8 +360,7 @@ static void vsp1_video_pipeline_run_partition(struct vsp1_pipeline *pipe,
 {
 	struct vsp1_entity *entity;
 
-	pipe->partition = vsp1_video_partition(pipe, pipe->div_size,
-					       pipe->current_partition);
+	pipe->partition = pipe->part_table[pipe->current_partition];
 
 	list_for_each_entry(entity, &pipe->entities, list_pipe) {
 		if (entity->ops->configure)
-- 
2.7.4

Re: [PATCH 3/4] v4l: vsp1: Calculate partition sizes at stream start.

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Friday 04 Nov 2016 18:19:29 Kieran Bingham wrote:
> Previously the active window and partition sizes for each partition is

s/is/were/

> calculated for each partition every frame. This data is constant and
> only needs to be calculated once at the start of the stream.
> 
> Extend the vsp1_pipe object to store the maximum number of partitions
> possible and pre-calculate the partition sizes into this table.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/vsp1/vsp1_pipe.h  | 6 ++++++
>  drivers/media/platform/vsp1/vsp1_video.c | 8 ++++++--
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h
> b/drivers/media/platform/vsp1/vsp1_pipe.h index f181949824c9..3af96c4ea244
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_pipe.h
> +++ b/drivers/media/platform/vsp1/vsp1_pipe.h
> @@ -20,6 +20,9 @@
> 
>  #include <media/media-entity.h>
> 
> +/* Max Video Width / Min Partition Size = 8190/128 */
> +#define VSP1_PIPE_MAX_PARTITIONS 64
> +
>  struct vsp1_dl_list;
>  struct vsp1_rwpf;
> 
> @@ -81,7 +84,9 @@ enum vsp1_pipeline_state {
>   * @dl: display list associated with the pipeline
>   * @div_size: The maximum allowed partition size for the pipeline
>   * @partitions: The number of partitions used to process one frame
> + * @partition: The current partition for configuration to process
>   * @current_partition: The partition number currently being configured
> + * @part_table: The pre-calculated partitions used by the pipeline
>   */
>  struct vsp1_pipeline {
>  	struct media_pipeline pipe;
> @@ -116,6 +121,7 @@ struct vsp1_pipeline {
>  	unsigned int partitions;
>  	struct v4l2_rect partition;
>  	unsigned int current_partition;
> +	struct v4l2_rect part_table[VSP1_PIPE_MAX_PARTITIONS];

That's an extra 1kB or kmalloc'ed data. I'd prefer allocating it dynamically 
as needed.

>  };
> 
>  void vsp1_pipeline_reset(struct vsp1_pipeline *pipe);
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
> b/drivers/media/platform/vsp1/vsp1_video.c index 6d43c02bbc56..c4a8c30df108
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -255,6 +255,7 @@ static void vsp1_video_pipeline_setup_partitions(struct
> vsp1_pipeline *pipe) const struct v4l2_mbus_framefmt *format;
>  	struct vsp1_entity *entity;
>  	unsigned int div_size;
> +	int i;

i can never be negative, you can make it an unsigned int.

Apart from that,

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

>  	/*
>  	 * Partitions are computed on the size before rotation, use the format
> @@ -269,6 +270,7 @@ static void vsp1_video_pipeline_setup_partitions(struct
> vsp1_pipeline *pipe) if (vsp1->info->gen == 2) {
>  		pipe->div_size = div_size;
>  		pipe->partitions = 1;
> +		pipe->part_table[0] = vsp1_video_partition(pipe, div_size, 0);
>  		return;
>  	}
> 
> @@ -284,6 +286,9 @@ static void vsp1_video_pipeline_setup_partitions(struct
> vsp1_pipeline *pipe)
> 
>  	pipe->div_size = div_size;
>  	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);
>  }
> 
>  /* ------------------------------------------------------------------------
> @@ -355,8 +360,7 @@ static void vsp1_video_pipeline_run_partition(struct
> vsp1_pipeline *pipe, {
>  	struct vsp1_entity *entity;
> 
> -	pipe->partition = vsp1_video_partition(pipe, pipe->div_size,
> -					       pipe->current_partition);
> +	pipe->partition = pipe->part_table[pipe->current_partition];
> 
>  	list_for_each_entry(entity, &pipe->entities, list_pipe) {
>  		if (entity->ops->configure)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/4] v4l: vsp1: Calculate partition sizes at stream start.

[Kieran Bingham]

Hi Laurent,

Thanks for the review,

V2 to be posted after testing.

On 13/02/17 21:21, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Friday 04 Nov 2016 18:19:29 Kieran Bingham wrote:
>> Previously the active window and partition sizes for each partition is
> 
> s/is/were/

Fixed.

> 
>> calculated for each partition every frame. This data is constant and
>> only needs to be calculated once at the start of the stream.
>>
>> Extend the vsp1_pipe object to store the maximum number of partitions
>> possible and pre-calculate the partition sizes into this table.
>>
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>> ---
>>  drivers/media/platform/vsp1/vsp1_pipe.h  | 6 ++++++
>>  drivers/media/platform/vsp1/vsp1_video.c | 8 ++++++--
>>  2 files changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h
>> b/drivers/media/platform/vsp1/vsp1_pipe.h index f181949824c9..3af96c4ea244
>> 100644
>> --- a/drivers/media/platform/vsp1/vsp1_pipe.h
>> +++ b/drivers/media/platform/vsp1/vsp1_pipe.h
>> @@ -20,6 +20,9 @@
>>
>>  #include <media/media-entity.h>
>>
>> +/* Max Video Width / Min Partition Size = 8190/128 */
>> +#define VSP1_PIPE_MAX_PARTITIONS 64
>> +

^ This define is removed with dynamic allocations.

>>  struct vsp1_dl_list;
>>  struct vsp1_rwpf;
>>
>> @@ -81,7 +84,9 @@ enum vsp1_pipeline_state {
>>   * @dl: display list associated with the pipeline
>>   * @div_size: The maximum allowed partition size for the pipeline
>>   * @partitions: The number of partitions used to process one frame
>> + * @partition: The current partition for configuration to process
>>   * @current_partition: The partition number currently being configured
>> + * @part_table: The pre-calculated partitions used by the pipeline
>>   */
>>  struct vsp1_pipeline {
>>  	struct media_pipeline pipe;
>> @@ -116,6 +121,7 @@ struct vsp1_pipeline {
>>  	unsigned int partitions;
>>  	struct v4l2_rect partition;
>>  	unsigned int current_partition;
>> +	struct v4l2_rect part_table[VSP1_PIPE_MAX_PARTITIONS];
> 
> That's an extra 1kB or kmalloc'ed data. I'd prefer allocating it dynamically 
> as needed.

Ok, allocating with kcalloc in vsp1_video_pipeline_setup_partitions() (The
earliest opportunity to know how many partitions are needed)

Free'd in vsp1_video_stop_streaming()

>>  };
>>
>>  void vsp1_pipeline_reset(struct vsp1_pipeline *pipe);
>> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
>> b/drivers/media/platform/vsp1/vsp1_video.c index 6d43c02bbc56..c4a8c30df108
>> 100644
>> --- a/drivers/media/platform/vsp1/vsp1_video.c
>> +++ b/drivers/media/platform/vsp1/vsp1_video.c
>> @@ -255,6 +255,7 @@ static void vsp1_video_pipeline_setup_partitions(struct
>> vsp1_pipeline *pipe) const struct v4l2_mbus_framefmt *format;
>>  	struct vsp1_entity *entity;
>>  	unsigned int div_size;
>> +	int i;
> 
> i can never be negative, you can make it an unsigned int.

Fixed.

> Apart from that,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thanks, I'll tentatively add this tag, but please check you are happy with the
dynamic allocation on the repost v2 :-) !

> 
>>  	/*
>>  	 * Partitions are computed on the size before rotation, use the format
>> @@ -269,6 +270,7 @@ static void vsp1_video_pipeline_setup_partitions(struct
>> vsp1_pipeline *pipe) if (vsp1->info->gen == 2) {
>>  		pipe->div_size = div_size;
>>  		pipe->partitions = 1;
>> +		pipe->part_table[0] = vsp1_video_partition(pipe, div_size, 0);
>>  		return;
>>  	}
>>
>> @@ -284,6 +286,9 @@ static void vsp1_video_pipeline_setup_partitions(struct
>> vsp1_pipeline *pipe)
>>
>>  	pipe->div_size = div_size;
>>  	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);
>>  }
>>
>>  /* ------------------------------------------------------------------------
>> @@ -355,8 +360,7 @@ static void vsp1_video_pipeline_run_partition(struct
>> vsp1_pipeline *pipe, {
>>  	struct vsp1_entity *entity;
>>
>> -	pipe->partition = vsp1_video_partition(pipe, pipe->div_size,
>> -					       pipe->current_partition);
>> +	pipe->partition = pipe->part_table[pipe->current_partition];
>>
>>  	list_for_each_entry(entity, &pipe->entities, list_pipe) {
>>  		if (entity->ops->configure)
> 

[PATCH 4/4] v4l: vsp1: Remove redundant context variables

[Kieran Bingham]

The vsp1_pipe object context variables for div_size and
current_partition allowed state to be maintained through processing the
partitions during processing.

Now that the partition tables are calculated during stream on, there is
no requirement to store these variables in the pipe object.

Utilise local variables for the processing as required.

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

diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h b/drivers/media/platform/vsp1/vsp1_pipe.h
index 3af96c4ea244..9e108ddcceb6 100644
--- a/drivers/media/platform/vsp1/vsp1_pipe.h
+++ b/drivers/media/platform/vsp1/vsp1_pipe.h
@@ -82,10 +82,8 @@ enum vsp1_pipeline_state {
  * @uds_input: entity at the input of the UDS, if the UDS is present
  * @entities: list of entities in the pipeline
  * @dl: display list associated with the pipeline
- * @div_size: The maximum allowed partition size for the pipeline
  * @partitions: The number of partitions used to process one frame
  * @partition: The current partition for configuration to process
- * @current_partition: The partition number currently being configured
  * @part_table: The pre-calculated partitions used by the pipeline
  */
 struct vsp1_pipeline {
@@ -117,10 +115,8 @@ struct vsp1_pipeline {
 
 	struct vsp1_dl_list *dl;
 
-	unsigned int div_size;
 	unsigned int partitions;
 	struct v4l2_rect partition;
-	unsigned int current_partition;
 	struct v4l2_rect part_table[VSP1_PIPE_MAX_PARTITIONS];
 };
 
diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index c4a8c30df108..9efaab2cc982 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -268,7 +268,6 @@ static void vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe)
 
 	/* Gen2 hardware doesn't require image partitioning. */
 	if (vsp1->info->gen == 2) {
-		pipe->div_size = div_size;
 		pipe->partitions = 1;
 		pipe->part_table[0] = vsp1_video_partition(pipe, div_size, 0);
 		return;
@@ -284,7 +283,6 @@ static void vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe)
 		}
 	}
 
-	pipe->div_size = div_size;
 	pipe->partitions = DIV_ROUND_UP(format->width, div_size);
 
 	for (i = 0; i < pipe->partitions; i++)
@@ -356,11 +354,12 @@ static void vsp1_video_frame_end(struct vsp1_pipeline *pipe,
 }
 
 static void vsp1_video_pipeline_run_partition(struct vsp1_pipeline *pipe,
-					      struct vsp1_dl_list *dl)
+					      struct vsp1_dl_list *dl,
+					      unsigned int partition_number)
 {
 	struct vsp1_entity *entity;
 
-	pipe->partition = pipe->part_table[pipe->current_partition];
+	pipe->partition = pipe->part_table[partition_number];
 
 	list_for_each_entry(entity, &pipe->entities, list_pipe) {
 		if (entity->ops->configure)
@@ -373,6 +372,7 @@ static void vsp1_video_pipeline_run(struct vsp1_pipeline *pipe)
 {
 	struct vsp1_device *vsp1 = pipe->output->entity.vsp1;
 	struct vsp1_entity *entity;
+	unsigned int current_partition = 0;
 
 	if (!pipe->dl)
 		pipe->dl = vsp1_dl_list_get(pipe->output->dlm);
@@ -389,13 +389,12 @@ static void vsp1_video_pipeline_run(struct vsp1_pipeline *pipe)
 	}
 
 	/* Run the first partition */
-	pipe->current_partition = 0;
-	vsp1_video_pipeline_run_partition(pipe, pipe->dl);
+	vsp1_video_pipeline_run_partition(pipe, pipe->dl, current_partition);
 
 	/* Process consecutive partitions as necessary */
-	for (pipe->current_partition = 1;
-	     pipe->current_partition < pipe->partitions;
-	     pipe->current_partition++) {
+	for (current_partition = 1;
+	     current_partition < pipe->partitions;
+	     current_partition++) {
 		struct vsp1_dl_list *dl;
 
 		/*
@@ -415,7 +414,7 @@ static void vsp1_video_pipeline_run(struct vsp1_pipeline *pipe)
 			break;
 		}
 
-		vsp1_video_pipeline_run_partition(pipe, dl);
+		vsp1_video_pipeline_run_partition(pipe, dl, current_partition);
 		vsp1_dl_list_add_chain(pipe->dl, dl);
 	}
 
-- 
2.7.4

Re: [PATCH 4/4] v4l: vsp1: Remove redundant context variables

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Friday 04 Nov 2016 18:19:30 Kieran Bingham wrote:
> The vsp1_pipe object context variables for div_size and
> current_partition allowed state to be maintained through processing the
> partitions during processing.
> 
> Now that the partition tables are calculated during stream on, there is
> no requirement to store these variables in the pipe object.
> 
> Utilise local variables for the processing as required.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/vsp1/vsp1_pipe.h  |  4 ----
>  drivers/media/platform/vsp1/vsp1_video.c | 19 +++++++++----------
>  2 files changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_pipe.h
> b/drivers/media/platform/vsp1/vsp1_pipe.h index 3af96c4ea244..9e108ddcceb6
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_pipe.h
> +++ b/drivers/media/platform/vsp1/vsp1_pipe.h
> @@ -82,10 +82,8 @@ enum vsp1_pipeline_state {
>   * @uds_input: entity at the input of the UDS, if the UDS is present
>   * @entities: list of entities in the pipeline
>   * @dl: display list associated with the pipeline
> - * @div_size: The maximum allowed partition size for the pipeline
>   * @partitions: The number of partitions used to process one frame
>   * @partition: The current partition for configuration to process
> - * @current_partition: The partition number currently being configured
>   * @part_table: The pre-calculated partitions used by the pipeline
>   */
>  struct vsp1_pipeline {
> @@ -117,10 +115,8 @@ struct vsp1_pipeline {
> 
>  	struct vsp1_dl_list *dl;
> 
> -	unsigned int div_size;
>  	unsigned int partitions;
>  	struct v4l2_rect partition;
> -	unsigned int current_partition;
>  	struct v4l2_rect part_table[VSP1_PIPE_MAX_PARTITIONS];
>  };
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
> b/drivers/media/platform/vsp1/vsp1_video.c index c4a8c30df108..9efaab2cc982
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -268,7 +268,6 @@ static void vsp1_video_pipeline_setup_partitions(struct
> vsp1_pipeline *pipe)
> 
>  	/* Gen2 hardware doesn't require image partitioning. */
>  	if (vsp1->info->gen == 2) {
> -		pipe->div_size = div_size;
>  		pipe->partitions = 1;
>  		pipe->part_table[0] = vsp1_video_partition(pipe, div_size, 0);
>  		return;
> @@ -284,7 +283,6 @@ static void vsp1_video_pipeline_setup_partitions(struct
> vsp1_pipeline *pipe) }
>  	}
> 
> -	pipe->div_size = div_size;
>  	pipe->partitions = DIV_ROUND_UP(format->width, div_size);
> 
>  	for (i = 0; i < pipe->partitions; i++)
> @@ -356,11 +354,12 @@ static void vsp1_video_frame_end(struct vsp1_pipeline
> *pipe, }
> 
>  static void vsp1_video_pipeline_run_partition(struct vsp1_pipeline *pipe,
> -					      struct vsp1_dl_list *dl)
> +					      struct vsp1_dl_list *dl,
> +					      unsigned int partition_number)
>  {
>  	struct vsp1_entity *entity;
> 
> -	pipe->partition = pipe->part_table[pipe->current_partition];
> +	pipe->partition = pipe->part_table[partition_number];
> 
>  	list_for_each_entry(entity, &pipe->entities, list_pipe) {
>  		if (entity->ops->configure)
> @@ -373,6 +372,7 @@ static void vsp1_video_pipeline_run(struct vsp1_pipeline
> *pipe) {
>  	struct vsp1_device *vsp1 = pipe->output->entity.vsp1;
>  	struct vsp1_entity *entity;
> +	unsigned int current_partition = 0;

I would call thus partition, current_partition is a bit long and doesn't carry 
much useful extra information in my opinion. There's also no need to 
initialize the variable to 0 if you hardcode the 0 value in the first call to 
vsp1_video_pipeline_run_partition(), as the for loop then takes care of 
initializing the variable to 1.

Apart from that,

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

> 
>  	if (!pipe->dl)
>  		pipe->dl = vsp1_dl_list_get(pipe->output->dlm);
> @@ -389,13 +389,12 @@ static void vsp1_video_pipeline_run(struct
> vsp1_pipeline *pipe) }
> 
>  	/* Run the first partition */
> -	pipe->current_partition = 0;
> -	vsp1_video_pipeline_run_partition(pipe, pipe->dl);
> +	vsp1_video_pipeline_run_partition(pipe, pipe->dl, current_partition);
> 
>  	/* Process consecutive partitions as necessary */
> -	for (pipe->current_partition = 1;
> -	     pipe->current_partition < pipe->partitions;
> -	     pipe->current_partition++) {
> +	for (current_partition = 1;
> +	     current_partition < pipe->partitions;
> +	     current_partition++) {
>  		struct vsp1_dl_list *dl;
> 
>  		/*
> @@ -415,7 +414,7 @@ static void vsp1_video_pipeline_run(struct vsp1_pipeline
> *pipe) break;
>  		}
> 
> -		vsp1_video_pipeline_run_partition(pipe, dl);
> +		vsp1_video_pipeline_run_partition(pipe, dl, 
current_partition);
>  		vsp1_dl_list_add_chain(pipe->dl, dl);
>  	}

-- 
Regards,

Laurent Pinchart