Hi,
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.
Looking at this, the original code in the old BSP tree was, which
somehow, along the long way, got turned into the above:
if (vq->num_buffers + *nbuffers < 3)
*nbuffers = 3 - vq->num_buffers;
So... I think that is the same as "q->min_reqbufs_allocation = 3"?
The distinction between min_queued_buffers and min_reqbufs_allocation,
or rather the need for the latter, still escapes me. If the HW/SW
requires N buffers to be queued, why would we require allocating more
than N buffers?
Tomi
