Hi Michael,

Thank you for the patch!

Is it possible to sent the v4l2-compliance output in the next version ?

On Wed, Apr 30, 2025 at 11:15:55AM +0200, Michael Riesch via B4 Relay wrote:

From: Michael Riesch <redacted>

SNIP

+irqreturn_t rkcif_dvp_isr(int irq, void *ctx)
+{
+	struct device *dev = ctx;
+	struct rkcif_device *rkcif = dev_get_drvdata(dev);
+	struct rkcif_stream *stream;
+	u32 intstat, lastline, lastpix, cif_frmst;
+	irqreturn_t ret = IRQ_NONE;
+
+	if (!rkcif->match_data->dvp)
+		return ret;
+
+	intstat = cif_dvp_read(rkcif, RKCIF_DVP_INTSTAT);
+	cif_frmst = cif_dvp_read(rkcif, RKCIF_DVP_FRAME_STATUS);
+	lastline = RKCIF_FETCH_Y(cif_dvp_read(rkcif, RKCIF_DVP_LAST_LINE));
+	lastpix = RKCIF_FETCH_Y(cif_dvp_read(rkcif, RKCIF_DVP_LAST_PIX));
+
+	if (intstat & RKCIF_INTSTAT_FRAME_END) {
+		cif_dvp_write(rkcif, RKCIF_DVP_INTSTAT,
+			      RKCIF_INTSTAT_FRAME_END_CLR |
+				      RKCIF_INTSTAT_LINE_END_CLR);
+
+		stream = &rkcif->interfaces[RKCIF_DVP].streams[RKCIF_ID0];
+
+		if (stream->stopping) {
+			cif_dvp_stop_streaming(stream);
+			wake_up(&stream->wq_stopped);
+			return IRQ_HANDLED;
+		}
+
+		if (lastline != stream->pix.height) {
+			v4l2_err(&rkcif->v4l2_dev,
+				 "bad frame, irq:%#x frmst:%#x size:%dx%d\n",
+				 intstat, cif_frmst, lastpix, lastline);
+
+			cif_dvp_reset_stream(rkcif);
+		}
+
+		rkcif_stream_pingpong(stream);
+
+		ret = IRQ_HANDLED;

just return IRQ_HANDLED like above ?

+	}
+
+	return ret;
+}
+
+int rkcif_dvp_register(struct rkcif_device *rkcif)
+{
+	struct rkcif_interface *interface;
+	int ret, i;
+
+	if (!rkcif->match_data->dvp)
+		return 0;
+
+	interface = &rkcif->interfaces[RKCIF_DVP];
+	interface->index = RKCIF_DVP;
+	interface->type = RKCIF_IF_DVP;
+	interface->in_fmts = rkcif->match_data->dvp->in_fmts;
+	interface->in_fmts_num = rkcif->match_data->dvp->in_fmts_num;
+	interface->set_crop = rkcif_dvp_set_crop;
+	ret = rkcif_interface_register(rkcif, interface);
+	if (ret)
+		return 0;

		|
		+-> Copy-paste error ?

+
+	if (rkcif->match_data->dvp->setup)
+		rkcif->match_data->dvp->setup(rkcif);
+
+	interface->streams_num = rkcif->match_data->dvp->has_ids ? 4 : 1;
+	for (i = 0; i < interface->streams_num; i++) {
+		struct rkcif_stream *stream = &interface->streams[i];
+
+		stream->id = i;
+		stream->interface = interface;
+		stream->out_fmts = rkcif->match_data->dvp->out_fmts;
+		stream->out_fmts_num = rkcif->match_data->dvp->out_fmts_num;
+		stream->queue_buffer = cif_dvp_queue_buffer;
+		stream->start_streaming = cif_dvp_start_streaming;
+		stream->stop_streaming = cif_dvp_stop_streaming;
+
+		ret = rkcif_stream_register(rkcif, stream);
+		if (ret)
+			goto err_streams_unregister;
+	}
+	return 0;
+
+err_streams_unregister:
+	for (; i >= 0; i--)
+		rkcif_stream_unregister(&interface->streams[i]);
+	rkcif_interface_unregister(interface);
+
+	return ret;
+}
+

SNIP

+static inline struct rkcif_buffer *to_rkcif_buffer(struct vb2_v4l2_buffer *vb)
+{
+	return container_of(vb, struct rkcif_buffer, vb);
+}
+
+static inline struct rkcif_stream *to_rkcif_stream(struct video_device *vdev)
+{
+	return container_of(vdev, struct rkcif_stream, vdev);
+}
+
+static struct rkcif_buffer *rkcif_stream_pop_buffer(struct rkcif_stream *stream)
+{
+	struct rkcif_buffer *buffer = NULL;
+	unsigned long lock_flags;
+
+	spin_lock_irqsave(&stream->driver_queue_lock, lock_flags);

guard(spinlock_irqsave)(&stream->driver_queue_lock) will simplify this function.

+
+	if (list_empty(&stream->driver_queue))
+		goto err_empty;
+
+	buffer = list_first_entry(&stream->driver_queue, struct rkcif_buffer,
+				  queue);
+	list_del(&buffer->queue);
+
+err_empty:
+	spin_unlock_irqrestore(&stream->driver_queue_lock, lock_flags);
+	return buffer;
+}
+
+static void rkcif_stream_push_buffer(struct rkcif_stream *stream,
+				     struct rkcif_buffer *buffer)
+{
+	unsigned long lock_flags;
+
+	spin_lock_irqsave(&stream->driver_queue_lock, lock_flags);
+	list_add_tail(&buffer->queue, &stream->driver_queue);
+	spin_unlock_irqrestore(&stream->driver_queue_lock, lock_flags);
+}
+
+static inline void rkcif_stream_return_buffer(struct rkcif_buffer *buffer,
+					      enum vb2_buffer_state state)
+{
+	struct vb2_v4l2_buffer *vb = &buffer->vb;
+
+	vb2_buffer_done(&vb->vb2_buf, state);
+}
+
+static void rkcif_stream_complete_buffer(struct rkcif_stream *stream,
+					 struct rkcif_buffer *buffer)
+{
+	struct vb2_v4l2_buffer *vb = &buffer->vb;
+
+	vb->vb2_buf.timestamp = ktime_get_ns();
+	vb->sequence = stream->frame_idx;
+	vb2_buffer_done(&vb->vb2_buf, VB2_BUF_STATE_DONE);
+	stream->frame_idx++;
+}
+
+void rkcif_stream_pingpong(struct rkcif_stream *stream)
+{
+	struct rkcif_buffer *buffer;
+
+	buffer = stream->buffers[stream->frame_phase];
+	if (!buffer->is_dummy)
+		rkcif_stream_complete_buffer(stream, buffer);

You can actually keep this frame dropping mechanism without using the
dummy buffer.

You can set a drop flag to TRUE: keep overwriting the buffer you already have
without returning it to user-space until you can get another buffer, set
the flag again to FALSE and resume returning the buffers to user-space.

+
+	buffer = rkcif_stream_pop_buffer(stream);
+	if (buffer) {
+		stream->buffers[stream->frame_phase] = buffer;
+		stream->buffers[stream->frame_phase]->is_dummy = false;
+	} else {
+		stream->buffers[stream->frame_phase] = &stream->dummy.buffer;
+		stream->buffers[stream->frame_phase]->is_dummy = true;
+		dev_warn(stream->rkcif->dev,
+			 "no buffer available, frame will be dropped\n");

This warning can quickly flood the kernel logs if the user-space is too slow in
enqueuing the buffers.

+	}
+
+	if (stream->queue_buffer)
+		stream->queue_buffer(stream, stream->frame_phase);

is this if statement really needed ?

+
+	stream->frame_phase = 1 - stream->frame_phase;
+}
+
+static int rkcif_stream_init_buffers(struct rkcif_stream *stream)
+{
+	struct v4l2_pix_format_mplane *pix = &stream->pix;
+	int i;
+
+	stream->buffers[0] = rkcif_stream_pop_buffer(stream);
+	if (!stream->buffers[0])
+		goto err_buff_0;
+
+	stream->buffers[1] = rkcif_stream_pop_buffer(stream);
+	if (!stream->buffers[1])
+		goto err_buff_1;
+
+	if (stream->queue_buffer) {
+		stream->queue_buffer(stream, 0);
+		stream->queue_buffer(stream, 1);
+	}
+
+	stream->dummy.size = pix->num_planes * pix->plane_fmt[0].sizeimage;
+	stream->dummy.vaddr =
+		dma_alloc_attrs(stream->rkcif->dev, stream->dummy.size,
+				&stream->dummy.buffer.buff_addr[0], GFP_KERNEL,
+				DMA_ATTR_NO_KERNEL_MAPPING);
+	if (!stream->dummy.vaddr)
+		goto err_dummy;
+
+	for (i = 1; i < pix->num_planes; i++)
+		stream->dummy.buffer.buff_addr[i] =
+			stream->dummy.buffer.buff_addr[i - 1] +
+			pix->plane_fmt[i - 1].bytesperline * pix->height;
+
+	return 0;
+
+err_dummy:
+	rkcif_stream_return_buffer(stream->buffers[1], VB2_BUF_STATE_QUEUED);
+	stream->buffers[1] = NULL;
+
+err_buff_1:
+	rkcif_stream_return_buffer(stream->buffers[0], VB2_BUF_STATE_QUEUED);
+	stream->buffers[0] = NULL;
+err_buff_0:
+	return -EINVAL;
+}
+

SNIP

+static int rkcif_stream_init_vb2_queue(struct vb2_queue *q,
+				       struct rkcif_stream *stream)
+{
+	q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
+	q->io_modes = VB2_MMAP | VB2_DMABUF;
+	q->drv_priv = stream;
+	q->ops = &rkcif_stream_vb2_ops;
+	q->mem_ops = &vb2_dma_contig_memops;
+	q->buf_struct_size = sizeof(struct rkcif_buffer);
+	q->min_queued_buffers = CIF_REQ_BUFS_MIN;

If I recall correctly min_queued_buffers should be the strict minimum
number of buffers you need to start streaming. So in this case it should
be 3 = 2 pingpong buffers + 1 dummy buffer.

VIDIOC_REQBUFS will allocate min_queued_buffers + 1 and user-space will
probably allocate even more anyway.

+	q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;
+	q->lock = &stream->vlock;
+	q->dev = stream->rkcif->dev;
+
+	return vb2_queue_init(q);
+}

--
Kind Regards
Mehdi Djait