Re: i.MX6 IPU CSI analog video input on Ventana

[Steve Longerbeam <slongerbeam@gmail.com>]

On 06/01/2018 06:13 AM, Philipp Zabel wrote:
> Hi Krzysztof,
>
> On Fri, 2018-06-01 at 12:02 +0200, Krzysztof Hałasa wrote:
>> Steve Longerbeam <slongerbeam@gmail.com> 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.

I agree, for NTSC the CSI will drop the first B field and start capturing
at the T field, and then capture fields across frame boundaries. At
least, that is if we understand how these CCIR registers work: the
CSI will look for H-S-V codes for the start and end of active and blanking
lines, that match the codes written to CCIR_CODE_1/2 for fields 0/1.

I think this must be legacy code from a Freescale BSP requirement
that the CSI must always capture in T-B order. We should remove this
code, so that the CSI always captures field 0 followed by field 1, 
irrespective
of field affinity, as in:

diff --git a/drivers/gpu/ipu-v3/ipu-csi.c b/drivers/gpu/ipu-v3/ipu-csi.c
index 5450a2d..b8b9b6d 100644
--- a/drivers/gpu/ipu-v3/ipu-csi.c
+++ b/drivers/gpu/ipu-v3/ipu-csi.c
@@ -398,41 +398,20 @@ int ipu_csi_init_interface(struct ipu_csi *csi,
                 break;
         case IPU_CSI_CLK_MODE_CCIR656_INTERLACED:
                 if (mbus_fmt->width == 720 && mbus_fmt->height == 576) {
-                       /*
-                        * PAL case
-                        *
-                        * Field0BlankEnd = 0x6, Field0BlankStart = 0x2,
-                        * Field0ActiveEnd = 0x4, Field0ActiveStart = 0
-                        * Field1BlankEnd = 0x7, Field1BlankStart = 0x3,
-                        * Field1ActiveEnd = 0x5, Field1ActiveStart = 0x1
-                        */
                         height = 625; /* framelines for PAL */
-
-                       ipu_csi_write(csi, 0x40596 | CSI_CCIR_ERR_DET_EN,
-                                         CSI_CCIR_CODE_1);
-                       ipu_csi_write(csi, 0xD07DF, CSI_CCIR_CODE_2);
-                       ipu_csi_write(csi, 0xFF0000, CSI_CCIR_CODE_3);
                 } else if (mbus_fmt->width == 720 && mbus_fmt->height 
== 480) {
-                       /*
-                        * NTSC case
-                        *
-                        * Field0BlankEnd = 0x7, Field0BlankStart = 0x3,
-                        * Field0ActiveEnd = 0x5, Field0ActiveStart = 0x1
-                        * Field1BlankEnd = 0x6, Field1BlankStart = 0x2,
-                        * Field1ActiveEnd = 0x4, Field1ActiveStart = 0
-                        */
                         height = 525; /* framelines for NTSC */
-
-                       ipu_csi_write(csi, 0xD07DF | CSI_CCIR_ERR_DET_EN,
-                                         CSI_CCIR_CODE_1);
-                       ipu_csi_write(csi, 0x40596, CSI_CCIR_CODE_2);
-                       ipu_csi_write(csi, 0xFF0000, CSI_CCIR_CODE_3);
                 } else {
                         dev_err(csi->ipu->dev,
                                 "Unsupported CCIR656 interlaced video 
mode\n");
                         spin_unlock_irqrestore(&csi->lock, flags);
                         return -EINVAL;
                 }
+
+               ipu_csi_write(csi, 0x40596 | CSI_CCIR_ERR_DET_EN,
+                             CSI_CCIR_CODE_1);
+               ipu_csi_write(csi, 0xD07DF, CSI_CCIR_CODE_2);
+               ipu_csi_write(csi, 0xFF0000, CSI_CCIR_CODE_3);
                 break;
         case IPU_CSI_CLK_MODE_CCIR1120_PROGRESSIVE_DDR:
         case IPU_CSI_CLK_MODE_CCIR1120_PROGRESSIVE_SDR: