Re: [PATCH v3 04/15] media: qcom: camss: Add camss-isp-bufq helper

[Loic Poulain]

Hi Bryan,

On Fri, May 8, 2026 at 11:57 AM Bryan O'Donoghue
<bryan.odonoghue@xxxxxxxxxx> wrote:
>
> On 07/05/2026 23:49, Loic Poulain wrote:
> > Add a per-queue ready-buffer FIFO helper for CAMSS offline ISP drivers.
> > camss_isp_bufq provides N spinlock-protected FIFO lists of ready vb2
> > buffers, one per queue index. This can help multi-queues management
> > and synchronization in ISP context.
> >
> > Signed-off-by: Loic Poulain <loic.poulain@xxxxxxxxxxxxxxxx>
> > ---
> >   drivers/media/platform/qcom/camss/Kconfig          |  14 +++
> >   drivers/media/platform/qcom/camss/Makefile         |   5 +
> >   drivers/media/platform/qcom/camss/camss-isp-bufq.c | 101 +++++++++++++++++
> >   drivers/media/platform/qcom/camss/camss-isp-bufq.h | 122 +++++++++++++++++++++
> >   4 files changed, 242 insertions(+)
> >
> > diff --git a/drivers/media/platform/qcom/camss/Kconfig b/drivers/media/platform/qcom/camss/Kconfig
> > index 4eda48cb1adf049a7fb6cb59b9da3c0870fe57f4..d77482f3f5eadc65856806b9b237d65ea484f267 100644
> > --- a/drivers/media/platform/qcom/camss/Kconfig
> > +++ b/drivers/media/platform/qcom/camss/Kconfig
> > @@ -7,3 +7,17 @@ config VIDEO_QCOM_CAMSS
> >       select VIDEO_V4L2_SUBDEV_API
> >       select VIDEOBUF2_DMA_SG
> >       select V4L2_FWNODE
> > +
> > +config VIDEO_QCOM_CAMSS_ISP
>
> I think this config option should be dropped entirely.
>
> > +     tristate "Qualcomm CAMSS ISP common helpers"
> > +     depends on VIDEO_DEV
> > +     depends on MEDIA_CONTROLLER
> > +     select V4L2_ISP
> > +     select VIDEOBUF2_CORE
> > +     help
> > +       Common helper library for Qualcomm CAMSS offline ISP drivers.
> > +       Provides buffer queue management, job scheduling, MC pipeline
> > +       topology builder, and ISP parameter buffer parsing.
> > +
> > +       This module is selected automatically by drivers that need it.
> > +
> > diff --git a/drivers/media/platform/qcom/camss/Makefile b/drivers/media/platform/qcom/camss/Makefile
> > index 5e349b4915130c71dbff90e73102e46dfede1520..bfc05db0eada1d801839ceb8a3b157baae613053 100644
> > --- a/drivers/media/platform/qcom/camss/Makefile
> > +++ b/drivers/media/platform/qcom/camss/Makefile
> > @@ -29,3 +29,8 @@ qcom-camss-objs += \
> >               camss-format.o \
> >
> >   obj-$(CONFIG_VIDEO_QCOM_CAMSS) += qcom-camss.o
> > +
> > +qcom-camss-isp-objs := camss-isp-bufq.o
> > +
> > +obj-$(CONFIG_VIDEO_QCOM_CAMSS_ISP) += qcom-camss-isp.o
> > +
> > diff --git a/drivers/media/platform/qcom/camss/camss-isp-bufq.c b/drivers/media/platform/qcom/camss/camss-isp-bufq.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..b1dcf60afcc63d112eee7bd143f08a7b4aac9a18
> > --- /dev/null
> > +++ b/drivers/media/platform/qcom/camss/camss-isp-bufq.c
> > @@ -0,0 +1,101 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * camss-isp-bufq.c
> > + *
> > + * CAMSS ISP per-queue ready-buffer FIFO.
> > + *
> > + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +
> > +#include "camss-isp-bufq.h"
> > +
> > +struct camss_isp_bufq *camss_isp_bufq_init(unsigned int num_queues)
> > +{
> > +     struct camss_isp_bufq *bufq;
> > +     unsigned int i;
> > +
> > +     bufq = kzalloc(struct_size(bufq, entries, num_queues), GFP_KERNEL);
> > +     if (!bufq)
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     bufq->num_queues = num_queues;
> > +
> > +     for (i = 0; i < num_queues; i++) {
> > +             INIT_LIST_HEAD(&bufq->entries[i].rdy_queue);
> > +             spin_lock_init(&bufq->entries[i].rdy_spinlock);
> > +     }
> > +
> > +     return bufq;
> > +}
> > +EXPORT_SYMBOL_GPL(camss_isp_bufq_init);
> > +
> > +void camss_isp_bufq_release(struct camss_isp_bufq *bufq)
> > +{
> > +     kfree(bufq);
> > +}
> > +EXPORT_SYMBOL_GPL(camss_isp_bufq_release);
> > +
> > +void camss_isp_bufq_queue(struct camss_isp_bufq *bufq, unsigned int queue_idx,
> > +                       struct vb2_v4l2_buffer *vbuf)
> > +{
> > +     struct camss_isp_buf *buf =
> > +             container_of(vbuf, struct camss_isp_buf, vb);
> > +     struct camss_isp_bufq_entry *entry = &bufq->entries[queue_idx];
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&entry->rdy_spinlock, flags);
> > +     list_add_tail(&buf->list, &entry->rdy_queue);
> > +     entry->num_rdy++;
> > +     spin_unlock_irqrestore(&entry->rdy_spinlock, flags);
> > +}
> > +EXPORT_SYMBOL_GPL(camss_isp_bufq_queue);
> > +
> > +struct vb2_v4l2_buffer *camss_isp_bufq_next(struct camss_isp_bufq *bufq, unsigned int queue_idx)
> > +{
> > +     struct camss_isp_bufq_entry *entry = &bufq->entries[queue_idx];
> > +     struct camss_isp_buf *buf;
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&entry->rdy_spinlock, flags);
> > +     buf = list_first_entry_or_null(&entry->rdy_queue,
> > +                                    struct camss_isp_buf, list);
> > +     spin_unlock_irqrestore(&entry->rdy_spinlock, flags);
> > +
> > +     return buf ? &buf->vb : NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(camss_isp_bufq_next);
> > +
> > +struct vb2_v4l2_buffer *camss_isp_bufq_remove(struct camss_isp_bufq *bufq, unsigned int queue_idx)
> > +{
> > +     struct camss_isp_bufq_entry *entry = &bufq->entries[queue_idx];
> > +     struct camss_isp_buf *buf;
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&entry->rdy_spinlock, flags);
> > +     buf = list_first_entry_or_null(&entry->rdy_queue,
> > +                                    struct camss_isp_buf, list);
> > +     if (buf) {
> > +             list_del(&buf->list);
> > +             entry->num_rdy--;
> > +     }
> > +     spin_unlock_irqrestore(&entry->rdy_spinlock, flags);
> > +
> > +     return buf ? &buf->vb : NULL;
> > +}
> > +EXPORT_SYMBOL_GPL(camss_isp_bufq_remove);
> > +
> > +void camss_isp_bufq_drain(struct camss_isp_bufq *bufq, unsigned int queue_idx,
> > +                       enum vb2_buffer_state state)
> > +{
> > +     struct vb2_v4l2_buffer *vbuf;
> > +
> > +     while ((vbuf = camss_isp_bufq_remove(bufq, queue_idx)))
> > +             camss_isp_buf_done(vbuf, state);
> > +}
> > +EXPORT_SYMBOL_GPL(camss_isp_bufq_drain);
> > +
> > +MODULE_DESCRIPTION("CAMSS ISP per-queue ready-buffer FIFO");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/media/platform/qcom/camss/camss-isp-bufq.h b/drivers/media/platform/qcom/camss/camss-isp-bufq.h
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..1a8bc7b112a1b039233cfc7be573f1f40fcda7c9
> > --- /dev/null
> > +++ b/drivers/media/platform/qcom/camss/camss-isp-bufq.h
> > @@ -0,0 +1,122 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * camss-isp-bufq.h
> > + *
> > + * CAMSS ISP per-queue ready-buffer FIFO.
> > + *
> > + * Provides N spinlock-protected FIFO lists of ready vb2 buffers, one per
> > + * queue index.  Drivers call these helpers from their vb2 ops and job
> > + * completion paths.
> > + *
> > + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> > + */
> > +
> > +#ifndef CAMSS_ISP_BUFQ_H
> > +#define CAMSS_ISP_BUFQ_H
> > +
> > +#include <linux/list.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/types.h>
> > +#include <media/videobuf2-v4l2.h>
> > +
> > +/**
> > + * struct camss_isp_buf - vb2 buffer wrapper
> > + *
> > + * Use as vb2_queue.buf_struct_size so buffers can be placed on the
> > + * ready lists managed by camss_isp_bufq.
> > + *
> > + * @vb:   The vb2 V4L2 buffer — must be first.
> > + * @list: Entry in the per-queue ready list.
> > + */
> > +struct camss_isp_buf {
> > +     struct vb2_v4l2_buffer  vb;     /* must be first */
> > +     struct list_head        list;
> > +};
> > +
> > +/**
> > + * struct camss_isp_bufq_entry - per-queue ready-buffer state (opaque)
> > + */
> > +struct camss_isp_bufq_entry {
> > +     struct list_head        rdy_queue;
> > +     spinlock_t              rdy_spinlock;
> > +     u32                     num_rdy;
> > +};
> > +
> > +/**
> > + * struct camss_isp_bufq - multi-queue ready-buffer state
> > + *
> > + * Allocate with camss_isp_bufq_init(), free with camss_isp_bufq_release().
> > + *
> > + * @num_queues: Number of entries in @entries.
> > + * @entries:    Per-queue state; flexible array.
> > + */
> > +struct camss_isp_bufq {
> > +     unsigned int                    num_queues;
> > +     struct camss_isp_bufq_entry     entries[] __counted_by(num_queues);
> > +};
> > +
> > +/**
> > + * camss_isp_bufq_init() - allocate a multi-queue ready-buffer state
> > + * @num_queues: number of per-queue FIFO lists to create
> > + *
> > + * Returns a pointer to the new bufq or ERR_PTR on allocation failure.
> > + */
> > +struct camss_isp_bufq *camss_isp_bufq_init(unsigned int num_queues);
> > +
> > +/**
> > + * camss_isp_bufq_release() - free a bufq allocated with camss_isp_bufq_init()
> > + * @bufq: bufq to free
> > + */
> > +void camss_isp_bufq_release(struct camss_isp_bufq *bufq);
> > +
> > +/**
> > + * camss_isp_bufq_queue() - append a buffer to the ready list for @queue_idx
> > + * @bufq:      target bufq
> > + * @queue_idx: queue index (must be < bufq->num_queues)
> > + * @vbuf:      buffer to enqueue; must be embedded in a &struct camss_isp_buf
> > + */
> > +void camss_isp_bufq_queue(struct camss_isp_bufq *bufq, unsigned int queue_idx,
> > +                        struct vb2_v4l2_buffer *vbuf);
> > +
> > +/**
> > + * camss_isp_bufq_next() - peek at the head of the ready list without removing
> > + * @bufq:      target bufq
> > + * @queue_idx: queue index
> > + *
> > + * Returns the head buffer or NULL if the list is empty.
> > + */
> > +struct vb2_v4l2_buffer *camss_isp_bufq_next(struct camss_isp_bufq *bufq,
> > +                                          unsigned int queue_idx);
> > +
> > +/**
> > + * camss_isp_bufq_remove() - dequeue and return the head of the ready list
> > + * @bufq:      target bufq
> > + * @queue_idx: queue index
> > + *
> > + * Returns the dequeued buffer or NULL if the list is empty.
> > + */
> > +struct vb2_v4l2_buffer *camss_isp_bufq_remove(struct camss_isp_bufq *bufq,
> > +                                            unsigned int queue_idx);
> > +
> > +/**
> > + * camss_isp_bufq_drain() - return all ready buffers with the given state
> > + * @bufq:      target bufq
> > + * @queue_idx: queue index
> > + * @state:     vb2 state to pass to vb2_buffer_done() for each buffer
> > + */
> > +void camss_isp_bufq_drain(struct camss_isp_bufq *bufq, unsigned int queue_idx,
> > +                        enum vb2_buffer_state state);
> > +
> > +static inline u32 camss_isp_bufq_num_ready(struct camss_isp_bufq *bufq,
> > +                                         unsigned int queue_idx)
> > +{
> > +     return bufq->entries[queue_idx].num_rdy;
> > +}
> > +
> > +static inline void camss_isp_buf_done(struct vb2_v4l2_buffer *vbuf,
> > +                                    enum vb2_buffer_state state)
> > +{
> > +     vb2_buffer_done(&vbuf->vb2_buf, state);
> > +}
> > +
> > +#endif /* CAMSS_ISP_BUFQ_H */
> >
>
> I honsestly don't think patches 4,5 and 6 are necessary and TBH they
> look at least partially generated to me.
>
> Several LLM patterns abound - em - dash and (parenthetical style) as an
> example.
>
> I just wonder is all of this code really necessary ? You could do all of
> this locking in the OPE itself and save ~200 LOC.

I'm inclined to agree there is no real added value in this change at
the moment, and that it can easily be handled within the OPE
code/structures. I’ll move it into OPE in the next version.

> I think in the previous cycle we discussed articulating some of these
> concepts in v4l2 itself - I think you could achieve what you want to do
> here with a struct list_head and a spinlock_t in the OPE driver context.

Yes, there are ongoing long-term discussions about improving framework
support for drivers requiring multi-context or job-based handling
(e.g., offline processing engines). Since these are longer-term
efforts, the current approach is to have OPE components such as job
handling, scheduling, and buffer management reasonably abstracted,
making it easier to transition to a generic V4L2/media solution when
it becomes available.

So I will (re)integrate this buf/q management in the OPE driver (4),
but can I have a second thought about 5 and 6? I agree they may not be
worth to be their own patches and modules but would it be ok to keep them
in separated files, creating a camss-offline/ or camss-ope/ directory in
camss in which I could have the OPE driver files (i.e  camss_ope.c,
camss_job.c and camss_pipeline.c), that would also allow a future
ICP/HFI based driver to be integrated and use the same components?

Regards,
Loic