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

[Steve Longerbeam <slongerbeam@gmail.com>]

Hi Philipp,


On 10/05/2018 03:43 AM, Philipp Zabel wrote:
> 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@pengutronix.de>
>> Signed-off-by: Steve Longerbeam <slongerbeam@gmail.com>
>> ---
>>   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?

interweave_offset was used by prp_vb2_buf_done() below, but in fact
that function is passed the non-rotation IDMAC channel (priv->out_ch)
_only_ if rotation is not enabled, so it is actually safe to use
vdev->fmt.fmt.bytesperline for the interweave offset in
prp_vb2_buf_done().

So I've gotten rid of interweave_offset in both imx-ic-prpencvf.c and
imx-media-csi.c, and replaced with a boolean interweave_swap as you
suggested. I agree it is much cleaner.
   

>>
>> 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?

Agreed.

>
>>   	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.

In the future yes, when we add support for alternate mode (I assume
that's what you are getting at?).


> Apart from that,
>
> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

Thanks.

Steve