[PATCH 0/6] VSP1 Updates

[PATCH 0/6] VSP1 Updates

[Kieran Bingham]

An assorted selection of fixes and updates for the VSP1 to improve code
quality and remove incorrect limitations.

The SRU and UDS are capable of larger partitions, as part of the
partition algorithm - but we only document this support for now, as
updating it should be done in consideration with the overlap and phase
corrections necessary.

Kieran Bingham (6):
  media: vsp1: Remove artificial pixel limitation
  media: vsp1: Correct the pitch on multiplanar formats
  media: vsp1: Implement partition algorithm restrictions
  media: vsp1: Document max_width restriction on SRU
  media: vsp1: Document max_width restriction on UDS
  media: vsp1: use periods at the end of comment sentences

 drivers/media/platform/vsp1/vsp1_brx.c    |  4 +--
 drivers/media/platform/vsp1/vsp1_drm.c    | 10 ++++++
 drivers/media/platform/vsp1/vsp1_drv.c    |  6 ++--
 drivers/media/platform/vsp1/vsp1_entity.c |  2 +-
 drivers/media/platform/vsp1/vsp1_rpf.c    |  4 +--
 drivers/media/platform/vsp1/vsp1_sru.c    | 13 ++++++--
 drivers/media/platform/vsp1/vsp1_sru.h    |  1 +
 drivers/media/platform/vsp1/vsp1_uds.c    | 14 +++++++--
 drivers/media/platform/vsp1/vsp1_video.c  | 38 ++++++++++++++++++-----
 drivers/media/platform/vsp1/vsp1_wpf.c    |  2 +-
 include/media/vsp1.h                      |  2 +-
 11 files changed, 73 insertions(+), 23 deletions(-)

-- 
2.17.1

[PATCH 1/6] media: vsp1: Remove artificial pixel limitation

[Kieran Bingham]

The VSP1 has a minimum width and height of a single pixel, with the
exception of pixel formats with sub-sampling.

Remove the artificial minimum width and minimum height limitation, and
instead clamp the minimum dimensions based upon the sub-sampling
parameter of that dimension.

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

diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index 81d47a09d7bc..e78eadd0295b 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -38,9 +38,7 @@
 #define VSP1_VIDEO_DEF_WIDTH		1024
 #define VSP1_VIDEO_DEF_HEIGHT		768
 
-#define VSP1_VIDEO_MIN_WIDTH		2U
 #define VSP1_VIDEO_MAX_WIDTH		8190U
-#define VSP1_VIDEO_MIN_HEIGHT		2U
 #define VSP1_VIDEO_MAX_HEIGHT		8190U
 
 /* -----------------------------------------------------------------------------
@@ -136,9 +134,8 @@ static int __vsp1_video_try_format(struct vsp1_video *video,
 	height = round_down(height, info->vsub);
 
 	/* Clamp the width and height. */
-	pix->width = clamp(width, VSP1_VIDEO_MIN_WIDTH, VSP1_VIDEO_MAX_WIDTH);
-	pix->height = clamp(height, VSP1_VIDEO_MIN_HEIGHT,
-			    VSP1_VIDEO_MAX_HEIGHT);
+	pix->width = clamp(width, info->hsub, VSP1_VIDEO_MAX_WIDTH);
+	pix->height = clamp(height, info->vsub, VSP1_VIDEO_MAX_HEIGHT);
 
 	/*
 	 * Compute and clamp the stride and image size. While not documented in
-- 
2.17.1

[PATCH 2/6] media: vsp1: Correct the pitch on multiplanar formats

[Kieran Bingham]

DRM pipelines now support tri-planar as well as packed formats with
YCbCr, however the pitch calculation was not updated to support this.

Correct this by adjusting the bytesperline accordingly when 3 planes are
used.

Fixes: 7863ac504bc5 ("drm: rcar-du: Add tri-planar memory formats support")
Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_drm.c | 10 ++++++++++
 include/media/vsp1.h                   |  2 +-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/vsp1/vsp1_drm.c b/drivers/media/platform/vsp1/vsp1_drm.c
index b9c0f695d002..b9afd98f6867 100644
--- a/drivers/media/platform/vsp1/vsp1_drm.c
+++ b/drivers/media/platform/vsp1/vsp1_drm.c
@@ -814,6 +814,16 @@ int vsp1_du_atomic_update(struct device *dev, unsigned int pipe_index,
 	rpf->format.num_planes = fmtinfo->planes;
 	rpf->format.plane_fmt[0].bytesperline = cfg->pitch;
 	rpf->format.plane_fmt[1].bytesperline = cfg->pitch;
+
+	/*
+	 * Packed YUV formats are subsampled, but the packing of two components
+	 * into a single plane compensates for this leaving the bytesperline
+	 * to be the correct value. For multiplanar formats we must adjust the
+	 * pitch accordingly.
+	 */
+	if (fmtinfo->planes == 3)
+		rpf->format.plane_fmt[1].bytesperline /= fmtinfo->hsub;
+
 	rpf->alpha = cfg->alpha;
 
 	rpf->mem.addr[0] = cfg->mem[0];
diff --git a/include/media/vsp1.h b/include/media/vsp1.h
index 3093b9cb9067..0ce19b595cc7 100644
--- a/include/media/vsp1.h
+++ b/include/media/vsp1.h
@@ -46,7 +46,7 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int pipe_index,
 /**
  * struct vsp1_du_atomic_config - VSP atomic configuration parameters
  * @pixelformat: plane pixel format (V4L2 4CC)
- * @pitch: line pitch in bytes, for all planes
+ * @pitch: line pitch in bytes
  * @mem: DMA memory address for each plane of the frame buffer
  * @src: source rectangle in the frame buffer (integer coordinates)
  * @dst: destination rectangle on the display (integer coordinates)
-- 
2.17.1

[PATCH 3/6] media: 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 after the UDS, it can only do so in identity mode.

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 04e4e05af6ae..f277700e5cc2 100644
--- a/drivers/media/platform/vsp1/vsp1_sru.c
+++ b/drivers/media/platform/vsp1/vsp1_sru.c
@@ -149,7 +149,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 {
@@ -201,7 +202,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 {
@@ -374,6 +376,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 ddb00eadd1ea..28da98d86366 100644
--- a/drivers/media/platform/vsp1/vsp1_sru.h
+++ b/drivers/media/platform/vsp1/vsp1_sru.h
@@ -26,6 +26,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 e78eadd0295b..9404d7968371 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -31,6 +31,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"
 
@@ -480,10 +481,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;
 	struct vsp1_brx *brx = NULL;
+	bool sru_found = false;
 	int ret;
 
 	ret = media_entity_enum_init(&ent_enum, &input->entity.vsp1->media_dev);
@@ -540,13 +543,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 = brx ? &brx->entity : &input->entity;
 		}
-- 
2.17.1

[PATCH 4/6] media: vsp1: Document max_width restriction on SRU

[Kieran Bingham]

The SRU is currently restricted to 256 pixels as part of the current
partition algorithm. Document that the actual capability of this
component is 288 pixels, but don't increase the implementation.

The extended partition algorithm may later choose to utilise a larger
input to support overlapping partitions.

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

diff --git a/drivers/media/platform/vsp1/vsp1_sru.c b/drivers/media/platform/vsp1/vsp1_sru.c
index f277700e5cc2..2a40e09b9aa7 100644
--- a/drivers/media/platform/vsp1/vsp1_sru.c
+++ b/drivers/media/platform/vsp1/vsp1_sru.c
@@ -314,6 +314,10 @@ static unsigned int sru_max_width(struct vsp1_entity *entity,
 	output = vsp1_entity_get_pad_format(&sru->entity, sru->entity.config,
 					    SRU_PAD_SOURCE);
 
+	/*
+	 * The maximum width of the SRU is 288 input pixels, but to support the
+	 * partition algorithm, this is currently kept at 256.
+	 */
 	if (input->width != output->width)
 		return 512;
 	else
-- 
2.17.1

[PATCH 5/6] media: vsp1: Document max_width restriction on UDS

[Kieran Bingham]

The UDS is currently restricted based on a partition size of 256 pixels.
Document the actual restrictions, but don't increase the implementation.

The extended partition algorithm may later choose to utilise a larger
partition size to support overlapping partitions.

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

diff --git a/drivers/media/platform/vsp1/vsp1_uds.c b/drivers/media/platform/vsp1/vsp1_uds.c
index c20c84b54936..7c11651db5d4 100644
--- a/drivers/media/platform/vsp1/vsp1_uds.c
+++ b/drivers/media/platform/vsp1/vsp1_uds.c
@@ -342,6 +342,14 @@ static unsigned int uds_max_width(struct vsp1_entity *entity,
 					    UDS_PAD_SOURCE);
 	hscale = output->width / input->width;
 
+	/*
+	 * The maximum width of the UDS is 304 pixels. These are input pixels
+	 * in the event of up-scaling, and output pixels in the event of
+	 * downscaling.
+	 *
+	 * To support the current parition algorithm, we clamp at units of 256.
+	 */
+
 	if (hscale <= 2)
 		return 256;
 	else if (hscale <= 4)
-- 
2.17.1

[PATCH 6/6] media: vsp1: use periods at the end of comment sentences

[Kieran Bingham]

The style of this driver uses periods at the end of sentences in
comments, but it is applied inconsitently.

Update a selection of comments which were discovered to be missing their
period. Also fix the spelling of one usage of 'instantiate'

Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
---
 drivers/media/platform/vsp1/vsp1_brx.c    | 4 ++--
 drivers/media/platform/vsp1/vsp1_drv.c    | 6 +++---
 drivers/media/platform/vsp1/vsp1_entity.c | 2 +-
 drivers/media/platform/vsp1/vsp1_rpf.c    | 4 ++--
 drivers/media/platform/vsp1/vsp1_sru.c    | 2 +-
 drivers/media/platform/vsp1/vsp1_uds.c    | 6 +++---
 drivers/media/platform/vsp1/vsp1_video.c  | 2 +-
 drivers/media/platform/vsp1/vsp1_wpf.c    | 2 +-
 8 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/media/platform/vsp1/vsp1_brx.c b/drivers/media/platform/vsp1/vsp1_brx.c
index 359917b5d842..5e50178b057d 100644
--- a/drivers/media/platform/vsp1/vsp1_brx.c
+++ b/drivers/media/platform/vsp1/vsp1_brx.c
@@ -153,7 +153,7 @@ static int brx_set_format(struct v4l2_subdev *subdev,
 	format = vsp1_entity_get_pad_format(&brx->entity, config, fmt->pad);
 	*format = fmt->format;
 
-	/* Reset the compose rectangle */
+	/* Reset the compose rectangle. */
 	if (fmt->pad != brx->entity.source_pad) {
 		struct v4l2_rect *compose;
 
@@ -164,7 +164,7 @@ static int brx_set_format(struct v4l2_subdev *subdev,
 		compose->height = format->height;
 	}
 
-	/* Propagate the format code to all pads */
+	/* Propagate the format code to all pads. */
 	if (fmt->pad == BRX_PAD_SINK(0)) {
 		unsigned int i;
 
diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c
index b6619c9c18bb..249963cb2ec0 100644
--- a/drivers/media/platform/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/vsp1/vsp1_drv.c
@@ -802,7 +802,7 @@ static int vsp1_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, vsp1);
 
-	/* I/O and IRQ resources (clock managed by the clock PM domain) */
+	/* I/O and IRQ resources (clock managed by the clock PM domain). */
 	io = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	vsp1->mmio = devm_ioremap_resource(&pdev->dev, io);
 	if (IS_ERR(vsp1->mmio))
@@ -821,7 +821,7 @@ static int vsp1_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	/* FCP (optional) */
+	/* FCP (optional). */
 	fcp_node = of_parse_phandle(pdev->dev.of_node, "renesas,fcp", 0);
 	if (fcp_node) {
 		vsp1->fcp = rcar_fcp_get(fcp_node);
@@ -869,7 +869,7 @@ static int vsp1_probe(struct platform_device *pdev)
 
 	dev_dbg(&pdev->dev, "IP version 0x%08x\n", vsp1->version);
 
-	/* Instanciate entities */
+	/* Instantiate entities. */
 	ret = vsp1_create_entities(vsp1);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to create entities\n");
diff --git a/drivers/media/platform/vsp1/vsp1_entity.c b/drivers/media/platform/vsp1/vsp1_entity.c
index 36a29e13109e..a54ab528b060 100644
--- a/drivers/media/platform/vsp1/vsp1_entity.c
+++ b/drivers/media/platform/vsp1/vsp1_entity.c
@@ -404,7 +404,7 @@ int vsp1_subdev_set_pad_format(struct v4l2_subdev *subdev,
 	format = vsp1_entity_get_pad_format(entity, config, entity->source_pad);
 	*format = fmt->format;
 
-	/* Reset the crop and compose rectangles */
+	/* Reset the crop and compose rectangles. */
 	selection = vsp1_entity_get_pad_selection(entity, config, fmt->pad,
 						  V4L2_SEL_TGT_CROP);
 	selection->left = 0;
diff --git a/drivers/media/platform/vsp1/vsp1_rpf.c b/drivers/media/platform/vsp1/vsp1_rpf.c
index f8005b60b9d2..616afa7e165f 100644
--- a/drivers/media/platform/vsp1/vsp1_rpf.c
+++ b/drivers/media/platform/vsp1/vsp1_rpf.c
@@ -108,7 +108,7 @@ static void rpf_configure_stream(struct vsp1_entity *entity,
 	vsp1_rpf_write(rpf, dlb, VI6_RPF_INFMT, infmt);
 	vsp1_rpf_write(rpf, dlb, VI6_RPF_DSWAP, fmtinfo->swap);
 
-	/* Output location */
+	/* Output location. */
 	if (pipe->brx) {
 		const struct v4l2_rect *compose;
 
@@ -309,7 +309,7 @@ static void rpf_configure_partition(struct vsp1_entity *entity,
 
 	/*
 	 * Interlaced pipelines will use the extended pre-cmd to process
-	 * SRCM_ADDR_{Y,C0,C1}
+	 * SRCM_ADDR_{Y,C0,C1}.
 	 */
 	if (pipe->interlaced) {
 		vsp1_rpf_configure_autofld(rpf, dl);
diff --git a/drivers/media/platform/vsp1/vsp1_sru.c b/drivers/media/platform/vsp1/vsp1_sru.c
index 2a40e09b9aa7..f48b085b5b5e 100644
--- a/drivers/media/platform/vsp1/vsp1_sru.c
+++ b/drivers/media/platform/vsp1/vsp1_sru.c
@@ -339,7 +339,7 @@ static void sru_partition(struct vsp1_entity *entity,
 	output = vsp1_entity_get_pad_format(&sru->entity, sru->entity.config,
 					    SRU_PAD_SOURCE);
 
-	/* Adapt if SRUx2 is enabled */
+	/* Adapt if SRUx2 is enabled. */
 	if (input->width != output->width) {
 		window->width /= 2;
 		window->left /= 2;
diff --git a/drivers/media/platform/vsp1/vsp1_uds.c b/drivers/media/platform/vsp1/vsp1_uds.c
index 7c11651db5d4..c704bdaf4edc 100644
--- a/drivers/media/platform/vsp1/vsp1_uds.c
+++ b/drivers/media/platform/vsp1/vsp1_uds.c
@@ -314,13 +314,13 @@ static void uds_configure_partition(struct vsp1_entity *entity,
 	output = vsp1_entity_get_pad_format(&uds->entity, uds->entity.config,
 					    UDS_PAD_SOURCE);
 
-	/* Input size clipping */
+	/* Input size clipping. */
 	vsp1_uds_write(uds, dlb, VI6_UDS_HSZCLIP, VI6_UDS_HSZCLIP_HCEN |
 		       (0 << VI6_UDS_HSZCLIP_HCL_OFST_SHIFT) |
 		       (partition->uds_sink.width
 				<< VI6_UDS_HSZCLIP_HCL_SIZE_SHIFT));
 
-	/* Output size clipping */
+	/* Output size clipping. */
 	vsp1_uds_write(uds, dlb, VI6_UDS_CLIP_SIZE,
 		       (partition->uds_source.width
 				<< VI6_UDS_CLIP_SIZE_HSIZE_SHIFT) |
@@ -374,7 +374,7 @@ static void uds_partition(struct vsp1_entity *entity,
 	const struct v4l2_mbus_framefmt *output;
 	const struct v4l2_mbus_framefmt *input;
 
-	/* Initialise the partition state */
+	/* Initialise the partition state. */
 	partition->uds_sink = *window;
 	partition->uds_source = *window;
 
diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
index 9404d7968371..c114cc4d2349 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -891,7 +891,7 @@ static void vsp1_video_cleanup_pipeline(struct vsp1_pipeline *pipe)
 	pipe->stream_config = NULL;
 	pipe->configured = false;
 
-	/* Release our partition table allocation */
+	/* Release our partition table allocation. */
 	kfree(pipe->part_table);
 	pipe->part_table = NULL;
 }
diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c b/drivers/media/platform/vsp1/vsp1_wpf.c
index c2a1a7f97e26..32bb207b2007 100644
--- a/drivers/media/platform/vsp1/vsp1_wpf.c
+++ b/drivers/media/platform/vsp1/vsp1_wpf.c
@@ -317,7 +317,7 @@ static void wpf_configure_stream(struct vsp1_entity *entity,
 
 	vsp1_wpf_write(wpf, dlb, VI6_WPF_SRCRPF, srcrpf);
 
-	/* Enable interrupts */
+	/* Enable interrupts. */
 	vsp1_dl_body_write(dlb, VI6_WPF_IRQ_STA(wpf->entity.index), 0);
 	vsp1_dl_body_write(dlb, VI6_WPF_IRQ_ENB(wpf->entity.index),
 			   VI6_WFP_IRQ_ENB_DFEE);
-- 
2.17.1

Re: [PATCH 1/6] media: vsp1: Remove artificial pixel limitation

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Friday, 31 August 2018 17:40:39 EEST Kieran Bingham wrote:
> The VSP1 has a minimum width and height of a single pixel, with the
> exception of pixel formats with sub-sampling.
> 
> Remove the artificial minimum width and minimum height limitation, and
> instead clamp the minimum dimensions based upon the sub-sampling
> parameter of that dimension.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

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

and applied to my tree.

> ---
>  drivers/media/platform/vsp1/vsp1_video.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
> b/drivers/media/platform/vsp1/vsp1_video.c index 81d47a09d7bc..e78eadd0295b
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -38,9 +38,7 @@
>  #define VSP1_VIDEO_DEF_WIDTH		1024
>  #define VSP1_VIDEO_DEF_HEIGHT		768
> 
> -#define VSP1_VIDEO_MIN_WIDTH		2U
>  #define VSP1_VIDEO_MAX_WIDTH		8190U
> -#define VSP1_VIDEO_MIN_HEIGHT		2U
>  #define VSP1_VIDEO_MAX_HEIGHT		8190U
> 
>  /*
> ---------------------------------------------------------------------------
> -- @@ -136,9 +134,8 @@ static int __vsp1_video_try_format(struct vsp1_video
> *video, height = round_down(height, info->vsub);
> 
>  	/* Clamp the width and height. */
> -	pix->width = clamp(width, VSP1_VIDEO_MIN_WIDTH, VSP1_VIDEO_MAX_WIDTH);
> -	pix->height = clamp(height, VSP1_VIDEO_MIN_HEIGHT,
> -			    VSP1_VIDEO_MAX_HEIGHT);
> +	pix->width = clamp(width, info->hsub, VSP1_VIDEO_MAX_WIDTH);
> +	pix->height = clamp(height, info->vsub, VSP1_VIDEO_MAX_HEIGHT);
> 
>  	/*
>  	 * Compute and clamp the stride and image size. While not documented in

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/6] media: vsp1: Correct the pitch on multiplanar formats

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Friday, 31 August 2018 17:40:40 EEST Kieran Bingham wrote:
> DRM pipelines now support tri-planar as well as packed formats with
> YCbCr, however the pitch calculation was not updated to support this.
> 
> Correct this by adjusting the bytesperline accordingly when 3 planes are
> used.
> 
> Fixes: 7863ac504bc5 ("drm: rcar-du: Add tri-planar memory formats support")
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

I already have a similar patch from Matsuoka-san in my tree, please see 
https://patchwork.kernel.org/patch/10425565/. I'll update it with the fixes 
tag.

> ---
>  drivers/media/platform/vsp1/vsp1_drm.c | 10 ++++++++++
>  include/media/vsp1.h                   |  2 +-
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
> b/drivers/media/platform/vsp1/vsp1_drm.c index b9c0f695d002..b9afd98f6867
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_drm.c
> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
> @@ -814,6 +814,16 @@ int vsp1_du_atomic_update(struct device *dev, unsigned
> int pipe_index, rpf->format.num_planes = fmtinfo->planes;
>  	rpf->format.plane_fmt[0].bytesperline = cfg->pitch;
>  	rpf->format.plane_fmt[1].bytesperline = cfg->pitch;
> +
> +	/*
> +	 * Packed YUV formats are subsampled, but the packing of two components
> +	 * into a single plane compensates for this leaving the bytesperline
> +	 * to be the correct value. For multiplanar formats we must adjust the
> +	 * pitch accordingly.
> +	 */
> +	if (fmtinfo->planes == 3)
> +		rpf->format.plane_fmt[1].bytesperline /= fmtinfo->hsub;
> +
>  	rpf->alpha = cfg->alpha;
> 
>  	rpf->mem.addr[0] = cfg->mem[0];
> diff --git a/include/media/vsp1.h b/include/media/vsp1.h
> index 3093b9cb9067..0ce19b595cc7 100644
> --- a/include/media/vsp1.h
> +++ b/include/media/vsp1.h
> @@ -46,7 +46,7 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int
> pipe_index, /**
>   * struct vsp1_du_atomic_config - VSP atomic configuration parameters
>   * @pixelformat: plane pixel format (V4L2 4CC)
> - * @pitch: line pitch in bytes, for all planes
> + * @pitch: line pitch in bytes

Should I update the above-mentioned patch with this as well ? How about 
phrasing it as "line pitch in bytes for the first plane" ?

>   * @mem: DMA memory address for each plane of the frame buffer
>   * @src: source rectangle in the frame buffer (integer coordinates)
>   * @dst: destination rectangle on the display (integer coordinates)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/6] media: vsp1: Implement partition algorithm restrictions

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Friday, 31 August 2018 17:40:41 EEST Kieran Bingham wrote:
> The partition algorithm introduced to support scaling, and rotation on

s/,//

> 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 after the UDS, it can only do so in identity mode.
> 
> 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 04e4e05af6ae..f277700e5cc2
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_sru.c
> +++ b/drivers/media/platform/vsp1/vsp1_sru.c
> @@ -149,7 +149,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 {
> @@ -201,7 +202,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 {

What if the format is configured first while the SRU isn't connected after a 
UDS, the pipeline then reconfigured to add the UDS, and streaming started ? 
Furthermore the force_identity_mode flag will only be set at streamon time, as 
that's when vsp1_video_pipeline_build_branch() is called.

> @@ -374,6 +376,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;

This is not needed as the whole structure is initialized to 0, but it doesn't 
hurt too much either.

>  	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 ddb00eadd1ea..28da98d86366
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_sru.h
> +++ b/drivers/media/platform/vsp1/vsp1_sru.h
> @@ -26,6 +26,7 @@ struct vsp1_sru {
>  	struct v4l2_ctrl_handler ctrls;
> 
>  	unsigned int intensity;
> +	bool force_identity_mode;
>  };

While at it, could you add kerneldoc for the structure ?

>  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 e78eadd0295b..9404d7968371
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -31,6 +31,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"
> 
> @@ -480,10 +481,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;
>  	struct vsp1_brx *brx = NULL;
> +	bool sru_found = false;
>  	int ret;
> 
>  	ret = media_entity_enum_init(&ent_enum, &input->entity.vsp1->media_dev);
> @@ -540,13 +543,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;

The flag is never reset to false.

Seems you're missing at least two unit tests for this patch :-)

> +
> +			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 = brx ? &brx->entity : &input->entity;
>  		}

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/6] media: vsp1: Document max_width restriction on SRU

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Friday, 31 August 2018 17:40:42 EEST Kieran Bingham wrote:
> The SRU is currently restricted to 256 pixels as part of the current
> partition algorithm. Document that the actual capability of this
> component is 288 pixels, but don't increase the implementation.
> 
> The extended partition algorithm may later choose to utilise a larger
> input to support overlapping partitions.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> ---
>  drivers/media/platform/vsp1/vsp1_sru.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_sru.c
> b/drivers/media/platform/vsp1/vsp1_sru.c index f277700e5cc2..2a40e09b9aa7
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_sru.c
> +++ b/drivers/media/platform/vsp1/vsp1_sru.c
> @@ -314,6 +314,10 @@ static unsigned int sru_max_width(struct vsp1_entity
> *entity, output = vsp1_entity_get_pad_format(&sru->entity,
> sru->entity.config, SRU_PAD_SOURCE);
> 
> +	/*
> +	 * The maximum width of the SRU is 288 input pixels, but to support the
> +	 * partition algorithm, this is currently kept at 256.
> +	 */

s/maximum width/maximum input width/

I think you should also explain why we restrict it to 256 pixels (unless I'm 
mistaken the idea is that up to 32 pixels can be used for overlapping 
partitions, so each partition will process up to 256 useful pixels).

Patch 5/6 should also be updated in a similar way to better document the 
rationale.

>  	if (input->width != output->width)
>  		return 512;
>  	else

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/6] media: vsp1: use periods at the end of comment sentences

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Friday, 31 August 2018 17:40:44 EEST Kieran Bingham wrote:
> The style of this driver uses periods at the end of sentences in
> comments, but it is applied inconsitently.
> 
> Update a selection of comments which were discovered to be missing their
> period. Also fix the spelling of one usage of 'instantiate'
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

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

and applied to my tree.

> ---
>  drivers/media/platform/vsp1/vsp1_brx.c    | 4 ++--
>  drivers/media/platform/vsp1/vsp1_drv.c    | 6 +++---
>  drivers/media/platform/vsp1/vsp1_entity.c | 2 +-
>  drivers/media/platform/vsp1/vsp1_rpf.c    | 4 ++--
>  drivers/media/platform/vsp1/vsp1_sru.c    | 2 +-
>  drivers/media/platform/vsp1/vsp1_uds.c    | 6 +++---
>  drivers/media/platform/vsp1/vsp1_video.c  | 2 +-
>  drivers/media/platform/vsp1/vsp1_wpf.c    | 2 +-
>  8 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_brx.c
> b/drivers/media/platform/vsp1/vsp1_brx.c index 359917b5d842..5e50178b057d
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_brx.c
> +++ b/drivers/media/platform/vsp1/vsp1_brx.c
> @@ -153,7 +153,7 @@ static int brx_set_format(struct v4l2_subdev *subdev,
>  	format = vsp1_entity_get_pad_format(&brx->entity, config, fmt->pad);
>  	*format = fmt->format;
> 
> -	/* Reset the compose rectangle */
> +	/* Reset the compose rectangle. */
>  	if (fmt->pad != brx->entity.source_pad) {
>  		struct v4l2_rect *compose;
> 
> @@ -164,7 +164,7 @@ static int brx_set_format(struct v4l2_subdev *subdev,
>  		compose->height = format->height;
>  	}
> 
> -	/* Propagate the format code to all pads */
> +	/* Propagate the format code to all pads. */
>  	if (fmt->pad == BRX_PAD_SINK(0)) {
>  		unsigned int i;
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
> b/drivers/media/platform/vsp1/vsp1_drv.c index b6619c9c18bb..249963cb2ec0
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> @@ -802,7 +802,7 @@ static int vsp1_probe(struct platform_device *pdev)
> 
>  	platform_set_drvdata(pdev, vsp1);
> 
> -	/* I/O and IRQ resources (clock managed by the clock PM domain) */
> +	/* I/O and IRQ resources (clock managed by the clock PM domain). */
>  	io = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	vsp1->mmio = devm_ioremap_resource(&pdev->dev, io);
>  	if (IS_ERR(vsp1->mmio))
> @@ -821,7 +821,7 @@ static int vsp1_probe(struct platform_device *pdev)
>  		return ret;
>  	}
> 
> -	/* FCP (optional) */
> +	/* FCP (optional). */
>  	fcp_node = of_parse_phandle(pdev->dev.of_node, "renesas,fcp", 0);
>  	if (fcp_node) {
>  		vsp1->fcp = rcar_fcp_get(fcp_node);
> @@ -869,7 +869,7 @@ static int vsp1_probe(struct platform_device *pdev)
> 
>  	dev_dbg(&pdev->dev, "IP version 0x%08x\n", vsp1->version);
> 
> -	/* Instanciate entities */
> +	/* Instantiate entities. */
>  	ret = vsp1_create_entities(vsp1);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "failed to create entities\n");
> diff --git a/drivers/media/platform/vsp1/vsp1_entity.c
> b/drivers/media/platform/vsp1/vsp1_entity.c index
> 36a29e13109e..a54ab528b060 100644
> --- a/drivers/media/platform/vsp1/vsp1_entity.c
> +++ b/drivers/media/platform/vsp1/vsp1_entity.c
> @@ -404,7 +404,7 @@ int vsp1_subdev_set_pad_format(struct v4l2_subdev
> *subdev, format = vsp1_entity_get_pad_format(entity, config,
> entity->source_pad); *format = fmt->format;
> 
> -	/* Reset the crop and compose rectangles */
> +	/* Reset the crop and compose rectangles. */
>  	selection = vsp1_entity_get_pad_selection(entity, config, fmt->pad,
>  						  V4L2_SEL_TGT_CROP);
>  	selection->left = 0;
> diff --git a/drivers/media/platform/vsp1/vsp1_rpf.c
> b/drivers/media/platform/vsp1/vsp1_rpf.c index f8005b60b9d2..616afa7e165f
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_rpf.c
> +++ b/drivers/media/platform/vsp1/vsp1_rpf.c
> @@ -108,7 +108,7 @@ static void rpf_configure_stream(struct vsp1_entity
> *entity, vsp1_rpf_write(rpf, dlb, VI6_RPF_INFMT, infmt);
>  	vsp1_rpf_write(rpf, dlb, VI6_RPF_DSWAP, fmtinfo->swap);
> 
> -	/* Output location */
> +	/* Output location. */
>  	if (pipe->brx) {
>  		const struct v4l2_rect *compose;
> 
> @@ -309,7 +309,7 @@ static void rpf_configure_partition(struct vsp1_entity
> *entity,
> 
>  	/*
>  	 * Interlaced pipelines will use the extended pre-cmd to process
> -	 * SRCM_ADDR_{Y,C0,C1}
> +	 * SRCM_ADDR_{Y,C0,C1}.
>  	 */
>  	if (pipe->interlaced) {
>  		vsp1_rpf_configure_autofld(rpf, dl);
> diff --git a/drivers/media/platform/vsp1/vsp1_sru.c
> b/drivers/media/platform/vsp1/vsp1_sru.c index 2a40e09b9aa7..f48b085b5b5e
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_sru.c
> +++ b/drivers/media/platform/vsp1/vsp1_sru.c
> @@ -339,7 +339,7 @@ static void sru_partition(struct vsp1_entity *entity,
>  	output = vsp1_entity_get_pad_format(&sru->entity, sru->entity.config,
>  					    SRU_PAD_SOURCE);
> 
> -	/* Adapt if SRUx2 is enabled */
> +	/* Adapt if SRUx2 is enabled. */
>  	if (input->width != output->width) {
>  		window->width /= 2;
>  		window->left /= 2;
> diff --git a/drivers/media/platform/vsp1/vsp1_uds.c
> b/drivers/media/platform/vsp1/vsp1_uds.c index 7c11651db5d4..c704bdaf4edc
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_uds.c
> +++ b/drivers/media/platform/vsp1/vsp1_uds.c
> @@ -314,13 +314,13 @@ static void uds_configure_partition(struct vsp1_entity
> *entity, output = vsp1_entity_get_pad_format(&uds->entity,
> uds->entity.config, UDS_PAD_SOURCE);
> 
> -	/* Input size clipping */
> +	/* Input size clipping. */
>  	vsp1_uds_write(uds, dlb, VI6_UDS_HSZCLIP, VI6_UDS_HSZCLIP_HCEN |
>  		       (0 << VI6_UDS_HSZCLIP_HCL_OFST_SHIFT) |
>  		       (partition->uds_sink.width
>  				<< VI6_UDS_HSZCLIP_HCL_SIZE_SHIFT));
> 
> -	/* Output size clipping */
> +	/* Output size clipping. */
>  	vsp1_uds_write(uds, dlb, VI6_UDS_CLIP_SIZE,
>  		       (partition->uds_source.width
>  				<< VI6_UDS_CLIP_SIZE_HSIZE_SHIFT) |
> @@ -374,7 +374,7 @@ static void uds_partition(struct vsp1_entity *entity,
>  	const struct v4l2_mbus_framefmt *output;
>  	const struct v4l2_mbus_framefmt *input;
> 
> -	/* Initialise the partition state */
> +	/* Initialise the partition state. */
>  	partition->uds_sink = *window;
>  	partition->uds_source = *window;
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
> b/drivers/media/platform/vsp1/vsp1_video.c index 9404d7968371..c114cc4d2349
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_video.c
> +++ b/drivers/media/platform/vsp1/vsp1_video.c
> @@ -891,7 +891,7 @@ static void vsp1_video_cleanup_pipeline(struct
> vsp1_pipeline *pipe) pipe->stream_config = NULL;
>  	pipe->configured = false;
> 
> -	/* Release our partition table allocation */
> +	/* Release our partition table allocation. */
>  	kfree(pipe->part_table);
>  	pipe->part_table = NULL;
>  }
> diff --git a/drivers/media/platform/vsp1/vsp1_wpf.c
> b/drivers/media/platform/vsp1/vsp1_wpf.c index c2a1a7f97e26..32bb207b2007
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_wpf.c
> +++ b/drivers/media/platform/vsp1/vsp1_wpf.c
> @@ -317,7 +317,7 @@ static void wpf_configure_stream(struct vsp1_entity
> *entity,
> 
>  	vsp1_wpf_write(wpf, dlb, VI6_WPF_SRCRPF, srcrpf);
> 
> -	/* Enable interrupts */
> +	/* Enable interrupts. */
>  	vsp1_dl_body_write(dlb, VI6_WPF_IRQ_STA(wpf->entity.index), 0);
>  	vsp1_dl_body_write(dlb, VI6_WPF_IRQ_ENB(wpf->entity.index),
>  			   VI6_WFP_IRQ_ENB_DFEE);


-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] media: vsp1: Remove artificial pixel limitation

[Laurent Pinchart]

Hi Kieran,

Would you mind changing the patch subject to "Remove artificial minimum width/
height limitation" ?

On Friday, 14 September 2018 13:23:04 EEST Laurent Pinchart wrote:
> On Friday, 31 August 2018 17:40:39 EEST Kieran Bingham wrote:
> > The VSP1 has a minimum width and height of a single pixel, with the
> > exception of pixel formats with sub-sampling.
> > 
> > Remove the artificial minimum width and minimum height limitation, and
> > instead clamp the minimum dimensions based upon the sub-sampling
> > parameter of that dimension.
> > 
> > Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> and applied to my tree.
> 
> > ---
> > 
> >  drivers/media/platform/vsp1/vsp1_video.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/media/platform/vsp1/vsp1_video.c
> > b/drivers/media/platform/vsp1/vsp1_video.c index
> > 81d47a09d7bc..e78eadd0295b
> > 100644
> > --- a/drivers/media/platform/vsp1/vsp1_video.c
> > +++ b/drivers/media/platform/vsp1/vsp1_video.c
> > @@ -38,9 +38,7 @@
> > 
> >  #define VSP1_VIDEO_DEF_WIDTH		1024
> >  #define VSP1_VIDEO_DEF_HEIGHT		768
> > 
> > -#define VSP1_VIDEO_MIN_WIDTH		2U
> > 
> >  #define VSP1_VIDEO_MAX_WIDTH		8190U
> > 
> > -#define VSP1_VIDEO_MIN_HEIGHT		2U
> > 
> >  #define VSP1_VIDEO_MAX_HEIGHT		8190U
> >  
> >  /*
> > 
> > --------------------------------------------------------------------------
> > -
> > -- @@ -136,9 +134,8 @@ static int __vsp1_video_try_format(struct
> > vsp1_video
> > *video, height = round_down(height, info->vsub);
> > 
> >  	/* Clamp the width and height. */
> > 
> > -	pix->width = clamp(width, VSP1_VIDEO_MIN_WIDTH, VSP1_VIDEO_MAX_WIDTH);
> > -	pix->height = clamp(height, VSP1_VIDEO_MIN_HEIGHT,
> > -			    VSP1_VIDEO_MAX_HEIGHT);
> > +	pix->width = clamp(width, info->hsub, VSP1_VIDEO_MAX_WIDTH);
> > +	pix->height = clamp(height, info->vsub, VSP1_VIDEO_MAX_HEIGHT);
> > 
> >  	/*
> >  	
> >  	 * Compute and clamp the stride and image size. While not documented in


-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/6] media: vsp1: Remove artificial pixel limitation

[Kieran Bingham]

On 14/09/18 11:47, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Would you mind changing the patch subject to "Remove artificial minimum width/
> height limitation" ?

Not at all.

> 
> On Friday, 14 September 2018 13:23:04 EEST Laurent Pinchart wrote:
>> On Friday, 31 August 2018 17:40:39 EEST Kieran Bingham wrote:
>>> The VSP1 has a minimum width and height of a single pixel, with the
>>> exception of pixel formats with sub-sampling.
>>>
>>> Remove the artificial minimum width and minimum height limitation, and
>>> instead clamp the minimum dimensions based upon the sub-sampling
>>> parameter of that dimension.
>>>
>>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
>>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>
>> and applied to my tree.

Would you like me to resend the patch ?

--
Regards

Kieran


>>
>>> ---
>>>
>>>  drivers/media/platform/vsp1/vsp1_video.c | 7 ++-----
>>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/vsp1/vsp1_video.c
>>> b/drivers/media/platform/vsp1/vsp1_video.c index
>>> 81d47a09d7bc..e78eadd0295b
>>> 100644
>>> --- a/drivers/media/platform/vsp1/vsp1_video.c
>>> +++ b/drivers/media/platform/vsp1/vsp1_video.c
>>> @@ -38,9 +38,7 @@
>>>
>>>  #define VSP1_VIDEO_DEF_WIDTH		1024
>>>  #define VSP1_VIDEO_DEF_HEIGHT		768
>>>
>>> -#define VSP1_VIDEO_MIN_WIDTH		2U
>>>
>>>  #define VSP1_VIDEO_MAX_WIDTH		8190U
>>>
>>> -#define VSP1_VIDEO_MIN_HEIGHT		2U
>>>
>>>  #define VSP1_VIDEO_MAX_HEIGHT		8190U
>>>  
>>>  /*
>>>
>>> --------------------------------------------------------------------------
>>> -
>>> -- @@ -136,9 +134,8 @@ static int __vsp1_video_try_format(struct
>>> vsp1_video
>>> *video, height = round_down(height, info->vsub);
>>>
>>>  	/* Clamp the width and height. */
>>>
>>> -	pix->width = clamp(width, VSP1_VIDEO_MIN_WIDTH, VSP1_VIDEO_MAX_WIDTH);
>>> -	pix->height = clamp(height, VSP1_VIDEO_MIN_HEIGHT,
>>> -			    VSP1_VIDEO_MAX_HEIGHT);
>>> +	pix->width = clamp(width, info->hsub, VSP1_VIDEO_MAX_WIDTH);
>>> +	pix->height = clamp(height, info->vsub, VSP1_VIDEO_MAX_HEIGHT);
>>>
>>>  	/*
>>>  	
>>>  	 * Compute and clamp the stride and image size. While not documented in
> 
> 

Re: [PATCH 2/6] media: vsp1: Correct the pitch on multiplanar formats

[Kieran Bingham]

Hi Laurent,

On 14/09/18 11:25, Laurent Pinchart wrote:
> Hi Kieran,
> 
> Thank you for the patch.
> 
> On Friday, 31 August 2018 17:40:40 EEST Kieran Bingham wrote:
>> DRM pipelines now support tri-planar as well as packed formats with
>> YCbCr, however the pitch calculation was not updated to support this.
>>
>> Correct this by adjusting the bytesperline accordingly when 3 planes are
>> used.
>>
>> Fixes: 7863ac504bc5 ("drm: rcar-du: Add tri-planar memory formats support")
>> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> I already have a similar patch from Matsuoka-san in my tree, please see 
> https://patchwork.kernel.org/patch/10425565/. I'll update it with the fixes 
> tag.
> 
>> ---
>>  drivers/media/platform/vsp1/vsp1_drm.c | 10 ++++++++++
>>  include/media/vsp1.h                   |  2 +-
>>  2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/vsp1/vsp1_drm.c
>> b/drivers/media/platform/vsp1/vsp1_drm.c index b9c0f695d002..b9afd98f6867
>> 100644
>> --- a/drivers/media/platform/vsp1/vsp1_drm.c
>> +++ b/drivers/media/platform/vsp1/vsp1_drm.c
>> @@ -814,6 +814,16 @@ int vsp1_du_atomic_update(struct device *dev, unsigned
>> int pipe_index, rpf->format.num_planes = fmtinfo->planes;
>>  	rpf->format.plane_fmt[0].bytesperline = cfg->pitch;
>>  	rpf->format.plane_fmt[1].bytesperline = cfg->pitch;
>> +
>> +	/*
>> +	 * Packed YUV formats are subsampled, but the packing of two components
>> +	 * into a single plane compensates for this leaving the bytesperline
>> +	 * to be the correct value. For multiplanar formats we must adjust the
>> +	 * pitch accordingly.
>> +	 */
>> +	if (fmtinfo->planes == 3)
>> +		rpf->format.plane_fmt[1].bytesperline /= fmtinfo->hsub;
>> +
>>  	rpf->alpha = cfg->alpha;
>>
>>  	rpf->mem.addr[0] = cfg->mem[0];
>> diff --git a/include/media/vsp1.h b/include/media/vsp1.h
>> index 3093b9cb9067..0ce19b595cc7 100644
>> --- a/include/media/vsp1.h
>> +++ b/include/media/vsp1.h
>> @@ -46,7 +46,7 @@ int vsp1_du_setup_lif(struct device *dev, unsigned int
>> pipe_index, /**
>>   * struct vsp1_du_atomic_config - VSP atomic configuration parameters
>>   * @pixelformat: plane pixel format (V4L2 4CC)
>> - * @pitch: line pitch in bytes, for all planes
>> + * @pitch: line pitch in bytes
> 
> Should I update the above-mentioned patch with this as well ? How about 
> phrasing it as "line pitch in bytes for the first plane" ?

Yes, your suggestion sounds fine.

The patch at [0] looks good to me as a fix for this issue.

for: "v4l: vsp1: Fix YCbCr planar formats pitch calculation" [0]
With the fixes tag, and documentation updated:

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

[0]  https://patchwork.kernel.org/patch/10425565

> 
>>   * @mem: DMA memory address for each plane of the frame buffer
>>   * @src: source rectangle in the frame buffer (integer coordinates)
>>   * @dst: destination rectangle on the display (integer coordinates)
>