linux-kernel - Re: [PATCH v6 3/4] media: raspberrypi: Add support for RP1-CFE

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241024120507.GA6081@pendragon.ideasonboard.com>
Date: Thu, 24 Oct 2024 15:05:07 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Hans Verkuil <hverkuil@...all.nl>
Cc: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Raspberry Pi Kernel Maintenance <kernel-list@...pberrypi.com>,
	Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Florian Fainelli <florian.fainelli@...adcom.com>,
	Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org, linux-rpi-kernel@...ts.infradead.org,
	linux-arm-kernel@...ts.infradead.org,
	Naushir Patuck <naush@...pberrypi.com>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	Jacopo Mondi <jacopo.mondi@...asonboard.com>,
	Kieran Bingham <kieran.bingham@...asonboard.com>
Subject: Re: [PATCH v6 3/4] media: raspberrypi: Add support for RP1-CFE

On Thu, Oct 24, 2024 at 01:21:43PM +0200, Hans Verkuil wrote:
> On 24/10/2024 13:08, Tomi Valkeinen wrote:
> > On 24/10/2024 11:20, Hans Verkuil wrote:
> >> Hi Tomi,
> >>
> >> I know this driver is already merged, but while checking for drivers that use
> >> q->max_num_buffers I stumbled on this cfe code:
> >>
> >> <snip>
> >>
> >>> +/*
> >>> + * vb2 ops
> >>> + */
> >>> +
> >>> +static int cfe_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
> >>> +               unsigned int *nplanes, unsigned int sizes[],
> >>> +               struct device *alloc_devs[])
> >>> +{
> >>> +    struct cfe_node *node = vb2_get_drv_priv(vq);
> >>> +    struct cfe_device *cfe = node->cfe;
> >>> +    unsigned int size = is_image_node(node) ?
> >>> +                    node->vid_fmt.fmt.pix.sizeimage :
> >>> +                    node->meta_fmt.fmt.meta.buffersize;
> >>> +
> >>> +    cfe_dbg(cfe, "%s: [%s] type:%u\n", __func__, node_desc[node->id].name,
> >>> +        node->buffer_queue.type);
> >>> +
> >>> +    if (vq->max_num_buffers + *nbuffers < 3)
> >>> +        *nbuffers = 3 - vq->max_num_buffers;
> >>
> >> This makes no sense: max_num_buffers is 32, unless explicitly set when vb2_queue_init
> >> is called. So 32 + *nbuffers is never < 3.
> >>
> >> If the idea is that at least 3 buffers should be allocated by REQBUFS, then set
> >> q->min_reqbufs_allocation = 3; before calling vb2_queue_init and vb2 will handle this
> >> for you.
> >>
> >> Drivers shouldn't modify *nbuffers, except in very rare circumstances, especially
> >> since the code is almost always wrong.
> > 
> > Indeed, the code doesn't make sense. I have to say I don't know what
> > was the intent here, but I think "at least 3 buffers should be
> > allocated by REQBUFS" is the likely explanation.
> > 
> > I think the hardware should work with even just a single buffer, so
> > is it then fine to not set either q->min_queued_buffers nor
> > q->min_reqbufs_allocation before calling vb2_queue_init()? This
> > seems to result in REQBUFS giving at least two buffers.
> 
> min_queued_buffers is really HW dependent. If not set, then
> start_streaming can be called even if there are no buffers queued.

Having min_queued_buffers > 1 is bad for userspace, and it's much nicer
to have it set to 0. The main issue with a value of 1 is that validation
of the pipeline ends up being deferred to the first QBUF if it occurs
after STREAMON, and error handling is then complicated.

It's not just a property of the hardware, kernel drivers can decide to
work with scratch buffers if needed. In many cases, a scratch buffer
allocated by the kernel could be very small, either relying on the same
physical page being mapped through the IOMMU to a larger DMA space, or
using a 0 stride value to write all lines to the same location.

For drivers supported by libcamera, we will require min_queued_buffers
<= 1 and may tighten that to == 0. Tomi, if you submit a patch, please
try to target 0, and if that's too much work for now, set it to 1 at
most.

> If your hardware can handle that, then it's fine to not set it.
> 
> >>> +
> >>> +    if (*nplanes) {
> >>> +        if (sizes[0] < size) {
> >>> +            cfe_err(cfe, "sizes[0] %i < size %u\n", sizes[0], size);
> >>> +            return -EINVAL;
> >>> +        }
> >>> +        size = sizes[0];
> >>> +    }
> >>> +
> >>> +    *nplanes = 1;
> >>> +    sizes[0] = size;
> >>> +
> >>> +    return 0;
> >>> +}

-- 
Regards,

Laurent Pinchart