| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <fx5zweayuzo2vcov7i5d6itlizw4bwmr3wwbd4m6mdjsiou5zb@osl3u2ijv3uj>
Date: Tue, 6 May 2025 12:37:34 +0200
From: Mehdi Djait <mehdi.djait@...ux.intel.com>
To: michael.riesch@...labora.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 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 ?
> + }
> +
> + 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
Powered by blists - more mailing lists