Re: [PATCH v6 3/4] media: raspberrypi: Add support for RP1-CFE

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Thu, Oct 24, 2024 at 01:21:43PM +0200, Hans Verkuil wrote:
> On 24/10/2024 13:08, Tomi Valkeinen wrote:
> > 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.
> 
> min_queued_buffers is really HW dependent. If not set, then
> start_streaming can be called even if there are no buffers queued.

Having min_queued_buffers > 1 is bad for userspace, and it's much nicer
to have it set to 0. The main issue with a value of 1 is that validation
of the pipeline ends up being deferred to the first QBUF if it occurs
after STREAMON, and error handling is then complicated.

It's not just a property of the hardware, kernel drivers can decide to
work with scratch buffers if needed. In many cases, a scratch buffer
allocated by the kernel could be very small, either relying on the same
physical page being mapped through the IOMMU to a larger DMA space, or
using a 0 stride value to write all lines to the same location.

For drivers supported by libcamera, we will require min_queued_buffers
<= 1 and may tighten that to == 0. Tomi, if you submit a patch, please
try to target 0, and if that's too much work for now, set it to 1 at
most.

> If your hardware can handle that, then it's fine to not set it.
> 
> >>> +
> >>> +    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;
> >>> +}

-- 
Regards,

Laurent Pinchart