[PATCH] gpu: ipu-v3: Allow negative offsets for interlaced scanning

[PATCH] gpu: ipu-v3: Allow negative offsets for interlaced scanning

[Philipp Zabel]

The IPU also supports interlaced buffers that start with the bottom field.
To achieve this, the the base address EBA has to be increased by a stride
length and the interlace offset ILO has to be set to the negative stride.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/ipu-v3/ipu-cpmem.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/ipu-v3/ipu-cpmem.c b/drivers/gpu/ipu-v3/ipu-cpmem.c
index 9f2d9ec42add..c1028f38c553 100644
--- a/drivers/gpu/ipu-v3/ipu-cpmem.c
+++ b/drivers/gpu/ipu-v3/ipu-cpmem.c
@@ -269,8 +269,14 @@ EXPORT_SYMBOL_GPL(ipu_cpmem_set_uv_offset);
 
 void ipu_cpmem_interlaced_scan(struct ipuv3_channel *ch, int stride)
 {
+	u32 ilo;
+
 	ipu_ch_param_write_field(ch, IPU_FIELD_SO, 1);
-	ipu_ch_param_write_field(ch, IPU_FIELD_ILO, stride / 8);
+	if (stride >= 0)
+		ilo = stride / 8;
+	else
+		ilo = 0x100000 - (-stride / 8);
+	ipu_ch_param_write_field(ch, IPU_FIELD_ILO, ilo);
 	ipu_ch_param_write_field(ch, IPU_FIELD_SLY, (stride * 2) - 1);
 };
 EXPORT_SYMBOL_GPL(ipu_cpmem_interlaced_scan);
-- 
2.17.1

Re: [PATCH] gpu: ipu-v3: Allow negative offsets for interlaced scanning

[Steve Longerbeam]

Hi Philipp,


On 06/01/2018 06:13 AM, Philipp Zabel wrote:
> The IPU also supports interlaced buffers that start with the bottom field.
> To achieve this, the the base address EBA has to be increased by a stride
> length and the interlace offset ILO has to be set to the negative stride.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>   drivers/gpu/ipu-v3/ipu-cpmem.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/ipu-v3/ipu-cpmem.c b/drivers/gpu/ipu-v3/ipu-cpmem.c
> index 9f2d9ec42add..c1028f38c553 100644
> --- a/drivers/gpu/ipu-v3/ipu-cpmem.c
> +++ b/drivers/gpu/ipu-v3/ipu-cpmem.c
> @@ -269,8 +269,14 @@ EXPORT_SYMBOL_GPL(ipu_cpmem_set_uv_offset);
>   
>   void ipu_cpmem_interlaced_scan(struct ipuv3_channel *ch, int stride)
>   {
> +	u32 ilo;
> +
>   	ipu_ch_param_write_field(ch, IPU_FIELD_SO, 1);
> -	ipu_ch_param_write_field(ch, IPU_FIELD_ILO, stride / 8);
> +	if (stride >= 0)
> +		ilo = stride / 8;
> +	else
> +		ilo = 0x100000 - (-stride / 8);
> +	ipu_ch_param_write_field(ch, IPU_FIELD_ILO, ilo);
>   	ipu_ch_param_write_field(ch, IPU_FIELD_SLY, (stride * 2) - 1);

This should be "(-stride * 2) - 1" for SLY when stride is negative.

After fixing that, interweaving seq-bt -> interlaced-bt works fine for
packed pixel formats, but there is still some problem for planar.

I haven't tracked down the issue with planar yet, but the corresponding
changes to imx-media that allow interweaving with line swapping are at

e9dd81da20 ("media: imx: Allow interweave with top/bottom lines swapped")

in branch fix-csi-interlaced.3 in my media-tree fork on github. Please 
have a
look and let me know if you see anything obvious.

Steve

Re: [PATCH] gpu: ipu-v3: Allow negative offsets for interlaced scanning

[Philipp Zabel]

Hi Steve,

On Sun, 2018-06-10 at 17:08 -0700, Steve Longerbeam wrote:
> Hi Philipp,
> 
> On 06/01/2018 06:13 AM, Philipp Zabel wrote:
> > The IPU also supports interlaced buffers that start with the bottom field.
> > To achieve this, the the base address EBA has to be increased by a stride
> > length and the interlace offset ILO has to be set to the negative stride.
> > 
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >   drivers/gpu/ipu-v3/ipu-cpmem.c | 8 +++++++-
> >   1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/ipu-v3/ipu-cpmem.c b/drivers/gpu/ipu-v3/ipu-cpmem.c
> > index 9f2d9ec42add..c1028f38c553 100644
> > --- a/drivers/gpu/ipu-v3/ipu-cpmem.c
> > +++ b/drivers/gpu/ipu-v3/ipu-cpmem.c
> > @@ -269,8 +269,14 @@ EXPORT_SYMBOL_GPL(ipu_cpmem_set_uv_offset);
> >   
> >   void ipu_cpmem_interlaced_scan(struct ipuv3_channel *ch, int stride)
> >   {
> > +	u32 ilo;
> > +
> >   	ipu_ch_param_write_field(ch, IPU_FIELD_SO, 1);
> > -	ipu_ch_param_write_field(ch, IPU_FIELD_ILO, stride / 8);
> > +	if (stride >= 0)
> > +		ilo = stride / 8;
> > +	else
> > +		ilo = 0x100000 - (-stride / 8);
> > +	ipu_ch_param_write_field(ch, IPU_FIELD_ILO, ilo);
> >   	ipu_ch_param_write_field(ch, IPU_FIELD_SLY, (stride * 2) - 1);
> 
> This should be "(-stride * 2) - 1" for SLY when stride is negative.
> 
> After fixing that, interweaving seq-bt -> interlaced-bt works fine for
> packed pixel formats,

Ouch, thanks.

>  but there is still some problem for planar.
> I haven't tracked down the issue with planar yet,

Just with the negative stride line? Only for YUV420 or also for NV12?
The problem could be that we also have to change UBO/VBO for planar
formats with a chroma stride (SLUV) that is half the luma stride (SLY)
when we move both Y and U/V frame start by a line length.

>  but the corresponding
> changes to imx-media that allow interweaving with line swapping are at
> 
> e9dd81da20 ("media: imx: Allow interweave with top/bottom lines swapped")
> 
> in branch fix-csi-interlaced.3 in my media-tree fork on github. Please 
> have a
> look and let me know if you see anything obvious.

In commit a19126a80d13 ("gpu: ipu-csi: Swap fields according to
input/output field types"), swap_fields = true is only set for
seq-bt -> seq-tb and seq-tb -> seq-bt. I think it should also be
enabled for alternate -> seq-bt (PAL) and alternate -> seq-tb (NTSC).

I've been made aware [1] that recently V4L2_FIELD_ALTERNATE has been
clarified [2] to specify that v4l2_mbus_fmt.height should contain the
number of lines per field, not per frame:

[1] https://patchwork.linuxtv.org/patch/50166/
[2] 0018147c964e ("media: v4l: doc: Clarify v4l2_mbus_fmt height
    definition")

regards
Philipp

Re: [PATCH] gpu: ipu-v3: Allow negative offsets for interlaced scanning

[Steve Longerbeam]

On 06/11/2018 02:19 AM, Philipp Zabel wrote:
> Hi Steve,
>
> On Sun, 2018-06-10 at 17:08 -0700, Steve Longerbeam wrote:
>> Hi Philipp,
>>
>> On 06/01/2018 06:13 AM, Philipp Zabel wrote:
>>> The IPU also supports interlaced buffers that start with the bottom field.
>>> To achieve this, the the base address EBA has to be increased by a stride
>>> length and the interlace offset ILO has to be set to the negative stride.
>>>
>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>> ---
>>>    drivers/gpu/ipu-v3/ipu-cpmem.c | 8 +++++++-
>>>    1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/ipu-v3/ipu-cpmem.c b/drivers/gpu/ipu-v3/ipu-cpmem.c
>>> index 9f2d9ec42add..c1028f38c553 100644
>>> --- a/drivers/gpu/ipu-v3/ipu-cpmem.c
>>> +++ b/drivers/gpu/ipu-v3/ipu-cpmem.c
>>> @@ -269,8 +269,14 @@ EXPORT_SYMBOL_GPL(ipu_cpmem_set_uv_offset);
>>>    
>>>    void ipu_cpmem_interlaced_scan(struct ipuv3_channel *ch, int stride)
>>>    {
>>> +	u32 ilo;
>>> +
>>>    	ipu_ch_param_write_field(ch, IPU_FIELD_SO, 1);
>>> -	ipu_ch_param_write_field(ch, IPU_FIELD_ILO, stride / 8);
>>> +	if (stride >= 0)
>>> +		ilo = stride / 8;
>>> +	else
>>> +		ilo = 0x100000 - (-stride / 8);
>>> +	ipu_ch_param_write_field(ch, IPU_FIELD_ILO, ilo);
>>>    	ipu_ch_param_write_field(ch, IPU_FIELD_SLY, (stride * 2) - 1);
>> This should be "(-stride * 2) - 1" for SLY when stride is negative.
>>
>> After fixing that, interweaving seq-bt -> interlaced-bt works fine for
>> packed pixel formats,
> Ouch, thanks.
>
>>   but there is still some problem for planar.
>> I haven't tracked down the issue with planar yet,
> Just with the negative stride line?

Correct, planar is broken (bad colors in captured frames) when
negative stride is enabled for interweave. Planar works fine otherwise.

>   Only for YUV420 or also for NV12?

I tested YV12 (three planes YUV420). I can't remember if I tested
NV12 (two planes). I'm currently not able to test as I'm away from
the hardware but I will try on Wednesday.

> The problem could be that we also have to change UBO/VBO for planar
> formats with a chroma stride (SLUV) that is half the luma stride (SLY)
> when we move both Y and U/V frame start by a line length.

Right, and I think I am doing that, by setting image.rect.top = 1
before calling ipu_cpmem_set_image(). And when dequeuing a
new v4l2_buffer, I am adding one luma stride to the buffer address
when calling ipu_cpmem_set_buffer() (grep for interweave_offset).

>
>>   but the corresponding
>> changes to imx-media that allow interweaving with line swapping are at
>>
>> e9dd81da20 ("media: imx: Allow interweave with top/bottom lines swapped")
>>
>> in branch fix-csi-interlaced.3 in my media-tree fork on github. Please
>> have a
>> look and let me know if you see anything obvious.
> In commit a19126a80d13 ("gpu: ipu-csi: Swap fields according to
> input/output field types"), swap_fields = true is only set for
> seq-bt -> seq-tb and seq-tb -> seq-bt. I think it should also be
> enabled for alternate -> seq-bt (PAL) and alternate -> seq-tb (NTSC).

It is, see ipu_csi_translate_field() -- it will translate alternate
to seq-bt or seq-tb before determining swap_fields.


>
> I've been made aware [1] that recently V4L2_FIELD_ALTERNATE has been
> clarified [2] to specify that v4l2_mbus_fmt.height should contain the
> number of lines per field, not per frame:

Yep! That was nagging at me as well. I noticed at least one other
platform (omap3isp) that doubles the source pad frame height
when the sensor reports ALTERNATE field mode, to capture a
whole frame. Makes sense. I think the crop height will need to
be doubled from the sink height in imx-media-csi.c to support
ALTERNATE. That also means sink height can't be used to
guess at input video standard (480 becomes 240 for NTSC).

Steve

>
> [1] https://patchwork.linuxtv.org/patch/50166/
> [2] 0018147c964e ("media: v4l: doc: Clarify v4l2_mbus_fmt height
>      definition")
>
> regards
> Philipp

Re: [PATCH] gpu: ipu-v3: Allow negative offsets for interlaced scanning

[Philipp Zabel]

On Mon, 2018-06-11 at 14:06 -0700, Steve Longerbeam wrote:
> 
> On 06/11/2018 02:19 AM, Philipp Zabel wrote:
> > Hi Steve,
> > 
> > On Sun, 2018-06-10 at 17:08 -0700, Steve Longerbeam wrote:
> > > Hi Philipp,
> > > 
> > > On 06/01/2018 06:13 AM, Philipp Zabel wrote:
> > > > The IPU also supports interlaced buffers that start with the bottom field.
> > > > To achieve this, the the base address EBA has to be increased by a stride
> > > > length and the interlace offset ILO has to be set to the negative stride.
> > > > 
> > > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > > ---
> > > >    drivers/gpu/ipu-v3/ipu-cpmem.c | 8 +++++++-
> > > >    1 file changed, 7 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/ipu-v3/ipu-cpmem.c b/drivers/gpu/ipu-v3/ipu-cpmem.c
> > > > index 9f2d9ec42add..c1028f38c553 100644
> > > > --- a/drivers/gpu/ipu-v3/ipu-cpmem.c
> > > > +++ b/drivers/gpu/ipu-v3/ipu-cpmem.c
> > > > @@ -269,8 +269,14 @@ EXPORT_SYMBOL_GPL(ipu_cpmem_set_uv_offset);
> > > >    
> > > >    void ipu_cpmem_interlaced_scan(struct ipuv3_channel *ch, int stride)
> > > >    {
> > > > +	u32 ilo;
> > > > +
> > > >    	ipu_ch_param_write_field(ch, IPU_FIELD_SO, 1);
> > > > -	ipu_ch_param_write_field(ch, IPU_FIELD_ILO, stride / 8);
> > > > +	if (stride >= 0)
> > > > +		ilo = stride / 8;
> > > > +	else
> > > > +		ilo = 0x100000 - (-stride / 8);
> > > > +	ipu_ch_param_write_field(ch, IPU_FIELD_ILO, ilo);
> > > >    	ipu_ch_param_write_field(ch, IPU_FIELD_SLY, (stride * 2) - 1);
> > > 
> > > This should be "(-stride * 2) - 1" for SLY when stride is negative.
> > > 
> > > After fixing that, interweaving seq-bt -> interlaced-bt works fine for
> > > packed pixel formats,
> > 
> > Ouch, thanks.
> > 
> > >   but there is still some problem for planar.
> > > I haven't tracked down the issue with planar yet,
> > 
> > Just with the negative stride line?
> 
> Correct, planar is broken (bad colors in captured frames) when
> negative stride is enabled for interweave. Planar works fine otherwise.
> 
> >   Only for YUV420 or also for NV12?
> 
> I tested YV12 (three planes YUV420). I can't remember if I tested
> NV12 (two planes). I'm currently not able to test as I'm away from
> the hardware but I will try on Wednesday.
> 
> > The problem could be that we also have to change UBO/VBO for planar
> > formats with a chroma stride (SLUV) that is half the luma stride (SLY)
> > when we move both Y and U/V frame start by a line length.
> 
> Right, and I think I am doing that, by setting image.rect.top = 1
> before calling ipu_cpmem_set_image(). And when dequeuing a
> new v4l2_buffer, I am adding one luma stride to the buffer address
> when calling ipu_cpmem_set_buffer() (grep for interweave_offset).

Oh, ok. Yes, that looks superficially correct, if a bit intransparent.

> > >   but the corresponding
> > > changes to imx-media that allow interweaving with line swapping are at
> > > 
> > > e9dd81da20 ("media: imx: Allow interweave with top/bottom lines swapped")
> > > 
> > > in branch fix-csi-interlaced.3 in my media-tree fork on github. Please
> > > have a
> > > look and let me know if you see anything obvious.
> > 
> > In commit a19126a80d13 ("gpu: ipu-csi: Swap fields according to
> > input/output field types"), swap_fields = true is only set for
> > seq-bt -> seq-tb and seq-tb -> seq-bt. I think it should also be
> > enabled for alternate -> seq-bt (PAL) and alternate -> seq-tb (NTSC).
> 
> It is, see ipu_csi_translate_field() -- it will translate alternate
> to seq-bt or seq-tb before determining swap_fields.

Right, I missed that too.

> > 
> > I've been made aware [1] that recently V4L2_FIELD_ALTERNATE has been
> > clarified [2] to specify that v4l2_mbus_fmt.height should contain the
> > number of lines per field, not per frame:
> 
> Yep! That was nagging at me as well. I noticed at least one other
> platform (omap3isp) that doubles the source pad frame height
> when the sensor reports ALTERNATE field mode, to capture a
> whole frame. Makes sense. I think the crop height will need to
> be doubled from the sink height in imx-media-csi.c to support
> ALTERNATE.

Yes.

> That also means sink height can't be used to
> guess at input video standard (480 becomes 240 for NTSC).

Well, the v4l2_std_id heuristic in ipu_csi_set_bt_interlaced_codes just
needs to check infmt->field == ALTERNATE.

regards
Philipp

Re: [PATCH] gpu: ipu-v3: Allow negative offsets for interlaced scanning

[Javier Martinez Canillas]

Hi Steve,

On 06/11/2018 11:06 PM, Steve Longerbeam wrote:

[snip]

>>
>> I've been made aware [1] that recently V4L2_FIELD_ALTERNATE has been
>> clarified [2] to specify that v4l2_mbus_fmt.height should contain the
>> number of lines per field, not per frame:
> 
> Yep! That was nagging at me as well. I noticed at least one other
> platform (omap3isp) that doubles the source pad frame height

Coincidentally I noticed this problem when testing on a board with an
omap3isp. This is the pipeline setup I've been using for a long time: 

$ media-ctl -l '"tvp5150 1-005c":1->"OMAP3 ISP CCDC":0[1]'
$ media-ctl -l '"OMAP3 ISP CCDC":1->"OMAP3 ISP CCDC output":0[1]'
$ media-ctl -V '"OMAP3 ISP CCDC":0 [UYVY2X8 720x240 field:alternate]'
$ media-ctl -V '"OMAP3 ISP CCDC":1 [UYVY 720x480 field:interlaced-tb]'

> when the sensor reports ALTERNATE field mode, to capture a
> whole frame. Makes sense. I think the crop height will need to

As you said, the ISP doubles the source pad height, and so the sink
pad is meant to have half of the frame height and this should match
the camera sensor height. But since the tvp5150 had the full frame
height (720x480) in its source pad, this didn't match the CCDC sink
pads height which lead to .link_validate callback to return -EPIPE:

ioctl(3, VIDIOC_STREAMON, 0xbeabea18)   = -1 EPIPE (Broken pipe)

After the revert, link validation / STREAMON works correctly and the
following is what the relevant media entities look like in the graph:

- entity 15: OMAP3 ISP CCDC (3 pads, 9 links)
             type V4L2 subdev subtype Unknown flags 0
             device node name /dev/v4l-subdev2
        pad0: Sink
                [fmt:UYVY2X8/720x240 field:alternate]
                <- "OMAP3 ISP CCP2":1 []
                <- "OMAP3 ISP CSI2a":1 []
                <- "tvp5150 1-005c":1 [ENABLED]
        pad1: Source
                [fmt:UYVY/720x480 field:interlaced-tb
                 crop.bounds:(0,0)/720x240
                 crop:(0,0)/720x240]
                -> "OMAP3 ISP CCDC output":0 [ENABLED]
                -> "OMAP3 ISP resizer":0 []

- entity 81: tvp5150 1-005c (4 pads, 1 link)
             type V4L2 subdev subtype Decoder flags 0
             device node name /dev/v4l-subdev8
        pad0: Sink
        pad1: Source
                [fmt:UYVY2X8/720x240 field:alternate
                 crop.bounds:(0,0)/720x480
                 crop:(0,0)/720x480]
                -> "OMAP3 ISP CCDC":0 [ENABLED]

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Re: [PATCH] gpu: ipu-v3: Allow negative offsets for interlaced scanning

[Steve Longerbeam]

Hi Javier, thanks for the confirmations. I'm working on a
fix for imx-media.

Steve


On 06/12/2018 10:27 AM, Javier Martinez Canillas wrote:
> Hi Steve,
>
> On 06/11/2018 11:06 PM, Steve Longerbeam wrote:
>
> [snip]
>
>>> I've been made aware [1] that recently V4L2_FIELD_ALTERNATE has been
>>> clarified [2] to specify that v4l2_mbus_fmt.height should contain the
>>> number of lines per field, not per frame:
>> Yep! That was nagging at me as well. I noticed at least one other
>> platform (omap3isp) that doubles the source pad frame height
> Coincidentally I noticed this problem when testing on a board with an
> omap3isp. This is the pipeline setup I've been using for a long time:
>
> $ media-ctl -l '"tvp5150 1-005c":1->"OMAP3 ISP CCDC":0[1]'
> $ media-ctl -l '"OMAP3 ISP CCDC":1->"OMAP3 ISP CCDC output":0[1]'
> $ media-ctl -V '"OMAP3 ISP CCDC":0 [UYVY2X8 720x240 field:alternate]'
> $ media-ctl -V '"OMAP3 ISP CCDC":1 [UYVY 720x480 field:interlaced-tb]'
>
>> when the sensor reports ALTERNATE field mode, to capture a
>> whole frame. Makes sense. I think the crop height will need to
> As you said, the ISP doubles the source pad height, and so the sink
> pad is meant to have half of the frame height and this should match
> the camera sensor height. But since the tvp5150 had the full frame
> height (720x480) in its source pad, this didn't match the CCDC sink
> pads height which lead to .link_validate callback to return -EPIPE:
>
> ioctl(3, VIDIOC_STREAMON, 0xbeabea18)   = -1 EPIPE (Broken pipe)
>
> After the revert, link validation / STREAMON works correctly and the
> following is what the relevant media entities look like in the graph:
>
> - entity 15: OMAP3 ISP CCDC (3 pads, 9 links)
>               type V4L2 subdev subtype Unknown flags 0
>               device node name /dev/v4l-subdev2
>          pad0: Sink
>                  [fmt:UYVY2X8/720x240 field:alternate]
>                  <- "OMAP3 ISP CCP2":1 []
>                  <- "OMAP3 ISP CSI2a":1 []
>                  <- "tvp5150 1-005c":1 [ENABLED]
>          pad1: Source
>                  [fmt:UYVY/720x480 field:interlaced-tb
>                   crop.bounds:(0,0)/720x240
>                   crop:(0,0)/720x240]
>                  -> "OMAP3 ISP CCDC output":0 [ENABLED]
>                  -> "OMAP3 ISP resizer":0 []
>
> - entity 81: tvp5150 1-005c (4 pads, 1 link)
>               type V4L2 subdev subtype Decoder flags 0
>               device node name /dev/v4l-subdev8
>          pad0: Sink
>          pad1: Source
>                  [fmt:UYVY2X8/720x240 field:alternate
>                   crop.bounds:(0,0)/720x480
>                   crop:(0,0)/720x480]
>                  -> "OMAP3 ISP CCDC":0 [ENABLED]
>
> Best regards,

Re: [PATCH] gpu: ipu-v3: Allow negative offsets for interlaced scanning

[Krzysztof Hałasa]

Reporting from the field :-)

The fix-csi-interlaced.3 branch is still a bit off the track I guess:

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

does:
"adv7180 2-0020":0 [fmt:UYVY2X8/720x576 field:alternate]
"ipu2_csi1_mux":1  [fmt:UYVY2X8/720x576 field:alternate]
"ipu2_csi1_mux":2  [fmt:UYVY2X8/720x576 field:alternate]
"ipu2_csi1":0      [fmt:UYVY2X8/720x576 field:alternate
                 crop.bounds:(0,0)/720x1152
                 crop:(0,0)/720x1152
                 compose.bounds:(0,0)/720x1152
                 compose:(0,0)/720x1152]
"ipu2_csi1":2      [fmt:AYUV32/720x1152 field:seq-tb]

... and not interlaced[-*], as with fix-csi-interlaced.2.

The double heights are funny, too - probably an ADV7180 issue.
-- 
Krzysztof Halasa

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

Re: [PATCH] gpu: ipu-v3: Allow negative offsets for interlaced scanning

[Steve Longerbeam]

Hi Krzysztof,


On 06/14/2018 02:39 AM, Krzysztof Hałasa wrote:
> Reporting from the field :-)
>
> The fix-csi-interlaced.3 branch is still a bit off the track I guess:

Yes, it's still a WIP. A couple things are remaining:

- fix interweave with negative offsets for planar pixel formats.
- update the doc again.


>     media-ctl -V "'adv7180 2-0020':0 [fmt:UYVY2X8/576 field:seq-tb]"
>     media-ctl -V "'ipu2_csi1_mux':2 [fmt:UYVY2X8/720x576]"
>     media-ctl -V "'ipu2_csi1':2 [fmt:AYUV32/720x576 field:interlaced-tb]"
>
> does:
> "adv7180 2-0020":0 [fmt:UYVY2X8/720x576 field:alternate]
> "ipu2_csi1_mux":1  [fmt:UYVY2X8/720x576 field:alternate]
> "ipu2_csi1_mux":2  [fmt:UYVY2X8/720x576 field:alternate]
> "ipu2_csi1":0      [fmt:UYVY2X8/720x576 field:alternate
>                   crop.bounds:(0,0)/720x1152
>                   crop:(0,0)/720x1152
>                   compose.bounds:(0,0)/720x1152
>                   compose:(0,0)/720x1152]
> "ipu2_csi1":2      [fmt:AYUV32/720x1152 field:seq-tb]
>
> ... and not interlaced[-*], as with fix-csi-interlaced.2.

Right, the selection of interweave is moved to the capture devices,
so the following will enable interweave:

v4l2-ctl -dN --set-fmt-video=field=interlaced_tb


>
> The double heights are funny, too - probably an ADV7180 issue.

That's because it's been confirmed that for sources that
report ALTERNATE, mbus format height must be the number
of lines per field, not the total frame lines.

See

0018147c964e ("media: v4l: doc: Clarify v4l2_mbus_fmt height definition")

So the patch to adv7180 needs to be modified to report # field lines.

Try the following:

--- a/drivers/media/i2c/adv7180.c
+++ b/drivers/media/i2c/adv7180.c
@@ -503,6 +503,9 @@ static int adv7180_set_power(struct adv7180_state 
*state, bool on)
          }
      }

+    if (on)
+        msleep(500);
+
      return 0;
  }

@@ -643,6 +646,8 @@ static int adv7180_mbus_fmt(struct v4l2_subdev *sd,
      fmt->colorspace = V4L2_COLORSPACE_SMPTE170M;
      fmt->width = 720;
      fmt->height = state->curr_norm & V4L2_STD_525_60 ? 480 : 576;
+    if (fmt->field == V4L2_FIELD_ALTERNATE)
+        fmt->height /= 2;

      return 0;
  }
@@ -694,8 +699,8 @@ static int adv7180_get_pad_format(struct v4l2_subdev 
*sd,
      if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
          format->format = *v4l2_subdev_get_try_format(sd, cfg, 0);
      } else {
-        adv7180_mbus_fmt(sd, &format->format);
          format->format.field = state->field;
+        adv7180_mbus_fmt(sd, &format->format);
      }

      return 0;
@@ -712,10 +717,10 @@ static int adv7180_set_pad_format(struct 
v4l2_subdev *sd,
      switch (format->format.field) {
      case V4L2_FIELD_NONE:
          if (!(state->chip_info->flags & ADV7180_FLAG_I2P))
-            format->format.field = V4L2_FIELD_INTERLACED;
+            format->format.field = V4L2_FIELD_ALTERNATE;
          break;
      default:
-        format->format.field = V4L2_FIELD_INTERLACED;
+        format->format.field = V4L2_FIELD_ALTERNATE;
          break;
      }

@@ -1291,7 +1296,7 @@ static int adv7180_probe(struct i2c_client *client,
          return -ENOMEM;

      state->client = client;
-    state->field = V4L2_FIELD_INTERLACED;
+    state->field = V4L2_FIELD_ALTERNATE;
      state->chip_info = (struct adv7180_chip_info *)id->driver_data;

      state->pwdn_gpio = devm_gpiod_get_optional(&client->dev, "powerdown",

Re: [PATCH] gpu: ipu-v3: Allow negative offsets for interlaced scanning

[Krzysztof Hałasa]

Steve Longerbeam <slongerbeam@gmail.com> writes:

> Right, the selection of interweave is moved to the capture devices,
> so the following will enable interweave:
>
> v4l2-ctl -dN --set-fmt-video=field=interlaced_tb

and

> So the patch to adv7180 needs to be modified to report # field lines.
>
> Try the following:
>
> --- a/drivers/media/i2c/adv7180.c
> +++ b/drivers/media/i2c/adv7180.c

With this patch, fix-csi-interlaced.3 seems to work for me.
"ipu2_csi1":2 reports [fmt:AYUV32/720x576 field:seq-tb], but the
/dev/videoX shows (when requested) 720 x 576 NV12 interlaced, top field
first, and I'm getting valid output.

Thanks for your work.
-- 
Krzysztof Halasa

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

Re: [PATCH] gpu: ipu-v3: Allow negative offsets for interlaced scanning

[Steve Longerbeam]

Hi Philipp, Krzysztof,


On 06/15/2018 01:33 AM, Krzysztof Hałasa wrote:
> Steve Longerbeam <slongerbeam@gmail.com> writes:
>
>> Right, the selection of interweave is moved to the capture devices,
>> so the following will enable interweave:
>>
>> v4l2-ctl -dN --set-fmt-video=field=interlaced_tb
> and
>
>> So the patch to adv7180 needs to be modified to report # field lines.
>>
>> Try the following:
>>
>> --- a/drivers/media/i2c/adv7180.c
>> +++ b/drivers/media/i2c/adv7180.c
> With this patch, fix-csi-interlaced.3 seems to work for me.
> "ipu2_csi1":2 reports [fmt:AYUV32/720x576 field:seq-tb], but the
> /dev/videoX shows (when requested) 720 x 576 NV12 interlaced, top field
> first, and I'm getting valid output.
>
> Thanks for your work.

I've found some time to diagnose the behavior of interweave with B/T line
swapping (to support interlaced-bt) with planar formats.

There are a couple problems (one known and one unknown):

1. This requires 32 pixel alignment to meet the IDMAC 8-byte alignment
     of the planar U/V buffer offsets, and 32 pixel alignment precludes
     capturing raw NTSC/PAL at 720 pixel line stride.

2. Even with 32 pixel aligned frames, for example by using the prpenc scaler
     to generate 704 pixel strides from 720, the colors are still wrong when
     capturing interlaced-bt. I thought for sure this must be because we 
also
     need to double the SLUV line strides in addition to doubling SLY 
line stride.
     But I tried this and the results are that it works only for YUV 
4:2:2. For 4:2:0
     it causes system hard lockups. (Aside note: interweave without line 
swap
     apparently has never worked for 4:2:2, even when doubling SLUV, so it's
     quite bizarre to me why 4:2:2 interweave _with_ line swap _does_ work
     after doubling SLUV).


For these reasons I think we should disallow interlaced-bt with planar 
formats.

If the user needs NTSC interlaced capture with planar, the fields can be 
swapped at
the CSI, by selecting seq-tb at the CSI source pad, which allows for 
interlaced-tb
at the capture interface, which doesn't require interweave line swapping.

Steve

Re: [PATCH] gpu: ipu-v3: Allow negative offsets for interlaced scanning

[Philipp Zabel]

Hi Steve,

On Tue, 2018-06-19 at 18:30 -0700, Steve Longerbeam wrote:
> Hi Philipp, Krzysztof,
> 
> On 06/15/2018 01:33 AM, Krzysztof Hałasa wrote:
> > Steve Longerbeam <slongerbeam@gmail.com> writes:
> > 
> > > Right, the selection of interweave is moved to the capture devices,
> > > so the following will enable interweave:
> > > 
> > > v4l2-ctl -dN --set-fmt-video=field=interlaced_tb
> > 
> > and
> > 
> > > So the patch to adv7180 needs to be modified to report # field lines.
> > > 
> > > Try the following:
> > > 
> > > --- a/drivers/media/i2c/adv7180.c
> > > +++ b/drivers/media/i2c/adv7180.c
> > 
> > With this patch, fix-csi-interlaced.3 seems to work for me.
> > "ipu2_csi1":2 reports [fmt:AYUV32/720x576 field:seq-tb], but the
> > /dev/videoX shows (when requested) 720 x 576 NV12 interlaced, top field
> > first, and I'm getting valid output.
> > 
> > Thanks for your work.
> 
> I've found some time to diagnose the behavior of interweave with B/T line
> swapping (to support interlaced-bt) with planar formats.
> 
> There are a couple problems (one known and one unknown):
> 
> 1. This requires 32 pixel alignment to meet the IDMAC 8-byte alignment
>      of the planar U/V buffer offsets, and 32 pixel alignment precludes
>      capturing raw NTSC/PAL at 720 pixel line stride.

What needs to be aligned to multiples of 32 pixels?

I thought 8 pixel width alignment should be good enough for NV12/NV16,
for which luma and chroma strides are equal to the width in pixels, and
16 pixel alignment for YUV420/YVU420/YUV422P, where chroma stride is
half the width in pixels.

> 2. Even with 32 pixel aligned frames, for example by using the prpenc scaler
>      to generate 704 pixel strides from 720, the colors are still wrong when
>      capturing interlaced-bt.

As a side note, we can't properly scale seq-tb/bt direct input from the
CSI vertically with the IC, as the bottom line of the first field will
be blended with the top line of the second field...

>  I thought for sure this must be because we 
> also
>      need to double the SLUV line strides in addition to doubling SLY 
> line stride.
>      But I tried this and the results are that it works only for YUV 
> 4:2:2. For 4:2:0
>      it causes system hard lockups. (Aside note: interweave without line 
> swap
>      apparently has never worked for 4:2:2, even when doubling SLUV, so it's
>      quite bizarre to me why 4:2:2 interweave _with_ line swap _does_ work
>      after doubling SLUV).

When you say 4:2:2 you only mean YUV422P, not NV16 or YUYV/UYVY ?

> For these reasons I think we should disallow interlaced-bt with planar 
> formats.

Does that include NV12/NV16? Capturing to NV12 could be desirable if at
all possible, because the result can be encoded by the CODA. The other
formats are bandwidth inefficient anyway.

> If the user needs NTSC interlaced capture with planar, the fields can be 
> swapped at
> the CSI, by selecting seq-tb at the CSI source pad, which allows for 
> interlaced-tb
> at the capture interface, which doesn't require interweave line swapping.

regards
Philipp

Re: [PATCH] gpu: ipu-v3: Allow negative offsets for interlaced scanning

[Steve Longerbeam]

On 06/20/2018 01:54 AM, Philipp Zabel wrote:
> Hi Steve,
>
> On Tue, 2018-06-19 at 18:30 -0700, Steve Longerbeam wrote:
>> I've found some time to diagnose the behavior of interweave with B/T line
>> swapping (to support interlaced-bt) with planar formats.
>>
>> There are a couple problems (one known and one unknown):
>>
>> 1. This requires 32 pixel alignment to meet the IDMAC 8-byte alignment
>>       of the planar U/V buffer offsets, and 32 pixel alignment precludes
>>       capturing raw NTSC/PAL at 720 pixel line stride.
> What needs to be aligned to multiples of 32 pixels?
>
> I thought 8 pixel width alignment should be good enough for NV12/NV16,
> for which luma and chroma strides are equal to the width in pixels, and
> 16 pixel alignment for YUV420/YVU420/YUV422P, where chroma stride is
> half the width in pixels.

I see where the problem is now. I was basing my mistaken 32 pixel
alignment from a read of the U_OFFSET/V_OFFSET macros in
ipu-cpmem.c:

u_offset = pix->width * pix->height + pix->width * y / 4 + x / 2

But you can probably see the bug now. This does not produce
a correct offset for odd values of y. It should read:

u_offset = pix->width * pix->height + pix->width * (y / 2) / 2 + x / 2

With that fix, interweave line swap with planar 4:2:0 is working now.
That includes YUV420, YVU420, and NV12.

NV16 is also working after programming SLUV with double
the chroma line stride.

>> 2. Even with 32 pixel aligned frames, for example by using the prpenc scaler
>>       to generate 704 pixel strides from 720, the colors are still wrong when
>>       capturing interlaced-bt.
> As a side note, we can't properly scale seq-tb/bt direct input from the
> CSI vertically with the IC, as the bottom line of the first field will
> be blended with the top line of the second field...
>
>>   I thought for sure this must be because we
>> also
>>       need to double the SLUV line strides in addition to doubling SLY
>> line stride.
>>       But I tried this and the results are that it works only for YUV
>> 4:2:2. For 4:2:0
>>       it causes system hard lockups. (Aside note: interweave without line
>> swap
>>       apparently has never worked for 4:2:2, even when doubling SLUV, so it's
>>       quite bizarre to me why 4:2:2 interweave _with_ line swap _does_ work
>>       after doubling SLUV).
> When you say 4:2:2 you only mean YUV422P, not NV16 or YUYV/UYVY ?

Correct, I meant planar YUV422P.


>
>> For these reasons I think we should disallow interlaced-bt with planar
>> formats.
> Does that include NV12/NV16? Capturing to NV12 could be desirable if at
> all possible, because the result can be encoded by the CODA. The other
> formats are bandwidth inefficient anyway.

Never mind, I found the bug described above in the U_OFFSET/V_OFFSET
macros.

In summary, at this point all planar formats are working with interlaced
bt and tb, except for YUV422P.

Steve