i.MX6 IPU CSI analog video input on Ventana

Commit Message

Krzysztof Hałasa May 24, 2018, 3:56 p.m. UTC

Hi,

I've experimented with the ADV7180 a bit and this is what I found.

First, I'm using (with a NTSC camera but I guess PAL won't be much
different):
media-ctl -V '"adv7180 2-0020":0[fmt:UYVY2X8 720x480 field:interlaced]'
media-ctl -V '"ipu2_csi1_mux":1[fmt:UYVY2X8 720x480 field:interlaced]'
media-ctl -V '"ipu2_csi1_mux":2[fmt:UYVY2X8 720x480 field:interlaced]'
media-ctl -V '"ipu2_csi1":0[fmt:UYVY2X8 720x480 field:interlaced]'
media-ctl -V '"ipu2_csi1":2[fmt:UYVY2X8 720x480 field:interlaced]'

IOW I set all of the parts to interlaced mode. If i set the last element
to "none", the CSI is not set for interlaced input, and nothing works at
the low level.

This requires a quick temporary hack:

Comments

Steve Longerbeam May 24, 2018, 6:12 p.m. UTC | #1

Hi Krzysztof,


On 05/24/2018 08:56 AM, Krzysztof Hałasa wrote:
> Hi,
>
> I've experimented with the ADV7180 a bit and this is what I found.
>
> First, I'm using (with a NTSC camera but I guess PAL won't be much
> different):
> media-ctl -V '"adv7180 2-0020":0[fmt:UYVY2X8 720x480 field:interlaced]'
> media-ctl -V '"ipu2_csi1_mux":1[fmt:UYVY2X8 720x480 field:interlaced]'
> media-ctl -V '"ipu2_csi1_mux":2[fmt:UYVY2X8 720x480 field:interlaced]'
> media-ctl -V '"ipu2_csi1":0[fmt:UYVY2X8 720x480 field:interlaced]'
> media-ctl -V '"ipu2_csi1":2[fmt:UYVY2X8 720x480 field:interlaced]'
>
> IOW I set all of the parts to interlaced mode. If i set the last element
> to "none", the CSI is not set for interlaced input, and nothing works at
> the low level.

This is what I don't understand. By setting pad ipu2_csi1:2 to
"none", the if statement below should be true (sink pad field
is "interlaced" and the capture field is propagated from ipu2_csi1:2
field so it is "none", thus ipu_cpmem_interlaced_scan() will be called.

And yes you are correct, ipu_cpmem_interlaced_scan() must be
called to enable IDMAC interweave, which is what you want.

>
> This requires a quick temporary hack:
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -474,8 +474,8 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
>   
>   	ipu_smfc_set_burstsize(priv->smfc, burst_size);
>   
> -	if (image.pix.field == V4L2_FIELD_NONE &&
> -	    V4L2_FIELD_HAS_BOTH(infmt->field))
> +	if (1 || (image.pix.field == V4L2_FIELD_NONE &&
> +		  V4L2_FIELD_HAS_BOTH(infmt->field)))
>   		ipu_cpmem_interlaced_scan(priv->idmac_ch,
>   					  image.pix.bytesperline);
>   
>
> I.e., I need to set CPMEM to interlaced mode when I operate CSI in
> interlaced mode. The original code is a bit unclear to me in fact.

No the code above is not unclear at all. The if statement is saying
that if the user wants progressive output (V4L2_FIELD_NONE), and
the input contains fields, then turn on interweave in the IDMAC
channel.

This might be a good time to bring up the fact that the ADV7180 driver 
is wrong
to set output to "interlaced". The ADV7180 does not transmit top lines 
interlaced
with bottom lines. It transmits all top lines followed by all bottom 
lines (or
vice-versa), i.e. it should be either V4L2_FIELD_SEQ_TB or 
V4L2_FIELD_SEQ_BT.
It can also be set to V4L2_FIELD_ALTERNATE, and then it is left up to 
downstream
elements to determine field order (TB or BT).

I've previously sent a patch to fix this at 
https://patchwork.linuxtv.org/patch/36193/
but it got lost. Niklas has said he will pick this up again.

>
> The following is required as well. Now the question is why we can't skip
> writing those odd UV rows. Anyway, with these 2 changes, I get a stable
> NTSC (and probably PAL) interlaced video stream.
>
> The manual says: Reduce Double Read or Writes (RDRW):
> This bit is relevant for YUV4:2:0 formats. For write channels:
> U and V components are not written to odd rows.
>
> How could it be so? With YUV420, are they normally written?

Well, given that this bit exists, and assuming I understand it correctly 
(1),
I guess the U and V components for odd rows normally are placed on the
AXI bus. Which is a total waste of bus bandwidth because in 4:2:0,
the U and V components are the same for odd and even rows.

In other words for writing 4:2:0 to memory, this bit should _always_ be set.

(1) OTOH I don't really understand what this bit is trying to say.
Whether this bit is set or not, the data in memory is correct
for planar 4:2:0: y plane buffer followed by U plane of 1/4 size
(decimated by 2 in width and height), followed by Y plane of 1/4
size.

So I assume it is saying that the IPU normally places U/V components
on the AXI bus for odd rows, that are identical to the even row values.
IOW somehow those identical odd rows are dropped before writing to
the U/V planes in memory.

Philipp please chime in if you have something to add here.

Steve

> OTOH it seems that not only UV is broken with this bit set.
> Y is broken as well.
>
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -413,14 +413,12 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
>   		passthrough_bits = 16;
>   		break;
>   	case V4L2_PIX_FMT_YUV420:
>   	case V4L2_PIX_FMT_NV12:
>   		burst_size = (image.pix.width & 0x3f) ?
>   			     ((image.pix.width & 0x1f) ?
>   			      ((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);
>   		break;
>   	case V4L2_PIX_FMT_YUYV:
>   	case V4L2_PIX_FMT_UYVY:

Philipp Zabel May 25, 2018, 6:32 a.m. UTC | #2

On Thu, 2018-05-24 at 11:12 -0700, Steve Longerbeam wrote:
[...]
> > The following is required as well. Now the question is why we can't skip
> > writing those odd UV rows. Anyway, with these 2 changes, I get a stable
> > NTSC (and probably PAL) interlaced video stream.
> > 
> > The manual says: Reduce Double Read or Writes (RDRW):
> > This bit is relevant for YUV4:2:0 formats. For write channels:
> > U and V components are not written to odd rows.
> > 
> > How could it be so? With YUV420, are they normally written?
> 
> Well, given that this bit exists, and assuming I understand it correctly 
> (1),
> I guess the U and V components for odd rows normally are placed on the
> AXI bus. Which is a total waste of bus bandwidth because in 4:2:0,
> the U and V components are the same for odd and even rows.
> 
> In other words for writing 4:2:0 to memory, this bit should _always_ be set.
> 
> (1) OTOH I don't really understand what this bit is trying to say.
> Whether this bit is set or not, the data in memory is correct
> for planar 4:2:0: y plane buffer followed by U plane of 1/4 size
> (decimated by 2 in width and height), followed by Y plane of 1/4
> size.
> 
> So I assume it is saying that the IPU normally places U/V components
> on the AXI bus for odd rows, that are identical to the even row values.

Whether they are identical depends on the input format.

The IDMAC always gets fed AYUV32 from the CSI or IC.
If the CSI captures YUV 4:2:x, odd and even lines will have the same
chroma values. But if the CSI captures YUV 4:4:4 (or RGB, fed through
the IC), we can have AYUV32 input with different chroma values on even
and odd lines.
In that case the IPU just writes the even chroma line and then
overwrites it with the odd line, unless the double write reduction bit
is set.

> IOW somehow those identical odd rows are dropped before writing to
> the U/V planes in memory.

potentially identical.

> Philipp please chime in if you have something to add here.

I suppose the bit could be used to choose to write the chroma values of
odd instead of even lines for 4:4:4 inputs, at the cost of increased
memory bandwidth usage.

> Steve
> 
> > OTOH it seems that not only UV is broken with this bit set.
> > Y is broken as well.

Maybe scanline interlave and double write reduction can't be used at the
same time?

regards
Philipp

Krzysztof Hałasa May 25, 2018, 7:07 a.m. UTC | #3

Steve Longerbeam <[e-mail]> writes:

>> The manual says: Reduce Double Read or Writes (RDRW):
>> This bit is relevant for YUV4:2:0 formats. For write channels:
>> U and V components are not written to odd rows.
>>
>> How could it be so? With YUV420, are they normally written?
>
> Well, given that this bit exists, and assuming I understand it
> correctly (1),
> I guess the U and V components for odd rows normally are placed on the
> AXI bus. Which is a total waste of bus bandwidth because in 4:2:0,
> the U and V components are the same for odd and even rows.

Right. Now, the AXI bus is just a "memory bus", it's a newer version of
the AHB. One can't simply "place data" on AXI, it must be a write to
a specific address, and the data will end up in RAM (assuming the
configuration is sane). How can we have two possible data formats (with
and without the RDRW bit) with fixed image format (420-type) is beyond
me.

> Commits
>
> 14330d7f08 ("media: imx: csi: enable double write reduction")
> b54a5c2dc8 ("media: imx: prpencvf: enable double write reduction")
>
> should be reverted for now, until the behavior of this bit is better
> understood.

I agree.

I have dumped a raw frame (720 x 480 NV12 frame size 518400 from
interlaced NTSC camera), with the RDRW bit set.

The Y plane contains, well, valid Y data (720 x 480 bytes).

The color plane (360 pixels x 240 line pairs * 2 colors) has every other
line pair zeroed. I.e., there is a 720-byte line pair filled with valid UV
data, then there are 720 zeros (360 zeroed UV pairs). Then there is valid
UV data and so on.

Not sure what could it be for. Some weird sort of YUV 4:1:0? I guess we
don't want it ATM.

WRT ADV7180 field format:

> This might be a good time to bring up the fact that the ADV7180 driver
> is wrong
> to set output to "interlaced". The ADV7180 does not transmit top lines
> interlaced
> with bottom lines. It transmits all top lines followed by all bottom
> lines (or
> vice-versa), i.e. it should be either V4L2_FIELD_SEQ_TB or
> V4L2_FIELD_SEQ_BT.
> It can also be set to V4L2_FIELD_ALTERNATE, and then it is left up to
> downstream
> elements to determine field order (TB or BT).

Right. ADV7180, AFAIK, doesn't have the hardware (frame buffer) to get
two interlaced fields and merge them to form a complete frame.
It simply transforms the incoming analog signal into binary data stream.
This issue should be fixed.

Thanks for your work,

Krzysztof Hałasa May 25, 2018, 7:18 a.m. UTC | #4

Philipp Zabel <[e-mail]> writes:

> Maybe scanline interlave and double write reduction can't be used at the
> same time?

Well, if it works in non-interlaced modes - it may be the case.

Perhaps the data reduction is done before the field merge step. This
would make it incompatible: in interlaced mode we need all color data
from a field (we could potentially remove all color info from the other
field, or use some average).

Steve Longerbeam May 25, 2018, 11:21 p.m. UTC | #5

Hi Philipp,


On 05/24/2018 11:32 PM, Philipp Zabel wrote:
> On Thu, 2018-05-24 at 11:12 -0700, Steve Longerbeam wrote:
> [...]
>>> The following is required as well. Now the question is why we can't skip
>>> writing those odd UV rows. Anyway, with these 2 changes, I get a stable
>>> NTSC (and probably PAL) interlaced video stream.
>>>
>>> The manual says: Reduce Double Read or Writes (RDRW):
>>> This bit is relevant for YUV4:2:0 formats. For write channels:
>>> U and V components are not written to odd rows.
>>>
>>> How could it be so? With YUV420, are they normally written?
>> Well, given that this bit exists, and assuming I understand it correctly
>> (1),
>> I guess the U and V components for odd rows normally are placed on the
>> AXI bus. Which is a total waste of bus bandwidth because in 4:2:0,
>> the U and V components are the same for odd and even rows.
>>
>> In other words for writing 4:2:0 to memory, this bit should _always_ be set.
>>
>> (1) OTOH I don't really understand what this bit is trying to say.
>> Whether this bit is set or not, the data in memory is correct
>> for planar 4:2:0: y plane buffer followed by U plane of 1/4 size
>> (decimated by 2 in width and height), followed by Y plane of 1/4
>> size.
>>
>> So I assume it is saying that the IPU normally places U/V components
>> on the AXI bus for odd rows, that are identical to the even row values.
> Whether they are identical depends on the input format.

Right, this is the part I was missing, thanks for clarifying. The
even and odd chroma rows coming into the IDMAC from the
CSI (or IC) may not be identical if the CSI has captured 4:4:4
(or 4:2:2 yeah? 4:2:2 is only decimated in width not height).

But still, when the IDMAC has finished pixel packing/unpacking and
is writing 4:2:0 to memory, it should always skip overwriting the even
rows with the odd rows, whether or not it has received identical chroma
even/odd lines from the CSI.

Unless interweave is enabled :) See below.
>
> The IDMAC always gets fed AYUV32 from the CSI or IC.
> If the CSI captures YUV 4:2:x, odd and even lines will have the same
> chroma values. But if the CSI captures YUV 4:4:4 (or RGB, fed through
> the IC), we can have AYUV32 input with different chroma values on even
> and odd lines.
> In that case the IPU just writes the even chroma line and then
> overwrites it with the odd line, unless the double write reduction bit
> is set.
>
>> IOW somehow those identical odd rows are dropped before writing to
>> the U/V planes in memory.
> potentially identical.

Right.

>
>> Philipp please chime in if you have something to add here.
> I suppose the bit could be used to choose to write the chroma values of
> odd instead of even lines for 4:4:4 inputs, at the cost of increased
> memory bandwidth usage.
>
>> Steve
>>
>>> OTOH it seems that not only UV is broken with this bit set.
>>> Y is broken as well.
> Maybe scanline interlave and double write reduction can't be used at the
> same time?

Yes, I just verified that. I went back to the SabreLite with the
progressive output OV5640, and double-write-reduction for
4:2:0 capture works fine, the images are correct.

Steve

Steve Longerbeam May 25, 2018, 11:39 p.m. UTC | #6

Hi Krzysztof, Philipp,


On 05/25/2018 12:18 AM, Krzysztof Hałasa wrote:
> Philipp Zabel <[e-mail]> writes:
>
>> Maybe scanline interlave and double write reduction can't be used at the
>> same time?
> Well, if it works in non-interlaced modes - it may be the case.
>
> Perhaps the data reduction is done before the field merge step.

Yeah, that might explain the incompatibility. The IDMAC top/bottom
line merging needs all the lines present. It won't have them if the
IDMAC has previously skipped the odd chroma lines. Or maybe I'm
over-simplifying.

In any case as I said they are proved to be incompatible. I am
preparing a patch-set with these fixes.

Krzysztof, in the meantime the patches are available in my
media-tree fork, for testing on the Ventana GW5300:

[e-mail]:slongerbeam/mediatree.git, branch 'fix-csi-interlaced'

Steve

Krzysztof Hałasa May 29, 2018, 7:26 a.m. UTC | #7

Hi Steve,

Steve Longerbeam <[e-mail]> writes:

> Krzysztof, in the meantime the patches are available in my
> media-tree fork, for testing on the Ventana GW5300:
>
> [e-mail]:slongerbeam/mediatree.git, branch 'fix-csi-interlaced'

I assume fix-csi-interlaced.2 is a newer version, isn't it?

I merged it and I think I can't set the correct config:
media-ctl -r -l '"adv7180 2-0020":0->"ipu2_csi1_mux":1[1],
                 "ipu2_csi1_mux":2->"ipu2_csi1":0[1],
                 "ipu2_csi1":2->"ipu2_csi1 capture":0[1]'

media-ctl -V "'adv7180 2-0020':0 [fmt:UYVY2X8/720x480 field:seq-bt]"
media-ctl -V "'ipu2_csi1_mux':2 [fmt:UYVY2X8/720x480]"
media-ctl -V "'ipu2_csi1':2 [fmt:AYUV32/720x480 field:interlaced]"

produces:
"adv7180 2-0020":0 [fmt:UYVY2X8/720x480 field:interlaced]
"ipu2_csi1_mux":1  [fmt:UYVY2X8/720x480 field:interlaced]
"ipu2_csi1_mux":2  [fmt:UYVY2X8/720x480 field:interlaced]
"ipu2_csi1":0      [fmt:UYVY2X8/720x480 field:interlaced]
"ipu2_csi1":2      [fmt:AYUV32/720x480  field:interlaced]

Do I need to patch ADV7180 for field type "sequential"?

It seems setting seq-bt on ADV7180 sets "interlaced" on ADV -> MUX input
-> MUX output. Setting "'ipu2_csi1_mux':2 [fmt:UYVY2X8/720x480]" sets
interlaced on all elements of the pipeline. The effect is a pair of
fields, not an interlaced frame.

Steve Longerbeam May 29, 2018, 2 p.m. UTC | #8

Hi Krzysztof,


On 05/29/2018 12:26 AM, Krzysztof Hałasa wrote:
> Hi Steve,
>
> Steve Longerbeam <[e-mail]> writes:
>
>> Krzysztof, in the meantime the patches are available in my
>> media-tree fork, for testing on the Ventana GW5300:
>>
>> [e-mail]:slongerbeam/mediatree.git, branch 'fix-csi-interlaced'
> I assume fix-csi-interlaced.2 is a newer version, isn't it?
>
> I merged it and I think I can't set the correct config:
> media-ctl -r -l '"adv7180 2-0020":0->"ipu2_csi1_mux":1[1],
>                   "ipu2_csi1_mux":2->"ipu2_csi1":0[1],
>                   "ipu2_csi1":2->"ipu2_csi1 capture":0[1]'
>
> media-ctl -V "'adv7180 2-0020':0 [fmt:UYVY2X8/720x480 field:seq-bt]"
> media-ctl -V "'ipu2_csi1_mux':2 [fmt:UYVY2X8/720x480]"
> media-ctl -V "'ipu2_csi1':2 [fmt:AYUV32/720x480 field:interlaced]"
>
> produces:
> "adv7180 2-0020":0 [fmt:UYVY2X8/720x480 field:interlaced]
> "ipu2_csi1_mux":1  [fmt:UYVY2X8/720x480 field:interlaced]
> "ipu2_csi1_mux":2  [fmt:UYVY2X8/720x480 field:interlaced]
> "ipu2_csi1":0      [fmt:UYVY2X8/720x480 field:interlaced]
> "ipu2_csi1":2      [fmt:AYUV32/720x480  field:interlaced]
>
> Do I need to patch ADV7180 for field type "sequential"?

Yes, you'll need to patch adv7180.c to select either
'seq-bt/tb' or 'alternate'. The current version will override
any attempt to set field to anything other than 'interlaced'.
This is in anticipation of getting a patch merged for adv7180
that fixes this.

Steve

Krzysztof Hałasa May 30, 2018, 8:53 a.m. UTC | #9

Steve Longerbeam <[e-mail]> writes:

> Yes, you'll need to patch adv7180.c to select either
> 'seq-bt/tb' or 'alternate'. The current version will override
> any attempt to set field to anything other than 'interlaced'.
> This is in anticipation of getting a patch merged for adv7180
> that fixes this.

Right. I've applied the patch from your adv718x-v6 branch (just the
"media: adv7180: fix field type" patch) and now it works.

Also, I have changed "seq-bt" to "alternate" (in the examples in
Documentation/media/v4l-drivers/imx.rst) - the data stream from ADV7180
to CSI consists of separate fields which can then be merged into frames
in any order requested by the user (e.g. in accordance with "digital PAL
/ NTSC" requirements).

The following:
media-ctl -V "'adv7180 2-0020':0 [fmt:UYVY2X8/720x480 field:alternate]"
media-ctl -V "'ipu2_csi1_mux':2 [fmt:UYVY2X8/720x480]"
media-ctl -V "'ipu2_csi1':2 [fmt:AYUV32/720x480 field:interlaced]"
now produces:

"adv7180 2-0020":0 [fmt:UYVY2X8/720x480 field:alternate]
"ipu2_csi1_mux":1  [fmt:UYVY2X8/720x480 field:alternate]
"ipu2_csi1_mux":2  [fmt:UYVY2X8/720x480 field:alternate]
"ipu2_csi1":0      [fmt:UYVY2X8/720x480 field:alternate]
"ipu2_csi1":2      [fmt:AYUV32/720x480 field:interlaced-bt]

and it works correctly.

The only issue is that I can't:
media-ctl -V "'ipu2_csi1':2 [fmt:AYUV32/720x480 field:interlaced-tb]"
(it remains fixed in -bt mode since NTSC is the default). I think we may
set TB/BT by default (depending on CSI input geometry or TV standard),
but it should be possible for the user to explicitly request the field
order on CSI output (I can make a patch I guess).

Steve Longerbeam May 30, 2018, 5:57 p.m. UTC | #10

Hi Krzysztof,


On 05/30/2018 01:53 AM, Krzysztof Hałasa wrote:
> Steve Longerbeam <[e-mail]> writes:
>
>> Yes, you'll need to patch adv7180.c to select either
>> 'seq-bt/tb' or 'alternate'. The current version will override
>> any attempt to set field to anything other than 'interlaced'.
>> This is in anticipation of getting a patch merged for adv7180
>> that fixes this.
> Right. I've applied the patch from your adv718x-v6 branch (just the
> "media: adv7180: fix field type" patch) and now it works.
>
> Also, I have changed "seq-bt" to "alternate" (in the examples in
> Documentation/media/v4l-drivers/imx.rst) - the data stream from ADV7180
> to CSI consists of separate fields which can then be merged into frames
> in any order requested by the user (e.g. in accordance with "digital PAL
> / NTSC" requirements).
>
> The following:
> media-ctl -V "'adv7180 2-0020':0 [fmt:UYVY2X8/720x480 field:alternate]"
> media-ctl -V "'ipu2_csi1_mux':2 [fmt:UYVY2X8/720x480]"
> media-ctl -V "'ipu2_csi1':2 [fmt:AYUV32/720x480 field:interlaced]"
> now produces:
>
> "adv7180 2-0020":0 [fmt:UYVY2X8/720x480 field:alternate]
> "ipu2_csi1_mux":1  [fmt:UYVY2X8/720x480 field:alternate]
> "ipu2_csi1_mux":2  [fmt:UYVY2X8/720x480 field:alternate]
> "ipu2_csi1":0      [fmt:UYVY2X8/720x480 field:alternate]
> "ipu2_csi1":2      [fmt:AYUV32/720x480 field:interlaced-bt]
>
> and it works correctly.
>
> The only issue is that I can't:
> media-ctl -V "'ipu2_csi1':2 [fmt:AYUV32/720x480 field:interlaced-tb]"
> (it remains fixed in -bt mode since NTSC is the default). I think we may
> set TB/BT by default (depending on CSI input geometry or TV standard),

Yes, that's what I've implemented. If the user requests an interlaced
field type ('interlaced', 'interlaced-bt', 'interlaced-tb'), but the field
order is not correct given the input height (480=NTSC, 576=PAL),
then the request field type is overridden with the correct field order.

> but it should be possible for the user to explicitly request the field
> order on CSI output (I can make a patch I guess).

If you think that is the correct behavior, I will remove the override
code. I suppose it makes sense to allow user to select field order even
if that order does not make sense given the input standard. I'm fine
either way, Philipp what is your opinion? I'll go with the popular vote :)

Steve

Krzysztof Hałasa May 30, 2018, 6:46 p.m. UTC | #11

Steve Longerbeam <[e-mail]> writes:

>> but it should be possible for the user to explicitly request the field
>> order on CSI output (I can make a patch I guess).
>
> If you think that is the correct behavior, I will remove the override
> code. I suppose it makes sense to allow user to select field order even
> if that order does not make sense given the input standard. I'm fine
> either way, Philipp what is your opinion? I'll go with the popular vote :)

I think it should be up to the user.
Actually, PAL and NTSC aren't valid names in the digital world. Their
meaning ends in the ADV7180 (or equivalent). I don't know if PAL and/or
NTSC specify the field order in the analog frame (meaningful when
someone hooks a camera with progressive sensor and analog, interlaced
output), but the digital YUV422 from ADV to CSI isn't NTSC/PAL anymore.
It's just WxH @ framerate + possible interlacing, sequential fields,
top-bottom or otherwise, etc. The analog standard names could be used
here but just as defaults.

If we were strict (and we don't want to force it), then we should set
NTSC/PAL thing on ADV7180 input, 720x480@29.97i (or 720x576@50i, or
704x... etc) on the input parts of the CSI/IPU (where there are no video
frames yet), and 720x480@29.97i B-T or T-B (or default, or separate
fields - whatever suits the user) on the output from CSI.

I remember the reversed field order was sometimes needed - for example,
PAL DV (the casette camcorder thing) produced B-T frames (same as NTSC),
and to avoid (slight) additional quality loss one had to process it
(up to e.g. .MP4, DVD, and then to HDMI, SCART etc) as B-T.
It wasn't a problem in otherwise-PAL-centric environment.

Steve Longerbeam May 30, 2018, 8:56 p.m. UTC | #12

On 05/30/2018 11:46 AM, Krzysztof Hałasa wrote:
> Steve Longerbeam <[e-mail]> writes:
>
>>> but it should be possible for the user to explicitly request the field
>>> order on CSI output (I can make a patch I guess).
>> If you think that is the correct behavior, I will remove the override
>> code. I suppose it makes sense to allow user to select field order even
>> if that order does not make sense given the input standard. I'm fine
>> either way, Philipp what is your opinion? I'll go with the popular vote :)
> I think it should be up to the user.
> Actually, PAL and NTSC aren't valid names in the digital world. Their
> meaning ends in the ADV7180 (or equivalent). I don't know if PAL and/or
> NTSC specify the field order in the analog frame (meaningful when
> someone hooks a camera with progressive sensor and analog, interlaced
> output), but the digital YUV422 from ADV to CSI isn't NTSC/PAL anymore.
> It's just WxH @ framerate + possible interlacing, sequential fields,
> top-bottom or otherwise, etc. The analog standard names could be used
> here but just as defaults.
>
> If we were strict (and we don't want to force it), then we should set
> NTSC/PAL thing on ADV7180 input, 720x480@29.97i (or 720x576@50i, or
> 704x... etc) on the input parts of the CSI/IPU (where there are no video
> frames yet), and 720x480@29.97i B-T or T-B (or default, or separate
> fields - whatever suits the user) on the output from CSI.
>
> I remember the reversed field order was sometimes needed - for example,
> PAL DV (the casette camcorder thing) produced B-T frames (same as NTSC),
> and to avoid (slight) additional quality loss one had to process it
> (up to e.g. .MP4, DVD, and then to HDMI, SCART etc) as B-T.
> It wasn't a problem in otherwise-PAL-centric environment.

I tend to agree, I've found conflicting info out there regarding
PAL vs. NTSC field order. And I've never liked having to guess
at input analog standard based on input # lines. I will go ahead
and remove the field order override code.

Steve

Philipp Zabel May 31, 2018, 6:29 a.m. UTC | #13

On Wed, May 30, 2018 at 01:56:34PM -0700, Steve Longerbeam wrote:
> 
> 
> On 05/30/2018 11:46 AM, Krzysztof Hałasa wrote:
> > Steve Longerbeam <[e-mail]> writes:
> > 
> > > > but it should be possible for the user to explicitly request the field
> > > > order on CSI output (I can make a patch I guess).
> > > If you think that is the correct behavior, I will remove the override
> > > code. I suppose it makes sense to allow user to select field order even
> > > if that order does not make sense given the input standard. I'm fine
> > > either way, Philipp what is your opinion? I'll go with the popular vote :)
> > I think it should be up to the user.
> > Actually, PAL and NTSC aren't valid names in the digital world. Their
> > meaning ends in the ADV7180 (or equivalent). I don't know if PAL and/or
> > NTSC specify the field order in the analog frame (meaningful when
> > someone hooks a camera with progressive sensor and analog, interlaced
> > output), but the digital YUV422 from ADV to CSI isn't NTSC/PAL anymore.
> > It's just WxH @ framerate + possible interlacing, sequential fields,
> > top-bottom or otherwise, etc. The analog standard names could be used
> > here but just as defaults.
> > 
> > If we were strict (and we don't want to force it), then we should set
> > NTSC/PAL thing on ADV7180 input, 720x480@29.97i (or 720x576@50i, or
> > 704x... etc) on the input parts of the CSI/IPU (where there are no video
> > frames yet), and 720x480@29.97i B-T or T-B (or default, or separate
> > fields - whatever suits the user) on the output from CSI.
> > 
> > I remember the reversed field order was sometimes needed - for example,
> > PAL DV (the casette camcorder thing) produced B-T frames (same as NTSC),
> > and to avoid (slight) additional quality loss one had to process it
> > (up to e.g. .MP4, DVD, and then to HDMI, SCART etc) as B-T.
> > It wasn't a problem in otherwise-PAL-centric environment.
> 
> I tend to agree, I've found conflicting info out there regarding
> PAL vs. NTSC field order. And I've never liked having to guess
> at input analog standard based on input # lines. I will go ahead
> and remove the field order override code.

Note that the code in ipu_csi_init_interface currently hard-codes field
order depending on frame size. It could be that selecting opposite field
order is as easy as switching the relevant parts of writes to registers
CCIR_CODE_2 and _3, but we'd have to pass the desired output field order
to this function. I'd welcome if somebody would verify that this works.

regards
Philipp

Krzysztof Hałasa June 1, 2018, 5:23 a.m. UTC | #14

Philipp Zabel <[e-mail]> writes:

> Note that the code in ipu_csi_init_interface currently hard-codes field
> order depending on frame size. It could be that selecting opposite field
> order is as easy as switching the relevant parts of writes to registers
> CCIR_CODE_2 and _3, but we'd have to pass the desired output field order
> to this function. I'd welcome if somebody would verify that this works.

I can test anything I guess.
Though, in this case, I would be surprised if there is something else
needed. We already do the opposite field order by switching the
CCIR_CODE_[23] values :-)

Krzysztof Hałasa June 1, 2018, 10:02 a.m. UTC | #15

Steve Longerbeam <[e-mail]> writes:

> I tend to agree, I've found conflicting info out there regarding
> PAL vs. NTSC field order. And I've never liked having to guess
> at input analog standard based on input # lines. I will go ahead
> and remove the field order override code.

I've merged your current fix-csi-interlaced.2 branch (2018-06-01
00:06:45 UTC 22ad9f30454b6e46979edf6f8122243591910a3e) along with
"media: adv7180: fix field type" commit and NTSC camera:

media-ctl -V "'adv7180 2-0020':0 [fmt:UYVY2X8/720x480 field:alternate]"
media-ctl -V "'ipu2_csi1_mux':2 [fmt:UYVY2X8/720x480]"
media-ctl -V "'ipu2_csi1':2 [fmt:AYUV32/720x480 field:interlaced/-bt/-tb]"

correctly sets:

"adv7180 2-0020":0 [fmt:UYVY2X8/720x480 field:alternate]
"ipu2_csi1_mux":1  [fmt:UYVY2X8/720x480 field:alternate]
"ipu2_csi1_mux":2  [fmt:UYVY2X8/720x480 field:alternate]
"ipu2_csi1":0      [fmt:UYVY2X8/720x480 field:alternate]
"ipu2_csi1":2      [fmt:AYUV32/720x480 field:interlaced/-bt/-tb]

but all 3 cases seem to produce top-first interlaced frames.
The CCIR_CODE_* register dump shows no differences:
2a38014: 010D07DF 00040596 00FF0000

...it's because the code in drivers/gpu/ipu-v3/ipu-csi.c still sets the
registers depending on the height of the image. Hovewer, I'm using 480
lines here, so it should be B-T instead. My guess is the CSI is skipping
the first incomplete line (half-line - the first visible line has full
length) and BT becomes TB.

It seems writing to the CCIR_CODE_[12] registers does the trick, though
(the captured frames aren't correct and have the lines swapped in pairs
because t/b field pointers aren't correctly set).

Philipp Zabel June 1, 2018, 1:13 p.m. UTC | #16

Hi Krzysztof,

On Fri, 2018-06-01 at 12:02 +0200, Krzysztof Hałasa wrote:
> Steve Longerbeam <[e-mail]> writes:
> 
> > I tend to agree, I've found conflicting info out there regarding
> > PAL vs. NTSC field order. And I've never liked having to guess
> > at input analog standard based on input # lines. I will go ahead
> > and remove the field order override code.
> 
> I've merged your current fix-csi-interlaced.2 branch (2018-06-01
> 00:06:45 UTC 22ad9f30454b6e46979edf6f8122243591910a3e) along with
> "media: adv7180: fix field type" commit and NTSC camera:
> 
> media-ctl -V "'adv7180 2-0020':0 [fmt:UYVY2X8/720x480 field:alternate]"
> media-ctl -V "'ipu2_csi1_mux':2 [fmt:UYVY2X8/720x480]"
> media-ctl -V "'ipu2_csi1':2 [fmt:AYUV32/720x480 field:interlaced/-bt/-tb]"
> 
> correctly sets:
> 
> "adv7180 2-0020":0 [fmt:UYVY2X8/720x480 field:alternate]
> "ipu2_csi1_mux":1  [fmt:UYVY2X8/720x480 field:alternate]
> "ipu2_csi1_mux":2  [fmt:UYVY2X8/720x480 field:alternate]
> "ipu2_csi1":0      [fmt:UYVY2X8/720x480 field:alternate]
> "ipu2_csi1":2      [fmt:AYUV32/720x480 field:interlaced/-bt/-tb]
> 
> but all 3 cases seem to produce top-first interlaced frames.
> The CCIR_CODE_* register dump shows no differences:
> 2a38014: 010D07DF 00040596 00FF0000
> 
> ...it's because the code in drivers/gpu/ipu-v3/ipu-csi.c still sets the
> registers depending on the height of the image.

Exactly.

>  Hovewer, I'm using 480
> lines here, so it should be B-T instead.

My understanding is that the CCIR codes for height == 480 (NTSC)
currently capture the second field (top) first, assuming that for NTSC
the EAV/SAV codes are bottom-field-first.

So the CSI captures SEQ_TB for both PAL and NTSC: The three-bit values
in CCIR_CODE_2/3 are in H,V,F order, and the NTSC case has F=1 for the
field that is captured first, where F=1 is the field that is marked as
second field on the wire, so top. Which means that the captured frame
has two fields captured across frame boundaries, which might be
problematic if the source data was originally progressive.

>  My guess is the CSI is skipping
> the first incomplete line (half-line - the first visible line has full
> length) and BT becomes TB.

That wouldn't make BT TB though, if we'd still capture the bottom field
(minus its first half line) first?

> It seems writing to the CCIR_CODE_[12] registers does the trick, though
> (the captured frames aren't correct and have the lines swapped in pairs
> because t/b field pointers aren't correctly set).

What are you writing exactly? 0x01040596 to CCIR_CODE_1 and 0x000d07df
to CCIR_CODE_2? That is what I would expect to capture SEQ_BT for NTSC
data, and the IPU could interweave this into INTERLACED_BT, correctly if
we fix ipu_cpmem_interlaced_scan to allow negative offsets.

regards
Philipp

Philipp Zabel June 1, 2018, 1:52 p.m. UTC | #17

On Fri, 2018-05-25 at 16:21 -0700, Steve Longerbeam wrote:
> Hi Philipp,
> 
> On 05/24/2018 11:32 PM, Philipp Zabel wrote:
> > On Thu, 2018-05-24 at 11:12 -0700, Steve Longerbeam wrote:
> > [...]
> > > > The following is required as well. Now the question is why we can't skip
> > > > writing those odd UV rows. Anyway, with these 2 changes, I get a stable
> > > > NTSC (and probably PAL) interlaced video stream.
> > > > 
> > > > The manual says: Reduce Double Read or Writes (RDRW):
> > > > This bit is relevant for YUV4:2:0 formats. For write channels:
> > > > U and V components are not written to odd rows.
> > > > 
> > > > How could it be so? With YUV420, are they normally written?
> > > 
> > > Well, given that this bit exists, and assuming I understand it correctly
> > > (1),
> > > I guess the U and V components for odd rows normally are placed on the
> > > AXI bus. Which is a total waste of bus bandwidth because in 4:2:0,
> > > the U and V components are the same for odd and even rows.
> > > 
> > > In other words for writing 4:2:0 to memory, this bit should _always_ be set.
> > > 
> > > (1) OTOH I don't really understand what this bit is trying to say.
> > > Whether this bit is set or not, the data in memory is correct
> > > for planar 4:2:0: y plane buffer followed by U plane of 1/4 size
> > > (decimated by 2 in width and height), followed by Y plane of 1/4
> > > size.
> > > 
> > > So I assume it is saying that the IPU normally places U/V components
> > > on the AXI bus for odd rows, that are identical to the even row values.
> > 
> > Whether they are identical depends on the input format.
> 
> Right, this is the part I was missing, thanks for clarifying. The
> even and odd chroma rows coming into the IDMAC from the
> CSI (or IC) may not be identical if the CSI has captured 4:4:4
> (or 4:2:2 yeah? 4:2:2 is only decimated in width not height).

Oh right, the MEDIA_BUS_FMT_YUYV variants are pretty common, they have
chroma for all the lines. Actually, that is my default test case (1080p
YUYV from TC358743), usually written to NV12 so it can be encoded.

> But still, when the IDMAC has finished pixel packing/unpacking and
> is writing 4:2:0 to memory, it should always skip overwriting the even
> rows with the odd rows, whether or not it has received identical chroma
> even/odd lines from the CSI.
> 
> Unless interweave is enabled :) See below.

Agreed.

regards
Philipp

Steve Longerbeam June 2, 2018, 5:33 p.m. UTC | #18

On 05/30/2018 11:29 PM, Philipp Zabel wrote:
> On Wed, May 30, 2018 at 01:56:34PM -0700, Steve Longerbeam wrote:
>>
>> On 05/30/2018 11:46 AM, Krzysztof Hałasa wrote:
>>> Steve Longerbeam <[e-mail]> writes:
>>>
>>>>> but it should be possible for the user to explicitly request the field
>>>>> order on CSI output (I can make a patch I guess).
>>>> If you think that is the correct behavior, I will remove the override
>>>> code. I suppose it makes sense to allow user to select field order even
>>>> if that order does not make sense given the input standard. I'm fine
>>>> either way, Philipp what is your opinion? I'll go with the popular vote :)
>>> I think it should be up to the user.
>>> Actually, PAL and NTSC aren't valid names in the digital world. Their
>>> meaning ends in the ADV7180 (or equivalent). I don't know if PAL and/or
>>> NTSC specify the field order in the analog frame (meaningful when
>>> someone hooks a camera with progressive sensor and analog, interlaced
>>> output), but the digital YUV422 from ADV to CSI isn't NTSC/PAL anymore.
>>> It's just WxH @ framerate + possible interlacing, sequential fields,
>>> top-bottom or otherwise, etc. The analog standard names could be used
>>> here but just as defaults.
>>>
>>> If we were strict (and we don't want to force it), then we should set
>>> NTSC/PAL thing on ADV7180 input, 720x480@29.97i (or 720x576@50i, or
>>> 704x... etc) on the input parts of the CSI/IPU (where there are no video
>>> frames yet), and 720x480@29.97i B-T or T-B (or default, or separate
>>> fields - whatever suits the user) on the output from CSI.
>>>
>>> I remember the reversed field order was sometimes needed - for example,
>>> PAL DV (the casette camcorder thing) produced B-T frames (same as NTSC),
>>> and to avoid (slight) additional quality loss one had to process it
>>> (up to e.g. .MP4, DVD, and then to HDMI, SCART etc) as B-T.
>>> It wasn't a problem in otherwise-PAL-centric environment.
>> I tend to agree, I've found conflicting info out there regarding
>> PAL vs. NTSC field order. And I've never liked having to guess
>> at input analog standard based on input # lines. I will go ahead
>> and remove the field order override code.
> Note that the code in ipu_csi_init_interface currently hard-codes field
> order depending on frame size. It could be that selecting opposite field
> order is as easy as switching the relevant parts of writes to registers
> CCIR_CODE_2 and _3, but we'd have to pass the desired output field order
> to this function. I'd welcome if somebody would verify that this works.

As I said in the other thread, I think we should put this off to some
other time, and remove the code in ipu_csi_init_interface() that
inverts field order according to frame size. This way, CSI will not
be lying to userspace when we tell it the order is BT but the CSI
has actually inverted that to TB.

Also I have concerns about the CSI capturing field 1 _before_ field
0 for NTSC. Doesn't that mean the CSI will drop the B-field in the
first captured frame on stream on, and thereafter mix fields from
different adjacent frames?

Steve

Krzysztof Hałasa June 4, 2018, 7:06 a.m. UTC | #19

Hi Philipp,

Philipp Zabel <[e-mail]> writes:

> My understanding is that the CCIR codes for height == 480 (NTSC)
> currently capture the second field (top) first, assuming that for NTSC
> the EAV/SAV codes are bottom-field-first.

2a38014: 010D07DF 00040596

        SA  EA         SB  EB  SB  EB
D07DF: 001 101 (0000) 011 111 011 111 (field 0)
40596: 000 100 (0000) 010 110 010 110 (field 1)

The codes apparently are 1=EAV (0=SAV), field#, 1=blanking.
Now BT.656 doesn't say a word about top and bottom fields. There are
just fields 1 and 2. So yes, the CCIR_CODE* registers currently seem to
swap the fields. It also depends on the ADV7180 sending correct codes
based on the even/odd analog fields. Interesting.

> So the CSI captures SEQ_TB for both PAL and NTSC: The three-bit values
> in CCIR_CODE_2/3 are in H,V,F order, and the NTSC case has F=1 for the
> field that is captured first, where F=1 is the field that is marked as
> second field on the wire, so top. Which means that the captured frame
> has two fields captured across frame boundaries, which might be
> problematic if the source data was originally progressive.

Exactly.
Especially if the complete frame is then passed straight to the display,
with the user treating it as progressive (which it isn't anymore).

>>  My guess is the CSI is skipping
>> the first incomplete line (half-line - the first visible line has full
>> length) and BT becomes TB.
>
> That wouldn't make BT TB though, if we'd still capture the bottom field
> (minus its first half line) first?

Well, the entire frame would shift up a line, the bottom "field" would
become top and vice versa. This would effectively make BT->TB and TB->BT.

>> It seems writing to the CCIR_CODE_[12] registers does the trick, though
>> (the captured frames aren't correct and have the lines swapped in pairs
>> because t/b field pointers aren't correctly set).
>
> What are you writing exactly? 0x01040596 to CCIR_CODE_1 and 0x000d07df
> to CCIR_CODE_2?

Yes.

> That is what I would expect to capture SEQ_BT for NTSC
> data, and the IPU could interweave this into INTERLACED_BT, correctly if
> we fix ipu_cpmem_interlaced_scan to allow negative offsets.

Exactly.

Philipp Zabel June 4, 2018, 8:38 a.m. UTC | #20

On Sat, 2018-06-02 at 10:33 -0700, Steve Longerbeam wrote:
[...]
> As I said in the other thread, I think we should put this off to some
> other time, and remove the code in ipu_csi_init_interface() that
> inverts field order according to frame size. This way, CSI will not
> be lying to userspace when we tell it the order is BT but the CSI
> has actually inverted that to TB.
> 
> Also I have concerns about the CSI capturing field 1 _before_ field
> 0 for NTSC. Doesn't that mean the CSI will drop the B-field in the
> first captured frame on stream on, and thereafter mix fields from
> different adjacent frames?

Yes, that is only a problem for 29.97 Hz progressive source material.
For real 59.94 Hz interlaced source material it does not matter which
two consecutive fields are displayed together as long as we get the
top/bottom ordering right.

regards
Philipp

Patch

--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -474,8 +474,8 @@  static int csi_idmac_setup_channel(struct csi_priv *priv)
 
 	ipu_smfc_set_burstsize(priv->smfc, burst_size);
 
-	if (image.pix.field == V4L2_FIELD_NONE &&
-	    V4L2_FIELD_HAS_BOTH(infmt->field))
+	if (1 || (image.pix.field == V4L2_FIELD_NONE &&
+		  V4L2_FIELD_HAS_BOTH(infmt->field)))
 		ipu_cpmem_interlaced_scan(priv->idmac_ch,
 					  image.pix.bytesperline);
 

I.e., I need to set CPMEM to interlaced mode when I operate CSI in
interlaced mode. The original code is a bit unclear to me in fact.

The following is required as well. Now the question is why we can't skip
writing those odd UV rows. Anyway, with these 2 changes, I get a stable
NTSC (and probably PAL) interlaced video stream.

The manual says: Reduce Double Read or Writes (RDRW):
This bit is relevant for YUV4:2:0 formats. For write channels:
U and V components are not written to odd rows.

How could it be so? With YUV420, are they normally written?
OTOH it seems that not only UV is broken with this bit set.
Y is broken as well.

--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -413,14 +413,12 @@  static int csi_idmac_setup_channel(struct csi_priv *priv)
 		passthrough_bits = 16;
 		break;
 	case V4L2_PIX_FMT_YUV420:
 	case V4L2_PIX_FMT_NV12:
 		burst_size = (image.pix.width & 0x3f) ?
 			     ((image.pix.width & 0x1f) ?
 			      ((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);
 		break;
 	case V4L2_PIX_FMT_YUYV:
 	case V4L2_PIX_FMT_UYVY: