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: <3a5f7d52-fbe9-4592-ab24-50cd6de548c2@xs4all.nl>
Date: Thu, 24 Oct 2024 14:13:07 +0200
From: Hans Verkuil <hverkuil@...all.nl>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
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 24/10/2024 14:05, Laurent Pinchart wrote:
> 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.

The validation can be done in the prepare_streaming callback instead of
in start_streaming. prepare_streaming is called at STREAMON time.

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

None of which is possible for some of the older drivers (e.g. TI davinci)
that do not have an IOMMU and require two contiguous buffers before they can
start streaming. But for modern devices you can solve it through a scratch
buffer, that's true.

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

Regards,

	Hans

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