[PATCH 0/6] imx-media: Fixes for bt.656 interlaced capture

[PATCH 0/6] imx-media: Fixes for bt.656 interlaced capture

[Steve Longerbeam]

A set of patches that fixes some bugs with capturing from a bt.656
interlaced source, and incompatibilites between IDMAC interlace
interweaving and 4:2:0 data write reduction.

Steve Longerbeam (6):
  media: imx-csi: Fix interlaced bt.656
  gpu: ipu-csi: Check for field type alternate
  media: videodev2.h: Add macro V4L2_FIELD_IS_SEQUENTIAL
  media: imx-csi: Enable interlaced scan for field type alternate
  media: imx-csi: Allow skipping odd chroma rows for YVU420
  media: staging/imx: interweave and odd-chroma-row skip are
    incompatible

 drivers/gpu/ipu-v3/ipu-csi.c                |  3 ++-
 drivers/staging/media/imx/imx-ic-prpencvf.c | 18 ++++++++++++-----
 drivers/staging/media/imx/imx-media-csi.c   | 31 ++++++++++++++---------------
 include/uapi/linux/videodev2.h              |  4 ++++
 4 files changed, 34 insertions(+), 22 deletions(-)

-- 
2.7.4

[PATCH 1/6] media: imx-csi: Fix interlaced bt.656

[Steve Longerbeam]

The output pad's field type was being passed to ipu_csi_init_interface(),
in order to deal with field type 'alternate' at the sink pad, which
must be translated to either 'seq-tb' or 'seq-bt' to be understood by
ipu_csi_init_interface(). Doing so breaks the CSI interface setup if
the output pad field type is set to 'none' and the input bus type is
BT.656.

So remove that code and pass unmodified sink pad field type to
ipu_csi_init_interface(). The latter function will have to explicity
deal with field type 'alternate' when setting up the CSI interface
for BT.656 busses.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/staging/media/imx/imx-media-csi.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index 95d7805..9bc555c 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -629,12 +629,10 @@ static void csi_idmac_stop(struct csi_priv *priv)
 /* Update the CSI whole sensor and active windows */
 static int csi_setup(struct csi_priv *priv)
 {
-	struct v4l2_mbus_framefmt *infmt, *outfmt;
+	struct v4l2_mbus_framefmt *infmt;
 	struct v4l2_mbus_config mbus_cfg;
-	struct v4l2_mbus_framefmt if_fmt;
 
 	infmt = &priv->format_mbus[CSI_SINK_PAD];
-	outfmt = &priv->format_mbus[priv->active_output_pad];
 
 	/* compose mbus_config from the upstream endpoint */
 	mbus_cfg.type = priv->upstream_ep.bus_type;
@@ -642,20 +640,13 @@ static int csi_setup(struct csi_priv *priv)
 		priv->upstream_ep.bus.mipi_csi2.flags :
 		priv->upstream_ep.bus.parallel.flags;
 
-	/*
-	 * we need to pass input frame to CSI interface, but
-	 * with translated field type from output format
-	 */
-	if_fmt = *infmt;
-	if_fmt.field = outfmt->field;
-
 	ipu_csi_set_window(priv->csi, &priv->crop);
 
 	ipu_csi_set_downsize(priv->csi,
 			     priv->crop.width == 2 * priv->compose.width,
 			     priv->crop.height == 2 * priv->compose.height);
 
-	ipu_csi_init_interface(priv->csi, &mbus_cfg, &if_fmt);
+	ipu_csi_init_interface(priv->csi, &mbus_cfg, infmt);
 
 	ipu_csi_set_dest(priv->csi, priv->dest);
 
-- 
2.7.4

[PATCH 2/6] gpu: ipu-csi: Check for field type alternate

[Steve Longerbeam]

When the CSI is receiving from a bt.656 bus, include a check for
field type 'alternate' when determining whether to set CSI clock
mode to CCIR656_INTERLACED or CCIR656_PROGRESSIVE.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/gpu/ipu-v3/ipu-csi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/ipu-v3/ipu-csi.c b/drivers/gpu/ipu-v3/ipu-csi.c
index caa05b0..5450a2d 100644
--- a/drivers/gpu/ipu-v3/ipu-csi.c
+++ b/drivers/gpu/ipu-v3/ipu-csi.c
@@ -339,7 +339,8 @@ static void fill_csi_bus_cfg(struct ipu_csi_bus_config *csicfg,
 		break;
 	case V4L2_MBUS_BT656:
 		csicfg->ext_vsync = 0;
-		if (V4L2_FIELD_HAS_BOTH(mbus_fmt->field))
+		if (V4L2_FIELD_HAS_BOTH(mbus_fmt->field) ||
+		    mbus_fmt->field == V4L2_FIELD_ALTERNATE)
 			csicfg->clk_mode = IPU_CSI_CLK_MODE_CCIR656_INTERLACED;
 		else
 			csicfg->clk_mode = IPU_CSI_CLK_MODE_CCIR656_PROGRESSIVE;
-- 
2.7.4

[PATCH 3/6] media: videodev2.h: Add macro V4L2_FIELD_IS_SEQUENTIAL

[Steve Longerbeam]

Add a macro that returns true if the given field type is 'sequential',
that is, the data is transmitted, or exists in memory, as all top field
lines followed by all bottom field lines, or vice-versa.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 include/uapi/linux/videodev2.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 600877b..408ee96 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -126,6 +126,10 @@ enum v4l2_field {
 	 (field) == V4L2_FIELD_INTERLACED_BT ||\
 	 (field) == V4L2_FIELD_SEQ_TB ||\
 	 (field) == V4L2_FIELD_SEQ_BT)
+#define V4L2_FIELD_IS_SEQUENTIAL(field) \
+	((field) == V4L2_FIELD_SEQ_TB ||\
+	 (field) == V4L2_FIELD_SEQ_BT ||\
+	 (field) == V4L2_FIELD_ALTERNATE)
 #define V4L2_FIELD_HAS_T_OR_B(field)	\
 	((field) == V4L2_FIELD_BOTTOM ||\
 	 (field) == V4L2_FIELD_TOP ||\
-- 
2.7.4

[PATCH 4/6] media: imx-csi: Enable interlaced scan for field type alternate

[Steve Longerbeam]

Interlaced scan, a.k.a. interweave, should be enabled at the CSI IDMAC
output pad if the input field type is 'alternate' (in addition to field
types 'seq-tb' and 'seq-bt').

Which brings up whether V4L2_FIELD_HAS_BOTH() macro should be used
to determine enabling interlaced/interweave scan. That macro
includes the 'interlaced' field types, and in those cases the data
is already interweaved with top/bottom field lines. A heads-up for
now that this if statement may need to call V4L2_FIELD_IS_SEQUENTIAL()
instead, I have no sensor hardware that sends 'interlaced' data, so can't
test.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/staging/media/imx/imx-media-csi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index 9bc555c..eef3483 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -477,7 +477,8 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
 	ipu_smfc_set_burstsize(priv->smfc, burst_size);
 
 	if (image.pix.field == V4L2_FIELD_NONE &&
-	    V4L2_FIELD_HAS_BOTH(infmt->field))
+	    (V4L2_FIELD_HAS_BOTH(infmt->field) ||
+	     infmt->field == V4L2_FIELD_ALTERNATE))
 		ipu_cpmem_interlaced_scan(priv->idmac_ch,
 					  image.pix.bytesperline);
 
-- 
2.7.4

[PATCH 5/6] media: imx-csi: Allow skipping odd chroma rows for YVU420

[Steve Longerbeam]

Skip writing U/V components to odd rows for YVU420 in addition to
YUV420 and NV12.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/staging/media/imx/imx-media-csi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index eef3483..6829c08 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -415,6 +415,7 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
 		passthrough_bits = 16;
 		break;
 	case V4L2_PIX_FMT_YUV420:
+	case V4L2_PIX_FMT_YVU420:
 	case V4L2_PIX_FMT_NV12:
 		burst_size = (image.pix.width & 0x3f) ?
 			     ((image.pix.width & 0x1f) ?
-- 
2.7.4

[PATCH 6/6] media: staging/imx: interweave and odd-chroma-row skip are incompatible

[Steve Longerbeam]

If IDMAC interweaving is enabled in a write channel, the channel must
write the odd chroma rows for 4:2:0 formats. Skipping writing the odd
chroma rows produces corrupted captured 4:2:0 images when interweave
is enabled.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/staging/media/imx/imx-ic-prpencvf.c | 18 +++++++++++++-----
 drivers/staging/media/imx/imx-media-csi.c   | 18 ++++++++++++------
 2 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
index ae453fd..b63b3f4 100644
--- a/drivers/staging/media/imx/imx-ic-prpencvf.c
+++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
@@ -353,6 +353,7 @@ static int prp_setup_channel(struct prp_priv *priv,
 	struct v4l2_mbus_framefmt *infmt;
 	unsigned int burst_size;
 	struct ipu_image image;
+	bool interweave;
 	int ret;
 
 	infmt = &priv->format_mbus[PRPENCVF_SINK_PAD];
@@ -365,6 +366,10 @@ static int prp_setup_channel(struct prp_priv *priv,
 	image.rect.width = image.pix.width;
 	image.rect.height = image.pix.height;
 
+	interweave = (image.pix.field == V4L2_FIELD_NONE &&
+		      V4L2_FIELD_HAS_BOTH(infmt->field) &&
+		      channel == priv->out_ch);
+
 	if (rot_swap_width_height) {
 		swap(image.pix.width, image.pix.height);
 		swap(image.rect.width, image.rect.height);
@@ -377,12 +382,17 @@ static int prp_setup_channel(struct prp_priv *priv,
 	image.phys0 = addr0;
 	image.phys1 = addr1;
 
-	if (channel == priv->out_ch || channel == priv->rot_out_ch) {
+	/*
+	 * Skip writing U and V components to odd rows in the output
+	 * channels for planar 4:2:0 (but not when enabling IDMAC
+	 * interweaving, they are incompatible).
+	 */
+	if (!interweave && (channel == priv->out_ch ||
+			    channel == priv->rot_out_ch)) {
 		switch (image.pix.pixelformat) {
 		case V4L2_PIX_FMT_YUV420:
 		case V4L2_PIX_FMT_YVU420:
 		case V4L2_PIX_FMT_NV12:
-			/* Skip writing U and V components to odd rows */
 			ipu_cpmem_skip_odd_chroma_rows(channel);
 			break;
 		}
@@ -405,9 +415,7 @@ static int prp_setup_channel(struct prp_priv *priv,
 	if (rot_mode)
 		ipu_cpmem_set_rotation(channel, rot_mode);
 
-	if (image.pix.field == V4L2_FIELD_NONE &&
-	    V4L2_FIELD_HAS_BOTH(infmt->field) &&
-	    channel == priv->out_ch)
+	if (interweave)
 		ipu_cpmem_interlaced_scan(channel, image.pix.bytesperline);
 
 	ret = ipu_ic_task_idma_init(priv->ic, channel,
diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index 6829c08..ab2de71 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -368,10 +368,10 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
 {
 	struct imx_media_video_dev *vdev = priv->vdev;
 	struct v4l2_mbus_framefmt *infmt;
+	bool passthrough, interweave;
 	struct ipu_image image;
 	u32 passthrough_bits;
 	dma_addr_t phys[2];
-	bool passthrough;
 	u32 burst_size;
 	int ret;
 
@@ -389,6 +389,10 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
 	image.phys0 = phys[0];
 	image.phys1 = phys[1];
 
+	interweave = (image.pix.field == V4L2_FIELD_NONE &&
+		      (V4L2_FIELD_HAS_BOTH(infmt->field) ||
+		       infmt->field == V4L2_FIELD_ALTERNATE));
+
 	/*
 	 * Check for conditions that require the IPU to handle the
 	 * data internally as generic data, aka passthrough mode:
@@ -422,8 +426,12 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
 			      ((image.pix.width & 0xf) ? 8 : 16) : 32) : 64;
 		passthrough = is_parallel_16bit_bus(&priv->upstream_ep);
 		passthrough_bits = 16;
-		/* Skip writing U and V components to odd rows */
-		ipu_cpmem_skip_odd_chroma_rows(priv->idmac_ch);
+		/*
+		 * Skip writing U and V components to odd rows (but not
+		 * when enabling IDMAC interweaving, they are incompatible).
+		 */
+		if (!interweave)
+			ipu_cpmem_skip_odd_chroma_rows(priv->idmac_ch);
 		break;
 	case V4L2_PIX_FMT_YUYV:
 	case V4L2_PIX_FMT_UYVY:
@@ -477,9 +485,7 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
 
 	ipu_smfc_set_burstsize(priv->smfc, burst_size);
 
-	if (image.pix.field == V4L2_FIELD_NONE &&
-	    (V4L2_FIELD_HAS_BOTH(infmt->field) ||
-	     infmt->field == V4L2_FIELD_ALTERNATE))
+	if (interweave)
 		ipu_cpmem_interlaced_scan(priv->idmac_ch,
 					  image.pix.bytesperline);
 
-- 
2.7.4

Re: [PATCH 3/6] media: videodev2.h: Add macro V4L2_FIELD_IS_SEQUENTIAL

[Nicolas Dufresne]

(in text this time, sorry)

Le vendredi 25 mai 2018 à 16:53 -0700, Steve Longerbeam a écrit :
> Add a macro that returns true if the given field type is
> 'sequential',
> that is, the data is transmitted, or exists in memory, as all top
> field
> lines followed by all bottom field lines, or vice-versa.
> 
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
>  include/uapi/linux/videodev2.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/uapi/linux/videodev2.h
> b/include/uapi/linux/videodev2.h
> index 600877b..408ee96 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -126,6 +126,10 @@ enum v4l2_field {
>  	 (field) == V4L2_FIELD_INTERLACED_BT ||\
>  	 (field) == V4L2_FIELD_SEQ_TB ||\
>  	 (field) == V4L2_FIELD_SEQ_BT)
> +#define V4L2_FIELD_IS_SEQUENTIAL(field) \
> +	((field) == V4L2_FIELD_SEQ_TB ||\
> +	 (field) == V4L2_FIELD_SEQ_BT ||\
> +	 (field) == V4L2_FIELD_ALTERNATE)

No, alternate has no place here, in alternate mode each buffers have
only one field.

>  #define V4L2_FIELD_HAS_T_OR_B(field)	\
>  	((field) == V4L2_FIELD_BOTTOM (||\
>  	 (field) == V4L2_FIELD_TOP ||\

Re: [PATCH 3/6] media: videodev2.h: Add macro V4L2_FIELD_IS_SEQUENTIAL

[Steve Longerbeam]

On 05/25/2018 05:10 PM, Nicolas Dufresne wrote:
> (in text this time, sorry)
>
> Le vendredi 25 mai 2018 à 16:53 -0700, Steve Longerbeam a écrit :
>> Add a macro that returns true if the given field type is
>> 'sequential',
>> that is, the data is transmitted, or exists in memory, as all top
>> field
>> lines followed by all bottom field lines, or vice-versa.
>>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>> ---
>>   include/uapi/linux/videodev2.h | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/include/uapi/linux/videodev2.h
>> b/include/uapi/linux/videodev2.h
>> index 600877b..408ee96 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -126,6 +126,10 @@ enum v4l2_field {
>>   	 (field) == V4L2_FIELD_INTERLACED_BT ||\
>>   	 (field) == V4L2_FIELD_SEQ_TB ||\
>>   	 (field) == V4L2_FIELD_SEQ_BT)
>> +#define V4L2_FIELD_IS_SEQUENTIAL(field) \
>> +	((field) == V4L2_FIELD_SEQ_TB ||\
>> +	 (field) == V4L2_FIELD_SEQ_BT ||\
>> +	 (field) == V4L2_FIELD_ALTERNATE)
> No, alternate has no place here, in alternate mode each buffers have
> only one field.

Then I misunderstand what is meant by "alternate". The name implies
to me that a source sends top or bottom field alternately, or top/bottom
fields exist in memory buffers alternately, but with no information about
which field came first. In other words, "alternate" is either seq-tb or 
seq-bt,
without any info about field order.

If it is just one field in a memory buffer, why isn't it called
V4L2_FIELD_TOP_OR_BOTTOM, e.g. we don't know which?

Steve

Re: [PATCH 3/6] media: videodev2.h: Add macro V4L2_FIELD_IS_SEQUENTIAL

[Nicolas Dufresne]

Le vendredi 25 mai 2018 à 17:19 -0700, Steve Longerbeam a écrit :
> 
> On 05/25/2018 05:10 PM, Nicolas Dufresne wrote:
> > (in text this time, sorry)
> > 
> > Le vendredi 25 mai 2018 à 16:53 -0700, Steve Longerbeam a écrit :
> > > Add a macro that returns true if the given field type is
> > > 'sequential',
> > > that is, the data is transmitted, or exists in memory, as all top
> > > field
> > > lines followed by all bottom field lines, or vice-versa.
> > > 
> > > Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> > > ---
> > >   include/uapi/linux/videodev2.h | 4 ++++
> > >   1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/include/uapi/linux/videodev2.h
> > > b/include/uapi/linux/videodev2.h
> > > index 600877b..408ee96 100644
> > > --- a/include/uapi/linux/videodev2.h
> > > +++ b/include/uapi/linux/videodev2.h
> > > @@ -126,6 +126,10 @@ enum v4l2_field {
> > >   	 (field) == V4L2_FIELD_INTERLACED_BT ||\
> > >   	 (field) == V4L2_FIELD_SEQ_TB ||\
> > >   	 (field) == V4L2_FIELD_SEQ_BT)
> > > +#define V4L2_FIELD_IS_SEQUENTIAL(field) \
> > > +	((field) == V4L2_FIELD_SEQ_TB ||\
> > > +	 (field) == V4L2_FIELD_SEQ_BT ||\
> > > +	 (field) == V4L2_FIELD_ALTERNATE)
> > 
> > No, alternate has no place here, in alternate mode each buffers have
> > only one field.
> 
> Then I misunderstand what is meant by "alternate". The name implies
> to me that a source sends top or bottom field alternately, or top/bottom
> fields exist in memory buffers alternately, but with no information about
> which field came first. In other words, "alternate" is either seq-tb or 
> seq-bt,
> without any info about field order.
> 
> If it is just one field in a memory buffer, why isn't it called
> V4L2_FIELD_TOP_OR_BOTTOM, e.g. we don't know which?

I don't see why this could be better then ALTERNATE, were buffers are
only top or bottom fields alternatively. And even if there was another
possible name, this is public API.

V4L2_FIELD_ALTERNATE is a mode, that will only be used with
v4l2_pix_format or v4l2_pix_format_mplane. I should never bet set on
the v4l2_buffer.field, instead the driver indicates the parity of the
field by setting V42_FIELD_TOP/BOTTOM on the v4l2_buffer returned by
DQBUF. This is a very different mode of operation compared to
sequential, hence why I believe it is wrong to make it part of the new
helper. So far, it's the only field value that has this asymmetric
usage and meaning.

> 
> Steve
> 

Re: [PATCH 3/6] media: videodev2.h: Add macro V4L2_FIELD_IS_SEQUENTIAL

[Nicolas Dufresne]

Le vendredi 25 mai 2018 à 21:14 -0400, Nicolas Dufresne a écrit :
> Le vendredi 25 mai 2018 à 17:19 -0700, Steve Longerbeam a écrit :
> > 
> > On 05/25/2018 05:10 PM, Nicolas Dufresne wrote:
> > > (in text this time, sorry)
> > > 
> > > Le vendredi 25 mai 2018 à 16:53 -0700, Steve Longerbeam a écrit :
> > > > Add a macro that returns true if the given field type is
> > > > 'sequential',
> > > > that is, the data is transmitted, or exists in memory, as all top
> > > > field
> > > > lines followed by all bottom field lines, or vice-versa.
> > > > 
> > > > Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> > > > ---
> > > >   include/uapi/linux/videodev2.h | 4 ++++
> > > >   1 file changed, 4 insertions(+)
> > > > 
> > > > diff --git a/include/uapi/linux/videodev2.h
> > > > b/include/uapi/linux/videodev2.h
> > > > index 600877b..408ee96 100644
> > > > --- a/include/uapi/linux/videodev2.h
> > > > +++ b/include/uapi/linux/videodev2.h
> > > > @@ -126,6 +126,10 @@ enum v4l2_field {
> > > >   	 (field) == V4L2_FIELD_INTERLACED_BT ||\
> > > >   	 (field) == V4L2_FIELD_SEQ_TB ||\
> > > >   	 (field) == V4L2_FIELD_SEQ_BT)
> > > > +#define V4L2_FIELD_IS_SEQUENTIAL(field) \
> > > > +	((field) == V4L2_FIELD_SEQ_TB ||\
> > > > +	 (field) == V4L2_FIELD_SEQ_BT ||\
> > > > +	 (field) == V4L2_FIELD_ALTERNATE)
> > > 
> > > No, alternate has no place here, in alternate mode each buffers have
> > > only one field.
> > 
> > Then I misunderstand what is meant by "alternate". The name implies
> > to me that a source sends top or bottom field alternately, or top/bottom
> > fields exist in memory buffers alternately, but with no information about
> > which field came first. In other words, "alternate" is either seq-tb or 
> > seq-bt,
> > without any info about field order.
> > 
> > If it is just one field in a memory buffer, why isn't it called
> > V4L2_FIELD_TOP_OR_BOTTOM, e.g. we don't know which?
> 
> I don't see why this could be better then ALTERNATE, were buffers are
> only top or bottom fields alternatively. And even if there was another
> possible name, this is public API.
> 
> V4L2_FIELD_ALTERNATE is a mode, that will only be used with
> v4l2_pix_format or v4l2_pix_format_mplane. I should never bet set on
> the v4l2_buffer.field, instead the driver indicates the parity of the
> field by setting V42_FIELD_TOP/BOTTOM on the v4l2_buffer returned by
> DQBUF. This is a very different mode of operation compared to
> sequential, hence why I believe it is wrong to make it part of the new
> helper. So far, it's the only field value that has this asymmetric
> usage and meaning.

I should have put some references. The explanation of the modes, with a
temporal representation of the fields. Small note, in ALTERNATE mode
bottom and top fields will likely not share the same timestamp, it is a
mode used to achieve lower latency.

https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/field-order.html#c.v4l2_field

And in this section, you'll see a paragraph that explain the field
values when running in ALTERNATE mode.

https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/buffer.html#c.v4l2_buffer

> 
> > 
> > Steve
> > 

Re: [PATCH 3/6] media: videodev2.h: Add macro V4L2_FIELD_IS_SEQUENTIAL

[Steve Longerbeam]

Hi Nicolas,


On 05/25/2018 06:21 PM, Nicolas Dufresne wrote:
> Le vendredi 25 mai 2018 à 21:14 -0400, Nicolas Dufresne a écrit :
>> Le vendredi 25 mai 2018 à 17:19 -0700, Steve Longerbeam a écrit :
>>> On 05/25/2018 05:10 PM, Nicolas Dufresne wrote:
>>>> (in text this time, sorry)
>>>>
>>>> Le vendredi 25 mai 2018 à 16:53 -0700, Steve Longerbeam a écrit :
>>>>> Add a macro that returns true if the given field type is
>>>>> 'sequential',
>>>>> that is, the data is transmitted, or exists in memory, as all top
>>>>> field
>>>>> lines followed by all bottom field lines, or vice-versa.
>>>>>
>>>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>>>> ---
>>>>>    include/uapi/linux/videodev2.h | 4 ++++
>>>>>    1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/include/uapi/linux/videodev2.h
>>>>> b/include/uapi/linux/videodev2.h
>>>>> index 600877b..408ee96 100644
>>>>> --- a/include/uapi/linux/videodev2.h
>>>>> +++ b/include/uapi/linux/videodev2.h
>>>>> @@ -126,6 +126,10 @@ enum v4l2_field {
>>>>>    	 (field) == V4L2_FIELD_INTERLACED_BT ||\
>>>>>    	 (field) == V4L2_FIELD_SEQ_TB ||\
>>>>>    	 (field) == V4L2_FIELD_SEQ_BT)
>>>>> +#define V4L2_FIELD_IS_SEQUENTIAL(field) \
>>>>> +	((field) == V4L2_FIELD_SEQ_TB ||\
>>>>> +	 (field) == V4L2_FIELD_SEQ_BT ||\
>>>>> +	 (field) == V4L2_FIELD_ALTERNATE)
>>>> No, alternate has no place here, in alternate mode each buffers have
>>>> only one field.
>>> Then I misunderstand what is meant by "alternate". The name implies
>>> to me that a source sends top or bottom field alternately, or top/bottom
>>> fields exist in memory buffers alternately, but with no information about
>>> which field came first. In other words, "alternate" is either seq-tb or
>>> seq-bt,
>>> without any info about field order.
>>>
>>> If it is just one field in a memory buffer, why isn't it called
>>> V4L2_FIELD_TOP_OR_BOTTOM, e.g. we don't know which?
>> I don't see why this could be better then ALTERNATE, were buffers are
>> only top or bottom fields alternatively. And even if there was another
>> possible name, this is public API.
>>
>> V4L2_FIELD_ALTERNATE is a mode, that will only be used with
>> v4l2_pix_format or v4l2_pix_format_mplane. I should never bet set on
>> the v4l2_buffer.field, instead the driver indicates the parity of the
>> field by setting V42_FIELD_TOP/BOTTOM on the v4l2_buffer returned by
>> DQBUF. This is a very different mode of operation compared to
>> sequential, hence why I believe it is wrong to make it part of the new
>> helper. So far, it's the only field value that has this asymmetric
>> usage and meaning.
> I should have put some references. The explanation of the modes, with a
> temporal representation of the fields. Small note, in ALTERNATE mode
> bottom and top fields will likely not share the same timestamp, it is a
> mode used to achieve lower latency.
>
> https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/field-order.html#c.v4l2_field
>
> And in this section, you'll see a paragraph that explain the field
> values when running in ALTERNATE mode.
>
> https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/buffer.html#c.v4l2_buffer

Ok, thanks for clarifying. I suppose it makes sense that this mode 
should not
be seen as sequential, since it represents a mode where each userspace 
buffer
is a single, self-contained field. I'll remove ALTERNATE from the helper 
macro.

Steve

Re: [PATCH 4/6] media: imx-csi: Enable interlaced scan for field type alternate

[Philipp Zabel]

On Fri, 2018-05-25 at 16:53 -0700, Steve Longerbeam wrote:
> Interlaced scan, a.k.a. interweave, should be enabled at the CSI IDMAC
> output pad if the input field type is 'alternate' (in addition to field
> types 'seq-tb' and 'seq-bt').
> 
> Which brings up whether V4L2_FIELD_HAS_BOTH() macro should be used
> to determine enabling interlaced/interweave scan. That macro
> includes the 'interlaced' field types, and in those cases the data
> is already interweaved with top/bottom field lines. A heads-up for
> now that this if statement may need to call V4L2_FIELD_IS_SEQUENTIAL()
> instead, I have no sensor hardware that sends 'interlaced' data, so can't
> test.

I agree, the check here should be IS_SEQUENTIAL || ALTERNATE, and
interlaced_scan should also be enabled if image.pix.field is
INTERLACED_TB/BT.
And for INTERLACED_TB/BT input, the logic should be inverted: then we'd
have to enable interlaced_scan whenever image.pix.field is SEQ_BT/TB.

regards
Philipp

> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
>  drivers/staging/media/imx/imx-media-csi.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index 9bc555c..eef3483 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -477,7 +477,8 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
>  	ipu_smfc_set_burstsize(priv->smfc, burst_size);
>  
>  	if (image.pix.field == V4L2_FIELD_NONE &&
> -	    V4L2_FIELD_HAS_BOTH(infmt->field))
> +	    (V4L2_FIELD_HAS_BOTH(infmt->field) ||
> +	     infmt->field == V4L2_FIELD_ALTERNATE))
>  		ipu_cpmem_interlaced_scan(priv->idmac_ch,
>  					  image.pix.bytesperline);
>  

Re: [PATCH 4/6] media: imx-csi: Enable interlaced scan for field type alternate

[Ian Arkver]

On 28/05/18 08:00, Philipp Zabel wrote:
> On Fri, 2018-05-25 at 16:53 -0700, Steve Longerbeam wrote:
>> Interlaced scan, a.k.a. interweave, should be enabled at the CSI IDMAC
>> output pad if the input field type is 'alternate' (in addition to field
>> types 'seq-tb' and 'seq-bt').
>>
>> Which brings up whether V4L2_FIELD_HAS_BOTH() macro should be used
>> to determine enabling interlaced/interweave scan. That macro
>> includes the 'interlaced' field types, and in those cases the data
>> is already interweaved with top/bottom field lines. A heads-up for
>> now that this if statement may need to call V4L2_FIELD_IS_SEQUENTIAL()
>> instead, I have no sensor hardware that sends 'interlaced' data, so can't
>> test.
> 
> I agree, the check here should be IS_SEQUENTIAL || ALTERNATE, and
> interlaced_scan should also be enabled if image.pix.field is
> INTERLACED_TB/BT.
> And for INTERLACED_TB/BT input, the logic should be inverted: then we'd
> have to enable interlaced_scan whenever image.pix.field is SEQ_BT/TB.

Hi Philipp,

If your intent here is to de-interweave the two fields back to two 
sequential fields, I don't believe the IDMAC operation would achieve 
that. It's basically line stride doubling and a line offset for the 
lines in the 2nd half of the incoming frame, right?

Regards,
Ian
> 
> regards
> Philipp
> 
>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>> ---
>>   drivers/staging/media/imx/imx-media-csi.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
>> index 9bc555c..eef3483 100644
>> --- a/drivers/staging/media/imx/imx-media-csi.c
>> +++ b/drivers/staging/media/imx/imx-media-csi.c
>> @@ -477,7 +477,8 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
>>   	ipu_smfc_set_burstsize(priv->smfc, burst_size);
>>   
>>   	if (image.pix.field == V4L2_FIELD_NONE &&
>> -	    V4L2_FIELD_HAS_BOTH(infmt->field))
>> +	    (V4L2_FIELD_HAS_BOTH(infmt->field) ||
>> +	     infmt->field == V4L2_FIELD_ALTERNATE))
>>   		ipu_cpmem_interlaced_scan(priv->idmac_ch,
>>   					  image.pix.bytesperline);
>>   

Re: [PATCH 4/6] media: imx-csi: Enable interlaced scan for field type alternate

[Steve Longerbeam]

On 05/28/2018 12:59 AM, Ian Arkver wrote:
> On 28/05/18 08:00, Philipp Zabel wrote:
>> On Fri, 2018-05-25 at 16:53 -0700, Steve Longerbeam wrote:
>>> Interlaced scan, a.k.a. interweave, should be enabled at the CSI IDMAC
>>> output pad if the input field type is 'alternate' (in addition to field
>>> types 'seq-tb' and 'seq-bt').
>>>
>>> Which brings up whether V4L2_FIELD_HAS_BOTH() macro should be used
>>> to determine enabling interlaced/interweave scan. That macro
>>> includes the 'interlaced' field types, and in those cases the data
>>> is already interweaved with top/bottom field lines. A heads-up for
>>> now that this if statement may need to call V4L2_FIELD_IS_SEQUENTIAL()
>>> instead, I have no sensor hardware that sends 'interlaced' data, so 
>>> can't
>>> test.
>>
>> I agree, the check here should be IS_SEQUENTIAL || ALTERNATE, and
>> interlaced_scan should also be enabled if image.pix.field is
>> INTERLACED_TB/BT.

Yep, in fact I have implemented that in v2. Interlace_scan will be enabled
only if input field type is SEQUENTIAL || ALTERNATE, and output field type
is INTERLACED_TB/BT or non-qualified INTERLACED.


>> And for INTERLACED_TB/BT input, the logic should be inverted: then we'd
>> have to enable interlaced_scan whenever image.pix.field is SEQ_BT/TB.
>
> Hi Philipp,
>
> If your intent here is to de-interweave the two fields back to two 
> sequential fields, I don't believe the IDMAC operation would achieve 
> that. It's basically line stride doubling and a line offset for the 
> lines in the 2nd half of the incoming frame, right?

I agree, I'm not sure interlaced_scan will perform the inverse (turn an 
interlaced
buffer into a sequential buffer). If the implementation involves a scan of
two lines, second line with a memory offset equal to field size, and 
doubling
the line stride to reach the next set of two lines, then running that 
operation
on an interlaced buffer would not produce a sequential buffer.

Steve


>
>>
>>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>>> ---
>>>   drivers/staging/media/imx/imx-media-csi.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/staging/media/imx/imx-media-csi.c 
>>> b/drivers/staging/media/imx/imx-media-csi.c
>>> index 9bc555c..eef3483 100644
>>> --- a/drivers/staging/media/imx/imx-media-csi.c
>>> +++ b/drivers/staging/media/imx/imx-media-csi.c
>>> @@ -477,7 +477,8 @@ static int csi_idmac_setup_channel(struct 
>>> csi_priv *priv)
>>>       ipu_smfc_set_burstsize(priv->smfc, burst_size);
>>>         if (image.pix.field == V4L2_FIELD_NONE &&
>>> -        V4L2_FIELD_HAS_BOTH(infmt->field))
>>> +        (V4L2_FIELD_HAS_BOTH(infmt->field) ||
>>> +         infmt->field == V4L2_FIELD_ALTERNATE))
>>>           ipu_cpmem_interlaced_scan(priv->idmac_ch,
>>>                         image.pix.bytesperline);

Re: [PATCH 4/6] media: imx-csi: Enable interlaced scan for field type alternate

[Philipp Zabel]

On Mon, May 28, 2018 at 09:38:42AM -0700, Steve Longerbeam wrote:
> On 05/28/2018 12:59 AM, Ian Arkver wrote:
> > If your intent here is to de-interweave the two fields back to two
> > sequential fields, I don't believe the IDMAC operation would achieve
> > that. It's basically line stride doubling and a line offset for the
> > lines in the 2nd half of the incoming frame, right?
> 
> I agree, I'm not sure interlaced_scan will perform the inverse (turn an
> interlaced
> buffer into a sequential buffer). If the implementation involves a scan of
> two lines, second line with a memory offset equal to field size, and
> doubling
> the line stride to reach the next set of two lines, then running that
> operation
> on an interlaced buffer would not produce a sequential buffer.

You are both right, of course. What I was thinking of only works
with enabling interlaced_scan on an IDMAC read channel. On an IDMAC
write channel there is no way to turn an interlaced frame back into
consecutive fields.

regards
Philipp