[PATCHv2 0/7] Fix adv7180 and rcar-vin field handling

[PATCHv2 0/7] 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 kept 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. Output 
of 'v4l2-compliance -fs' is attached bellow and I have tested all fields 
using qv4l2 and it looks OK to me. I need to disable 'Enable Video 
Scaling' in the 'Capture' Menu for ODD/EVEN/ALTERNATE or I get a 
horizontally stretched image. Also for ALTERNATE the 1 line difference 
between the fields are noticeable. The image jumps up/down 1 line for 
each other field, but I guess that is normal since the fields are 
different right?

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.

* Changes since v1
- Added patch so that V4L2_FIELD_INTERLACED is not treated the same as 
  V4L2_FIELD_INTERLACED_TB. Instead G_STD will be used to get the video 
  standard and make a TB/BT decision based on that.
- Add changelog to Stevens patch which I dropped by mistake when I 
  applied it to my tree.
- Add better commit message, comment explaining that INTERLACED will be 
  used as the default if the subdeivce uses ALTERNATE field mode and 
  implements G_STD.


# v4l2-compliance -d 3 -fs
v4l2-compliance SHA   : 7785594dd82b4fa04585928e5b825a0df73a2774

Driver Info:
	Driver name   : rcar_vin
	Card type     : R_Car_VIN
	Bus info      : platform:e6ef1000.video
	Driver version: 4.7.0
	Capabilities  : 0x85200001
		Video Capture
		Read/Write
		Streaming
		Extended Pix Format
		Device Capabilities
	Device Caps   : 0x05200001
		Video Capture
		Read/Write
		Streaming
		Extended Pix Format

Compliance test for device /dev/video3 (not using libv4l2):

Required ioctls:
	test VIDIOC_QUERYCAP: OK

Allow for multiple opens:
	test second video open: OK
	test VIDIOC_QUERYCAP: OK
	test VIDIOC_G/S_PRIORITY: OK
	test for unlimited opens: OK

Debug ioctls:
	test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
	test VIDIOC_LOG_STATUS: OK

Input ioctls:
	test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
	test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
	test VIDIOC_ENUMAUDIO: OK (Not Supported)
	test VIDIOC_G/S/ENUMINPUT: OK
	test VIDIOC_G/S_AUDIO: OK (Not Supported)
	Inputs: 1 Audio Inputs: 0 Tuners: 0

Output ioctls:
	test VIDIOC_G/S_MODULATOR: OK (Not Supported)
	test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
	test VIDIOC_ENUMAUDOUT: OK (Not Supported)
	test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
	test VIDIOC_G/S_AUDOUT: OK (Not Supported)
	Outputs: 0 Audio Outputs: 0 Modulators: 0

Input/Output configuration ioctls:
	test VIDIOC_ENUM/G/S/QUERY_STD: OK
	test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
	test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
	test VIDIOC_G/S_EDID: OK (Not Supported)

Test input 0:

	Control ioctls:
		test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK
		test VIDIOC_QUERYCTRL: OK
		test VIDIOC_G/S_CTRL: OK
		test VIDIOC_G/S/TRY_EXT_CTRLS: OK
		test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK
		test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
		Standard Controls: 5 Private Controls: 1

	Format ioctls:
		test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
		test VIDIOC_G/S_PARM: OK
		test VIDIOC_G_FBUF: OK (Not Supported)
		test VIDIOC_G_FMT: OK
		test VIDIOC_TRY_FMT: OK
		test VIDIOC_S_FMT: OK
		test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
		test Cropping: OK
		test Composing: OK
		test Scaling: OK

	Codec ioctls:
		test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
		test VIDIOC_G_ENC_INDEX: OK (Not Supported)
		test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)

	Buffer ioctls:
		test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
		test VIDIOC_EXPBUF: OK

Test input 0:

Streaming ioctls:
	test read/write: OK
	test MMAP: OK
	test USERPTR: OK (Not Supported)
	test DMABUF: Cannot test, specify --expbuf-device

Stream using all formats:
	test MMAP for Format NV16, Frame Size 2x4:
		Crop 720x480@0x0, Compose 6x4@0x0, Stride 32, Field None: OK
		Crop 720x240@0x0, Compose 6x4@0x0, Stride 32, Field Top: OK
		Crop 720x240@0x0, Compose 6x4@0x0, Stride 32, Field Bottom: OK
		Crop 720x240@0x0, Compose 6x4@0x0, Stride 32, Field Interlaced: OK
		Crop 720x240@0x0, Compose 6x4@0x0, Stride 32, Field Alternating: OK
		Crop 720x240@0x0, Compose 6x4@0x0, Stride 32, Field Interlaced Top-Bottom: OK
		Crop 720x240@0x0, Compose 6x4@0x0, Stride 32, Field Interlaced Bottom-Top: OK
	test MMAP for Format NV16, Frame Size 2048x2048:
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 2048, Field None: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 2048, Field Top: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 2048, Field Bottom: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 2048, Field Interlaced: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 2048, Field Alternating: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 2048, Field Interlaced Top-Bottom: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 2048, Field Interlaced Bottom-Top: OK
	test MMAP for Format NV16, Frame Size 736x480:
		Crop 720x480@0x0, Compose 736x480@0x0, Stride 736, Field None: OK
		Crop 720x240@0x0, Compose 736x480@0x0, Stride 736, Field Top: OK
		Crop 720x240@0x0, Compose 736x480@0x0, Stride 736, Field Bottom: OK
		Crop 720x240@0x0, Compose 736x480@0x0, Stride 736, Field Interlaced: OK
		Crop 720x240@0x0, Compose 736x480@0x0, Stride 736, Field Alternating: OK
		Crop 720x240@0x0, Compose 736x480@0x0, Stride 736, Field Interlaced Top-Bottom: OK
		Crop 720x240@0x0, Compose 736x480@0x0, Stride 736, Field Interlaced Bottom-Top: OK
	test MMAP for Format YUYV, Frame Size 32x4:
		Crop 720x480@0x0, Compose 32x4@0x0, Stride 64, Field None: OK
		Crop 720x240@0x0, Compose 32x4@0x0, Stride 64, Field Top: OK
		Crop 720x240@0x0, Compose 32x4@0x0, Stride 64, Field Bottom: OK
		Crop 720x240@0x0, Compose 32x4@0x0, Stride 64, Field Interlaced: OK
		Crop 720x240@0x0, Compose 32x4@0x0, Stride 64, Field Alternating: OK
		Crop 720x240@0x0, Compose 32x4@0x0, Stride 64, Field Interlaced Top-Bottom: OK
		Crop 720x240@0x0, Compose 32x4@0x0, Stride 64, Field Interlaced Bottom-Top: OK
	test MMAP for Format YUYV, Frame Size 2048x2048:
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field None: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 4096, Field Top: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 4096, Field Bottom: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 4096, Field Interlaced: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 4096, Field Alternating: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 4096, Field Interlaced Top-Bottom: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 4096, Field Interlaced Bottom-Top: OK
	test MMAP for Format YUYV, Frame Size 736x480:
		Crop 720x480@0x0, Compose 736x480@0x0, Stride 1472, Field None: OK
		Crop 720x240@0x0, Compose 736x480@0x0, Stride 1472, Field Top: OK
		Crop 720x240@0x0, Compose 736x480@0x0, Stride 1472, Field Bottom: OK
		Crop 720x240@0x0, Compose 736x480@0x0, Stride 1472, Field Interlaced: OK
		Crop 720x240@0x0, Compose 736x480@0x0, Stride 1472, Field Alternating: OK
		Crop 720x240@0x0, Compose 736x480@0x0, Stride 1472, Field Interlaced Top-Bottom: OK
		Crop 720x240@0x0, Compose 736x480@0x0, Stride 1472, Field Interlaced Bottom-Top: OK
	test MMAP for Format UYVY, Frame Size 2x4:
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 4, Field None: OK
		Crop 720x240@0x0, Compose 2x4@0x0, Stride 4, Field Top: OK
		Crop 720x240@0x0, Compose 2x4@0x0, Stride 4, Field Bottom: OK
		Crop 720x240@0x0, Compose 2x4@0x0, Stride 4, Field Interlaced: OK
		Crop 720x240@0x0, Compose 2x4@0x0, Stride 4, Field Alternating: OK
		Crop 720x240@0x0, Compose 2x4@0x0, Stride 4, Field Interlaced Top-Bottom: OK
		Crop 720x240@0x0, Compose 2x4@0x0, Stride 4, Field Interlaced Bottom-Top: OK
	test MMAP for Format UYVY, Frame Size 2048x2048:
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field None: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 4096, Field Top: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 4096, Field Bottom: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 4096, Field Interlaced: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 4096, Field Alternating: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 4096, Field Interlaced Top-Bottom: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 4096, Field Interlaced Bottom-Top: OK
	test MMAP for Format UYVY, Frame Size 720x480:
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 1440, Field None: OK
		Crop 720x240@0x0, Compose 720x480@0x0, Stride 1440, Field Top: OK
		Crop 720x240@0x0, Compose 720x480@0x0, Stride 1440, Field Bottom: OK
		Crop 720x240@0x0, Compose 720x480@0x0, Stride 1440, Field Interlaced: OK
		Crop 720x240@0x0, Compose 720x480@0x0, Stride 1440, Field Alternating: OK
		Crop 720x240@0x0, Compose 720x480@0x0, Stride 1440, Field Interlaced Top-Bottom: OK
		Crop 720x240@0x0, Compose 720x480@0x0, Stride 1440, Field Interlaced Bottom-Top: OK
	test MMAP for Format RGBP, Frame Size 2x4:
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 4, Field None: OK
		Crop 720x240@0x0, Compose 2x4@0x0, Stride 4, Field Top: OK
		Crop 720x240@0x0, Compose 2x4@0x0, Stride 4, Field Bottom: OK
		Crop 720x240@0x0, Compose 2x4@0x0, Stride 4, Field Interlaced: OK
		Crop 720x240@0x0, Compose 2x4@0x0, Stride 4, Field Alternating: OK
		Crop 720x240@0x0, Compose 2x4@0x0, Stride 4, Field Interlaced Top-Bottom: OK
		Crop 720x240@0x0, Compose 2x4@0x0, Stride 4, Field Interlaced Bottom-Top: OK
	test MMAP for Format RGBP, Frame Size 2048x2048:
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field None: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 4096, Field Top: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 4096, Field Bottom: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 4096, Field Interlaced: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 4096, Field Alternating: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 4096, Field Interlaced Top-Bottom: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 4096, Field Interlaced Bottom-Top: OK
	test MMAP for Format RGBP, Frame Size 720x480:
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 1440, Field None: OK
		Crop 720x240@0x0, Compose 720x480@0x0, Stride 1440, Field Top: OK
		Crop 720x240@0x0, Compose 720x480@0x0, Stride 1440, Field Bottom: OK
		Crop 720x240@0x0, Compose 720x480@0x0, Stride 1440, Field Interlaced: OK
		Crop 720x240@0x0, Compose 720x480@0x0, Stride 1440, Field Alternating: OK
		Crop 720x240@0x0, Compose 720x480@0x0, Stride 1440, Field Interlaced Top-Bottom: OK
		Crop 720x240@0x0, Compose 720x480@0x0, Stride 1440, Field Interlaced Bottom-Top: OK
	test MMAP for Format XR15, Frame Size 2x4:
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 4, Field None: OK
		Crop 720x240@0x0, Compose 2x4@0x0, Stride 4, Field Top: OK
		Crop 720x240@0x0, Compose 2x4@0x0, Stride 4, Field Bottom: OK
		Crop 720x240@0x0, Compose 2x4@0x0, Stride 4, Field Interlaced: OK
		Crop 720x240@0x0, Compose 2x4@0x0, Stride 4, Field Alternating: OK
		Crop 720x240@0x0, Compose 2x4@0x0, Stride 4, Field Interlaced Top-Bottom: OK
		Crop 720x240@0x0, Compose 2x4@0x0, Stride 4, Field Interlaced Bottom-Top: OK
	test MMAP for Format XR15, Frame Size 2048x2048:
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 4096, Field None: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 4096, Field Top: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 4096, Field Bottom: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 4096, Field Interlaced: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 4096, Field Alternating: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 4096, Field Interlaced Top-Bottom: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 4096, Field Interlaced Bottom-Top: OK
	test MMAP for Format XR15, Frame Size 720x480:
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 1440, Field None: OK
		Crop 720x240@0x0, Compose 720x480@0x0, Stride 1440, Field Top: OK
		Crop 720x240@0x0, Compose 720x480@0x0, Stride 1440, Field Bottom: OK
		Crop 720x240@0x0, Compose 720x480@0x0, Stride 1440, Field Interlaced: OK
		Crop 720x240@0x0, Compose 720x480@0x0, Stride 1440, Field Alternating: OK
		Crop 720x240@0x0, Compose 720x480@0x0, Stride 1440, Field Interlaced Top-Bottom: OK
		Crop 720x240@0x0, Compose 720x480@0x0, Stride 1440, Field Interlaced Bottom-Top: OK
	test MMAP for Format XR24, Frame Size 2x4:
		Crop 720x480@0x0, Compose 2x4@0x0, Stride 8, Field None: OK
		Crop 720x240@0x0, Compose 2x4@0x0, Stride 8, Field Top: OK
		Crop 720x240@0x0, Compose 2x4@0x0, Stride 8, Field Bottom: OK
		Crop 720x240@0x0, Compose 2x4@0x0, Stride 8, Field Interlaced: OK
		Crop 720x240@0x0, Compose 2x4@0x0, Stride 8, Field Alternating: OK
		Crop 720x240@0x0, Compose 2x4@0x0, Stride 8, Field Interlaced Top-Bottom: OK
		Crop 720x240@0x0, Compose 2x4@0x0, Stride 8, Field Interlaced Bottom-Top: OK
	test MMAP for Format XR24, Frame Size 2048x2048:
		Crop 720x480@0x0, Compose 2048x2048@0x0, Stride 8192, Field None: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 8192, Field Top: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 8192, Field Bottom: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 8192, Field Interlaced: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 8192, Field Alternating: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 8192, Field Interlaced Top-Bottom: OK
		Crop 720x240@0x0, Compose 2048x2048@0x0, Stride 8192, Field Interlaced Bottom-Top: OK
	test MMAP for Format XR24, Frame Size 720x480:
		Crop 720x480@0x0, Compose 720x480@0x0, Stride 2880, Field None: OK
		Crop 720x240@0x0, Compose 720x480@0x0, Stride 2880, Field Top: OK
		Crop 720x240@0x0, Compose 720x480@0x0, Stride 2880, Field Bottom: OK
		Crop 720x240@0x0, Compose 720x480@0x0, Stride 2880, Field Interlaced: OK
		Crop 720x240@0x0, Compose 720x480@0x0, Stride 2880, Field Alternating: OK
		Crop 720x240@0x0, Compose 720x480@0x0, Stride 2880, Field Interlaced Top-Bottom: OK
		Crop 720x240@0x0, Compose 720x480@0x0, Stride 2880, Field Interlaced Bottom-Top: OK

Total: 172, Succeeded: 172, Failed: 0, Warnings: 0

Niklas Söderlund (6):
  media: rcar-vin: make V4L2_FIELD_INTERLACED standard dependent
  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  |  34 ++++--
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 156 +++++++++++++++++-----------
 3 files changed, 136 insertions(+), 75 deletions(-)

-- 
2.9.0

[PATCHv2 1/7] media: rcar-vin: make V4L2_FIELD_INTERLACED standard dependent

[Niklas Söderlund]

The field V4L2_FIELD_INTERLACED is standard dependent and should not
unconditionally be equivalent to V4L2_FIELD_INTERLACED_TB.

This patch adds a check to see if the video standard can be obtained and
if it's a 60 Hz format. If the condition is meet V4L2_FIELD_INTERLACED
is treated as V4L2_FIELD_INTERLACED_BT if not as
V4L2_FIELD_INTERLACED_TB.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 496aa97..4063775 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -131,6 +131,7 @@ static u32 rvin_read(struct rvin_dev *vin, u32 offset)
 static int rvin_setup(struct rvin_dev *vin)
 {
 	u32 vnmc, dmr, dmr2, interrupts;
+	v4l2_std_id std;
 	bool progressive = false, output_is_yuv = false, input_is_yuv = false;
 
 	switch (vin->format.field) {
@@ -141,6 +142,13 @@ static int rvin_setup(struct rvin_dev *vin)
 		vnmc = VNMC_IM_EVEN;
 		break;
 	case V4L2_FIELD_INTERLACED:
+		/* Default to TB */
+		vnmc = VNMC_IM_FULL;
+		/* Use BT if video standard can be read and is 60 Hz format */
+		if (!v4l2_subdev_call(vin_to_source(vin), video, g_std, &std))
+			if (std & V4L2_STD_525_60)
+				vnmc = VNMC_IM_FULL | VNMC_FOC;
+		break;
 	case V4L2_FIELD_INTERLACED_TB:
 		vnmc = VNMC_IM_FULL;
 		break;
-- 
2.9.0

[PATCHv2 2/7] 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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 10a5c10..6d4086a 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;
 
@@ -144,6 +149,10 @@ static int __rvin_try_format(struct rvin_dev *vin,
 	rwidth = pix->width;
 	rheight = pix->height;
 
+	/* Keep current field if no specific one is asked for */
+	if (pix->field == V4L2_FIELD_ANY)
+		pix->field = vin->format.field;
+
 	/*
 	 * Retrieve format information and select the current format if the
 	 * requested format isn't supported.
-- 
2.9.0

[PATCHv2 3/7] 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 6d4086a..33435d7 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,
@@ -251,6 +299,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;
 }
 
@@ -442,35 +492,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)
@@ -776,10 +805,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)
@@ -842,31 +867,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

[PATCHv2 4/7] 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 33435d7..c31ac73 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:
@@ -221,21 +223,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:
@@ -245,6 +239,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

[PATCHv2 5/7] 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 V4L2_FIELD_ALTERNATE. This patch add support for
this mode.

At probe time and when S_STD is called the driver will default to use
V4L2_FIELD_INTERLACED if the subdevice reports V4L2_FIELD_ALTERNATE. The
driver will only change the field type if the subdevice implements
G_STD, if not it will keep the default at V4L2_FIELD_ALTERNATE.

The user can always explicitly ask for V4L2_FIELD_ALTERNATE in S_FTM and
the driver will use that field format.

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 | 13 +++++++++++++
 2 files changed, 33 insertions(+), 6 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 4063775..80139ad 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)
 
@@ -155,6 +156,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;
@@ -330,15 +332,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;
@@ -879,7 +892,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;
@@ -906,7 +919,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",
@@ -921,7 +935,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 c31ac73..125f87e 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -122,9 +122,21 @@ static int rvin_reset_format(struct rvin_dev *vin)
 	vin->format.colorspace	= mf->colorspace;
 	vin->format.field	= mf->field;
 
+	/*
+	 * If the subdevice uses ALTERNATE field mode and G_STD is
+	 * implemented use the VIN HW to combine the two fields to
+	 * one INTERLACED frame. The ALTERNATE field mode can still
+	 * be requested in S_FMT and be respected, this is just the
+	 * default which is applied at probing or when S_STD is called.
+	 */
+	if (vin->format.field == V4L2_FIELD_ALTERNATE &&
+	    v4l2_subdev_has_op(vin_to_source(vin), video, g_std))
+		vin->format.field = V4L2_FIELD_INTERLACED;
+
 	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:
@@ -226,6 +238,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

[PATCHv2 6/7] 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

[PATCHv2 7/7] [PATCHv5] 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>

---
v5:
- Readd patch version and changelog which was dropped in v4.

v4:
- Switch V4L2_FIELD_SEQ_TB/V4L2_FIELD_SEQ_BT to V4L2_FIELD_ALTERNATE.
- Update of Steves patch by Niklas.

v3: no changes

v2:
- the init of state->curr_norm in probe needs to be moved up, ahead
  of the init of state->field.
---
 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: [PATCHv2 7/7] [PATCHv5] media: adv7180: fix field type

[Lars-Peter Clausen]

[...]
> 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;

I'm not convinced this is correct. As far as I understand it when the I2P
feature is enabled the core outputs full progressive frames at the full
framerate. If it is bypassed it outputs half-frames. So we have the option
of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I
think this branch should setup the field format to be ALTERNATE regardless
of whether the I2P feature is available.

> +		else
> +			format->format.field = V4L2_FIELD_ALTERNATE;
>  		break;
>  	}
>  

Re: [PATCHv2 7/7] [PATCHv5] media: adv7180: fix field type

[Niklas Söderlund]

On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote:
> [...]
> > 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;
> 
> I'm not convinced this is correct. As far as I understand it when the I2P
> feature is enabled the core outputs full progressive frames at the full
> framerate. If it is bypassed it outputs half-frames. So we have the option
> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I
> think this branch should setup the field format to be ALTERNATE regardless
> of whether the I2P feature is available.

I be happy to update the patch in such manner, but I feel this is more 
for Steven to handle. I have no deep understanding of the adv7180 driver 
and the only HW I have is the adv7180 and not adv7280, adv7280_m, 
adv7282 or adv7282_m which is the models which have the ADV7180_FLAG_I2P 
flag. So I can't really test such a change.

Steven do you want to update this patch and resend it? I can drop it 
from this series altogether since the rcar-vin changes do not depend on 
it. I only kept it since I want the rcar-vin changes to be picked up 
before the adv7180 changes, otherwise the rcar-vin driver will stop to 
function on Koelsch. I can also ofc make the change suggested by Lars if 
you prefer, but then I want your blessing to do so. I have already 
changed so much of your original patch :-)

> 
> > +		else
> > +			format->format.field = V4L2_FIELD_ALTERNATE;
> >  		break;
> >  	}
> >  
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCHv2 7/7] [PATCHv5] media: adv7180: fix field type

[Niklas Söderlund]

On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote:
> [...]
> > 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;
> 
> I'm not convinced this is correct. As far as I understand it when the I2P
> feature is enabled the core outputs full progressive frames at the full
> framerate. If it is bypassed it outputs half-frames. So we have the option
> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I
> think this branch should setup the field format to be ALTERNATE regardless
> of whether the I2P feature is available.

I be happy to update the patch in such manner, but I feel this is more 
for Steven to handle. I have no deep understanding of the adv7180 driver 
and the only HW I have is the adv7180 and not adv7280, adv7280_m, 
adv7282 or adv7282_m which is the models which have the ADV7180_FLAG_I2P 
flag. So I can't really test such a change.

Steven do you want to update this patch and resend it? I can drop it 
from this series altogether since the rcar-vin changes do not depend on 
it. I only kept it since I want the rcar-vin changes to be picked up 
before the adv7180 changes, otherwise the rcar-vin driver will stop to 
function on Koelsch. I can also ofc make the change suggested by Lars if 
you prefer, but then I want your blessing to do so. I have already 
changed so much of your original patch :-)

> 
> > +		else
> > +			format->format.field = V4L2_FIELD_ALTERNATE;
> >  		break;
> >  	}
> >  
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCHv2 5/7] media: rcar-vin: add support for V4L2_FIELD_ALTERNATE

[Sergei Shtylyov]

Hello.

On 08/02/2016 05:51 PM, Niklas Söderlund wrote:

> The HW can capture both ODD and EVEN fields in separate buffers so it's
> possible to support V4L2_FIELD_ALTERNATE. This patch add support for
> this mode.
>
> At probe time and when S_STD is called the driver will default to use
> V4L2_FIELD_INTERLACED if the subdevice reports V4L2_FIELD_ALTERNATE. The
> driver will only change the field type if the subdevice implements
> G_STD, if not it will keep the default at V4L2_FIELD_ALTERNATE.
>
> The user can always explicitly ask for V4L2_FIELD_ALTERNATE in S_FTM and

    S_FMT?

> the driver will use that field format.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
[...]

MBR, Sergei

Re: [PATCHv2 5/7] media: rcar-vin: add support for V4L2_FIELD_ALTERNATE

[Niklas Söderlund]

On 2016-08-03 16:22:22 +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 08/02/2016 05:51 PM, Niklas Söderlund wrote:
> 
> > The HW can capture both ODD and EVEN fields in separate buffers so it's
> > possible to support V4L2_FIELD_ALTERNATE. This patch add support for
> > this mode.
> > 
> > At probe time and when S_STD is called the driver will default to use
> > V4L2_FIELD_INTERLACED if the subdevice reports V4L2_FIELD_ALTERNATE. The
> > driver will only change the field type if the subdevice implements
> > G_STD, if not it will keep the default at V4L2_FIELD_ALTERNATE.
> > 
> > The user can always explicitly ask for V4L2_FIELD_ALTERNATE in S_FTM and
> 
>    S_FMT?

yes :-)

> 
> > the driver will use that field format.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> [...]
> 
> MBR, Sergei
> 

-- 
Regards,
Niklas Söderlund

Re: [PATCHv2 5/7] media: rcar-vin: add support for V4L2_FIELD_ALTERNATE

[Niklas Söderlund]

On 2016-08-03 16:22:22 +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 08/02/2016 05:51 PM, Niklas S�derlund wrote:
> 
> > The HW can capture both ODD and EVEN fields in separate buffers so it's
> > possible to support V4L2_FIELD_ALTERNATE. This patch add support for
> > this mode.
> > 
> > At probe time and when S_STD is called the driver will default to use
> > V4L2_FIELD_INTERLACED if the subdevice reports V4L2_FIELD_ALTERNATE. The
> > driver will only change the field type if the subdevice implements
> > G_STD, if not it will keep the default at V4L2_FIELD_ALTERNATE.
> > 
> > The user can always explicitly ask for V4L2_FIELD_ALTERNATE in S_FTM and
> 
>    S_FMT?

yes :-)

> 
> > the driver will use that field format.
> > 
> > Signed-off-by: Niklas S�derlund <niklas.soderlund+renesas@ragnatech.se>
> [...]
> 
> MBR, Sergei
> 

-- 
Regards,
Niklas S�derlund

Re: [PATCHv2 1/7] media: rcar-vin: make V4L2_FIELD_INTERLACED standard dependent

[Sergei Shtylyov]

On 08/02/2016 05:51 PM, Niklas Söderlund wrote:

> The field V4L2_FIELD_INTERLACED is standard dependent and should not
> unconditionally be equivalent to V4L2_FIELD_INTERLACED_TB.
>
> This patch adds a check to see if the video standard can be obtained and
> if it's a 60 Hz format. If the condition is meet V4L2_FIELD_INTERLACED

    s/meet/met/.

> is treated as V4L2_FIELD_INTERLACED_BT if not as
> V4L2_FIELD_INTERLACED_TB.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 496aa97..4063775 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -131,6 +131,7 @@ static u32 rvin_read(struct rvin_dev *vin, u32 offset)
>  static int rvin_setup(struct rvin_dev *vin)
>  {
>  	u32 vnmc, dmr, dmr2, interrupts;
> +	v4l2_std_id std;
>  	bool progressive = false, output_is_yuv = false, input_is_yuv = false;
>
>  	switch (vin->format.field) {
> @@ -141,6 +142,13 @@ static int rvin_setup(struct rvin_dev *vin)
>  		vnmc = VNMC_IM_EVEN;
>  		break;
>  	case V4L2_FIELD_INTERLACED:
> +		/* Default to TB */
> +		vnmc = VNMC_IM_FULL;
> +		/* Use BT if video standard can be read and is 60 Hz format */
> +		if (!v4l2_subdev_call(vin_to_source(vin), video, g_std, &std))
> +			if (std & V4L2_STD_525_60)
> +				vnmc = VNMC_IM_FULL | VNMC_FOC;

    I think you either need to fold 2 *if* statements, or add {} in the 1st one.

[...]

MBR, Sergei

Re: [PATCHv2 7/7] [PATCHv5] media: adv7180: fix field type

[Hans Verkuil]

On 08/03/2016 03:21 PM, Niklas Söderlund wrote:
> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote:
>> [...]
>>> 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;
>>
>> I'm not convinced this is correct. As far as I understand it when the I2P
>> feature is enabled the core outputs full progressive frames at the full
>> framerate. If it is bypassed it outputs half-frames. So we have the option
>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I
>> think this branch should setup the field format to be ALTERNATE regardless
>> of whether the I2P feature is available.

Actually, that's not true. If the progressive frame is obtained by combining
two fields, then it should return FIELD_INTERLACED. This is how most SDTV
receivers operate.

FIELD_NONE is reserved for pure progressive formats like 720p.

Regards,

	Hans

> 
> I be happy to update the patch in such manner, but I feel this is more 
> for Steven to handle. I have no deep understanding of the adv7180 driver 
> and the only HW I have is the adv7180 and not adv7280, adv7280_m, 
> adv7282 or adv7282_m which is the models which have the ADV7180_FLAG_I2P 
> flag. So I can't really test such a change.
> 
> Steven do you want to update this patch and resend it? I can drop it 
> from this series altogether since the rcar-vin changes do not depend on 
> it. I only kept it since I want the rcar-vin changes to be picked up 
> before the adv7180 changes, otherwise the rcar-vin driver will stop to 
> function on Koelsch. I can also ofc make the change suggested by Lars if 
> you prefer, but then I want your blessing to do so. I have already 
> changed so much of your original patch :-)
> 
>>
>>> +		else
>>> +			format->format.field = V4L2_FIELD_ALTERNATE;
>>>  		break;
>>>  	}
>>>  
>>
> 

Re: [PATCHv2 7/7] [PATCHv5] media: adv7180: fix field type

[Hans Verkuil]

On 08/03/2016 03:21 PM, Niklas S�derlund wrote:
> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote:
>> [...]
>>> 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;
>>
>> I'm not convinced this is correct. As far as I understand it when the I2P
>> feature is enabled the core outputs full progressive frames at the full
>> framerate. If it is bypassed it outputs half-frames. So we have the option
>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I
>> think this branch should setup the field format to be ALTERNATE regardless
>> of whether the I2P feature is available.

Actually, that's not true. If the progressive frame is obtained by combining
two fields, then it should return FIELD_INTERLACED. This is how most SDTV
receivers operate.

FIELD_NONE is reserved for pure progressive formats like 720p.

Regards,

	Hans

> 
> I be happy to update the patch in such manner, but I feel this is more 
> for Steven to handle. I have no deep understanding of the adv7180 driver 
> and the only HW I have is the adv7180 and not adv7280, adv7280_m, 
> adv7282 or adv7282_m which is the models which have the ADV7180_FLAG_I2P 
> flag. So I can't really test such a change.
> 
> Steven do you want to update this patch and resend it? I can drop it 
> from this series altogether since the rcar-vin changes do not depend on 
> it. I only kept it since I want the rcar-vin changes to be picked up 
> before the adv7180 changes, otherwise the rcar-vin driver will stop to 
> function on Koelsch. I can also ofc make the change suggested by Lars if 
> you prefer, but then I want your blessing to do so. I have already 
> changed so much of your original patch :-)
> 
>>
>>> +		else
>>> +			format->format.field = V4L2_FIELD_ALTERNATE;
>>>  		break;
>>>  	}
>>>  
>>
> 

Re: [PATCHv2 7/7] [PATCHv5] media: adv7180: fix field type

[Lars-Peter Clausen]

On 08/03/2016 04:11 PM, Hans Verkuil wrote:
> 
> 
> On 08/03/2016 03:21 PM, Niklas Söderlund wrote:
>> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote:
>>> [...]
>>>> 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;
>>>
>>> I'm not convinced this is correct. As far as I understand it when the I2P
>>> feature is enabled the core outputs full progressive frames at the full
>>> framerate. If it is bypassed it outputs half-frames. So we have the option
>>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I
>>> think this branch should setup the field format to be ALTERNATE regardless
>>> of whether the I2P feature is available.
> 
> Actually, that's not true. If the progressive frame is obtained by combining
> two fields, then it should return FIELD_INTERLACED. This is how most SDTV
> receivers operate.

This is definitely not covered by the current definition of INTERLACED. It
says that the temporal order of the odd and even lines is the same for each
frame. Whereas for a deinterlaced frame the temporal order changes from
frame to frame.

E.g. lets say you have half frames A, B, C, D, E, F ...

The output of the I2P core are frames like (A,B) (C,B) (C,D) (E,D) (E, F) ...

The first frame is INTERLACED_TB, the second INTERLACED_BT, the third
INTERLACED_TB again and so on. Also you get the same amount of pixels as for
a progressive setup so the data-output-rate is higher. Maybe we need a
FIELD_DEINTERLACED to denote such a setup?

Re: [PATCHv2 7/7] [PATCHv5] media: adv7180: fix field type

[Lars-Peter Clausen]

On 08/03/2016 04:11 PM, Hans Verkuil wrote:
> 
> 
> On 08/03/2016 03:21 PM, Niklas S�derlund wrote:
>> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote:
>>> [...]
>>>> 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;
>>>
>>> I'm not convinced this is correct. As far as I understand it when the I2P
>>> feature is enabled the core outputs full progressive frames at the full
>>> framerate. If it is bypassed it outputs half-frames. So we have the option
>>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I
>>> think this branch should setup the field format to be ALTERNATE regardless
>>> of whether the I2P feature is available.
> 
> Actually, that's not true. If the progressive frame is obtained by combining
> two fields, then it should return FIELD_INTERLACED. This is how most SDTV
> receivers operate.

This is definitely not covered by the current definition of INTERLACED. It
says that the temporal order of the odd and even lines is the same for each
frame. Whereas for a deinterlaced frame the temporal order changes from
frame to frame.

E.g. lets say you have half frames A, B, C, D, E, F ...

The output of the I2P core are frames like (A,B) (C,B) (C,D) (E,D) (E, F) ...

The first frame is INTERLACED_TB, the second INTERLACED_BT, the third
INTERLACED_TB again and so on. Also you get the same amount of pixels as for
a progressive setup so the data-output-rate is higher. Maybe we need a
FIELD_DEINTERLACED to denote such a setup?

Re: [PATCHv2 7/7] [PATCHv5] media: adv7180: fix field type

[Lars-Peter Clausen]

On 08/03/2016 04:23 PM, Lars-Peter Clausen wrote:
> On 08/03/2016 04:11 PM, Hans Verkuil wrote:
>>
>>
>> On 08/03/2016 03:21 PM, Niklas Söderlund wrote:
>>> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote:
>>>> [...]
>>>>> 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;
>>>>
>>>> I'm not convinced this is correct. As far as I understand it when the I2P
>>>> feature is enabled the core outputs full progressive frames at the full
>>>> framerate. If it is bypassed it outputs half-frames. So we have the option
>>>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I
>>>> think this branch should setup the field format to be ALTERNATE regardless
>>>> of whether the I2P feature is available.
>>
>> Actually, that's not true. If the progressive frame is obtained by combining
>> two fields, then it should return FIELD_INTERLACED. This is how most SDTV
>> receivers operate.
> 
> This is definitely not covered by the current definition of INTERLACED. It
> says that the temporal order of the odd and even lines is the same for each
> frame. Whereas for a deinterlaced frame the temporal order changes from
> frame to frame.
> 
> E.g. lets say you have half frames A, B, C, D, E, F ...
> 
> The output of the I2P core are frames like (A,B) (C,B) (C,D) (E,D) (E, F) ...

Just for completeness the output expected for INTERLACED would be

(A, B), (C, D), (E, F), ...

> 
> The first frame is INTERLACED_TB, the second INTERLACED_BT, the third
> INTERLACED_TB again and so on. Also you get the same amount of pixels as for
> a progressive setup so the data-output-rate is higher. Maybe we need a
> FIELD_DEINTERLACED to denote such a setup?
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCHv2 7/7] [PATCHv5] media: adv7180: fix field type

[Lars-Peter Clausen]

On 08/03/2016 04:23 PM, Lars-Peter Clausen wrote:
> On 08/03/2016 04:11 PM, Hans Verkuil wrote:
>>
>>
>> On 08/03/2016 03:21 PM, Niklas S�derlund wrote:
>>> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote:
>>>> [...]
>>>>> 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;
>>>>
>>>> I'm not convinced this is correct. As far as I understand it when the I2P
>>>> feature is enabled the core outputs full progressive frames at the full
>>>> framerate. If it is bypassed it outputs half-frames. So we have the option
>>>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I
>>>> think this branch should setup the field format to be ALTERNATE regardless
>>>> of whether the I2P feature is available.
>>
>> Actually, that's not true. If the progressive frame is obtained by combining
>> two fields, then it should return FIELD_INTERLACED. This is how most SDTV
>> receivers operate.
> 
> This is definitely not covered by the current definition of INTERLACED. It
> says that the temporal order of the odd and even lines is the same for each
> frame. Whereas for a deinterlaced frame the temporal order changes from
> frame to frame.
> 
> E.g. lets say you have half frames A, B, C, D, E, F ...
> 
> The output of the I2P core are frames like (A,B) (C,B) (C,D) (E,D) (E, F) ...

Just for completeness the output expected for INTERLACED would be

(A, B), (C, D), (E, F), ...

> 
> The first frame is INTERLACED_TB, the second INTERLACED_BT, the third
> INTERLACED_TB again and so on. Also you get the same amount of pixels as for
> a progressive setup so the data-output-rate is higher. Maybe we need a
> FIELD_DEINTERLACED to denote such a setup?
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCHv2 7/7] [PATCHv5] media: adv7180: fix field type

[Hans Verkuil]

On 08/03/2016 04:23 PM, Lars-Peter Clausen wrote:
> On 08/03/2016 04:11 PM, Hans Verkuil wrote:
>>
>>
>> On 08/03/2016 03:21 PM, Niklas Söderlund wrote:
>>> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote:
>>>> [...]
>>>>> 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;
>>>>
>>>> I'm not convinced this is correct. As far as I understand it when the I2P
>>>> feature is enabled the core outputs full progressive frames at the full
>>>> framerate. If it is bypassed it outputs half-frames. So we have the option
>>>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I
>>>> think this branch should setup the field format to be ALTERNATE regardless
>>>> of whether the I2P feature is available.
>>
>> Actually, that's not true. If the progressive frame is obtained by combining
>> two fields, then it should return FIELD_INTERLACED. This is how most SDTV
>> receivers operate.
> 
> This is definitely not covered by the current definition of INTERLACED. It
> says that the temporal order of the odd and even lines is the same for each
> frame. Whereas for a deinterlaced frame the temporal order changes from
> frame to frame.
> 
> E.g. lets say you have half frames A, B, C, D, E, F ...
> 
> The output of the I2P core are frames like (A,B) (C,B) (C,D) (E,D) (E, F) ...

Yuck.

What most devices do is (A,B) (C,D) (E,F) ...

That's FIELD_INTERLACED.

> 
> The first frame is INTERLACED_TB, the second INTERLACED_BT, the third
> INTERLACED_TB again and so on. Also you get the same amount of pixels as for
> a progressive setup so the data-output-rate is higher. Maybe we need a
> FIELD_DEINTERLACED to denote such a setup?
> 

Yeah, this is a completely different mode. Do we even want to support this?

Does anyone need this mode? I think we should leave it out until someone actually
wants to use it. And then we need to come up with a new FIELD_ mode.

Regards,

	Hans

Re: [PATCHv2 7/7] [PATCHv5] media: adv7180: fix field type

[Hans Verkuil]

On 08/03/2016 04:23 PM, Lars-Peter Clausen wrote:
> On 08/03/2016 04:11 PM, Hans Verkuil wrote:
>>
>>
>> On 08/03/2016 03:21 PM, Niklas S�derlund wrote:
>>> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote:
>>>> [...]
>>>>> 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;
>>>>
>>>> I'm not convinced this is correct. As far as I understand it when the I2P
>>>> feature is enabled the core outputs full progressive frames at the full
>>>> framerate. If it is bypassed it outputs half-frames. So we have the option
>>>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I
>>>> think this branch should setup the field format to be ALTERNATE regardless
>>>> of whether the I2P feature is available.
>>
>> Actually, that's not true. If the progressive frame is obtained by combining
>> two fields, then it should return FIELD_INTERLACED. This is how most SDTV
>> receivers operate.
> 
> This is definitely not covered by the current definition of INTERLACED. It
> says that the temporal order of the odd and even lines is the same for each
> frame. Whereas for a deinterlaced frame the temporal order changes from
> frame to frame.
> 
> E.g. lets say you have half frames A, B, C, D, E, F ...
> 
> The output of the I2P core are frames like (A,B) (C,B) (C,D) (E,D) (E, F) ...

Yuck.

What most devices do is (A,B) (C,D) (E,F) ...

That's FIELD_INTERLACED.

> 
> The first frame is INTERLACED_TB, the second INTERLACED_BT, the third
> INTERLACED_TB again and so on. Also you get the same amount of pixels as for
> a progressive setup so the data-output-rate is higher. Maybe we need a
> FIELD_DEINTERLACED to denote such a setup?
> 

Yeah, this is a completely different mode. Do we even want to support this?

Does anyone need this mode? I think we should leave it out until someone actually
wants to use it. And then we need to come up with a new FIELD_ mode.

Regards,

	Hans

Re: [PATCHv2 7/7] [PATCHv5] media: adv7180: fix field type

[Ian Arkver]

On 03/08/16 15:23, Lars-Peter Clausen wrote:
> On 08/03/2016 04:11 PM, Hans Verkuil wrote:
>>
>> On 08/03/2016 03:21 PM, Niklas Söderlund wrote:
>>> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote:
>>>> [...]
>>>>> 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;
>>>> I'm not convinced this is correct. As far as I understand it when the I2P
>>>> feature is enabled the core outputs full progressive frames at the full
>>>> framerate. If it is bypassed it outputs half-frames. So we have the option
>>>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I
>>>> think this branch should setup the field format to be ALTERNATE regardless
>>>> of whether the I2P feature is available.
>> Actually, that's not true. If the progressive frame is obtained by combining
>> two fields, then it should return FIELD_INTERLACED. This is how most SDTV
>> receivers operate.
> This is definitely not covered by the current definition of INTERLACED. It
> says that the temporal order of the odd and even lines is the same for each
> frame. Whereas for a deinterlaced frame the temporal order changes from
> frame to frame.
>
> E.g. lets say you have half frames A, B, C, D, E, F ...
>
> The output of the I2P core are frames like (A,B) (C,B) (C,D) (E,D) (E, F) ...
>
> The first frame is INTERLACED_TB, the second INTERLACED_BT, the third
> INTERLACED_TB again and so on. Also you get the same amount of pixels as for
> a progressive setup so the data-output-rate is higher. Maybe we need a
> FIELD_DEINTERLACED to denote such a setup?
I don't think this is correct. The ADV7280 has no framestore, just a 
small linebuffer. It does I2P by line doubling plus some filtering and a 
little bit of proprietary magic, allegedly.

I believe the output in I2P mode for your example would be (AA) (BB) 
(CC). The clock rate and pixel rate is doubled since it sends a full 
(faked up) frame per field time.

I don't know what the FIELD_* mode is for line doubled pseudo-progressive.

Also, I don't know why anyone would use this mode. I don't see a 
scenario where it would actually improve video quality over a more 
sophisticated motion adaptive deinterlace and to restore a 25/30fps feed 
you'd need to decimate and lose information.

Quote from "Rob.Analog", who uses the word "frame" freely, here: 
https://ez.analog.com/thread/39382

"2) In I2P mode the number of lines per frame doubles. The ADV7280 still 
outputs 50 frames per second ( or 60 frames in NTSC mode) but each frame 
now consists of twice as many lines. e.g. if a frame consisted of 288 
lines of active video in interlaced mode, this is doubled to 576 lines 
of active video in progressive mode. The line doubling is achieved by 
the ADV7280 interpolating between two lines of video (e.g. between lines 
1 and 3 on an odd frame) and inserting an extra line (e.g. line 2). 
There are also some ADI propriety algorithms that prevent low angle 
noise artifacts.

In order to achieve this line doubling, the LLC clock doubles from a 
nominal 27MHz to a nominal 54MHz"


Regards,
IanJ

>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 7/7] [PATCHv5] media: adv7180: fix field type

[Ian Arkver]

On 03/08/16 15:23, Lars-Peter Clausen wrote:
> On 08/03/2016 04:11 PM, Hans Verkuil wrote:
>>
>> On 08/03/2016 03:21 PM, Niklas S�derlund wrote:
>>> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote:
>>>> [...]
>>>>> 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;
>>>> I'm not convinced this is correct. As far as I understand it when the I2P
>>>> feature is enabled the core outputs full progressive frames at the full
>>>> framerate. If it is bypassed it outputs half-frames. So we have the option
>>>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I
>>>> think this branch should setup the field format to be ALTERNATE regardless
>>>> of whether the I2P feature is available.
>> Actually, that's not true. If the progressive frame is obtained by combining
>> two fields, then it should return FIELD_INTERLACED. This is how most SDTV
>> receivers operate.
> This is definitely not covered by the current definition of INTERLACED. It
> says that the temporal order of the odd and even lines is the same for each
> frame. Whereas for a deinterlaced frame the temporal order changes from
> frame to frame.
>
> E.g. lets say you have half frames A, B, C, D, E, F ...
>
> The output of the I2P core are frames like (A,B) (C,B) (C,D) (E,D) (E, F) ...
>
> The first frame is INTERLACED_TB, the second INTERLACED_BT, the third
> INTERLACED_TB again and so on. Also you get the same amount of pixels as for
> a progressive setup so the data-output-rate is higher. Maybe we need a
> FIELD_DEINTERLACED to denote such a setup?
I don't think this is correct. The ADV7280 has no framestore, just a 
small linebuffer. It does I2P by line doubling plus some filtering and a 
little bit of proprietary magic, allegedly.

I believe the output in I2P mode for your example would be (AA) (BB) 
(CC). The clock rate and pixel rate is doubled since it sends a full 
(faked up) frame per field time.

I don't know what the FIELD_* mode is for line doubled pseudo-progressive.

Also, I don't know why anyone would use this mode. I don't see a 
scenario where it would actually improve video quality over a more 
sophisticated motion adaptive deinterlace and to restore a 25/30fps feed 
you'd need to decimate and lose information.

Quote from "Rob.Analog", who uses the word "frame" freely, here: 
https://ez.analog.com/thread/39382

"2) In I2P mode the number of lines per frame doubles. The ADV7280 still 
outputs 50 frames per second ( or 60 frames in NTSC mode) but each frame 
now consists of twice as many lines. e.g. if a frame consisted of 288 
lines of active video in interlaced mode, this is doubled to 576 lines 
of active video in progressive mode. The line doubling is achieved by 
the ADV7280 interpolating between two lines of video (e.g. between lines 
1 and 3 on an odd frame) and inserting an extra line (e.g. line 2). 
There are also some ADI propriety algorithms that prevent low angle 
noise artifacts.

In order to achieve this line doubling, the LLC clock doubles from a 
nominal 27MHz to a nominal 54MHz"


Regards,
IanJ

>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 7/7] [PATCHv5] media: adv7180: fix field type

[Hans Verkuil]

On 08/03/2016 04:42 PM, Ian Arkver wrote:
> On 03/08/16 15:23, Lars-Peter Clausen wrote:
>> On 08/03/2016 04:11 PM, Hans Verkuil wrote:
>>>
>>> On 08/03/2016 03:21 PM, Niklas Söderlund wrote:
>>>> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote:
>>>>> [...]
>>>>>> 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;
>>>>> I'm not convinced this is correct. As far as I understand it when the I2P
>>>>> feature is enabled the core outputs full progressive frames at the full
>>>>> framerate. If it is bypassed it outputs half-frames. So we have the option
>>>>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I
>>>>> think this branch should setup the field format to be ALTERNATE regardless
>>>>> of whether the I2P feature is available.
>>> Actually, that's not true. If the progressive frame is obtained by combining
>>> two fields, then it should return FIELD_INTERLACED. This is how most SDTV
>>> receivers operate.
>> This is definitely not covered by the current definition of INTERLACED. It
>> says that the temporal order of the odd and even lines is the same for each
>> frame. Whereas for a deinterlaced frame the temporal order changes from
>> frame to frame.
>>
>> E.g. lets say you have half frames A, B, C, D, E, F ...
>>
>> The output of the I2P core are frames like (A,B) (C,B) (C,D) (E,D) (E, F) ...
>>
>> The first frame is INTERLACED_TB, the second INTERLACED_BT, the third
>> INTERLACED_TB again and so on. Also you get the same amount of pixels as for
>> a progressive setup so the data-output-rate is higher. Maybe we need a
>> FIELD_DEINTERLACED to denote such a setup?
> I don't think this is correct. The ADV7280 has no framestore, just a 
> small linebuffer. It does I2P by line doubling plus some filtering and a 
> little bit of proprietary magic, allegedly.
> 
> I believe the output in I2P mode for your example would be (AA) (BB) 
> (CC). The clock rate and pixel rate is doubled since it sends a full 
> (faked up) frame per field time.
> 
> I don't know what the FIELD_* mode is for line doubled pseudo-progressive.

We don't have one for that either. You really shouldn't do tricks like that,
it's rather pointless and gives horrible quality.

> 
> Also, I don't know why anyone would use this mode. I don't see a 
> scenario where it would actually improve video quality over a more 
> sophisticated motion adaptive deinterlace and to restore a 25/30fps feed 
> you'd need to decimate and lose information.

Indeed.

> 
> Quote from "Rob.Analog", who uses the word "frame" freely, here: 
> https://ez.analog.com/thread/39382
> 
> "2) In I2P mode the number of lines per frame doubles. The ADV7280 still 
> outputs 50 frames per second ( or 60 frames in NTSC mode) but each frame 
> now consists of twice as many lines. e.g. if a frame consisted of 288 
> lines of active video in interlaced mode, this is doubled to 576 lines 
> of active video in progressive mode. The line doubling is achieved by 
> the ADV7280 interpolating between two lines of video (e.g. between lines 
> 1 and 3 on an odd frame) and inserting an extra line (e.g. line 2). 
> There are also some ADI propriety algorithms that prevent low angle 
> noise artifacts.
> 
> In order to achieve this line doubling, the LLC clock doubles from a 
> nominal 27MHz to a nominal 54MHz"

It looks like you are correct. I wouldn't bother implementing this.

Regards,

	Hans

Re: [PATCHv2 7/7] [PATCHv5] media: adv7180: fix field type

[Hans Verkuil]

On 08/03/2016 04:42 PM, Ian Arkver wrote:
> On 03/08/16 15:23, Lars-Peter Clausen wrote:
>> On 08/03/2016 04:11 PM, Hans Verkuil wrote:
>>>
>>> On 08/03/2016 03:21 PM, Niklas S�derlund wrote:
>>>> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote:
>>>>> [...]
>>>>>> 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;
>>>>> I'm not convinced this is correct. As far as I understand it when the I2P
>>>>> feature is enabled the core outputs full progressive frames at the full
>>>>> framerate. If it is bypassed it outputs half-frames. So we have the option
>>>>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I
>>>>> think this branch should setup the field format to be ALTERNATE regardless
>>>>> of whether the I2P feature is available.
>>> Actually, that's not true. If the progressive frame is obtained by combining
>>> two fields, then it should return FIELD_INTERLACED. This is how most SDTV
>>> receivers operate.
>> This is definitely not covered by the current definition of INTERLACED. It
>> says that the temporal order of the odd and even lines is the same for each
>> frame. Whereas for a deinterlaced frame the temporal order changes from
>> frame to frame.
>>
>> E.g. lets say you have half frames A, B, C, D, E, F ...
>>
>> The output of the I2P core are frames like (A,B) (C,B) (C,D) (E,D) (E, F) ...
>>
>> The first frame is INTERLACED_TB, the second INTERLACED_BT, the third
>> INTERLACED_TB again and so on. Also you get the same amount of pixels as for
>> a progressive setup so the data-output-rate is higher. Maybe we need a
>> FIELD_DEINTERLACED to denote such a setup?
> I don't think this is correct. The ADV7280 has no framestore, just a 
> small linebuffer. It does I2P by line doubling plus some filtering and a 
> little bit of proprietary magic, allegedly.
> 
> I believe the output in I2P mode for your example would be (AA) (BB) 
> (CC). The clock rate and pixel rate is doubled since it sends a full 
> (faked up) frame per field time.
> 
> I don't know what the FIELD_* mode is for line doubled pseudo-progressive.

We don't have one for that either. You really shouldn't do tricks like that,
it's rather pointless and gives horrible quality.

> 
> Also, I don't know why anyone would use this mode. I don't see a 
> scenario where it would actually improve video quality over a more 
> sophisticated motion adaptive deinterlace and to restore a 25/30fps feed 
> you'd need to decimate and lose information.

Indeed.

> 
> Quote from "Rob.Analog", who uses the word "frame" freely, here: 
> https://ez.analog.com/thread/39382
> 
> "2) In I2P mode the number of lines per frame doubles. The ADV7280 still 
> outputs 50 frames per second ( or 60 frames in NTSC mode) but each frame 
> now consists of twice as many lines. e.g. if a frame consisted of 288 
> lines of active video in interlaced mode, this is doubled to 576 lines 
> of active video in progressive mode. The line doubling is achieved by 
> the ADV7280 interpolating between two lines of video (e.g. between lines 
> 1 and 3 on an odd frame) and inserting an extra line (e.g. line 2). 
> There are also some ADI propriety algorithms that prevent low angle 
> noise artifacts.
> 
> In order to achieve this line doubling, the LLC clock doubles from a 
> nominal 27MHz to a nominal 54MHz"

It looks like you are correct. I wouldn't bother implementing this.

Regards,

	Hans

Re: [PATCHv2 7/7] [PATCHv5] media: adv7180: fix field type

[Lars-Peter Clausen]

On 08/03/2016 04:42 PM, Ian Arkver wrote:
> On 03/08/16 15:23, Lars-Peter Clausen wrote:
>> On 08/03/2016 04:11 PM, Hans Verkuil wrote:
>>>
>>> On 08/03/2016 03:21 PM, Niklas Söderlund wrote:
>>>> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote:
>>>>> [...]
>>>>>> 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;
>>>>> I'm not convinced this is correct. As far as I understand it when the I2P
>>>>> feature is enabled the core outputs full progressive frames at the full
>>>>> framerate. If it is bypassed it outputs half-frames. So we have the option
>>>>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I
>>>>> think this branch should setup the field format to be ALTERNATE regardless
>>>>> of whether the I2P feature is available.
>>> Actually, that's not true. If the progressive frame is obtained by combining
>>> two fields, then it should return FIELD_INTERLACED. This is how most SDTV
>>> receivers operate.
>> This is definitely not covered by the current definition of INTERLACED. It
>> says that the temporal order of the odd and even lines is the same for each
>> frame. Whereas for a deinterlaced frame the temporal order changes from
>> frame to frame.
>>
>> E.g. lets say you have half frames A, B, C, D, E, F ...
>>
>> The output of the I2P core are frames like (A,B) (C,B) (C,D) (E,D) (E, F) ...
>>
>> The first frame is INTERLACED_TB, the second INTERLACED_BT, the third
>> INTERLACED_TB again and so on. Also you get the same amount of pixels as for
>> a progressive setup so the data-output-rate is higher. Maybe we need a
>> FIELD_DEINTERLACED to denote such a setup?
> I don't think this is correct. The ADV7280 has no framestore, just a small
> linebuffer. It does I2P by line doubling plus some filtering and a little
> bit of proprietary magic, allegedly.
> 
> I believe the output in I2P mode for your example would be (AA) (BB) (CC).
> The clock rate and pixel rate is doubled since it sends a full (faked up)
> frame per field time.
> 
> I don't know what the FIELD_* mode is for line doubled pseudo-progressive.
> 
> Also, I don't know why anyone would use this mode. I don't see a scenario
> where it would actually improve video quality over a more sophisticated
> motion adaptive deinterlace and to restore a 25/30fps feed you'd need to
> decimate and lose information.
> 
> Quote from "Rob.Analog", who uses the word "frame" freely, here:
> https://ez.analog.com/thread/39382
> 
> "2) In I2P mode the number of lines per frame doubles. The ADV7280 still
> outputs 50 frames per second ( or 60 frames in NTSC mode) but each frame now
> consists of twice as many lines. e.g. if a frame consisted of 288 lines of
> active video in interlaced mode, this is doubled to 576 lines of active
> video in progressive mode. The line doubling is achieved by the ADV7280
> interpolating between two lines of video (e.g. between lines 1 and 3 on an
> odd frame) and inserting an extra line (e.g. line 2). There are also some
> ADI propriety algorithms that prevent low angle noise artifacts.
> 
> In order to achieve this line doubling, the LLC clock doubles from a nominal
> 27MHz to a nominal 54MHz"

Hm, so it is basically just actually a scaling feature rather than a
deinterlacer. I guess we should expose it as that then.

Re: [PATCHv2 7/7] [PATCHv5] media: adv7180: fix field type

[Lars-Peter Clausen]

On 08/03/2016 04:42 PM, Ian Arkver wrote:
> On 03/08/16 15:23, Lars-Peter Clausen wrote:
>> On 08/03/2016 04:11 PM, Hans Verkuil wrote:
>>>
>>> On 08/03/2016 03:21 PM, Niklas S�derlund wrote:
>>>> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote:
>>>>> [...]
>>>>>> 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;
>>>>> I'm not convinced this is correct. As far as I understand it when the I2P
>>>>> feature is enabled the core outputs full progressive frames at the full
>>>>> framerate. If it is bypassed it outputs half-frames. So we have the option
>>>>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I
>>>>> think this branch should setup the field format to be ALTERNATE regardless
>>>>> of whether the I2P feature is available.
>>> Actually, that's not true. If the progressive frame is obtained by combining
>>> two fields, then it should return FIELD_INTERLACED. This is how most SDTV
>>> receivers operate.
>> This is definitely not covered by the current definition of INTERLACED. It
>> says that the temporal order of the odd and even lines is the same for each
>> frame. Whereas for a deinterlaced frame the temporal order changes from
>> frame to frame.
>>
>> E.g. lets say you have half frames A, B, C, D, E, F ...
>>
>> The output of the I2P core are frames like (A,B) (C,B) (C,D) (E,D) (E, F) ...
>>
>> The first frame is INTERLACED_TB, the second INTERLACED_BT, the third
>> INTERLACED_TB again and so on. Also you get the same amount of pixels as for
>> a progressive setup so the data-output-rate is higher. Maybe we need a
>> FIELD_DEINTERLACED to denote such a setup?
> I don't think this is correct. The ADV7280 has no framestore, just a small
> linebuffer. It does I2P by line doubling plus some filtering and a little
> bit of proprietary magic, allegedly.
> 
> I believe the output in I2P mode for your example would be (AA) (BB) (CC).
> The clock rate and pixel rate is doubled since it sends a full (faked up)
> frame per field time.
> 
> I don't know what the FIELD_* mode is for line doubled pseudo-progressive.
> 
> Also, I don't know why anyone would use this mode. I don't see a scenario
> where it would actually improve video quality over a more sophisticated
> motion adaptive deinterlace and to restore a 25/30fps feed you'd need to
> decimate and lose information.
> 
> Quote from "Rob.Analog", who uses the word "frame" freely, here:
> https://ez.analog.com/thread/39382
> 
> "2) In I2P mode the number of lines per frame doubles. The ADV7280 still
> outputs 50 frames per second ( or 60 frames in NTSC mode) but each frame now
> consists of twice as many lines. e.g. if a frame consisted of 288 lines of
> active video in interlaced mode, this is doubled to 576 lines of active
> video in progressive mode. The line doubling is achieved by the ADV7280
> interpolating between two lines of video (e.g. between lines 1 and 3 on an
> odd frame) and inserting an extra line (e.g. line 2). There are also some
> ADI propriety algorithms that prevent low angle noise artifacts.
> 
> In order to achieve this line doubling, the LLC clock doubles from a nominal
> 27MHz to a nominal 54MHz"

Hm, so it is basically just actually a scaling feature rather than a
deinterlacer. I guess we should expose it as that then.

Re: [PATCHv2 4/7] media: rcar-vin: fix height for TOP and BOTTOM fields

[Sergei Shtylyov]

On 08/02/2016 05:51 PM, Niklas Söderlund wrote:

> 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

    s/filed/field/.

> frame.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
[...]

MBR, Sergei

Re: [PATCHv2 7/7] [PATCHv5] media: adv7180: fix field type

[Steve Longerbeam]

On 08/03/2016 06:21 AM, Niklas Söderlund wrote:
> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote:
>> [...]
>>> 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;
>> I'm not convinced this is correct. As far as I understand it when the I2P
>> feature is enabled the core outputs full progressive frames at the full
>> framerate. If it is bypassed it outputs half-frames. So we have the option
>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I
>> think this branch should setup the field format to be ALTERNATE regardless
>> of whether the I2P feature is available.
> I be happy to update the patch in such manner, but I feel this is more 
> for Steven to handle. I have no deep understanding of the adv7180 driver 
> and the only HW I have is the adv7180 and not adv7280, adv7280_m, 
> adv7282 or adv7282_m which is the models which have the ADV7180_FLAG_I2P 
> flag. So I can't really test such a change.
>
> Steven do you want to update this patch and resend it? 

Hi Niklas, I can update this patch, but it sounds like the whole thing
is "up in the air" at this point, and we may want to yank out the I2P
support altogether. I'll leave it up to Lars and others to work that out
first.

Steve

Re: [PATCHv2 7/7] [PATCHv5] media: adv7180: fix field type

[Steve Longerbeam]

On 08/03/2016 06:21 AM, Niklas S�derlund wrote:
> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote:
>> [...]
>>> 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;
>> I'm not convinced this is correct. As far as I understand it when the I2P
>> feature is enabled the core outputs full progressive frames at the full
>> framerate. If it is bypassed it outputs half-frames. So we have the option
>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I
>> think this branch should setup the field format to be ALTERNATE regardless
>> of whether the I2P feature is available.
> I be happy to update the patch in such manner, but I feel this is more 
> for Steven to handle. I have no deep understanding of the adv7180 driver 
> and the only HW I have is the adv7180 and not adv7280, adv7280_m, 
> adv7282 or adv7282_m which is the models which have the ADV7180_FLAG_I2P 
> flag. So I can't really test such a change.
>
> Steven do you want to update this patch and resend it? 

Hi Niklas, I can update this patch, but it sounds like the whole thing
is "up in the air" at this point, and we may want to yank out the I2P
support altogether. I'll leave it up to Lars and others to work that out
first.

Steve

Re: [PATCHv2 7/7] [PATCHv5] media: adv7180: fix field type

[Lars-Peter Clausen]

On 08/03/2016 06:55 PM, Steve Longerbeam wrote:
> On 08/03/2016 06:21 AM, Niklas Söderlund wrote:
>> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote:
>>> [...]
>>>> 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;
>>> I'm not convinced this is correct. As far as I understand it when the I2P
>>> feature is enabled the core outputs full progressive frames at the full
>>> framerate. If it is bypassed it outputs half-frames. So we have the option
>>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I
>>> think this branch should setup the field format to be ALTERNATE regardless
>>> of whether the I2P feature is available.
>> I be happy to update the patch in such manner, but I feel this is more 
>> for Steven to handle. I have no deep understanding of the adv7180 driver 
>> and the only HW I have is the adv7180 and not adv7280, adv7280_m, 
>> adv7282 or adv7282_m which is the models which have the ADV7180_FLAG_I2P 
>> flag. So I can't really test such a change.
>>
>> Steven do you want to update this patch and resend it? 
> 
> Hi Niklas, I can update this patch, but it sounds like the whole thing
> is "up in the air" at this point, and we may want to yank out the I2P
> support altogether. I'll leave it up to Lars and others to work that out
> first.

Yeah, we should remove the whole I2P stuff, I was misinformed about how it
works. But either way I think this patch should simply not touch the current
behavior, so don't add new if (FLAG_I2P) checks.

Re: [PATCHv2 7/7] [PATCHv5] media: adv7180: fix field type

[Lars-Peter Clausen]

On 08/03/2016 06:55 PM, Steve Longerbeam wrote:
> On 08/03/2016 06:21 AM, Niklas S�derlund wrote:
>> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote:
>>> [...]
>>>> 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;
>>> I'm not convinced this is correct. As far as I understand it when the I2P
>>> feature is enabled the core outputs full progressive frames at the full
>>> framerate. If it is bypassed it outputs half-frames. So we have the option
>>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I
>>> think this branch should setup the field format to be ALTERNATE regardless
>>> of whether the I2P feature is available.
>> I be happy to update the patch in such manner, but I feel this is more 
>> for Steven to handle. I have no deep understanding of the adv7180 driver 
>> and the only HW I have is the adv7180 and not adv7280, adv7280_m, 
>> adv7282 or adv7282_m which is the models which have the ADV7180_FLAG_I2P 
>> flag. So I can't really test such a change.
>>
>> Steven do you want to update this patch and resend it? 
> 
> Hi Niklas, I can update this patch, but it sounds like the whole thing
> is "up in the air" at this point, and we may want to yank out the I2P
> support altogether. I'll leave it up to Lars and others to work that out
> first.

Yeah, we should remove the whole I2P stuff, I was misinformed about how it
works. But either way I think this patch should simply not touch the current
behavior, so don't add new if (FLAG_I2P) checks.

Re: [PATCHv2 7/7] [PATCHv5] media: adv7180: fix field type

[Steve Longerbeam]

On 08/03/2016 09:58 AM, Lars-Peter Clausen wrote:
> On 08/03/2016 06:55 PM, Steve Longerbeam wrote:
>> On 08/03/2016 06:21 AM, Niklas Söderlund wrote:
>>> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote:
>>>> [...]
>>>>> 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;
>>>> I'm not convinced this is correct. As far as I understand it when the I2P
>>>> feature is enabled the core outputs full progressive frames at the full
>>>> framerate. If it is bypassed it outputs half-frames. So we have the option
>>>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I
>>>> think this branch should setup the field format to be ALTERNATE regardless
>>>> of whether the I2P feature is available.
>>> I be happy to update the patch in such manner, but I feel this is more 
>>> for Steven to handle. I have no deep understanding of the adv7180 driver 
>>> and the only HW I have is the adv7180 and not adv7280, adv7280_m, 
>>> adv7282 or adv7282_m which is the models which have the ADV7180_FLAG_I2P 
>>> flag. So I can't really test such a change.
>>>
>>> Steven do you want to update this patch and resend it? 
>> Hi Niklas, I can update this patch, but it sounds like the whole thing
>> is "up in the air" at this point, and we may want to yank out the I2P
>> support altogether. I'll leave it up to Lars and others to work that out
>> first.
> Yeah, we should remove the whole I2P stuff, I was misinformed about how it
> works. But either way I think this patch should simply not touch the current
> behavior, so don't add new if (FLAG_I2P) checks.

Hi Lars, Ok I can do that. I'll resubmit in next version of my patchset.

Steve

Re: [PATCHv2 7/7] [PATCHv5] media: adv7180: fix field type

[Steve Longerbeam]

On 08/03/2016 09:58 AM, Lars-Peter Clausen wrote:
> On 08/03/2016 06:55 PM, Steve Longerbeam wrote:
>> On 08/03/2016 06:21 AM, Niklas S�derlund wrote:
>>> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote:
>>>> [...]
>>>>> 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;
>>>> I'm not convinced this is correct. As far as I understand it when the I2P
>>>> feature is enabled the core outputs full progressive frames at the full
>>>> framerate. If it is bypassed it outputs half-frames. So we have the option
>>>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I
>>>> think this branch should setup the field format to be ALTERNATE regardless
>>>> of whether the I2P feature is available.
>>> I be happy to update the patch in such manner, but I feel this is more 
>>> for Steven to handle. I have no deep understanding of the adv7180 driver 
>>> and the only HW I have is the adv7180 and not adv7280, adv7280_m, 
>>> adv7282 or adv7282_m which is the models which have the ADV7180_FLAG_I2P 
>>> flag. So I can't really test such a change.
>>>
>>> Steven do you want to update this patch and resend it? 
>> Hi Niklas, I can update this patch, but it sounds like the whole thing
>> is "up in the air" at this point, and we may want to yank out the I2P
>> support altogether. I'll leave it up to Lars and others to work that out
>> first.
> Yeah, we should remove the whole I2P stuff, I was misinformed about how it
> works. But either way I think this patch should simply not touch the current
> behavior, so don't add new if (FLAG_I2P) checks.

Hi Lars, Ok I can do that. I'll resubmit in next version of my patchset.

Steve

Re: [PATCHv2 7/7] [PATCHv5] media: adv7180: fix field type

[Niklas Söderlund]

On 2016-08-03 10:14:45 -0700, Steve Longerbeam wrote:
> On 08/03/2016 09:58 AM, Lars-Peter Clausen wrote:
> > On 08/03/2016 06:55 PM, Steve Longerbeam wrote:
> >> On 08/03/2016 06:21 AM, Niklas Söderlund wrote:
> >>> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote:
> >>>> [...]
> >>>>> 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;
> >>>> I'm not convinced this is correct. As far as I understand it when the I2P
> >>>> feature is enabled the core outputs full progressive frames at the full
> >>>> framerate. If it is bypassed it outputs half-frames. So we have the option
> >>>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I
> >>>> think this branch should setup the field format to be ALTERNATE regardless
> >>>> of whether the I2P feature is available.
> >>> I be happy to update the patch in such manner, but I feel this is more 
> >>> for Steven to handle. I have no deep understanding of the adv7180 driver 
> >>> and the only HW I have is the adv7180 and not adv7280, adv7280_m, 
> >>> adv7282 or adv7282_m which is the models which have the ADV7180_FLAG_I2P 
> >>> flag. So I can't really test such a change.
> >>>
> >>> Steven do you want to update this patch and resend it? 
> >> Hi Niklas, I can update this patch, but it sounds like the whole thing
> >> is "up in the air" at this point, and we may want to yank out the I2P
> >> support altogether. I'll leave it up to Lars and others to work that out
> >> first.
> > Yeah, we should remove the whole I2P stuff, I was misinformed about how it
> > works. But either way I think this patch should simply not touch the current
> > behavior, so don't add new if (FLAG_I2P) checks.
> 
> Hi Lars, Ok I can do that. I'll resubmit in next version of my patchset.

Thanks Steven, then I will drop this patch in my v3. Can you pleas CC me 
when you send out your patch?

-- 
Regards,
Niklas Söderlund

Re: [PATCHv2 7/7] [PATCHv5] media: adv7180: fix field type

[Niklas Söderlund]

On 2016-08-03 10:14:45 -0700, Steve Longerbeam wrote:
> On 08/03/2016 09:58 AM, Lars-Peter Clausen wrote:
> > On 08/03/2016 06:55 PM, Steve Longerbeam wrote:
> >> On 08/03/2016 06:21 AM, Niklas S�derlund wrote:
> >>> On 2016-08-02 17:00:07 +0200, Lars-Peter Clausen wrote:
> >>>> [...]
> >>>>> 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;
> >>>> I'm not convinced this is correct. As far as I understand it when the I2P
> >>>> feature is enabled the core outputs full progressive frames at the full
> >>>> framerate. If it is bypassed it outputs half-frames. So we have the option
> >>>> of either V4L2_FIELD_NONE or V4L2_FIELD_ALTERNATE, but never interlaced. I
> >>>> think this branch should setup the field format to be ALTERNATE regardless
> >>>> of whether the I2P feature is available.
> >>> I be happy to update the patch in such manner, but I feel this is more 
> >>> for Steven to handle. I have no deep understanding of the adv7180 driver 
> >>> and the only HW I have is the adv7180 and not adv7280, adv7280_m, 
> >>> adv7282 or adv7282_m which is the models which have the ADV7180_FLAG_I2P 
> >>> flag. So I can't really test such a change.
> >>>
> >>> Steven do you want to update this patch and resend it? 
> >> Hi Niklas, I can update this patch, but it sounds like the whole thing
> >> is "up in the air" at this point, and we may want to yank out the I2P
> >> support altogether. I'll leave it up to Lars and others to work that out
> >> first.
> > Yeah, we should remove the whole I2P stuff, I was misinformed about how it
> > works. But either way I think this patch should simply not touch the current
> > behavior, so don't add new if (FLAG_I2P) checks.
> 
> Hi Lars, Ok I can do that. I'll resubmit in next version of my patchset.

Thanks Steven, then I will drop this patch in my v3. Can you pleas CC me 
when you send out your patch?

-- 
Regards,
Niklas S�derlund