[PATCH v2 0/3] rcar-vin: add support for UDS (Up Down Scaler)

[PATCH v2 0/3] rcar-vin: add support for UDS (Up Down Scaler)

[Niklas Söderlund]

Hi,

This series adds support for Renesas R-Car Gen3 VIN Up Down Scaler
(UDS). Some VIN instances have access to a often shared UDS which can be
used to scale the captured image up or down. If the scaler is shared it
can only be used exclusively by one VIN at a time, switching in runtime
and detection if a UDS are in use is supported in this series. If the
user tries to start a capture on a VIN which would require the use of a
scaler but that scaler is in use -EBUSY is returned.

Patch 1/3 fix a format alignment issue found when working with UDS
support. While patch 2/3 ands the UDS logic and 3/3 defines which VIN on
which SoC have access to a UDS and how it's shared.

The series is based on top of media-tree/master and is tested on R-Car
Gen3 H3, M3-W, M3-N and Gen2 Koelsch (checking for regressions as Gen2
have no UDS).

* Changes since v1
- Patch 1/3 have been replaced with a less strict version after good 
  comments from Hans.

Niklas Söderlund (3):
  rcar-vin: align width before stream start
  rcar-vin: add support for UDS (Up Down Scaler)
  rcar-vin: declare which VINs can use a Up Down Scaler (UDS)

 drivers/media/platform/rcar-vin/rcar-core.c |  18 +++
 drivers/media/platform/rcar-vin/rcar-dma.c  | 139 ++++++++++++++++++--
 drivers/media/platform/rcar-vin/rcar-v4l2.c |   9 ++
 drivers/media/platform/rcar-vin/rcar-vin.h  |  24 ++++
 4 files changed, 180 insertions(+), 10 deletions(-)

-- 
2.19.0

[PATCH v2 1/3] rcar-vin: align width before stream start

[Niklas Söderlund]

Instead of aligning the image width to match the image stride at stream
start time do so when configuring the format. This allows the format
width to strictly match the image stride which is useful when enabling
scaling on Gen3.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-dma.c  | 5 +----
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 9 +++++++++
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 92323310f7352147..e752bc86e40153b1 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -597,10 +597,7 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
 	if (vin->info->model != RCAR_GEN3)
 		rvin_crop_scale_comp_gen2(vin);
 
-	if (vin->format.pixelformat == V4L2_PIX_FMT_NV16)
-		rvin_write(vin, ALIGN(vin->format.width, 0x20), VNIS_REG);
-	else
-		rvin_write(vin, ALIGN(vin->format.width, 0x10), VNIS_REG);
+	rvin_write(vin, vin->format.width, VNIS_REG);
 }
 
 /* -----------------------------------------------------------------------------
diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index dc77682b47857c97..94bc559a0cb1e47a 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -96,6 +96,15 @@ static void rvin_format_align(struct rvin_dev *vin, struct v4l2_pix_format *pix)
 	     pix->pixelformat == V4L2_PIX_FMT_XBGR32))
 		pix->pixelformat = RVIN_DEFAULT_FORMAT;
 
+	switch (pix->pixelformat) {
+	case V4L2_PIX_FMT_NV16:
+		pix->width = ALIGN(pix->width, 0x20);
+		break;
+	default:
+		pix->width = ALIGN(pix->width, 0x10);
+		break;
+	}
+
 	switch (pix->field) {
 	case V4L2_FIELD_TOP:
 	case V4L2_FIELD_BOTTOM:
-- 
2.19.0

[PATCH v2 2/3] rcar-vin: add support for UDS (Up Down Scaler)

[Niklas Söderlund]

Some VIN instances have access to a Up Down Scaler (UDS). The UDS are on
most SoCs shared between two VINs, the UDS can of course only be used by
one VIN at a time.

Add support to configure the UDS registers which are mapped to both VINs
sharing the UDS address-space. While validations the format at stream
start make sure the companion VIN is not already using the scaler. If
the scaler already is in use return -EBUSY.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-dma.c | 134 ++++++++++++++++++++-
 drivers/media/platform/rcar-vin/rcar-vin.h |  24 ++++
 2 files changed, 152 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index e752bc86e40153b1..f33146bda9300c21 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -74,6 +74,10 @@
 
 /* Register offsets specific for Gen3 */
 #define VNCSI_IFMD_REG		0x20 /* Video n CSI2 Interface Mode Register */
+#define VNUDS_CTRL_REG		0x80 /* Video n scaling control register */
+#define VNUDS_SCALE_REG		0x84 /* Video n scaling factor register */
+#define VNUDS_PASS_BWIDTH_REG	0x90 /* Video n passband register */
+#define VNUDS_CLIP_SIZE_REG	0xa4 /* Video n UDS output size clipping reg */
 
 /* Register bit fields for R-Car VIN */
 /* Video n Main Control Register bits */
@@ -129,6 +133,9 @@
 #define VNCSI_IFMD_CSI_CHSEL(n) (((n) & 0xf) << 0)
 #define VNCSI_IFMD_CSI_CHSEL_MASK 0xf
 
+/* Video n scaling control register (Gen3) */
+#define VNUDS_CTRL_AMD		(1 << 30)
+
 struct rvin_buffer {
 	struct vb2_v4l2_buffer vb;
 	struct list_head list;
@@ -572,6 +579,78 @@ static void rvin_crop_scale_comp_gen2(struct rvin_dev *vin)
 		0, 0);
 }
 
+static unsigned int rvin_scale_ratio(unsigned int in, unsigned int out)
+{
+	unsigned int ratio;
+
+	ratio = in * 4096 / out;
+	return ratio >= 0x10000 ? 0xffff : ratio;
+}
+
+static unsigned int rvin_ratio_to_bwidth(unsigned int ratio)
+{
+	unsigned int mant, frac;
+
+	mant = (ratio & 0xF000) >> 12;
+	frac = ratio & 0x0FFF;
+	if (mant)
+		return 64 * 4096 * mant / (4096 * mant + frac);
+
+	return 64;
+}
+
+static bool rvin_gen3_need_scaling(struct rvin_dev *vin)
+{
+	if (vin->info->model != RCAR_GEN3)
+		return false;
+
+	return vin->crop.width != vin->format.width ||
+		vin->crop.height != vin->format.height;
+}
+
+static void rvin_crop_scale_comp_gen3(struct rvin_dev *vin)
+{
+	unsigned int ratio_h, ratio_v;
+	unsigned int bwidth_h, bwidth_v;
+	u32 vnmc, clip_size;
+
+	if (!rvin_gen3_need_scaling(vin))
+		return;
+
+	ratio_h = rvin_scale_ratio(vin->crop.width, vin->format.width);
+	bwidth_h = rvin_ratio_to_bwidth(ratio_h);
+
+	ratio_v = rvin_scale_ratio(vin->crop.height, vin->format.height);
+	bwidth_v = rvin_ratio_to_bwidth(ratio_v);
+
+	clip_size = vin->format.width << 16;
+
+	switch (vin->format.field) {
+	case V4L2_FIELD_INTERLACED_TB:
+	case V4L2_FIELD_INTERLACED_BT:
+	case V4L2_FIELD_INTERLACED:
+	case V4L2_FIELD_SEQ_TB:
+	case V4L2_FIELD_SEQ_BT:
+		clip_size |= vin->format.height / 2;
+		break;
+	default:
+		clip_size |= vin->format.height;
+		break;
+	}
+
+	vnmc = rvin_read(vin, VNMC_REG);
+	rvin_write(vin, (vnmc & ~VNMC_VUP) | VNMC_SCLE, VNMC_REG);
+	rvin_write(vin, VNUDS_CTRL_AMD, VNUDS_CTRL_REG);
+	rvin_write(vin, (ratio_h << 16) | ratio_v, VNUDS_SCALE_REG);
+	rvin_write(vin, (bwidth_h << 16) | bwidth_v, VNUDS_PASS_BWIDTH_REG);
+	rvin_write(vin, clip_size, VNUDS_CLIP_SIZE_REG);
+	rvin_write(vin, vnmc, VNMC_REG);
+
+	vin_dbg(vin, "Pre-Clip: %ux%u@%u:%u Post-Clip: %ux%u@%u:%u\n",
+		vin->crop.width, vin->crop.height, vin->crop.left,
+		vin->crop.top, vin->format.width, vin->format.height, 0, 0);
+}
+
 void rvin_crop_scale_comp(struct rvin_dev *vin)
 {
 	/* Set Start/End Pixel/Line Pre-Clip */
@@ -593,8 +672,9 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
 		break;
 	}
 
-	/* TODO: Add support for the UDS scaler. */
-	if (vin->info->model != RCAR_GEN3)
+	if (vin->info->model == RCAR_GEN3)
+		rvin_crop_scale_comp_gen3(vin);
+	else
 		rvin_crop_scale_comp_gen2(vin);
 
 	rvin_write(vin, vin->format.width, VNIS_REG);
@@ -748,6 +828,9 @@ static int rvin_setup(struct rvin_dev *vin)
 			vnmc |= VNMC_DPINE;
 	}
 
+	if (rvin_gen3_need_scaling(vin))
+		vnmc |= VNMC_SCLE;
+
 	/* Progressive or interlaced mode */
 	interrupts = progressive ? VNIE_FIE : VNIE_EFE;
 
@@ -1078,10 +1161,42 @@ static int rvin_mc_validate_format(struct rvin_dev *vin, struct v4l2_subdev *sd,
 		return -EPIPE;
 	}
 
-	if (fmt.format.width != vin->format.width ||
-	    fmt.format.height != vin->format.height ||
-	    fmt.format.code != vin->mbus_code)
-		return -EPIPE;
+	vin->crop.width = fmt.format.width;
+	vin->crop.height = fmt.format.height;
+
+	if (rvin_gen3_need_scaling(vin)) {
+		const struct rvin_group_scaler *scaler;
+		struct rvin_dev *companion;
+
+		if (fmt.format.code != vin->mbus_code)
+			return -EPIPE;
+
+		if (!vin->info->scalers)
+			return -EPIPE;
+
+		for (scaler = vin->info->scalers;
+		     scaler->vin || scaler->companion; scaler++)
+			if (scaler->vin == vin->id)
+				break;
+
+		/* No scaler found for VIN. */
+		if (!scaler->vin && !scaler->companion)
+			return -EPIPE;
+
+		/* Make sure companion not using scaler. */
+		if (scaler->companion != -1) {
+			companion = vin->group->vin[scaler->companion];
+			if (companion &&
+			    companion->state != STOPPED &&
+			    rvin_gen3_need_scaling(companion))
+				return -EBUSY;
+		}
+	} else {
+		if (fmt.format.width != vin->format.width ||
+		    fmt.format.height != vin->format.height ||
+		    fmt.format.code != vin->mbus_code)
+			return -EPIPE;
+	}
 
 	return 0;
 }
@@ -1189,6 +1304,7 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
 	struct rvin_dev *vin = vb2_get_drv_priv(vq);
 	unsigned long flags;
 	int retries = 0;
+	u32 vnmc;
 
 	spin_lock_irqsave(&vin->qlock, flags);
 
@@ -1220,6 +1336,12 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
 		vin->state = STOPPED;
 	}
 
+	/* Clear UDS usage after we have stopped */
+	if (vin->info->model == RCAR_GEN3) {
+		vnmc = rvin_read(vin, VNMC_REG) & ~(VNMC_SCLE | VNMC_VUP);
+		rvin_write(vin, vnmc, VNMC_REG);
+	}
+
 	/* Release all active buffers */
 	return_all_buffers(vin, VB2_BUF_STATE_ERROR);
 
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index 0b13b34d03e3dce4..5a617a30ba8c9a5a 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -122,6 +122,28 @@ struct rvin_group_route {
 	unsigned int mask;
 };
 
+/**
+ * struct rvin_group_scaler - describes a scaler attached to a VIN
+ *
+ * @vin:	Numerical VIN id that have access to a UDS.
+ * @companion:  Numerical VIN id that @vin share the UDS with.
+ *
+ * -- note::
+ *	Some R-Car VIN instances have access to a Up Down Scaler (UDS).
+ *	If a VIN have a UDS attached it's almost always shared between
+ *	two VIN instances. The UDS can only be used by one VIN at a time,
+ *	so the companion relationship needs to be described as well.
+ *
+ *	There are at most two VINs sharing a UDS. For each UDS shared
+ *	between two VINs there needs to be two instances of struct
+ *	rvin_group_scaler describing each of the VINs individually. If
+ *	a VIN do not share its UDS set companion to -1.
+ */
+struct rvin_group_scaler {
+	int vin;
+	int companion;
+};
+
 /**
  * struct rvin_info - Information about the particular VIN implementation
  * @model:		VIN model
@@ -130,6 +152,7 @@ struct rvin_group_route {
  * @max_height:		max input height the VIN supports
  * @routes:		list of possible routes from the CSI-2 recivers to
  *			all VINs. The list mush be NULL terminated.
+ * @scalers:		List of available scalers, must be NULL terminated.
  */
 struct rvin_info {
 	enum model_id model;
@@ -138,6 +161,7 @@ struct rvin_info {
 	unsigned int max_width;
 	unsigned int max_height;
 	const struct rvin_group_route *routes;
+	const struct rvin_group_scaler *scalers;
 };
 
 /**
-- 
2.19.0

[PATCH v2 3/3] rcar-vin: declare which VINs can use a Up Down Scaler (UDS)

[Niklas Söderlund]

Add information about which VINs on which SoC have access to a UDS
scaler.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 01e418c2d4c6792e..337ae8bbe1e0b14c 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -919,12 +919,21 @@ static const struct rvin_group_route rcar_info_r8a7795_routes[] = {
 	{ /* Sentinel */ }
 };
 
+static const struct rvin_group_scaler rcar_info_h3_m3w_m3n_scalers[] = {
+	{ .vin = 0, .companion = 1 },
+	{ .vin = 1, .companion = 0 },
+	{ .vin = 4, .companion = 5 },
+	{ .vin = 5, .companion = 4 },
+	{ /* Sentinel */ }
+};
+
 static const struct rvin_info rcar_info_r8a7795 = {
 	.model = RCAR_GEN3,
 	.use_mc = true,
 	.max_width = 4096,
 	.max_height = 4096,
 	.routes = rcar_info_r8a7795_routes,
+	.scalers = rcar_info_h3_m3w_m3n_scalers,
 };
 
 static const struct rvin_group_route rcar_info_r8a7795es1_routes[] = {
@@ -979,6 +988,7 @@ static const struct rvin_info rcar_info_r8a7795es1 = {
 	.max_width = 4096,
 	.max_height = 4096,
 	.routes = rcar_info_r8a7795es1_routes,
+	.scalers = rcar_info_h3_m3w_m3n_scalers,
 };
 
 static const struct rvin_group_route rcar_info_r8a7796_routes[] = {
@@ -1019,6 +1029,7 @@ static const struct rvin_info rcar_info_r8a7796 = {
 	.max_width = 4096,
 	.max_height = 4096,
 	.routes = rcar_info_r8a7796_routes,
+	.scalers = rcar_info_h3_m3w_m3n_scalers,
 };
 
 static const struct rvin_group_route rcar_info_r8a77965_routes[] = {
@@ -1063,6 +1074,7 @@ static const struct rvin_info rcar_info_r8a77965 = {
 	.max_width = 4096,
 	.max_height = 4096,
 	.routes = rcar_info_r8a77965_routes,
+	.scalers = rcar_info_h3_m3w_m3n_scalers,
 };
 
 static const struct rvin_group_route rcar_info_r8a77970_routes[] = {
@@ -1088,12 +1100,18 @@ static const struct rvin_group_route rcar_info_r8a77995_routes[] = {
 	{ /* Sentinel */ }
 };
 
+static const struct rvin_group_scaler rcar_info_r8a77995_scalers[] = {
+	{ .vin = 4, .companion = -1 },
+	{ /* Sentinel */ }
+};
+
 static const struct rvin_info rcar_info_r8a77995 = {
 	.model = RCAR_GEN3,
 	.use_mc = true,
 	.max_width = 4096,
 	.max_height = 4096,
 	.routes = rcar_info_r8a77995_routes,
+	.scalers = rcar_info_r8a77995_scalers,
 };
 
 static const struct of_device_id rvin_of_id_table[] = {
-- 
2.19.0

Re: [PATCH v2 1/3] rcar-vin: align width before stream start

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Thursday, 4 October 2018 23:04:00 EEST Niklas Söderlund wrote:
> Instead of aligning the image width to match the image stride at stream
> start time do so when configuring the format. This allows the format
> width to strictly match the image stride which is useful when enabling
> scaling on Gen3.

But is this required ? Aren't there use cases where an image with a width not 
aligned with the stride requirements should be captured ? As long as the 
stride itself matches the hardware requirements, I don't see a reason to 
disallow that.

> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c  | 5 +----
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 9 +++++++++
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> b/drivers/media/platform/rcar-vin/rcar-dma.c index
> 92323310f7352147..e752bc86e40153b1 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -597,10 +597,7 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
>  	if (vin->info->model != RCAR_GEN3)
>  		rvin_crop_scale_comp_gen2(vin);
> 
> -	if (vin->format.pixelformat == V4L2_PIX_FMT_NV16)
> -		rvin_write(vin, ALIGN(vin->format.width, 0x20), VNIS_REG);
> -	else
> -		rvin_write(vin, ALIGN(vin->format.width, 0x10), VNIS_REG);
> +	rvin_write(vin, vin->format.width, VNIS_REG);
>  }
> 
>  /*
> ---------------------------------------------------------------------------
> -- diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> dc77682b47857c97..94bc559a0cb1e47a 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -96,6 +96,15 @@ static void rvin_format_align(struct rvin_dev *vin,
> struct v4l2_pix_format *pix) pix->pixelformat == V4L2_PIX_FMT_XBGR32))
>  		pix->pixelformat = RVIN_DEFAULT_FORMAT;
> 
> +	switch (pix->pixelformat) {
> +	case V4L2_PIX_FMT_NV16:
> +		pix->width = ALIGN(pix->width, 0x20);
> +		break;
> +	default:
> +		pix->width = ALIGN(pix->width, 0x10);
> +		break;
> +	}
> +
>  	switch (pix->field) {
>  	case V4L2_FIELD_TOP:
>  	case V4L2_FIELD_BOTTOM:

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 3/3] rcar-vin: declare which VINs can use a Up Down Scaler (UDS)

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Thursday, 4 October 2018 23:04:02 EEST Niklas Söderlund wrote:
> Add information about which VINs on which SoC have access to a UDS
> scaler.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

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

> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c
> b/drivers/media/platform/rcar-vin/rcar-core.c index
> 01e418c2d4c6792e..337ae8bbe1e0b14c 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -919,12 +919,21 @@ static const struct rvin_group_route
> rcar_info_r8a7795_routes[] = { { /* Sentinel */ }
>  };
> 
> +static const struct rvin_group_scaler rcar_info_h3_m3w_m3n_scalers[] = {
> +	{ .vin = 0, .companion = 1 },
> +	{ .vin = 1, .companion = 0 },
> +	{ .vin = 4, .companion = 5 },
> +	{ .vin = 5, .companion = 4 },
> +	{ /* Sentinel */ }
> +};
> +
>  static const struct rvin_info rcar_info_r8a7795 = {
>  	.model = RCAR_GEN3,
>  	.use_mc = true,
>  	.max_width = 4096,
>  	.max_height = 4096,
>  	.routes = rcar_info_r8a7795_routes,
> +	.scalers = rcar_info_h3_m3w_m3n_scalers,
>  };
> 
>  static const struct rvin_group_route rcar_info_r8a7795es1_routes[] = {
> @@ -979,6 +988,7 @@ static const struct rvin_info rcar_info_r8a7795es1 = {
>  	.max_width = 4096,
>  	.max_height = 4096,
>  	.routes = rcar_info_r8a7795es1_routes,
> +	.scalers = rcar_info_h3_m3w_m3n_scalers,
>  };
> 
>  static const struct rvin_group_route rcar_info_r8a7796_routes[] = {
> @@ -1019,6 +1029,7 @@ static const struct rvin_info rcar_info_r8a7796 = {
>  	.max_width = 4096,
>  	.max_height = 4096,
>  	.routes = rcar_info_r8a7796_routes,
> +	.scalers = rcar_info_h3_m3w_m3n_scalers,
>  };
> 
>  static const struct rvin_group_route rcar_info_r8a77965_routes[] = {
> @@ -1063,6 +1074,7 @@ static const struct rvin_info rcar_info_r8a77965 = {
>  	.max_width = 4096,
>  	.max_height = 4096,
>  	.routes = rcar_info_r8a77965_routes,
> +	.scalers = rcar_info_h3_m3w_m3n_scalers,
>  };
> 
>  static const struct rvin_group_route rcar_info_r8a77970_routes[] = {
> @@ -1088,12 +1100,18 @@ static const struct rvin_group_route
> rcar_info_r8a77995_routes[] = { { /* Sentinel */ }
>  };
> 
> +static const struct rvin_group_scaler rcar_info_r8a77995_scalers[] = {
> +	{ .vin = 4, .companion = -1 },
> +	{ /* Sentinel */ }
> +};
> +
>  static const struct rvin_info rcar_info_r8a77995 = {
>  	.model = RCAR_GEN3,
>  	.use_mc = true,
>  	.max_width = 4096,
>  	.max_height = 4096,
>  	.routes = rcar_info_r8a77995_routes,
> +	.scalers = rcar_info_r8a77995_scalers,
>  };
> 
>  static const struct of_device_id rvin_of_id_table[] = {


-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/3] rcar-vin: align width before stream start

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2018-10-04 23:11:50 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Thursday, 4 October 2018 23:04:00 EEST Niklas Söderlund wrote:
> > Instead of aligning the image width to match the image stride at stream
> > start time do so when configuring the format. This allows the format
> > width to strictly match the image stride which is useful when enabling
> > scaling on Gen3.
> 
> But is this required ? Aren't there use cases where an image with a width not 
> aligned with the stride requirements should be captured ? As long as the 
> stride itself matches the hardware requirements, I don't see a reason to 
> disallow that.

Yes there is a use-case for that. And the rcar-vin driver is starting to 
reaching a point where the whole format handling for buffers, source 
format, croping and scaling needs to be rewritten to enable more valid 
use-cases.

This fix is however in my view required at this time with the current 
driver design. If we keep aligning the width at stream on and enable the 
UDS it becomes apparent that when the alignment is needed the values for 
the stride register conflicts which how the scaling coefficients are 
calculated and the captured frame is distorted.

My hope is to add upstream to support for the UDS, support for 
sequential field captures and some more output pixel formats. And once 
the driver feature complete on Gen3 come back and simplify and if 
possible align the Gen2 and Gen3 format handling which in part adds to 
the somewhat messy current state.

> 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-dma.c  | 5 +----
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c | 9 +++++++++
> >  2 files changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> > b/drivers/media/platform/rcar-vin/rcar-dma.c index
> > 92323310f7352147..e752bc86e40153b1 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -597,10 +597,7 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
> >  	if (vin->info->model != RCAR_GEN3)
> >  		rvin_crop_scale_comp_gen2(vin);
> > 
> > -	if (vin->format.pixelformat == V4L2_PIX_FMT_NV16)
> > -		rvin_write(vin, ALIGN(vin->format.width, 0x20), VNIS_REG);
> > -	else
> > -		rvin_write(vin, ALIGN(vin->format.width, 0x10), VNIS_REG);
> > +	rvin_write(vin, vin->format.width, VNIS_REG);
> >  }
> > 
> >  /*
> > ---------------------------------------------------------------------------
> > -- diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> > dc77682b47857c97..94bc559a0cb1e47a 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -96,6 +96,15 @@ static void rvin_format_align(struct rvin_dev *vin,
> > struct v4l2_pix_format *pix) pix->pixelformat == V4L2_PIX_FMT_XBGR32))
> >  		pix->pixelformat = RVIN_DEFAULT_FORMAT;
> > 
> > +	switch (pix->pixelformat) {
> > +	case V4L2_PIX_FMT_NV16:
> > +		pix->width = ALIGN(pix->width, 0x20);
> > +		break;
> > +	default:
> > +		pix->width = ALIGN(pix->width, 0x10);
> > +		break;
> > +	}
> > +
> >  	switch (pix->field) {
> >  	case V4L2_FIELD_TOP:
> >  	case V4L2_FIELD_BOTTOM:
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH v2 1/3] rcar-vin: align width before stream start

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2018-10-04 23:11:50 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Thursday, 4 October 2018 23:04:00 EEST Niklas S�derlund wrote:
> > Instead of aligning the image width to match the image stride at stream
> > start time do so when configuring the format. This allows the format
> > width to strictly match the image stride which is useful when enabling
> > scaling on Gen3.
> 
> But is this required ? Aren't there use cases where an image with a width not 
> aligned with the stride requirements should be captured ? As long as the 
> stride itself matches the hardware requirements, I don't see a reason to 
> disallow that.

Yes there is a use-case for that. And the rcar-vin driver is starting to 
reaching a point where the whole format handling for buffers, source 
format, croping and scaling needs to be rewritten to enable more valid 
use-cases.

This fix is however in my view required at this time with the current 
driver design. If we keep aligning the width at stream on and enable the 
UDS it becomes apparent that when the alignment is needed the values for 
the stride register conflicts which how the scaling coefficients are 
calculated and the captured frame is distorted.

My hope is to add upstream to support for the UDS, support for 
sequential field captures and some more output pixel formats. And once 
the driver feature complete on Gen3 come back and simplify and if 
possible align the Gen2 and Gen3 format handling which in part adds to 
the somewhat messy current state.

> 
> > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-dma.c  | 5 +----
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c | 9 +++++++++
> >  2 files changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> > b/drivers/media/platform/rcar-vin/rcar-dma.c index
> > 92323310f7352147..e752bc86e40153b1 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -597,10 +597,7 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
> >  	if (vin->info->model != RCAR_GEN3)
> >  		rvin_crop_scale_comp_gen2(vin);
> > 
> > -	if (vin->format.pixelformat == V4L2_PIX_FMT_NV16)
> > -		rvin_write(vin, ALIGN(vin->format.width, 0x20), VNIS_REG);
> > -	else
> > -		rvin_write(vin, ALIGN(vin->format.width, 0x10), VNIS_REG);
> > +	rvin_write(vin, vin->format.width, VNIS_REG);
> >  }
> > 
> >  /*
> > ---------------------------------------------------------------------------
> > -- diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> > dc77682b47857c97..94bc559a0cb1e47a 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -96,6 +96,15 @@ static void rvin_format_align(struct rvin_dev *vin,
> > struct v4l2_pix_format *pix) pix->pixelformat == V4L2_PIX_FMT_XBGR32))
> >  		pix->pixelformat = RVIN_DEFAULT_FORMAT;
> > 
> > +	switch (pix->pixelformat) {
> > +	case V4L2_PIX_FMT_NV16:
> > +		pix->width = ALIGN(pix->width, 0x20);
> > +		break;
> > +	default:
> > +		pix->width = ALIGN(pix->width, 0x10);
> > +		break;
> > +	}
> > +
> >  	switch (pix->field) {
> >  	case V4L2_FIELD_TOP:
> >  	case V4L2_FIELD_BOTTOM:
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH v2 2/3] rcar-vin: add support for UDS (Up Down Scaler)

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Thursday, 4 October 2018 23:04:01 EEST Niklas Söderlund wrote:
> Some VIN instances have access to a Up Down Scaler (UDS). The UDS are on
> most SoCs shared between two VINs, the UDS can of course only be used by
> one VIN at a time.

I would drop the "of course" as it's not so evident.

s/the UDS can of course only/but a UDS can only/

> Add support to configure the UDS registers which are mapped to both VINs
> sharing the UDS address-space. While validations the format at stream

s/validations/validating/

> start make sure the companion VIN is not already using the scaler. If
> the scaler already is in use return -EBUSY.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 134 ++++++++++++++++++++-
>  drivers/media/platform/rcar-vin/rcar-vin.h |  24 ++++
>  2 files changed, 152 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> b/drivers/media/platform/rcar-vin/rcar-dma.c index
> e752bc86e40153b1..f33146bda9300c21 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -74,6 +74,10 @@
> 
>  /* Register offsets specific for Gen3 */
>  #define VNCSI_IFMD_REG		0x20 /* Video n CSI2 Interface Mode Register */
> +#define VNUDS_CTRL_REG		0x80 /* Video n scaling control register */
> +#define VNUDS_SCALE_REG		0x84 /* Video n scaling factor register */
> +#define VNUDS_PASS_BWIDTH_REG	0x90 /* Video n passband register */
> +#define VNUDS_CLIP_SIZE_REG	0xa4 /* Video n UDS output size clipping reg */
> 
>  /* Register bit fields for R-Car VIN */
>  /* Video n Main Control Register bits */
> @@ -129,6 +133,9 @@
>  #define VNCSI_IFMD_CSI_CHSEL(n) (((n) & 0xf) << 0)
>  #define VNCSI_IFMD_CSI_CHSEL_MASK 0xf
> 
> +/* Video n scaling control register (Gen3) */
> +#define VNUDS_CTRL_AMD		(1 << 30)
> +
>  struct rvin_buffer {
>  	struct vb2_v4l2_buffer vb;
>  	struct list_head list;
> @@ -572,6 +579,78 @@ static void rvin_crop_scale_comp_gen2(struct rvin_dev
> *vin) 0, 0);
>  }
> 
> +static unsigned int rvin_scale_ratio(unsigned int in, unsigned int out)
> +{
> +	unsigned int ratio;
> +
> +	ratio = in * 4096 / out;
> +	return ratio >= 0x10000 ? 0xffff : ratio;
> +}
> +
> +static unsigned int rvin_ratio_to_bwidth(unsigned int ratio)

I initially thought this referred to a bandwidth as in a throughput. How about 
renaming the function to rvin_ratio_to_filter_width() ? Or possibly 
standardize the UDS function naming to rvin_uds_*, and name it 
rvin_uds_filter_width() or rvin_uds_passband_width() ?

> +{
> +	unsigned int mant, frac;
> +
> +	mant = (ratio & 0xF000) >> 12;
> +	frac = ratio & 0x0FFF;

Maybe lowercase hex constants ?

> +	if (mant)
> +		return 64 * 4096 * mant / (4096 * mant + frac);

Isn't the denumerator equal to ratio ? How about

	if (ratio >= 0x1000)
		return 64 * (ratio & 0xf000) / ratio;
	else
		return 64;

> +	return 64;
> +}
> +
> +static bool rvin_gen3_need_scaling(struct rvin_dev *vin)
> +{
> +	if (vin->info->model != RCAR_GEN3)
> +		return false;

One of the two callers don't need this check, so you could move it to the 
caller that does.

> +	return vin->crop.width != vin->format.width ||
> +		vin->crop.height != vin->format.height;
> +}
> +
> +static void rvin_crop_scale_comp_gen3(struct rvin_dev *vin)
> +{
> +	unsigned int ratio_h, ratio_v;
> +	unsigned int bwidth_h, bwidth_v;
> +	u32 vnmc, clip_size;
> +
> +	if (!rvin_gen3_need_scaling(vin))
> +		return;
> +
> +	ratio_h = rvin_scale_ratio(vin->crop.width, vin->format.width);

Is there anything that prevents the output width to be so massively bigger 
than the input width to result in a ratio of 0, crashing the passband filter 
width computation ?

> +	bwidth_h = rvin_ratio_to_bwidth(ratio_h);
> +
> +	ratio_v = rvin_scale_ratio(vin->crop.height, vin->format.height);
> +	bwidth_v = rvin_ratio_to_bwidth(ratio_v);
> +
> +	clip_size = vin->format.width << 16;
> +
> +	switch (vin->format.field) {
> +	case V4L2_FIELD_INTERLACED_TB:
> +	case V4L2_FIELD_INTERLACED_BT:
> +	case V4L2_FIELD_INTERLACED:
> +	case V4L2_FIELD_SEQ_TB:
> +	case V4L2_FIELD_SEQ_BT:
> +		clip_size |= vin->format.height / 2;
> +		break;
> +	default:
> +		clip_size |= vin->format.height;
> +		break;
> +	}
> +
> +	vnmc = rvin_read(vin, VNMC_REG);
> +	rvin_write(vin, (vnmc & ~VNMC_VUP) | VNMC_SCLE, VNMC_REG);
> +	rvin_write(vin, VNUDS_CTRL_AMD, VNUDS_CTRL_REG);
> +	rvin_write(vin, (ratio_h << 16) | ratio_v, VNUDS_SCALE_REG);
> +	rvin_write(vin, (bwidth_h << 16) | bwidth_v, VNUDS_PASS_BWIDTH_REG);
> +	rvin_write(vin, clip_size, VNUDS_CLIP_SIZE_REG);
> +	rvin_write(vin, vnmc, VNMC_REG);
> +
> +	vin_dbg(vin, "Pre-Clip: %ux%u@%u:%u Post-Clip: %ux%u@%u:%u\n",
> +		vin->crop.width, vin->crop.height, vin->crop.left,
> +		vin->crop.top, vin->format.width, vin->format.height, 0, 0);

Now that you have (hopefully :-)) debugged the code, do you still need this ?

> +}
> +
>  void rvin_crop_scale_comp(struct rvin_dev *vin)
>  {
>  	/* Set Start/End Pixel/Line Pre-Clip */
> @@ -593,8 +672,9 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
>  		break;
>  	}
> 
> -	/* TODO: Add support for the UDS scaler. */
> -	if (vin->info->model != RCAR_GEN3)
> +	if (vin->info->model == RCAR_GEN3)
> +		rvin_crop_scale_comp_gen3(vin);
> +	else
>  		rvin_crop_scale_comp_gen2(vin);
> 
>  	rvin_write(vin, vin->format.width, VNIS_REG);
> @@ -748,6 +828,9 @@ static int rvin_setup(struct rvin_dev *vin)
>  			vnmc |= VNMC_DPINE;
>  	}
> 
> +	if (rvin_gen3_need_scaling(vin))
> +		vnmc |= VNMC_SCLE;
> +
>  	/* Progressive or interlaced mode */
>  	interrupts = progressive ? VNIE_FIE : VNIE_EFE;
> 
> @@ -1078,10 +1161,42 @@ static int rvin_mc_validate_format(struct rvin_dev
> *vin, struct v4l2_subdev *sd, return -EPIPE;
>  	}
> 
> -	if (fmt.format.width != vin->format.width ||
> -	    fmt.format.height != vin->format.height ||
> -	    fmt.format.code != vin->mbus_code)
> -		return -EPIPE;
> +	vin->crop.width = fmt.format.width;
> +	vin->crop.height = fmt.format.height;

Isn't the crop rectangle supposed to come from userspace ?

> +	if (rvin_gen3_need_scaling(vin)) {
> +		const struct rvin_group_scaler *scaler;
> +		struct rvin_dev *companion;
> +
> +		if (fmt.format.code != vin->mbus_code)
> +			return -EPIPE;

As this check is needed in both cases I'd move it above the scaling check.

> +		if (!vin->info->scalers)
> +			return -EPIPE;
> +
> +		for (scaler = vin->info->scalers;
> +		     scaler->vin || scaler->companion; scaler++)
> +			if (scaler->vin == vin->id)
> +				break;
> +
> +		/* No scaler found for VIN. */
> +		if (!scaler->vin && !scaler->companion)
> +			return -EPIPE;
> +
> +		/* Make sure companion not using scaler. */
> +		if (scaler->companion != -1) {
> +			companion = vin->group->vin[scaler->companion];
> +			if (companion &&
> +			    companion->state != STOPPED &&
> +			    rvin_gen3_need_scaling(companion))
> +				return -EBUSY;

Without any locking, this screams of a race condition :-)

> +		}
> +	} else {
> +		if (fmt.format.width != vin->format.width ||
> +		    fmt.format.height != vin->format.height ||
> +		    fmt.format.code != vin->mbus_code)
> +			return -EPIPE;
> +	}
> 
>  	return 0;
>  }
> @@ -1189,6 +1304,7 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
>  	struct rvin_dev *vin = vb2_get_drv_priv(vq);
>  	unsigned long flags;
>  	int retries = 0;
> +	u32 vnmc;
> 
>  	spin_lock_irqsave(&vin->qlock, flags);
> 
> @@ -1220,6 +1336,12 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
> vin->state = STOPPED;
>  	}
> 
> +	/* Clear UDS usage after we have stopped */
> +	if (vin->info->model == RCAR_GEN3) {
> +		vnmc = rvin_read(vin, VNMC_REG) & ~(VNMC_SCLE | VNMC_VUP);
> +		rvin_write(vin, vnmc, VNMC_REG);
> +	}

You have quite a few read-modify-write sequences for the VNMC register in the 
driver, would it make sense to cache its value ?

>  	/* Release all active buffers */
>  	return_all_buffers(vin, VB2_BUF_STATE_ERROR);
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h
> b/drivers/media/platform/rcar-vin/rcar-vin.h index
> 0b13b34d03e3dce4..5a617a30ba8c9a5a 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -122,6 +122,28 @@ struct rvin_group_route {
>  	unsigned int mask;
>  };
> 
> +/**
> + * struct rvin_group_scaler - describes a scaler attached to a VIN
> + *
> + * @vin:	Numerical VIN id that have access to a UDS.

s/have/has/

> + * @companion:  Numerical VIN id that @vin share the UDS with.

s/share/shares/

And I think I would write "Numerical ID of the VIN ..." for both.

> + *
> + * -- note::
> + *	Some R-Car VIN instances have access to a Up Down Scaler (UDS).
> + *	If a VIN have a UDS attached it's almost always shared between

s/have/has/

> + *	two VIN instances. The UDS can only be used by one VIN at a time,
> + *	so the companion relationship needs to be described as well.
> + *
> + *	There are at most two VINs sharing a UDS. For each UDS shared
> + *	between two VINs there needs to be two instances of struct
> + *	rvin_group_scaler describing each of the VINs individually. If
> + *	a VIN do not share its UDS set companion to -1.

s/do/does/
s/companion/@companion/

> + */
> +struct rvin_group_scaler {
> +	int vin;

unsigned int ?

> +	int companion;
> +};
> +
>  /**
>   * struct rvin_info - Information about the particular VIN implementation
>   * @model:		VIN model
> @@ -130,6 +152,7 @@ struct rvin_group_route {
>   * @max_height:		max input height the VIN supports
>   * @routes:		list of possible routes from the CSI-2 recivers to
>   *			all VINs. The list mush be NULL terminated.
> + * @scalers:		List of available scalers, must be NULL terminated.
>   */
>  struct rvin_info {
>  	enum model_id model;
> @@ -138,6 +161,7 @@ struct rvin_info {
>  	unsigned int max_width;
>  	unsigned int max_height;
>  	const struct rvin_group_route *routes;
> +	const struct rvin_group_scaler *scalers;
>  };
> 
>  /**

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/3] rcar-vin: align width before stream start

[Laurent Pinchart]

Hi Niklas,

On Thursday, 4 October 2018 23:29:51 EEST Niklas Söderlund wrote:
> On 2018-10-04 23:11:50 +0300, Laurent Pinchart wrote:
> > On Thursday, 4 October 2018 23:04:00 EEST Niklas Söderlund wrote:
> >> Instead of aligning the image width to match the image stride at stream
> >> start time do so when configuring the format. This allows the format
> >> width to strictly match the image stride which is useful when enabling
> >> scaling on Gen3.
> > 
> > But is this required ? Aren't there use cases where an image with a width
> > not aligned with the stride requirements should be captured ? As long as
> > the stride itself matches the hardware requirements, I don't see a reason
> > to disallow that.
> 
> Yes there is a use-case for that. And the rcar-vin driver is starting to
> reaching a point where the whole format handling for buffers, source
> format, croping and scaling needs to be rewritten to enable more valid
> use-cases.
> 
> This fix is however in my view required at this time with the current
> driver design. If we keep aligning the width at stream on and enable the
> UDS it becomes apparent that when the alignment is needed the values for
> the stride register conflicts which how the scaling coefficients are
> calculated and the captured frame is distorted.
> 
> My hope is to add upstream to support for the UDS, support for
> sequential field captures and some more output pixel formats. And once
> the driver feature complete on Gen3 come back and simplify and if
> possible align the Gen2 and Gen3 format handling which in part adds to
> the somewhat messy current state.

I'd argue that it would be better to do it the other way around, but I won't 
block this patch series. You might however get angry e-mails from VIN users 
who all of a sudden realize that their use case stopped functioning.

> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > ---
> > > 
> > >  drivers/media/platform/rcar-vin/rcar-dma.c  | 5 +----
> > >  drivers/media/platform/rcar-vin/rcar-v4l2.c | 9 +++++++++
> > >  2 files changed, 10 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> > > b/drivers/media/platform/rcar-vin/rcar-dma.c index
> > > 92323310f7352147..e752bc86e40153b1 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > > @@ -597,10 +597,7 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
> > > 
> > >  	if (vin->info->model != RCAR_GEN3)
> > >  	
> > >  		rvin_crop_scale_comp_gen2(vin);
> > > 
> > > -	if (vin->format.pixelformat == V4L2_PIX_FMT_NV16)
> > > -		rvin_write(vin, ALIGN(vin->format.width, 0x20), VNIS_REG);
> > > -	else
> > > -		rvin_write(vin, ALIGN(vin->format.width, 0x10), VNIS_REG);
> > > +	rvin_write(vin, vin->format.width, VNIS_REG);
> > > 
> > >  }
> > >  
> > >  /*
> > > 
> > > ------------------------------------------------------------------------
> > > ---
> > > -- diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > b/drivers/media/platform/rcar-vin/rcar-v4l2.c index
> > > dc77682b47857c97..94bc559a0cb1e47a 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > > @@ -96,6 +96,15 @@ static void rvin_format_align(struct rvin_dev *vin,
> > > struct v4l2_pix_format *pix) pix->pixelformat == V4L2_PIX_FMT_XBGR32))
> > > 
> > >  		pix->pixelformat = RVIN_DEFAULT_FORMAT;
> > > 
> > > +	switch (pix->pixelformat) {
> > > +	case V4L2_PIX_FMT_NV16:
> > > +		pix->width = ALIGN(pix->width, 0x20);
> > > +		break;
> > > +	default:
> > > +		pix->width = ALIGN(pix->width, 0x10);
> > > +		break;
> > > +	}
> > > +
> > > 
> > >  	switch (pix->field) {
> > >  	case V4L2_FIELD_TOP:
> > >  	case V4L2_FIELD_BOTTOM:


-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 2/3] rcar-vin: add support for UDS (Up Down Scaler)

[Sakari Ailus]

Hi Niklas,

On Thu, Oct 04, 2018 at 10:04:01PM +0200, Niklas Söderlund wrote:
> Some VIN instances have access to a Up Down Scaler (UDS). The UDS are on
> most SoCs shared between two VINs, the UDS can of course only be used by
> one VIN at a time.
> 
> Add support to configure the UDS registers which are mapped to both VINs
> sharing the UDS address-space. While validations the format at stream
> start make sure the companion VIN is not already using the scaler. If
> the scaler already is in use return -EBUSY.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 134 ++++++++++++++++++++-
>  drivers/media/platform/rcar-vin/rcar-vin.h |  24 ++++
>  2 files changed, 152 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index e752bc86e40153b1..f33146bda9300c21 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -74,6 +74,10 @@
>  
>  /* Register offsets specific for Gen3 */
>  #define VNCSI_IFMD_REG		0x20 /* Video n CSI2 Interface Mode Register */
> +#define VNUDS_CTRL_REG		0x80 /* Video n scaling control register */
> +#define VNUDS_SCALE_REG		0x84 /* Video n scaling factor register */
> +#define VNUDS_PASS_BWIDTH_REG	0x90 /* Video n passband register */
> +#define VNUDS_CLIP_SIZE_REG	0xa4 /* Video n UDS output size clipping reg */
>  
>  /* Register bit fields for R-Car VIN */
>  /* Video n Main Control Register bits */
> @@ -129,6 +133,9 @@
>  #define VNCSI_IFMD_CSI_CHSEL(n) (((n) & 0xf) << 0)
>  #define VNCSI_IFMD_CSI_CHSEL_MASK 0xf
>  
> +/* Video n scaling control register (Gen3) */
> +#define VNUDS_CTRL_AMD		(1 << 30)
> +
>  struct rvin_buffer {
>  	struct vb2_v4l2_buffer vb;
>  	struct list_head list;
> @@ -572,6 +579,78 @@ static void rvin_crop_scale_comp_gen2(struct rvin_dev *vin)
>  		0, 0);
>  }
>  
> +static unsigned int rvin_scale_ratio(unsigned int in, unsigned int out)
> +{
> +	unsigned int ratio;
> +
> +	ratio = in * 4096 / out;
> +	return ratio >= 0x10000 ? 0xffff : ratio;
> +}
> +
> +static unsigned int rvin_ratio_to_bwidth(unsigned int ratio)
> +{
> +	unsigned int mant, frac;
> +
> +	mant = (ratio & 0xF000) >> 12;
> +	frac = ratio & 0x0FFF;
> +	if (mant)
> +		return 64 * 4096 * mant / (4096 * mant + frac);
> +
> +	return 64;
> +}
> +
> +static bool rvin_gen3_need_scaling(struct rvin_dev *vin)
> +{
> +	if (vin->info->model != RCAR_GEN3)
> +		return false;
> +
> +	return vin->crop.width != vin->format.width ||
> +		vin->crop.height != vin->format.height;
> +}
> +
> +static void rvin_crop_scale_comp_gen3(struct rvin_dev *vin)
> +{
> +	unsigned int ratio_h, ratio_v;
> +	unsigned int bwidth_h, bwidth_v;
> +	u32 vnmc, clip_size;
> +
> +	if (!rvin_gen3_need_scaling(vin))
> +		return;
> +
> +	ratio_h = rvin_scale_ratio(vin->crop.width, vin->format.width);
> +	bwidth_h = rvin_ratio_to_bwidth(ratio_h);
> +
> +	ratio_v = rvin_scale_ratio(vin->crop.height, vin->format.height);
> +	bwidth_v = rvin_ratio_to_bwidth(ratio_v);
> +
> +	clip_size = vin->format.width << 16;
> +
> +	switch (vin->format.field) {
> +	case V4L2_FIELD_INTERLACED_TB:
> +	case V4L2_FIELD_INTERLACED_BT:
> +	case V4L2_FIELD_INTERLACED:
> +	case V4L2_FIELD_SEQ_TB:
> +	case V4L2_FIELD_SEQ_BT:
> +		clip_size |= vin->format.height / 2;
> +		break;
> +	default:
> +		clip_size |= vin->format.height;
> +		break;
> +	}
> +
> +	vnmc = rvin_read(vin, VNMC_REG);
> +	rvin_write(vin, (vnmc & ~VNMC_VUP) | VNMC_SCLE, VNMC_REG);
> +	rvin_write(vin, VNUDS_CTRL_AMD, VNUDS_CTRL_REG);
> +	rvin_write(vin, (ratio_h << 16) | ratio_v, VNUDS_SCALE_REG);
> +	rvin_write(vin, (bwidth_h << 16) | bwidth_v, VNUDS_PASS_BWIDTH_REG);
> +	rvin_write(vin, clip_size, VNUDS_CLIP_SIZE_REG);
> +	rvin_write(vin, vnmc, VNMC_REG);
> +
> +	vin_dbg(vin, "Pre-Clip: %ux%u@%u:%u Post-Clip: %ux%u@%u:%u\n",
> +		vin->crop.width, vin->crop.height, vin->crop.left,
> +		vin->crop.top, vin->format.width, vin->format.height, 0, 0);
> +}

How's this visible to the user? I'd expect this to be configurable using
the COMPOSE selection target.

And... how about the support for the file handle specific try
configuration?

> +
>  void rvin_crop_scale_comp(struct rvin_dev *vin)
>  {
>  	/* Set Start/End Pixel/Line Pre-Clip */
> @@ -593,8 +672,9 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
>  		break;
>  	}
>  
> -	/* TODO: Add support for the UDS scaler. */
> -	if (vin->info->model != RCAR_GEN3)
> +	if (vin->info->model == RCAR_GEN3)
> +		rvin_crop_scale_comp_gen3(vin);
> +	else
>  		rvin_crop_scale_comp_gen2(vin);
>  
>  	rvin_write(vin, vin->format.width, VNIS_REG);
> @@ -748,6 +828,9 @@ static int rvin_setup(struct rvin_dev *vin)
>  			vnmc |= VNMC_DPINE;
>  	}
>  
> +	if (rvin_gen3_need_scaling(vin))
> +		vnmc |= VNMC_SCLE;
> +
>  	/* Progressive or interlaced mode */
>  	interrupts = progressive ? VNIE_FIE : VNIE_EFE;
>  
> @@ -1078,10 +1161,42 @@ static int rvin_mc_validate_format(struct rvin_dev *vin, struct v4l2_subdev *sd,
>  		return -EPIPE;
>  	}
>  
> -	if (fmt.format.width != vin->format.width ||
> -	    fmt.format.height != vin->format.height ||
> -	    fmt.format.code != vin->mbus_code)
> -		return -EPIPE;
> +	vin->crop.width = fmt.format.width;
> +	vin->crop.height = fmt.format.height;
> +
> +	if (rvin_gen3_need_scaling(vin)) {
> +		const struct rvin_group_scaler *scaler;
> +		struct rvin_dev *companion;
> +
> +		if (fmt.format.code != vin->mbus_code)
> +			return -EPIPE;
> +
> +		if (!vin->info->scalers)
> +			return -EPIPE;
> +
> +		for (scaler = vin->info->scalers;
> +		     scaler->vin || scaler->companion; scaler++)
> +			if (scaler->vin == vin->id)
> +				break;
> +
> +		/* No scaler found for VIN. */
> +		if (!scaler->vin && !scaler->companion)
> +			return -EPIPE;
> +
> +		/* Make sure companion not using scaler. */
> +		if (scaler->companion != -1) {
> +			companion = vin->group->vin[scaler->companion];
> +			if (companion &&
> +			    companion->state != STOPPED &&
> +			    rvin_gen3_need_scaling(companion))
> +				return -EBUSY;
> +		}
> +	} else {
> +		if (fmt.format.width != vin->format.width ||
> +		    fmt.format.height != vin->format.height ||
> +		    fmt.format.code != vin->mbus_code)
> +			return -EPIPE;
> +	}
>  
>  	return 0;
>  }
> @@ -1189,6 +1304,7 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
>  	struct rvin_dev *vin = vb2_get_drv_priv(vq);
>  	unsigned long flags;
>  	int retries = 0;
> +	u32 vnmc;
>  
>  	spin_lock_irqsave(&vin->qlock, flags);
>  
> @@ -1220,6 +1336,12 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
>  		vin->state = STOPPED;
>  	}
>  
> +	/* Clear UDS usage after we have stopped */
> +	if (vin->info->model == RCAR_GEN3) {
> +		vnmc = rvin_read(vin, VNMC_REG) & ~(VNMC_SCLE | VNMC_VUP);
> +		rvin_write(vin, vnmc, VNMC_REG);
> +	}
> +
>  	/* Release all active buffers */
>  	return_all_buffers(vin, VB2_BUF_STATE_ERROR);
>  
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index 0b13b34d03e3dce4..5a617a30ba8c9a5a 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -122,6 +122,28 @@ struct rvin_group_route {
>  	unsigned int mask;
>  };
>  
> +/**
> + * struct rvin_group_scaler - describes a scaler attached to a VIN
> + *
> + * @vin:	Numerical VIN id that have access to a UDS.
> + * @companion:  Numerical VIN id that @vin share the UDS with.
> + *
> + * -- note::
> + *	Some R-Car VIN instances have access to a Up Down Scaler (UDS).
> + *	If a VIN have a UDS attached it's almost always shared between
> + *	two VIN instances. The UDS can only be used by one VIN at a time,
> + *	so the companion relationship needs to be described as well.
> + *
> + *	There are at most two VINs sharing a UDS. For each UDS shared
> + *	between two VINs there needs to be two instances of struct
> + *	rvin_group_scaler describing each of the VINs individually. If
> + *	a VIN do not share its UDS set companion to -1.
> + */
> +struct rvin_group_scaler {
> +	int vin;
> +	int companion;
> +};
> +
>  /**
>   * struct rvin_info - Information about the particular VIN implementation
>   * @model:		VIN model
> @@ -130,6 +152,7 @@ struct rvin_group_route {
>   * @max_height:		max input height the VIN supports
>   * @routes:		list of possible routes from the CSI-2 recivers to
>   *			all VINs. The list mush be NULL terminated.
> + * @scalers:		List of available scalers, must be NULL terminated.
>   */
>  struct rvin_info {
>  	enum model_id model;
> @@ -138,6 +161,7 @@ struct rvin_info {
>  	unsigned int max_width;
>  	unsigned int max_height;
>  	const struct rvin_group_route *routes;
> +	const struct rvin_group_scaler *scalers;
>  };
>  
>  /**

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH v2 2/3] rcar-vin: add support for UDS (Up Down Scaler)

[Sakari Ailus]

Hi Niklas,

On Thu, Oct 04, 2018 at 10:04:01PM +0200, Niklas S�derlund wrote:
> Some VIN instances have access to a Up Down Scaler (UDS). The UDS are on
> most SoCs shared between two VINs, the UDS can of course only be used by
> one VIN at a time.
> 
> Add support to configure the UDS registers which are mapped to both VINs
> sharing the UDS address-space. While validations the format at stream
> start make sure the companion VIN is not already using the scaler. If
> the scaler already is in use return -EBUSY.
> 
> Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 134 ++++++++++++++++++++-
>  drivers/media/platform/rcar-vin/rcar-vin.h |  24 ++++
>  2 files changed, 152 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index e752bc86e40153b1..f33146bda9300c21 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -74,6 +74,10 @@
>  
>  /* Register offsets specific for Gen3 */
>  #define VNCSI_IFMD_REG		0x20 /* Video n CSI2 Interface Mode Register */
> +#define VNUDS_CTRL_REG		0x80 /* Video n scaling control register */
> +#define VNUDS_SCALE_REG		0x84 /* Video n scaling factor register */
> +#define VNUDS_PASS_BWIDTH_REG	0x90 /* Video n passband register */
> +#define VNUDS_CLIP_SIZE_REG	0xa4 /* Video n UDS output size clipping reg */
>  
>  /* Register bit fields for R-Car VIN */
>  /* Video n Main Control Register bits */
> @@ -129,6 +133,9 @@
>  #define VNCSI_IFMD_CSI_CHSEL(n) (((n) & 0xf) << 0)
>  #define VNCSI_IFMD_CSI_CHSEL_MASK 0xf
>  
> +/* Video n scaling control register (Gen3) */
> +#define VNUDS_CTRL_AMD		(1 << 30)
> +
>  struct rvin_buffer {
>  	struct vb2_v4l2_buffer vb;
>  	struct list_head list;
> @@ -572,6 +579,78 @@ static void rvin_crop_scale_comp_gen2(struct rvin_dev *vin)
>  		0, 0);
>  }
>  
> +static unsigned int rvin_scale_ratio(unsigned int in, unsigned int out)
> +{
> +	unsigned int ratio;
> +
> +	ratio = in * 4096 / out;
> +	return ratio >= 0x10000 ? 0xffff : ratio;
> +}
> +
> +static unsigned int rvin_ratio_to_bwidth(unsigned int ratio)
> +{
> +	unsigned int mant, frac;
> +
> +	mant = (ratio & 0xF000) >> 12;
> +	frac = ratio & 0x0FFF;
> +	if (mant)
> +		return 64 * 4096 * mant / (4096 * mant + frac);
> +
> +	return 64;
> +}
> +
> +static bool rvin_gen3_need_scaling(struct rvin_dev *vin)
> +{
> +	if (vin->info->model != RCAR_GEN3)
> +		return false;
> +
> +	return vin->crop.width != vin->format.width ||
> +		vin->crop.height != vin->format.height;
> +}
> +
> +static void rvin_crop_scale_comp_gen3(struct rvin_dev *vin)
> +{
> +	unsigned int ratio_h, ratio_v;
> +	unsigned int bwidth_h, bwidth_v;
> +	u32 vnmc, clip_size;
> +
> +	if (!rvin_gen3_need_scaling(vin))
> +		return;
> +
> +	ratio_h = rvin_scale_ratio(vin->crop.width, vin->format.width);
> +	bwidth_h = rvin_ratio_to_bwidth(ratio_h);
> +
> +	ratio_v = rvin_scale_ratio(vin->crop.height, vin->format.height);
> +	bwidth_v = rvin_ratio_to_bwidth(ratio_v);
> +
> +	clip_size = vin->format.width << 16;
> +
> +	switch (vin->format.field) {
> +	case V4L2_FIELD_INTERLACED_TB:
> +	case V4L2_FIELD_INTERLACED_BT:
> +	case V4L2_FIELD_INTERLACED:
> +	case V4L2_FIELD_SEQ_TB:
> +	case V4L2_FIELD_SEQ_BT:
> +		clip_size |= vin->format.height / 2;
> +		break;
> +	default:
> +		clip_size |= vin->format.height;
> +		break;
> +	}
> +
> +	vnmc = rvin_read(vin, VNMC_REG);
> +	rvin_write(vin, (vnmc & ~VNMC_VUP) | VNMC_SCLE, VNMC_REG);
> +	rvin_write(vin, VNUDS_CTRL_AMD, VNUDS_CTRL_REG);
> +	rvin_write(vin, (ratio_h << 16) | ratio_v, VNUDS_SCALE_REG);
> +	rvin_write(vin, (bwidth_h << 16) | bwidth_v, VNUDS_PASS_BWIDTH_REG);
> +	rvin_write(vin, clip_size, VNUDS_CLIP_SIZE_REG);
> +	rvin_write(vin, vnmc, VNMC_REG);
> +
> +	vin_dbg(vin, "Pre-Clip: %ux%u@%u:%u Post-Clip: %ux%u@%u:%u\n",
> +		vin->crop.width, vin->crop.height, vin->crop.left,
> +		vin->crop.top, vin->format.width, vin->format.height, 0, 0);
> +}

How's this visible to the user? I'd expect this to be configurable using
the COMPOSE selection target.

And... how about the support for the file handle specific try
configuration?

> +
>  void rvin_crop_scale_comp(struct rvin_dev *vin)
>  {
>  	/* Set Start/End Pixel/Line Pre-Clip */
> @@ -593,8 +672,9 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
>  		break;
>  	}
>  
> -	/* TODO: Add support for the UDS scaler. */
> -	if (vin->info->model != RCAR_GEN3)
> +	if (vin->info->model == RCAR_GEN3)
> +		rvin_crop_scale_comp_gen3(vin);
> +	else
>  		rvin_crop_scale_comp_gen2(vin);
>  
>  	rvin_write(vin, vin->format.width, VNIS_REG);
> @@ -748,6 +828,9 @@ static int rvin_setup(struct rvin_dev *vin)
>  			vnmc |= VNMC_DPINE;
>  	}
>  
> +	if (rvin_gen3_need_scaling(vin))
> +		vnmc |= VNMC_SCLE;
> +
>  	/* Progressive or interlaced mode */
>  	interrupts = progressive ? VNIE_FIE : VNIE_EFE;
>  
> @@ -1078,10 +1161,42 @@ static int rvin_mc_validate_format(struct rvin_dev *vin, struct v4l2_subdev *sd,
>  		return -EPIPE;
>  	}
>  
> -	if (fmt.format.width != vin->format.width ||
> -	    fmt.format.height != vin->format.height ||
> -	    fmt.format.code != vin->mbus_code)
> -		return -EPIPE;
> +	vin->crop.width = fmt.format.width;
> +	vin->crop.height = fmt.format.height;
> +
> +	if (rvin_gen3_need_scaling(vin)) {
> +		const struct rvin_group_scaler *scaler;
> +		struct rvin_dev *companion;
> +
> +		if (fmt.format.code != vin->mbus_code)
> +			return -EPIPE;
> +
> +		if (!vin->info->scalers)
> +			return -EPIPE;
> +
> +		for (scaler = vin->info->scalers;
> +		     scaler->vin || scaler->companion; scaler++)
> +			if (scaler->vin == vin->id)
> +				break;
> +
> +		/* No scaler found for VIN. */
> +		if (!scaler->vin && !scaler->companion)
> +			return -EPIPE;
> +
> +		/* Make sure companion not using scaler. */
> +		if (scaler->companion != -1) {
> +			companion = vin->group->vin[scaler->companion];
> +			if (companion &&
> +			    companion->state != STOPPED &&
> +			    rvin_gen3_need_scaling(companion))
> +				return -EBUSY;
> +		}
> +	} else {
> +		if (fmt.format.width != vin->format.width ||
> +		    fmt.format.height != vin->format.height ||
> +		    fmt.format.code != vin->mbus_code)
> +			return -EPIPE;
> +	}
>  
>  	return 0;
>  }
> @@ -1189,6 +1304,7 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
>  	struct rvin_dev *vin = vb2_get_drv_priv(vq);
>  	unsigned long flags;
>  	int retries = 0;
> +	u32 vnmc;
>  
>  	spin_lock_irqsave(&vin->qlock, flags);
>  
> @@ -1220,6 +1336,12 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
>  		vin->state = STOPPED;
>  	}
>  
> +	/* Clear UDS usage after we have stopped */
> +	if (vin->info->model == RCAR_GEN3) {
> +		vnmc = rvin_read(vin, VNMC_REG) & ~(VNMC_SCLE | VNMC_VUP);
> +		rvin_write(vin, vnmc, VNMC_REG);
> +	}
> +
>  	/* Release all active buffers */
>  	return_all_buffers(vin, VB2_BUF_STATE_ERROR);
>  
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index 0b13b34d03e3dce4..5a617a30ba8c9a5a 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -122,6 +122,28 @@ struct rvin_group_route {
>  	unsigned int mask;
>  };
>  
> +/**
> + * struct rvin_group_scaler - describes a scaler attached to a VIN
> + *
> + * @vin:	Numerical VIN id that have access to a UDS.
> + * @companion:  Numerical VIN id that @vin share the UDS with.
> + *
> + * -- note::
> + *	Some R-Car VIN instances have access to a Up Down Scaler (UDS).
> + *	If a VIN have a UDS attached it's almost always shared between
> + *	two VIN instances. The UDS can only be used by one VIN at a time,
> + *	so the companion relationship needs to be described as well.
> + *
> + *	There are at most two VINs sharing a UDS. For each UDS shared
> + *	between two VINs there needs to be two instances of struct
> + *	rvin_group_scaler describing each of the VINs individually. If
> + *	a VIN do not share its UDS set companion to -1.
> + */
> +struct rvin_group_scaler {
> +	int vin;
> +	int companion;
> +};
> +
>  /**
>   * struct rvin_info - Information about the particular VIN implementation
>   * @model:		VIN model
> @@ -130,6 +152,7 @@ struct rvin_group_route {
>   * @max_height:		max input height the VIN supports
>   * @routes:		list of possible routes from the CSI-2 recivers to
>   *			all VINs. The list mush be NULL terminated.
> + * @scalers:		List of available scalers, must be NULL terminated.
>   */
>  struct rvin_info {
>  	enum model_id model;
> @@ -138,6 +161,7 @@ struct rvin_info {
>  	unsigned int max_width;
>  	unsigned int max_height;
>  	const struct rvin_group_route *routes;
> +	const struct rvin_group_scaler *scalers;
>  };
>  
>  /**

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi