Hi Hans, On 24/10/2024 11:20, Hans Verkuil wrote:
Hi Tomi, I know this driver is already merged, but while checking for drivers that use q->max_num_buffers I stumbled on this cfe code: <snip>+/* + * vb2 ops + */ + +static int cfe_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers, + unsigned int *nplanes, unsigned int sizes[], + struct device *alloc_devs[]) +{ + struct cfe_node *node = vb2_get_drv_priv(vq); + struct cfe_device *cfe = node->cfe; + unsigned int size = is_image_node(node) ? + node->vid_fmt.fmt.pix.sizeimage : + node->meta_fmt.fmt.meta.buffersize; + + cfe_dbg(cfe, "%s: [%s] type:%u\n", __func__, node_desc[node->id].name, + node->buffer_queue.type); + + if (vq->max_num_buffers + *nbuffers < 3) + *nbuffers = 3 - vq->max_num_buffers;This makes no sense: max_num_buffers is 32, unless explicitly set when vb2_queue_init is called. So 32 + *nbuffers is never < 3. If the idea is that at least 3 buffers should be allocated by REQBUFS, then set q->min_reqbufs_allocation = 3; before calling vb2_queue_init and vb2 will handle this for you. Drivers shouldn't modify *nbuffers, except in very rare circumstances, especially since the code is almost always wrong.
Indeed, the code doesn't make sense. I have to say I don't know what was the intent here, but I think "at least 3 buffers should be allocated by REQBUFS" is the likely explanation.
I think the hardware should work with even just a single buffer, so is it then fine to not set either q->min_queued_buffers nor q->min_reqbufs_allocation before calling vb2_queue_init()? This seems to result in REQBUFS giving at least two buffers.
Tomi
Regards, Hans+ + if (*nplanes) { + if (sizes[0] < size) { + cfe_err(cfe, "sizes[0] %i < size %u\n", sizes[0], size); + return -EINVAL; + } + size = sizes[0]; + } + + *nplanes = 1; + sizes[0] = size; + + return 0; +}
