Re: [PATCH v8 06/13] media: rockchip: add a driver for the rockchip camera interface

[Michael Riesch <michael.riesch@collabora.com>]

Hi Mehdi,

On 6/13/25 14:33, Mehdi Djait wrote:
> Hi Michael,
> 
> [...]

Let's cut the long story short, shall we :-)

>> +static void rkcif_stream_fill_format(struct rkcif_stream *stream,
>> +				     struct v4l2_pix_format_mplane *pix)
>> +{
>> +	const struct rkcif_output_fmt *fmt;
>> +	u32 height, width;
>> +
>> +	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);
>> +	v4l2_fill_pixfmt_mp(pix, fmt->fourcc, width, height);
> 
> The rkcif supports the SRGGB10P: the packed version of the SRGGB10.
> 
> When you try to capture with SRGGB10P, the following fails in
> v4l2_fill_pixfmt_mp()
> 
> 	info = v4l2_format_info(pixelformat);
> 	if (!info)
> 		return -EINVAL;
> 
> The return value is not checked in rkcif_stream_fill_format() resulting
> in a call to queue_setup returning with sizes[0] = 0

Thanks for pointing that out. I failed to realize that this helper
function may return an error. What I can do is to implement the error
propagation correctly. Of course this will only avoid the kernel
warning, you still won't be able to stream.

> This will cause a kernel Warning in the vb2_core_reqbufs() because
> plane_size = 0
> 
> Exactly here:
> 
> 	for (i = 0; i < num_planes; i++)
> 		if (WARN_ON(!plane_sizes[i])) {
> 			ret = -EINVAL;
> 			goto error;
> 		}
> 
> I still don't have the solution here but wanted to let you know about
> it.

I only had a quick look at it, but apparently the compact formats are
missing in the format list here:
https://elixir.bootlin.com/linux/v6.16-rc2/source/drivers/media/v4l2-core/v4l2-common.c#L244
for whatever reason.

Have you by chance tried adding a line like
{
	.format = V4L2_PIX_FMT_SRGGB10P,
	.pixel_enc = V4L2_PIXEL_ENC_BAYER,
	.mem_planes = 1,
	.comp_planes = 1,
	.bpp = { 5, 0, 0, 0 },
	.bpp_div = { 4, 1, 1, 1 },
	.hdiv = 1,
	.vdiv = 1,
}
?


Maybe we can fix this, but please note that this issue should not block
merging this patch series.

Best regards,
Michael