| Message ID | 20180601131316.18728-1-p.zabel@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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,
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,
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.
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);
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(-)