Re: [PATCH 2/2] media: rockchip: rkcif: Honour the requested bytesperline on MIPI

[Michael Riesch]

Hi there,

Thanks for the patch!

On 11/6/26 00:23, Faizel K B wrote:
> The MIPI DMA uses bytesperline as the line stride, which must be a
> multiple of 8 bytes, but v4l2_fill_pixfmt_mp() doesn't align it.

Come to think of it, TRM p. 2111 says for e.g. VICAP_MIPI0_VLW_ID0:
"Virtual line width of even frame for ID0 path(must be aligned to double
word)." which I would interpret as "it must be a multiple of four
bytes". What do you think?

> Any larger bytesperline requested by userspace is also replaced with
> the minimum.
> 
> Align bytesperline to 8 bytes and honour a larger requested value, up
> to the limit of the line width register. This allows DMA-BUF importers
> that need a larger stride alignment to use the buffers directly.
> 
> Signed-off-by: Faizel K B <faizel.kb@xxxxxxxxx>
> ---
>  .../platform/rockchip/rkcif/rkcif-stream.c    | 22 ++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkcif/rkcif-stream.c b/drivers/media/platform/rockchip/rkcif/rkcif-stream.c
> index f2a24832a8cf..e50f20e40f7a 100644
> --- a/drivers/media/platform/rockchip/rkcif/rkcif-stream.c
> +++ b/drivers/media/platform/rockchip/rkcif/rkcif-stream.c
> @@ -23,6 +23,7 @@
>  #define CIF_MIN_HEIGHT	 64
>  #define CIF_MAX_WIDTH	 8192
>  #define CIF_MAX_HEIGHT	 8192
> +#define CIF_MAX_BYTESPERLINE 0xffff8 /* VLW: 20 bits, 8-byte aligned */
>  
>  static inline struct rkcif_buffer *to_rkcif_buffer(struct vb2_v4l2_buffer *vb)
>  {
> @@ -334,20 +335,39 @@ static const struct vb2_ops rkcif_stream_vb2_ops = {
>  	.stop_streaming = rkcif_stream_stop_streaming,
>  };
>  
> +static void rkcif_stream_set_bytesperline(struct v4l2_pix_format_mplane *pix,
> +					  u32 req_bpl)
> +{
> +	u32 min_bpl = ALIGN(pix->plane_fmt[0].bytesperline, 8);

Please add support for the bytesperline field in all the plane_fmt
entries here...

> +	u32 max_bpl = min_t(u32, CIF_MAX_BYTESPERLINE,
> +			    ALIGN_DOWN(U32_MAX / pix->height, 8));
> +	u32 bpl = min_bpl;
> +
> +	if (req_bpl > min_bpl && req_bpl <= max_bpl)
> +		bpl = ALIGN(req_bpl, 8);
> +
> +	pix->plane_fmt[0].bytesperline = bpl;
> +	pix->plane_fmt[0].sizeimage = bpl * pix->height;
> +}
> +
>  static int rkcif_stream_fill_format(struct rkcif_stream *stream,
>  				    struct v4l2_pix_format_mplane *pix)
>  {
>  	const struct rkcif_output_fmt *fmt;
> -	u32 height, width;
> +	u32 height, width, req_bpl;
>  	int ret;
>  
>  	fmt = rkcif_stream_find_output_fmt(stream, true, pix->pixelformat);
>  	height = clamp_t(u32, pix->height, CIF_MIN_HEIGHT, CIF_MAX_HEIGHT);
>  	width = clamp_t(u32, pix->width, CIF_MIN_WIDTH, CIF_MAX_WIDTH);
> +	req_bpl = pix->plane_fmt[0].bytesperline;

... and here.

>  	ret = v4l2_fill_pixfmt_mp(pix, fmt->fourcc, width, height);
>  	if (ret)
>  		return ret;
>  
> +	if (stream->interface->type == RKCIF_IF_MIPI)
> +		rkcif_stream_set_bytesperline(pix, req_bpl);

Why is this restricted to the MIPI interfaces?

Best regards,
Michael

> +
>  	pix->field = V4L2_FIELD_NONE;
>  
>  	return 0;