Re: [PATCH v3 11/17] media: intel/ipu6: input system video capture nodes

[Andreas Helbech Kleist <andreaskleist@gmail.com>]

Hi Bingbu,

A small comment, discovered while using this code for IPU4.

On Thu, 2024-01-11 at 14:55 +0800, bingbu.cao@intel.com wrote:
> From: Bingbu Cao <bingbu.cao@intel.com>
> 
> Register v4l2 video device and setup the vb2 queue to
> support basic video capture. Video streaming callback
> will trigger the input system driver to construct a
> input system stream configuration for firmware based on
> data type and stream ID and then queue buffers to firmware
> to do capture.
> 
> Signed-off-by: Bingbu Cao <bingbu.cao@intel.com>
> ---
>  .../media/pci/intel/ipu6/ipu6-isys-queue.c    |  825 +++++++++++
>  .../media/pci/intel/ipu6/ipu6-isys-queue.h    |   76 +
>  .../media/pci/intel/ipu6/ipu6-isys-video.c    | 1253 +++++++++++++++++
>  .../media/pci/intel/ipu6/ipu6-isys-video.h    |  136 ++
>  4 files changed, 2290 insertions(+)
>  create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
>  create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys-queue.h
>  create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys-video.c
>  create mode 100644 drivers/media/pci/intel/ipu6/ipu6-isys-video.h
> 
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c
> new file mode 100644
> index 000000000000..735d2d642d87
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.c

...

> +void ipu6_isys_queue_buf_done(struct ipu6_isys_buffer *ib)
> +{
> +	struct vb2_buffer *vb = ipu6_isys_buffer_to_vb2_buffer(ib);
> +
> +	if (atomic_read(&ib->str2mmio_flag)) {
> +		vb2_buffer_done(vb, VB2_BUF_STATE_ERROR);
> +		/*
> +		 * Operation on buffer is ended with error and will be reported
> +		 * to the userspace when it is de-queued
> +		 */
> +		atomic_set(&ib->str2mmio_flag, 0);
> +	} else {
> +		vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
> +	}
> +}
> +void ipu6_isys_queue_buf_ready(struct ipu6_isys_stream *stream,
> +			       struct ipu6_fw_isys_resp_info_abi *info)
> +{
> +	struct ipu6_isys_queue *aq = stream->output_pins[info->pin_id].aq;
> +	struct ipu6_isys *isys = stream->isys;
> +	struct device *dev = &isys->adev->auxdev.dev;
> +	struct ipu6_isys_buffer *ib;
> +	struct vb2_buffer *vb;
> +	unsigned long flags;
> +	bool first = true;
> +	struct vb2_v4l2_buffer *buf;
> +
> +	spin_lock_irqsave(&aq->lock, flags);
> +	if (list_empty(&aq->active)) {
> +		spin_unlock_irqrestore(&aq->lock, flags);
> +		dev_err(dev, "active queue empty\n");
> +		return;
> +	}
> +
> +	list_for_each_entry_reverse(ib, &aq->active, head) {
> +		dma_addr_t addr;
> +
> +		vb = ipu6_isys_buffer_to_vb2_buffer(ib);
> +		addr = vb2_dma_contig_plane_dma_addr(vb, 0);
> +
> +		if (info->pin.addr != addr) {
> +			if (first)
> +				dev_err(dev, "Unexpected buffer address %pad\n",
> +					&addr);
> +			first = false;
> +			continue;
> +		}
> +
> +		if (info->error_info.error ==
> +		    IPU6_FW_ISYS_ERROR_HW_REPORTED_STR2MMIO) {
> +			/*
> +			 * Check for error message:
> +			 * 'IPU6_FW_ISYS_ERROR_HW_REPORTED_STR2MMIO'
> +			 */
> +			atomic_set(&ib->str2mmio_flag, 1);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> +		}
> +		dev_dbg(dev, "buffer: found buffer %pad\n", &addr);
> +
> +		buf = to_vb2_v4l2_buffer(vb);
> +		buf->field = V4L2_FIELD_NONE;
> +
> +		list_del(&ib->head);
> +		spin_unlock_irqrestore(&aq->lock, flags);
> +
> +		ipu6_isys_buf_calc_sequence_time(ib, info);
> +
> +		ipu6_isys_queue_buf_done(ib);

Why is `ib->str2mmio_flag`, which is set above an atomic? It seems like
it could just be a function argument to `ipu6_isys_queue_buf_done`,
which is the only place it is used.

> +
> +		return;
> +	}
> +
> +	dev_err(dev, "Failed to find a matching video buffer");
> +
> +	spin_unlock_irqrestore(&aq->lock, flags);
> +}
...
> diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-queue.h b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.h
> new file mode 100644
> index 000000000000..9fb454577bb5
> --- /dev/null
> +++ b/drivers/media/pci/intel/ipu6/ipu6-isys-queue.h
...
> +void ipu6_isys_queue_buf_done(struct ipu6_isys_buffer *ib);
...

This function is only used in ipu6-isys-queue.c, so could be static.


/Andreas