Hi Bryan, On Fri, May 8, 2026 at 11:57 AM Bryan O'Donoghue <[email protected]> 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 <[email protected]> > > --- > > 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
