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 >
