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

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

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@oss.qualcomm.com>
> ---
>   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 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.

---
bod