Re: [PATCH v4 10/11] media: imx: Allow interweave with top/bottom lines swapped

From: Philipp Zabel
Date: Fri Oct 05 2018 - 06:43:47 EST


Hi Steve,

On Thu, 2018-10-04 at 11:54 -0700, Steve Longerbeam wrote:
> Allow sequential->interlaced interweaving but with top/bottom
> lines swapped to the output buffer.
>
> This can be accomplished by adding one line length to IDMAC output
> channel address, with a negative line length for the interlace offset.
>
> This is to allow the seq-bt -> interlaced-bt transformation, where
> bottom lines are still dominant (older in time) but with top lines
> first in the interweaved output buffer.
>
> With this support, the CSI can now allow seq-bt at its source pads,
> e.g. the following transformations are allowed in CSI from sink to
> source:
>
> seq-tb -> seq-bt
> seq-bt -> seq-bt
> alternate -> seq-bt
>
> Suggested-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> Signed-off-by: Steve Longerbeam <slongerbeam@xxxxxxxxx>
> ---
> drivers/staging/media/imx/imx-ic-prpencvf.c | 17 +++++++-
> drivers/staging/media/imx/imx-media-csi.c | 46 +++++++++++++++++----
> 2 files changed, 53 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
> index cf76b0432371..1499b0c62d74 100644
> --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
> +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
> @@ -106,6 +106,8 @@ struct prp_priv {
> u32 frame_sequence; /* frame sequence counter */
> bool last_eof; /* waiting for last EOF at stream off */
> bool nfb4eof; /* NFB4EOF encountered during streaming */
> + u32 interweave_offset; /* interweave line offset to swap
> + top/bottom lines */

We have to store this instead of using vdev->fmt.fmt.bytesperline
because this potentially is the pre-rotation stride instead?

> struct completion last_eof_comp;
> };
>
> @@ -235,6 +237,9 @@ static void prp_vb2_buf_done(struct prp_priv *priv, struct ipuv3_channel *ch)
> if (ipu_idmac_buffer_is_ready(ch, priv->ipu_buf_num))
> ipu_idmac_clear_buffer(ch, priv->ipu_buf_num);
>
> + if (ch == priv->out_ch)
> + phys += priv->interweave_offset;
> +
> ipu_cpmem_set_buffer(ch, priv->ipu_buf_num, phys);
> }
>
> @@ -388,6 +393,13 @@ static int prp_setup_channel(struct prp_priv *priv,
> (image.pix.width * outcc->bpp) >> 3;
> }
>
> + priv->interweave_offset = 0;
> +
> + if (interweave && image.pix.field == V4L2_FIELD_INTERLACED_BT) {
> + image.rect.top = 1;
> + priv->interweave_offset = image.pix.bytesperline;
> + }
> +
> image.phys0 = addr0;
> image.phys1 = addr1;
>
> @@ -425,7 +437,10 @@ static int prp_setup_channel(struct prp_priv *priv,
> ipu_cpmem_set_rotation(channel, rot_mode);
>
> if (interweave)
> - ipu_cpmem_interlaced_scan(channel, image.pix.bytesperline,
> + ipu_cpmem_interlaced_scan(channel,
> + priv->interweave_offset ?
> + -image.pix.bytesperline :
> + image.pix.bytesperline,
> image.pix.pixelformat);
>
> ret = ipu_ic_task_idma_init(priv->ic, channel,
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index 679295da5dde..592f7d6edec1 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -114,6 +114,8 @@ struct csi_priv {
> u32 frame_sequence; /* frame sequence counter */
> bool last_eof; /* waiting for last EOF at stream off */
> bool nfb4eof; /* NFB4EOF encountered during streaming */
> + u32 interweave_offset; /* interweave line offset to swap
> + top/bottom lines */

This doesn't seem necessary. Since there is no rotation here, the offset
is just vdev->fmt.fmt.pix.bytesperline if interweave_swap is enabled.
Maybe turn this into a bool interweave_swap?

> struct completion last_eof_comp;
> };
>
> @@ -286,7 +288,8 @@ static void csi_vb2_buf_done(struct csi_priv *priv)
> if (ipu_idmac_buffer_is_ready(priv->idmac_ch, priv->ipu_buf_num))
> ipu_idmac_clear_buffer(priv->idmac_ch, priv->ipu_buf_num);
>
> - ipu_cpmem_set_buffer(priv->idmac_ch, priv->ipu_buf_num, phys);
> + ipu_cpmem_set_buffer(priv->idmac_ch, priv->ipu_buf_num,
> + phys + priv->interweave_offset);
> }
>
> static irqreturn_t csi_idmac_eof_interrupt(int irq, void *dev_id)
> @@ -396,10 +399,10 @@ static void csi_idmac_unsetup_vb2_buf(struct csi_priv *priv,
> static int csi_idmac_setup_channel(struct csi_priv *priv)
> {
> struct imx_media_video_dev *vdev = priv->vdev;
> + bool passthrough, interweave, interweave_swap;
> const struct imx_media_pixfmt *incc;
> struct v4l2_mbus_framefmt *infmt;
> struct v4l2_mbus_framefmt *outfmt;
> - bool passthrough, interweave;
> struct ipu_image image;
> u32 passthrough_bits;
> u32 passthrough_cycles;
> @@ -433,6 +436,8 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
> */
> interweave = V4L2_FIELD_IS_INTERLACED(image.pix.field) &&
> V4L2_FIELD_IS_SEQUENTIAL(outfmt->field);
> + interweave_swap = interweave &&
> + image.pix.field == V4L2_FIELD_INTERLACED_BT;

Although this could just as well be recalculated in csi_vb2_buf_done.
Apart from that,

Reviewed-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>

regards
Philipp