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

[Tim Harvey <tharvey@gateworks.com>]

On Tue, Jan 22, 2019 at 4:10 PM Steve Longerbeam <slongerbeam@gmail.com> wrote:
>
>
>
> On 1/21/19 12:24 PM, Tim Harvey wrote:
> > On Tue, Jan 15, 2019 at 3:54 PM Steve Longerbeam <slongerbeam@gmail.com> wrote:
> >> Hi Tim,
> >>
> >> On 1/15/19 1:58 PM, Tim Harvey wrote:
> >>> On Wed, Jan 9, 2019 at 10:30 AM Steve Longerbeam <slongerbeam@gmail.com> wrote:
> >>>> Also add an example pipeline for unconverted capture with interweave
> >>>> on SabreAuto.
> >>>>
> >>>> Cleanup some language in various places in the process.
> >>>>
> >>>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
> >>>> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
> >>>> ---
> >>>> Changes since v4:
> >>>> - Make clear that it is IDMAC channel that does pixel reordering and
> >>>>     interweave, not the CSI. Caught by Philipp Zabel.
> >>>> Changes since v3:
> >>>> - none.
> >>>> Changes since v2:
> >>>> - expand on idmac interweave behavior in CSI subdev.
> >>>> - switch second SabreAuto pipeline example to PAL to give
> >>>>     both NTSC and PAL examples.
> >>>> - Cleanup some language in various places.
> >>>> ---
> >>>>    Documentation/media/v4l-drivers/imx.rst | 103 +++++++++++++++---------
> >>>>    1 file changed, 66 insertions(+), 37 deletions(-)
> >>>>
> >>> <snip>
> >>>>    Capture Pipelines
> >>>>    -----------------
> >>>> @@ -516,10 +522,33 @@ 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, 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:
> >>>> +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-bt' for
> >>>> +NTSC, 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]"
> >>>> +   # Configure "ipu1_csi0 capture" interface (assumed at /dev/video4)
> >>>> +   v4l2-ctl -d4 --set-fmt-video=field=interlaced_bt
> >>>> +
> >>>> +Streaming can then begin on /dev/video4. The v4l2-ctl tool can also be
> >>>> +used to select any supported YUV pixelformat on /dev/video4.
> >>>> +
> >>> Hi Steve,
> >>>
> >>> I'm testing 4.20 with this patchset on top.
> >>>
> >>> I'm on a GW5104 which has an IMX6Q with the adv7180 on ipu1_csi0 like
> >>> the SabeAuto example above I can't get the simple interveave example
> >>> to work:
> >>>
> >>> media-ctl -r # reset all links
> >>> # Setup links (ADV7180 IPU1_CSI0)
> >>> media-ctl -l '"adv7180 2-0020":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]' # /dev/video4
> >>> # Configure pads
> >>> media-ctl -V "'adv7180 2-0020':0 [fmt:UYVY2X8/720x480 field:seq-bt]"
> >>> media-ctl -V "'ipu1_csi0_mux':2 [fmt:UYVY2X8/720x480]"
> >>> media-ctl -V "'ipu1_csi0':0 [fmt:AYUV32/720x480]"
> >> This is the reason. The adv7180 is only allowing to configure alternate
> >> field mode, and thus it reports the field height on the mbus, not the
> >> full frame height. Imx deals with alternate field mode by capturing a
> >> full frame, so the CSI entity sets the output pad height to double the
> >> height.
> >>
> >> So the CSI input pad needs to be configured with the field height:
> >>
> >> media-ctl -V "'ipu1_csi0':0 [fmt:AYUV32/720x240]"
> >>
> >> It should work for you after doing that. And better yet, don't bother
> >> configuring the input pad, because media-ctl will propagate formats from
> >> source to sink pads for you, so it's better to rely on the propagation,
> >> and set the CSI output pad format instead (full frame height at output pad):
> >>
> >> media-ctl -V "'ipu1_csi0':2 [fmt:AYUV32/720x480]"
> >>
> > Steve,
> >
> > Thanks - that makes sense.
> >
> > I also noticed that if I setup one of the vdic pipelines first then
> > went back after a 'media-ctl -r' and setup the example that failed it
> > no longer failed. I'm thinking that this is because 'media-ctl -r'
> > make reset all the links but does not reset all the V4L2 formats on
> > pads?
> >
> >> Final note: the imx.rst doc is technically correct even though it is
> >> showing full frame heights being configured at the pads, because it is
> >> expecting the adv7180 has accepted 'seq-bt'. But even the example given
> >> in that doc works for alternate field mode, because the pad heights are
> >> forced to the correct field height for alternate mode.
> >>
> > hmmm... I don't quite follow this statement. It sounds like the
> > example would only be correct if you were setting 'field:alternate'
> > but the example sets 'field:seq-bt' instead.
>
> The example is consistent for a sensor that sends seq-bt. Here is the
> example config from the imx.rst doc again, a (ntsc) height of 480 lines
> is correct for a seq-bt source:
>
>     # 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]"
>     # Configure "ipu1_csi0 capture" interface (assumed at /dev/video4)
>     v4l2-ctl -d4 --set-fmt-video=field=interlaced_bt
>
> > I wonder if you should add some verbiage explaining the difference in
> > format (resolution specifically) between the input and output pads
> > and/or change the example to set the output pad format so people don't
> > run into what I did trying to follow the example.
> >
>
> Well, the example *is* setting the output pad format (media-ctl -V
> "ipu1_csi0:2 ...").
>
> But I suppose wording could be added such as "this example assumes the
> sensor (adv7180) supports seq-bt".
>

Steve,

Your absolutely right - my usage which set the input pad wasn't even
following your example (by accident) so your example is good and makes
sense.

Your explanation for the failure of linking vdic->ic_prp->ic_prpenc in
the previous message makes sense also.

I'm testing again with the tda1997x HDMI receiver which provides a
wide variety of input's to test IMX6 capture with (resolutions,
progressive vs interlaced, bt656 fmt vs yuvsmp bus format) and running
into some issues but I'll post them in a new thread.

This series looks good and does fix several interlaced capture issues.

Acked-by: Tim Harvey <tharvey@gateworks.com>

Tested on GW5104/GW5404 with adv7180

Tested-by: Tim Harvey <tharvey@gateowrks.com>

Thanks,

Tim