Re: [PATCH 3/3] media: imx: lift CSI width alignment restriction

[Steve Longerbeam <slongerbeam@gmail.com>]

On 11/9/18 6:50 AM, Philipp Zabel wrote:
> On Thu, 2018-11-08 at 21:46 -0800, Steve Longerbeam wrote:
>> On 11/5/18 7:20 AM, Philipp Zabel wrote:
>>> The CSI subdevice shouldn't have to care about IDMAC line start
>>> address alignment. With compose rectangle support in the capture
>>> driver, it doesn't have to anymore.
>>>
>>> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>>> ---
>>>    drivers/staging/media/imx/imx-media-capture.c |  9 ++++-----
>>>    drivers/staging/media/imx/imx-media-csi.c     |  2 +-
>>>    drivers/staging/media/imx/imx-media-utils.c   | 15 ++++++++++++---
>>>    3 files changed, 17 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
>>> index 2d49d9573056..f87d6e8019e5 100644
>>> --- a/drivers/staging/media/imx/imx-media-capture.c
>>> +++ b/drivers/staging/media/imx/imx-media-capture.c
>>> @@ -204,10 +204,9 @@ static int capture_g_fmt_vid_cap(struct file *file, void *fh,
>>>    }
>>>    
>>>    static int __capture_try_fmt_vid_cap(struct capture_priv *priv,
>>> -				     struct v4l2_subev_format *fmt_src,
>>> +				     struct v4l2_subdev_format *fmt_src,
>>>    				     struct v4l2_format *f)
>>>    {
>>> -	struct capture_priv *priv = video_drvdata(file);
>>>    	const struct imx_media_pixfmt *cc, *cc_src;
>>>    
>>>    	cc_src = imx_media_find_ipu_format(fmt_src->format.code, CS_SEL_ANY);
>>> @@ -250,7 +249,7 @@ static int capture_try_fmt_vid_cap(struct file *file, void *fh,
>>>    	if (ret)
>>>    		return ret;
>>>    
>>> -	return __capture_try_fmt(priv, &fmt_src, f);
>>> +	return __capture_try_fmt_vid_cap(priv, &fmt_src, f);
>>>    }
>>>    
>>>    static int capture_s_fmt_vid_cap(struct file *file, void *fh,
>>> @@ -280,8 +279,8 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
>>>    					      CS_SEL_ANY, true);
>>>    	priv->vdev.compose.left = 0;
>>>    	priv->vdev.compose.top = 0;
>>> -	priv->vdev.compose.width = fmt_src.width;
>>> -	priv->vdev.compose.height = fmt_src.height;
>>> +	priv->vdev.compose.width = fmt_src.format.width;
>>> +	priv->vdev.compose.height = fmt_src.format.height;
>>>    
>>>    	return 0;
>>>    }
>>> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
>>> index c4523afe7b48..d39682192a67 100644
>>> --- a/drivers/staging/media/imx/imx-media-csi.c
>>> +++ b/drivers/staging/media/imx/imx-media-csi.c
>>> @@ -41,7 +41,7 @@
>>>    #define MIN_H       144
>>>    #define MAX_W      4096
>>>    #define MAX_H      4096
>>> -#define W_ALIGN    4 /* multiple of 16 pixels */
>>> +#define W_ALIGN    1 /* multiple of 2 pixels */
>>
>> This works for the IDMAC output pad because the channel's cpmem width
>> and stride can be rounded up, but width align at the CSI sink still
>> needs to be 8 pixels when directed to the IC via the CSI_SRC_PAD_DIRECT
>> pad, in order to support the 8x8 block rotator in the IC PRP, and
>> there's no way AFAIK to do the same trick of rounding up width and
>> stride for non-IDMAC direct paths through the IPU.
> Actually, this is not necessary at all. csi_try_crop takes care of this
> by setting:
> 	crop->width &= ~0x7;
> Which is then used to set compose rectangle and source pad formats.

Ah you are right, I had forgotten about that line.

>
> So this should be relaxed as well, if the SRC_DIRECT pad is not enabled.

Agreed, crop->width align can be relaxed but only if SRC_DIRECT pad
not enabled.

> And further, I think there is no reason to align crop->left to multiples
> of 4 pixels?

Honestly, I don't know the reason for that either. Along with crop->top 
this defines the CSI active image frame position (HSC/VSC fields in 
IPU_CSI_OUT_FRM_CTRL register), and I don't see any h/w restrictions on 
HSC in the ref manual. I do remember that this alignment exists in 
FSL/NXP BSP's though, so maybe it is an undocumented restriction.

Steve