Message-ID: <90192c74-f5ca-404b-8b95-3df0819e4bc9@collabora.com>
Date: Tue, 6 May 2025 22:32:59 +0200
From: Michael Riesch <michael.riesch@...labora.com>
To: Mehdi Djait <mehdi.djait@...ux.intel.com>
Cc: Maxime Chevallier <maxime.chevallier@...tlin.com>,
 Théo Lebrun <theo.lebrun@...tlin.com>,
 Gerald Loacker <gerald.loacker@...fvision.net>,
 Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
 Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring
 <robh+dt@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
 Kever Yang <kever.yang@...k-chips.com>,
 Nicolas Dufresne <nicolas.dufresne@...labora.com>,
 Sebastian Reichel <sebastian.reichel@...labora.com>,
 Collabora Kernel Team <kernel@...labora.com>,
 Paul Kocialkowski <paulk@...-base.io>,
 Alexander Shiyan <eagle.alexander923@...il.com>,
 Val Packett <val@...kett.cool>, Rob Herring <robh@...nel.org>,
 Philipp Zabel <p.zabel@...gutronix.de>,
 Sakari Ailus <sakari.ailus@...ux.intel.com>, linux-media@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org,
 Michael Riesch <michael.riesch@...fvision.net>,
 Mehdi Djait <mehdi.djait@...tlin.com>
Subject: Re: [PATCH v6 06/13] media: rockchip: add a driver for the rockchip
 camera interface

Hi Mehdi,

Thanks for your review!

On 5/6/25 12:37, Mehdi Djait wrote:
> 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 <michael.riesch@...fvision.net>
>>
> 
> 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 ?

I think I'll go along Bryan's suggestion to make it more consistent.

> 
>> +	}
>> +
>> +	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 ?

Hm. It's not a mistake. But maybe it is a bit misleading.

The point here is that if something fails with registering the DVP, the
driver may continue to register other entities, such as the MIPI capture
thing.

I'll have another look over this mechanism and will try to make it more
comprehensible.

> 
>> +
>> +	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.

I'll guard up these methods in v7.

> 
>> +
>> +	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.

The approach you describe is what the downstream driver does and I am
not really happy with it. A perfectly fine frame is sacrificed in a
buffer starvation situation.

The approach in the patch series at hand follows the example in the
rkisp1 driver, which should be a good reference.

>> +
>> +	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.

True. dev_warn_ratelimited(...)?

> 
>> +	}
>> +
>> +	if (stream->queue_buffer)
>> +		stream->queue_buffer(stream, stream->frame_phase);
> 
> is this if statement really needed ?

I find it good practice to check the callbacks before calling them. But
this is a matter of taste, of course.

> 
>> +
>> +	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.

The dummy buffer is allocated separately and does not need to be
accounted for.

Two pingpong buffers is what the hardware can queue, but in practice, to
start (and, above all, keep on) streaming you'll need more.

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

Is that so? I found that user space relies too much on this minimum
buffer count and experienced several buffer starvation situations
because kernel AND user space were to cheap in terms of buffer count.
Maybe 8 is too many, but in practice four buffers are required at least
for a decent 2160p stream (one ready for DMA write, one ongoing DMA
write, one stable for processing (maybe DRM scanout or whatever the
application is), one spare).

I am open to suggestions but please keep real life situations in mind
and move away from theoretical stand-alone-capture-hw setups.

Thanks and best regards,
Michael


> 
>> +	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

Powered by blists - more mailing lists