Re: [PATCH v2 01/10] media: imx-csi: Pass sink pad field to ipu_csi_init_interface
Steve Longerbeam writes: > I think you misunderstood me. Of course there is a first and second > field. By first I am referring to the first field transmitted, which could > be top or bottom. Right. I was thinking the fields are even and odd, but that's not actually the case (I mean, the numbering uses field 1 and 2 and not E/O). > Progressive sensors have no fields, the entire image is captured at > once as you said. There are progressive cameras with analog PAL/NTSC output. The signal is obviously interlaced and consists of fields. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH v2 01/10] media: imx-csi: Pass sink pad field to ipu_csi_init_interface
On 06/03/2018 10:25 PM, Krzysztof Hałasa wrote: Steve Longerbeam writes: I think we should return to enforcing field order to userspace that matches field order from the source, which is what I had implemented previously. I agree with you that we should put off allowing inverting field order. There is no any particular field order at the source, most of the time. The odd field is followed by the even field, and so on, sure. But there is no "first" and "second" field, any field can be the "first". I think you misunderstood me. Of course there is a first and second field. By first I am referring to the first field transmitted, which could be top or bottom. The exception to this is a camera with a progressive sensor - both "fields" are taken at the same time and transmitted one after the other, so in this case the order is defined (by the camera, e.g. B-T on DV even with PAL version). But this isn't exactly PAL/NTSC. Progressive sensors have no fields, the entire image is captured at once as you said. Steve
Re: [PATCH v2 01/10] media: imx-csi: Pass sink pad field to ipu_csi_init_interface
On Mon, 2018-06-04 at 07:25 +0200, Krzysztof Hałasa wrote: > Steve Longerbeam writes: > > > I think we should return to enforcing field order to userspace that > > matches field order from the source, which is what I had implemented > > previously. I agree with you that we should put off allowing inverting > > field order. > > There is no any particular field order at the source, most of the time. > The odd field is followed by the even field, and so on, sure. But there > is no "first" and "second" field, any field can be the "first". There is no particular field order in the data itself. But for BT.656 there is a specific field order, defined by the F flag in the SAV/EAV codes. For PAL usually F=0 is the top field and F=1 is the bottom field. For NTSC it usually is the other way around. > The exception to this is a camera with a progressive sensor - both > "fields" are taken at the same time and transmitted one after the other, > so in this case the order is defined (by the camera, e.g. B-T on DV even > with PAL version). But this isn't exactly PAL/NTSC. That's why I'd like to make it obvious to the user when the field order is switched. Whoever selects seq-bt -> seq-tb or seq-tb -> seq-bt transformation for progressive sources can expect combing artifacts. regards Philipp
Re: [PATCH v2 01/10] media: imx-csi: Pass sink pad field to ipu_csi_init_interface
Steve Longerbeam writes: > I think we should return to enforcing field order to userspace that > matches field order from the source, which is what I had implemented > previously. I agree with you that we should put off allowing inverting > field order. There is no any particular field order at the source, most of the time. The odd field is followed by the even field, and so on, sure. But there is no "first" and "second" field, any field can be the "first". The exception to this is a camera with a progressive sensor - both "fields" are taken at the same time and transmitted one after the other, so in this case the order is defined (by the camera, e.g. B-T on DV even with PAL version). But this isn't exactly PAL/NTSC. -- Krzysztof Halasa Industrial Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
Re: [PATCH v2 01/10] media: imx-csi: Pass sink pad field to ipu_csi_init_interface
On 06/01/2018 06:22 AM, Philipp Zabel wrote:
On Thu, 2018-05-31 at 17:30 -0700, Steve Longerbeam wrote:
The output pad's field type was being passed to ipu_csi_init_interface(),
in order to deal with field type 'alternate' at the sink pad, which
is not understood by ipu_csi_init_interface().
Remove that code and pass the sink pad field to ipu_csi_init_interface().
The latter function will have to explicity deal with field type 'alternate'
when setting up the CSI interface for BT.656 busses.
I fear this won't be enough. If we want to capture
sink:ALTERNATE/SEQ_TB/SEQ_BT -> src:SEQ_TB we have to configure the CSI
differently than if we want to capture
ALTERNATE/SEQ_TB/SEQ_BT -> src:SEQ_BT. (And differently for NTSC and
PAL). For NTSC sink:ALTERNATE should behave like sink:SEQ_BT, and for
PAL sink:ALTERNATE should behave like sink:SEQ_TB.
I think we should return to enforcing field order to userspace that
matches field order from the source, which is what I had implemented
previously. I agree with you that we should put off allowing inverting
field order.
Interweaving SEQ_TB to INTERLACED_TB should work right now, but to
interweave SEQ_BT to INTERLACED_BT, we need to add one line offset to
the frame start and use a negative interlaced scanline offset.
Is that because ipu_csi_init_interface() is inverting the F-bit for
NTSC? I think we should remove that code, I will comment on
that in another thread.
Steve
Reported-by: Krzysztof Hałasa
Signed-off-by: Steve Longerbeam
---
drivers/staging/media/imx/imx-media-csi.c | 13 ++---
1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/media/imx/imx-media-csi.c
b/drivers/staging/media/imx/imx-media-csi.c
index 95d7805..9bc555c 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -629,12 +629,10 @@ static void csi_idmac_stop(struct csi_priv *priv)
/* Update the CSI whole sensor and active windows */
static int csi_setup(struct csi_priv *priv)
{
- struct v4l2_mbus_framefmt *infmt, *outfmt;
+ struct v4l2_mbus_framefmt *infmt;
struct v4l2_mbus_config mbus_cfg;
- struct v4l2_mbus_framefmt if_fmt;
infmt = >format_mbus[CSI_SINK_PAD];
- outfmt = >format_mbus[priv->active_output_pad];
/* compose mbus_config from the upstream endpoint */
mbus_cfg.type = priv->upstream_ep.bus_type;
@@ -642,20 +640,13 @@ static int csi_setup(struct csi_priv *priv)
priv->upstream_ep.bus.mipi_csi2.flags :
priv->upstream_ep.bus.parallel.flags;
- /*
-* we need to pass input frame to CSI interface, but
-* with translated field type from output format
-*/
- if_fmt = *infmt;
- if_fmt.field = outfmt->field;
-
ipu_csi_set_window(priv->csi, >crop);
ipu_csi_set_downsize(priv->csi,
priv->crop.width == 2 * priv->compose.width,
priv->crop.height == 2 * priv->compose.height);
- ipu_csi_init_interface(priv->csi, _cfg, _fmt);
+ ipu_csi_init_interface(priv->csi, _cfg, infmt);
ipu_csi_set_dest(priv->csi, priv->dest);
Re: [PATCH v2 01/10] media: imx-csi: Pass sink pad field to ipu_csi_init_interface
On Thu, 2018-05-31 at 17:30 -0700, Steve Longerbeam wrote:
> The output pad's field type was being passed to ipu_csi_init_interface(),
> in order to deal with field type 'alternate' at the sink pad, which
> is not understood by ipu_csi_init_interface().
>
> Remove that code and pass the sink pad field to ipu_csi_init_interface().
> The latter function will have to explicity deal with field type 'alternate'
> when setting up the CSI interface for BT.656 busses.
I fear this won't be enough. If we want to capture
sink:ALTERNATE/SEQ_TB/SEQ_BT -> src:SEQ_TB we have to configure the CSI
differently than if we want to capture
ALTERNATE/SEQ_TB/SEQ_BT -> src:SEQ_BT. (And differently for NTSC and
PAL). For NTSC sink:ALTERNATE should behave like sink:SEQ_BT, and for
PAL sink:ALTERNATE should behave like sink:SEQ_TB.
Interweaving SEQ_TB to INTERLACED_TB should work right now, but to
interweave SEQ_BT to INTERLACED_BT, we need to add one line offset to
the frame start and use a negative interlaced scanline offset.
regards
Philipp
> Reported-by: Krzysztof Hałasa
> Signed-off-by: Steve Longerbeam
> ---
> drivers/staging/media/imx/imx-media-csi.c | 13 ++---
> 1 file changed, 2 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-csi.c
> b/drivers/staging/media/imx/imx-media-csi.c
> index 95d7805..9bc555c 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -629,12 +629,10 @@ static void csi_idmac_stop(struct csi_priv *priv)
> /* Update the CSI whole sensor and active windows */
> static int csi_setup(struct csi_priv *priv)
> {
> - struct v4l2_mbus_framefmt *infmt, *outfmt;
> + struct v4l2_mbus_framefmt *infmt;
> struct v4l2_mbus_config mbus_cfg;
> - struct v4l2_mbus_framefmt if_fmt;
>
> infmt = >format_mbus[CSI_SINK_PAD];
> - outfmt = >format_mbus[priv->active_output_pad];
>
> /* compose mbus_config from the upstream endpoint */
> mbus_cfg.type = priv->upstream_ep.bus_type;
> @@ -642,20 +640,13 @@ static int csi_setup(struct csi_priv *priv)
> priv->upstream_ep.bus.mipi_csi2.flags :
> priv->upstream_ep.bus.parallel.flags;
>
> - /*
> - * we need to pass input frame to CSI interface, but
> - * with translated field type from output format
> - */
> - if_fmt = *infmt;
> - if_fmt.field = outfmt->field;
> -
> ipu_csi_set_window(priv->csi, >crop);
>
> ipu_csi_set_downsize(priv->csi,
>priv->crop.width == 2 * priv->compose.width,
>priv->crop.height == 2 * priv->compose.height);
>
> - ipu_csi_init_interface(priv->csi, _cfg, _fmt);
> + ipu_csi_init_interface(priv->csi, _cfg, infmt);
>
> ipu_csi_set_dest(priv->csi, priv->dest);
>
