Message ID | m3h8mxqc7t.fsf@t19.piap.pl (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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:
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
Steve Longerbeam <slongerbeam@gmail.com> 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,
Philipp Zabel <p.zabel@pengutronix.de> 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).
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
Hi Krzysztof, Philipp, On 05/25/2018 12:18 AM, Krzysztof Hałasa wrote: > Philipp Zabel <p.zabel@pengutronix.de> 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: git@github.com:slongerbeam/mediatree.git, branch 'fix-csi-interlaced' Steve
Hi Steve, Steve Longerbeam <slongerbeam@gmail.com> writes: > Krzysztof, in the meantime the patches are available in my > media-tree fork, for testing on the Ventana GW5300: > > git@github.com: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.
Hi Krzysztof, On 05/29/2018 12:26 AM, Krzysztof Hałasa wrote: > Hi Steve, > > Steve Longerbeam <slongerbeam@gmail.com> writes: > >> Krzysztof, in the meantime the patches are available in my >> media-tree fork, for testing on the Ventana GW5300: >> >> git@github.com: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
Steve Longerbeam <slongerbeam@gmail.com> 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).
Hi Krzysztof, On 05/30/2018 01:53 AM, Krzysztof Hałasa wrote: > Steve Longerbeam <slongerbeam@gmail.com> 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
Steve Longerbeam <slongerbeam@gmail.com> 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.
On 05/30/2018 11:46 AM, Krzysztof Hałasa wrote: > Steve Longerbeam <slongerbeam@gmail.com> 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
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 <slongerbeam@gmail.com> 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
Philipp Zabel <pza@pengutronix.de> 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 :-)
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. 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).
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. > 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
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
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 <slongerbeam@gmail.com> 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
Hi Philipp, Philipp Zabel <p.zabel@pengutronix.de> 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.
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
--- 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: