Re: [PATCH 00/14] staging: vc04_services: bcm2835-isp support

From: Stefan Wahren <stefan.wahren@i2se.com>
To: linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 00/14] staging: vc04_services: bcm2835-isp support
Date: Fri, 2 Dec 2022 13:25:52 +0100	[thread overview]
Message-ID: <f5b1d2e8-f878-8c41-9891-257012117f31@i2se.com> (raw)
In-Reply-To: <CAPY8ntAK9yh6ZQkuOrZqUn2GC2=qGaJrcxEjWyGep7KcWvxkhQ@mail.gmail.com>

Hi Dave,

Am 02.12.22 um 12:23 schrieb Dave Stevenson:
> Hi Laurent, Umang, and Stefan.
>
> On Fri, 2 Dec 2022 at 09:17, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>> Hi Umang,
>>
>> On Fri, Dec 02, 2022 at 11:57:18AM +0800, Umang Jain wrote:
>>> On 12/2/22 6:45 AM, Stefan Wahren wrote:
>>>> Am 30.11.22 um 11:58 schrieb Umang Jain:
>>>>> On 11/27/22 6:56 AM, Stefan Wahren wrote:
>>>>>> Am 26.11.22 um 17:26 schrieb Umang Jain:
>>>>>>> On 11/26/22 8:12 PM, Stefan Wahren wrote:
>>>>>>>> Am 21.11.22 um 22:47 schrieb Umang Jain:
>>>>>>>>> This series aims to upport bcm2835-isp from the RPi kernel [1] and is a
>>>>>>>>> independent subset of earlier series [2] posted to upport CSI-2/CCP2
>>>>>>>>> receiver IP core("Unicam) + the ISP driver found in BCM283x and compatible
>>>>>>>>> SoCs (namely BCM2711). Unicam is still under active development to work
>>>>>>>>> with multistream support to get into mainline. Hence only the ISP driver
>>>>>>>>> will remain the primary area of this series.
>>>>>>>> thanks for working on this. But honestly i would prefer that vchiq
>>>>>>>> comes out of staging before adding more features. As Greg said
>>>>>>>> some time ago staging is not a place to "dump code and run away".
>>>>>>>> These new files are in the same bad shape as the rest of vc04
>>>>>>>> before the clean-up here in staging started.
>>>>>>> Certainly, I am not here to do that - but I am still learning the ropes.
>>>>>> no problem.
>>>>>>
>>>>>>> If the staging issue is becoming a blocker for bcm2835-isp going
>>>>>>> upstream, I would be happy to help here! Though I must mention that
>>>>>>> I still have limited visibility so my aim would be to chart out a
>>>>>>> plan of things needed to be done to get vc04_services out of staging!
>>>>>> The vchiq driver is in staging since 2016, so every step forwards is
>>>>>> good. Unfortunately all of the low hanging fruits has been gathered.
>>>>>>
>>>>>> For me the most important, but not to tricky steps to get vchiq out
>>>>>> of staging would be:
>>>>>>
>>>>>> * Cleanup logging mechanism
>>>>>>
>>>>>> * Get rid of custom function return values
>>>>>>
>>>>>> There was already an attempt for this [1]
>>>>>>
>>>>>> * Get rid of all non essential global structures and create a proper per
>>>>>> device structure
>>>>>>
>>>>>>>> I agree that VCSM is on the TODO list for vchiq, but this driver
>>>>>>>> is not necessary for making bcm2835-audio & bcm2835-camera leave
>>>>>>>> staging. It just binds more resources on a new feature.
>>>>> bcm2835-camera is the legacy camera stack which probably need to be
>>>>> dropped from hereon...
>>>> I don't not know if there any users left, so i would be careful here.
>>>> Can bcm2835-isp completely replace bcm2835-camera? Sorry, for this
>>>> dumb question but i'm not expert here.
>>> I am careful too here and probably need Input from RaspberryPi in order
>>> to proceed to drop it. But from my perspective - bcm2835-camera is _not_
>>> going out of staging - it'll either sit here (or probably dropped) as
>>> statied from [1]
>>>
>>> ```
>>> + * There are two camera drivers in the kernel for BCM283x - this one
>>> + * and bcm2835-camera (currently in staging).
>>> ```
>>>
>>> The bcm2835-camera is meant to be replaced by unicam [1] , but the ISP
>>> (bcm2835-isp) is meant to be worked with unicam [1]. In fact, I have
>>> mentioned in my cover the testing of bcm2835-isp happened on top of
>>> unicam patches.
>> To be accurate, the bcm2835-camera driver supports the VC4
>> firmware-based camera stack. In that setup, the camera sensors (OV5647
>> or IMX219), CSI-2 receiver (Unicam) and ISP are all controlled by the
>> firmware, which provides a high-level interface towards the kernel. This
>> architecture has been replaced by Linux-side control of the camera
>> sensors (through existing drivers in drivers/media/i2c/), Unicam
>> (through the driver from [1]) and ISP (through this driver). Moving
>> control to the Linux side requires complex processing in userspace,
>> handled by libcamera.
Thanks for clarification. Okay, so Unicam + bcm2835-isp are able to 
handle the old camera (OV5647)?
>>
>> bcm2835-camera is thus replaced by multiple drivers combined with
>> libcamera, and that is the camera stack that is shipped by Raspberry Pi
>> these days. While this may affect some userspace use cases), we will not
>> work on destaging bcm2835-camera, and as far as I'm aware, nobody else
>> is planning to do so either. I don't mind much if the driver stays in
>> staging for some more time, but I'd rather drop it if possible.
> It would be reasonable to drop it at the point that Libcamera can work
> to a similar level with at least the following list of applications:
> - FFmpeg
> - Gstreamer
> - Chromium
> - Firefox
> - Motion
> And that still leaves a huge number of existing V4L2 apps out in the cold.
>
> Do you wish to make any predictions as to when that would be
> achievable? Or even when a v1.0 release of libcamera is going to
> happen?
> Dropping anything prior to those points would be rather premature in my book.
>
>
> The TODOs on bcm2835-camera are:
> 1) Zero copy. That comes almost for free as bcm2835-isp already does
> this, but it does rely on vcsm-cma.
> The main reason I haven't pushed it is that it then requires
> reasonable amounts of CMA heap for all the buffers, which until
> recently haven't been present in the default configurations. With the
> vc4 DRM driver now being default (at least for the vendor kernel) and
> also requiring CMA, making the change makes more sense.
> AFAIK there is no easy way to have one driver choosing between using
> vb2_vmalloc_memops and vb2_dma_contig_memops at runtime, but I may be
> wrong.
> Actually bcm2835_defconfig appears to only allocate a 32MB CMA heap,
> so perhaps we don't get very far.
>
> 2) This isn't workable within the current V4L2 frameworks. The
> multi-planar V4L2 pixel formats are currently allocated as independent
> buffers for each plane, whereas the firmware needs a single buffer
> with (currently) specific offsets for the chroma planes. The
> V4L2/videobuf2 core changes required to implement that are going to be
> significant, and have minimal gain.
> The specific stride handling is already dealt with (set bytesperline
> appropriately), it's the padding of the height to a multiple of 16
> before the chroma planes on YUV420 and NV12 formats that require the
> firmware to do a small amount of repacking. The performance hit is
> actually minimal anyway.
>
> If bcm2835-camera is the only thing holding back vc04_services, then I
> can have a look at it.
No, it's the vchiq interface which needs the work.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel