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

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

On Thu, Oct 24, 2024 at 02:13:07PM +0200, Hans Verkuil wrote:
> On 24/10/2024 14:05, Laurent Pinchart wrote:
> > 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.
> 
> The validation can be done in the prepare_streaming callback instead of
> in start_streaming. prepare_streaming is called at STREAMON time.

True, we have that now, I keep forgetting about it. I've noticed the
annoying behaviour with drivers that predate .prepare_streaming(). It
should be a requirement to validate the pipeline in .prepare_streaming()
(or possibly earlier in some cases, not sure).

> > 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.
> 
> None of which is possible for some of the older drivers (e.g. TI davinci)
> that do not have an IOMMU and require two contiguous buffers before they can
> start streaming. But for modern devices you can solve it through a scratch
> buffer, that's true.

In the worst case kernel drivers could allocate a full scratch buffer.
That's not nice from a memory consumption point of view of course. The
stride hack may have worked with TI davinci, I haven't checked. It's
something that driver authors don't usually think about even if the
hardware can do it. Even if the device supports a 0 stride, it also
needs to support programming the stride and the buffer addresses in a
race-free way.

> > 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.
> 
> Regards,
> 
> 	Hans
> 
> >> 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