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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxx>
>>>>
>>>> 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@xxxxxxxxxxxxxxx>
>>>> Signed-off-by: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
>>>> Signed-off-by: Jacopo Mondi <jacopo@xxxxxxxxxx>
>>>> ---
>>>>  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
> 




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux