Message-ID: <pmjd65zzypo7kyi3mkpqd4pf6dqz5ssxxhwnicav57trzxt3ni@ph665okjfo2s>
Date: Wed, 7 May 2025 11:36:24 +0200
From: Mehdi Djait <mehdi.djait@...ux.intel.com>
To: Michael Riesch <michael.riesch@...labora.com>, 
	Laurent Pinchart <laurent.pinchart@...asonboard.com>, Hans Verkuil <hans@...erkuil.nl>
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 Michael,

On Tue, May 06, 2025 at 10:32:59PM +0200, Michael Riesch wrote:
> 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.

what if you want to register the DVP interface and it fails ? Maybe two
separate function for rkcif_{dvp,mipi}_interface_register(), call one of
them based on match_data and verify the ret code --> fail if non-zero ?

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

Oh I thought the downstream driver does it with the dummy buffer.

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

Ack.

> 
> >> +
> >> +	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(...)?
> 

Does frame dropping deserve a warning ? If you don't think so, maybe a
debug or info ?

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

so the documentation says:
--------------------------------------------------------------------------
min_queued_buffers is used when a DMA engine cannot be started unless at
least this number of buffers have been queued into the driver.
--------------------------------------------------------------------------

and:
--------------------------------------------------------------------------
VIDIOC_REQBUFS will ensure at least @min_queued_buffers + 1
buffers will be allocated.
--------------------------------------------------------------------------

I also found theses patches:
https://lore.kernel.org/linux-media/20231211133251.150999-1-benjamin.gaignard@collabora.com/
https://lore.kernel.org/all/20241007124225.63463-1-jacopo.mondi@ideasonboard.com/

If I understood correctly there is a difference between:

- the minimal number of buffers to be allocated with VIDIOC_REQBUFS
- the minimal number of buffers to make it possible to start streaming

what you are setting is the latter, which means you need 8 buffers to
even start streaming which should not be the case for rkcif, which
should only need two (of course when using pingpong)

what you mentioned with the minimum number of buffers for a decent stream seems
to point more towards @min_reqbufs_allocation:
--------------------------------------------------------------------------
Drivers can set this if there has to be a certain number of
buffers available for the hardware to work effectively.
--------------------------------------------------------------------------

of course this being said I am not the expert here so feel free to ask
@Laurent @Hans

--
Kind Regards
Mehdi Djait

Powered by blists - more mailing lists