[PATCH 0/6] Fix adv7180 and rcar-vin field handling

[PATCH 0/6] Fix adv7180 and rcar-vin field handling

[Niklas Söderlund]

Hi,

This series add V4L2_FIELD_ALTERNATE support to the rcar-vin driver and 
changes the field mode reported by adv7180 from V4L2_FIELD_INTERLACED to 
V4L2_FIELD_ALTERNATE.

The change field mode reported by adv7180 was first done by Steve 
Longerbeam (https://lkml.org/lkml/2016/7/23/107), I have keept and 
reworked Steves patch to report V4L2_FIELD_ALTERNATE instead of 
V4L2_FIELD_SEQ_{TB,BT}, after discussions on #v4l this seems more
correct.

The rcar-vin changes contains some bug fixes needed to enable 
V4L2_FIELD_ALTERNATE.

All work is based on top of media-next and is tested on Koelsch.

This series touch two drivers which is not a good thing. But I could not 
figure out a good way to post them separately since if the adv7180 parts 
where too be merged before the rcar-vin changes the driver would stop to 
work on the Koelsch. If some one wants this series split in two let me 
know.

Niklas Söderlund (5):
  media: rcar-vin: allow field to be changed
  media: rcar-vin: fix bug in scaling
  media: rcar-vin: fix height for TOP and BOTTOM fields
  media: rcar-vin: add support for V4L2_FIELD_ALTERNATE
  media: adv7180: fill in mbus format in set_fmt

Steve Longerbeam (1):
  media: adv7180: fix field type

 drivers/media/i2c/adv7180.c                 |  21 ++--
 drivers/media/platform/rcar-vin/rcar-dma.c  |  26 +++--
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 151 ++++++++++++++++------------
 3 files changed, 123 insertions(+), 75 deletions(-)

-- 
2.9.0

[PATCH 1/6] media: rcar-vin: allow field to be changed

[Niklas Söderlund]

The driver forced whatever field was set by the source subdevice to be
used. This patch allows the user to change from the default field.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 10a5c10..346339f 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -102,6 +102,7 @@ static int __rvin_try_format_source(struct rvin_dev *vin,
 	struct v4l2_subdev_format format = {
 		.which = which,
 	};
+	enum v4l2_field field;
 	int ret;
 
 	sd = vin_to_source(vin);
@@ -114,6 +115,8 @@ static int __rvin_try_format_source(struct rvin_dev *vin,
 
 	format.pad = vin->src_pad_idx;
 
+	field = pix->field;
+
 	ret = v4l2_device_call_until_err(sd->v4l2_dev, 0, pad, set_fmt,
 					 pad_cfg, &format);
 	if (ret < 0)
@@ -121,6 +124,8 @@ static int __rvin_try_format_source(struct rvin_dev *vin,
 
 	v4l2_fill_pix_format(pix, &format.format);
 
+	pix->field = field;
+
 	source->width = pix->width;
 	source->height = pix->height;
 
-- 
2.9.0

[PATCH 2/6] media: rcar-vin: fix bug in scaling

[Niklas Söderlund]

It was not possible to scale beyond the image size of the video source
limitation. The output frame would be bigger but the captured image was
limited to the size of the video source.

The error was that the crop boundary was set after the requested frame
size and not the video source size. This patch breaks out the resetting
of the crop, compose and format to separate functions so the error wont
creep back.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 346339f..b8c3836 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -92,6 +92,54 @@ static u32 rvin_format_sizeimage(struct v4l2_pix_format *pix)
  * V4L2
  */
 
+static void rvin_reset_crop_compose(struct rvin_dev *vin)
+{
+	vin->crop.top = vin->crop.left = 0;
+	vin->crop.width = vin->source.width;
+	vin->crop.height = vin->source.height;
+
+	vin->compose.top = vin->compose.left = 0;
+	vin->compose.width = vin->format.width;
+	vin->compose.height = vin->format.height;
+}
+
+static int rvin_reset_format(struct rvin_dev *vin)
+{
+	struct v4l2_subdev_format fmt = {
+		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+	};
+	struct v4l2_mbus_framefmt *mf = &fmt.format;
+	int ret;
+
+	fmt.pad = vin->src_pad_idx;
+
+	ret = v4l2_subdev_call(vin_to_source(vin), pad, get_fmt, NULL, &fmt);
+	if (ret)
+		return ret;
+
+	vin->format.width	= mf->width;
+	vin->format.height	= mf->height;
+	vin->format.colorspace	= mf->colorspace;
+	vin->format.field	= mf->field;
+
+	switch (vin->format.field) {
+	case V4L2_FIELD_TOP:
+	case V4L2_FIELD_BOTTOM:
+	case V4L2_FIELD_NONE:
+	case V4L2_FIELD_INTERLACED_TB:
+	case V4L2_FIELD_INTERLACED_BT:
+	case V4L2_FIELD_INTERLACED:
+		break;
+	default:
+		vin->format.field = V4L2_FIELD_NONE;
+		break;
+	}
+
+	rvin_reset_crop_compose(vin);
+
+	return 0;
+}
+
 static int __rvin_try_format_source(struct rvin_dev *vin,
 					u32 which,
 					struct v4l2_pix_format *pix,
@@ -247,6 +295,8 @@ static int rvin_s_fmt_vid_cap(struct file *file, void *priv,
 
 	vin->format = f->fmt.pix;
 
+	rvin_reset_crop_compose(vin);
+
 	return 0;
 }
 
@@ -438,35 +488,14 @@ static int rvin_querystd(struct file *file, void *priv, v4l2_std_id *a)
 static int rvin_s_std(struct file *file, void *priv, v4l2_std_id a)
 {
 	struct rvin_dev *vin = video_drvdata(file);
-	struct v4l2_subdev *sd = vin_to_source(vin);
-	struct v4l2_subdev_format fmt = {
-		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
-	};
-	struct v4l2_mbus_framefmt *mf = &fmt.format;
-	int ret = v4l2_subdev_call(sd, video, s_std, a);
+	int ret;
 
+	ret = v4l2_subdev_call(vin_to_source(vin), video, s_std, a);
 	if (ret < 0)
 		return ret;
 
 	/* Changing the standard will change the width/height */
-	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
-	if (ret) {
-		vin_err(vin, "Failed to get initial format\n");
-		return ret;
-	}
-
-	vin->format.width = mf->width;
-	vin->format.height = mf->height;
-
-	vin->crop.top = vin->crop.left = 0;
-	vin->crop.width = mf->width;
-	vin->crop.height = mf->height;
-
-	vin->compose.top = vin->compose.left = 0;
-	vin->compose.width = mf->width;
-	vin->compose.height = mf->height;
-
-	return 0;
+	return rvin_reset_format(vin);
 }
 
 static int rvin_g_std(struct file *file, void *priv, v4l2_std_id *a)
@@ -772,10 +801,6 @@ static void rvin_notify(struct v4l2_subdev *sd,
 
 int rvin_v4l2_probe(struct rvin_dev *vin)
 {
-	struct v4l2_subdev_format fmt = {
-		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
-	};
-	struct v4l2_mbus_framefmt *mf = &fmt.format;
 	struct video_device *vdev = &vin->vdev;
 	struct v4l2_subdev *sd = vin_to_source(vin);
 #if defined(CONFIG_MEDIA_CONTROLLER)
@@ -838,31 +863,9 @@ int rvin_v4l2_probe(struct rvin_dev *vin)
 
 	vin->src_pad_idx = pad_idx;
 #endif
-	fmt.pad = vin->src_pad_idx;
-
-	/* Try to improve our guess of a reasonable window format */
-	ret = v4l2_subdev_call(sd, pad, get_fmt, NULL, &fmt);
-	if (ret) {
-		vin_err(vin, "Failed to get initial format\n");
-		return ret;
-	}
 
-	/* Set default format */
-	vin->format.width	= mf->width;
-	vin->format.height	= mf->height;
-	vin->format.colorspace	= mf->colorspace;
-	vin->format.field	= mf->field;
 	vin->format.pixelformat	= RVIN_DEFAULT_FORMAT;
-
-
-	/* Set initial crop and compose */
-	vin->crop.top = vin->crop.left = 0;
-	vin->crop.width = mf->width;
-	vin->crop.height = mf->height;
-
-	vin->compose.top = vin->compose.left = 0;
-	vin->compose.width = mf->width;
-	vin->compose.height = mf->height;
+	rvin_reset_format(vin);
 
 	ret = video_register_device(&vin->vdev, VFL_TYPE_GRABBER, -1);
 	if (ret) {
-- 
2.9.0

[PATCH 3/6] media: rcar-vin: fix height for TOP and BOTTOM fields

[Niklas Söderlund]

The height used for V4L2_FIELD_TOP and V4L2_FIELD_BOTTOM where wrong.
The frames only contain one filed so the height should be half of the
frame.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index b8c3836..b6e40ea 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -125,6 +125,8 @@ static int rvin_reset_format(struct rvin_dev *vin)
 	switch (vin->format.field) {
 	case V4L2_FIELD_TOP:
 	case V4L2_FIELD_BOTTOM:
+		vin->format.height /= 2;
+		break;
 	case V4L2_FIELD_NONE:
 	case V4L2_FIELD_INTERLACED_TB:
 	case V4L2_FIELD_INTERLACED_BT:
@@ -217,21 +219,13 @@ static int __rvin_try_format(struct rvin_dev *vin,
 	/* Limit to source capabilities */
 	__rvin_try_format_source(vin, which, pix, source);
 
-	/* If source can't match format try if VIN can scale */
-	if (source->width != rwidth || source->height != rheight)
-		rvin_scale_try(vin, pix, rwidth, rheight);
-
-	/* HW limit width to a multiple of 32 (2^5) for NV16 else 2 (2^1) */
-	walign = vin->format.pixelformat == V4L2_PIX_FMT_NV16 ? 5 : 1;
-
-	/* Limit to VIN capabilities */
-	v4l_bound_align_image(&pix->width, 2, RVIN_MAX_WIDTH, walign,
-			      &pix->height, 4, RVIN_MAX_HEIGHT, 2, 0);
-
 	switch (pix->field) {
-	case V4L2_FIELD_NONE:
 	case V4L2_FIELD_TOP:
 	case V4L2_FIELD_BOTTOM:
+		pix->height /= 2;
+		source->height /= 2;
+		break;
+	case V4L2_FIELD_NONE:
 	case V4L2_FIELD_INTERLACED_TB:
 	case V4L2_FIELD_INTERLACED_BT:
 	case V4L2_FIELD_INTERLACED:
@@ -241,6 +235,17 @@ static int __rvin_try_format(struct rvin_dev *vin,
 		break;
 	}
 
+	/* If source can't match format try if VIN can scale */
+	if (source->width != rwidth || source->height != rheight)
+		rvin_scale_try(vin, pix, rwidth, rheight);
+
+	/* HW limit width to a multiple of 32 (2^5) for NV16 else 2 (2^1) */
+	walign = vin->format.pixelformat == V4L2_PIX_FMT_NV16 ? 5 : 1;
+
+	/* Limit to VIN capabilities */
+	v4l_bound_align_image(&pix->width, 2, RVIN_MAX_WIDTH, walign,
+			      &pix->height, 4, RVIN_MAX_HEIGHT, 2, 0);
+
 	pix->bytesperline = max_t(u32, pix->bytesperline,
 				  rvin_format_bytesperline(pix));
 	pix->sizeimage = max_t(u32, pix->sizeimage,
-- 
2.9.0

[PATCH 4/6] media: rcar-vin: add support for V4L2_FIELD_ALTERNATE

[Niklas Söderlund]

The HW can capture both ODD and EVEN fields in separate buffers so it's
possible to support this field mode.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index dad3b03..bcdec46 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -95,6 +95,7 @@
 /* Video n Module Status Register bits */
 #define VNMS_FBS_MASK		(3 << 3)
 #define VNMS_FBS_SHIFT		3
+#define VNMS_FS			(1 << 2)
 #define VNMS_AV			(1 << 1)
 #define VNMS_CA			(1 << 0)
 
@@ -147,6 +148,7 @@ static int rvin_setup(struct rvin_dev *vin)
 	case V4L2_FIELD_INTERLACED_BT:
 		vnmc = VNMC_IM_FULL | VNMC_FOC;
 		break;
+	case V4L2_FIELD_ALTERNATE:
 	case V4L2_FIELD_NONE:
 		if (vin->continuous) {
 			vnmc = VNMC_IM_ODD_EVEN;
@@ -322,15 +324,26 @@ static bool rvin_capture_active(struct rvin_dev *vin)
 	return rvin_read(vin, VNMS_REG) & VNMS_CA;
 }
 
-static int rvin_get_active_slot(struct rvin_dev *vin)
+static int rvin_get_active_slot(struct rvin_dev *vin, u32 vnms)
 {
 	if (vin->continuous)
-		return (rvin_read(vin, VNMS_REG) & VNMS_FBS_MASK)
-			>> VNMS_FBS_SHIFT;
+		return (vnms & VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
 
 	return 0;
 }
 
+static enum v4l2_field rvin_get_active_field(struct rvin_dev *vin, u32 vnms)
+{
+	if (vin->format.field == V4L2_FIELD_ALTERNATE) {
+		/* If FS is set it's a Even field */
+		if (vnms & VNMS_FS)
+			return V4L2_FIELD_BOTTOM;
+		return V4L2_FIELD_TOP;
+	}
+
+	return vin->format.field;
+}
+
 static void rvin_set_slot_addr(struct rvin_dev *vin, int slot, dma_addr_t addr)
 {
 	const struct rvin_video_format *fmt;
@@ -871,7 +884,7 @@ static bool rvin_fill_hw(struct rvin_dev *vin)
 static irqreturn_t rvin_irq(int irq, void *data)
 {
 	struct rvin_dev *vin = data;
-	u32 int_status;
+	u32 int_status, vnms;
 	int slot;
 	unsigned int sequence, handled = 0;
 	unsigned long flags;
@@ -898,7 +911,8 @@ static irqreturn_t rvin_irq(int irq, void *data)
 	}
 
 	/* Prepare for capture and update state */
-	slot = rvin_get_active_slot(vin);
+	vnms = rvin_read(vin, VNMS_REG);
+	slot = rvin_get_active_slot(vin, vnms);
 	sequence = vin->sequence++;
 
 	vin_dbg(vin, "IRQ %02d: %d\tbuf0: %c buf1: %c buf2: %c\tmore: %d\n",
@@ -913,7 +927,7 @@ static irqreturn_t rvin_irq(int irq, void *data)
 		goto done;
 
 	/* Capture frame */
-	vin->queue_buf[slot]->field = vin->format.field;
+	vin->queue_buf[slot]->field = rvin_get_active_field(vin, vnms);
 	vin->queue_buf[slot]->sequence = sequence;
 	vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
 	vb2_buffer_done(&vin->queue_buf[slot]->vb2_buf, VB2_BUF_STATE_DONE);
diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index b6e40ea..00ac2b6 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -109,6 +109,7 @@ static int rvin_reset_format(struct rvin_dev *vin)
 		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
 	};
 	struct v4l2_mbus_framefmt *mf = &fmt.format;
+	v4l2_std_id std;
 	int ret;
 
 	fmt.pad = vin->src_pad_idx;
@@ -122,9 +123,19 @@ static int rvin_reset_format(struct rvin_dev *vin)
 	vin->format.colorspace	= mf->colorspace;
 	vin->format.field	= mf->field;
 
+	/* If we have a video standard use HW to deinterlace */
+	if (vin->format.field == V4L2_FIELD_ALTERNATE &&
+	    !v4l2_subdev_call(vin_to_source(vin), video, g_std, &std)) {
+		if (std & V4L2_STD_625_50)
+			vin->format.field = V4L2_FIELD_INTERLACED_TB;
+		else
+			vin->format.field = V4L2_FIELD_INTERLACED_BT;
+	}
+
 	switch (vin->format.field) {
 	case V4L2_FIELD_TOP:
 	case V4L2_FIELD_BOTTOM:
+	case V4L2_FIELD_ALTERNATE:
 		vin->format.height /= 2;
 		break;
 	case V4L2_FIELD_NONE:
@@ -222,6 +233,7 @@ static int __rvin_try_format(struct rvin_dev *vin,
 	switch (pix->field) {
 	case V4L2_FIELD_TOP:
 	case V4L2_FIELD_BOTTOM:
+	case V4L2_FIELD_ALTERNATE:
 		pix->height /= 2;
 		source->height /= 2;
 		break;
-- 
2.9.0

[PATCH 5/6] media: adv7180: fill in mbus format in set_fmt

[Niklas Söderlund]

If the V4L2_SUBDEV_FORMAT_TRY is used in set_fmt the width, height etc
would not be filled.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/i2c/adv7180.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index b77b0a4..a8b434b 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -675,6 +675,7 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd,
 {
 	struct adv7180_state *state = to_state(sd);
 	struct v4l2_mbus_framefmt *framefmt;
+	int ret;
 
 	switch (format->format.field) {
 	case V4L2_FIELD_NONE:
@@ -686,8 +687,9 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd,
 		break;
 	}
 
+	ret = adv7180_mbus_fmt(sd,  &format->format);
+
 	if (format->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
-		framefmt = &format->format;
 		if (state->field != format->format.field) {
 			state->field = format->format.field;
 			adv7180_set_power(state, false);
@@ -699,7 +701,7 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd,
 		*framefmt = format->format;
 	}
 
-	return adv7180_mbus_fmt(sd, framefmt);
+	return ret;
 }
 
 static int adv7180_g_mbus_config(struct v4l2_subdev *sd,
-- 
2.9.0

[PATCH 6/6] media: adv7180: fix field type

[Niklas Söderlund]

From: Steve Longerbeam <slongerbeam@gmail.com>

The ADV7180 and ADV7182 transmit whole fields, bottom field followed
by top (or vice-versa, depending on detected video standard). So
for chips that do not have support for explicitly setting the field
mode, set the field mode to V4L2_FIELD_ALTERNATE.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
[Niklas: changed filed type from V4L2_FIELD_SEQ_{TB,BT} to
V4L2_FIELD_ALTERNATE]
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/i2c/adv7180.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/adv7180.c b/drivers/media/i2c/adv7180.c
index a8b434b..c6fed71 100644
--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -680,10 +680,13 @@ static int adv7180_set_pad_format(struct v4l2_subdev *sd,
 	switch (format->format.field) {
 	case V4L2_FIELD_NONE:
 		if (!(state->chip_info->flags & ADV7180_FLAG_I2P))
-			format->format.field = V4L2_FIELD_INTERLACED;
+			format->format.field = V4L2_FIELD_ALTERNATE;
 		break;
 	default:
-		format->format.field = V4L2_FIELD_INTERLACED;
+		if (state->chip_info->flags & ADV7180_FLAG_I2P)
+			format->format.field = V4L2_FIELD_INTERLACED;
+		else
+			format->format.field = V4L2_FIELD_ALTERNATE;
 		break;
 	}
 
@@ -1253,8 +1256,13 @@ static int adv7180_probe(struct i2c_client *client,
 		return -ENOMEM;
 
 	state->client = client;
-	state->field = V4L2_FIELD_INTERLACED;
 	state->chip_info = (struct adv7180_chip_info *)id->driver_data;
+	state->curr_norm = V4L2_STD_NTSC;
+
+	if (state->chip_info->flags & ADV7180_FLAG_I2P)
+		state->field = V4L2_FIELD_INTERLACED;
+	else
+		state->field = V4L2_FIELD_ALTERNATE;
 
 	if (state->chip_info->flags & ADV7180_FLAG_MIPI_CSI2) {
 		state->csi_client = i2c_new_dummy(client->adapter,
@@ -1274,7 +1282,6 @@ static int adv7180_probe(struct i2c_client *client,
 
 	state->irq = client->irq;
 	mutex_init(&state->mutex);
-	state->curr_norm = V4L2_STD_NTSC;
 	if (state->chip_info->flags & ADV7180_FLAG_RESET_POWERED)
 		state->powered = true;
 	else
-- 
2.9.0

Re: [PATCH 6/6] media: adv7180: fix field type

[Sergei Shtylyov]

On 07/29/2016 08:40 PM, Niklas Söderlund wrote:

> From: Steve Longerbeam <slongerbeam@gmail.com>
>
> The ADV7180 and ADV7182 transmit whole fields, bottom field followed
> by top (or vice-versa, depending on detected video standard). So
> for chips that do not have support for explicitly setting the field
> mode, set the field mode to V4L2_FIELD_ALTERNATE.
>
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> [Niklas: changed filed type from V4L2_FIELD_SEQ_{TB,BT} to
> V4L2_FIELD_ALTERNATE]
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Tested-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

    IIUC, it's a 4th version of this patch; you should have kept the original 
change log (below --- tearline) and indicated that in the subject.

MBR, Sergei

Re: [PATCH 6/6] media: adv7180: fix field type

[Steve Longerbeam]

On 07/29/2016 12:10 PM, Sergei Shtylyov wrote:
> On 07/29/2016 08:40 PM, Niklas Söderlund wrote:
>
>> From: Steve Longerbeam <slongerbeam@gmail.com>
>>
>> The ADV7180 and ADV7182 transmit whole fields, bottom field followed
>> by top (or vice-versa, depending on detected video standard). So
>> for chips that do not have support for explicitly setting the field
>> mode, set the field mode to V4L2_FIELD_ALTERNATE.
>>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>> [Niklas: changed filed type from V4L2_FIELD_SEQ_{TB,BT} to
>> V4L2_FIELD_ALTERNATE]
>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>
> Tested-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>
>    IIUC, it's a 4th version of this patch; you should have kept the 
> original change log (below --- tearline) and indicated that in the 
> subject.
>
> MBR, Sergei

This version is fine with me. The i.mx6 h/w motion-compensation 
deinterlacer (VDIC)
needs to know the field order, and it can't get that info from 
V4L2_FIELD_ALTERNATE,
but it can still determine the order via querystd().

But I agree the change log should be preserved, and the 
V4L2_FIELD_ALTERNATE change
added to the change log.

Acked-by: Steve Longerbeam <slongerbeam@gmail.com>

Steve

Re: [PATCH 6/6] media: adv7180: fix field type

[Niklas Söderlund]

On 2016-07-29 12:32:30 -0700, Steve Longerbeam wrote:
> 
> On 07/29/2016 12:10 PM, Sergei Shtylyov wrote:
> > On 07/29/2016 08:40 PM, Niklas Söderlund wrote:
> > 
> > > From: Steve Longerbeam <slongerbeam@gmail.com>
> > > 
> > > The ADV7180 and ADV7182 transmit whole fields, bottom field followed
> > > by top (or vice-versa, depending on detected video standard). So
> > > for chips that do not have support for explicitly setting the field
> > > mode, set the field mode to V4L2_FIELD_ALTERNATE.
> > > 
> > > Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> > > [Niklas: changed filed type from V4L2_FIELD_SEQ_{TB,BT} to
> > > V4L2_FIELD_ALTERNATE]
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > 
> > Tested-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> > 
> >    IIUC, it's a 4th version of this patch; you should have kept the
> > original change log (below --- tearline) and indicated that in the
> > subject.
> > 
> > MBR, Sergei
> 
> This version is fine with me. The i.mx6 h/w motion-compensation deinterlacer
> (VDIC)
> needs to know the field order, and it can't get that info from
> V4L2_FIELD_ALTERNATE,
> but it can still determine the order via querystd().
> 
> But I agree the change log should be preserved, and the V4L2_FIELD_ALTERNATE
> change
> added to the change log.

Yes, I will send a v2 containing the changelog. My bad for dropping it.

> 
> Acked-by: Steve Longerbeam <slongerbeam@gmail.com>
> 
> Steve
> 
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH 6/6] media: adv7180: fix field type

[Niklas Söderlund]

On 2016-07-29 12:32:30 -0700, Steve Longerbeam wrote:
> 
> On 07/29/2016 12:10 PM, Sergei Shtylyov wrote:
> > On 07/29/2016 08:40 PM, Niklas S�derlund wrote:
> > 
> > > From: Steve Longerbeam <slongerbeam@gmail.com>
> > > 
> > > The ADV7180 and ADV7182 transmit whole fields, bottom field followed
> > > by top (or vice-versa, depending on detected video standard). So
> > > for chips that do not have support for explicitly setting the field
> > > mode, set the field mode to V4L2_FIELD_ALTERNATE.
> > > 
> > > Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> > > [Niklas: changed filed type from V4L2_FIELD_SEQ_{TB,BT} to
> > > V4L2_FIELD_ALTERNATE]
> > > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> > 
> > Tested-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> > 
> >    IIUC, it's a 4th version of this patch; you should have kept the
> > original change log (below --- tearline) and indicated that in the
> > subject.
> > 
> > MBR, Sergei
> 
> This version is fine with me. The i.mx6 h/w motion-compensation deinterlacer
> (VDIC)
> needs to know the field order, and it can't get that info from
> V4L2_FIELD_ALTERNATE,
> but it can still determine the order via querystd().
> 
> But I agree the change log should be preserved, and the V4L2_FIELD_ALTERNATE
> change
> added to the change log.

Yes, I will send a v2 containing the changelog. My bad for dropping it.

> 
> Acked-by: Steve Longerbeam <slongerbeam@gmail.com>
> 
> Steve
> 
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH 1/6] media: rcar-vin: allow field to be changed

[Sergei Shtylyov]

On 07/29/2016 08:40 PM, Niklas Söderlund wrote:

> The driver forced whatever field was set by the source subdevice to be
> used. This patch allows the user to change from the default field.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

    I didn't apply this patch at first (thinking it was unnecessary), and the 
capture worked fine. The field order appeared swapped again after I did import 
this patch as well. :-(

MBR, Sergei

Re: [PATCH 4/6] media: rcar-vin: add support for V4L2_FIELD_ALTERNATE

[Sergei Shtylyov]

Hello.

On 07/29/2016 08:40 PM, Niklas Söderlund wrote:

> The HW can capture both ODD and EVEN fields in separate buffers so it's
> possible to support this field mode.
>
> Signed-off-by: Niklas Söderlund <niklas.soder

    It's probably worth adding that if the subdevice presents the video data 
in this mode, we prefer to use the hardware de-interlacing.

MBR, Sergei

PS: I think I have a patch adding support for this mode to the old driver, so 
that it doesn't get borked with the patch #6 in this series.

Re: [PATCH 1/6] media: rcar-vin: allow field to be changed

[Niklas Söderlund]

Hi Sergei,

Thanks for testing!

On 2016-07-30 00:04:33 +0300, Sergei Shtylyov wrote:
> On 07/29/2016 08:40 PM, Niklas Söderlund wrote:
> 
> > The driver forced whatever field was set by the source subdevice to be
> > used. This patch allows the user to change from the default field.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
>    I didn't apply this patch at first (thinking it was unnecessary), and the
> capture worked fine. The field order appeared swapped again after I did
> import this patch as well. :-(

I had a look at the test tool you told me you use 
(https://linuxtv.org/downloads/v4l-dvb-apis/capture-example.html) and 
the reason the field order is swapped is a combination of that tool and 
how the rcar-vin driver interprets V4L2_FIELD_INTERLACED.

1. The tool you use asks for V4L2_FIELD_INTERLACED if the -f switch is 
   used. You told me #v4l that you do use that switch, but have modified 
   the tool to use a different pixelformat than used in the link above, 
   correct?

2. The rcar-vin driver interprets V4L2_FIELD_INTERLACED as 
   V4L2_FIELD_INTERLACED_TB. If this is correct or not I do not know, 
   the old soc-camera version of the driver do it this way so I have 
   kept that logic.

This is the reason why the field order is wrong when you apply this 
patch. Without it the field order would be locked to whatever the 
subdevice reports, V4L2_FIELD_ALTERNATE in this case.

I don't know if it's correct to treat V4L2_FIELD_INTERLACED as 
V4L2_FIELD_INTERLACED_TB or if I should try and use G_STD if 
V4L2_FIELD_INTERLACED is requested and change the field to _TB or _BT 
according to the result of that. I feel this will only push the problem 
further down. What if G_STD is not implemented by the subdevice? Then a 
default fallback interpretation to ether _TB or _BT would still be 
needed. I'm open to suggestion on how to handle this case.

There is also a feature missing in this patch. The field order was set 
to V4L2_FIELD_NONE if the requested format asks for V4L2_FIELD_ANY.  
This have no effect for how you test but i did run into it while trying 
to figure this out. I will send out a v2 which solves this by retaining 
the current field mode if V4L2_FIELD_ANY is asked for.

> 
> MBR, Sergei
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH 1/6] media: rcar-vin: allow field to be changed

[Niklas Söderlund]

Hi Sergei,

Thanks for testing!

On 2016-07-30 00:04:33 +0300, Sergei Shtylyov wrote:
> On 07/29/2016 08:40 PM, Niklas S�derlund wrote:
> 
> > The driver forced whatever field was set by the source subdevice to be
> > used. This patch allows the user to change from the default field.
> > 
> > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> 
>    I didn't apply this patch at first (thinking it was unnecessary), and the
> capture worked fine. The field order appeared swapped again after I did
> import this patch as well. :-(

I had a look at the test tool you told me you use 
(https://linuxtv.org/downloads/v4l-dvb-apis/capture-example.html) and 
the reason the field order is swapped is a combination of that tool and 
how the rcar-vin driver interprets V4L2_FIELD_INTERLACED.

1. The tool you use asks for V4L2_FIELD_INTERLACED if the -f switch is 
   used. You told me #v4l that you do use that switch, but have modified 
   the tool to use a different pixelformat than used in the link above, 
   correct?

2. The rcar-vin driver interprets V4L2_FIELD_INTERLACED as 
   V4L2_FIELD_INTERLACED_TB. If this is correct or not I do not know, 
   the old soc-camera version of the driver do it this way so I have 
   kept that logic.

This is the reason why the field order is wrong when you apply this 
patch. Without it the field order would be locked to whatever the 
subdevice reports, V4L2_FIELD_ALTERNATE in this case.

I don't know if it's correct to treat V4L2_FIELD_INTERLACED as 
V4L2_FIELD_INTERLACED_TB or if I should try and use G_STD if 
V4L2_FIELD_INTERLACED is requested and change the field to _TB or _BT 
according to the result of that. I feel this will only push the problem 
further down. What if G_STD is not implemented by the subdevice? Then a 
default fallback interpretation to ether _TB or _BT would still be 
needed. I'm open to suggestion on how to handle this case.

There is also a feature missing in this patch. The field order was set 
to V4L2_FIELD_NONE if the requested format asks for V4L2_FIELD_ANY.  
This have no effect for how you test but i did run into it while trying 
to figure this out. I will send out a v2 which solves this by retaining 
the current field mode if V4L2_FIELD_ANY is asked for.

> 
> MBR, Sergei
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH 4/6] media: rcar-vin: add support for V4L2_FIELD_ALTERNATE

[Niklas Söderlund]

Hi Sergei,

On 2016-07-31 00:55:04 +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 07/29/2016 08:40 PM, Niklas Söderlund wrote:
> 
> > The HW can capture both ODD and EVEN fields in separate buffers so it's
> > possible to support this field mode.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soder
> 
>    It's probably worth adding that if the subdevice presents the video data
> in this mode, we prefer to use the hardware de-interlacing.

Will include this in v2, thanks for pointing it out.

> 
> MBR, Sergei
> 
> PS: I think I have a patch adding support for this mode to the old driver,
> so that it doesn't get borked with the patch #6 in this series.
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCH 4/6] media: rcar-vin: add support for V4L2_FIELD_ALTERNATE

[Niklas Söderlund]

Hi Sergei,

On 2016-07-31 00:55:04 +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 07/29/2016 08:40 PM, Niklas S�derlund wrote:
> 
> > The HW can capture both ODD and EVEN fields in separate buffers so it's
> > possible to support this field mode.
> > 
> > Signed-off-by: Niklas S�derlund <niklas.soder
> 
>    It's probably worth adding that if the subdevice presents the video data
> in this mode, we prefer to use the hardware de-interlacing.

Will include this in v2, thanks for pointing it out.

> 
> MBR, Sergei
> 
> PS: I think I have a patch adding support for this mode to the old driver,
> so that it doesn't get borked with the patch #6 in this series.
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCH 1/6] media: rcar-vin: allow field to be changed

[Hans Verkuil]

On 08/01/2016 06:52 PM, Niklas Söderlund wrote:
> Hi Sergei,
> 
> Thanks for testing!
> 
> On 2016-07-30 00:04:33 +0300, Sergei Shtylyov wrote:
>> On 07/29/2016 08:40 PM, Niklas Söderlund wrote:
>>
>>> The driver forced whatever field was set by the source subdevice to be
>>> used. This patch allows the user to change from the default field.
>>>
>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>
>>    I didn't apply this patch at first (thinking it was unnecessary), and the
>> capture worked fine. The field order appeared swapped again after I did
>> import this patch as well. :-(
> 
> I had a look at the test tool you told me you use 
> (https://linuxtv.org/downloads/v4l-dvb-apis/capture-example.html) and 
> the reason the field order is swapped is a combination of that tool and 
> how the rcar-vin driver interprets V4L2_FIELD_INTERLACED.
> 
> 1. The tool you use asks for V4L2_FIELD_INTERLACED if the -f switch is 
>    used. You told me #v4l that you do use that switch, but have modified 
>    the tool to use a different pixelformat than used in the link above, 
>    correct?
> 
> 2. The rcar-vin driver interprets V4L2_FIELD_INTERLACED as 
>    V4L2_FIELD_INTERLACED_TB. If this is correct or not I do not know, 

That's wrong. FIELD_INTERLACED is standard dependent: it is effectively
equal to INTERLACED_TB for 50 Hz formats and equal to INTERLACED_BT for
60 Hz formats. For non-SDTV timings (e.g. 720i) it is equal to INTERLACED_TB.

Stick to FIELD_INTERLACED, that's what you normally want to use.

>    the old soc-camera version of the driver do it this way so I have 
>    kept that logic.
> 
> This is the reason why the field order is wrong when you apply this 
> patch. Without it the field order would be locked to whatever the 
> subdevice reports, V4L2_FIELD_ALTERNATE in this case.
> 
> I don't know if it's correct to treat V4L2_FIELD_INTERLACED as 
> V4L2_FIELD_INTERLACED_TB or if I should try and use G_STD if 
> V4L2_FIELD_INTERLACED is requested and change the field to _TB or _BT 
> according to the result of that. I feel this will only push the problem 
> further down. What if G_STD is not implemented by the subdevice? Then a 
> default fallback interpretation to ether _TB or _BT would still be 
> needed. I'm open to suggestion on how to handle this case.
> 
> There is also a feature missing in this patch. The field order was set 
> to V4L2_FIELD_NONE if the requested format asks for V4L2_FIELD_ANY.  
> This have no effect for how you test but i did run into it while trying 
> to figure this out. I will send out a v2 which solves this by retaining 
> the current field mode if V4L2_FIELD_ANY is asked for.
> 
>>
>> MBR, Sergei
>>
> 

I plan on reviewing all these field-related patches tomorrow.

Regards,

	Hans

Re: [PATCH 1/6] media: rcar-vin: allow field to be changed

[Hans Verkuil]

On 08/01/2016 06:52 PM, Niklas S�derlund wrote:
> Hi Sergei,
> 
> Thanks for testing!
> 
> On 2016-07-30 00:04:33 +0300, Sergei Shtylyov wrote:
>> On 07/29/2016 08:40 PM, Niklas S�derlund wrote:
>>
>>> The driver forced whatever field was set by the source subdevice to be
>>> used. This patch allows the user to change from the default field.
>>>
>>> Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
>>
>>    I didn't apply this patch at first (thinking it was unnecessary), and the
>> capture worked fine. The field order appeared swapped again after I did
>> import this patch as well. :-(
> 
> I had a look at the test tool you told me you use 
> (https://linuxtv.org/downloads/v4l-dvb-apis/capture-example.html) and 
> the reason the field order is swapped is a combination of that tool and 
> how the rcar-vin driver interprets V4L2_FIELD_INTERLACED.
> 
> 1. The tool you use asks for V4L2_FIELD_INTERLACED if the -f switch is 
>    used. You told me #v4l that you do use that switch, but have modified 
>    the tool to use a different pixelformat than used in the link above, 
>    correct?
> 
> 2. The rcar-vin driver interprets V4L2_FIELD_INTERLACED as 
>    V4L2_FIELD_INTERLACED_TB. If this is correct or not I do not know, 

That's wrong. FIELD_INTERLACED is standard dependent: it is effectively
equal to INTERLACED_TB for 50 Hz formats and equal to INTERLACED_BT for
60 Hz formats. For non-SDTV timings (e.g. 720i) it is equal to INTERLACED_TB.

Stick to FIELD_INTERLACED, that's what you normally want to use.

>    the old soc-camera version of the driver do it this way so I have 
>    kept that logic.
> 
> This is the reason why the field order is wrong when you apply this 
> patch. Without it the field order would be locked to whatever the 
> subdevice reports, V4L2_FIELD_ALTERNATE in this case.
> 
> I don't know if it's correct to treat V4L2_FIELD_INTERLACED as 
> V4L2_FIELD_INTERLACED_TB or if I should try and use G_STD if 
> V4L2_FIELD_INTERLACED is requested and change the field to _TB or _BT 
> according to the result of that. I feel this will only push the problem 
> further down. What if G_STD is not implemented by the subdevice? Then a 
> default fallback interpretation to ether _TB or _BT would still be 
> needed. I'm open to suggestion on how to handle this case.
> 
> There is also a feature missing in this patch. The field order was set 
> to V4L2_FIELD_NONE if the requested format asks for V4L2_FIELD_ANY.  
> This have no effect for how you test but i did run into it while trying 
> to figure this out. I will send out a v2 which solves this by retaining 
> the current field mode if V4L2_FIELD_ANY is asked for.
> 
>>
>> MBR, Sergei
>>
> 

I plan on reviewing all these field-related patches tomorrow.

Regards,

	Hans

Re: [PATCH 4/6] media: rcar-vin: add support for V4L2_FIELD_ALTERNATE

[Hans Verkuil]

On 07/29/2016 07:40 PM, Niklas Söderlund wrote:
> The HW can capture both ODD and EVEN fields in separate buffers so it's
> possible to support this field mode.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c  | 26 ++++++++++++++++++++------
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 12 ++++++++++++
>  2 files changed, 32 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index dad3b03..bcdec46 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -95,6 +95,7 @@
>  /* Video n Module Status Register bits */
>  #define VNMS_FBS_MASK		(3 << 3)
>  #define VNMS_FBS_SHIFT		3
> +#define VNMS_FS			(1 << 2)
>  #define VNMS_AV			(1 << 1)
>  #define VNMS_CA			(1 << 0)
>  
> @@ -147,6 +148,7 @@ static int rvin_setup(struct rvin_dev *vin)
>  	case V4L2_FIELD_INTERLACED_BT:
>  		vnmc = VNMC_IM_FULL | VNMC_FOC;
>  		break;
> +	case V4L2_FIELD_ALTERNATE:
>  	case V4L2_FIELD_NONE:
>  		if (vin->continuous) {
>  			vnmc = VNMC_IM_ODD_EVEN;
> @@ -322,15 +324,26 @@ static bool rvin_capture_active(struct rvin_dev *vin)
>  	return rvin_read(vin, VNMS_REG) & VNMS_CA;
>  }
>  
> -static int rvin_get_active_slot(struct rvin_dev *vin)
> +static int rvin_get_active_slot(struct rvin_dev *vin, u32 vnms)
>  {
>  	if (vin->continuous)
> -		return (rvin_read(vin, VNMS_REG) & VNMS_FBS_MASK)
> -			>> VNMS_FBS_SHIFT;
> +		return (vnms & VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
>  
>  	return 0;
>  }
>  
> +static enum v4l2_field rvin_get_active_field(struct rvin_dev *vin, u32 vnms)
> +{
> +	if (vin->format.field == V4L2_FIELD_ALTERNATE) {
> +		/* If FS is set it's a Even field */
> +		if (vnms & VNMS_FS)
> +			return V4L2_FIELD_BOTTOM;
> +		return V4L2_FIELD_TOP;
> +	}
> +
> +	return vin->format.field;
> +}
> +
>  static void rvin_set_slot_addr(struct rvin_dev *vin, int slot, dma_addr_t addr)
>  {
>  	const struct rvin_video_format *fmt;
> @@ -871,7 +884,7 @@ static bool rvin_fill_hw(struct rvin_dev *vin)
>  static irqreturn_t rvin_irq(int irq, void *data)
>  {
>  	struct rvin_dev *vin = data;
> -	u32 int_status;
> +	u32 int_status, vnms;
>  	int slot;
>  	unsigned int sequence, handled = 0;
>  	unsigned long flags;
> @@ -898,7 +911,8 @@ static irqreturn_t rvin_irq(int irq, void *data)
>  	}
>  
>  	/* Prepare for capture and update state */
> -	slot = rvin_get_active_slot(vin);
> +	vnms = rvin_read(vin, VNMS_REG);
> +	slot = rvin_get_active_slot(vin, vnms);
>  	sequence = vin->sequence++;
>  
>  	vin_dbg(vin, "IRQ %02d: %d\tbuf0: %c buf1: %c buf2: %c\tmore: %d\n",
> @@ -913,7 +927,7 @@ static irqreturn_t rvin_irq(int irq, void *data)
>  		goto done;
>  
>  	/* Capture frame */
> -	vin->queue_buf[slot]->field = vin->format.field;
> +	vin->queue_buf[slot]->field = rvin_get_active_field(vin, vnms);
>  	vin->queue_buf[slot]->sequence = sequence;
>  	vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
>  	vb2_buffer_done(&vin->queue_buf[slot]->vb2_buf, VB2_BUF_STATE_DONE);
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index b6e40ea..00ac2b6 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -109,6 +109,7 @@ static int rvin_reset_format(struct rvin_dev *vin)
>  		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>  	};
>  	struct v4l2_mbus_framefmt *mf = &fmt.format;
> +	v4l2_std_id std;
>  	int ret;
>  
>  	fmt.pad = vin->src_pad_idx;
> @@ -122,9 +123,19 @@ static int rvin_reset_format(struct rvin_dev *vin)
>  	vin->format.colorspace	= mf->colorspace;
>  	vin->format.field	= mf->field;
>  
> +	/* If we have a video standard use HW to deinterlace */
> +	if (vin->format.field == V4L2_FIELD_ALTERNATE &&
> +	    !v4l2_subdev_call(vin_to_source(vin), video, g_std, &std)) {
> +		if (std & V4L2_STD_625_50)
> +			vin->format.field = V4L2_FIELD_INTERLACED_TB;
> +		else
> +			vin->format.field = V4L2_FIELD_INTERLACED_BT;
> +	}

Huh? ALTERNATE means that the fields are captured separately, i.e. one buffer
per field.

There is no HW deinterlacing going on in that case, and ALTERNATE is certainly
not equal to FIELD_INTERLACED_BT/TB.

If ALTERNATE is chosen as the field format, then VIDIOC_G_FMT should return
ALTERNATE as the field format, but in struct v4l2_buffer the field will always
be TOP or BOTTOM.

> +
>  	switch (vin->format.field) {
>  	case V4L2_FIELD_TOP:
>  	case V4L2_FIELD_BOTTOM:
> +	case V4L2_FIELD_ALTERNATE:
>  		vin->format.height /= 2;
>  		break;
>  	case V4L2_FIELD_NONE:
> @@ -222,6 +233,7 @@ static int __rvin_try_format(struct rvin_dev *vin,
>  	switch (pix->field) {
>  	case V4L2_FIELD_TOP:
>  	case V4L2_FIELD_BOTTOM:
> +	case V4L2_FIELD_ALTERNATE:
>  		pix->height /= 2;
>  		source->height /= 2;
>  		break;
> 

Regards,

	Hans

Re: [PATCH 0/6] Fix adv7180 and rcar-vin field handling

[Hans Verkuil]

On 07/29/2016 07:40 PM, Niklas Söderlund wrote:
> Hi,
> 
> This series add V4L2_FIELD_ALTERNATE support to the rcar-vin driver and 
> changes the field mode reported by adv7180 from V4L2_FIELD_INTERLACED to 
> V4L2_FIELD_ALTERNATE.
> 
> The change field mode reported by adv7180 was first done by Steve 
> Longerbeam (https://lkml.org/lkml/2016/7/23/107), I have keept and 
> reworked Steves patch to report V4L2_FIELD_ALTERNATE instead of 
> V4L2_FIELD_SEQ_{TB,BT}, after discussions on #v4l this seems more
> correct.
> 
> The rcar-vin changes contains some bug fixes needed to enable 
> V4L2_FIELD_ALTERNATE.
> 
> All work is based on top of media-next and is tested on Koelsch.
> 
> This series touch two drivers which is not a good thing. But I could not 
> figure out a good way to post them separately since if the adv7180 parts 
> where too be merged before the rcar-vin changes the driver would stop to 
> work on the Koelsch. If some one wants this series split in two let me 
> know.

When you post v2, please also run 'v4l2-compliance -f': this will test all
format/field combinations.

I also recommend testing with qv4l2 if you can to verify that it looks
correct. The qv4l2 utility is guaranteed to handle all field settings
correctly.

Regards,

	Hans

> 
> Niklas Söderlund (5):
>   media: rcar-vin: allow field to be changed
>   media: rcar-vin: fix bug in scaling
>   media: rcar-vin: fix height for TOP and BOTTOM fields
>   media: rcar-vin: add support for V4L2_FIELD_ALTERNATE
>   media: adv7180: fill in mbus format in set_fmt
> 
> Steve Longerbeam (1):
>   media: adv7180: fix field type
> 
>  drivers/media/i2c/adv7180.c                 |  21 ++--
>  drivers/media/platform/rcar-vin/rcar-dma.c  |  26 +++--
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 151 ++++++++++++++++------------
>  3 files changed, 123 insertions(+), 75 deletions(-)
> 

Re: [PATCH 4/6] media: rcar-vin: add support for V4L2_FIELD_ALTERNATE

[Niklas Söderlund]

Hi Hans,

Thanks for your feedback.

On 2016-08-02 11:41:15 +0200, Hans Verkuil wrote:
> 
> 
> On 07/29/2016 07:40 PM, Niklas Söderlund wrote:
> > The HW can capture both ODD and EVEN fields in separate buffers so it's
> > possible to support this field mode.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-dma.c  | 26 ++++++++++++++++++++------
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c | 12 ++++++++++++
> >  2 files changed, 32 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index dad3b03..bcdec46 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -95,6 +95,7 @@
> >  /* Video n Module Status Register bits */
> >  #define VNMS_FBS_MASK		(3 << 3)
> >  #define VNMS_FBS_SHIFT		3
> > +#define VNMS_FS			(1 << 2)
> >  #define VNMS_AV			(1 << 1)
> >  #define VNMS_CA			(1 << 0)
> >  
> > @@ -147,6 +148,7 @@ static int rvin_setup(struct rvin_dev *vin)
> >  	case V4L2_FIELD_INTERLACED_BT:
> >  		vnmc = VNMC_IM_FULL | VNMC_FOC;
> >  		break;
> > +	case V4L2_FIELD_ALTERNATE:
> >  	case V4L2_FIELD_NONE:
> >  		if (vin->continuous) {
> >  			vnmc = VNMC_IM_ODD_EVEN;
> > @@ -322,15 +324,26 @@ static bool rvin_capture_active(struct rvin_dev *vin)
> >  	return rvin_read(vin, VNMS_REG) & VNMS_CA;
> >  }
> >  
> > -static int rvin_get_active_slot(struct rvin_dev *vin)
> > +static int rvin_get_active_slot(struct rvin_dev *vin, u32 vnms)
> >  {
> >  	if (vin->continuous)
> > -		return (rvin_read(vin, VNMS_REG) & VNMS_FBS_MASK)
> > -			>> VNMS_FBS_SHIFT;
> > +		return (vnms & VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
> >  
> >  	return 0;
> >  }
> >  
> > +static enum v4l2_field rvin_get_active_field(struct rvin_dev *vin, u32 vnms)
> > +{
> > +	if (vin->format.field == V4L2_FIELD_ALTERNATE) {
> > +		/* If FS is set it's a Even field */
> > +		if (vnms & VNMS_FS)
> > +			return V4L2_FIELD_BOTTOM;
> > +		return V4L2_FIELD_TOP;
> > +	}
> > +
> > +	return vin->format.field;
> > +}
> > +
> >  static void rvin_set_slot_addr(struct rvin_dev *vin, int slot, dma_addr_t addr)
> >  {
> >  	const struct rvin_video_format *fmt;
> > @@ -871,7 +884,7 @@ static bool rvin_fill_hw(struct rvin_dev *vin)
> >  static irqreturn_t rvin_irq(int irq, void *data)
> >  {
> >  	struct rvin_dev *vin = data;
> > -	u32 int_status;
> > +	u32 int_status, vnms;
> >  	int slot;
> >  	unsigned int sequence, handled = 0;
> >  	unsigned long flags;
> > @@ -898,7 +911,8 @@ static irqreturn_t rvin_irq(int irq, void *data)
> >  	}
> >  
> >  	/* Prepare for capture and update state */
> > -	slot = rvin_get_active_slot(vin);
> > +	vnms = rvin_read(vin, VNMS_REG);
> > +	slot = rvin_get_active_slot(vin, vnms);
> >  	sequence = vin->sequence++;
> >  
> >  	vin_dbg(vin, "IRQ %02d: %d\tbuf0: %c buf1: %c buf2: %c\tmore: %d\n",
> > @@ -913,7 +927,7 @@ static irqreturn_t rvin_irq(int irq, void *data)
> >  		goto done;
> >  
> >  	/* Capture frame */
> > -	vin->queue_buf[slot]->field = vin->format.field;
> > +	vin->queue_buf[slot]->field = rvin_get_active_field(vin, vnms);
> >  	vin->queue_buf[slot]->sequence = sequence;
> >  	vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
> >  	vb2_buffer_done(&vin->queue_buf[slot]->vb2_buf, VB2_BUF_STATE_DONE);
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > index b6e40ea..00ac2b6 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -109,6 +109,7 @@ static int rvin_reset_format(struct rvin_dev *vin)
> >  		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> >  	};
> >  	struct v4l2_mbus_framefmt *mf = &fmt.format;
> > +	v4l2_std_id std;
> >  	int ret;
> >  
> >  	fmt.pad = vin->src_pad_idx;
> > @@ -122,9 +123,19 @@ static int rvin_reset_format(struct rvin_dev *vin)
> >  	vin->format.colorspace	= mf->colorspace;
> >  	vin->format.field	= mf->field;
> >  
> > +	/* If we have a video standard use HW to deinterlace */
> > +	if (vin->format.field == V4L2_FIELD_ALTERNATE &&
> > +	    !v4l2_subdev_call(vin_to_source(vin), video, g_std, &std)) {
> > +		if (std & V4L2_STD_625_50)
> > +			vin->format.field = V4L2_FIELD_INTERLACED_TB;
> > +		else
> > +			vin->format.field = V4L2_FIELD_INTERLACED_BT;
> > +	}
> 
> Huh? ALTERNATE means that the fields are captured separately, i.e. one buffer
> per field.
> 
> There is no HW deinterlacing going on in that case, and ALTERNATE is certainly
> not equal to FIELD_INTERLACED_BT/TB.
> 
> If ALTERNATE is chosen as the field format, then VIDIOC_G_FMT should return
> ALTERNATE as the field format, but in struct v4l2_buffer the field will always
> be TOP or BOTTOM.

Yes, if S_FMT request ALTERNATE then G_FMT will return ALTERNATE. This 
code was meant to make INTERLACE_{TB,BT} the default field selection if 
the subdevice uses V4L2_FIELD_ALTERNATE. The rvin_reset_format() is only 
called in the following cases:

- When the driver is first probed to get initial default values from the 
  subdevice.

- S_STD is called and the width, hight and other parameters from the 
  subdevice needs to be updated.

Is it wrong to use an INTERLACE field as default if the subdevice 
provides ALTERNATE? My goal was to not change the behavior of the 
rcar-vin driver which default uses INTERLACE today? I'm happy to drop 
this part for v2 if it's the wrong thing to do in this case.

> 
> > +
> >  	switch (vin->format.field) {
> >  	case V4L2_FIELD_TOP:
> >  	case V4L2_FIELD_BOTTOM:
> > +	case V4L2_FIELD_ALTERNATE:
> >  		vin->format.height /= 2;
> >  		break;
> >  	case V4L2_FIELD_NONE:
> > @@ -222,6 +233,7 @@ static int __rvin_try_format(struct rvin_dev *vin,
> >  	switch (pix->field) {
> >  	case V4L2_FIELD_TOP:
> >  	case V4L2_FIELD_BOTTOM:
> > +	case V4L2_FIELD_ALTERNATE:
> >  		pix->height /= 2;
> >  		source->height /= 2;
> >  		break;
> > 
> 
> Regards,
> 
> 	Hans

-- 
Regards,
Niklas Söderlund

Re: [PATCH 4/6] media: rcar-vin: add support for V4L2_FIELD_ALTERNATE

[Niklas Söderlund]

Hi Hans,

Thanks for your feedback.

On 2016-08-02 11:41:15 +0200, Hans Verkuil wrote:
> 
> 
> On 07/29/2016 07:40 PM, Niklas S�derlund wrote:
> > The HW can capture both ODD and EVEN fields in separate buffers so it's
> > possible to support this field mode.
> > 
> > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-dma.c  | 26 ++++++++++++++++++++------
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c | 12 ++++++++++++
> >  2 files changed, 32 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> > index dad3b03..bcdec46 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> > @@ -95,6 +95,7 @@
> >  /* Video n Module Status Register bits */
> >  #define VNMS_FBS_MASK		(3 << 3)
> >  #define VNMS_FBS_SHIFT		3
> > +#define VNMS_FS			(1 << 2)
> >  #define VNMS_AV			(1 << 1)
> >  #define VNMS_CA			(1 << 0)
> >  
> > @@ -147,6 +148,7 @@ static int rvin_setup(struct rvin_dev *vin)
> >  	case V4L2_FIELD_INTERLACED_BT:
> >  		vnmc = VNMC_IM_FULL | VNMC_FOC;
> >  		break;
> > +	case V4L2_FIELD_ALTERNATE:
> >  	case V4L2_FIELD_NONE:
> >  		if (vin->continuous) {
> >  			vnmc = VNMC_IM_ODD_EVEN;
> > @@ -322,15 +324,26 @@ static bool rvin_capture_active(struct rvin_dev *vin)
> >  	return rvin_read(vin, VNMS_REG) & VNMS_CA;
> >  }
> >  
> > -static int rvin_get_active_slot(struct rvin_dev *vin)
> > +static int rvin_get_active_slot(struct rvin_dev *vin, u32 vnms)
> >  {
> >  	if (vin->continuous)
> > -		return (rvin_read(vin, VNMS_REG) & VNMS_FBS_MASK)
> > -			>> VNMS_FBS_SHIFT;
> > +		return (vnms & VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
> >  
> >  	return 0;
> >  }
> >  
> > +static enum v4l2_field rvin_get_active_field(struct rvin_dev *vin, u32 vnms)
> > +{
> > +	if (vin->format.field == V4L2_FIELD_ALTERNATE) {
> > +		/* If FS is set it's a Even field */
> > +		if (vnms & VNMS_FS)
> > +			return V4L2_FIELD_BOTTOM;
> > +		return V4L2_FIELD_TOP;
> > +	}
> > +
> > +	return vin->format.field;
> > +}
> > +
> >  static void rvin_set_slot_addr(struct rvin_dev *vin, int slot, dma_addr_t addr)
> >  {
> >  	const struct rvin_video_format *fmt;
> > @@ -871,7 +884,7 @@ static bool rvin_fill_hw(struct rvin_dev *vin)
> >  static irqreturn_t rvin_irq(int irq, void *data)
> >  {
> >  	struct rvin_dev *vin = data;
> > -	u32 int_status;
> > +	u32 int_status, vnms;
> >  	int slot;
> >  	unsigned int sequence, handled = 0;
> >  	unsigned long flags;
> > @@ -898,7 +911,8 @@ static irqreturn_t rvin_irq(int irq, void *data)
> >  	}
> >  
> >  	/* Prepare for capture and update state */
> > -	slot = rvin_get_active_slot(vin);
> > +	vnms = rvin_read(vin, VNMS_REG);
> > +	slot = rvin_get_active_slot(vin, vnms);
> >  	sequence = vin->sequence++;
> >  
> >  	vin_dbg(vin, "IRQ %02d: %d\tbuf0: %c buf1: %c buf2: %c\tmore: %d\n",
> > @@ -913,7 +927,7 @@ static irqreturn_t rvin_irq(int irq, void *data)
> >  		goto done;
> >  
> >  	/* Capture frame */
> > -	vin->queue_buf[slot]->field = vin->format.field;
> > +	vin->queue_buf[slot]->field = rvin_get_active_field(vin, vnms);
> >  	vin->queue_buf[slot]->sequence = sequence;
> >  	vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
> >  	vb2_buffer_done(&vin->queue_buf[slot]->vb2_buf, VB2_BUF_STATE_DONE);
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > index b6e40ea..00ac2b6 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -109,6 +109,7 @@ static int rvin_reset_format(struct rvin_dev *vin)
> >  		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> >  	};
> >  	struct v4l2_mbus_framefmt *mf = &fmt.format;
> > +	v4l2_std_id std;
> >  	int ret;
> >  
> >  	fmt.pad = vin->src_pad_idx;
> > @@ -122,9 +123,19 @@ static int rvin_reset_format(struct rvin_dev *vin)
> >  	vin->format.colorspace	= mf->colorspace;
> >  	vin->format.field	= mf->field;
> >  
> > +	/* If we have a video standard use HW to deinterlace */
> > +	if (vin->format.field == V4L2_FIELD_ALTERNATE &&
> > +	    !v4l2_subdev_call(vin_to_source(vin), video, g_std, &std)) {
> > +		if (std & V4L2_STD_625_50)
> > +			vin->format.field = V4L2_FIELD_INTERLACED_TB;
> > +		else
> > +			vin->format.field = V4L2_FIELD_INTERLACED_BT;
> > +	}
> 
> Huh? ALTERNATE means that the fields are captured separately, i.e. one buffer
> per field.
> 
> There is no HW deinterlacing going on in that case, and ALTERNATE is certainly
> not equal to FIELD_INTERLACED_BT/TB.
> 
> If ALTERNATE is chosen as the field format, then VIDIOC_G_FMT should return
> ALTERNATE as the field format, but in struct v4l2_buffer the field will always
> be TOP or BOTTOM.

Yes, if S_FMT request ALTERNATE then G_FMT will return ALTERNATE. This 
code was meant to make INTERLACE_{TB,BT} the default field selection if 
the subdevice uses V4L2_FIELD_ALTERNATE. The rvin_reset_format() is only 
called in the following cases:

- When the driver is first probed to get initial default values from the 
  subdevice.

- S_STD is called and the width, hight and other parameters from the 
  subdevice needs to be updated.

Is it wrong to use an INTERLACE field as default if the subdevice 
provides ALTERNATE? My goal was to not change the behavior of the 
rcar-vin driver which default uses INTERLACE today? I'm happy to drop 
this part for v2 if it's the wrong thing to do in this case.

> 
> > +
> >  	switch (vin->format.field) {
> >  	case V4L2_FIELD_TOP:
> >  	case V4L2_FIELD_BOTTOM:
> > +	case V4L2_FIELD_ALTERNATE:
> >  		vin->format.height /= 2;
> >  		break;
> >  	case V4L2_FIELD_NONE:
> > @@ -222,6 +233,7 @@ static int __rvin_try_format(struct rvin_dev *vin,
> >  	switch (pix->field) {
> >  	case V4L2_FIELD_TOP:
> >  	case V4L2_FIELD_BOTTOM:
> > +	case V4L2_FIELD_ALTERNATE:
> >  		pix->height /= 2;
> >  		source->height /= 2;
> >  		break;
> > 
> 
> Regards,
> 
> 	Hans

-- 
Regards,
Niklas S�derlund

Re: [PATCH 4/6] media: rcar-vin: add support for V4L2_FIELD_ALTERNATE

[Hans Verkuil]

On 08/02/2016 12:32 PM, Niklas Söderlund wrote:
> Hi Hans,
> 
> Thanks for your feedback.
> 
> On 2016-08-02 11:41:15 +0200, Hans Verkuil wrote:
>>
>>
>> On 07/29/2016 07:40 PM, Niklas Söderlund wrote:
>>> The HW can capture both ODD and EVEN fields in separate buffers so it's
>>> possible to support this field mode.
>>>
>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>> ---
>>>  drivers/media/platform/rcar-vin/rcar-dma.c  | 26 ++++++++++++++++++++------
>>>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 12 ++++++++++++
>>>  2 files changed, 32 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
>>> index dad3b03..bcdec46 100644
>>> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
>>> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
>>> @@ -95,6 +95,7 @@
>>>  /* Video n Module Status Register bits */
>>>  #define VNMS_FBS_MASK		(3 << 3)
>>>  #define VNMS_FBS_SHIFT		3
>>> +#define VNMS_FS			(1 << 2)
>>>  #define VNMS_AV			(1 << 1)
>>>  #define VNMS_CA			(1 << 0)
>>>  
>>> @@ -147,6 +148,7 @@ static int rvin_setup(struct rvin_dev *vin)
>>>  	case V4L2_FIELD_INTERLACED_BT:
>>>  		vnmc = VNMC_IM_FULL | VNMC_FOC;
>>>  		break;
>>> +	case V4L2_FIELD_ALTERNATE:
>>>  	case V4L2_FIELD_NONE:
>>>  		if (vin->continuous) {
>>>  			vnmc = VNMC_IM_ODD_EVEN;
>>> @@ -322,15 +324,26 @@ static bool rvin_capture_active(struct rvin_dev *vin)
>>>  	return rvin_read(vin, VNMS_REG) & VNMS_CA;
>>>  }
>>>  
>>> -static int rvin_get_active_slot(struct rvin_dev *vin)
>>> +static int rvin_get_active_slot(struct rvin_dev *vin, u32 vnms)
>>>  {
>>>  	if (vin->continuous)
>>> -		return (rvin_read(vin, VNMS_REG) & VNMS_FBS_MASK)
>>> -			>> VNMS_FBS_SHIFT;
>>> +		return (vnms & VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
>>>  
>>>  	return 0;
>>>  }
>>>  
>>> +static enum v4l2_field rvin_get_active_field(struct rvin_dev *vin, u32 vnms)
>>> +{
>>> +	if (vin->format.field == V4L2_FIELD_ALTERNATE) {
>>> +		/* If FS is set it's a Even field */
>>> +		if (vnms & VNMS_FS)
>>> +			return V4L2_FIELD_BOTTOM;
>>> +		return V4L2_FIELD_TOP;
>>> +	}
>>> +
>>> +	return vin->format.field;
>>> +}
>>> +
>>>  static void rvin_set_slot_addr(struct rvin_dev *vin, int slot, dma_addr_t addr)
>>>  {
>>>  	const struct rvin_video_format *fmt;
>>> @@ -871,7 +884,7 @@ static bool rvin_fill_hw(struct rvin_dev *vin)
>>>  static irqreturn_t rvin_irq(int irq, void *data)
>>>  {
>>>  	struct rvin_dev *vin = data;
>>> -	u32 int_status;
>>> +	u32 int_status, vnms;
>>>  	int slot;
>>>  	unsigned int sequence, handled = 0;
>>>  	unsigned long flags;
>>> @@ -898,7 +911,8 @@ static irqreturn_t rvin_irq(int irq, void *data)
>>>  	}
>>>  
>>>  	/* Prepare for capture and update state */
>>> -	slot = rvin_get_active_slot(vin);
>>> +	vnms = rvin_read(vin, VNMS_REG);
>>> +	slot = rvin_get_active_slot(vin, vnms);
>>>  	sequence = vin->sequence++;
>>>  
>>>  	vin_dbg(vin, "IRQ %02d: %d\tbuf0: %c buf1: %c buf2: %c\tmore: %d\n",
>>> @@ -913,7 +927,7 @@ static irqreturn_t rvin_irq(int irq, void *data)
>>>  		goto done;
>>>  
>>>  	/* Capture frame */
>>> -	vin->queue_buf[slot]->field = vin->format.field;
>>> +	vin->queue_buf[slot]->field = rvin_get_active_field(vin, vnms);
>>>  	vin->queue_buf[slot]->sequence = sequence;
>>>  	vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
>>>  	vb2_buffer_done(&vin->queue_buf[slot]->vb2_buf, VB2_BUF_STATE_DONE);
>>> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
>>> index b6e40ea..00ac2b6 100644
>>> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
>>> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
>>> @@ -109,6 +109,7 @@ static int rvin_reset_format(struct rvin_dev *vin)
>>>  		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>>>  	};
>>>  	struct v4l2_mbus_framefmt *mf = &fmt.format;
>>> +	v4l2_std_id std;
>>>  	int ret;
>>>  
>>>  	fmt.pad = vin->src_pad_idx;
>>> @@ -122,9 +123,19 @@ static int rvin_reset_format(struct rvin_dev *vin)
>>>  	vin->format.colorspace	= mf->colorspace;
>>>  	vin->format.field	= mf->field;
>>>  
>>> +	/* If we have a video standard use HW to deinterlace */
>>> +	if (vin->format.field == V4L2_FIELD_ALTERNATE &&
>>> +	    !v4l2_subdev_call(vin_to_source(vin), video, g_std, &std)) {
>>> +		if (std & V4L2_STD_625_50)
>>> +			vin->format.field = V4L2_FIELD_INTERLACED_TB;
>>> +		else
>>> +			vin->format.field = V4L2_FIELD_INTERLACED_BT;
>>> +	}
>>
>> Huh? ALTERNATE means that the fields are captured separately, i.e. one buffer
>> per field.
>>
>> There is no HW deinterlacing going on in that case, and ALTERNATE is certainly
>> not equal to FIELD_INTERLACED_BT/TB.
>>
>> If ALTERNATE is chosen as the field format, then VIDIOC_G_FMT should return
>> ALTERNATE as the field format, but in struct v4l2_buffer the field will always
>> be TOP or BOTTOM.
> 
> Yes, if S_FMT request ALTERNATE then G_FMT will return ALTERNATE. This 
> code was meant to make INTERLACE_{TB,BT} the default field selection if 
> the subdevice uses V4L2_FIELD_ALTERNATE. The rvin_reset_format() is only 
> called in the following cases:
> 
> - When the driver is first probed to get initial default values from the 
>   subdevice.
> 
> - S_STD is called and the width, hight and other parameters from the 
>   subdevice needs to be updated.
> 
> Is it wrong to use an INTERLACE field as default if the subdevice 
> provides ALTERNATE? My goal was to not change the behavior of the 
> rcar-vin driver which default uses INTERLACE today? I'm happy to drop 
> this part for v2 if it's the wrong thing to do in this case.

It depends. If the subdev returns ALTERNATE, then the SoC receives the
video data as successive fields. How are those processed? Are they combined
into frames? If so, then INTERLACED would be correct. If they are kept as
separate fields, then the rcar driver should say ALTERNATE as well. If they
are placed in one buffer as the top field followed by the bottom field, then
SEQ_BT/TB is the correct field format.

Since I don't know the capabilities of the rcar HW and driver in this regard,
I can't really say if it is right or wrong.

Regards,

	Hans

> 
>>
>>> +
>>>  	switch (vin->format.field) {
>>>  	case V4L2_FIELD_TOP:
>>>  	case V4L2_FIELD_BOTTOM:
>>> +	case V4L2_FIELD_ALTERNATE:
>>>  		vin->format.height /= 2;
>>>  		break;
>>>  	case V4L2_FIELD_NONE:
>>> @@ -222,6 +233,7 @@ static int __rvin_try_format(struct rvin_dev *vin,
>>>  	switch (pix->field) {
>>>  	case V4L2_FIELD_TOP:
>>>  	case V4L2_FIELD_BOTTOM:
>>> +	case V4L2_FIELD_ALTERNATE:
>>>  		pix->height /= 2;
>>>  		source->height /= 2;
>>>  		break;
>>>
>>
>> Regards,
>>
>> 	Hans
> 

Re: [PATCH 4/6] media: rcar-vin: add support for V4L2_FIELD_ALTERNATE

[Hans Verkuil]

On 08/02/2016 12:32 PM, Niklas S�derlund wrote:
> Hi Hans,
> 
> Thanks for your feedback.
> 
> On 2016-08-02 11:41:15 +0200, Hans Verkuil wrote:
>>
>>
>> On 07/29/2016 07:40 PM, Niklas S�derlund wrote:
>>> The HW can capture both ODD and EVEN fields in separate buffers so it's
>>> possible to support this field mode.
>>>
>>> Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
>>> ---
>>>  drivers/media/platform/rcar-vin/rcar-dma.c  | 26 ++++++++++++++++++++------
>>>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 12 ++++++++++++
>>>  2 files changed, 32 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
>>> index dad3b03..bcdec46 100644
>>> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
>>> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
>>> @@ -95,6 +95,7 @@
>>>  /* Video n Module Status Register bits */
>>>  #define VNMS_FBS_MASK		(3 << 3)
>>>  #define VNMS_FBS_SHIFT		3
>>> +#define VNMS_FS			(1 << 2)
>>>  #define VNMS_AV			(1 << 1)
>>>  #define VNMS_CA			(1 << 0)
>>>  
>>> @@ -147,6 +148,7 @@ static int rvin_setup(struct rvin_dev *vin)
>>>  	case V4L2_FIELD_INTERLACED_BT:
>>>  		vnmc = VNMC_IM_FULL | VNMC_FOC;
>>>  		break;
>>> +	case V4L2_FIELD_ALTERNATE:
>>>  	case V4L2_FIELD_NONE:
>>>  		if (vin->continuous) {
>>>  			vnmc = VNMC_IM_ODD_EVEN;
>>> @@ -322,15 +324,26 @@ static bool rvin_capture_active(struct rvin_dev *vin)
>>>  	return rvin_read(vin, VNMS_REG) & VNMS_CA;
>>>  }
>>>  
>>> -static int rvin_get_active_slot(struct rvin_dev *vin)
>>> +static int rvin_get_active_slot(struct rvin_dev *vin, u32 vnms)
>>>  {
>>>  	if (vin->continuous)
>>> -		return (rvin_read(vin, VNMS_REG) & VNMS_FBS_MASK)
>>> -			>> VNMS_FBS_SHIFT;
>>> +		return (vnms & VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
>>>  
>>>  	return 0;
>>>  }
>>>  
>>> +static enum v4l2_field rvin_get_active_field(struct rvin_dev *vin, u32 vnms)
>>> +{
>>> +	if (vin->format.field == V4L2_FIELD_ALTERNATE) {
>>> +		/* If FS is set it's a Even field */
>>> +		if (vnms & VNMS_FS)
>>> +			return V4L2_FIELD_BOTTOM;
>>> +		return V4L2_FIELD_TOP;
>>> +	}
>>> +
>>> +	return vin->format.field;
>>> +}
>>> +
>>>  static void rvin_set_slot_addr(struct rvin_dev *vin, int slot, dma_addr_t addr)
>>>  {
>>>  	const struct rvin_video_format *fmt;
>>> @@ -871,7 +884,7 @@ static bool rvin_fill_hw(struct rvin_dev *vin)
>>>  static irqreturn_t rvin_irq(int irq, void *data)
>>>  {
>>>  	struct rvin_dev *vin = data;
>>> -	u32 int_status;
>>> +	u32 int_status, vnms;
>>>  	int slot;
>>>  	unsigned int sequence, handled = 0;
>>>  	unsigned long flags;
>>> @@ -898,7 +911,8 @@ static irqreturn_t rvin_irq(int irq, void *data)
>>>  	}
>>>  
>>>  	/* Prepare for capture and update state */
>>> -	slot = rvin_get_active_slot(vin);
>>> +	vnms = rvin_read(vin, VNMS_REG);
>>> +	slot = rvin_get_active_slot(vin, vnms);
>>>  	sequence = vin->sequence++;
>>>  
>>>  	vin_dbg(vin, "IRQ %02d: %d\tbuf0: %c buf1: %c buf2: %c\tmore: %d\n",
>>> @@ -913,7 +927,7 @@ static irqreturn_t rvin_irq(int irq, void *data)
>>>  		goto done;
>>>  
>>>  	/* Capture frame */
>>> -	vin->queue_buf[slot]->field = vin->format.field;
>>> +	vin->queue_buf[slot]->field = rvin_get_active_field(vin, vnms);
>>>  	vin->queue_buf[slot]->sequence = sequence;
>>>  	vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
>>>  	vb2_buffer_done(&vin->queue_buf[slot]->vb2_buf, VB2_BUF_STATE_DONE);
>>> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
>>> index b6e40ea..00ac2b6 100644
>>> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
>>> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
>>> @@ -109,6 +109,7 @@ static int rvin_reset_format(struct rvin_dev *vin)
>>>  		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>>>  	};
>>>  	struct v4l2_mbus_framefmt *mf = &fmt.format;
>>> +	v4l2_std_id std;
>>>  	int ret;
>>>  
>>>  	fmt.pad = vin->src_pad_idx;
>>> @@ -122,9 +123,19 @@ static int rvin_reset_format(struct rvin_dev *vin)
>>>  	vin->format.colorspace	= mf->colorspace;
>>>  	vin->format.field	= mf->field;
>>>  
>>> +	/* If we have a video standard use HW to deinterlace */
>>> +	if (vin->format.field == V4L2_FIELD_ALTERNATE &&
>>> +	    !v4l2_subdev_call(vin_to_source(vin), video, g_std, &std)) {
>>> +		if (std & V4L2_STD_625_50)
>>> +			vin->format.field = V4L2_FIELD_INTERLACED_TB;
>>> +		else
>>> +			vin->format.field = V4L2_FIELD_INTERLACED_BT;
>>> +	}
>>
>> Huh? ALTERNATE means that the fields are captured separately, i.e. one buffer
>> per field.
>>
>> There is no HW deinterlacing going on in that case, and ALTERNATE is certainly
>> not equal to FIELD_INTERLACED_BT/TB.
>>
>> If ALTERNATE is chosen as the field format, then VIDIOC_G_FMT should return
>> ALTERNATE as the field format, but in struct v4l2_buffer the field will always
>> be TOP or BOTTOM.
> 
> Yes, if S_FMT request ALTERNATE then G_FMT will return ALTERNATE. This 
> code was meant to make INTERLACE_{TB,BT} the default field selection if 
> the subdevice uses V4L2_FIELD_ALTERNATE. The rvin_reset_format() is only 
> called in the following cases:
> 
> - When the driver is first probed to get initial default values from the 
>   subdevice.
> 
> - S_STD is called and the width, hight and other parameters from the 
>   subdevice needs to be updated.
> 
> Is it wrong to use an INTERLACE field as default if the subdevice 
> provides ALTERNATE? My goal was to not change the behavior of the 
> rcar-vin driver which default uses INTERLACE today? I'm happy to drop 
> this part for v2 if it's the wrong thing to do in this case.

It depends. If the subdev returns ALTERNATE, then the SoC receives the
video data as successive fields. How are those processed? Are they combined
into frames? If so, then INTERLACED would be correct. If they are kept as
separate fields, then the rcar driver should say ALTERNATE as well. If they
are placed in one buffer as the top field followed by the bottom field, then
SEQ_BT/TB is the correct field format.

Since I don't know the capabilities of the rcar HW and driver in this regard,
I can't really say if it is right or wrong.

Regards,

	Hans

> 
>>
>>> +
>>>  	switch (vin->format.field) {
>>>  	case V4L2_FIELD_TOP:
>>>  	case V4L2_FIELD_BOTTOM:
>>> +	case V4L2_FIELD_ALTERNATE:
>>>  		vin->format.height /= 2;
>>>  		break;
>>>  	case V4L2_FIELD_NONE:
>>> @@ -222,6 +233,7 @@ static int __rvin_try_format(struct rvin_dev *vin,
>>>  	switch (pix->field) {
>>>  	case V4L2_FIELD_TOP:
>>>  	case V4L2_FIELD_BOTTOM:
>>> +	case V4L2_FIELD_ALTERNATE:
>>>  		pix->height /= 2;
>>>  		source->height /= 2;
>>>  		break;
>>>
>>
>> Regards,
>>
>> 	Hans
> 

Re: [PATCH 4/6] media: rcar-vin: add support for V4L2_FIELD_ALTERNATE

[Niklas Söderlund]

On 2016-08-02 12:39:40 +0200, Hans Verkuil wrote:
> 
> 
> On 08/02/2016 12:32 PM, Niklas Söderlund wrote:
> > Hi Hans,
> > 
> > Thanks for your feedback.
> > 
> > On 2016-08-02 11:41:15 +0200, Hans Verkuil wrote:
> >>
> >>
> >> On 07/29/2016 07:40 PM, Niklas Söderlund wrote:
> >>> The HW can capture both ODD and EVEN fields in separate buffers so it's
> >>> possible to support this field mode.
> >>>
> >>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >>> ---
> >>>  drivers/media/platform/rcar-vin/rcar-dma.c  | 26 ++++++++++++++++++++------
> >>>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 12 ++++++++++++
> >>>  2 files changed, 32 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> >>> index dad3b03..bcdec46 100644
> >>> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> >>> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> >>> @@ -95,6 +95,7 @@
> >>>  /* Video n Module Status Register bits */
> >>>  #define VNMS_FBS_MASK		(3 << 3)
> >>>  #define VNMS_FBS_SHIFT		3
> >>> +#define VNMS_FS			(1 << 2)
> >>>  #define VNMS_AV			(1 << 1)
> >>>  #define VNMS_CA			(1 << 0)
> >>>  
> >>> @@ -147,6 +148,7 @@ static int rvin_setup(struct rvin_dev *vin)
> >>>  	case V4L2_FIELD_INTERLACED_BT:
> >>>  		vnmc = VNMC_IM_FULL | VNMC_FOC;
> >>>  		break;
> >>> +	case V4L2_FIELD_ALTERNATE:
> >>>  	case V4L2_FIELD_NONE:
> >>>  		if (vin->continuous) {
> >>>  			vnmc = VNMC_IM_ODD_EVEN;
> >>> @@ -322,15 +324,26 @@ static bool rvin_capture_active(struct rvin_dev *vin)
> >>>  	return rvin_read(vin, VNMS_REG) & VNMS_CA;
> >>>  }
> >>>  
> >>> -static int rvin_get_active_slot(struct rvin_dev *vin)
> >>> +static int rvin_get_active_slot(struct rvin_dev *vin, u32 vnms)
> >>>  {
> >>>  	if (vin->continuous)
> >>> -		return (rvin_read(vin, VNMS_REG) & VNMS_FBS_MASK)
> >>> -			>> VNMS_FBS_SHIFT;
> >>> +		return (vnms & VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
> >>>  
> >>>  	return 0;
> >>>  }
> >>>  
> >>> +static enum v4l2_field rvin_get_active_field(struct rvin_dev *vin, u32 vnms)
> >>> +{
> >>> +	if (vin->format.field == V4L2_FIELD_ALTERNATE) {
> >>> +		/* If FS is set it's a Even field */
> >>> +		if (vnms & VNMS_FS)
> >>> +			return V4L2_FIELD_BOTTOM;
> >>> +		return V4L2_FIELD_TOP;
> >>> +	}
> >>> +
> >>> +	return vin->format.field;
> >>> +}
> >>> +
> >>>  static void rvin_set_slot_addr(struct rvin_dev *vin, int slot, dma_addr_t addr)
> >>>  {
> >>>  	const struct rvin_video_format *fmt;
> >>> @@ -871,7 +884,7 @@ static bool rvin_fill_hw(struct rvin_dev *vin)
> >>>  static irqreturn_t rvin_irq(int irq, void *data)
> >>>  {
> >>>  	struct rvin_dev *vin = data;
> >>> -	u32 int_status;
> >>> +	u32 int_status, vnms;
> >>>  	int slot;
> >>>  	unsigned int sequence, handled = 0;
> >>>  	unsigned long flags;
> >>> @@ -898,7 +911,8 @@ static irqreturn_t rvin_irq(int irq, void *data)
> >>>  	}
> >>>  
> >>>  	/* Prepare for capture and update state */
> >>> -	slot = rvin_get_active_slot(vin);
> >>> +	vnms = rvin_read(vin, VNMS_REG);
> >>> +	slot = rvin_get_active_slot(vin, vnms);
> >>>  	sequence = vin->sequence++;
> >>>  
> >>>  	vin_dbg(vin, "IRQ %02d: %d\tbuf0: %c buf1: %c buf2: %c\tmore: %d\n",
> >>> @@ -913,7 +927,7 @@ static irqreturn_t rvin_irq(int irq, void *data)
> >>>  		goto done;
> >>>  
> >>>  	/* Capture frame */
> >>> -	vin->queue_buf[slot]->field = vin->format.field;
> >>> +	vin->queue_buf[slot]->field = rvin_get_active_field(vin, vnms);
> >>>  	vin->queue_buf[slot]->sequence = sequence;
> >>>  	vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
> >>>  	vb2_buffer_done(&vin->queue_buf[slot]->vb2_buf, VB2_BUF_STATE_DONE);
> >>> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> >>> index b6e40ea..00ac2b6 100644
> >>> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> >>> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> >>> @@ -109,6 +109,7 @@ static int rvin_reset_format(struct rvin_dev *vin)
> >>>  		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> >>>  	};
> >>>  	struct v4l2_mbus_framefmt *mf = &fmt.format;
> >>> +	v4l2_std_id std;
> >>>  	int ret;
> >>>  
> >>>  	fmt.pad = vin->src_pad_idx;
> >>> @@ -122,9 +123,19 @@ static int rvin_reset_format(struct rvin_dev *vin)
> >>>  	vin->format.colorspace	= mf->colorspace;
> >>>  	vin->format.field	= mf->field;
> >>>  
> >>> +	/* If we have a video standard use HW to deinterlace */
> >>> +	if (vin->format.field == V4L2_FIELD_ALTERNATE &&
> >>> +	    !v4l2_subdev_call(vin_to_source(vin), video, g_std, &std)) {
> >>> +		if (std & V4L2_STD_625_50)
> >>> +			vin->format.field = V4L2_FIELD_INTERLACED_TB;
> >>> +		else
> >>> +			vin->format.field = V4L2_FIELD_INTERLACED_BT;
> >>> +	}
> >>
> >> Huh? ALTERNATE means that the fields are captured separately, i.e. one buffer
> >> per field.
> >>
> >> There is no HW deinterlacing going on in that case, and ALTERNATE is certainly
> >> not equal to FIELD_INTERLACED_BT/TB.
> >>
> >> If ALTERNATE is chosen as the field format, then VIDIOC_G_FMT should return
> >> ALTERNATE as the field format, but in struct v4l2_buffer the field will always
> >> be TOP or BOTTOM.
> > 
> > Yes, if S_FMT request ALTERNATE then G_FMT will return ALTERNATE. This 
> > code was meant to make INTERLACE_{TB,BT} the default field selection if 
> > the subdevice uses V4L2_FIELD_ALTERNATE. The rvin_reset_format() is only 
> > called in the following cases:
> > 
> > - When the driver is first probed to get initial default values from the 
> >   subdevice.
> > 
> > - S_STD is called and the width, hight and other parameters from the 
> >   subdevice needs to be updated.
> > 
> > Is it wrong to use an INTERLACE field as default if the subdevice 
> > provides ALTERNATE? My goal was to not change the behavior of the 
> > rcar-vin driver which default uses INTERLACE today? I'm happy to drop 
> > this part for v2 if it's the wrong thing to do in this case.
> 
> It depends. If the subdev returns ALTERNATE, then the SoC receives the
> video data as successive fields. How are those processed? Are they combined
> into frames? If so, then INTERLACED would be correct. If they are kept as
> separate fields, then the rcar driver should say ALTERNATE as well. If they
> are placed in one buffer as the top field followed by the bottom field, then
> SEQ_BT/TB is the correct field format.

The driver can process video data received as separate successive fields 
in two ways.

1. It can keep them in separate fields and provide them in separate 
   buffers to userspace in the ALTERNATE field format. In this mode it 
   will sett the ODD and EVEN field type to each buffer. This is what 
   happens if the driver is asked with S_FMT to use the ALTERNATE field 
   format.

2. It can combined the two fields into a frame and present that in one 
   buffer to userspace. This is what happens if the driver is asked with 
   S_FMT to use a INTERLACED field format.


I added the logic in question to try to keep the current behavior of the 
rcar-vin driver which would default to a INTERLACED format if it was 
hooked up a adv7180 subdevice.

So the question in this case as I see it is if it's sane to try to 
preserve that or if I should just drop the logic above and default to 
whatever field format the subdevice is using.

The only filed mode I can't figure out how to support with the VIN HW is 
the SEQ_{BT,TB} formats. I guess I can do some tricks with doing two 
captures in to the same buffer only changing the offset. I do however do 
not see a need to add support for this field mode right now.

> 
> Since I don't know the capabilities of the rcar HW and driver in this regard,
> I can't really say if it is right or wrong.
> 
> Regards,
> 
> 	Hans
> 
> > 
> >>
> >>> +
> >>>  	switch (vin->format.field) {
> >>>  	case V4L2_FIELD_TOP:
> >>>  	case V4L2_FIELD_BOTTOM:
> >>> +	case V4L2_FIELD_ALTERNATE:
> >>>  		vin->format.height /= 2;
> >>>  		break;
> >>>  	case V4L2_FIELD_NONE:
> >>> @@ -222,6 +233,7 @@ static int __rvin_try_format(struct rvin_dev *vin,
> >>>  	switch (pix->field) {
> >>>  	case V4L2_FIELD_TOP:
> >>>  	case V4L2_FIELD_BOTTOM:
> >>> +	case V4L2_FIELD_ALTERNATE:
> >>>  		pix->height /= 2;
> >>>  		source->height /= 2;
> >>>  		break;
> >>>
> >>
> >> Regards,
> >>
> >> 	Hans
> > 

-- 
Regards,
Niklas Söderlund

Re: [PATCH 4/6] media: rcar-vin: add support for V4L2_FIELD_ALTERNATE

[Niklas Söderlund]

On 2016-08-02 12:39:40 +0200, Hans Verkuil wrote:
> 
> 
> On 08/02/2016 12:32 PM, Niklas S�derlund wrote:
> > Hi Hans,
> > 
> > Thanks for your feedback.
> > 
> > On 2016-08-02 11:41:15 +0200, Hans Verkuil wrote:
> >>
> >>
> >> On 07/29/2016 07:40 PM, Niklas S�derlund wrote:
> >>> The HW can capture both ODD and EVEN fields in separate buffers so it's
> >>> possible to support this field mode.
> >>>
> >>> Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> >>> ---
> >>>  drivers/media/platform/rcar-vin/rcar-dma.c  | 26 ++++++++++++++++++++------
> >>>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 12 ++++++++++++
> >>>  2 files changed, 32 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> >>> index dad3b03..bcdec46 100644
> >>> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> >>> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> >>> @@ -95,6 +95,7 @@
> >>>  /* Video n Module Status Register bits */
> >>>  #define VNMS_FBS_MASK		(3 << 3)
> >>>  #define VNMS_FBS_SHIFT		3
> >>> +#define VNMS_FS			(1 << 2)
> >>>  #define VNMS_AV			(1 << 1)
> >>>  #define VNMS_CA			(1 << 0)
> >>>  
> >>> @@ -147,6 +148,7 @@ static int rvin_setup(struct rvin_dev *vin)
> >>>  	case V4L2_FIELD_INTERLACED_BT:
> >>>  		vnmc = VNMC_IM_FULL | VNMC_FOC;
> >>>  		break;
> >>> +	case V4L2_FIELD_ALTERNATE:
> >>>  	case V4L2_FIELD_NONE:
> >>>  		if (vin->continuous) {
> >>>  			vnmc = VNMC_IM_ODD_EVEN;
> >>> @@ -322,15 +324,26 @@ static bool rvin_capture_active(struct rvin_dev *vin)
> >>>  	return rvin_read(vin, VNMS_REG) & VNMS_CA;
> >>>  }
> >>>  
> >>> -static int rvin_get_active_slot(struct rvin_dev *vin)
> >>> +static int rvin_get_active_slot(struct rvin_dev *vin, u32 vnms)
> >>>  {
> >>>  	if (vin->continuous)
> >>> -		return (rvin_read(vin, VNMS_REG) & VNMS_FBS_MASK)
> >>> -			>> VNMS_FBS_SHIFT;
> >>> +		return (vnms & VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
> >>>  
> >>>  	return 0;
> >>>  }
> >>>  
> >>> +static enum v4l2_field rvin_get_active_field(struct rvin_dev *vin, u32 vnms)
> >>> +{
> >>> +	if (vin->format.field == V4L2_FIELD_ALTERNATE) {
> >>> +		/* If FS is set it's a Even field */
> >>> +		if (vnms & VNMS_FS)
> >>> +			return V4L2_FIELD_BOTTOM;
> >>> +		return V4L2_FIELD_TOP;
> >>> +	}
> >>> +
> >>> +	return vin->format.field;
> >>> +}
> >>> +
> >>>  static void rvin_set_slot_addr(struct rvin_dev *vin, int slot, dma_addr_t addr)
> >>>  {
> >>>  	const struct rvin_video_format *fmt;
> >>> @@ -871,7 +884,7 @@ static bool rvin_fill_hw(struct rvin_dev *vin)
> >>>  static irqreturn_t rvin_irq(int irq, void *data)
> >>>  {
> >>>  	struct rvin_dev *vin = data;
> >>> -	u32 int_status;
> >>> +	u32 int_status, vnms;
> >>>  	int slot;
> >>>  	unsigned int sequence, handled = 0;
> >>>  	unsigned long flags;
> >>> @@ -898,7 +911,8 @@ static irqreturn_t rvin_irq(int irq, void *data)
> >>>  	}
> >>>  
> >>>  	/* Prepare for capture and update state */
> >>> -	slot = rvin_get_active_slot(vin);
> >>> +	vnms = rvin_read(vin, VNMS_REG);
> >>> +	slot = rvin_get_active_slot(vin, vnms);
> >>>  	sequence = vin->sequence++;
> >>>  
> >>>  	vin_dbg(vin, "IRQ %02d: %d\tbuf0: %c buf1: %c buf2: %c\tmore: %d\n",
> >>> @@ -913,7 +927,7 @@ static irqreturn_t rvin_irq(int irq, void *data)
> >>>  		goto done;
> >>>  
> >>>  	/* Capture frame */
> >>> -	vin->queue_buf[slot]->field = vin->format.field;
> >>> +	vin->queue_buf[slot]->field = rvin_get_active_field(vin, vnms);
> >>>  	vin->queue_buf[slot]->sequence = sequence;
> >>>  	vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
> >>>  	vb2_buffer_done(&vin->queue_buf[slot]->vb2_buf, VB2_BUF_STATE_DONE);
> >>> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> >>> index b6e40ea..00ac2b6 100644
> >>> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> >>> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> >>> @@ -109,6 +109,7 @@ static int rvin_reset_format(struct rvin_dev *vin)
> >>>  		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
> >>>  	};
> >>>  	struct v4l2_mbus_framefmt *mf = &fmt.format;
> >>> +	v4l2_std_id std;
> >>>  	int ret;
> >>>  
> >>>  	fmt.pad = vin->src_pad_idx;
> >>> @@ -122,9 +123,19 @@ static int rvin_reset_format(struct rvin_dev *vin)
> >>>  	vin->format.colorspace	= mf->colorspace;
> >>>  	vin->format.field	= mf->field;
> >>>  
> >>> +	/* If we have a video standard use HW to deinterlace */
> >>> +	if (vin->format.field == V4L2_FIELD_ALTERNATE &&
> >>> +	    !v4l2_subdev_call(vin_to_source(vin), video, g_std, &std)) {
> >>> +		if (std & V4L2_STD_625_50)
> >>> +			vin->format.field = V4L2_FIELD_INTERLACED_TB;
> >>> +		else
> >>> +			vin->format.field = V4L2_FIELD_INTERLACED_BT;
> >>> +	}
> >>
> >> Huh? ALTERNATE means that the fields are captured separately, i.e. one buffer
> >> per field.
> >>
> >> There is no HW deinterlacing going on in that case, and ALTERNATE is certainly
> >> not equal to FIELD_INTERLACED_BT/TB.
> >>
> >> If ALTERNATE is chosen as the field format, then VIDIOC_G_FMT should return
> >> ALTERNATE as the field format, but in struct v4l2_buffer the field will always
> >> be TOP or BOTTOM.
> > 
> > Yes, if S_FMT request ALTERNATE then G_FMT will return ALTERNATE. This 
> > code was meant to make INTERLACE_{TB,BT} the default field selection if 
> > the subdevice uses V4L2_FIELD_ALTERNATE. The rvin_reset_format() is only 
> > called in the following cases:
> > 
> > - When the driver is first probed to get initial default values from the 
> >   subdevice.
> > 
> > - S_STD is called and the width, hight and other parameters from the 
> >   subdevice needs to be updated.
> > 
> > Is it wrong to use an INTERLACE field as default if the subdevice 
> > provides ALTERNATE? My goal was to not change the behavior of the 
> > rcar-vin driver which default uses INTERLACE today? I'm happy to drop 
> > this part for v2 if it's the wrong thing to do in this case.
> 
> It depends. If the subdev returns ALTERNATE, then the SoC receives the
> video data as successive fields. How are those processed? Are they combined
> into frames? If so, then INTERLACED would be correct. If they are kept as
> separate fields, then the rcar driver should say ALTERNATE as well. If they
> are placed in one buffer as the top field followed by the bottom field, then
> SEQ_BT/TB is the correct field format.

The driver can process video data received as separate successive fields 
in two ways.

1. It can keep them in separate fields and provide them in separate 
   buffers to userspace in the ALTERNATE field format. In this mode it 
   will sett the ODD and EVEN field type to each buffer. This is what 
   happens if the driver is asked with S_FMT to use the ALTERNATE field 
   format.

2. It can combined the two fields into a frame and present that in one 
   buffer to userspace. This is what happens if the driver is asked with 
   S_FMT to use a INTERLACED field format.


I added the logic in question to try to keep the current behavior of the 
rcar-vin driver which would default to a INTERLACED format if it was 
hooked up a adv7180 subdevice.

So the question in this case as I see it is if it's sane to try to 
preserve that or if I should just drop the logic above and default to 
whatever field format the subdevice is using.

The only filed mode I can't figure out how to support with the VIN HW is 
the SEQ_{BT,TB} formats. I guess I can do some tricks with doing two 
captures in to the same buffer only changing the offset. I do however do 
not see a need to add support for this field mode right now.

> 
> Since I don't know the capabilities of the rcar HW and driver in this regard,
> I can't really say if it is right or wrong.
> 
> Regards,
> 
> 	Hans
> 
> > 
> >>
> >>> +
> >>>  	switch (vin->format.field) {
> >>>  	case V4L2_FIELD_TOP:
> >>>  	case V4L2_FIELD_BOTTOM:
> >>> +	case V4L2_FIELD_ALTERNATE:
> >>>  		vin->format.height /= 2;
> >>>  		break;
> >>>  	case V4L2_FIELD_NONE:
> >>> @@ -222,6 +233,7 @@ static int __rvin_try_format(struct rvin_dev *vin,
> >>>  	switch (pix->field) {
> >>>  	case V4L2_FIELD_TOP:
> >>>  	case V4L2_FIELD_BOTTOM:
> >>> +	case V4L2_FIELD_ALTERNATE:
> >>>  		pix->height /= 2;
> >>>  		source->height /= 2;
> >>>  		break;
> >>>
> >>
> >> Regards,
> >>
> >> 	Hans
> > 

-- 
Regards,
Niklas S�derlund

Re: [PATCH 4/6] media: rcar-vin: add support for V4L2_FIELD_ALTERNATE

[Hans Verkuil]

On 08/02/2016 01:02 PM, Niklas Söderlund wrote:
> On 2016-08-02 12:39:40 +0200, Hans Verkuil wrote:
>>
>>
>> On 08/02/2016 12:32 PM, Niklas Söderlund wrote:
>>> Hi Hans,
>>>
>>> Thanks for your feedback.
>>>
>>> On 2016-08-02 11:41:15 +0200, Hans Verkuil wrote:
>>>>
>>>>
>>>> On 07/29/2016 07:40 PM, Niklas Söderlund wrote:
>>>>> The HW can capture both ODD and EVEN fields in separate buffers so it's
>>>>> possible to support this field mode.
>>>>>
>>>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>>>> ---
>>>>>  drivers/media/platform/rcar-vin/rcar-dma.c  | 26 ++++++++++++++++++++------
>>>>>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 12 ++++++++++++
>>>>>  2 files changed, 32 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
>>>>> index dad3b03..bcdec46 100644
>>>>> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
>>>>> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
>>>>> @@ -95,6 +95,7 @@
>>>>>  /* Video n Module Status Register bits */
>>>>>  #define VNMS_FBS_MASK		(3 << 3)
>>>>>  #define VNMS_FBS_SHIFT		3
>>>>> +#define VNMS_FS			(1 << 2)
>>>>>  #define VNMS_AV			(1 << 1)
>>>>>  #define VNMS_CA			(1 << 0)
>>>>>  
>>>>> @@ -147,6 +148,7 @@ static int rvin_setup(struct rvin_dev *vin)
>>>>>  	case V4L2_FIELD_INTERLACED_BT:
>>>>>  		vnmc = VNMC_IM_FULL | VNMC_FOC;
>>>>>  		break;
>>>>> +	case V4L2_FIELD_ALTERNATE:
>>>>>  	case V4L2_FIELD_NONE:
>>>>>  		if (vin->continuous) {
>>>>>  			vnmc = VNMC_IM_ODD_EVEN;
>>>>> @@ -322,15 +324,26 @@ static bool rvin_capture_active(struct rvin_dev *vin)
>>>>>  	return rvin_read(vin, VNMS_REG) & VNMS_CA;
>>>>>  }
>>>>>  
>>>>> -static int rvin_get_active_slot(struct rvin_dev *vin)
>>>>> +static int rvin_get_active_slot(struct rvin_dev *vin, u32 vnms)
>>>>>  {
>>>>>  	if (vin->continuous)
>>>>> -		return (rvin_read(vin, VNMS_REG) & VNMS_FBS_MASK)
>>>>> -			>> VNMS_FBS_SHIFT;
>>>>> +		return (vnms & VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
>>>>>  
>>>>>  	return 0;
>>>>>  }
>>>>>  
>>>>> +static enum v4l2_field rvin_get_active_field(struct rvin_dev *vin, u32 vnms)
>>>>> +{
>>>>> +	if (vin->format.field == V4L2_FIELD_ALTERNATE) {
>>>>> +		/* If FS is set it's a Even field */
>>>>> +		if (vnms & VNMS_FS)
>>>>> +			return V4L2_FIELD_BOTTOM;
>>>>> +		return V4L2_FIELD_TOP;
>>>>> +	}
>>>>> +
>>>>> +	return vin->format.field;
>>>>> +}
>>>>> +
>>>>>  static void rvin_set_slot_addr(struct rvin_dev *vin, int slot, dma_addr_t addr)
>>>>>  {
>>>>>  	const struct rvin_video_format *fmt;
>>>>> @@ -871,7 +884,7 @@ static bool rvin_fill_hw(struct rvin_dev *vin)
>>>>>  static irqreturn_t rvin_irq(int irq, void *data)
>>>>>  {
>>>>>  	struct rvin_dev *vin = data;
>>>>> -	u32 int_status;
>>>>> +	u32 int_status, vnms;
>>>>>  	int slot;
>>>>>  	unsigned int sequence, handled = 0;
>>>>>  	unsigned long flags;
>>>>> @@ -898,7 +911,8 @@ static irqreturn_t rvin_irq(int irq, void *data)
>>>>>  	}
>>>>>  
>>>>>  	/* Prepare for capture and update state */
>>>>> -	slot = rvin_get_active_slot(vin);
>>>>> +	vnms = rvin_read(vin, VNMS_REG);
>>>>> +	slot = rvin_get_active_slot(vin, vnms);
>>>>>  	sequence = vin->sequence++;
>>>>>  
>>>>>  	vin_dbg(vin, "IRQ %02d: %d\tbuf0: %c buf1: %c buf2: %c\tmore: %d\n",
>>>>> @@ -913,7 +927,7 @@ static irqreturn_t rvin_irq(int irq, void *data)
>>>>>  		goto done;
>>>>>  
>>>>>  	/* Capture frame */
>>>>> -	vin->queue_buf[slot]->field = vin->format.field;
>>>>> +	vin->queue_buf[slot]->field = rvin_get_active_field(vin, vnms);
>>>>>  	vin->queue_buf[slot]->sequence = sequence;
>>>>>  	vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
>>>>>  	vb2_buffer_done(&vin->queue_buf[slot]->vb2_buf, VB2_BUF_STATE_DONE);
>>>>> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
>>>>> index b6e40ea..00ac2b6 100644
>>>>> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
>>>>> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
>>>>> @@ -109,6 +109,7 @@ static int rvin_reset_format(struct rvin_dev *vin)
>>>>>  		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>>>>>  	};
>>>>>  	struct v4l2_mbus_framefmt *mf = &fmt.format;
>>>>> +	v4l2_std_id std;
>>>>>  	int ret;
>>>>>  
>>>>>  	fmt.pad = vin->src_pad_idx;
>>>>> @@ -122,9 +123,19 @@ static int rvin_reset_format(struct rvin_dev *vin)
>>>>>  	vin->format.colorspace	= mf->colorspace;
>>>>>  	vin->format.field	= mf->field;
>>>>>  
>>>>> +	/* If we have a video standard use HW to deinterlace */
>>>>> +	if (vin->format.field == V4L2_FIELD_ALTERNATE &&
>>>>> +	    !v4l2_subdev_call(vin_to_source(vin), video, g_std, &std)) {
>>>>> +		if (std & V4L2_STD_625_50)
>>>>> +			vin->format.field = V4L2_FIELD_INTERLACED_TB;
>>>>> +		else
>>>>> +			vin->format.field = V4L2_FIELD_INTERLACED_BT;
>>>>> +	}
>>>>
>>>> Huh? ALTERNATE means that the fields are captured separately, i.e. one buffer
>>>> per field.
>>>>
>>>> There is no HW deinterlacing going on in that case, and ALTERNATE is certainly
>>>> not equal to FIELD_INTERLACED_BT/TB.
>>>>
>>>> If ALTERNATE is chosen as the field format, then VIDIOC_G_FMT should return
>>>> ALTERNATE as the field format, but in struct v4l2_buffer the field will always
>>>> be TOP or BOTTOM.
>>>
>>> Yes, if S_FMT request ALTERNATE then G_FMT will return ALTERNATE. This 
>>> code was meant to make INTERLACE_{TB,BT} the default field selection if 
>>> the subdevice uses V4L2_FIELD_ALTERNATE. The rvin_reset_format() is only 
>>> called in the following cases:
>>>
>>> - When the driver is first probed to get initial default values from the 
>>>   subdevice.
>>>
>>> - S_STD is called and the width, hight and other parameters from the 
>>>   subdevice needs to be updated.
>>>
>>> Is it wrong to use an INTERLACE field as default if the subdevice 
>>> provides ALTERNATE? My goal was to not change the behavior of the 
>>> rcar-vin driver which default uses INTERLACE today? I'm happy to drop 
>>> this part for v2 if it's the wrong thing to do in this case.
>>
>> It depends. If the subdev returns ALTERNATE, then the SoC receives the
>> video data as successive fields. How are those processed? Are they combined
>> into frames? If so, then INTERLACED would be correct. If they are kept as
>> separate fields, then the rcar driver should say ALTERNATE as well. If they
>> are placed in one buffer as the top field followed by the bottom field, then
>> SEQ_BT/TB is the correct field format.
> 
> The driver can process video data received as separate successive fields 
> in two ways.
> 
> 1. It can keep them in separate fields and provide them in separate 
>    buffers to userspace in the ALTERNATE field format. In this mode it 
>    will sett the ODD and EVEN field type to each buffer. This is what 
>    happens if the driver is asked with S_FMT to use the ALTERNATE field 
>    format.
> 
> 2. It can combined the two fields into a frame and present that in one 
>    buffer to userspace. This is what happens if the driver is asked with 
>    S_FMT to use a INTERLACED field format.
> 
> 
> I added the logic in question to try to keep the current behavior of the 
> rcar-vin driver which would default to a INTERLACED format if it was 
> hooked up a adv7180 subdevice.
> 
> So the question in this case as I see it is if it's sane to try to 
> preserve that or if I should just drop the logic above and default to 
> whatever field format the subdevice is using.

No, combining the two fields into a single interlaced frame is the way to
go by default. Most applications expect INTERLACED. Support for ALTERNATE
is a lot less common.

It would probably be helpful if you added a few comments about this to the
code.

> The only filed mode I can't figure out how to support with the VIN HW is 
> the SEQ_{BT,TB} formats. I guess I can do some tricks with doing two 
> captures in to the same buffer only changing the offset. I do however do 
> not see a need to add support for this field mode right now.

These are rarely used and few applications can handle this.

I understand that you're working on a v2 of this patch series? If so, then
I'll review that v2 with this information in mind.

Regards,

	Hans

Re: [PATCH 4/6] media: rcar-vin: add support for V4L2_FIELD_ALTERNATE

[Hans Verkuil]

On 08/02/2016 01:02 PM, Niklas S�derlund wrote:
> On 2016-08-02 12:39:40 +0200, Hans Verkuil wrote:
>>
>>
>> On 08/02/2016 12:32 PM, Niklas S�derlund wrote:
>>> Hi Hans,
>>>
>>> Thanks for your feedback.
>>>
>>> On 2016-08-02 11:41:15 +0200, Hans Verkuil wrote:
>>>>
>>>>
>>>> On 07/29/2016 07:40 PM, Niklas S�derlund wrote:
>>>>> The HW can capture both ODD and EVEN fields in separate buffers so it's
>>>>> possible to support this field mode.
>>>>>
>>>>> Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
>>>>> ---
>>>>>  drivers/media/platform/rcar-vin/rcar-dma.c  | 26 ++++++++++++++++++++------
>>>>>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 12 ++++++++++++
>>>>>  2 files changed, 32 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
>>>>> index dad3b03..bcdec46 100644
>>>>> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
>>>>> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
>>>>> @@ -95,6 +95,7 @@
>>>>>  /* Video n Module Status Register bits */
>>>>>  #define VNMS_FBS_MASK		(3 << 3)
>>>>>  #define VNMS_FBS_SHIFT		3
>>>>> +#define VNMS_FS			(1 << 2)
>>>>>  #define VNMS_AV			(1 << 1)
>>>>>  #define VNMS_CA			(1 << 0)
>>>>>  
>>>>> @@ -147,6 +148,7 @@ static int rvin_setup(struct rvin_dev *vin)
>>>>>  	case V4L2_FIELD_INTERLACED_BT:
>>>>>  		vnmc = VNMC_IM_FULL | VNMC_FOC;
>>>>>  		break;
>>>>> +	case V4L2_FIELD_ALTERNATE:
>>>>>  	case V4L2_FIELD_NONE:
>>>>>  		if (vin->continuous) {
>>>>>  			vnmc = VNMC_IM_ODD_EVEN;
>>>>> @@ -322,15 +324,26 @@ static bool rvin_capture_active(struct rvin_dev *vin)
>>>>>  	return rvin_read(vin, VNMS_REG) & VNMS_CA;
>>>>>  }
>>>>>  
>>>>> -static int rvin_get_active_slot(struct rvin_dev *vin)
>>>>> +static int rvin_get_active_slot(struct rvin_dev *vin, u32 vnms)
>>>>>  {
>>>>>  	if (vin->continuous)
>>>>> -		return (rvin_read(vin, VNMS_REG) & VNMS_FBS_MASK)
>>>>> -			>> VNMS_FBS_SHIFT;
>>>>> +		return (vnms & VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
>>>>>  
>>>>>  	return 0;
>>>>>  }
>>>>>  
>>>>> +static enum v4l2_field rvin_get_active_field(struct rvin_dev *vin, u32 vnms)
>>>>> +{
>>>>> +	if (vin->format.field == V4L2_FIELD_ALTERNATE) {
>>>>> +		/* If FS is set it's a Even field */
>>>>> +		if (vnms & VNMS_FS)
>>>>> +			return V4L2_FIELD_BOTTOM;
>>>>> +		return V4L2_FIELD_TOP;
>>>>> +	}
>>>>> +
>>>>> +	return vin->format.field;
>>>>> +}
>>>>> +
>>>>>  static void rvin_set_slot_addr(struct rvin_dev *vin, int slot, dma_addr_t addr)
>>>>>  {
>>>>>  	const struct rvin_video_format *fmt;
>>>>> @@ -871,7 +884,7 @@ static bool rvin_fill_hw(struct rvin_dev *vin)
>>>>>  static irqreturn_t rvin_irq(int irq, void *data)
>>>>>  {
>>>>>  	struct rvin_dev *vin = data;
>>>>> -	u32 int_status;
>>>>> +	u32 int_status, vnms;
>>>>>  	int slot;
>>>>>  	unsigned int sequence, handled = 0;
>>>>>  	unsigned long flags;
>>>>> @@ -898,7 +911,8 @@ static irqreturn_t rvin_irq(int irq, void *data)
>>>>>  	}
>>>>>  
>>>>>  	/* Prepare for capture and update state */
>>>>> -	slot = rvin_get_active_slot(vin);
>>>>> +	vnms = rvin_read(vin, VNMS_REG);
>>>>> +	slot = rvin_get_active_slot(vin, vnms);
>>>>>  	sequence = vin->sequence++;
>>>>>  
>>>>>  	vin_dbg(vin, "IRQ %02d: %d\tbuf0: %c buf1: %c buf2: %c\tmore: %d\n",
>>>>> @@ -913,7 +927,7 @@ static irqreturn_t rvin_irq(int irq, void *data)
>>>>>  		goto done;
>>>>>  
>>>>>  	/* Capture frame */
>>>>> -	vin->queue_buf[slot]->field = vin->format.field;
>>>>> +	vin->queue_buf[slot]->field = rvin_get_active_field(vin, vnms);
>>>>>  	vin->queue_buf[slot]->sequence = sequence;
>>>>>  	vin->queue_buf[slot]->vb2_buf.timestamp = ktime_get_ns();
>>>>>  	vb2_buffer_done(&vin->queue_buf[slot]->vb2_buf, VB2_BUF_STATE_DONE);
>>>>> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
>>>>> index b6e40ea..00ac2b6 100644
>>>>> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
>>>>> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
>>>>> @@ -109,6 +109,7 @@ static int rvin_reset_format(struct rvin_dev *vin)
>>>>>  		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
>>>>>  	};
>>>>>  	struct v4l2_mbus_framefmt *mf = &fmt.format;
>>>>> +	v4l2_std_id std;
>>>>>  	int ret;
>>>>>  
>>>>>  	fmt.pad = vin->src_pad_idx;
>>>>> @@ -122,9 +123,19 @@ static int rvin_reset_format(struct rvin_dev *vin)
>>>>>  	vin->format.colorspace	= mf->colorspace;
>>>>>  	vin->format.field	= mf->field;
>>>>>  
>>>>> +	/* If we have a video standard use HW to deinterlace */
>>>>> +	if (vin->format.field == V4L2_FIELD_ALTERNATE &&
>>>>> +	    !v4l2_subdev_call(vin_to_source(vin), video, g_std, &std)) {
>>>>> +		if (std & V4L2_STD_625_50)
>>>>> +			vin->format.field = V4L2_FIELD_INTERLACED_TB;
>>>>> +		else
>>>>> +			vin->format.field = V4L2_FIELD_INTERLACED_BT;
>>>>> +	}
>>>>
>>>> Huh? ALTERNATE means that the fields are captured separately, i.e. one buffer
>>>> per field.
>>>>
>>>> There is no HW deinterlacing going on in that case, and ALTERNATE is certainly
>>>> not equal to FIELD_INTERLACED_BT/TB.
>>>>
>>>> If ALTERNATE is chosen as the field format, then VIDIOC_G_FMT should return
>>>> ALTERNATE as the field format, but in struct v4l2_buffer the field will always
>>>> be TOP or BOTTOM.
>>>
>>> Yes, if S_FMT request ALTERNATE then G_FMT will return ALTERNATE. This 
>>> code was meant to make INTERLACE_{TB,BT} the default field selection if 
>>> the subdevice uses V4L2_FIELD_ALTERNATE. The rvin_reset_format() is only 
>>> called in the following cases:
>>>
>>> - When the driver is first probed to get initial default values from the 
>>>   subdevice.
>>>
>>> - S_STD is called and the width, hight and other parameters from the 
>>>   subdevice needs to be updated.
>>>
>>> Is it wrong to use an INTERLACE field as default if the subdevice 
>>> provides ALTERNATE? My goal was to not change the behavior of the 
>>> rcar-vin driver which default uses INTERLACE today? I'm happy to drop 
>>> this part for v2 if it's the wrong thing to do in this case.
>>
>> It depends. If the subdev returns ALTERNATE, then the SoC receives the
>> video data as successive fields. How are those processed? Are they combined
>> into frames? If so, then INTERLACED would be correct. If they are kept as
>> separate fields, then the rcar driver should say ALTERNATE as well. If they
>> are placed in one buffer as the top field followed by the bottom field, then
>> SEQ_BT/TB is the correct field format.
> 
> The driver can process video data received as separate successive fields 
> in two ways.
> 
> 1. It can keep them in separate fields and provide them in separate 
>    buffers to userspace in the ALTERNATE field format. In this mode it 
>    will sett the ODD and EVEN field type to each buffer. This is what 
>    happens if the driver is asked with S_FMT to use the ALTERNATE field 
>    format.
> 
> 2. It can combined the two fields into a frame and present that in one 
>    buffer to userspace. This is what happens if the driver is asked with 
>    S_FMT to use a INTERLACED field format.
> 
> 
> I added the logic in question to try to keep the current behavior of the 
> rcar-vin driver which would default to a INTERLACED format if it was 
> hooked up a adv7180 subdevice.
> 
> So the question in this case as I see it is if it's sane to try to 
> preserve that or if I should just drop the logic above and default to 
> whatever field format the subdevice is using.

No, combining the two fields into a single interlaced frame is the way to
go by default. Most applications expect INTERLACED. Support for ALTERNATE
is a lot less common.

It would probably be helpful if you added a few comments about this to the
code.

> The only filed mode I can't figure out how to support with the VIN HW is 
> the SEQ_{BT,TB} formats. I guess I can do some tricks with doing two 
> captures in to the same buffer only changing the offset. I do however do 
> not see a need to add support for this field mode right now.

These are rarely used and few applications can handle this.

I understand that you're working on a v2 of this patch series? If so, then
I'll review that v2 with this information in mind.

Regards,

	Hans