[PATCH v2 00/10] imx-media: Fixes for interlaced capture

[PATCH v2 00/10] imx-media: Fixes for interlaced capture

[Steve Longerbeam]

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

History:
v2:
- update media driver doc.
- enable idmac interweave only if input field is sequential/alternate,
  and output field is 'interlaced*'.
- move field try logic out of *try_fmt and into separate function.
- fix bug with resetting crop/compose rectangles.
- add a patch that fixes a field order bug in VDIC indirect mode.
- remove alternate field type from V4L2_FIELD_IS_SEQUENTIAL() macro
  Suggested-by: Nicolas Dufresne <nicolas@ndufresne.ca>.
- add macro V4L2_FIELD_IS_INTERLACED().
  
Steve Longerbeam (10):
  media: imx-csi: Pass sink pad field to ipu_csi_init_interface
  gpu: ipu-csi: Check for field type alternate
  media: videodev2.h: Add macros V4L2_FIELD_IS_{INTERLACED|SEQUENTIAL}
  media: imx: interweave only for sequential input/interlaced output
    fields
  media: imx: interweave and odd-chroma-row skip are incompatible
  media: imx: Fix field setting logic in try_fmt
  media: imx-csi: Allow skipping odd chroma rows for YVU420
  media: imx: vdic: rely on VDIC for correct field order
  media: imx-csi: Move crop/compose reset after filling default mbus
    fields
  media: imx.rst: Update doc to reflect fixes to interlaced capture

 Documentation/media/v4l-drivers/imx.rst     |  51 +++++++++----
 drivers/gpu/ipu-v3/ipu-csi.c                |   3 +-
 drivers/staging/media/imx/imx-ic-prpencvf.c |  51 +++++++++++--
 drivers/staging/media/imx/imx-media-csi.c   | 114 ++++++++++++++++++----------
 drivers/staging/media/imx/imx-media-vdic.c  |  12 +--
 include/uapi/linux/videodev2.h              |   7 ++
 6 files changed, 165 insertions(+), 73 deletions(-)

-- 
2.7.4

[PATCH v2 01/10] media: imx-csi: Pass sink pad field to ipu_csi_init_interface

[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
is not understood by ipu_csi_init_interface().

Remove that code and pass the sink pad field 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.

Reported-by: Krzysztof Hałasa <khalasa@piap.pl>
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

Re: [PATCH v2 01/10] media: imx-csi: Pass sink pad field to ipu_csi_init_interface

[Philipp Zabel]

On Thu, 2018-05-31 at 17:30 -0700, Steve Longerbeam wrote:
> 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
> is not understood by ipu_csi_init_interface().
> 
> Remove that code and pass the sink pad field 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.

I fear this won't be enough. If we want to capture
sink:ALTERNATE/SEQ_TB/SEQ_BT -> src:SEQ_TB we have to configure the CSI
differently than if we want to capture
ALTERNATE/SEQ_TB/SEQ_BT -> src:SEQ_BT. (And differently for NTSC and
PAL). For NTSC sink:ALTERNATE should behave like sink:SEQ_BT, and for
PAL sink:ALTERNATE should behave like sink:SEQ_TB.

Interweaving SEQ_TB to INTERLACED_TB should work right now, but to
interweave SEQ_BT to INTERLACED_BT, we need to add one line offset to
the frame start and use a negative interlaced scanline offset.

regards
Philipp

> Reported-by: Krzysztof Hałasa <khalasa@piap.pl>
> 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);
>  

Re: [PATCH v2 01/10] media: imx-csi: Pass sink pad field to ipu_csi_init_interface

[Steve Longerbeam]

On 06/01/2018 06:22 AM, Philipp Zabel wrote:
> On Thu, 2018-05-31 at 17:30 -0700, Steve Longerbeam wrote:
>> 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
>> is not understood by ipu_csi_init_interface().
>>
>> Remove that code and pass the sink pad field 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.
> I fear this won't be enough. If we want to capture
> sink:ALTERNATE/SEQ_TB/SEQ_BT -> src:SEQ_TB we have to configure the CSI
> differently than if we want to capture
> ALTERNATE/SEQ_TB/SEQ_BT -> src:SEQ_BT. (And differently for NTSC and
> PAL). For NTSC sink:ALTERNATE should behave like sink:SEQ_BT, and for
> PAL sink:ALTERNATE should behave like sink:SEQ_TB.

I think we should return to enforcing field order to userspace that
matches field order from the source, which is what I had implemented
previously. I agree with you that we should put off allowing inverting
field order.

>
> Interweaving SEQ_TB to INTERLACED_TB should work right now, but to
> interweave SEQ_BT to INTERLACED_BT, we need to add one line offset to
> the frame start and use a negative interlaced scanline offset.

Is that because ipu_csi_init_interface() is inverting the F-bit for
NTSC? I think we should remove that code, I will comment on
that in another thread.

Steve


>
>
>
>> Reported-by: Krzysztof Hałasa <khalasa@piap.pl>
>> 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);
>>   

Re: [PATCH v2 01/10] media: imx-csi: Pass sink pad field to ipu_csi_init_interface

[Krzysztof Hałasa]

Steve Longerbeam <slongerbeam@gmail.com> writes:

> I think we should return to enforcing field order to userspace that
> matches field order from the source, which is what I had implemented
> previously. I agree with you that we should put off allowing inverting
> field order.

There is no any particular field order at the source, most of the time.
The odd field is followed by the even field, and so on, sure. But there
is no "first" and "second" field, any field can be the "first".

The exception to this is a camera with a progressive sensor - both
"fields" are taken at the same time and transmitted one after the other,
so in this case the order is defined (by the camera, e.g. B-T on DV even
with PAL version). But this isn't exactly PAL/NTSC.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: [PATCH v2 01/10] media: imx-csi: Pass sink pad field to ipu_csi_init_interface

[Philipp Zabel]

On Mon, 2018-06-04 at 07:25 +0200, Krzysztof Hałasa wrote:
> Steve Longerbeam <slongerbeam@gmail.com> writes:
> 
> > I think we should return to enforcing field order to userspace that
> > matches field order from the source, which is what I had implemented
> > previously. I agree with you that we should put off allowing inverting
> > field order.
> 
> There is no any particular field order at the source, most of the time.
> The odd field is followed by the even field, and so on, sure. But there
> is no "first" and "second" field, any field can be the "first".

There is no particular field order in the data itself. But for BT.656
there is a specific field order, defined by the F flag in the SAV/EAV
codes. For PAL usually F=0 is the top field and F=1 is the bottom field.
For NTSC it usually is the other way around.

> The exception to this is a camera with a progressive sensor - both
> "fields" are taken at the same time and transmitted one after the other,
> so in this case the order is defined (by the camera, e.g. B-T on DV even
> with PAL version). But this isn't exactly PAL/NTSC.

That's why I'd like to make it obvious to the user when the field order
is switched. Whoever selects seq-bt -> seq-tb or seq-tb -> seq-bt
transformation for progressive sources can expect combing artifacts.

regards
Philipp

Re: [PATCH v2 01/10] media: imx-csi: Pass sink pad field to ipu_csi_init_interface

[Steve Longerbeam]

On 06/03/2018 10:25 PM, Krzysztof Hałasa wrote:
> Steve Longerbeam <slongerbeam@gmail.com> writes:
>
>> I think we should return to enforcing field order to userspace that
>> matches field order from the source, which is what I had implemented
>> previously. I agree with you that we should put off allowing inverting
>> field order.
> There is no any particular field order at the source, most of the time.
> The odd field is followed by the even field, and so on, sure. But there
> is no "first" and "second" field, any field can be the "first".

I think you misunderstood me. Of course there is a first and second
field. By first I am referring to the first field transmitted, which could
be top or bottom.

> The exception to this is a camera with a progressive sensor - both
> "fields" are taken at the same time and transmitted one after the other,
> so in this case the order is defined (by the camera, e.g. B-T on DV even
> with PAL version). But this isn't exactly PAL/NTSC.

Progressive sensors have no fields, the entire image is captured at
once as you said.

Steve

Re: [PATCH v2 01/10] media: imx-csi: Pass sink pad field to ipu_csi_init_interface

[Krzysztof Hałasa]

Steve Longerbeam <steve_longerbeam@mentor.com> writes:

> I think you misunderstood me. Of course there is a first and second
> field. By first I am referring to the first field transmitted, which could
> be top or bottom.

Right. I was thinking the fields are even and odd, but that's not
actually the case (I mean, the numbering uses field 1 and 2 and not
E/O).

> Progressive sensors have no fields, the entire image is captured at
> once as you said.

There are progressive cameras with analog PAL/NTSC output. The signal
is obviously interlaced and consists of fields.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

[PATCH v2 02/10] 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

Re: [PATCH v2 02/10] gpu: ipu-csi: Check for field type alternate

[Philipp Zabel]

Hi Steve,

On Thu, 2018-05-31 at 17:30 -0700, Steve Longerbeam wrote:
> 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;

Thank you, applied to imx-drm/next.

regards
Philipp

[PATCH v2 03/10] media: videodev2.h: Add macros V4L2_FIELD_IS_{INTERLACED|SEQUENTIAL}

[Steve Longerbeam]

Adds two helper macros:

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

Add a macro that returns true if the given field type is 'interlaced',
that is a full frame is transmitted, or exists in memory, as top field
lines interlaced with bottom field lines.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
Changes since v1:
- add the complement macro V4L2_FIELD_IS_INTERLACED
- remove V4L2_FIELD_ALTERNATE from V4L2_FIELD_IS_SEQUENTIAL macro.
- moved new macros past end of existing V4L2_FIELD_HAS_* macros.
---
 include/uapi/linux/videodev2.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 600877b..d4e12f4 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -130,6 +130,13 @@ enum v4l2_field {
 	((field) == V4L2_FIELD_BOTTOM ||\
 	 (field) == V4L2_FIELD_TOP ||\
 	 (field) == V4L2_FIELD_ALTERNATE)
+#define V4L2_FIELD_IS_INTERLACED(field) \
+	((field) == V4L2_FIELD_INTERLACED ||\
+	 (field) == V4L2_FIELD_INTERLACED_TB ||\
+	 (field) == V4L2_FIELD_INTERLACED_BT)
+#define V4L2_FIELD_IS_SEQUENTIAL(field) \
+	((field) == V4L2_FIELD_SEQ_TB ||\
+	 (field) == V4L2_FIELD_SEQ_BT)
 
 enum v4l2_buf_type {
 	V4L2_BUF_TYPE_VIDEO_CAPTURE        = 1,
-- 
2.7.4

[PATCH v2 04/10] media: imx: interweave only for sequential input/interlaced output fields

[Steve Longerbeam]

IDMAC interlaced scan, a.k.a. interweave, should be enabled at the
IDMAC output pads only if the input field type is 'seq-bt' or 'seq-tb',
and the IDMAC output pad field type is 'interlaced*'. Move this
determination to a new macro idmac_interweave().

V4L2_FIELD_HAS_BOTH() macro should not be used on the input 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.

The CSI will capture whole frames when the source specifies alternate
field mode. So the CSI also enables interweave at the IDMAC output pad
for alternate input field type.

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

diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
index ae453fd..894db21 100644
--- a/drivers/staging/media/imx/imx-ic-prpencvf.c
+++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
@@ -132,6 +132,18 @@ static inline struct prp_priv *sd_to_priv(struct v4l2_subdev *sd)
 	return ic_priv->task_priv;
 }
 
+/*
+ * If the field type at IDMAC output pad is interlaced, and
+ * the input is sequential fields, the IDMAC output channel
+ * can accommodate by interweaving.
+ */
+static inline bool idmac_interweave(enum v4l2_field outfield,
+				    enum v4l2_field infield)
+{
+	return V4L2_FIELD_IS_INTERLACED(outfield) &&
+		V4L2_FIELD_IS_SEQUENTIAL(infield);
+}
+
 static void prp_put_ipu_resources(struct prp_priv *priv)
 {
 	if (priv->ic)
@@ -353,6 +365,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 +378,9 @@ static int prp_setup_channel(struct prp_priv *priv,
 	image.rect.width = image.pix.width;
 	image.rect.height = image.pix.height;
 
+	interweave = (idmac_interweave(image.pix.field, 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);
@@ -405,9 +421,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,
@@ -833,7 +847,7 @@ static void prp_try_fmt(struct prp_priv *priv,
 	infmt = __prp_get_fmt(priv, cfg, PRPENCVF_SINK_PAD, sdformat->which);
 
 	if (sdformat->pad == PRPENCVF_SRC_PAD) {
-		if (sdformat->format.field != V4L2_FIELD_NONE)
+		if (!V4L2_FIELD_IS_INTERLACED(sdformat->format.field))
 			sdformat->format.field = infmt->field;
 
 		prp_bound_align_output(&sdformat->format, infmt,
diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index 9bc555c..2c77ef9 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -128,6 +128,19 @@ static inline bool is_parallel_16bit_bus(struct v4l2_fwnode_endpoint *ep)
 }
 
 /*
+ * If the field type at IDMAC output pad is interlaced, and
+ * the input is sequential or alternating fields, the IDMAC
+ * output channel can accommodate by interweaving.
+ */
+static inline bool idmac_interweave(enum v4l2_field outfield,
+				    enum v4l2_field infield)
+{
+	return V4L2_FIELD_IS_INTERLACED(outfield) &&
+		(V4L2_FIELD_IS_SEQUENTIAL(infield) ||
+		 infield == V4L2_FIELD_ALTERNATE);
+}
+
+/*
  * Parses the fwnode endpoint from the source pad of the entity
  * connected to this CSI. This will either be the entity directly
  * upstream from the CSI-2 receiver, or directly upstream from the
@@ -368,10 +381,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 +402,8 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
 	image.phys0 = phys[0];
 	image.phys1 = phys[1];
 
+	interweave = idmac_interweave(image.pix.field, infmt->field);
+
 	/*
 	 * Check for conditions that require the IPU to handle the
 	 * data internally as generic data, aka passthrough mode:
@@ -476,8 +491,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))
+	if (interweave)
 		ipu_cpmem_interlaced_scan(priv->idmac_ch,
 					  image.pix.bytesperline);
 
@@ -1294,7 +1308,7 @@ static void csi_try_fmt(struct csi_priv *priv,
 		}
 
 		if (sdformat->pad == CSI_SRC_PAD_DIRECT ||
-		    sdformat->format.field != V4L2_FIELD_NONE)
+		    !V4L2_FIELD_IS_INTERLACED(sdformat->format.field))
 			sdformat->format.field = infmt->field;
 
 		/*
-- 
2.7.4

Re: [PATCH v2 04/10] media: imx: interweave only for sequential input/interlaced output fields

[Philipp Zabel]

Hi Steve,

On Thu, 2018-05-31 at 17:30 -0700, Steve Longerbeam wrote:
> IDMAC interlaced scan, a.k.a. interweave, should be enabled at the
> IDMAC output pads only if the input field type is 'seq-bt' or 'seq-tb',
> and the IDMAC output pad field type is 'interlaced*'. Move this
> determination to a new macro idmac_interweave().
> 
> V4L2_FIELD_HAS_BOTH() macro should not be used on the input 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.
> 
> The CSI will capture whole frames when the source specifies alternate
> field mode. So the CSI also enables interweave at the IDMAC output pad
> for alternate input field type.
> 
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
>  drivers/staging/media/imx/imx-ic-prpencvf.c | 22 ++++++++++++++++++----
>  drivers/staging/media/imx/imx-media-csi.c   | 22 ++++++++++++++++++----
>  2 files changed, 36 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
> index ae453fd..894db21 100644
> --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
> +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
> @@ -132,6 +132,18 @@ static inline struct prp_priv *sd_to_priv(struct v4l2_subdev *sd)
>  	return ic_priv->task_priv;
>  }
>  
> +/*
> + * If the field type at IDMAC output pad is interlaced, and
> + * the input is sequential fields, the IDMAC output channel
> + * can accommodate by interweaving.
> + */
> +static inline bool idmac_interweave(enum v4l2_field outfield,
> +				    enum v4l2_field infield)
> +{
> +	return V4L2_FIELD_IS_INTERLACED(outfield) &&
> +		V4L2_FIELD_IS_SEQUENTIAL(infield);
> +}

This is ok in this patch, but we can't use this check in the following
TRY_FMT patch as there is no way to interweave
SEQ_TB -> INTERLACED_BT (because in SEQ_TB the B field is newer than T,
but in INTERLACED_BT it has to be older) or SEQ_BT -> INTERLACED_TB (the
other way around).

> +
>  static void prp_put_ipu_resources(struct prp_priv *priv)
>  {
>  	if (priv->ic)
> @@ -353,6 +365,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 +378,9 @@ static int prp_setup_channel(struct prp_priv *priv,
>  	image.rect.width = image.pix.width;
>  	image.rect.height = image.pix.height;
>  
> +	interweave = (idmac_interweave(image.pix.field, 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);
> @@ -405,9 +421,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);

This only works for SEQ_TB -> INTERLACED_TB interweave.
For SEQ_BT -> INTERLACED_BT we have to start at +1 line offset and set
ipu_cpmem_interlaced_scan to -image.pix.bytesperline.

regards
Philipp

Re: [PATCH v2 04/10] media: imx: interweave only for sequential input/interlaced output fields

[Steve Longerbeam]

On 06/01/2018 06:33 AM, Philipp Zabel wrote:
> Hi Steve,
>
> On Thu, 2018-05-31 at 17:30 -0700, Steve Longerbeam wrote:
>> IDMAC interlaced scan, a.k.a. interweave, should be enabled at the
>> IDMAC output pads only if the input field type is 'seq-bt' or 'seq-tb',
>> and the IDMAC output pad field type is 'interlaced*'. Move this
>> determination to a new macro idmac_interweave().
>>
>> V4L2_FIELD_HAS_BOTH() macro should not be used on the input 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.
>>
>> The CSI will capture whole frames when the source specifies alternate
>> field mode. So the CSI also enables interweave at the IDMAC output pad
>> for alternate input field type.
>>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>> ---
>>   drivers/staging/media/imx/imx-ic-prpencvf.c | 22 ++++++++++++++++++----
>>   drivers/staging/media/imx/imx-media-csi.c   | 22 ++++++++++++++++++----
>>   2 files changed, 36 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
>> index ae453fd..894db21 100644
>> --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
>> +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
>> @@ -132,6 +132,18 @@ static inline struct prp_priv *sd_to_priv(struct v4l2_subdev *sd)
>>   	return ic_priv->task_priv;
>>   }
>>   
>> +/*
>> + * If the field type at IDMAC output pad is interlaced, and
>> + * the input is sequential fields, the IDMAC output channel
>> + * can accommodate by interweaving.
>> + */
>> +static inline bool idmac_interweave(enum v4l2_field outfield,
>> +				    enum v4l2_field infield)
>> +{
>> +	return V4L2_FIELD_IS_INTERLACED(outfield) &&
>> +		V4L2_FIELD_IS_SEQUENTIAL(infield);
>> +}
> This is ok in this patch, but we can't use this check in the following
> TRY_FMT patch as there is no way to interweave
> SEQ_TB -> INTERLACED_BT (because in SEQ_TB the B field is newer than T,
> but in INTERLACED_BT it has to be older) or SEQ_BT -> INTERLACED_TB (the
> other way around).

Agreed, let's enforce userspace field order to same order from
the originating source.

Steve


>
>> +
>>   static void prp_put_ipu_resources(struct prp_priv *priv)
>>   {
>>   	if (priv->ic)
>> @@ -353,6 +365,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 +378,9 @@ static int prp_setup_channel(struct prp_priv *priv,
>>   	image.rect.width = image.pix.width;
>>   	image.rect.height = image.pix.height;
>>   
>> +	interweave = (idmac_interweave(image.pix.field, 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);
>> @@ -405,9 +421,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);
> This only works for SEQ_TB -> INTERLACED_TB interweave.
> For SEQ_BT -> INTERLACED_BT we have to start at +1 line offset and set
> ipu_cpmem_interlaced_scan to -image.pix.bytesperline.
>
> regards
> Philipp

Re: [PATCH v2 04/10] media: imx: interweave only for sequential input/interlaced output fields

[Krzysztof Hałasa]

Philipp Zabel <p.zabel@pengutronix.de> writes:

> This is ok in this patch, but we can't use this check in the following
> TRY_FMT patch as there is no way to interweave
> SEQ_TB -> INTERLACED_BT (because in SEQ_TB the B field is newer than T,
> but in INTERLACED_BT it has to be older) or SEQ_BT -> INTERLACED_TB (the
> other way around).

Actually we can do SEQ_TB -> INTERLACED_BT and SEQ_BT -> INTERLACED_TB
rather easily. We only need to skip a single field at start :-)
That's what CCIR_CODE_* registers do.

To be honest, SEQ_TB and SEQ_BT are precisely the same thing
(i.e., SEQUENTIAL). It's up to the user to say which field is the first.
There is the progressive sensor exception, though, and the TB/BT could
be a hint for downstream elements (i.e., setting the default field
order).

But I think we should be able to request INTERLACED_TB or INTERLACED_BT
(with any analog signal on input) and the CCIR_CODE registers should be
set accordingly. This should all magically work fine.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: [PATCH v2 04/10] media: imx: interweave only for sequential input/interlaced output fields

[Philipp Zabel]

On Mon, 2018-06-04 at 07:35 +0200, Krzysztof Hałasa wrote:
> Philipp Zabel <p.zabel@pengutronix.de> writes:
> 
> > This is ok in this patch, but we can't use this check in the following
> > TRY_FMT patch as there is no way to interweave
> > SEQ_TB -> INTERLACED_BT (because in SEQ_TB the B field is newer than T,
> > but in INTERLACED_BT it has to be older) or SEQ_BT -> INTERLACED_TB (the
> > other way around).
> 
> Actually we can do SEQ_TB -> INTERLACED_BT and SEQ_BT -> INTERLACED_TB
> rather easily. We only need to skip a single field at start :-)
> That's what CCIR_CODE_* registers do.
> 
> To be honest, SEQ_TB and SEQ_BT are precisely the same thing
> (i.e., SEQUENTIAL). It's up to the user to say which field is the first.
> There is the progressive sensor exception, though, and the TB/BT could
> be a hint for downstream elements (i.e., setting the default field
> order).
> 
> But I think we should be able to request INTERLACED_TB or INTERLACED_BT
> (with any analog signal on input) and the CCIR_CODE registers should be
> set accordingly. This should all magically work fine.

The CSI subdevice itself can't interweave at all, this is done in the
IDMAC.
In my opinion the CSI subdev should allow the following src -> sink
field transformations for BT.656:

none -> none
seq-tb -> seq-tb
seq-tb -> seq-bt
seq-bt -> seq-bt
seq-bt -> seq-tb
alternate -> seq-tb
alternate -> seq-bt
interlaced -> interlaced
interlaced-tb -> interlaced-tb
interlaced-bt -> interlaced-bt

The capture video device should then additionally allow selecting
the field order that can be produced by IDMAC interweaving:
INTERLACED_TB if the pad is seq-tb and INTERLACED_BT if the pad is seq-
bt, as that is what the IDMAC can convert.

seq-tb -> seq-tb and seq-bt -> seq-bt should always capture field 0
first, as we currently do for PAL.
seq->tb -> seq-bt and seq-bt -> seq-tb should always capture field 1
first, as we currently do for NTSC.
alternate -> seq-tb and alternate -> seq-bt should match seq-tb -> * for
PAL and seq-bt -> * for NTSC.
The interlaced* -> interlaced* would be handled as progressive.

regards
Philipp

Re: [PATCH v2 04/10] media: imx: interweave only for sequential input/interlaced output fields

[Steve Longerbeam]

On 06/04/2018 01:27 AM, Philipp Zabel wrote:
> On Mon, 2018-06-04 at 07:35 +0200, Krzysztof Hałasa wrote:
>> Philipp Zabel <p.zabel@pengutronix.de> writes:
>>
>>> This is ok in this patch, but we can't use this check in the following
>>> TRY_FMT patch as there is no way to interweave
>>> SEQ_TB -> INTERLACED_BT (because in SEQ_TB the B field is newer than T,
>>> but in INTERLACED_BT it has to be older) or SEQ_BT -> INTERLACED_TB (the
>>> other way around).
>> Actually we can do SEQ_TB -> INTERLACED_BT and SEQ_BT -> INTERLACED_TB
>> rather easily. We only need to skip a single field at start :-)
>> That's what CCIR_CODE_* registers do.
>>
>> To be honest, SEQ_TB and SEQ_BT are precisely the same thing
>> (i.e., SEQUENTIAL). It's up to the user to say which field is the first.
>> There is the progressive sensor exception, though, and the TB/BT could
>> be a hint for downstream elements (i.e., setting the default field
>> order).
>>
>> But I think we should be able to request INTERLACED_TB or INTERLACED_BT
>> (with any analog signal on input) and the CCIR_CODE registers should be
>> set accordingly. This should all magically work fine.
> The CSI subdevice itself can't interweave at all, this is done in the
> IDMAC.
> In my opinion the CSI subdev should allow the following src -> sink
> field transformations for BT.656:
>
> none -> none
> seq-tb -> seq-tb
> seq-tb -> seq-bt
> seq-bt -> seq-bt
> seq-bt -> seq-tb
> alternate -> seq-tb
> alternate -> seq-bt
> interlaced -> interlaced
> interlaced-tb -> interlaced-tb
> interlaced-bt -> interlaced-bt
>
> The capture video device should then additionally allow selecting
> the field order that can be produced by IDMAC interweaving:
> INTERLACED_TB if the pad is seq-tb and INTERLACED_BT if the pad is seq-
> bt, as that is what the IDMAC can convert.

Good idea. This is also in-line with how planar YUV is selected
at the capture interface instead of at the CSI/PRPENCVF source
pad.

Philipp, Krzysztof, please see branch fix-csi-interlaced.3 in my github
mediatree fork. I've implemented the above and it works great for
both NTSC and PAL sources to the ADV7180.

>
> seq-tb -> seq-tb and seq-bt -> seq-bt should always capture field 0
> first, as we currently do for PAL.
> seq->tb -> seq-bt and seq-bt -> seq-tb should always capture field 1
> first, as we currently do for NTSC.
> alternate -> seq-tb and alternate -> seq-bt should match seq-tb -> * for
> PAL and seq-bt -> * for NTSC.

Yes, I had already implemented this idea yesterday, I've added it
to branch fix-csi-interlaced.3. The CSI will swap field capture
(field 1 first, then field 2, by inverting F bit in CCIR registers) if
the field order input to the CSI is different from the requested
output field order.

Philipp, a word about the idea of using negative ILO line stride and
an extra line added to EBA start address, for interweaving. I believe
the result of this is to also invert field order when interweaving
'seq-bt/tb', which would produce 'interlaced-tb/bt' in memory.

I don't think this is necessary now, because field order swapping
can already be done earlier at the CSI sink->src using the CCIR registers.
For example here is a pipeline for an NTSC adv7180 source that swapped
NTSC 'seq-bt' (well assumed NTSC 'seq-bt' since adv7180 is 'alternate') to
'seq-tb' at the CSI source pad:

'adv7180 3-0021':0
         [fmt:UYVY8_2X8/720x480 field:alternate colorspace:smpte170m]
'ipu1_csi0_mux':1
         [fmt:UYVY8_2X8/720x480 field:alternate colorspace:smpte170m]
'ipu1_csi0_mux':2
         [fmt:UYVY8_2X8/720x480 field:alternate colorspace:smpte170m]
'ipu1_csi0':0
         [fmt:UYVY8_2X8/720x480@1/30 field:alternate ...]
          crop.bounds:(0,0)/720x480
          crop:(0,2)/720x480
          compose.bounds:(0,0)/720x480
          compose:(0,0)/720x480]
'ipu1_csi0':2
         [fmt:AYUV8_1X32/720x480@1/30 field:seq-tb ...]

And at the capture interface:

# v4l2-ctl -d4 -V
Format Video Capture:
     Width/Height      : 720/480
     Pixel Format      : 'YV12'
     Field             : Interlaced Top-Bottom
     Bytes per Line    : 1440
     Size Image        : 691200
     Colorspace        : SMPTE 170M
     Transfer Function : Rec. 709
     YCbCr/HSV Encoding: ITU-R 601
     Quantization      : Limited Range
     Flags             :

So we've accomplished 'seq-bt' -> 'interlaced-tb' without needing
to swap field order using the modified interweave idea.

I've run tests for both PAL and NTSC inputs to the adv7180 on SabreAuto,
and the results are consistent:

NTSC seq-bt -> interlaced-tb produces good interweave images as expected
NTSC seq-bt -> interlaced-bt produces interweave images with a "mauve" 
artifact as expected
PAL seq-tb -> interlaced-tb produces good interweave images as expected
PAL seq-tb -> interlaced-bt produces interweave images with a "mauve" 
artifact as expected

Steve

Re: [PATCH v2 04/10] media: imx: interweave only for sequential input/interlaced output fields

[Philipp Zabel]

Hi Steve,

On Mon, 2018-06-04 at 17:56 -0700, Steve Longerbeam wrote:
> 
> On 06/04/2018 01:27 AM, Philipp Zabel wrote:
> > On Mon, 2018-06-04 at 07:35 +0200, Krzysztof Hałasa wrote:
> > > Philipp Zabel <p.zabel@pengutronix.de> writes:
> > > 
> > > > This is ok in this patch, but we can't use this check in the following
> > > > TRY_FMT patch as there is no way to interweave
> > > > SEQ_TB -> INTERLACED_BT (because in SEQ_TB the B field is newer than T,
> > > > but in INTERLACED_BT it has to be older) or SEQ_BT -> INTERLACED_TB (the
> > > > other way around).
> > > 
> > > Actually we can do SEQ_TB -> INTERLACED_BT and SEQ_BT -> INTERLACED_TB
> > > rather easily. We only need to skip a single field at start :-)
> > > That's what CCIR_CODE_* registers do.
> > > 
> > > To be honest, SEQ_TB and SEQ_BT are precisely the same thing
> > > (i.e., SEQUENTIAL). It's up to the user to say which field is the first.
> > > There is the progressive sensor exception, though, and the TB/BT could
> > > be a hint for downstream elements (i.e., setting the default field
> > > order).
> > > 
> > > But I think we should be able to request INTERLACED_TB or INTERLACED_BT
> > > (with any analog signal on input) and the CCIR_CODE registers should be
> > > set accordingly. This should all magically work fine.
> > 
> > The CSI subdevice itself can't interweave at all, this is done in the
> > IDMAC.
> > In my opinion the CSI subdev should allow the following src -> sink
> > field transformations for BT.656:
> > 
> > none -> none
> > seq-tb -> seq-tb
> > seq-tb -> seq-bt
> > seq-bt -> seq-bt
> > seq-bt -> seq-tb
> > alternate -> seq-tb
> > alternate -> seq-bt
> > interlaced -> interlaced
> > interlaced-tb -> interlaced-tb
> > interlaced-bt -> interlaced-bt
> > 
> > The capture video device should then additionally allow selecting
> > the field order that can be produced by IDMAC interweaving:
> > INTERLACED_TB if the pad is seq-tb and INTERLACED_BT if the pad is seq-
> > bt, as that is what the IDMAC can convert.
> 
> Good idea. This is also in-line with how planar YUV is selected
> at the capture interface instead of at the CSI/PRPENCVF source
> pad.
> 
> Philipp, Krzysztof, please see branch fix-csi-interlaced.3 in my github
> mediatree fork. I've implemented the above and it works great for
> both NTSC and PAL sources to the ADV7180.

Thanks! I'll have a look.

> > seq-tb -> seq-tb and seq-bt -> seq-bt should always capture field 0
> > first, as we currently do for PAL.
> > seq->tb -> seq-bt and seq-bt -> seq-tb should always capture field 1
> > first, as we currently do for NTSC.
> > alternate -> seq-tb and alternate -> seq-bt should match seq-tb -> * for
> > PAL and seq-bt -> * for NTSC.
> 
> Yes, I had already implemented this idea yesterday, I've added it
> to branch fix-csi-interlaced.3. The CSI will swap field capture
> (field 1 first, then field 2, by inverting F bit in CCIR registers) if
> the field order input to the CSI is different from the requested
> output field order.
> 
> Philipp, a word about the idea of using negative ILO line stride and
> an extra line added to EBA start address, for interweaving. I believe
> the result of this is to also invert field order when interweaving
> 'seq-bt/tb', which would produce 'interlaced-tb/bt' in memory.

I'm probably misunderstanding you, so at the risk of overexplaining:
There is no way we can ever produce a correct interlaced-tb frame in
memory from a seq-bt frame output by the CSI, as the interweaving step
only has access to a single frame.

A seq-tb PAL frame has the older top field in lines 0-287 and the newer
bottom field in lines 288-576. From that interlaced-tb can be written
via 0-287 -> 0,2,4,...,286 and 288-575 -> 1,3,5,...,287 [1]. This is
what interweaving does if the interlace offset is set to positive line
stride.

A seq-bt NTSC frame has the older bottom field in lines 0-239 and the
newer top field in lines 240-439. We can create an interlaced-bt frame
from that by writing 0-239 -> 1,3,5,...,239 and 240-439 -> 0,2,4,...,238
[2]. This can be achieved by offsetting EBA by +stride and setting ILO
to -stride.

[1] https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/field-order.html?highlight=field#field-order-top-field-first-transmitted
[2] https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/field-order.html?highlight=field#field-order-bottom-field-first-transmitted

Writing a seq-tb frame with negative ILO or a seq-bt frame with positive
ILO causes misaligned interlaced frames with even and odd lines
switched.

> I don't think this is necessary now, because field order swapping
> can already be done earlier at the CSI sink->src using the CCIR registers.
> For example here is a pipeline for an NTSC adv7180 source that swapped
> NTSC 'seq-bt' (well assumed NTSC 'seq-bt' since adv7180 is 'alternate') to
> 'seq-tb' at the CSI source pad:
> 
> 'adv7180 3-0021':0
>          [fmt:UYVY8_2X8/720x480 field:alternate colorspace:smpte170m]
> 'ipu1_csi0_mux':1
>          [fmt:UYVY8_2X8/720x480 field:alternate colorspace:smpte170m]
> 'ipu1_csi0_mux':2
>          [fmt:UYVY8_2X8/720x480 field:alternate colorspace:smpte170m]
> 'ipu1_csi0':0
>          [fmt:UYVY8_2X8/720x480@1/30 field:alternate ...]

Yes, and due to 480 height the CSI would behave as if field:seq-bt was
set.

>           crop.bounds:(0,0)/720x480
>           crop:(0,2)/720x480
>           compose.bounds:(0,0)/720x480
>           compose:(0,0)/720x480]
> 'ipu1_csi0':2
>          [fmt:AYUV8_1X32/720x480@1/30 field:seq-tb ...]

So this causes CSI to sample field 1 first, and then field 0 of the next
frame.

> 
> And at the capture interface:
> 
> # v4l2-ctl -d4 -V
> Format Video Capture:
>      Width/Height      : 720/480
>      Pixel Format      : 'YV12'
>      Field             : Interlaced Top-Bottom
>      Bytes per Line    : 1440
>      Size Image        : 691200
>      Colorspace        : SMPTE 170M
>      Transfer Function : Rec. 709
>      YCbCr/HSV Encoding: ITU-R 601
>      Quantization      : Limited Range
>      Flags             :
> 
> So we've accomplished 'seq-bt' -> 'interlaced-tb' without needing
> to swap field order using the modified interweave idea.

I don't follow. For NTSC this setting looks exactly like it should swap
field order in the CSI by first capturing field F=1 (top) and then field
F=0 of the next frame (bottom) into a single frame.

> I've run tests for both PAL and NTSC inputs to the adv7180 on SabreAuto,
> and the results are consistent:
> 
> NTSC seq-bt -> interlaced-tb produces good interweave images as expected

So that is seq-bt or alternate on the CSI sink pad, seq-tb on the CSI
src pad and interlaced-tb in the video capture device?

> NTSC seq-bt -> interlaced-bt produces interweave images with a "mauve" 
> artifact as expected

We can fix this to produce perfect results with seq-bt or alternate on
the CSI sink pad, seq-bt on the src pad, and interlaced-bt in the video
capture device by implementing the negative ILO idea.

> PAL seq-tb -> interlaced-tb produces good interweave images as expected
> PAL seq-tb -> interlaced-bt produces interweave images with a "mauve" 
> artifact as expected

Same as above, with negative ILO we should be able to do field switching
in the CSI, setting the sink pad to alternate or seq-tb, the src pad to
seq-bt, and create proper interlaced-bt from that in the IDMAC.

regards
Philipp

Re: [PATCH v2 04/10] media: imx: interweave only for sequential input/interlaced output fields

[Krzysztof Hałasa]

Philipp Zabel <p.zabel@pengutronix.de> writes:

> I'm probably misunderstanding you, so at the risk of overexplaining:
> There is no way we can ever produce a correct interlaced-tb frame in
> memory from a seq-bt frame output by the CSI, as the interweaving step
> only has access to a single frame.

Anyway we can use CCIR_CODE registers to get the fields in required
order. Actually I think it's the preferred way, even if there are
others.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: [PATCH v2 04/10] media: imx: interweave only for sequential input/interlaced output fields

[Steve Longerbeam]

Hi Philipp,


On 06/05/2018 01:07 AM, Philipp Zabel wrote:
> Hi Steve,
>
> On Mon, 2018-06-04 at 17:56 -0700, Steve Longerbeam wrote:
>> On 06/04/2018 01:27 AM, Philipp Zabel wrote:
>>> On Mon, 2018-06-04 at 07:35 +0200, Krzysztof Hałasa wrote:
>>>> Philipp Zabel <p.zabel@pengutronix.de> writes:
>>>>
>>>>> This is ok in this patch, but we can't use this check in the following
>>>>> TRY_FMT patch as there is no way to interweave
>>>>> SEQ_TB -> INTERLACED_BT (because in SEQ_TB the B field is newer than T,
>>>>> but in INTERLACED_BT it has to be older) or SEQ_BT -> INTERLACED_TB (the
>>>>> other way around).
>>>> Actually we can do SEQ_TB -> INTERLACED_BT and SEQ_BT -> INTERLACED_TB
>>>> rather easily. We only need to skip a single field at start :-)
>>>> That's what CCIR_CODE_* registers do.
>>>>
>>>> To be honest, SEQ_TB and SEQ_BT are precisely the same thing
>>>> (i.e., SEQUENTIAL). It's up to the user to say which field is the first.
>>>> There is the progressive sensor exception, though, and the TB/BT could
>>>> be a hint for downstream elements (i.e., setting the default field
>>>> order).
>>>>
>>>> But I think we should be able to request INTERLACED_TB or INTERLACED_BT
>>>> (with any analog signal on input) and the CCIR_CODE registers should be
>>>> set accordingly. This should all magically work fine.
>>> The CSI subdevice itself can't interweave at all, this is done in the
>>> IDMAC.
>>> In my opinion the CSI subdev should allow the following src -> sink
>>> field transformations for BT.656:
>>>
>>> none -> none
>>> seq-tb -> seq-tb
>>> seq-tb -> seq-bt
>>> seq-bt -> seq-bt
>>> seq-bt -> seq-tb
>>> alternate -> seq-tb
>>> alternate -> seq-bt
>>> interlaced -> interlaced
>>> interlaced-tb -> interlaced-tb
>>> interlaced-bt -> interlaced-bt
>>>
>>> The capture video device should then additionally allow selecting
>>> the field order that can be produced by IDMAC interweaving:
>>> INTERLACED_TB if the pad is seq-tb and INTERLACED_BT if the pad is seq-
>>> bt, as that is what the IDMAC can convert.
>> Good idea. This is also in-line with how planar YUV is selected
>> at the capture interface instead of at the CSI/PRPENCVF source
>> pad.
>>
>> Philipp, Krzysztof, please see branch fix-csi-interlaced.3 in my github
>> mediatree fork. I've implemented the above and it works great for
>> both NTSC and PAL sources to the ADV7180.
> Thanks! I'll have a look.
>
>>> seq-tb -> seq-tb and seq-bt -> seq-bt should always capture field 0
>>> first, as we currently do for PAL.
>>> seq->tb -> seq-bt and seq-bt -> seq-tb should always capture field 1
>>> first, as we currently do for NTSC.
>>> alternate -> seq-tb and alternate -> seq-bt should match seq-tb -> * for
>>> PAL and seq-bt -> * for NTSC.
>> Yes, I had already implemented this idea yesterday, I've added it
>> to branch fix-csi-interlaced.3. The CSI will swap field capture
>> (field 1 first, then field 2, by inverting F bit in CCIR registers) if
>> the field order input to the CSI is different from the requested
>> output field order.
>>
>> Philipp, a word about the idea of using negative ILO line stride and
>> an extra line added to EBA start address, for interweaving. I believe
>> the result of this is to also invert field order when interweaving
>> 'seq-bt/tb', which would produce 'interlaced-tb/bt' in memory.
> I'm probably misunderstanding you, so at the risk of overexplaining:
> There is no way we can ever produce a correct interlaced-tb frame in
> memory from a seq-bt frame output by the CSI, as the interweaving step
> only has access to a single frame.

I don't follow you, yes the interweaving step only has access to
a single frame, but why would interweave need access to another
frame to carry out seq-bt -> interlaced-tb ? See below...
>
> A seq-tb PAL frame has the older top field in lines 0-287 and the newer
> bottom field in lines 288-576. From that interlaced-tb can be written
> via 0-287 -> 0,2,4,...,286 and 288-575 -> 1,3,5,...,287 [1]. This is
> what interweaving does if the interlace offset is set to positive line
> stride.

Right, that was my understanding as well. And how interweave
actually works in the IDMAC to achieve the above is :

By turning on SO bit in cpmem, the IDMAC will write the first one-half
lines of the frame received by the IDMAC channel to memory, starting
at the EBA address, with a line stride equal to cpmem SLY. When it
completes writing out the first half lines of the frame, the IDMAC begins
to write the lines from the second half of frame to memory, but starts
again at EBA address, and with an offset equal to cpmem ILO.

So by setting SO=1, SLY=2*linestride, and ILO=linestride, we achieve
interweave where:

lines from first half of frame are written to lines 0,2,4,...,HEIGHT-2 
in memory, and
lines from the second half of the frame are written to lines 
1,3,...,HEIGHT-1 in memory.

So this setting achieves seq-bt -> interlaced-bt or seq-tb -> 
interlaced-tb, e.g.
interweave *does not change field order*, bottom lines 1,3,,, are written to
0,2,4,,, in memory if IDMAC receives seq-bt, or top lines 0,2,4,,, are 
written to
0,2,4,,, in memory if IDMAC receives seq-tb.

And by setting SO=1, SLY=2*linestride, ILO=-linestride, and adding one
linestride to EBA:

lines from first half of the frame are written to lines 1,3,...,HEIGHT-1 
in memory, and
lines from the second half of the frame are written to lines 
0,2,...,HEIGHT-2 in memory.

So this achieves  seq-bt -> interlaced-tb or seq-tb -> interlaced-bt, 
e.g. field order
has been swapped in memory.


> A seq-bt NTSC frame has the older bottom field in lines 0-239 and the
> newer top field in lines 240-439. We can create an interlaced-bt frame
> from that by writing 0-239 -> 1,3,5,...,239 and 240-439 -> 0,2,4,...,238
> [2]. This can be achieved by offsetting EBA by +stride and setting ILO
> to -stride.

Agreed except that this is seq-bt -> interlaced-tb, not
seq-bt -> interlaced-bt:

Bottom lines = 1,3,5,...,H-1
Top lines = 0,2,4,...,H-2

So seq-bt is written as interlaced-tb to memory:

Bottom lines = 1,3,5,...,H-1 go to memory lines 1,3,5,...H-1
Top lines = 0,2,4,...,H-2 go to memory lines 0,2,4,...,H-2.

So this is TB order in memory.

>
> [1] https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/field-order.html?highlight=field#field-order-top-field-first-transmitted
> [2] https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/field-order.html?highlight=field#field-order-bottom-field-first-transmitted
>
> Writing a seq-tb frame with negative ILO or a seq-bt frame with positive
> ILO causes misaligned interlaced frames with even and odd lines
> switched.
>
>> I don't think this is necessary now, because field order swapping
>> can already be done earlier at the CSI sink->src using the CCIR registers.
>> For example here is a pipeline for an NTSC adv7180 source that swapped
>> NTSC 'seq-bt' (well assumed NTSC 'seq-bt' since adv7180 is 'alternate') to
>> 'seq-tb' at the CSI source pad:
>>
>> 'adv7180 3-0021':0
>>           [fmt:UYVY8_2X8/720x480 field:alternate colorspace:smpte170m]
>> 'ipu1_csi0_mux':1
>>           [fmt:UYVY8_2X8/720x480 field:alternate colorspace:smpte170m]
>> 'ipu1_csi0_mux':2
>>           [fmt:UYVY8_2X8/720x480 field:alternate colorspace:smpte170m]
>> 'ipu1_csi0':0
>>           [fmt:UYVY8_2X8/720x480@1/30 field:alternate ...]
> Yes, and due to 480 height the CSI would behave as if field:seq-bt was
> set.

Right. So input to CSI is (assumed to be) seq-bt.

>
>>            crop.bounds:(0,0)/720x480
>>            crop:(0,2)/720x480
>>            compose.bounds:(0,0)/720x480
>>            compose:(0,0)/720x480]
>> 'ipu1_csi0':2
>>           [fmt:AYUV8_1X32/720x480@1/30 field:seq-tb ...]
> So this causes CSI to sample field 1 first, and then field 0 of the next
> frame.

Right. So output from CSI is seq-tb.


>
>> And at the capture interface:
>>
>> # v4l2-ctl -d4 -V
>> Format Video Capture:
>>       Width/Height      : 720/480
>>       Pixel Format      : 'YV12'
>>       Field             : Interlaced Top-Bottom
>>       Bytes per Line    : 1440
>>       Size Image        : 691200
>>       Colorspace        : SMPTE 170M
>>       Transfer Function : Rec. 709
>>       YCbCr/HSV Encoding: ITU-R 601
>>       Quantization      : Limited Range
>>       Flags             :
>>
>> So we've accomplished 'seq-bt' -> 'interlaced-tb' without needing
>> to swap field order using the modified interweave idea.
> I don't follow. For NTSC this setting looks exactly like it should swap
> field order in the CSI by first capturing field F=1 (top) and then field
> F=0 of the next frame (bottom) into a single frame.

Right! CSI swaps seq-bt at its sink pad to seq-tb at its source pad.
>
>> I've run tests for both PAL and NTSC inputs to the adv7180 on SabreAuto,
>> and the results are consistent:
>>
>> NTSC seq-bt -> interlaced-tb produces good interweave images as expected
> So that is seq-bt or alternate on the CSI sink pad, seq-tb on the CSI
> src pad and interlaced-tb in the video capture device?

Correct! The final outcome is seq-bt from the originating source,
to interlaced-tb in memory.


>
>> NTSC seq-bt -> interlaced-bt produces interweave images with a "mauve"
>> artifact as expected
> We can fix this to produce perfect results with seq-bt or alternate on
> the CSI sink pad, seq-bt on the src pad, and interlaced-bt in the video
> capture device by implementing the negative ILO idea.

Well sure, we could use the negative ILO idea to swap field order
(for a second time) at the CSI src pad to capture interface, e.g.
seq-bt at CSI sink -> seq-tb at CSI src -> interlaced-bt at capture
interface.

But I don't see the need for doing that. If the user wishes to swap
field order, it can do that at the CSI source pad. Why swap field
order for a second time? But sure we could provide that ability.

>
>> PAL seq-tb -> interlaced-tb produces good interweave images as expected
>> PAL seq-tb -> interlaced-bt produces interweave images with a "mauve"
>> artifact as expected
> Same as above, with negative ILO we should be able to do field switching
> in the CSI, setting the sink pad to alternate or seq-tb, the src pad to
> seq-bt, and create proper interlaced-bt from that in the IDMAC.

seq-bt at src pad to interlaced-bt in memory doesn't require negative
ILO as I explained above.

Steve

Re: [PATCH v2 04/10] media: imx: interweave only for sequential input/interlaced output fields

[Krzysztof Hałasa]

Steve Longerbeam <steve_longerbeam@mentor.com> writes:

> I don't follow you, yes the interweaving step only has access to
> a single frame, but why would interweave need access to another
> frame to carry out seq-bt -> interlaced-tb ? See below...

You can't to that.
You can delay the input stream (skip one field) so the bottom-first
becomes top-first (or top-first - bottom-first), probably with some loss
of chroma quality, but you can't reorder odd and even lines.

To convert (anything)-bt -> (anything)-tb you need two consecutive
fields, the top one and then the bottom one. If the input is *-bt, this
means two "frames" (if the word "frame" is applicable at this point).

CCIR_CODE_* registers are fine, though. They don't change the geometry,
the just skip a single field (sort of, actually they sync to the
required field).
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: [PATCH v2 04/10] media: imx: interweave only for sequential input/interlaced output fields

[Philipp Zabel]

Hi Steve,

On Tue, 2018-06-05 at 12:00 -0700, Steve Longerbeam wrote:
> > I'm probably misunderstanding you, so at the risk of overexplaining:
> > There is no way we can ever produce a correct interlaced-tb frame in
> > memory from a seq-bt frame output by the CSI, as the interweaving step
> > only has access to a single frame.
> 
> I don't follow you, yes the interweaving step only has access to
> a single frame, but why would interweave need access to another
> frame to carry out seq-bt -> interlaced-tb ? See below...

A seq-bt frame has a bottom field (first in memory) with an older
timestamp than the top field (second in memory). Without access to a
second input frame we can only ever produce an interlaced frame where
the bottom lines are older than the top lines, which is interlaced-bt.

interlaced-tb requires the top lines to have the older timestamp, and
the bottom lines to be newer.

> > A seq-tb PAL frame has the older top field in lines 0-287 and the newer
> > bottom field in lines 288-576. From that interlaced-tb can be written
> > via 0-287 -> 0,2,4,...,286 and 288-575 -> 1,3,5,...,287 [1]. This is
> > what interweaving does if the interlace offset is set to positive line
> > stride.
> 
> Right, that was my understanding as well. And how interweave
> actually works in the IDMAC to achieve the above is :
> 
> By turning on SO bit in cpmem, the IDMAC will write the first one-half
> lines of the frame received by the IDMAC channel to memory, starting
> at the EBA address, with a line stride equal to cpmem SLY. When it
> completes writing out the first half lines of the frame, the IDMAC begins
> to write the lines from the second half of frame to memory, but starts
> again at EBA address, and with an offset equal to cpmem ILO.
> 
> So by setting SO=1, SLY=2*linestride, and ILO=linestride, we achieve
> interweave where:
> 
> lines from first half of frame are written to lines 0,2,4,...,HEIGHT-2 
> in memory, and
> lines from the second half of the frame are written to lines 
> 1,3,...,HEIGHT-1 in memory.
> 
> So this setting achieves seq-bt -> interlaced-bt

This is incorrect. Since the bottom field comes first in memory, with
this setting the bottom lines are written to where the top lines should
be and the top lines end up where the bottom lines should be.

Since odd and even lines are switched, this will not produce a correct
frame.

>  or seq-tb -> interlaced-tb,

This is correct.

>  e.g.
> interweave *does not change field order*, bottom lines 1,3,,, are written to
> 0,2,4,,, in memory if IDMAC receives seq-bt, or top lines 0,2,4,,, are 
> written to
> 0,2,4,,, in memory if IDMAC receives seq-tb.

Of course we can't change field order, we are arguing the same point
here. I think our misunderstanding comes from the definition of
interlaced-bt [1]:

  "Images contain both fields, interleaved line by line, top field
   first. The bottom field is transmitted first."

[1] https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/field-order.html?highlight=field#enum-v4l2-field

The BT or TB part of the v4l2_field enums refer to the order in time,
not to the order in memory.

> And by setting SO=1, SLY=2*linestride, ILO=-linestride, and adding one
> linestride to EBA:
> 
> lines from first half of the frame are written to lines 1,3,...,HEIGHT-1 
> in memory, and
> lines from the second half of the frame are written to lines 
> 0,2,...,HEIGHT-2 in memory.
> 
> So this achieves  seq-bt -> interlaced-tb

No this is seq-bt -> interlaced-bt

>  or seq-tb -> interlaced-bt, 

and this again produces an incorrect frame.

> e.g. field order has been swapped in memory.
> 
> 
> > A seq-bt NTSC frame has the older bottom field in lines 0-239 and the
> > newer top field in lines 240-439. We can create an interlaced-bt frame
> > from that by writing 0-239 -> 1,3,5,...,239 and 240-439 -> 0,2,4,...,238
> > [2]. This can be achieved by offsetting EBA by +stride and setting ILO
> > to -stride.
> 
> Agreed except that this is seq-bt -> interlaced-tb, not
> seq-bt -> interlaced-bt:

No, see above.

> Bottom lines = 1,3,5,...,H-1
> Top lines = 0,2,4,...,H-2

Yes, but the bottom lines are older, so this is interlaced-bt.

> So seq-bt is written as interlaced-tb to memory:
> 
> Bottom lines = 1,3,5,...,H-1 go to memory lines 1,3,5,...H-1
> Top lines = 0,2,4,...,H-2 go to memory lines 0,2,4,...,H-2.
> 
> So this is TB order in memory.

All V4L2 interlaced variants are stored in TB order, which allows to
just display them as a progressive frame. The V4L2_FIELD_INTERLACED
description is not clear on that, but the V4L2_FIELD_INTERLACED_TB/BT
descriptions explicitly state TB order in memory.

So that is seq-bt or alternate on the CSI sink pad, seq-tb on the CSI

> Well sure, we could use the negative ILO idea to swap field order
> (for a second time) at the CSI src pad to capture interface, e.g.
> seq-bt at CSI sink -> seq-tb at CSI src -> interlaced-bt at capture
> interface.

Yes, we swap field order (in memory), as that's how we get a correct
frame. Bot none of this can switch field order in time, BT is always
bottom first, top later, and TB is always top first, bottom later.

> But I don't see the need for doing that. If the user wishes to swap
> field order, it can do that at the CSI source pad. Why swap field
> order for a second time? But sure we could provide that ability.

But that swaps field order in time (in the sense that first we have
bottom-top order in time and then we capture fields from to consecutive
frames such that we get top-bottom order in time), which is different.

> > > PAL seq-tb -> interlaced-tb produces good interweave images as expected
> > > PAL seq-tb -> interlaced-bt produces interweave images with a "mauve"
> > > artifact as expected
> > 
> > Same as above, with negative ILO we should be able to do field switching
> > in the CSI, setting the sink pad to alternate or seq-tb, the src pad to
> > seq-bt, and create proper interlaced-bt from that in the IDMAC.
> 
> seq-bt at src pad to interlaced-bt in memory doesn't require negative
> ILO as I explained above.

This is not correct. Writing seq-bt with SO=1, ILO=stride, SLY=2*stride
causes an incorrect frame with switched odd and even lines.

regards
Philipp

Re: [PATCH v2 04/10] media: imx: interweave only for sequential input/interlaced output fields

[Steve Longerbeam]

Hi Philipp,

I think now I understand your interpretation of  the v4l2_field
enums:

SEQ_BT/TB is specifying both field order and field dominance.

But the TB/BT in INTERLACED_TB/BT has a different interpretation,
in this case it is specifying _only_ field dominance, and top field is
first in memory for both cases.

I wasn't interpreting it this way. My interpretation was that BT/TB
for all field types referred only to field order and does not say
anything about field dominance.

So in summary:

SEQ_BT = bottom field first in memory AND bottom field is dominant
(B lines are older lines in time, e.g. recorded first, then T lines).

SEQ_TB = top field first in memory AND top field is dominant.

INTERLACED_BT = top field first in memory and bottom field is dominant.

INTERLACED_TB = top field first in memory and top field is dominant.

With that, your explanations below make sense...

On 06/06/2018 02:05 AM, Philipp Zabel wrote:
> Hi Steve,
>
> On Tue, 2018-06-05 at 12:00 -0700, Steve Longerbeam wrote:
>>> I'm probably misunderstanding you, so at the risk of overexplaining:
>>> There is no way we can ever produce a correct interlaced-tb frame in
>>> memory from a seq-bt frame output by the CSI, as the interweaving step
>>> only has access to a single frame.
>> I don't follow you, yes the interweaving step only has access to
>> a single frame, but why would interweave need access to another
>> frame to carry out seq-bt -> interlaced-tb ? See below...
> A seq-bt frame has a bottom field (first in memory) with an older
> timestamp than the top field (second in memory). Without access to a
> second input frame we can only ever produce an interlaced frame where
> the bottom lines are older than the top lines, which is interlaced-bt.
>
> interlaced-tb requires the top lines to have the older timestamp, and
> the bottom lines to be newer.

Right, in seq-bt the CSI can skip bottom field and capture top field, then
bottom field of next frame, which results in flipping field dominance.
So seq-bt becomes seq-tb and indeed the top field in seq-tb is now
the older (dominant) field, and the bottom field is now newer.

Which means we need to span two frames to accomplish
seq-bt -> seq-tb in order to flip field dominance.

>
>>> A seq-tb PAL frame has the older top field in lines 0-287 and the newer
>>> bottom field in lines 288-576. From that interlaced-tb can be written
>>> via 0-287 -> 0,2,4,...,286 and 288-575 -> 1,3,5,...,287 [1]. This is
>>> what interweaving does if the interlace offset is set to positive line
>>> stride.
>> Right, that was my understanding as well. And how interweave
>> actually works in the IDMAC to achieve the above is :
>>
>> By turning on SO bit in cpmem, the IDMAC will write the first one-half
>> lines of the frame received by the IDMAC channel to memory, starting
>> at the EBA address, with a line stride equal to cpmem SLY. When it
>> completes writing out the first half lines of the frame, the IDMAC begins
>> to write the lines from the second half of frame to memory, but starts
>> again at EBA address, and with an offset equal to cpmem ILO.
>>
>> So by setting SO=1, SLY=2*linestride, and ILO=linestride, we achieve
>> interweave where:
>>
>> lines from first half of frame are written to lines 0,2,4,...,HEIGHT-2
>> in memory, and
>> lines from the second half of the frame are written to lines
>> 1,3,...,HEIGHT-1 in memory.
>>
>> So this setting achieves seq-bt -> interlaced-bt
> This is incorrect. Since the bottom field comes first in memory, with
> this setting the bottom lines are written to where the top lines should
> be and the top lines end up where the bottom lines should be.
>
> Since odd and even lines are switched, this will not produce a correct
> frame.

Yes, if 'interlaced-bt' is interpreted as: bottom field is dominant
and top field must be first in memory.

>>   or seq-tb -> interlaced-tb,
> This is correct.

Yes, a simple interweave accomplishes this, and since
odd/even lines are not switched, interlaced-tb would look
correct if displayed. And top is still dominant.


>
>>   e.g.
>> interweave *does not change field order*, bottom lines 1,3,,, are written to
>> 0,2,4,,, in memory if IDMAC receives seq-bt, or top lines 0,2,4,,, are
>> written to
>> 0,2,4,,, in memory if IDMAC receives seq-tb.
> Of course we can't change field order, we are arguing the same point
> here. I think our misunderstanding comes from the definition of
> interlaced-bt [1]:
>
>    "Images contain both fields, interleaved line by line, top field
>     first. The bottom field is transmitted first."
>
> [1] https://linuxtv.org/downloads/v4l-dvb-apis-new/uapi/v4l/field-order.html?highlight=field#enum-v4l2-field
>
> The BT or TB part of the v4l2_field enums refer to the order in time,
> not to the order in memory.

Yes BT/TB refers to field dominance for both 'seq-*' and
'interlaced-*'. But TB/BT only refers to field order for the
'seq-*' definitions, in 'interlaced-*' top field is fixed as first
field in memory.

But I'm still not so sure because of the sentence "The bottom/top
field is transmitted first" in interlaced-bt/tb. Is that referring to
field dominance or is it referring just to what it says, that
bottom/top field is transmitted first but may not be the older
field?

>> And by setting SO=1, SLY=2*linestride, ILO=-linestride, and adding one
>> linestride to EBA:
>>
>> lines from first half of the frame are written to lines 1,3,...,HEIGHT-1
>> in memory, and
>> lines from the second half of the frame are written to lines
>> 0,2,...,HEIGHT-2 in memory.
>>
>> So this achieves  seq-bt -> interlaced-tb
> No this is seq-bt -> interlaced-bt

Understood now with your interpretation.


>
>> So seq-bt is written as interlaced-tb to memory:
>>
>> Bottom lines = 1,3,5,...,H-1 go to memory lines 1,3,5,...H-1
>> Top lines = 0,2,4,...,H-2 go to memory lines 0,2,4,...,H-2.
>>
>> So this is TB order in memory.
> All V4L2 interlaced variants are stored in TB order, which allows to
> just display them as a progressive frame. The V4L2_FIELD_INTERLACED
> description is not clear on that, but the V4L2_FIELD_INTERLACED_TB/BT
> descriptions explicitly state TB order in memory.

Yes, but as I said above it's not so clear for me in terms of
field dominance (what does "The bottom/top field is transmitted
first" mean?).

>
> So that is seq-bt or alternate on the CSI sink pad, seq-tb on the CSI
>
>> Well sure, we could use the negative ILO idea to swap field order
>> (for a second time) at the CSI src pad to capture interface, e.g.
>> seq-bt at CSI sink -> seq-tb at CSI src -> interlaced-bt at capture
>> interface.
> Yes, we swap field order (in memory), as that's how we get a correct
> frame. Bot none of this can switch field order in time, BT is always
> bottom first, top later, and TB is always top first, bottom later.
>
>> But I don't see the need for doing that. If the user wishes to swap
>> field order, it can do that at the CSI source pad. Why swap field
>> order for a second time? But sure we could provide that ability.
> But that swaps field order in time (in the sense that first we have
> bottom-top order in time and then we capture fields from to consecutive
> frames such that we get top-bottom order in time), which is different.

Yep, again this was due to my interpretation that TB/BT referred
only to field order for all field types.

One final note, it is incorrect to assign 'seq-tb' to a PAL signal according
to this new understanding. Because according to various sites (for example
[1]), both standard definition NTSC and PAL are bottom field dominant,
so 'seq-tb' is not correct for PAL.

[1] 
https://larryjordan.com/articles/figuring-out-video-field-order-frames-and-interlacing/

Steve

Re: [PATCH v2 04/10] media: imx: interweave only for sequential input/interlaced output fields

[Krzysztof Hałasa]

Steve Longerbeam <steve_longerbeam@mentor.com> writes:

> One final note, it is incorrect to assign 'seq-tb' to a PAL signal according
> to this new understanding. Because according to various sites (for example
> [1]), both standard definition NTSC and PAL are bottom field dominant,
> so 'seq-tb' is not correct for PAL.

Actually this isn't the case:

- real PAL (= analog) is (was) interlaced, so you could choose any
  "dominant field" and it would work fine (stuff originating as film
  movies being an exception).

- the general idea at these times was that NTSC-style digital video was
  bottom-first while PAL-style was top-first.

- for example, most (practically all?) commercial TV-style interlaced
  PAL DVDs were top-first (movies were usually progressive).

- standard TV (DVB 576i) uses (used) top-first as well.

- this seems to be confirmed by e.g. ITU-R REC-BR.469-7-2002 (annex 1).
  Hovewer, this suggests that field 1 is the top one and 2 is bottom
  (and not first and second in terms of time).

- if field 1 = top and 2 = bottom indeed, then we're back at BT.656 and
  my original idea that the SAV/EAV codes show straigh the so called
  odd/even lines and not some magic, standard-dependent lines of first
  and second fields. This needs to be verified.
  I think the ADV7180 forces the SAV/EAV codes (one can't set them
  depending od PAL/NTSC etc), so we could assume it does it right.

- a notable exception to PAL = top first rule was DV and similar stuff
  (the casette camcorder things, including Digital8, miniDV, and
  probably derivatives). DV (including PAL) used bottom-first
  universally.

I think we should stick to default PAL=TB, NTSC=BT rule, though the user
should be able to override it (if working with e.g. PAL DV camcorder
connected with an analog cable - not very probable, I guess).
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: [PATCH v2 04/10] media: imx: interweave only for sequential input/interlaced output fields

[Krzysztof Hałasa]

Steve Longerbeam <steve_longerbeam@mentor.com> writes:

> Yes, I had already implemented this idea yesterday, I've added it
> to branch fix-csi-interlaced.3. The CSI will swap field capture
> (field 1 first, then field 2, by inverting F bit in CCIR registers) if
> the field order input to the CSI is different from the requested
> output field order.

It seems the fix-csi-interlaced.2 was a bit better.

Now I do:
media-ctl -V "'adv7180 2-0020':0 [fmt:UYVY2X8/720x576 field:alternate]"
media-ctl -V "'ipu2_csi1_mux':2 [fmt:UYVY2X8/720x576]"
media-ctl -V "'ipu2_csi1':2 [fmt:AYUV32/720x576 field:interlaced]"

and get:
"adv7180 2-0020":0 [fmt:UYVY2X8/720x576 field:alternate]
"ipu2_csi1_mux":1  [fmt:UYVY2X8/720x576 field:alternate]
"ipu2_csi1_mux":2  [fmt:UYVY2X8/720x576 field:alternate]
"ipu2_csi1":0      [fmt:UYVY2X8/720x576 field:alternate]
"ipu2_csi1":2      [fmt:AYUV32/720x576 field:seq-tb]

Needless to say, the output isn't an interlaced frame.
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

[PATCH v2 05/10] media: 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.

Reported-by: Krzysztof Hałasa <khalasa@piap.pl>
Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 drivers/staging/media/imx/imx-ic-prpencvf.c | 9 +++++++--
 drivers/staging/media/imx/imx-media-csi.c   | 8 ++++++--
 2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
index 894db21..7e1e0c3 100644
--- a/drivers/staging/media/imx/imx-ic-prpencvf.c
+++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
@@ -393,12 +393,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;
 		}
diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index 2c77ef9..6a2a47c 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -436,8 +436,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:
-- 
2.7.4

[PATCH v2 06/10] media: imx: Fix field setting logic in try_fmt

[Steve Longerbeam]

The logic for setting field type in try_fmt at CSI and PRPENCVF
entities wasn't quite right. The behavior should be:

- No restrictions on field type at sink pads (except ANY, which is filled
  with current sink pad field by imx_media_fill_default_mbus_fields()).

- At IDMAC output pads, if the caller asks for an interlaced output, and
  the input is sequential fields, the IDMAC output channel can accommodate
  by interweaving. The CSI can also interweave if input is alternate
  fields.

- If final source pad field type is alternate, translate to seq_bt or
  seq_tb. But the field order translation was backwards, SD NTSC is BT
  order, SD PAL is TB.

Move this logic to new functions csi_try_field() and prp_try_field().

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

diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
index 7e1e0c3..1002eb1 100644
--- a/drivers/staging/media/imx/imx-ic-prpencvf.c
+++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
@@ -833,6 +833,21 @@ static int prp_get_fmt(struct v4l2_subdev *sd,
 	return ret;
 }
 
+static void prp_try_field(struct prp_priv *priv,
+			  struct v4l2_subdev_pad_config *cfg,
+			  struct v4l2_subdev_format *sdformat)
+{
+	struct v4l2_mbus_framefmt *infmt =
+		__prp_get_fmt(priv, cfg, PRPENCVF_SINK_PAD, sdformat->which);
+
+	/* no restrictions on sink pad field type */
+	if (sdformat->pad == PRPENCVF_SINK_PAD)
+		return;
+
+	if (!idmac_interweave(sdformat->format.field, infmt->field))
+		sdformat->format.field = infmt->field;
+}
+
 static void prp_try_fmt(struct prp_priv *priv,
 			struct v4l2_subdev_pad_config *cfg,
 			struct v4l2_subdev_format *sdformat,
@@ -852,12 +867,11 @@ static void prp_try_fmt(struct prp_priv *priv,
 	infmt = __prp_get_fmt(priv, cfg, PRPENCVF_SINK_PAD, sdformat->which);
 
 	if (sdformat->pad == PRPENCVF_SRC_PAD) {
-		if (!V4L2_FIELD_IS_INTERLACED(sdformat->format.field))
-			sdformat->format.field = infmt->field;
-
 		prp_bound_align_output(&sdformat->format, infmt,
 				       priv->rot_mode);
 
+		prp_try_field(priv, cfg, sdformat);
+
 		/* propagate colorimetry from sink */
 		sdformat->format.colorspace = infmt->colorspace;
 		sdformat->format.xfer_func = infmt->xfer_func;
@@ -870,6 +884,8 @@ static void prp_try_fmt(struct prp_priv *priv,
 				      MIN_H_SINK, MAX_H_SINK, H_ALIGN_SINK,
 				      S_ALIGN);
 
+		prp_try_field(priv, cfg, sdformat);
+
 		imx_media_fill_default_mbus_fields(&sdformat->format, infmt,
 						   true);
 	}
diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index 6a2a47c..6101e2ed 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -1272,6 +1272,38 @@ static int csi_get_fmt(struct v4l2_subdev *sd,
 	return ret;
 }
 
+static void csi_try_field(struct csi_priv *priv,
+			  struct v4l2_subdev_pad_config *cfg,
+			  struct v4l2_subdev_format *sdformat)
+{
+	struct v4l2_mbus_framefmt *infmt =
+		__csi_get_fmt(priv, cfg, CSI_SINK_PAD, sdformat->which);
+
+	switch (sdformat->pad) {
+	case CSI_SRC_PAD_DIRECT:
+		sdformat->format.field = infmt->field;
+		break;
+	case CSI_SRC_PAD_IDMAC:
+		if (!idmac_interweave(sdformat->format.field, infmt->field))
+			sdformat->format.field = infmt->field;
+		break;
+	case CSI_SINK_PAD:
+		/* no restrictions on sink pad field type */
+		return;
+	}
+
+	/*
+	 * This driver does not support alternate field mode, and the
+	 * CSI captures a whole frame, so translate ALTERNATE at either
+	 * source pad to SEQ_TB or SEQ_BT depending on input height
+	 * (assume NTSC bt order if 480 lines, otherwise PAL tb order).
+	 */
+	if (sdformat->format.field == V4L2_FIELD_ALTERNATE) {
+		sdformat->format.field = (infmt->height == 480) ?
+			V4L2_FIELD_SEQ_BT : V4L2_FIELD_SEQ_TB;
+	}
+}
+
 static void csi_try_fmt(struct csi_priv *priv,
 			struct v4l2_fwnode_endpoint *upstream_ep,
 			struct v4l2_subdev_pad_config *cfg,
@@ -1311,25 +1343,14 @@ static void csi_try_fmt(struct csi_priv *priv,
 			}
 		}
 
-		if (sdformat->pad == CSI_SRC_PAD_DIRECT ||
-		    !V4L2_FIELD_IS_INTERLACED(sdformat->format.field))
-			sdformat->format.field = infmt->field;
-
-		/*
-		 * translate V4L2_FIELD_ALTERNATE to SEQ_TB or SEQ_BT
-		 * depending on input height (assume NTSC top-bottom
-		 * order if 480 lines, otherwise PAL bottom-top order).
-		 */
-		if (sdformat->format.field == V4L2_FIELD_ALTERNATE) {
-			sdformat->format.field =  (infmt->height == 480) ?
-				V4L2_FIELD_SEQ_TB : V4L2_FIELD_SEQ_BT;
-		}
+		csi_try_field(priv, cfg, sdformat);
 
 		/* propagate colorimetry from sink */
 		sdformat->format.colorspace = infmt->colorspace;
 		sdformat->format.xfer_func = infmt->xfer_func;
 		sdformat->format.quantization = infmt->quantization;
 		sdformat->format.ycbcr_enc = infmt->ycbcr_enc;
+
 		break;
 	case CSI_SINK_PAD:
 		v4l_bound_align_image(&sdformat->format.width, MIN_W, MAX_W,
@@ -1357,9 +1378,12 @@ static void csi_try_fmt(struct csi_priv *priv,
 			sdformat->format.code = (*cc)->codes[0];
 		}
 
+		csi_try_field(priv, cfg, sdformat);
+
 		imx_media_fill_default_mbus_fields(
 			&sdformat->format, infmt,
 			priv->active_output_pad == CSI_SRC_PAD_DIRECT);
+
 		break;
 	}
 }
-- 
2.7.4

Re: [PATCH v2 06/10] media: imx: Fix field setting logic in try_fmt

[Philipp Zabel]

On Thu, 2018-05-31 at 17:30 -0700, Steve Longerbeam wrote:
> The logic for setting field type in try_fmt at CSI and PRPENCVF
> entities wasn't quite right. The behavior should be:
> 
> - No restrictions on field type at sink pads (except ANY, which is filled
>   with current sink pad field by imx_media_fill_default_mbus_fields()).
> 
> - At IDMAC output pads, if the caller asks for an interlaced output, and
>   the input is sequential fields, the IDMAC output channel can accommodate
>   by interweaving. The CSI can also interweave if input is alternate
>   fields.
> 
> - If final source pad field type is alternate, translate to seq_bt or
>   seq_tb. But the field order translation was backwards, SD NTSC is BT
>   order, SD PAL is TB.
> 
> Move this logic to new functions csi_try_field() and prp_try_field().
> 
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
>  drivers/staging/media/imx/imx-ic-prpencvf.c | 22 +++++++++++--
>  drivers/staging/media/imx/imx-media-csi.c   | 50 +++++++++++++++++++++--------
>  2 files changed, 56 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
> index 7e1e0c3..1002eb1 100644
> --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
> +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
> @@ -833,6 +833,21 @@ static int prp_get_fmt(struct v4l2_subdev *sd,
>  	return ret;
>  }
>  
> +static void prp_try_field(struct prp_priv *priv,
> +			  struct v4l2_subdev_pad_config *cfg,
> +			  struct v4l2_subdev_format *sdformat)
> +{
> +	struct v4l2_mbus_framefmt *infmt =
> +		__prp_get_fmt(priv, cfg, PRPENCVF_SINK_PAD, sdformat->which);
> +
> +	/* no restrictions on sink pad field type */
> +	if (sdformat->pad == PRPENCVF_SINK_PAD)
> +		return;
> +
> +	if (!idmac_interweave(sdformat->format.field, infmt->field))
> +		sdformat->format.field = infmt->field;

This is not strict enough. As I wrote in reply to patch 4, we can only
do SEQ_TB -> INTERLACED_TB and SEQ_BT -> INTERLACED_BT interweaving.

regards
Philipp

Re: [PATCH v2 06/10] media: imx: Fix field setting logic in try_fmt

[Steve Longerbeam]

On 06/01/2018 06:34 AM, Philipp Zabel wrote:
> On Thu, 2018-05-31 at 17:30 -0700, Steve Longerbeam wrote:
>> The logic for setting field type in try_fmt at CSI and PRPENCVF
>> entities wasn't quite right. The behavior should be:
>>
>> - No restrictions on field type at sink pads (except ANY, which is filled
>>    with current sink pad field by imx_media_fill_default_mbus_fields()).
>>
>> - At IDMAC output pads, if the caller asks for an interlaced output, and
>>    the input is sequential fields, the IDMAC output channel can accommodate
>>    by interweaving. The CSI can also interweave if input is alternate
>>    fields.
>>
>> - If final source pad field type is alternate, translate to seq_bt or
>>    seq_tb. But the field order translation was backwards, SD NTSC is BT
>>    order, SD PAL is TB.
>>
>> Move this logic to new functions csi_try_field() and prp_try_field().
>>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>> ---
>>   drivers/staging/media/imx/imx-ic-prpencvf.c | 22 +++++++++++--
>>   drivers/staging/media/imx/imx-media-csi.c   | 50 +++++++++++++++++++++--------
>>   2 files changed, 56 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
>> index 7e1e0c3..1002eb1 100644
>> --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
>> +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
>> @@ -833,6 +833,21 @@ static int prp_get_fmt(struct v4l2_subdev *sd,
>>   	return ret;
>>   }
>>   
>> +static void prp_try_field(struct prp_priv *priv,
>> +			  struct v4l2_subdev_pad_config *cfg,
>> +			  struct v4l2_subdev_format *sdformat)
>> +{
>> +	struct v4l2_mbus_framefmt *infmt =
>> +		__prp_get_fmt(priv, cfg, PRPENCVF_SINK_PAD, sdformat->which);
>> +
>> +	/* no restrictions on sink pad field type */
>> +	if (sdformat->pad == PRPENCVF_SINK_PAD)
>> +		return;
>> +
>> +	if (!idmac_interweave(sdformat->format.field, infmt->field))
>> +		sdformat->format.field = infmt->field;
> This is not strict enough. As I wrote in reply to patch 4, we can only
> do SEQ_TB -> INTERLACED_TB and SEQ_BT -> INTERLACED_BT interweaving.

Agreed.

Steve

[PATCH v2 07/10] 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 6101e2ed..c878a00 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -430,6 +430,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

Re: [PATCH v2 07/10] media: imx-csi: Allow skipping odd chroma rows for YVU420

[Philipp Zabel]

On Thu, 2018-05-31 at 17:30 -0700, Steve Longerbeam wrote:
> 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 6101e2ed..c878a00 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -430,6 +430,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) ?

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

[PATCH v2 08/10] media: imx: vdic: rely on VDIC for correct field order

[Steve Longerbeam]

prepare_vdi_in_buffers() was setting up the dma pointers as if the
VDIC is always programmed to receive the fields in bottom-top order,
i.e. as if ipu_vdi_set_field_order() only programs BT order in the VDIC.
But that's not true, ipu_vdi_set_field_order() is working correctly.

So fix prepare_vdi_in_buffers() to give the VDIC the fields in whatever
order they were received by the video source, and rely on the VDIC to
sort out which is top and which is bottom.

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

diff --git a/drivers/staging/media/imx/imx-media-vdic.c b/drivers/staging/media/imx/imx-media-vdic.c
index 482250d..4a89071 100644
--- a/drivers/staging/media/imx/imx-media-vdic.c
+++ b/drivers/staging/media/imx/imx-media-vdic.c
@@ -219,26 +219,18 @@ static void __maybe_unused prepare_vdi_in_buffers(struct vdic_priv *priv,
 
 	switch (priv->fieldtype) {
 	case V4L2_FIELD_SEQ_TB:
-		prev_phys = vb2_dma_contig_plane_dma_addr(prev_vb, 0);
-		curr_phys = vb2_dma_contig_plane_dma_addr(curr_vb, 0) + fs;
-		next_phys = vb2_dma_contig_plane_dma_addr(curr_vb, 0);
-		break;
 	case V4L2_FIELD_SEQ_BT:
 		prev_phys = vb2_dma_contig_plane_dma_addr(prev_vb, 0) + fs;
 		curr_phys = vb2_dma_contig_plane_dma_addr(curr_vb, 0);
 		next_phys = vb2_dma_contig_plane_dma_addr(curr_vb, 0) + fs;
 		break;
+	case V4L2_FIELD_INTERLACED_TB:
 	case V4L2_FIELD_INTERLACED_BT:
+	case V4L2_FIELD_INTERLACED:
 		prev_phys = vb2_dma_contig_plane_dma_addr(prev_vb, 0) + is;
 		curr_phys = vb2_dma_contig_plane_dma_addr(curr_vb, 0);
 		next_phys = vb2_dma_contig_plane_dma_addr(curr_vb, 0) + is;
 		break;
-	default:
-		/* assume V4L2_FIELD_INTERLACED_TB */
-		prev_phys = vb2_dma_contig_plane_dma_addr(prev_vb, 0);
-		curr_phys = vb2_dma_contig_plane_dma_addr(curr_vb, 0) + is;
-		next_phys = vb2_dma_contig_plane_dma_addr(curr_vb, 0);
-		break;
 	}
 
 	ipu_cpmem_set_buffer(priv->vdi_in_ch_p, 0, prev_phys);
-- 
2.7.4

[PATCH v2 09/10] media: imx-csi: Move crop/compose reset after filling default mbus fields

[Steve Longerbeam]

If caller passes un-initialized field type V4L2_FIELD_ANY to CSI
sink pad, the reset CSI crop window would not be correct, because
the crop window depends on the current input field type. To fix move
the reset of crop and compose windows to after the call to
imx_media_fill_default_mbus_fields().

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

diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index c878a00..471130a 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -1358,17 +1358,6 @@ static void csi_try_fmt(struct csi_priv *priv,
 				      W_ALIGN, &sdformat->format.height,
 				      MIN_H, MAX_H, H_ALIGN, S_ALIGN);
 
-		/* Reset crop and compose rectangles */
-		crop->left = 0;
-		crop->top = 0;
-		crop->width = sdformat->format.width;
-		crop->height = sdformat->format.height;
-		csi_try_crop(priv, crop, cfg, &sdformat->format, upstream_ep);
-		compose->left = 0;
-		compose->top = 0;
-		compose->width = crop->width;
-		compose->height = crop->height;
-
 		*cc = imx_media_find_mbus_format(sdformat->format.code,
 						 CS_SEL_ANY, true);
 		if (!*cc) {
@@ -1385,6 +1374,17 @@ static void csi_try_fmt(struct csi_priv *priv,
 			&sdformat->format, infmt,
 			priv->active_output_pad == CSI_SRC_PAD_DIRECT);
 
+		/* Reset crop and compose rectangles */
+		crop->left = 0;
+		crop->top = 0;
+		crop->width = sdformat->format.width;
+		crop->height = sdformat->format.height;
+		csi_try_crop(priv, crop, cfg, &sdformat->format, upstream_ep);
+		compose->left = 0;
+		compose->top = 0;
+		compose->width = crop->width;
+		compose->height = crop->height;
+
 		break;
 	}
 }
-- 
2.7.4

[PATCH v2 10/10] media: imx.rst: Update doc to reflect fixes to interlaced capture

[Steve Longerbeam]

Also add an example pipeline for unconverted capture with interweave
on SabreAuto.

Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
---
 Documentation/media/v4l-drivers/imx.rst | 51 ++++++++++++++++++++++++---------
 1 file changed, 37 insertions(+), 14 deletions(-)

diff --git a/Documentation/media/v4l-drivers/imx.rst b/Documentation/media/v4l-drivers/imx.rst
index 65d3d15..4149b76 100644
--- a/Documentation/media/v4l-drivers/imx.rst
+++ b/Documentation/media/v4l-drivers/imx.rst
@@ -179,9 +179,10 @@ sink pad can take UYVY2X8, but the IDMAC source pad can output YUYV2X8.
 If the sink pad is receiving YUV, the output at the capture device can
 also be converted to a planar YUV format such as YUV420.
 
-It will also perform simple de-interlace without motion compensation,
-which is activated if the sink pad's field type is an interlaced type,
-and the IDMAC source pad field type is set to none.
+It will also perform simple interweave without motion compensation,
+which is activated if the sink pad's field type is sequential top-bottom
+or bottom-top or alternate, and the IDMAC source pad field type is
+interlaced (t-b, b-t, or unqualified interlaced).
 
 This subdev can generate the following event when enabling the second
 IDMAC source pad:
@@ -383,13 +384,13 @@ and CSC operations and flip/rotation controls. It will receive and
 process de-interlaced frames from the ipuX_vdic if ipuX_ic_prp is
 receiving from ipuX_vdic.
 
-Like the ipuX_csiY IDMAC source, it can perform simple de-interlace
+Like the ipuX_csiY IDMAC source, it can perform simple interweaving
 without motion compensation. However, note that if the ipuX_vdic is
 included in the pipeline (ipuX_ic_prp is receiving from ipuX_vdic),
-it's not possible to use simple de-interlace in ipuX_ic_prpvf, since
-the ipuX_vdic has already carried out de-interlacing (with motion
-compensation) and therefore the field type output from ipuX_ic_prp can
-only be none.
+it's not possible to use interweave in ipuX_ic_prpvf, since the
+ipuX_vdic has already carried out de-interlacing (with motion
+compensation) and therefore the field type output from ipuX_vdic
+can only be none (progressive).
 
 Capture Pipelines
 -----------------
@@ -514,10 +515,32 @@ On the SabreAuto, an on-board ADV7180 SD decoder is connected to the
 parallel bus input on the internal video mux to IPU1 CSI0.
 
 The following example configures a pipeline to capture from the ADV7180
+video decoder, assuming NTSC 720x480 input signals, using simple
+interweave (unconverted and without motion compensation). The adv7180
+must output sequential or alternating fields (field type 'seq-tb',
+'seq-bt', or 'alternate'):
+
+.. code-block:: none
+
+   # Setup links
+   media-ctl -l "'adv7180 3-0021':0 -> 'ipu1_csi0_mux':1[1]"
+   media-ctl -l "'ipu1_csi0_mux':2 -> 'ipu1_csi0':0[1]"
+   media-ctl -l "'ipu1_csi0':2 -> 'ipu1_csi0 capture':0[1]"
+   # Configure pads
+   media-ctl -V "'adv7180 3-0021':0 [fmt:UYVY2X8/720x480 field:seq-bt]"
+   media-ctl -V "'ipu1_csi0_mux':2 [fmt:UYVY2X8/720x480]"
+   media-ctl -V "'ipu1_csi0':2 [fmt:AYUV32/720x480 field:interlaced]"
+
+Streaming can then begin on the capture device node at
+"ipu1_csi0 capture". The v4l2-ctl tool can be used to select any
+supported YUV pixelformat on the capture device node.
+
+This example configures a pipeline to capture from the ADV7180
 video decoder, assuming NTSC 720x480 input signals, with Motion
-Compensated de-interlacing. Pad field types assume the adv7180 outputs
-"interlaced". $outputfmt can be any format supported by the ipu1_ic_prpvf
-entity at its output pad:
+Compensated de-interlacing. The adv7180 must output sequential or
+alternating fields (field type 'seq-tb', 'seq-bt', or 'alternate').
+$outputfmt can be any format supported by the ipu1_ic_prpvf entity
+at its output pad:
 
 .. code-block:: none
 
@@ -529,9 +552,9 @@ entity at its output pad:
    media-ctl -l "'ipu1_ic_prp':2 -> 'ipu1_ic_prpvf':0[1]"
    media-ctl -l "'ipu1_ic_prpvf':1 -> 'ipu1_ic_prpvf capture':0[1]"
    # Configure pads
-   media-ctl -V "'adv7180 3-0021':0 [fmt:UYVY2X8/720x480]"
-   media-ctl -V "'ipu1_csi0_mux':2 [fmt:UYVY2X8/720x480 field:interlaced]"
-   media-ctl -V "'ipu1_csi0':1 [fmt:AYUV32/720x480 field:interlaced]"
+   media-ctl -V "'adv7180 3-0021':0 [fmt:UYVY2X8/720x480 field:seq-bt]"
+   media-ctl -V "'ipu1_csi0_mux':2 [fmt:UYVY2X8/720x480]"
+   media-ctl -V "'ipu1_csi0':1 [fmt:AYUV32/720x480]"
    media-ctl -V "'ipu1_vdic':2 [fmt:AYUV32/720x480 field:none]"
    media-ctl -V "'ipu1_ic_prp':2 [fmt:AYUV32/720x480 field:none]"
    media-ctl -V "'ipu1_ic_prpvf':1 [fmt:$outputfmt field:none]"
-- 
2.7.4

Re: [PATCH v2 10/10] media: imx.rst: Update doc to reflect fixes to interlaced capture

[Philipp Zabel]

On Thu, 2018-05-31 at 17:30 -0700, Steve Longerbeam wrote:
> Also add an example pipeline for unconverted capture with interweave
> on SabreAuto.
> 
> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
> ---
>  Documentation/media/v4l-drivers/imx.rst | 51 ++++++++++++++++++++++++---------
>  1 file changed, 37 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/media/v4l-drivers/imx.rst b/Documentation/media/v4l-drivers/imx.rst
> index 65d3d15..4149b76 100644
> --- a/Documentation/media/v4l-drivers/imx.rst
> +++ b/Documentation/media/v4l-drivers/imx.rst
> @@ -179,9 +179,10 @@ sink pad can take UYVY2X8, but the IDMAC source pad can output YUYV2X8.
>  If the sink pad is receiving YUV, the output at the capture device can
>  also be converted to a planar YUV format such as YUV420.
>  
> -It will also perform simple de-interlace without motion compensation,
> -which is activated if the sink pad's field type is an interlaced type,
> -and the IDMAC source pad field type is set to none.
> +It will also perform simple interweave without motion compensation,
> +which is activated if the sink pad's field type is sequential top-bottom
> +or bottom-top or alternate, and the IDMAC source pad field type is
> +interlaced (t-b, b-t, or unqualified interlaced).

I think sink pad alternate behaviour should be equal to sink pad top-
bottom for PAL and sink pad bottom-top for NTSC. If we agree on this, we
should mention that here.

>  This subdev can generate the following event when enabling the second
>  IDMAC source pad:
> @@ -383,13 +384,13 @@ and CSC operations and flip/rotation controls. It will receive and
>  process de-interlaced frames from the ipuX_vdic if ipuX_ic_prp is
>  receiving from ipuX_vdic.
>  
> -Like the ipuX_csiY IDMAC source, it can perform simple de-interlace
> +Like the ipuX_csiY IDMAC source, it can perform simple interweaving
>  without motion compensation. However, note that if the ipuX_vdic is
>  included in the pipeline (ipuX_ic_prp is receiving from ipuX_vdic),
> -it's not possible to use simple de-interlace in ipuX_ic_prpvf, since
> -the ipuX_vdic has already carried out de-interlacing (with motion
> -compensation) and therefore the field type output from ipuX_ic_prp can
> -only be none.
> +it's not possible to use interweave in ipuX_ic_prpvf, since the
> +ipuX_vdic has already carried out de-interlacing (with motion
> +compensation) and therefore the field type output from ipuX_vdic
> +can only be none (progressive).
>  
>  Capture Pipelines
>  -----------------
> @@ -514,10 +515,32 @@ On the SabreAuto, an on-board ADV7180 SD decoder is connected to the
>  parallel bus input on the internal video mux to IPU1 CSI0.
>  
>  The following example configures a pipeline to capture from the ADV7180
> +video decoder, assuming NTSC 720x480 input signals, using simple
> +interweave (unconverted and without motion compensation). The adv7180
> +must output sequential or alternating fields (field type 'seq-tb',
> +'seq-bt', or 'alternate'):
> +
> +.. code-block:: none
> +
> +   # Setup links
> +   media-ctl -l "'adv7180 3-0021':0 -> 'ipu1_csi0_mux':1[1]"
> +   media-ctl -l "'ipu1_csi0_mux':2 -> 'ipu1_csi0':0[1]"
> +   media-ctl -l "'ipu1_csi0':2 -> 'ipu1_csi0 capture':0[1]"
> +   # Configure pads
> +   media-ctl -V "'adv7180 3-0021':0 [fmt:UYVY2X8/720x480 field:seq-bt]"
> +   media-ctl -V "'ipu1_csi0_mux':2 [fmt:UYVY2X8/720x480]"
> +   media-ctl -V "'ipu1_csi0':2 [fmt:AYUV32/720x480 field:interlaced]"

Could the example suggest using interlaced-bt to be explicit here?
Actually, I don't think we should allow interlaced on the CSI src pads
at all in this case. Technically it always writes either seq-tb or seq-
bt into the smfc, never interlaced (unless the input is already
interlaced).

> +Streaming can then begin on the capture device node at
> +"ipu1_csi0 capture". The v4l2-ctl tool can be used to select any
> +supported YUV pixelformat on the capture device node.
> +
> +This example configures a pipeline to capture from the ADV7180
>  video decoder, assuming NTSC 720x480 input signals, with Motion
> -Compensated de-interlacing. Pad field types assume the adv7180 outputs
> -"interlaced". $outputfmt can be any format supported by the ipu1_ic_prpvf
> -entity at its output pad:
> +Compensated de-interlacing. The adv7180 must output sequential or
> +alternating fields (field type 'seq-tb', 'seq-bt', or 'alternate').
> +$outputfmt can be any format supported by the ipu1_ic_prpvf entity
> +at its output pad:
>  
>  .. code-block:: none
>  
> @@ -529,9 +552,9 @@ entity at its output pad:
>     media-ctl -l "'ipu1_ic_prp':2 -> 'ipu1_ic_prpvf':0[1]"
>     media-ctl -l "'ipu1_ic_prpvf':1 -> 'ipu1_ic_prpvf capture':0[1]"
>     # Configure pads
> -   media-ctl -V "'adv7180 3-0021':0 [fmt:UYVY2X8/720x480]"
> -   media-ctl -V "'ipu1_csi0_mux':2 [fmt:UYVY2X8/720x480 field:interlaced]"
> -   media-ctl -V "'ipu1_csi0':1 [fmt:AYUV32/720x480 field:interlaced]"
> +   media-ctl -V "'adv7180 3-0021':0 [fmt:UYVY2X8/720x480 field:seq-bt]"
> +   media-ctl -V "'ipu1_csi0_mux':2 [fmt:UYVY2X8/720x480]"
> +   media-ctl -V "'ipu1_csi0':1 [fmt:AYUV32/720x480]"
>     media-ctl -V "'ipu1_vdic':2 [fmt:AYUV32/720x480 field:none]"
>     media-ctl -V "'ipu1_ic_prp':2 [fmt:AYUV32/720x480 field:none]"
>     media-ctl -V "'ipu1_ic_prpvf':1 [fmt:$outputfmt field:none]"

This looks good to me.

regards
Philipp

Re: [PATCH v2 10/10] media: imx.rst: Update doc to reflect fixes to interlaced capture

[Steve Longerbeam]

On 06/01/2018 06:44 AM, Philipp Zabel wrote:
> On Thu, 2018-05-31 at 17:30 -0700, Steve Longerbeam wrote:
>> Also add an example pipeline for unconverted capture with interweave
>> on SabreAuto.
>>
>> Signed-off-by: Steve Longerbeam <steve_longerbeam@mentor.com>
>> ---
>>   Documentation/media/v4l-drivers/imx.rst | 51 ++++++++++++++++++++++++---------
>>   1 file changed, 37 insertions(+), 14 deletions(-)
>>
>> diff --git a/Documentation/media/v4l-drivers/imx.rst b/Documentation/media/v4l-drivers/imx.rst
>> index 65d3d15..4149b76 100644
>> --- a/Documentation/media/v4l-drivers/imx.rst
>> +++ b/Documentation/media/v4l-drivers/imx.rst
>> @@ -179,9 +179,10 @@ sink pad can take UYVY2X8, but the IDMAC source pad can output YUYV2X8.
>>   If the sink pad is receiving YUV, the output at the capture device can
>>   also be converted to a planar YUV format such as YUV420.
>>   
>> -It will also perform simple de-interlace without motion compensation,
>> -which is activated if the sink pad's field type is an interlaced type,
>> -and the IDMAC source pad field type is set to none.
>> +It will also perform simple interweave without motion compensation,
>> +which is activated if the sink pad's field type is sequential top-bottom
>> +or bottom-top or alternate, and the IDMAC source pad field type is
>> +interlaced (t-b, b-t, or unqualified interlaced).
> I think sink pad alternate behaviour should be equal to sink pad top-
> bottom for PAL and sink pad bottom-top for NTSC. If we agree on this, we
> should mention that here.

Agreed.

>
>>   This subdev can generate the following event when enabling the second
>>   IDMAC source pad:
>> @@ -383,13 +384,13 @@ and CSC operations and flip/rotation controls. It will receive and
>>   process de-interlaced frames from the ipuX_vdic if ipuX_ic_prp is
>>   receiving from ipuX_vdic.
>>   
>> -Like the ipuX_csiY IDMAC source, it can perform simple de-interlace
>> +Like the ipuX_csiY IDMAC source, it can perform simple interweaving
>>   without motion compensation. However, note that if the ipuX_vdic is
>>   included in the pipeline (ipuX_ic_prp is receiving from ipuX_vdic),
>> -it's not possible to use simple de-interlace in ipuX_ic_prpvf, since
>> -the ipuX_vdic has already carried out de-interlacing (with motion
>> -compensation) and therefore the field type output from ipuX_ic_prp can
>> -only be none.
>> +it's not possible to use interweave in ipuX_ic_prpvf, since the
>> +ipuX_vdic has already carried out de-interlacing (with motion
>> +compensation) and therefore the field type output from ipuX_vdic
>> +can only be none (progressive).
>>   
>>   Capture Pipelines
>>   -----------------
>> @@ -514,10 +515,32 @@ On the SabreAuto, an on-board ADV7180 SD decoder is connected to the
>>   parallel bus input on the internal video mux to IPU1 CSI0.
>>   
>>   The following example configures a pipeline to capture from the ADV7180
>> +video decoder, assuming NTSC 720x480 input signals, using simple
>> +interweave (unconverted and without motion compensation). The adv7180
>> +must output sequential or alternating fields (field type 'seq-tb',
>> +'seq-bt', or 'alternate'):
>> +
>> +.. code-block:: none
>> +
>> +   # Setup links
>> +   media-ctl -l "'adv7180 3-0021':0 -> 'ipu1_csi0_mux':1[1]"
>> +   media-ctl -l "'ipu1_csi0_mux':2 -> 'ipu1_csi0':0[1]"
>> +   media-ctl -l "'ipu1_csi0':2 -> 'ipu1_csi0 capture':0[1]"
>> +   # Configure pads
>> +   media-ctl -V "'adv7180 3-0021':0 [fmt:UYVY2X8/720x480 field:seq-bt]"
>> +   media-ctl -V "'ipu1_csi0_mux':2 [fmt:UYVY2X8/720x480]"
>> +   media-ctl -V "'ipu1_csi0':2 [fmt:AYUV32/720x480 field:interlaced]"
> Could the example suggest using interlaced-bt to be explicit here?
> Actually, I don't think we should allow interlaced on the CSI src pads
> at all in this case. Technically it always writes either seq-tb or seq-
> bt into the smfc, never interlaced (unless the input is already
> interlaced).

Agreed, I'll make that change in v3 and update the doc.

Steve

>> +Streaming can then begin on the capture device node at
>> +"ipu1_csi0 capture". The v4l2-ctl tool can be used to select any
>> +supported YUV pixelformat on the capture device node.
>> +
>> +This example configures a pipeline to capture from the ADV7180
>>   video decoder, assuming NTSC 720x480 input signals, with Motion
>> -Compensated de-interlacing. Pad field types assume the adv7180 outputs
>> -"interlaced". $outputfmt can be any format supported by the ipu1_ic_prpvf
>> -entity at its output pad:
>> +Compensated de-interlacing. The adv7180 must output sequential or
>> +alternating fields (field type 'seq-tb', 'seq-bt', or 'alternate').
>> +$outputfmt can be any format supported by the ipu1_ic_prpvf entity
>> +at its output pad:
>>   
>>   .. code-block:: none
>>   
>> @@ -529,9 +552,9 @@ entity at its output pad:
>>      media-ctl -l "'ipu1_ic_prp':2 -> 'ipu1_ic_prpvf':0[1]"
>>      media-ctl -l "'ipu1_ic_prpvf':1 -> 'ipu1_ic_prpvf capture':0[1]"
>>      # Configure pads
>> -   media-ctl -V "'adv7180 3-0021':0 [fmt:UYVY2X8/720x480]"
>> -   media-ctl -V "'ipu1_csi0_mux':2 [fmt:UYVY2X8/720x480 field:interlaced]"
>> -   media-ctl -V "'ipu1_csi0':1 [fmt:AYUV32/720x480 field:interlaced]"
>> +   media-ctl -V "'adv7180 3-0021':0 [fmt:UYVY2X8/720x480 field:seq-bt]"
>> +   media-ctl -V "'ipu1_csi0_mux':2 [fmt:UYVY2X8/720x480]"
>> +   media-ctl -V "'ipu1_csi0':1 [fmt:AYUV32/720x480]"
>>      media-ctl -V "'ipu1_vdic':2 [fmt:AYUV32/720x480 field:none]"
>>      media-ctl -V "'ipu1_ic_prp':2 [fmt:AYUV32/720x480 field:none]"
>>      media-ctl -V "'ipu1_ic_prpvf':1 [fmt:$outputfmt field:none]"
> This looks good to me.
>
> regards
> Philipp

Re: [PATCH v2 10/10] media: imx.rst: Update doc to reflect fixes to interlaced capture

[Steve Longerbeam]

On 06/01/2018 06:44 AM, Philipp Zabel wrote:
> On Thu, 2018-05-31 at 17:30 -0700, Steve Longerbeam wrote:
> <snip>
>> +
>> +.. code-block:: none
>> +
>> +   # Setup links
>> +   media-ctl -l "'adv7180 3-0021':0 -> 'ipu1_csi0_mux':1[1]"
>> +   media-ctl -l "'ipu1_csi0_mux':2 -> 'ipu1_csi0':0[1]"
>> +   media-ctl -l "'ipu1_csi0':2 -> 'ipu1_csi0 capture':0[1]"
>> +   # Configure pads
>> +   media-ctl -V "'adv7180 3-0021':0 [fmt:UYVY2X8/720x480 field:seq-bt]"
>> +   media-ctl -V "'ipu1_csi0_mux':2 [fmt:UYVY2X8/720x480]"
>> +   media-ctl -V "'ipu1_csi0':2 [fmt:AYUV32/720x480 field:interlaced]"
> Could the example suggest using interlaced-bt to be explicit here?
> Actually, I don't think we should allow interlaced on the CSI src pads
> at all in this case. Technically it always writes either seq-tb or seq-
> bt into the smfc, never interlaced (unless the input is already
> interlaced).
>

Hmm, if the sink is 'alternate', and the requested source is
'interlaced*', perhaps we should allow the source to be
'interlaced*' and not override it. For example, if requested
is 'interlaced-tb', let it be that. IOW assume user knows something
we don't about the original field order, or is experimenting
with finding the correct field order.

Steve

Re: [PATCH v2 10/10] media: imx.rst: Update doc to reflect fixes to interlaced capture

[Krzysztof Hałasa]

Steve Longerbeam <steve_longerbeam@mentor.com> writes:

> Hmm, if the sink is 'alternate', and the requested source is
> 'interlaced*', perhaps we should allow the source to be
> 'interlaced*' and not override it. For example, if requested
> is 'interlaced-tb', let it be that. IOW assume user knows something
> we don't about the original field order, or is experimenting
> with finding the correct field order.

Yes, this is clearly possible. In fact the analog signal doesn't carry
information about the field order (if any). The video editing/encoding
software does motion estimation for this, and there is no other way
(given that the video material can change from progressive to interlaced
and vice versa, and probably can change the field order, at any time).
-- 
Krzysztof Halasa

Industrial Research Institute for Automation and Measurements PIAP
Al. Jerozolimskie 202, 02-486 Warsaw, Poland

Re: [PATCH v2 10/10] media: imx.rst: Update doc to reflect fixes to interlaced capture

[Philipp Zabel]

On Sat, 2018-06-02 at 11:44 -0700, Steve Longerbeam wrote:
> 
> On 06/01/2018 06:44 AM, Philipp Zabel wrote:
> > On Thu, 2018-05-31 at 17:30 -0700, Steve Longerbeam wrote:
> > <snip>
> > > +
> > > +.. code-block:: none
> > > +
> > > +   # Setup links
> > > +   media-ctl -l "'adv7180 3-0021':0 -> 'ipu1_csi0_mux':1[1]"
> > > +   media-ctl -l "'ipu1_csi0_mux':2 -> 'ipu1_csi0':0[1]"
> > > +   media-ctl -l "'ipu1_csi0':2 -> 'ipu1_csi0 capture':0[1]"
> > > +   # Configure pads
> > > +   media-ctl -V "'adv7180 3-0021':0 [fmt:UYVY2X8/720x480 field:seq-bt]"
> > > +   media-ctl -V "'ipu1_csi0_mux':2 [fmt:UYVY2X8/720x480]"
> > > +   media-ctl -V "'ipu1_csi0':2 [fmt:AYUV32/720x480 field:interlaced]"
> > 
> > Could the example suggest using interlaced-bt to be explicit here?
> > Actually, I don't think we should allow interlaced on the CSI src pads
> > at all in this case. Technically it always writes either seq-tb or seq-
> > bt into the smfc, never interlaced (unless the input is already
> > interlaced).
> > 
> 
> Hmm, if the sink is 'alternate', and the requested source is
> 'interlaced*', perhaps we should allow the source to be
> 'interlaced*' and not override it. For example, if requested
> is 'interlaced-tb', let it be that. IOW assume user knows something
> we don't about the original field order, or is experimenting
> with finding the correct field order.

If the source material is really interlaced and not alternate, shouldn't
the sink pad be set to interlaced?

regards
Philipp