Re: [PATCH v2 06/34] staging: vc04_services: Add new vc-sm-cma driver

From: Hans Verkuil <hverkuil@xs4all.nl>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Dave Stevenson <dave.stevenson@raspberrypi.com>
Cc: "Nicolas Saenz Julienne" <nsaenzjulienne@suse.de>,
	"Linux Media Mailing List" <linux-media@vger.kernel.org>,
	"Dave Stevenson" <dave.stevenson@raspberrypi.org>,
	"Kieran Bingham" <kieran.bingham@ideasonboard.com>,
	"Jacopo Mondi" <jacopo@jmondi.org>,
	"Niklas Söderlund" <niklas.soderlund@ragnatech.se>,
	"Naushir Patuck" <naush@raspberrypi.com>
Subject: Re: [PATCH v2 06/34] staging: vc04_services: Add new vc-sm-cma driver
Date: Mon, 18 May 2020 14:06:08 +0200	[thread overview]
Message-ID: <4057ae9a-eccb-1f7f-254e-54ac2c49a4f7@xs4all.nl> (raw)
In-Reply-To: <20200508001119.GI5838@pendragon.ideasonboard.com>

On 08/05/2020 02:11, Laurent Pinchart wrote:
> Hi Dave,
> 
> On Wed, May 06, 2020 at 08:24:38PM +0100, Dave Stevenson wrote:
>> On Wed, 6 May 2020 at 19:04, Nicolas Saenz Julienne wrote:
>>> On Mon, 2020-05-04 at 12:25 +0300, Laurent Pinchart wrote:
>>>> From: Dave Stevenson <dave.stevenson@raspberrypi.org>
>>>>
>>>> Add Broadcom VideoCore Shared Memory support.
>>>>
>>>> This new driver allows contiguous memory blocks to be imported
>>>> into the VideoCore VPU memory map, and manages the lifetime of
>>>> those objects, only releasing the source dmabuf once the VPU has
>>>> confirmed it has finished with it.
>>>
>>> I'm still digesting all this, but a question came up, who is using the
>>> ioctls?
>>
>> We have a userspace library that uses it [1].
>> It is used by things like MMAL to share buffers between the VPU and
>> ARM, rather than having to get VCHI to copy all the data between
>> mirrored buffers.
>>
>> I think what has happened here is that Laurent has picked up the
>> version of the driver from the top of our downstream kernel tree.
>> For libcamera and the ISP driver, we need a significantly smaller
>> feature set, basically import of dmabufs only, no allocations or cache
>> management. For the ISP driver it's mainly dmabuf import from
>> videobuf2 for the image buffers, but there's also a need to pass in
>> lens shading tables which are relatively large. With a small amount of
>> rework in libcamera, we can make it so that we use dma-buf heaps to do
>> the allocation, and pass in a dmabuf fd to the ISP driver to then map
>> onto the VPU. That removes all the ioctls handling from this driver.
>>
>> Downstream we do have other use cases that want to be able to do other
>> functions on shared memory, but that too should be reworkable into
>> using dma-buf heaps for allocations, and vcsm only handles importing
>> dmabufs via an ioctl. All that can be hidden away in the vcsm library,
>> so applications don't care.
>> We've also got some legacy code kicking around, as there was
>> originally a version of the driver that mapped the VPU's memory blocks
>> to the ARM. That's why the vcsm library has two code paths through
>> almost every function - one for each driver.
>>
>> Laurent: What's your view? Halt the review this particular patch for
>> now and rework, or try and get this all integrated?
>> Mainline obviously already has dma-buf heaps merged, whilst I have a
>> PR cherry-picking it back into our downstream 5.4. The main reason it
>> hasn't been merged is that I haven't had a test case to prove it
>> works. The rework should be relatively simple, but will need small
>> updates to both libcamera and ISP driver.
> 
> My preference would be to go for a solution based on dma-buf heap right
> away for mainline, to minimize the code going into staging. There's no
> hurry there, and I can help integrating the changes in libcamera if
> needed.

So just to be clear, these vc-sm-cma ioctls will disappear in the next version?

I had the same question while reviewing this as Nicolas had :-)

Regards,

	Hans

> 
>> [1] https://github.com/raspberrypi/userland/tree/master/host_applications/linux/libs/sm
>>
>>>> Driver upported from the RaspberryPi BSP at revision:
>>>> 890691d1c996 ("staging: vc04_services: Fix vcsm overflow bug when
>>>> counting transactions")
>>>> forward ported to recent mainline kernel version.
>>>>
>>>> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
>>>> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>>> ---
>>>>  drivers/staging/vc04_services/Kconfig         |    2 +
>>>>  drivers/staging/vc04_services/Makefile        |    1 +
>>>>  .../include/linux/broadcom/vc_sm_cma_ioctl.h  |  114 ++
>>>>  .../staging/vc04_services/vc-sm-cma/Kconfig   |   10 +
>>>>  .../staging/vc04_services/vc-sm-cma/Makefile  |   13 +
>>>>  drivers/staging/vc04_services/vc-sm-cma/TODO  |    1 +
>>>>  .../staging/vc04_services/vc-sm-cma/vc_sm.c   | 1732 +++++++++++++++++
>>>>  .../staging/vc04_services/vc-sm-cma/vc_sm.h   |   84 +
>>>>  .../vc04_services/vc-sm-cma/vc_sm_cma_vchi.c  |  505 +++++
>>>>  .../vc04_services/vc-sm-cma/vc_sm_cma_vchi.h  |   63 +
>>>>  .../vc04_services/vc-sm-cma/vc_sm_defs.h      |  300 +++
>>>>  .../vc04_services/vc-sm-cma/vc_sm_knl.h       |   28 +
>>>>  12 files changed, 2853 insertions(+)
>>>>  create mode 100644
>>>> drivers/staging/vc04_services/include/linux/broadcom/vc_sm_cma_ioctl.h
>>>>  create mode 100644 drivers/staging/vc04_services/vc-sm-cma/Kconfig
>>>>  create mode 100644 drivers/staging/vc04_services/vc-sm-cma/Makefile
>>>>  create mode 100644 drivers/staging/vc04_services/vc-sm-cma/TODO
>>>>  create mode 100644 drivers/staging/vc04_services/vc-sm-cma/vc_sm.c
>>>>  create mode 100644 drivers/staging/vc04_services/vc-sm-cma/vc_sm.h
>>>>  create mode 100644 drivers/staging/vc04_services/vc-sm-cma/vc_sm_cma_vchi.c
>>>>  create mode 100644 drivers/staging/vc04_services/vc-sm-cma/vc_sm_cma_vchi.h
>>>>  create mode 100644 drivers/staging/vc04_services/vc-sm-cma/vc_sm_defs.h
>>>>  create mode 100644 drivers/staging/vc04_services/vc-sm-cma/vc_sm_knl.h
> 