Hi Bingbu,
A small comment, discovered while using this code for IPU4.
On Thu, 2024-01-11 at 14:55 +0800, bingbu.cao@xxxxxxxxx wrote:
> From: Bingbu Cao <bingbu.cao@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> ---
> .../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
