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. > [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 -- Regards, Laurent Pinchart
