On Mon, 2020-05-18 at 16:48 +0100, Dave Stevenson wrote: > Hi Nicolas > > > + exp_info.ops = &dma_buf_import_ops; > > > + exp_info.size = import.size; > > > + exp_info.flags = O_RDWR; > > > + exp_info.priv = buffer; > > > + > > > + buffer->dma_buf = dma_buf_export(&exp_info); > > > > Could you comment on the need for this second dma_buf? I've only reviewed > > code > > related to mmal-vchiq imports, but it seems to me that it would be saner to > > do > > the unmapping and unattaching explicitly as opposed to having this second > > buffer refcount hit 0. Although, I can imagine this being needed for the > > userspace interface. > > Indeed, as it is needed for the userspace interface it seemed to make > more sense to have common handling rather than two code paths doing > nearly the same thing but in different ways. > Downstream we need a userspace import at least to allow MMAL to set up > zero copy, so unless it raises any real objections then it would be > useful to keep it. > > > When you talk about moving to dmabuf heaps, I've pictured a specific dmabuf > > heap for vc4 that takes care of all the importing and unimporting (aside > > from > > cma allocations). Am I right? If so, I'm pretty confident we can do away > > with > > this. > > (Note I'm talking about the VideoCore4 VPU and other blocks, and not > the vc4 DRM/KMS and V3D drivers) > > No, I'm looking at using the existing cma_heap driver to do the > allocations, and then this driver will import them and handle the > lifetime on behalf of the VPU. There's no need for VPU allocations to > be split off into yet another heap. Fair enough. > One of the things we are trying to get away from is having the gpu_mem > reserved lump that Linux can't get access to at all, so allocating > from the CMA heap and importing to the VPU avoids that. That's great! Will this also apply at some point to the GPU side of things? I vaguely recall having to reserve up to 300M on rpi3 to get mesa to work on a desktop environment. > I'll give some history here, which also hopefully covers your query > over switching mmal-vchiq to zero copy. > > Almost all the VC4 blocks need contiguous memory, so fragmentation was > an issue. To resolve that we (back in Broadcom days) had the > "relocatable heap" - allocations that needed to be locked before > access and unlocked after. Unlocked blocks could be copied and moved > around to free up larger contiguous blocks. These allocations use a > handle instead of a pointer, and have internal refcounting etc. > Basically providing some of the features of an MMU when you don't have > one. > > The original VCSM driver allowed userspace to make a relocatable heap > allocation, lock it, and the kernel to map the relevant pages into the > ARM memory space. Now you have a shared buffer, and VCHIQ no longer > has to copy the data back and forth. (Cache flushing was also > handled). > So MMAL in zero copy mode passes the VPU relocatable heap handle > across in the VCHIQ message, not a pointer to the actual data. VCSM > did the allocation on behalf of the MMAL client, and provides the > mapping and VPU handle to the buffer. This still leaves the allocation > being made from gpu_mem though. > > The rewrite (vc-sm-cma) was to make use of an import feature into the > relocatable heap, termed internally as mem wrapping. Take a CMA > allocation made by something, pass the DMA address and size across to > the VPU, and it can insert it as a relocatable heap object that can be > used in exactly the same way gpu_mem allocations. gpu_mem can now be > shrunk in size :-) It was using a dma-buf as a convenient object to > manage the allocation, and handle importing buffers allocated by other > subsystems > Note that we still have refcounting internally to the relocatable > heap, so at the point the client says it has finished with it, the VPU > may not have done. When the last relocatable heap reference is > released, the kernel gets a callback (VC_SM_MSG_TYPE_RELEASED), and it > is only at that point that it is safe to drop the reference to the > imported dmabuf. > > V4L2 can do the relevant import and wrapping to a relocatable heap > handle as part of the buffer passing. MMAL needs to do it manually > from userspace as VCHIQ is the only in-kernel service that it uses, > hence we need an import ioctl and free mechanism (if the handle is a > dmabuf, then that's close). > > > From a platform level it would be nice to have the userspace ioctl for > importing a dmabuf in mainline, however it isn't necessary for the > V4L2 use cases that we're trying to upstream here. The driver without > userspace API would look pretty much like the one in [1]. I'll try and > update that to include the basic import userspace API to give a > comparison. > I don't mind which way this goes as to whether the userspace ioctl > remains as downstream patches, but losing the dmabuf as the handle > within vc-sm-cma will make that patch huge, and they're almost > guaranteed to diverge. > Ignore the caching ioctls - they're irrelevant. > > I hope that makes the situation a little clearer. Thanks a lot for the in-depth explanation, it does indeed. Actually, it wouldn't hurt to add a subset of this into a text file alongside with the new driver. Regards, Nicolas
Attachment:
signature.asc
Description: This is a digitally signed message part
