- Linaro-mm-sig - lists.linaro.org

[PATCH 03/15] dma-buf: Document dma-buf implicit fencing/resv fencing rules

by Daniel Vetter

Docs for struct dma_resv are fairly clear: "A reservation object can have attached one exclusive fence (normally associated with write operations) or N shared fences (read operations)." https://dri.freedesktop.org/docs/drm/driver-api/dma-buf.html#reservation-ob… Furthermore a review across all of upstream. First of render drivers and how they set implicit fences: - nouveau follows this contract, see in validate_fini_no_ticket() nouveau_bo_fence(nvbo, fence, !!b->write_domains); and that last boolean controls whether the exclusive or shared fence slot is used. - radeon follows this contract by setting p->relocs[i].tv.num_shared = !r->write_domain; in radeon_cs_parser_relocs(), which ensures that the call to ttm_eu_fence_buffer_objects() in radeon_cs_parser_fini() will do the right thing. - vmwgfx seems to follow this contract with the shotgun approach of always setting ttm_val_buf->num_shared = 0, which means ttm_eu_fence_buffer_objects() will only use the exclusive slot. - etnaviv follows this contract, as can be trivially seen by looking at submit_attach_object_fences() - i915 is a bit a convoluted maze with multiple paths leading to i915_vma_move_to_active(). Which sets the exclusive flag if EXEC_OBJECT_WRITE is set. This can either come as a buffer flag for softpin mode, or through the write_domain when using relocations. It follows this contract. - lima follows this contract, see lima_gem_submit() which sets the exclusive fence when the LIMA_SUBMIT_BO_WRITE flag is set for that bo - msm follows this contract, see msm_gpu_submit() which sets the exclusive flag when the MSM_SUBMIT_BO_WRITE is set for that buffer - panfrost follows this contract with the shotgun approach of just always setting the exclusive fence, see panfrost_attach_object_fences(). Benefits of a single engine I guess - v3d follows this contract with the same shotgun approach in v3d_attach_fences_and_unlock_reservation(), but it has at least an XXX comment that maybe this should be improved - v4c uses the same shotgun approach of always setting an exclusive fence, see vc4_update_bo_seqnos() - vgem also follows this contract, see vgem_fence_attach_ioctl() and the VGEM_FENCE_WRITE. This is used in some igts to validate prime sharing with i915.ko without the need of a 2nd gpu - vritio follows this contract again with the shotgun approach of always setting an exclusive fence, see virtio_gpu_array_add_fence() This covers the setting of the exclusive fences when writing. Synchronizing against the exclusive fence is a lot more tricky, and I only spot checked a few: - i915 does it, with the optional EXEC_OBJECT_ASYNC to skip all implicit dependencies (which is used by vulkan) - etnaviv does this. Implicit dependencies are collected in submit_fence_sync(), again with an opt-out flag ETNA_SUBMIT_NO_IMPLICIT. These are then picked up in etnaviv_sched_dependency which is the drm_sched_backend_ops->dependency callback. - v4c seems to not do much here, maybe gets away with it by not having a scheduler and only a single engine. Since all newer broadcom chips than the OG vc4 use v3d for rendering, which follows this contract, the impact of this issue is fairly small. - v3d does this using the drm_gem_fence_array_add_implicit() helper, which then it's drm_sched_backend_ops->dependency callback v3d_job_dependency() picks up. - panfrost is nice here and tracks the implicit fences in panfrost_job->implicit_fences, which again the drm_sched_backend_ops->dependency callback panfrost_job_dependency() picks up. It is mildly questionable though since it only picks up exclusive fences in panfrost_acquire_object_fences(), but not buggy in practice because it also always sets the exclusive fence. It should pick up both sets of fences, just in case there's ever going to be a 2nd gpu in a SoC with a mali gpu. Or maybe a mali SoC with a pcie port and a real gpu, which might actually happen eventually. A bug, but easy to fix. Should probably use the drm_gem_fence_array_add_implicit() helper. - lima is nice an easy, uses drm_gem_fence_array_add_implicit() and the same schema as v3d. - msm is mildly entertaining. It also supports MSM_SUBMIT_NO_IMPLICIT, but because it doesn't use the drm/scheduler it handles fences from the wrong context with a synchronous dma_fence_wait. See submit_fence_sync() leading to msm_gem_sync_object(). Investing into a scheduler might be a good idea. - all the remaining drivers are ttm based, where I hope they do appropriately obey implicit fences already. I didn't do the full audit there because a) not follow the contract would confuse ttm quite well and b) reading non-standard scheduler and submit code which isn't based on drm/scheduler is a pain. Onwards to the display side. - Any driver using the drm_gem_plane_helper_prepare_fb() helper will correctly. Overwhelmingly most drivers get this right, except a few totally dont. I'll follow up with a patch to make this the default and avoid a bunch of bugs. - I didn't audit the ttm drivers, but given that dma_resv started there I hope they get this right. In conclusion this IS the contract, both as documented and overwhelmingly implemented, specically as implemented by all render drivers except amdgpu. Amdgpu tried to fix this already in commit 049aca4363d8af87cab8d53de5401602db3b9999 Author: Christian König <christian.koenig(a)amd.com> Date: Wed Sep 19 16:54:35 2018 +0200 drm/amdgpu: fix using shared fence for exported BOs v2 but this fix falls short on a number of areas: - It's racy, by the time the buffer is shared it might be too late. To make sure there's definitely never a problem we need to set the fences correctly for any buffer that's potentially exportable. - It's breaking uapi, dma-buf fds support poll() and differentitiate between, which was introduced in commit 9b495a5887994a6d74d5c261d012083a92b94738 Author: Maarten Lankhorst <maarten.lankhorst(a)canonical.com> Date: Tue Jul 1 12:57:43 2014 +0200 dma-buf: add poll support, v3 - Christian König wants to nack new uapi building further on this dma_resv contract because it breaks amdgpu, quoting "Yeah, and that is exactly the reason why I will NAK this uAPI change. "This doesn't works for amdgpu at all for the reasons outlined above." https://lore.kernel.org/dri-devel/f2eb6751-2f82-9b23-f57e-548de5b729de@gmai… Rejecting new development because your own driver is broken and violates established cross driver contracts and uapi is really not how upstream works. Now this patch will have a severe performance impact on anything that runs on multiple engines. So we can't just merge it outright, but need a bit a plan: - amdgpu needs a proper uapi for handling implicit fencing. The funny thing is that to do it correctly, implicit fencing must be treated as a very strange IPC mechanism for transporting fences, where both setting the fence and dependency intercepts must be handled explicitly. Current best practices is a per-bo flag to indicate writes, and a per-bo flag to to skip implicit fencing in the CS ioctl as a new chunk. - Since amdgpu has been shipping with broken behaviour we need an opt-out flag from the butchered implicit fencing model to enable the proper explicit implicit fencing model. - for kernel memory fences due to bo moves at least the i915 idea is to use ttm_bo->moving. amdgpu probably needs the same. - since the current p2p dma-buf interface assumes the kernel memory fence is in the exclusive dma_resv fence slot we need to add a new fence slot for kernel fences, which must never be ignored. Since currently only amdgpu supports this there's no real problem here yet, until amdgpu gains a NO_IMPLICIT CS flag. - New userspace needs to ship in enough desktop distros so that users wont notice the perf impact. I think we can ignore LTS distros who upgrade their kernels but not their mesa3d snapshot. - Then when this is all in place we can merge this patch here. What is not a solution to this problem here is trying to make the dma_resv rules in the kernel more clever. The fundamental issue here is that the amdgpu CS uapi is the least expressive one across all drivers (only equalled by panfrost, which has an actual excuse) by not allowing any userspace control over how implicit sync is conducted. Until this is fixed it's completely pointless to make the kernel more clever to improve amdgpu, because all we're doing is papering over this uapi design issue. amdgpu needs to attain the status quo established by other drivers first, once that's achieved we can tackle the remaining issues in a consistent way across drivers. v2: Bas pointed me at AMDGPU_GEM_CREATE_EXPLICIT_SYNC, which I entirely missed. This is great because it means the amdgpu specific piece for proper implicit fence handling exists already, and that since a while. The only thing that's now missing is - fishing the implicit fences out of a shared object at the right time - setting the exclusive implicit fence slot at the right time. Jason has a patch series to fill that gap with a bunch of generic ioctl on the dma-buf fd: https://lore.kernel.org/dri-devel/20210520190007.534046-1-jason@jlekstrand.… v3: Since Christian has fixed amdgpu now in commit 8c505bdc9c8b955223b054e34a0be9c3d841cd20 (drm-misc/drm-misc-next) Author: Christian König <christian.koenig(a)amd.com> Date: Wed Jun 9 13:51:36 2021 +0200 drm/amdgpu: rework dma_resv handling v3 Use the audit covered in this commit message as the excuse to update the dma-buf docs around dma_buf.resv usage across drivers. Since dynamic importers have different rules also hammer these in again while we're at it. Cc: mesa-dev(a)lists.freedesktop.org Cc: Bas Nieuwenhuizen <bas(a)basnieuwenhuizen.nl> Cc: Dave Airlie <airlied(a)gmail.com> Cc: Rob Clark <robdclark(a)chromium.org> Cc: Kristian H. Kristensen <hoegsberg(a)google.com> Cc: Michel Dänzer <michel(a)daenzer.net> Cc: Daniel Stone <daniels(a)collabora.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: Alex Deucher <alexander.deucher(a)amd.com> Cc: Daniel Vetter <daniel.vetter(a)ffwll.ch> Cc: Deepak R Varma <mh12gx2825(a)gmail.com> Cc: Chen Li <chenli(a)uniontech.com> Cc: Kevin Wang <kevin1.wang(a)amd.com> Cc: Dennis Li <Dennis.Li(a)amd.com> Cc: Luben Tuikov <luben.tuikov(a)amd.com> Cc: linaro-mm-sig(a)lists.linaro.org Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> --- include/linux/dma-buf.h | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index 6d18b9e448b9..4807cefe81f5 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -388,6 +388,45 @@ struct dma_buf { * @resv: * * Reservation object linked to this dma-buf. + * + * IMPLICIT SYNCHRONIZATION RULES: + * + * Drivers which support implicit synchronization of buffer access as + * e.g. exposed in `Implicit Fence Poll Support`_ should follow the + * below rules. + * + * - Drivers should add a shared fence through + * dma_resv_add_shared_fence() for anything the userspace API + * considers a read access. This highly depends upon the API and + * window system: E.g. OpenGL is generally implicitly synchronized on + * Linux, but explicitly synchronized on Android. Whereas Vulkan is + * generally explicitly synchronized for everything, and window system + * buffers have explicit API calls (which then need to make sure the + * implicit fences store here in @resv are updated correctly). + * + * - Similarly drivers should set the exclusive fence through + * dma_resv_add_excl_fence() for anything the userspace API considers + * write access. + * + * - Drivers may just always set the exclusive fence, since that only + * causes unecessarily synchronization, but no correctness issues. + * + * - Some drivers only expose a synchronous userspace API with no + * pipelining across drivers. These do not set any fences for their + * access. An example here is v4l. + * + * DYNAMIC IMPORTER RULES: + * + * Dynamic importers, see dma_buf_attachment_is_dynamic(), have + * additional constraints on how they set up fences: + * + * - Dynamic importers must obey the exclusive fence and wait for it to + * signal before allowing access to the buffer's underlying storage + * through. + * + * - Dynamic importers should set fences for any access that they can't + * disable immediately from their @dma_buf_attach_ops.move_notify + * callback. */ struct dma_resv *resv; -- 2.32.0.rc2

3 years, 11 months

4
7
0 0

Re: [Linaro-mm-sig] [PATCH v3 1/2] habanalabs: define uAPI to export FD for DMA-BUF

by Christoph Hellwig

On Wed, Jun 23, 2021 at 10:39:48PM +0300, Oded Gabbay wrote: > hmm, I thought using dma_map_resource will solve the IOMMU issues, no ? > We talked about it yesterday, and you said that it will "work" > (although I noticed a tone of reluctance when you said that). > > If I use dma_map_resource to set the addresses inside the SGL before I > export the dma-buf, and guarantee no one will use the SGL in the > dma-buf for any other purpose than device p2p, what else is needed ? dma_map_resource works in the sense of that helps with mapping an arbitrary phys_addr_t for DMA. It does not take various pitfalls of PCI P2P into account, such as the offset between the CPU physical address and the PCIe bus address, or the whole support of mapping between two devices behding a switch and not going through the limited root port support. Comparing dma_direct_map_resource/iommu_dma_map_resource with with pci_p2pdma_map_sg_attrs/__pci_p2pdma_map_sg should make that very clear. So if you want a non-page based mapping you need a "resource"-level version of pci_p2pdma_map_sg_attrs. Which totall doable, and in fact mostly trivial. But no one has even looked into providing one and just keeps arguing.

3 years, 11 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v3 1/2] habanalabs: define uAPI to export FD for DMA-BUF

by Jason Gunthorpe

On Wed, Jun 23, 2021 at 10:39:48PM +0300, Oded Gabbay wrote: > On Wed, Jun 23, 2021 at 10:34 PM Jason Gunthorpe <jgg(a)ziepe.ca> wrote: > > > > On Wed, Jun 23, 2021 at 10:00:29PM +0300, Oded Gabbay wrote: > > > On Wed, Jun 23, 2021 at 9:50 PM Jason Gunthorpe <jgg(a)ziepe.ca> wrote: > > > > > > > > On Wed, Jun 23, 2021 at 09:43:04PM +0300, Oded Gabbay wrote: > > > > > > > > > Can you please explain why it is so important to (allow) access them > > > > > through the CPU ? > > > > > > > > It is not so much important, as it reflects significant design choices > > > > that are already tightly baked into alot of our stacks. > > > > > > > > A SGL is CPU accessible by design - that is baked into this thing and > > > > places all over the place assume it. Even in RDMA we have > > > > RXE/SWI/HFI1/qib that might want to use the CPU side (grep for sg_page > > > > to see) > > > > > > > > So, the thing at the top of the stack - in this case the gaudi driver > > > > - simply can't assume what the rest of the stack is going to do and > > > > omit the CPU side. It breaks everything. > > > > > > > > Logan's patch series is the most fully developed way out of this > > > > predicament so far. > > > > > > I understand the argument and I agree that for the generic case, the > > > top of the stack can't assume anything. > > > Having said that, in this case the SGL is encapsulated inside a dma-buf object. > > > > > > Maybe its a stupid/over-simplified suggestion, but can't we add a > > > property to the dma-buf object, > > > that will be set by the exporter, which will "tell" the importer it > > > can't use any CPU fallback ? Only "real" p2p ? > > > > The block stack has been trying to do something like this. > > > > The flag doesn't solve the DMA API/IOMMU problems though. > hmm, I thought using dma_map_resource will solve the IOMMU issues, > no ? dma_map_resource() will configure the IOMMU but it is not the correct API to use when building a SG list for DMA, that would be dma_map_sg or sgtable. So it works, but it is an API abuse to build things this way. > If I use dma_map_resource to set the addresses inside the SGL before I > export the dma-buf, and guarantee no one will use the SGL in the > dma-buf for any other purpose than device p2p, what else is needed ? You still have to check the p2p stuff to ensure that p2p is even possible And this approach is misusing all the APIs and has been NAK'd by Christoph, so up to Greg if he wants to take it or insist you work with Logan to get the proper generlized solution finished. Jason

3 years, 11 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v3 1/2] habanalabs: define uAPI to export FD for DMA-BUF

by Jason Gunthorpe

On Wed, Jun 23, 2021 at 10:00:29PM +0300, Oded Gabbay wrote: > On Wed, Jun 23, 2021 at 9:50 PM Jason Gunthorpe <jgg(a)ziepe.ca> wrote: > > > > On Wed, Jun 23, 2021 at 09:43:04PM +0300, Oded Gabbay wrote: > > > > > Can you please explain why it is so important to (allow) access them > > > through the CPU ? > > > > It is not so much important, as it reflects significant design choices > > that are already tightly baked into alot of our stacks. > > > > A SGL is CPU accessible by design - that is baked into this thing and > > places all over the place assume it. Even in RDMA we have > > RXE/SWI/HFI1/qib that might want to use the CPU side (grep for sg_page > > to see) > > > > So, the thing at the top of the stack - in this case the gaudi driver > > - simply can't assume what the rest of the stack is going to do and > > omit the CPU side. It breaks everything. > > > > Logan's patch series is the most fully developed way out of this > > predicament so far. > > I understand the argument and I agree that for the generic case, the > top of the stack can't assume anything. > Having said that, in this case the SGL is encapsulated inside a dma-buf object. > > Maybe its a stupid/over-simplified suggestion, but can't we add a > property to the dma-buf object, > that will be set by the exporter, which will "tell" the importer it > can't use any CPU fallback ? Only "real" p2p ? The block stack has been trying to do something like this. The flag doesn't solve the DMA API/IOMMU problems though. Jason

3 years, 11 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH 06/15] drm/panfrost: Fix implicit sync

by Daniel Vetter

On Wed, Jun 23, 2021 at 06:47:37PM +0200, Boris Brezillon wrote: > On Tue, 22 Jun 2021 18:55:02 +0200 > Daniel Vetter <daniel.vetter(a)ffwll.ch> wrote: > > > Currently this has no practial relevance I think because there's not > > many who can pull off a setup with panfrost and another gpu in the > > same system. But the rules are that if you're setting an exclusive > > fence, indicating a gpu write access in the implicit fencing system, > > then you need to wait for all fences, not just the previous exclusive > > fence. > > > > panfrost against itself has no problem, because it always sets the > > exclusive fence (but that's probably something that will need to be > > fixed for vulkan and/or multi-engine gpus, or you'll suffer badly). > > Also no problem with that against display. > > > > With the prep work done to switch over to the dependency helpers this > > is now a oneliner. > > > > Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> > > Cc: Rob Herring <robh(a)kernel.org> > > Cc: Tomeu Vizoso <tomeu.vizoso(a)collabora.com> > > Cc: Steven Price <steven.price(a)arm.com> > > Cc: Alyssa Rosenzweig <alyssa.rosenzweig(a)collabora.com> > > Cc: Sumit Semwal <sumit.semwal(a)linaro.org> > > Reviewed-by: Boris Brezillon <boris.brezillon(a)collabora.com> Pushed the 3 panfrost patches to drm-misc-next, thanks for reviewing them. -Daniel > > > Cc: "Christian König" <christian.koenig(a)amd.com> > > Cc: linux-media(a)vger.kernel.org > > Cc: linaro-mm-sig(a)lists.linaro.org > > --- > > drivers/gpu/drm/panfrost/panfrost_job.c | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c > > index 71cd43fa1b36..ef004d587dc4 100644 > > --- a/drivers/gpu/drm/panfrost/panfrost_job.c > > +++ b/drivers/gpu/drm/panfrost/panfrost_job.c > > @@ -203,9 +203,8 @@ static int panfrost_acquire_object_fences(struct drm_gem_object **bos, > > int i, ret; > > > > for (i = 0; i < bo_count; i++) { > > - struct dma_fence *fence = dma_resv_get_excl_unlocked(bos[i]->resv); > > - > > - ret = drm_gem_fence_array_add(deps, fence); > > + /* panfrost always uses write mode in its current uapi */ > > + ret = drm_gem_fence_array_add_implicit(deps, bos[i], true); > > if (ret) > > return ret; > > } > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 11 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v3 1/2] habanalabs: define uAPI to export FD for DMA-BUF

by Jason Gunthorpe

On Wed, Jun 23, 2021 at 09:43:04PM +0300, Oded Gabbay wrote: > Can you please explain why it is so important to (allow) access them > through the CPU ? It is not so much important, as it reflects significant design choices that are already tightly baked into alot of our stacks. A SGL is CPU accessible by design - that is baked into this thing and places all over the place assume it. Even in RDMA we have RXE/SWI/HFI1/qib that might want to use the CPU side (grep for sg_page to see) So, the thing at the top of the stack - in this case the gaudi driver - simply can't assume what the rest of the stack is going to do and omit the CPU side. It breaks everything. Logan's patch series is the most fully developed way out of this predicament so far. > The whole purpose is that the other device accesses my device, > bypassing the CPU. Sure, but you don't know that will happen, or if it is even possible in any given system configuration. The purpose is to allow for that optimization when possible, not exclude CPU based approaches. Jason

3 years, 11 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v3 1/2] habanalabs: define uAPI to export FD for DMA-BUF

by Jason Gunthorpe

On Tue, Jun 22, 2021 at 11:42:27AM +0300, Oded Gabbay wrote: > On Tue, Jun 22, 2021 at 9:37 AM Christian König > <ckoenig.leichtzumerken(a)gmail.com> wrote: > > > > Am 22.06.21 um 01:29 schrieb Jason Gunthorpe: > > > On Mon, Jun 21, 2021 at 10:24:16PM +0300, Oded Gabbay wrote: > > > > > >> Another thing I want to emphasize is that we are doing p2p only > > >> through the export/import of the FD. We do *not* allow the user to > > >> mmap the dma-buf as we do not support direct IO. So there is no access > > >> to these pages through the userspace. > > > Arguably mmaping the memory is a better choice, and is the direction > > > that Logan's series goes in. Here the use of DMABUF was specifically > > > designed to allow hitless revokation of the memory, which this isn't > > > even using. > > > > The major problem with this approach is that DMA-buf is also used for > > memory which isn't CPU accessible. That isn't an issue here because the memory is only intended to be used with P2P transfers so it must be CPU accessible. > > That was one of the reasons we didn't even considered using the mapping > > memory approach for GPUs. Well, now we have DEVICE_PRIVATE memory that can meet this need too.. Just nobody has wired it up to hmm_range_fault() > > > So you are taking the hit of very limited hardware support and reduced > > > performance just to squeeze into DMABUF.. > > Thanks Jason for the clarification, but I honestly prefer to use > DMA-BUF at the moment. > It gives us just what we need (even more than what we need as you > pointed out), it is *already* integrated and tested in the RDMA > subsystem, and I'm feeling comfortable using it as I'm somewhat > familiar with it from my AMD days. You still have the issue that this patch is doing all of this P2P stuff wrong - following the already NAK'd AMD approach. > I'll go and read Logan's patch-set to see if that will work for us in > the future. Please remember, as Daniel said, we don't have struct page > backing our device memory, so if that is a requirement to connect to > Logan's work, then I don't think we will want to do it at this point. It is trivial to get the struct page for a PCI BAR. Jason

3 years, 11 months

4
9
0 0

[PATCH 02/15] dma-buf: Switch to inline kerneldoc

by Daniel Vetter

Also review & update everything while we're at it. This is prep work to smash a ton of stuff into the kerneldoc for @resv. Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: Alex Deucher <alexander.deucher(a)amd.com> Cc: Daniel Vetter <daniel.vetter(a)ffwll.ch> Cc: Dave Airlie <airlied(a)redhat.com> Cc: Nirmoy Das <nirmoy.das(a)amd.com> Cc: Deepak R Varma <mh12gx2825(a)gmail.com> Cc: Chen Li <chenli(a)uniontech.com> Cc: Kevin Wang <kevin1.wang(a)amd.com> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org --- include/linux/dma-buf.h | 107 +++++++++++++++++++++++++++++++--------- 1 file changed, 83 insertions(+), 24 deletions(-) diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index 92eec38a03aa..6d18b9e448b9 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -289,28 +289,6 @@ struct dma_buf_ops { /** * struct dma_buf - shared buffer object - * @size: size of the buffer; invariant over the lifetime of the buffer. - * @file: file pointer used for sharing buffers across, and for refcounting. - * @attachments: list of dma_buf_attachment that denotes all devices attached, - * protected by dma_resv lock. - * @ops: dma_buf_ops associated with this buffer object. - * @lock: used internally to serialize list manipulation, attach/detach and - * vmap/unmap - * @vmapping_counter: used internally to refcnt the vmaps - * @vmap_ptr: the current vmap ptr if vmapping_counter > 0 - * @exp_name: name of the exporter; useful for debugging. - * @name: userspace-provided name; useful for accounting and debugging, - * protected by @resv. - * @name_lock: spinlock to protect name access - * @owner: pointer to exporter module; used for refcounting when exporter is a - * kernel module. - * @list_node: node for dma_buf accounting and debugging. - * @priv: exporter specific private data for this buffer object. - * @resv: reservation object linked to this dma-buf - * @poll: for userspace poll support - * @cb_excl: for userspace poll support - * @cb_shared: for userspace poll support - * @sysfs_entry: for exposing information about this buffer in sysfs. * The attachment_uid member of @sysfs_entry is protected by dma_resv lock * and is incremented on each attach. * @@ -324,24 +302,100 @@ struct dma_buf_ops { * Device DMA access is handled by the separate &struct dma_buf_attachment. */ struct dma_buf { + /** + * @size: + * + * Size of the buffer; invariant over the lifetime of the buffer. + */ size_t size; + + /** + * @file: + * + * File pointer used for sharing buffers across, and for refcounting. + * See dma_buf_get() and dma_buf_put(). + */ struct file *file; + + /** + * @attachments: + * + * List of dma_buf_attachment that denotes all devices attached, + * protected by &dma_resv lock @resv. + */ struct list_head attachments; + + /** @ops: dma_buf_ops associated with this buffer object. */ const struct dma_buf_ops *ops; + + /** + * @lock: + * + * Used internally to serialize list manipulation, attach/detach and + * vmap/unmap. Note that in many cases this is superseeded by + * dma_resv_lock() on @resv. + */ struct mutex lock; + + /** + * @vmapping_counter: + * + * Used internally to refcnt the vmaps returned by dma_buf_vmap(). + * Protected by @lock. + */ unsigned vmapping_counter; + + /** + * @vmap_ptr: + * The current vmap ptr if @vmapping_counter > 0. Protected by @lock. + */ struct dma_buf_map vmap_ptr; + + /** + * @exp_name: + * + * Name of the exporter; useful for debugging. See the + * DMA_BUF_SET_NAME IOCTL. + */ const char *exp_name; + + /** + * @name: + * + * Userspace-provided name; useful for accounting and debugging, + * protected by dma_resv_lock() on @resv and @name_lock for read access. + */ const char *name; + + /** @name_lock: Spinlock to protect name acces for read access. */ spinlock_t name_lock; + + /** + * @owner: + * + * Pointer to exporter module; used for refcounting when exporter is a + * kernel module. + */ struct module *owner; + + /** @list_node: node for dma_buf accounting and debugging. */ struct list_head list_node; + + /** @priv: exporter specific private data for this buffer object. */ void *priv; + + /** + * @resv: + * + * Reservation object linked to this dma-buf. + */ struct dma_resv *resv; - /* poll support */ + /** @poll: for userspace poll support */ wait_queue_head_t poll; + /** @cb_excl: for userspace poll support */ + /** @cb_shared: for userspace poll support */ struct dma_buf_poll_cb_t { struct dma_fence_cb cb; wait_queue_head_t *poll; @@ -349,7 +403,12 @@ struct dma_buf { __poll_t active; } cb_excl, cb_shared; #ifdef CONFIG_DMABUF_SYSFS_STATS - /* for sysfs stats */ + /** + * @sysfs_entry: + * + * For exposing information about this buffer in sysfs. See also + * `DMA-BUF statistics`_ for the uapi this enables. + */ struct dma_buf_sysfs_entry { struct kobject kobj; struct dma_buf *dmabuf; -- 2.32.0.rc2

3 years, 11 months

4
6
0 0

[PATCH 15/15] RFC: drm/amdgpu: Implement a proper implicit fencing uapi

by Daniel Vetter

WARNING: Absolutely untested beyond "gcc isn't dying in agony". Implicit fencing done properly needs to treat the implicit fencing slots like a funny kind of IPC mailbox. In other words it needs to be explicitly. This is the only way it will mesh well with explicit fencing userspace like vk, and it's also the bare minimum required to be able to manage anything else that wants to use the same buffer on multiple engines in parallel, and still be able to share it through implicit sync. amdgpu completely lacks such an uapi. Fix this. Luckily the concept of ignoring implicit fences exists already, and takes care of all the complexities of making sure that non-optional fences (like bo moves) are not ignored. This support was added in commit 177ae09b5d699a5ebd1cafcee78889db968abf54 Author: Andres Rodriguez <andresx7(a)gmail.com> Date: Fri Sep 15 20:44:06 2017 -0400 drm/amdgpu: introduce AMDGPU_GEM_CREATE_EXPLICIT_SYNC v2 Unfortuantely it's the wrong semantics, because it's a bo flag and disables implicit sync on an allocated buffer completely. We _do_ want implicit sync, but control it explicitly. For this we need a flag on the drm_file, so that a given userspace (like vulkan) can manage the implicit sync slots explicitly. The other side of the pipeline (compositor, other process or just different stage in a media pipeline in the same process) can then either do the same, or fully participate in the implicit sync as implemented by the kernel by default. By building on the existing flag for buffers we avoid any issues with opening up additional security concerns - anything this new flag here allows is already. All drivers which supports this concept of a userspace-specific opt-out of implicit sync have a flag in their CS ioctl, but in reality that turned out to be a bit too inflexible. See the discussion below, let's try to do a bit better for amdgpu. This alone only allows us to completely avoid any stalls due to implicit sync, it does not yet allow us to use implicit sync as a strange form of IPC for sync_file. For that we need two more pieces: - a way to get the current implicit sync fences out of a buffer. Could be done in a driver ioctl, but everyone needs this, and generally a dma-buf is involved anyway to establish the sharing. So an ioctl on the dma-buf makes a ton more sense: https://lore.kernel.org/dri-devel/20210520190007.534046-4-jason@jlekstrand.… Current drivers in upstream solves this by having the opt-out flag on their CS ioctl. This has the downside that very often the CS which must actually stall for the implicit fence is run a while after the implicit fence point was logically sampled per the api spec (vk passes an explicit syncobj around for that afaiui), and so results in oversync. Converting the implicit sync fences into a snap-shot sync_file is actually accurate. - Simillar we need to be able to set the exclusive implicit fence. Current drivers again do this with a CS ioctl flag, with again the same problems that the time the CS happens additional dependencies have been added. An explicit ioctl to only insert a sync_file (while respecting the rules for how exclusive and shared fence slots must be update in struct dma_resv) is much better. This is proposed here: https://lore.kernel.org/dri-devel/20210520190007.534046-5-jason@jlekstrand.… These three pieces together allow userspace to fully control implicit fencing and remove all unecessary stall points due to them. Well, as much as the implicit fencing model fundamentally allows: There is only one set of fences, you can only choose to sync against only writers (exclusive slot), or everyone. Hence suballocating multiple buffers or anything else like this is fundamentally not possible, and can only be fixed by a proper explicit fencing model. Aside from that caveat this model gets implicit fencing as closely to explicit fencing semantics as possible: On the actual implementation I opted for a simple setparam ioctl, no locking (just atomic reads/writes) for simplicity. There is a nice flag parameter in the VM ioctl which we could use, except: - it's not checked, so userspace likely passes garbage - there's already a comment that userspace _does_ pass garbage in the priority field So yeah unfortunately this flag parameter for setting vm flags is useless, and we need to hack up a new one. v2: Explain why a new SETPARAM (Jason) v3: Bas noticed I forgot to hook up the dependency-side shortcut. We need both, or this doesn't do much. v4: Rebase over the amdgpu patch to always set the implicit sync fences. Cc: mesa-dev(a)lists.freedesktop.org Cc: Bas Nieuwenhuizen <bas(a)basnieuwenhuizen.nl> Cc: Dave Airlie <airlied(a)gmail.com> Cc: Rob Clark <robdclark(a)chromium.org> Cc: Kristian H. Kristensen <hoegsberg(a)google.com> Cc: Michel Dänzer <michel(a)daenzer.net> Cc: Daniel Stone <daniels(a)collabora.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: Alex Deucher <alexander.deucher(a)amd.com> Cc: Daniel Vetter <daniel.vetter(a)ffwll.ch> Cc: Deepak R Varma <mh12gx2825(a)gmail.com> Cc: Chen Li <chenli(a)uniontech.com> Cc: Kevin Wang <kevin1.wang(a)amd.com> Cc: Dennis Li <Dennis.Li(a)amd.com> Cc: Luben Tuikov <luben.tuikov(a)amd.com> Cc: linaro-mm-sig(a)lists.linaro.org Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> --- drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 7 +++++-- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 21 +++++++++++++++++++++ drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 6 ++++++ include/uapi/drm/amdgpu_drm.h | 10 ++++++++++ 4 files changed, 42 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index 65df34c17264..c5386d13eb4a 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -498,6 +498,7 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p, struct amdgpu_bo *gds; struct amdgpu_bo *gws; struct amdgpu_bo *oa; + bool no_implicit_sync = READ_ONCE(fpriv->vm.no_implicit_sync); int r; INIT_LIST_HEAD(&p->validated); @@ -577,7 +578,8 @@ static int amdgpu_cs_parser_bos(struct amdgpu_cs_parser *p, e->bo_va = amdgpu_vm_bo_find(vm, bo); - if (bo->tbo.base.dma_buf && !amdgpu_bo_explicit_sync(bo)) { + if (bo->tbo.base.dma_buf && + !(no_implicit_sync || amdgpu_bo_explicit_sync(bo))) { e->chain = dma_fence_chain_alloc(); if (!e->chain) { r = -ENOMEM; @@ -649,6 +651,7 @@ static int amdgpu_cs_sync_rings(struct amdgpu_cs_parser *p) { struct amdgpu_fpriv *fpriv = p->filp->driver_priv; struct amdgpu_bo_list_entry *e; + bool no_implicit_sync = READ_ONCE(fpriv->vm.no_implicit_sync); int r; list_for_each_entry(e, &p->validated, tv.head) { @@ -656,7 +659,7 @@ static int amdgpu_cs_sync_rings(struct amdgpu_cs_parser *p) struct dma_resv *resv = bo->tbo.base.resv; enum amdgpu_sync_mode sync_mode; - sync_mode = amdgpu_bo_explicit_sync(bo) ? + sync_mode = no_implicit_sync || amdgpu_bo_explicit_sync(bo) ? AMDGPU_SYNC_EXPLICIT : AMDGPU_SYNC_NE_OWNER; r = amdgpu_sync_resv(p->adev, &p->job->sync, resv, sync_mode, &fpriv->vm); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index c080ba15ae77..f982626b5328 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -1724,6 +1724,26 @@ int amdgpu_file_to_fpriv(struct file *filp, struct amdgpu_fpriv **fpriv) return 0; } +int amdgpu_setparam_ioctl(struct drm_device *dev, void *data, + struct drm_file *filp) +{ + struct drm_amdgpu_setparam *setparam = data; + struct amdgpu_fpriv *fpriv = filp->driver_priv; + + switch (setparam->param) { + case AMDGPU_SETPARAM_NO_IMPLICIT_SYNC: + if (setparam->value) + WRITE_ONCE(fpriv->vm.no_implicit_sync, true); + else + WRITE_ONCE(fpriv->vm.no_implicit_sync, false); + break; + default: + return -EINVAL; + } + + return 0; +} + const struct drm_ioctl_desc amdgpu_ioctls_kms[] = { DRM_IOCTL_DEF_DRV(AMDGPU_GEM_CREATE, amdgpu_gem_create_ioctl, DRM_AUTH|DRM_RENDER_ALLOW), DRM_IOCTL_DEF_DRV(AMDGPU_CTX, amdgpu_ctx_ioctl, DRM_AUTH|DRM_RENDER_ALLOW), @@ -1742,6 +1762,7 @@ const struct drm_ioctl_desc amdgpu_ioctls_kms[] = { DRM_IOCTL_DEF_DRV(AMDGPU_GEM_VA, amdgpu_gem_va_ioctl, DRM_AUTH|DRM_RENDER_ALLOW), DRM_IOCTL_DEF_DRV(AMDGPU_GEM_OP, amdgpu_gem_op_ioctl, DRM_AUTH|DRM_RENDER_ALLOW), DRM_IOCTL_DEF_DRV(AMDGPU_GEM_USERPTR, amdgpu_gem_userptr_ioctl, DRM_AUTH|DRM_RENDER_ALLOW), + DRM_IOCTL_DEF_DRV(AMDGPU_SETPARAM, amdgpu_setparam_ioctl, DRM_AUTH|DRM_RENDER_ALLOW), }; static const struct drm_driver amdgpu_kms_driver = { diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h index ddb85a85cbba..0e8c440c6303 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h @@ -321,6 +321,12 @@ struct amdgpu_vm { bool bulk_moveable; /* Flag to indicate if VM is used for compute */ bool is_compute_context; + /* + * Flag to indicate whether implicit sync should always be skipped on + * this context. We do not care about races at all, userspace is allowed + * to shoot itself with implicit sync to its fullest liking. + */ + bool no_implicit_sync; }; struct amdgpu_vm_manager { diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h index 0cbd1540aeac..9eae245c14d6 100644 --- a/include/uapi/drm/amdgpu_drm.h +++ b/include/uapi/drm/amdgpu_drm.h @@ -54,6 +54,7 @@ extern "C" { #define DRM_AMDGPU_VM 0x13 #define DRM_AMDGPU_FENCE_TO_HANDLE 0x14 #define DRM_AMDGPU_SCHED 0x15 +#define DRM_AMDGPU_SETPARAM 0x16 #define DRM_IOCTL_AMDGPU_GEM_CREATE DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_GEM_CREATE, union drm_amdgpu_gem_create) #define DRM_IOCTL_AMDGPU_GEM_MMAP DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_GEM_MMAP, union drm_amdgpu_gem_mmap) @@ -71,6 +72,7 @@ extern "C" { #define DRM_IOCTL_AMDGPU_VM DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_VM, union drm_amdgpu_vm) #define DRM_IOCTL_AMDGPU_FENCE_TO_HANDLE DRM_IOWR(DRM_COMMAND_BASE + DRM_AMDGPU_FENCE_TO_HANDLE, union drm_amdgpu_fence_to_handle) #define DRM_IOCTL_AMDGPU_SCHED DRM_IOW(DRM_COMMAND_BASE + DRM_AMDGPU_SCHED, union drm_amdgpu_sched) +#define DRM_IOCTL_AMDGPU_SETPARAM DRM_IOW(DRM_COMMAND_BASE + DRM_AMDGPU_SETPARAM, struct drm_amdgpu_setparam) /** * DOC: memory domains @@ -306,6 +308,14 @@ union drm_amdgpu_sched { struct drm_amdgpu_sched_in in; }; +#define AMDGPU_SETPARAM_NO_IMPLICIT_SYNC 1 + +struct drm_amdgpu_setparam { + /* AMDGPU_SETPARAM_* */ + __u32 param; + __u32 value; +}; + /* * This is not a reliable API and you should expect it to fail for any * number of reasons and have fallback path that do not use userptr to -- 2.32.0.rc2

3 years, 11 months

4
13
0 0

[PATCH 01/15] dma-resv: Fix kerneldoc

by Daniel Vetter

Oversight from commit 6edbd6abb783d54f6ac4c3ed5cd9e50cff6c15e9 Author: Christian König <christian.koenig(a)amd.com> Date: Mon May 10 16:14:09 2021 +0200 dma-buf: rename and cleanup dma_resv_get_excl v3 Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org --- include/linux/dma-resv.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index 562b885cf9c3..e1ca2080a1ff 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -212,7 +212,7 @@ static inline void dma_resv_unlock(struct dma_resv *obj) } /** - * dma_resv_exclusive - return the object's exclusive fence + * dma_resv_excl_fence - return the object's exclusive fence * @obj: the reservation object * * Returns the exclusive fence (if any). Caller must either hold the objects -- 2.32.0.rc2

3 years, 11 months

5
5
0 0

Re: [Linaro-mm-sig] [PATCH v3 1/2] drm: add a locked version of drm_is_current_master

by Daniel Vetter

On Sun, Jun 20, 2021 at 07:03:26PM +0800, Desmond Cheong Zhi Xi wrote: > While checking the master status of the DRM file in > drm_is_current_master(), the device's master mutex should be > held. Without the mutex, the pointer fpriv->master may be freed > concurrently by another process calling drm_setmaster_ioctl(). This > could lead to use-after-free errors when the pointer is subsequently > dereferenced in drm_lease_owner(). > > The callers of drm_is_current_master() from drm_auth.c hold the > device's master mutex, but external callers do not. Hence, we implement > drm_is_current_master_locked() to be used within drm_auth.c, and > modify drm_is_current_master() to grab the device's master mutex > before checking the master status. > > Reported-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx(a)gmail.com> > Reviewed-by: Emil Velikov <emil.l.velikov(a)gmail.com> Merged to drm-misc-fixes, thanks for your patch. -Daniel > --- > drivers/gpu/drm/drm_auth.c | 51 ++++++++++++++++++++++++-------------- > 1 file changed, 32 insertions(+), 19 deletions(-) > > diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c > index 232abbba3686..86d4b72e95cb 100644 > --- a/drivers/gpu/drm/drm_auth.c > +++ b/drivers/gpu/drm/drm_auth.c > @@ -61,6 +61,35 @@ > * trusted clients. > */ > > +static bool drm_is_current_master_locked(struct drm_file *fpriv) > +{ > + lockdep_assert_held_once(&fpriv->master->dev->master_mutex); > + > + return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master; > +} > + > +/** > + * drm_is_current_master - checks whether @priv is the current master > + * @fpriv: DRM file private > + * > + * Checks whether @fpriv is current master on its device. This decides whether a > + * client is allowed to run DRM_MASTER IOCTLs. > + * > + * Most of the modern IOCTL which require DRM_MASTER are for kernel modesetting > + * - the current master is assumed to own the non-shareable display hardware. > + */ > +bool drm_is_current_master(struct drm_file *fpriv) > +{ > + bool ret; > + > + mutex_lock(&fpriv->master->dev->master_mutex); > + ret = drm_is_current_master_locked(fpriv); > + mutex_unlock(&fpriv->master->dev->master_mutex); > + > + return ret; > +} > +EXPORT_SYMBOL(drm_is_current_master); > + > int drm_getmagic(struct drm_device *dev, void *data, struct drm_file *file_priv) > { > struct drm_auth *auth = data; > @@ -223,7 +252,7 @@ int drm_setmaster_ioctl(struct drm_device *dev, void *data, > if (ret) > goto out_unlock; > > - if (drm_is_current_master(file_priv)) > + if (drm_is_current_master_locked(file_priv)) > goto out_unlock; > > if (dev->master) { > @@ -272,7 +301,7 @@ int drm_dropmaster_ioctl(struct drm_device *dev, void *data, > if (ret) > goto out_unlock; > > - if (!drm_is_current_master(file_priv)) { > + if (!drm_is_current_master_locked(file_priv)) { > ret = -EINVAL; > goto out_unlock; > } > @@ -321,7 +350,7 @@ void drm_master_release(struct drm_file *file_priv) > if (file_priv->magic) > idr_remove(&file_priv->master->magic_map, file_priv->magic); > > - if (!drm_is_current_master(file_priv)) > + if (!drm_is_current_master_locked(file_priv)) > goto out; > > drm_legacy_lock_master_cleanup(dev, master); > @@ -342,22 +371,6 @@ void drm_master_release(struct drm_file *file_priv) > mutex_unlock(&dev->master_mutex); > } > > -/** > - * drm_is_current_master - checks whether @priv is the current master > - * @fpriv: DRM file private > - * > - * Checks whether @fpriv is current master on its device. This decides whether a > - * client is allowed to run DRM_MASTER IOCTLs. > - * > - * Most of the modern IOCTL which require DRM_MASTER are for kernel modesetting > - * - the current master is assumed to own the non-shareable display hardware. > - */ > -bool drm_is_current_master(struct drm_file *fpriv) > -{ > - return fpriv->is_master && drm_lease_owner(fpriv->master) == fpriv->minor->dev->master; > -} > -EXPORT_SYMBOL(drm_is_current_master); > - > /** > * drm_master_get - reference a master pointer > * @master: &struct drm_master > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 11 months

2
1
0 0

Re: [Linaro-mm-sig] [PATCH v3 2/2] drm: protect drm_master pointers in drm_lease.c

by Daniel Vetter

On Wed, Jun 23, 2021 at 5:49 AM Desmond Cheong Zhi Xi <desmondcheongzx(a)gmail.com> wrote: > > On 21/6/21 10:47 pm, Daniel Vetter wrote: > > On Sun, Jun 20, 2021 at 07:03:27PM +0800, Desmond Cheong Zhi Xi wrote: > >> diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c > >> index 86d4b72e95cb..0c64a77c67a6 100644 > >> --- a/drivers/gpu/drm/drm_auth.c > >> +++ b/drivers/gpu/drm/drm_auth.c > >> @@ -384,6 +384,28 @@ struct drm_master *drm_master_get(struct drm_master *master) > >> } > >> EXPORT_SYMBOL(drm_master_get); > >> > >> +/** > >> + * drm_file_get_master - reference @file_priv->master > >> + * @file_priv: DRM file private > >> + * > >> + * Increments the reference count of @file_priv->master and returns > > > > Does this format correctly? I'd go with "&drm_file.master of @file_priv". > > > > Got it. "file_priv->master" was bolded, but no link to drm_file.master > was generated. I'll update this. > > >> + * @file_priv->master. > >> + * > >> + * Master pointers returned from this function should be unreferenced using > >> + * drm_master_put(). > >> + */ > >> +struct drm_master *drm_file_get_master(struct drm_file *file_priv) > >> +{ > >> + struct drm_master *master; > >> + > >> + mutex_lock(&file_priv->master->dev->master_mutex); > >> + master = drm_master_get(file_priv->master); > >> + mutex_unlock(&file_priv->master->dev->master_mutex); > >> + > >> + return master; > >> +} > >> +EXPORT_SYMBOL(drm_file_get_master); > >> + > >> static void drm_master_destroy(struct kref *kref) > >> { > >> struct drm_master *master = container_of(kref, struct drm_master, refcount); > >> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c > >> index da4f085fc09e..65eab82f8acc 100644 > >> --- a/drivers/gpu/drm/drm_lease.c > >> +++ b/drivers/gpu/drm/drm_lease.c > >> @@ -107,10 +107,17 @@ static bool _drm_has_leased(struct drm_master *master, int id) > >> */ > >> bool _drm_lease_held(struct drm_file *file_priv, int id) > >> { > >> + bool ret; > >> + struct drm_master *master; > >> + > >> if (!file_priv || !file_priv->master) > > > > So here we still have a ->master access outside of the locked code > > section. I think the best fix for that would be to move the NULL check > > into drm_file_get_master (where we grab the lock already anyway), and > > update the kerneldoc to state that it might return NULL. > > > > Same with all the checks for ->master below. > > > > Moving the check into drm_file_get_master sounds good. Grabbing the lock > before performing the NULL check poses a little chicken-and-egg problem > though. > > It's true that without the lock, even if file_priv->master passes the > NULL check, it could be freed in the time between the check and grabbing > the lock. > > However, based on the original code, it seems there's the possibility > that file_priv->master might be NULL. In this case, grabbing the lock > results in a null ptr dereference because we get the mutex via > &file_priv->master->dev->master_mutex. > > By this reasoning, I think the safer method is still to perform the NULL > check before grabbing the lock. file_priv->dev->master_mutex should also work and avoid the trouble. Please also cc intel-gfx list, there's a CI system there to test your patches. Since patch 1 of this series had pretty bad deadlock that I didn't see would be good to make sure we get more test coverage on these. Thanks, Daniel > > >> return true; > >> > >> - return _drm_lease_held_master(file_priv->master, id); > >> + master = drm_file_get_master(file_priv); > >> + ret = _drm_lease_held_master(master, id); > >> + drm_master_put(&master); > >> + > >> + return ret; > >> } > >> > >> /** > >> @@ -132,10 +139,11 @@ bool drm_lease_held(struct drm_file *file_priv, int id) > >> if (!file_priv || !file_priv->master || !file_priv->master->lessor) > >> return true; > > > > master->lessor dereferenced outside the lock or without holding a > > reference. > > > >> > >> - master = file_priv->master; > >> + master = drm_file_get_master(file_priv); > >> mutex_lock(&master->dev->mode_config.idr_mutex); > >> ret = _drm_lease_held_master(master, id); > >> mutex_unlock(&master->dev->mode_config.idr_mutex); > >> + drm_master_put(&master); > >> return ret; > >> } > >> > >> @@ -158,7 +166,7 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in) > >> if (!file_priv || !file_priv->master || !file_priv->master->lessor) > >> return crtcs_in; > > > > Same here. > > > >> > >> - master = file_priv->master; > >> + master = drm_file_get_master(file_priv); > >> dev = master->dev; > >> > >> count_in = count_out = 0; > >> @@ -177,6 +185,7 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in) > >> count_in++; > >> } > >> mutex_unlock(&master->dev->mode_config.idr_mutex); > >> + drm_master_put(&master); > >> return crtcs_out; > >> } > >> > >> @@ -490,7 +499,7 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > >> size_t object_count; > >> int ret = 0; > >> struct idr leases; > >> - struct drm_master *lessor = lessor_priv->master; > >> + struct drm_master *lessor; > >> struct drm_master *lessee = NULL; > >> struct file *lessee_file = NULL; > >> struct file *lessor_file = lessor_priv->filp; > >> @@ -502,12 +511,6 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > >> if (!drm_core_check_feature(dev, DRIVER_MODESET)) > >> return -EOPNOTSUPP; > >> > >> - /* Do not allow sub-leases */ > >> - if (lessor->lessor) { > >> - DRM_DEBUG_LEASE("recursive leasing not allowed\n"); > >> - return -EINVAL; > >> - } > >> - > >> /* need some objects */ > >> if (cl->object_count == 0) { > >> DRM_DEBUG_LEASE("no objects in lease\n"); > >> @@ -519,12 +522,22 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > >> return -EINVAL; > >> } > >> > >> + lessor = drm_file_get_master(lessor_priv); > >> + /* Do not allow sub-leases */ > >> + if (lessor->lessor) { > > > > Here we check after grabbing the reference, so looks correct. > > > >> + DRM_DEBUG_LEASE("recursive leasing not allowed\n"); > >> + ret = -EINVAL; > >> + goto out_lessor; > >> + } > >> + > >> object_count = cl->object_count; > >> > >> object_ids = memdup_user(u64_to_user_ptr(cl->object_ids), > >> array_size(object_count, sizeof(__u32))); > >> - if (IS_ERR(object_ids)) > >> - return PTR_ERR(object_ids); > >> + if (IS_ERR(object_ids)) { > >> + ret = PTR_ERR(object_ids); > >> + goto out_lessor; > >> + } > >> > >> idr_init(&leases); > >> > >> @@ -535,14 +548,15 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > >> if (ret) { > >> DRM_DEBUG_LEASE("lease object lookup failed: %i\n", ret); > >> idr_destroy(&leases); > >> - return ret; > >> + goto out_lessor; > >> } > >> > >> /* Allocate a file descriptor for the lease */ > >> fd = get_unused_fd_flags(cl->flags & (O_CLOEXEC | O_NONBLOCK)); > >> if (fd < 0) { > >> idr_destroy(&leases); > >> - return fd; > >> + ret = fd; > >> + goto out_lessor; > >> } > >> > >> DRM_DEBUG_LEASE("Creating lease\n"); > >> @@ -578,6 +592,7 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > >> /* Hook up the fd */ > >> fd_install(fd, lessee_file); > >> > >> + drm_master_put(&lessor); > >> DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl succeeded\n"); > >> return 0; > >> > >> @@ -587,6 +602,8 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > >> out_leases: > >> put_unused_fd(fd); > >> > >> +out_lessor: > >> + drm_master_put(&lessor); > >> DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl failed: %d\n", ret); > >> return ret; > >> } > >> @@ -609,7 +626,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev, > >> struct drm_mode_list_lessees *arg = data; > >> __u32 __user *lessee_ids = (__u32 __user *) (uintptr_t) (arg->lessees_ptr); > >> __u32 count_lessees = arg->count_lessees; > >> - struct drm_master *lessor = lessor_priv->master, *lessee; > >> + struct drm_master *lessor, *lessee; > >> int count; > >> int ret = 0; > >> > >> @@ -620,6 +637,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev, > >> if (!drm_core_check_feature(dev, DRIVER_MODESET)) > >> return -EOPNOTSUPP; > >> > >> + lessor = drm_file_get_master(lessor_priv); > >> DRM_DEBUG_LEASE("List lessees for %d\n", lessor->lessee_id); > >> > >> mutex_lock(&dev->mode_config.idr_mutex); > >> @@ -643,6 +661,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev, > >> arg->count_lessees = count; > >> > >> mutex_unlock(&dev->mode_config.idr_mutex); > >> + drm_master_put(&lessor); > >> > >> return ret; > >> } > >> @@ -662,7 +681,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev, > >> struct drm_mode_get_lease *arg = data; > >> __u32 __user *object_ids = (__u32 __user *) (uintptr_t) (arg->objects_ptr); > >> __u32 count_objects = arg->count_objects; > >> - struct drm_master *lessee = lessee_priv->master; > >> + struct drm_master *lessee; > >> struct idr *object_idr; > >> int count; > >> void *entry; > >> @@ -676,6 +695,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev, > >> if (!drm_core_check_feature(dev, DRIVER_MODESET)) > >> return -EOPNOTSUPP; > >> > >> + lessee = drm_file_get_master(lessee_priv); > >> DRM_DEBUG_LEASE("get lease for %d\n", lessee->lessee_id); > >> > >> mutex_lock(&dev->mode_config.idr_mutex); > >> @@ -703,6 +723,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev, > >> arg->count_objects = count; > >> > >> mutex_unlock(&dev->mode_config.idr_mutex); > >> + drm_master_put(&lessee); > >> > >> return ret; > >> } > >> @@ -721,7 +742,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev, > >> void *data, struct drm_file *lessor_priv) > >> { > >> struct drm_mode_revoke_lease *arg = data; > >> - struct drm_master *lessor = lessor_priv->master; > >> + struct drm_master *lessor; > >> struct drm_master *lessee; > >> int ret = 0; > >> > >> @@ -731,6 +752,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev, > >> if (!drm_core_check_feature(dev, DRIVER_MODESET)) > >> return -EOPNOTSUPP; > >> > >> + lessor = drm_file_get_master(lessor_priv); > >> mutex_lock(&dev->mode_config.idr_mutex); > >> > >> lessee = _drm_find_lessee(lessor, arg->lessee_id); > >> @@ -751,6 +773,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev, > >> > >> fail: > >> mutex_unlock(&dev->mode_config.idr_mutex); > >> + drm_master_put(&lessor); > >> > >> return ret; > >> } > >> diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h > >> index 6bf8b2b78991..f99d3417f304 100644 > >> --- a/include/drm/drm_auth.h > >> +++ b/include/drm/drm_auth.h > >> @@ -107,6 +107,7 @@ struct drm_master { > >> }; > >> > >> struct drm_master *drm_master_get(struct drm_master *master); > >> +struct drm_master *drm_file_get_master(struct drm_file *file_priv); > >> void drm_master_put(struct drm_master **master); > >> bool drm_is_current_master(struct drm_file *fpriv); > >> > >> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h > >> index b81b3bfb08c8..e9931fca4ab7 100644 > >> --- a/include/drm/drm_file.h > >> +++ b/include/drm/drm_file.h > >> @@ -226,9 +226,18 @@ struct drm_file { > >> /** > >> * @master: > >> * > >> - * Master this node is currently associated with. Only relevant if > >> - * drm_is_primary_client() returns true. Note that this only > >> - * matches &drm_device.master if the master is the currently active one. > >> + * Master this node is currently associated with. Protected by struct > >> + * &drm_device.master_mutex. > >> + * > >> + * Only relevant if drm_is_primary_client() returns true. Note that > >> + * this only matches &drm_device.master if the master is the currently > >> + * active one. > >> + * > >> + * When obtaining a copy of this pointer, it is recommended to either > >> + * hold struct &drm_device.master_mutex for the duration of the > >> + * pointer's use, or to use drm_file_get_master() if struct > >> + * &drm_device.master_mutex is not currently held and there is no other > >> + * need to hold it. This prevents @master from being freed during use. > >> * > >> * See also @authentication and @is_master and the :ref:`section on > >> * primary nodes and authentication <drm_primary_node>`. > >> -- > >> 2.25.1 > >> > > > > Thanks for the feedback, Daniel. I'll send out an updated patch to > address these issues. > > Best wishes, > Desmond -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 11 months

1
0
0 0

[PATCH 06/15] drm/panfrost: Fix implicit sync

by Daniel Vetter

Currently this has no practial relevance I think because there's not many who can pull off a setup with panfrost and another gpu in the same system. But the rules are that if you're setting an exclusive fence, indicating a gpu write access in the implicit fencing system, then you need to wait for all fences, not just the previous exclusive fence. panfrost against itself has no problem, because it always sets the exclusive fence (but that's probably something that will need to be fixed for vulkan and/or multi-engine gpus, or you'll suffer badly). Also no problem with that against display. With the prep work done to switch over to the dependency helpers this is now a oneliner. Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: Rob Herring <robh(a)kernel.org> Cc: Tomeu Vizoso <tomeu.vizoso(a)collabora.com> Cc: Steven Price <steven.price(a)arm.com> Cc: Alyssa Rosenzweig <alyssa.rosenzweig(a)collabora.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org --- drivers/gpu/drm/panfrost/panfrost_job.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c index 71cd43fa1b36..ef004d587dc4 100644 --- a/drivers/gpu/drm/panfrost/panfrost_job.c +++ b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -203,9 +203,8 @@ static int panfrost_acquire_object_fences(struct drm_gem_object **bos, int i, ret; for (i = 0; i < bo_count; i++) { - struct dma_fence *fence = dma_resv_get_excl_unlocked(bos[i]->resv); - - ret = drm_gem_fence_array_add(deps, fence); + /* panfrost always uses write mode in its current uapi */ + ret = drm_gem_fence_array_add_implicit(deps, bos[i], true); if (ret) return ret; } -- 2.32.0.rc2

3 years, 11 months

1
0
0 0

[PATCH 05/15] drm/panfrost: Use xarray and helpers for depedency tracking

by Daniel Vetter

More consistency and prep work for the next patch. Aside: I wonder whether we shouldn't just move this entire xarray business into the scheduler so that not everyone has to reinvent the same wheels. Cc'ing some scheduler people for this too. v2: Correctly handle sched_lock since Lucas pointed out it's needed. v3: Rebase, dma_resv_get_excl_unlocked got renamed v4: Don't leak job references on failure (Steven). Cc: Lucas Stach <l.stach(a)pengutronix.de> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: Luben Tuikov <luben.tuikov(a)amd.com> Cc: Alex Deucher <alexander.deucher(a)amd.com> Cc: Lee Jones <lee.jones(a)linaro.org> Cc: Steven Price <steven.price(a)arm.com> Cc: Rob Herring <robh(a)kernel.org> Cc: Tomeu Vizoso <tomeu.vizoso(a)collabora.com> Cc: Alyssa Rosenzweig <alyssa.rosenzweig(a)collabora.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> --- drivers/gpu/drm/panfrost/panfrost_drv.c | 41 +++++++--------- drivers/gpu/drm/panfrost/panfrost_job.c | 65 +++++++++++-------------- drivers/gpu/drm/panfrost/panfrost_job.h | 8 ++- 3 files changed, 49 insertions(+), 65 deletions(-) diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index 075ec0ef746c..3ee828f1e7a5 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -138,12 +138,6 @@ panfrost_lookup_bos(struct drm_device *dev, if (!job->bo_count) return 0; - job->implicit_fences = kvmalloc_array(job->bo_count, - sizeof(struct dma_fence *), - GFP_KERNEL | __GFP_ZERO); - if (!job->implicit_fences) - return -ENOMEM; - ret = drm_gem_objects_lookup(file_priv, (void __user *)(uintptr_t)args->bo_handles, job->bo_count, &job->bos); @@ -174,7 +168,7 @@ panfrost_lookup_bos(struct drm_device *dev, } /** - * panfrost_copy_in_sync() - Sets up job->in_fences[] with the sync objects + * panfrost_copy_in_sync() - Sets up job->deps with the sync objects * referenced by the job. * @dev: DRM device * @file_priv: DRM file for this fd @@ -194,22 +188,14 @@ panfrost_copy_in_sync(struct drm_device *dev, { u32 *handles; int ret = 0; - int i; + int i, in_fence_count; - job->in_fence_count = args->in_sync_count; + in_fence_count = args->in_sync_count; - if (!job->in_fence_count) + if (!in_fence_count) return 0; - job->in_fences = kvmalloc_array(job->in_fence_count, - sizeof(struct dma_fence *), - GFP_KERNEL | __GFP_ZERO); - if (!job->in_fences) { - DRM_DEBUG("Failed to allocate job in fences\n"); - return -ENOMEM; - } - - handles = kvmalloc_array(job->in_fence_count, sizeof(u32), GFP_KERNEL); + handles = kvmalloc_array(in_fence_count, sizeof(u32), GFP_KERNEL); if (!handles) { ret = -ENOMEM; DRM_DEBUG("Failed to allocate incoming syncobj handles\n"); @@ -218,16 +204,23 @@ panfrost_copy_in_sync(struct drm_device *dev, if (copy_from_user(handles, (void __user *)(uintptr_t)args->in_syncs, - job->in_fence_count * sizeof(u32))) { + in_fence_count * sizeof(u32))) { ret = -EFAULT; DRM_DEBUG("Failed to copy in syncobj handles\n"); goto fail; } - for (i = 0; i < job->in_fence_count; i++) { + for (i = 0; i < in_fence_count; i++) { + struct dma_fence *fence; + ret = drm_syncobj_find_fence(file_priv, handles[i], 0, 0, - &job->in_fences[i]); - if (ret == -EINVAL) + &fence); + if (ret) + goto fail; + + ret = drm_gem_fence_array_add(&job->deps, fence); + + if (ret) goto fail; } @@ -265,6 +258,8 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data, kref_init(&job->refcount); + xa_init_flags(&job->deps, XA_FLAGS_ALLOC); + job->pfdev = pfdev; job->jc = args->jc; job->requirements = args->requirements; diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c index 38f8580c19f1..71cd43fa1b36 100644 --- a/drivers/gpu/drm/panfrost/panfrost_job.c +++ b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -196,14 +196,21 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js) job_write(pfdev, JS_COMMAND_NEXT(js), JS_COMMAND_START); } -static void panfrost_acquire_object_fences(struct drm_gem_object **bos, - int bo_count, - struct dma_fence **implicit_fences) +static int panfrost_acquire_object_fences(struct drm_gem_object **bos, + int bo_count, + struct xarray *deps) { - int i; + int i, ret; - for (i = 0; i < bo_count; i++) - implicit_fences[i] = dma_resv_get_excl_unlocked(bos[i]->resv); + for (i = 0; i < bo_count; i++) { + struct dma_fence *fence = dma_resv_get_excl_unlocked(bos[i]->resv); + + ret = drm_gem_fence_array_add(deps, fence); + if (ret) + return ret; + } + + return 0; } static void panfrost_attach_object_fences(struct drm_gem_object **bos, @@ -240,10 +247,14 @@ int panfrost_job_push(struct panfrost_job *job) job->render_done_fence = dma_fence_get(&job->base.s_fence->finished); - kref_get(&job->refcount); /* put by scheduler job completion */ + ret = panfrost_acquire_object_fences(job->bos, job->bo_count, + &job->deps); + if (ret) { + mutex_unlock(&pfdev->sched_lock); + goto unlock; + } - panfrost_acquire_object_fences(job->bos, job->bo_count, - job->implicit_fences); + kref_get(&job->refcount); /* put by scheduler job completion */ drm_sched_entity_push_job(&job->base, entity); @@ -262,18 +273,15 @@ static void panfrost_job_cleanup(struct kref *ref) { struct panfrost_job *job = container_of(ref, struct panfrost_job, refcount); + struct dma_fence *fence; + unsigned long index; unsigned int i; - if (job->in_fences) { - for (i = 0; i < job->in_fence_count; i++) - dma_fence_put(job->in_fences[i]); - kvfree(job->in_fences); - } - if (job->implicit_fences) { - for (i = 0; i < job->bo_count; i++) - dma_fence_put(job->implicit_fences[i]); - kvfree(job->implicit_fences); + xa_for_each(&job->deps, index, fence) { + dma_fence_put(fence); } + xa_destroy(&job->deps); + dma_fence_put(job->done_fence); dma_fence_put(job->render_done_fence); @@ -316,26 +324,9 @@ static struct dma_fence *panfrost_job_dependency(struct drm_sched_job *sched_job struct drm_sched_entity *s_entity) { struct panfrost_job *job = to_panfrost_job(sched_job); - struct dma_fence *fence; - unsigned int i; - - /* Explicit fences */ - for (i = 0; i < job->in_fence_count; i++) { - if (job->in_fences[i]) { - fence = job->in_fences[i]; - job->in_fences[i] = NULL; - return fence; - } - } - /* Implicit fences, max. one per BO */ - for (i = 0; i < job->bo_count; i++) { - if (job->implicit_fences[i]) { - fence = job->implicit_fences[i]; - job->implicit_fences[i] = NULL; - return fence; - } - } + if (!xa_empty(&job->deps)) + return xa_erase(&job->deps, job->last_dep++); return NULL; } diff --git a/drivers/gpu/drm/panfrost/panfrost_job.h b/drivers/gpu/drm/panfrost/panfrost_job.h index bbd3ba97ff67..82306a03b57e 100644 --- a/drivers/gpu/drm/panfrost/panfrost_job.h +++ b/drivers/gpu/drm/panfrost/panfrost_job.h @@ -19,9 +19,9 @@ struct panfrost_job { struct panfrost_device *pfdev; struct panfrost_file_priv *file_priv; - /* Optional fences userspace can pass in for the job to depend on. */ - struct dma_fence **in_fences; - u32 in_fence_count; + /* Contains both explicit and implicit fences */ + struct xarray deps; + unsigned long last_dep; /* Fence to be signaled by IRQ handler when the job is complete. */ struct dma_fence *done_fence; @@ -30,8 +30,6 @@ struct panfrost_job { __u32 requirements; __u32 flush_id; - /* Exclusive fences we have taken from the BOs to wait for */ - struct dma_fence **implicit_fences; struct panfrost_gem_mapping **mappings; struct drm_gem_object **bos; u32 bo_count; -- 2.32.0.rc2

3 years, 11 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v3 1/2] habanalabs: define uAPI to export FD for DMA-BUF

by Christian König

Am 22.06.21 um 17:40 schrieb Oded Gabbay: > On Tue, Jun 22, 2021 at 6:31 PM Christian König > <christian.koenig(a)amd.com> wrote: >> >> >> Am 22.06.21 um 17:28 schrieb Jason Gunthorpe: >>> On Tue, Jun 22, 2021 at 05:24:08PM +0200, Christian König wrote: >>> >>>>>> I will take two GAUDI devices and use one as an exporter and one as an >>>>>> importer. I want to see that the solution works end-to-end, with real >>>>>> device DMA from importer to exporter. >>>>> I can tell you it doesn't. Stuffing physical addresses directly into >>>>> the sg list doesn't involve any of the IOMMU code so any configuration >>>>> that requires IOMMU page table setup will not work. >>>> Sure it does. See amdgpu_vram_mgr_alloc_sgt: >>>> >>>> amdgpu_res_first(res, offset, length, &cursor); >>> ^^^^^^^^^^ >>> >>> I'm not talking about the AMD driver, I'm talking about this patch. >>> >>> + bar_address = hdev->dram_pci_bar_start + >>> + (pages[cur_page] - prop->dram_base_address); >>> + sg_dma_address(sg) = bar_address; >> Yeah, that is indeed not working. >> >> Oded you need to use dma_map_resource() for this. >> >> Christian. > Yes, of course. > But will it be enough ? > Jason said that supporting IOMMU isn't nice when we don't have struct pages. > I fail to understand the connection, I need to dig into this. Question is what you want to do with this? A struct page is always needed if you want to do stuff like HMM with it, if you only want P2P between device I actually recommend to avoid it. Christian. > > Oded > >> >> >>> Jason

3 years, 11 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v3 1/2] habanalabs: define uAPI to export FD for DMA-BUF

by Jason Gunthorpe

On Tue, Jun 22, 2021 at 06:24:28PM +0300, Oded Gabbay wrote: > On Tue, Jun 22, 2021 at 6:11 PM Jason Gunthorpe <jgg(a)ziepe.ca> wrote: > > > > On Tue, Jun 22, 2021 at 04:12:26PM +0300, Oded Gabbay wrote: > > > > > > 1) Setting sg_page to NULL > > > > 2) 'mapping' pages for P2P DMA without going through the iommu > > > > 3) Allowing P2P DMA without using the p2p dma API to validate that it > > > > can work at all in the first place. > > > > > > > > All of these result in functional bugs in certain system > > > > configurations. > > > > > > > > Jason > > > > > > Hi Jason, > > > Thanks for the feedback. > > > Regarding point 1, why is that a problem if we disable the option to > > > mmap the dma-buf from user-space ? > > > > Userspace has nothing to do with needing struct pages or not > > > > Point 1 and 2 mostly go together, you supporting the iommu is not nice > > if you dont have struct pages. > > > > You should study Logan's patches I pointed you at as they are solving > > exactly this problem. > Yes, I do need to study them. I agree with you here. It appears I > have a hole in my understanding. I'm missing the connection between > iommu support (which I must have of course) and struct pages. Chistian explained what the AMD driver is doing by calling dma_map_resource(). Which is a hacky and slow way of achieving what Logan's series is doing. > > No, the design of the dmabuf requires the exporter to do the dma maps > > and so it is only the exporter that is wrong to omit all the iommu and > > p2p logic. > > > > RDMA is OK today only because nobody has implemented dma buf support > > in rxe/si - mainly because the only implementations of exporters don't > > Can you please educate me, what is rxe/si ? Sorry, rxe/siw - these are the all-software implementations of RDMA and they require the struct page to do a SW memory copy. They can't implement dmabuf without it. > ok... > so how come that patch-set was merged into 5.12 if it's buggy ? We only implemented true dma devices for RDMA DMABUF support, so it is isn't buggy right now. > Yes, that's what I expect to see. But I want to see it with my own > eyes and then figure out how to solve this. It might be tricky to test because you have to ensure the iommu is turned on and has a non-idenity page table. Basically if it doesn't trigger a IOMMU failure then the IOMMU isn't setup properly. Jason

3 years, 11 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v3 1/2] habanalabs: define uAPI to export FD for DMA-BUF

by Jason Gunthorpe

On Tue, Jun 22, 2021 at 04:12:26PM +0300, Oded Gabbay wrote: > > 1) Setting sg_page to NULL > > 2) 'mapping' pages for P2P DMA without going through the iommu > > 3) Allowing P2P DMA without using the p2p dma API to validate that it > > can work at all in the first place. > > > > All of these result in functional bugs in certain system > > configurations. > > > > Jason > > Hi Jason, > Thanks for the feedback. > Regarding point 1, why is that a problem if we disable the option to > mmap the dma-buf from user-space ? Userspace has nothing to do with needing struct pages or not Point 1 and 2 mostly go together, you supporting the iommu is not nice if you dont have struct pages. You should study Logan's patches I pointed you at as they are solving exactly this problem. > In addition, I didn't see any problem with sg_page being NULL in the > RDMA p2p dma-buf code. Did I miss something here ? No, the design of the dmabuf requires the exporter to do the dma maps and so it is only the exporter that is wrong to omit all the iommu and p2p logic. RDMA is OK today only because nobody has implemented dma buf support in rxe/si - mainly because the only implementations of exporters don't set the struct page and are thus buggy. > I will take two GAUDI devices and use one as an exporter and one as an > importer. I want to see that the solution works end-to-end, with real > device DMA from importer to exporter. I can tell you it doesn't. Stuffing physical addresses directly into the sg list doesn't involve any of the IOMMU code so any configuration that requires IOMMU page table setup will not work. Jason

3 years, 11 months

2
3
0 0

Re: [Linaro-mm-sig] [PATCH v3 1/2] habanalabs: define uAPI to export FD for DMA-BUF

by Jason Gunthorpe

On Tue, Jun 22, 2021 at 03:04:30PM +0300, Oded Gabbay wrote: > On Tue, Jun 22, 2021 at 3:01 PM Jason Gunthorpe <jgg(a)ziepe.ca> wrote: > > > > On Tue, Jun 22, 2021 at 11:42:27AM +0300, Oded Gabbay wrote: > > > On Tue, Jun 22, 2021 at 9:37 AM Christian König > > > <ckoenig.leichtzumerken(a)gmail.com> wrote: > > > > > > > > Am 22.06.21 um 01:29 schrieb Jason Gunthorpe: > > > > > On Mon, Jun 21, 2021 at 10:24:16PM +0300, Oded Gabbay wrote: > > > > > > > > > >> Another thing I want to emphasize is that we are doing p2p only > > > > >> through the export/import of the FD. We do *not* allow the user to > > > > >> mmap the dma-buf as we do not support direct IO. So there is no access > > > > >> to these pages through the userspace. > > > > > Arguably mmaping the memory is a better choice, and is the direction > > > > > that Logan's series goes in. Here the use of DMABUF was specifically > > > > > designed to allow hitless revokation of the memory, which this isn't > > > > > even using. > > > > > > > > The major problem with this approach is that DMA-buf is also used for > > > > memory which isn't CPU accessible. > > > > That isn't an issue here because the memory is only intended to be > > used with P2P transfers so it must be CPU accessible. > > > > > > That was one of the reasons we didn't even considered using the mapping > > > > memory approach for GPUs. > > > > Well, now we have DEVICE_PRIVATE memory that can meet this need > > too.. Just nobody has wired it up to hmm_range_fault() > > > > > > > So you are taking the hit of very limited hardware support and reduced > > > > > performance just to squeeze into DMABUF.. > > > > > > Thanks Jason for the clarification, but I honestly prefer to use > > > DMA-BUF at the moment. > > > It gives us just what we need (even more than what we need as you > > > pointed out), it is *already* integrated and tested in the RDMA > > > subsystem, and I'm feeling comfortable using it as I'm somewhat > > > familiar with it from my AMD days. > > > > You still have the issue that this patch is doing all of this P2P > > stuff wrong - following the already NAK'd AMD approach. > > Could you please point me exactly to the lines of code that are wrong > in your opinion ? 1) Setting sg_page to NULL 2) 'mapping' pages for P2P DMA without going through the iommu 3) Allowing P2P DMA without using the p2p dma API to validate that it can work at all in the first place. All of these result in functional bugs in certain system configurations. Jason

3 years, 11 months

1
0
0 0

Re: [Linaro-mm-sig] [PATCH v3 1/2] habanalabs: define uAPI to export FD for DMA-BUF

by Jason Gunthorpe

On Mon, Jun 21, 2021 at 10:24:16PM +0300, Oded Gabbay wrote: > Another thing I want to emphasize is that we are doing p2p only > through the export/import of the FD. We do *not* allow the user to > mmap the dma-buf as we do not support direct IO. So there is no access > to these pages through the userspace. Arguably mmaping the memory is a better choice, and is the direction that Logan's series goes in. Here the use of DMABUF was specifically designed to allow hitless revokation of the memory, which this isn't even using. So you are taking the hit of very limited hardware support and reduced performance just to squeeze into DMABUF.. Jason

3 years, 11 months

2
1
0 0

Re: [Linaro-mm-sig] [PATCH v3 1/2] habanalabs: define uAPI to export FD for DMA-BUF

by Jason Gunthorpe

On Mon, Jun 21, 2021 at 07:26:14PM +0300, Oded Gabbay wrote: > On Mon, Jun 21, 2021 at 5:12 PM Jason Gunthorpe <jgg(a)ziepe.ca> wrote: > > > > On Mon, Jun 21, 2021 at 03:02:10PM +0200, Greg KH wrote: > > > On Mon, Jun 21, 2021 at 02:28:48PM +0200, Daniel Vetter wrote: > > > > > > Also I'm wondering which is the other driver that we share buffers > > > > with. The gaudi stuff doesn't have real struct pages as backing > > > > storage, it only fills out the dma_addr_t. That tends to blow up with > > > > other drivers, and the only place where this is guaranteed to work is > > > > if you have a dynamic importer which sets the allow_peer2peer flag. > > > > Adding maintainers from other subsystems who might want to chime in > > > > here. So even aside of the big question as-is this is broken. > > > > > > From what I can tell this driver is sending the buffers to other > > > instances of the same hardware, > > > > A dmabuf is consumed by something else in the kernel calling > > dma_buf_map_attachment() on the FD. > > > > What is the other side of this? I don't see any > > dma_buf_map_attachment() calls in drivers/misc, or added in this patch > > set. > > This patch-set is only to enable the support for the exporter side. > The "other side" is any generic RDMA networking device that will want > to perform p2p communication over PCIe with our GAUDI accelerator. > An example is indeed the mlnx5 card which has already integrated > support for being an "importer". It raises the question of how you are testing this if you aren't using it with the only intree driver: mlx5. Jason

3 years, 11 months

2
1
0 0

Re: [Linaro-mm-sig] [PATCH v3 1/2] habanalabs: define uAPI to export FD for DMA-BUF

by Daniel Vetter

On Fri, Jun 18, 2021 at 2:36 PM Oded Gabbay <ogabbay(a)kernel.org> wrote: > User process might want to share the device memory with another > driver/device, and to allow it to access it over PCIe (P2P). > > To enable this, we utilize the dma-buf mechanism and add a dma-buf > exporter support, so the other driver can import the device memory and > access it. > > The device memory is allocated using our existing allocation uAPI, > where the user will get a handle that represents the allocation. > > The user will then need to call the new > uAPI (HL_MEM_OP_EXPORT_DMABUF_FD) and give the handle as a parameter. > > The driver will return a FD that represents the DMA-BUF object that > was created to match that allocation. > > Signed-off-by: Oded Gabbay <ogabbay(a)kernel.org> > Reviewed-by: Tomer Tayar <ttayar(a)habana.ai> Mission acomplished, we've gone full circle, and the totally-not-a-gpu driver is now trying to use gpu infrastructure. And seems to have gained vram meanwhile too. Next up is going to be synchronization using dma_fence so you can pass buffers back&forth without stalls among drivers. Bonus points for this being at v3 before it shows up on dri-devel and cc's dma-buf folks properly (not quite all, I added the missing people). I think we roughly have two options here a) Greg continues to piss off dri-devel folks while trying to look cute&cuddly and steadfastly claiming that this accelator doesn't work like any of the other accelerator drivers we have in drivers/gpu/drm. All while the driver ever more looks like one of these other accel drivers. b) We finally do what we should have done years back and treat this as a proper driver submission and review it on dri-devel instead of sneaking it in through other channels because the merge criteria dri-devel has are too onerous and people who don't have experience with accel stacks for the past 20 years or so don't like them. "But this probably means a new driver and big disruption!" Not my problem, I'm not the dude who has to come up with an excuse for this because I didn't merge the driver in the first place. I do get to throw a "we all told you so" in though, but that's not helping. Also I'm wondering which is the other driver that we share buffers with. The gaudi stuff doesn't have real struct pages as backing storage, it only fills out the dma_addr_t. That tends to blow up with other drivers, and the only place where this is guaranteed to work is if you have a dynamic importer which sets the allow_peer2peer flag. Adding maintainers from other subsystems who might want to chime in here. So even aside of the big question as-is this is broken. Currently only 2 drivers set allow_peer2peer, so those are the only ones who can consume these buffers from device memory. Pinging those folks specifically. Doug/Jason from infiniband: Should we add linux-rdma to the dma-buf wildcard match so that you can catch these next time around too? At least when people use scripts/get_maintainers.pl correctly. All the other subsystems using dma-buf are on there already (dri-devel, linux-media and linaro-mm-sig for android/arm embedded stuff). Cheers, Daniel > --- > include/uapi/misc/habanalabs.h | 28 +++++++++++++++++++++++++++- > 1 file changed, 27 insertions(+), 1 deletion(-) > > diff --git a/include/uapi/misc/habanalabs.h b/include/uapi/misc/habanalabs.h > index a47a731e4527..aa3d8e0ba060 100644 > --- a/include/uapi/misc/habanalabs.h > +++ b/include/uapi/misc/habanalabs.h > @@ -808,6 +808,10 @@ union hl_wait_cs_args { > #define HL_MEM_OP_UNMAP 3 > /* Opcode to map a hw block */ > #define HL_MEM_OP_MAP_BLOCK 4 > +/* Opcode to create DMA-BUF object for an existing device memory allocation > + * and to export an FD of that DMA-BUF back to the caller > + */ > +#define HL_MEM_OP_EXPORT_DMABUF_FD 5 > > /* Memory flags */ > #define HL_MEM_CONTIGUOUS 0x1 > @@ -878,11 +882,26 @@ struct hl_mem_in { > /* Virtual address returned from HL_MEM_OP_MAP */ > __u64 device_virt_addr; > } unmap; > + > + /* HL_MEM_OP_EXPORT_DMABUF_FD */ > + struct { > + /* Handle returned from HL_MEM_OP_ALLOC. In Gaudi, > + * where we don't have MMU for the device memory, the > + * driver expects a physical address (instead of > + * a handle) in the device memory space. > + */ > + __u64 handle; > + /* Size of memory allocation. Relevant only for GAUDI */ > + __u64 mem_size; > + } export_dmabuf_fd; > }; > > /* HL_MEM_OP_* */ > __u32 op; > - /* HL_MEM_* flags */ > + /* HL_MEM_* flags. > + * For the HL_MEM_OP_EXPORT_DMABUF_FD opcode, this field holds the > + * DMA-BUF file/FD flags. > + */ > __u32 flags; > /* Context ID - Currently not in use */ > __u32 ctx_id; > @@ -919,6 +938,13 @@ struct hl_mem_out { > > __u32 pad; > }; > + > + /* Returned in HL_MEM_OP_EXPORT_DMABUF_FD. Represents the > + * DMA-BUF object that was created to describe a memory > + * allocation on the device's memory space. The FD should be > + * passed to the importer driver > + */ > + __u64 fd; > }; > }; > > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 11 months

4
5
0 0

Re: [Linaro-mm-sig] [PATCH v3 2/2] drm: protect drm_master pointers in drm_lease.c

by Daniel Vetter

On Sun, Jun 20, 2021 at 07:03:27PM +0800, Desmond Cheong Zhi Xi wrote: > Currently, direct copies of drm_file->master pointers should be > protected by drm_device.master_mutex when being dereferenced. This is > because drm_file->master is not invariant for the lifetime of > drm_file. If drm_file is not the creator of master, then > drm_file->is_master is false, and a call to drm_setmaster_ioctl will > invoke drm_new_set_master, which then allocates a new master for > drm_file and puts the old master. > > Thus, without holding drm_device.master_mutex, the old value of > drm_file->master could be freed while it is being used by another > concurrent process. > > In drm_lease.c, there are multiple instances where drm_file->master is > accessed and dereferenced while drm_device.master_mutex is not > held. This makes drm_lease.c vulnerable to use-after-free bugs. > > We address this issue as follows: > > 1. Clarify in the kerneldoc that drm_file->master is protected by > drm_device.master_mutex. > > 2. Add a new drm_file_get_master() function that calls drm_master_get > on drm_file->master while holding on to drm_device.master_mutex. Since > drm_master_get increments the reference count of master, this > prevents master from being freed until we unreference it with > drm_master_put. > > 3. In each case where drm_file->master is directly accessed and > eventually dereferenced in drm_lease.c, we wrap the access in a call > to the new drm_file_get_master function, then unreference the master > pointer once we are done using it. > > Reported-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> > Signed-off-by: Desmond Cheong Zhi Xi <desmondcheongzx(a)gmail.com> I think this approach looks much clearer than the previous. I've found a few smaller things below still. I think at least ... Cheers, Daniel > --- > drivers/gpu/drm/drm_auth.c | 22 ++++++++++++++ > drivers/gpu/drm/drm_lease.c | 57 ++++++++++++++++++++++++++----------- > include/drm/drm_auth.h | 1 + > include/drm/drm_file.h | 15 ++++++++-- > 4 files changed, 75 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c > index 86d4b72e95cb..0c64a77c67a6 100644 > --- a/drivers/gpu/drm/drm_auth.c > +++ b/drivers/gpu/drm/drm_auth.c > @@ -384,6 +384,28 @@ struct drm_master *drm_master_get(struct drm_master *master) > } > EXPORT_SYMBOL(drm_master_get); > > +/** > + * drm_file_get_master - reference @file_priv->master > + * @file_priv: DRM file private > + * > + * Increments the reference count of @file_priv->master and returns Does this format correctly? I'd go with "&drm_file.master of @file_priv". > + * @file_priv->master. > + * > + * Master pointers returned from this function should be unreferenced using > + * drm_master_put(). > + */ > +struct drm_master *drm_file_get_master(struct drm_file *file_priv) > +{ > + struct drm_master *master; > + > + mutex_lock(&file_priv->master->dev->master_mutex); > + master = drm_master_get(file_priv->master); > + mutex_unlock(&file_priv->master->dev->master_mutex); > + > + return master; > +} > +EXPORT_SYMBOL(drm_file_get_master); > + > static void drm_master_destroy(struct kref *kref) > { > struct drm_master *master = container_of(kref, struct drm_master, refcount); > diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c > index da4f085fc09e..65eab82f8acc 100644 > --- a/drivers/gpu/drm/drm_lease.c > +++ b/drivers/gpu/drm/drm_lease.c > @@ -107,10 +107,17 @@ static bool _drm_has_leased(struct drm_master *master, int id) > */ > bool _drm_lease_held(struct drm_file *file_priv, int id) > { > + bool ret; > + struct drm_master *master; > + > if (!file_priv || !file_priv->master) So here we still have a ->master access outside of the locked code section. I think the best fix for that would be to move the NULL check into drm_file_get_master (where we grab the lock already anyway), and update the kerneldoc to state that it might return NULL. Same with all the checks for ->master below. > return true; > > - return _drm_lease_held_master(file_priv->master, id); > + master = drm_file_get_master(file_priv); > + ret = _drm_lease_held_master(master, id); > + drm_master_put(&master); > + > + return ret; > } > > /** > @@ -132,10 +139,11 @@ bool drm_lease_held(struct drm_file *file_priv, int id) > if (!file_priv || !file_priv->master || !file_priv->master->lessor) > return true; master->lessor dereferenced outside the lock or without holding a reference. > > - master = file_priv->master; > + master = drm_file_get_master(file_priv); > mutex_lock(&master->dev->mode_config.idr_mutex); > ret = _drm_lease_held_master(master, id); > mutex_unlock(&master->dev->mode_config.idr_mutex); > + drm_master_put(&master); > return ret; > } > > @@ -158,7 +166,7 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in) > if (!file_priv || !file_priv->master || !file_priv->master->lessor) > return crtcs_in; Same here. > > - master = file_priv->master; > + master = drm_file_get_master(file_priv); > dev = master->dev; > > count_in = count_out = 0; > @@ -177,6 +185,7 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in) > count_in++; > } > mutex_unlock(&master->dev->mode_config.idr_mutex); > + drm_master_put(&master); > return crtcs_out; > } > > @@ -490,7 +499,7 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > size_t object_count; > int ret = 0; > struct idr leases; > - struct drm_master *lessor = lessor_priv->master; > + struct drm_master *lessor; > struct drm_master *lessee = NULL; > struct file *lessee_file = NULL; > struct file *lessor_file = lessor_priv->filp; > @@ -502,12 +511,6 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > if (!drm_core_check_feature(dev, DRIVER_MODESET)) > return -EOPNOTSUPP; > > - /* Do not allow sub-leases */ > - if (lessor->lessor) { > - DRM_DEBUG_LEASE("recursive leasing not allowed\n"); > - return -EINVAL; > - } > - > /* need some objects */ > if (cl->object_count == 0) { > DRM_DEBUG_LEASE("no objects in lease\n"); > @@ -519,12 +522,22 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > return -EINVAL; > } > > + lessor = drm_file_get_master(lessor_priv); > + /* Do not allow sub-leases */ > + if (lessor->lessor) { Here we check after grabbing the reference, so looks correct. > + DRM_DEBUG_LEASE("recursive leasing not allowed\n"); > + ret = -EINVAL; > + goto out_lessor; > + } > + > object_count = cl->object_count; > > object_ids = memdup_user(u64_to_user_ptr(cl->object_ids), > array_size(object_count, sizeof(__u32))); > - if (IS_ERR(object_ids)) > - return PTR_ERR(object_ids); > + if (IS_ERR(object_ids)) { > + ret = PTR_ERR(object_ids); > + goto out_lessor; > + } > > idr_init(&leases); > > @@ -535,14 +548,15 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > if (ret) { > DRM_DEBUG_LEASE("lease object lookup failed: %i\n", ret); > idr_destroy(&leases); > - return ret; > + goto out_lessor; > } > > /* Allocate a file descriptor for the lease */ > fd = get_unused_fd_flags(cl->flags & (O_CLOEXEC | O_NONBLOCK)); > if (fd < 0) { > idr_destroy(&leases); > - return fd; > + ret = fd; > + goto out_lessor; > } > > DRM_DEBUG_LEASE("Creating lease\n"); > @@ -578,6 +592,7 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > /* Hook up the fd */ > fd_install(fd, lessee_file); > > + drm_master_put(&lessor); > DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl succeeded\n"); > return 0; > > @@ -587,6 +602,8 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > out_leases: > put_unused_fd(fd); > > +out_lessor: > + drm_master_put(&lessor); > DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl failed: %d\n", ret); > return ret; > } > @@ -609,7 +626,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev, > struct drm_mode_list_lessees *arg = data; > __u32 __user *lessee_ids = (__u32 __user *) (uintptr_t) (arg->lessees_ptr); > __u32 count_lessees = arg->count_lessees; > - struct drm_master *lessor = lessor_priv->master, *lessee; > + struct drm_master *lessor, *lessee; > int count; > int ret = 0; > > @@ -620,6 +637,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev, > if (!drm_core_check_feature(dev, DRIVER_MODESET)) > return -EOPNOTSUPP; > > + lessor = drm_file_get_master(lessor_priv); > DRM_DEBUG_LEASE("List lessees for %d\n", lessor->lessee_id); > > mutex_lock(&dev->mode_config.idr_mutex); > @@ -643,6 +661,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev, > arg->count_lessees = count; > > mutex_unlock(&dev->mode_config.idr_mutex); > + drm_master_put(&lessor); > > return ret; > } > @@ -662,7 +681,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev, > struct drm_mode_get_lease *arg = data; > __u32 __user *object_ids = (__u32 __user *) (uintptr_t) (arg->objects_ptr); > __u32 count_objects = arg->count_objects; > - struct drm_master *lessee = lessee_priv->master; > + struct drm_master *lessee; > struct idr *object_idr; > int count; > void *entry; > @@ -676,6 +695,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev, > if (!drm_core_check_feature(dev, DRIVER_MODESET)) > return -EOPNOTSUPP; > > + lessee = drm_file_get_master(lessee_priv); > DRM_DEBUG_LEASE("get lease for %d\n", lessee->lessee_id); > > mutex_lock(&dev->mode_config.idr_mutex); > @@ -703,6 +723,7 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev, > arg->count_objects = count; > > mutex_unlock(&dev->mode_config.idr_mutex); > + drm_master_put(&lessee); > > return ret; > } > @@ -721,7 +742,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev, > void *data, struct drm_file *lessor_priv) > { > struct drm_mode_revoke_lease *arg = data; > - struct drm_master *lessor = lessor_priv->master; > + struct drm_master *lessor; > struct drm_master *lessee; > int ret = 0; > > @@ -731,6 +752,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev, > if (!drm_core_check_feature(dev, DRIVER_MODESET)) > return -EOPNOTSUPP; > > + lessor = drm_file_get_master(lessor_priv); > mutex_lock(&dev->mode_config.idr_mutex); > > lessee = _drm_find_lessee(lessor, arg->lessee_id); > @@ -751,6 +773,7 @@ int drm_mode_revoke_lease_ioctl(struct drm_device *dev, > > fail: > mutex_unlock(&dev->mode_config.idr_mutex); > + drm_master_put(&lessor); > > return ret; > } > diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h > index 6bf8b2b78991..f99d3417f304 100644 > --- a/include/drm/drm_auth.h > +++ b/include/drm/drm_auth.h > @@ -107,6 +107,7 @@ struct drm_master { > }; > > struct drm_master *drm_master_get(struct drm_master *master); > +struct drm_master *drm_file_get_master(struct drm_file *file_priv); > void drm_master_put(struct drm_master **master); > bool drm_is_current_master(struct drm_file *fpriv); > > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h > index b81b3bfb08c8..e9931fca4ab7 100644 > --- a/include/drm/drm_file.h > +++ b/include/drm/drm_file.h > @@ -226,9 +226,18 @@ struct drm_file { > /** > * @master: > * > - * Master this node is currently associated with. Only relevant if > - * drm_is_primary_client() returns true. Note that this only > - * matches &drm_device.master if the master is the currently active one. > + * Master this node is currently associated with. Protected by struct > + * &drm_device.master_mutex. > + * > + * Only relevant if drm_is_primary_client() returns true. Note that > + * this only matches &drm_device.master if the master is the currently > + * active one. > + * > + * When obtaining a copy of this pointer, it is recommended to either > + * hold struct &drm_device.master_mutex for the duration of the > + * pointer's use, or to use drm_file_get_master() if struct > + * &drm_device.master_mutex is not currently held and there is no other > + * need to hold it. This prevents @master from being freed during use. > * > * See also @authentication and @is_master and the :ref:`section on > * primary nodes and authentication <drm_primary_node>`. > -- > 2.25.1 > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

3 years, 11 months

1
0
0 0

Fixing problems around shared fences and RCU in DMA-buf

by Christian König

Hi everyone and especially Daniel, this is the revised patch set to fix and rework dma_buf_poll(). The new code should avoid problems with RCU and also now correctly waits for all fences in the resv object. The rest of the series is then the well known change to dma_resv_test_signaled(), nouveau and now new also msm. Then last are two patches which drop the workarounds from amdgpu, but those can wait till the next cycle. I think it would be rather good if the have at least to change to dma_buf_poll() pushed in this merge window and maybe even CC stable since this looks really broken to me. Please review, test and/or comment. Thanks, Christian.

3 years, 11 months

2
8
0 0

Introduce fence iterators to abstract dma_resv RCU handling

by Christian König

Hi guys, during the recent discussion about SLAB_TYPESAFE_BY_RCU, dma_fence_get_rcu and dma_fence_get_rcu_safe we found that the RCU handling for dma_resv objects was implemented multiple times. Unfortunately a lot of those implementations get the rather complicated dance with RCU and the sequence number handling wrong. So this patch set aims to audit and unify this by providing an iterator which automatically restarts when a modification to the dma_resv object is detected. The result is pretty impressive I think since this not only mean that we got rid of all those incorrect dma_fence_get_rcu() cases, but also reduce the overall loc count quite a bit. Please review and/or comment. Cheers, Christian.

3 years, 11 months

1
16
0 0

[PATCH v6] dmabuf: Add the capability to expose DMA-BUF stats in sysfs

by Hridya Valsaraju

Overview ======== The patch adds DMA-BUF statistics to /sys/kernel/dmabuf/buffers. It allows statistics to be enabled for each DMA-BUF in sysfs by enabling the config CONFIG_DMABUF_SYSFS_STATS. The following stats will be exposed by the interface: /sys/kernel/dmabuf/buffers/<inode_number>/exporter_name /sys/kernel/dmabuf/buffers/<inode_number>/size /sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attach_uid>/device /sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attach_uid>/map_counter The inode_number is unique for each DMA-BUF and was added earlier [1] in order to allow userspace to track DMA-BUF usage across different processes. Use Cases ========= The interface provides a way to gather DMA-BUF per-buffer statistics from production devices. These statistics will be used to derive DMA-BUF per-exporter stats and per-device usage stats for Android Bug reports. The corresponding userspace changes can be found at [2]. Telemetry tools will also capture this information(along with other memory metrics) periodically as well as on important events like a foreground app kill (which might have been triggered by Low Memory Killer). It will also contribute to provide a snapshot of the system memory usage on other events such as OOM kills and Application Not Responding events. Background ========== Currently, there are two existing interfaces that provide information about DMA-BUFs. 1) /sys/kernel/debug/dma_buf/bufinfo debugfs is however unsuitable to be mounted in production systems and cannot be considered as an alternative to the sysfs interface being proposed. 2) proc/<pid>/fdinfo/<fd> The proc/<pid>/fdinfo/<fd> files expose information about DMA-BUF fds. However, the existing procfs interfaces can only provide information about the buffers for which processes hold fds or have the buffers mmapped into their address space. Since the procfs interfaces alone cannot provide a full picture of all DMA-BUFs in the system, there is the need for an alternate interface to provide this information on production systems. The patch contains the following major improvements over v1: 1) Each attachment is represented by its own directory to allow creating a symlink to the importing device and to also provide room for future expansion. 2) The number of distinct mappings of each attachment is exposed in a separate file. 3) The per-buffer statistics are now in /sys/kernel/dmabuf/buffers inorder to make the interface expandable in future. All of the improvements above are based on suggestions/feedback from Daniel Vetter and Christian König. A shell script that can be run on a classic Linux environment to read out the DMA-BUF statistics can be found at [3](suggested by John Stultz). [1]: https://lore.kernel.org/patchwork/patch/1088791/ [2]: https://android-review.googlesource.com/q/topic:%22dmabuf-sysfs%22+(status:… [3]: https://android-review.googlesource.com/c/platform/system/memory/libmeminfo… Reviewed-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org> Signed-off-by: Hridya Valsaraju <hridya(a)google.com> Reported-by: kernel test robot <lkp(a)intel.com> --- Hello Daniel, I have added the documentation as a DOC: overview section in the dma-buf-sysfs-stats.c file as per your suggestion. Please do take a look when you get a chance. Thanks in advance! Regards, Hridya Change in v6: -Moved documentation content from Documentation/driver-api/dma-buf.rst to drivers/dma-buf/dma-buf-sysfs-stats.c as a DOC section and linked to it from Documentation/driver-api/dma-buf.rst. Based on feedback from Daniel Vetter. Change in v5: -Added a section on DMA-BUF statistics to Documentation/driver-api/dma-buf.rst. Organized the commit message to clearly state the need for the new interface and provide the background on why the existing means of DMA-BUF accounting will not suffice. Based on feedback from Daniel Vetter. Changes in v4: -Suppress uevents from kset creation to avoid waking up uevent listeners on DMA-BUF export/release. Changes in v3: -Fix a warning reported by the kernel test robot. Changes in v2: -Move statistics to /sys/kernel/dmabuf/buffers in oder to allow addition of other DMA-BUF-related sysfs stats in future. Based on feedback from Daniel Vetter. -Each attachment has its own directory to represent attached devices as symlinks and to introduce map_count as a separate file. Based on feedback from Daniel Vetter and Christian König. Thank you both! -Commit messages updated to point to userspace code in AOSP that will read the DMA-BUF sysfs stats. .../ABI/testing/sysfs-kernel-dmabuf-buffers | 52 +++ Documentation/driver-api/dma-buf.rst | 5 + drivers/dma-buf/Kconfig | 11 + drivers/dma-buf/Makefile | 1 + drivers/dma-buf/dma-buf-sysfs-stats.c | 337 ++++++++++++++++++ drivers/dma-buf/dma-buf-sysfs-stats.h | 62 ++++ drivers/dma-buf/dma-buf.c | 37 ++ include/linux/dma-buf.h | 20 ++ 8 files changed, 525 insertions(+) create mode 100644 Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.c create mode 100644 drivers/dma-buf/dma-buf-sysfs-stats.h diff --git a/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers new file mode 100644 index 000000000000..a243984ed420 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers @@ -0,0 +1,52 @@ +What: /sys/kernel/dmabuf/buffers +Date: May 2021 +KernelVersion: v5.13 +Contact: Hridya Valsaraju <hridya(a)google.com> +Description: The /sys/kernel/dmabuf/buffers directory contains a + snapshot of the internal state of every DMA-BUF. + /sys/kernel/dmabuf/buffers/<inode_number> will contain the + statistics for the DMA-BUF with the unique inode number + <inode_number> +Users: kernel memory tuning/debugging tools + +What: /sys/kernel/dmabuf/buffers/<inode_number>/exporter_name +Date: May 2021 +KernelVersion: v5.13 +Contact: Hridya Valsaraju <hridya(a)google.com> +Description: This file is read-only and contains the name of the exporter of + the DMA-BUF. + +What: /sys/kernel/dmabuf/buffers/<inode_number>/size +Date: May 2021 +KernelVersion: v5.13 +Contact: Hridya Valsaraju <hridya(a)google.com> +Description: This file is read-only and specifies the size of the DMA-BUF in + bytes. + +What: /sys/kernel/dmabuf/buffers/<inode_number>/attachments +Date: May 2021 +KernelVersion: v5.13 +Contact: Hridya Valsaraju <hridya(a)google.com> +Description: This directory will contain subdirectories representing every + attachment of the DMA-BUF. + +What: /sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attachment_uid> +Date: May 2021 +KernelVersion: v5.13 +Contact: Hridya Valsaraju <hridya(a)google.com> +Description: This directory will contain information on the attached device + and the number of current distinct device mappings. + +What: /sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attachment_uid>/device +Date: May 2021 +KernelVersion: v5.13 +Contact: Hridya Valsaraju <hridya(a)google.com> +Description: This file is read-only and is a symlink to the attached device's + sysfs entry. + +What: /sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attachment_uid>/map_counter +Date: May 2021 +KernelVersion: v5.13 +Contact: Hridya Valsaraju <hridya(a)google.com> +Description: This file is read-only and contains a map_counter indicating the + number of distinct device mappings of the attachment. diff --git a/Documentation/driver-api/dma-buf.rst b/Documentation/driver-api/dma-buf.rst index 7f37ec30d9fd..d47a429dc549 100644 --- a/Documentation/driver-api/dma-buf.rst +++ b/Documentation/driver-api/dma-buf.rst @@ -106,6 +106,11 @@ Implicit Fence Poll Support .. kernel-doc:: drivers/dma-buf/dma-buf.c :doc: implicit fence polling +DMA-BUF statistics +~~~~~~~~~~~~~~~~~~ +.. kernel-doc:: drivers/dma-buf/dma-buf-sysfs-stats.c + :doc: overview + Kernel Functions and Structures Reference ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/drivers/dma-buf/Kconfig b/drivers/dma-buf/Kconfig index 4e16c71c24b7..9561e3d2d428 100644 --- a/drivers/dma-buf/Kconfig +++ b/drivers/dma-buf/Kconfig @@ -72,6 +72,17 @@ menuconfig DMABUF_HEAPS allows userspace to allocate dma-bufs that can be shared between drivers. +menuconfig DMABUF_SYSFS_STATS + bool "DMA-BUF sysfs statistics" + select DMA_SHARED_BUFFER + help + Choose this option to enable DMA-BUF sysfs statistics + in location /sys/kernel/dmabuf/buffers. + + /sys/kernel/dmabuf/buffers/<inode_number> will contain + statistics for the DMA-BUF with the unique inode number + <inode_number>. + source "drivers/dma-buf/heaps/Kconfig" endmenu diff --git a/drivers/dma-buf/Makefile b/drivers/dma-buf/Makefile index 995e05f609ff..40d81f23cacf 100644 --- a/drivers/dma-buf/Makefile +++ b/drivers/dma-buf/Makefile @@ -6,6 +6,7 @@ obj-$(CONFIG_DMABUF_HEAPS) += heaps/ obj-$(CONFIG_SYNC_FILE) += sync_file.o obj-$(CONFIG_SW_SYNC) += sw_sync.o sync_debug.o obj-$(CONFIG_UDMABUF) += udmabuf.o +obj-$(CONFIG_DMABUF_SYSFS_STATS) += dma-buf-sysfs-stats.o dmabuf_selftests-y := \ selftest.o \ diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.c b/drivers/dma-buf/dma-buf-sysfs-stats.c new file mode 100644 index 000000000000..a2638e84199c --- /dev/null +++ b/drivers/dma-buf/dma-buf-sysfs-stats.c @@ -0,0 +1,337 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * DMA-BUF sysfs statistics. + * + * Copyright (C) 2021 Google LLC. + */ + +#include <linux/dma-buf.h> +#include <linux/dma-resv.h> +#include <linux/kobject.h> +#include <linux/printk.h> +#include <linux/slab.h> +#include <linux/sysfs.h> + +#include "dma-buf-sysfs-stats.h" + +#define to_dma_buf_entry_from_kobj(x) container_of(x, struct dma_buf_sysfs_entry, kobj) + +/** + * DOC: overview + * + * ``/sys/kernel/debug/dma_buf/bufinfo`` provides an overview of every DMA-BUF + * in the system. However, since debugfs is not safe to be mounted in + * production, procfs and sysfs can be used to gather DMA-BUF statistics on + * production systems. + * + * The ``/proc/<pid>/fdinfo/<fd>`` files in procfs can be used to gather + * information about DMA-BUF fds. Detailed documentation about the interface + * is present in Documentation/filesystems/proc.rst. + * + * Unfortunately, the existing procfs interfaces can only provide information + * about the DMA-BUFs for which processes hold fds or have the buffers mmapped + * into their address space. This necessitated the creation of the DMA-BUF sysfs + * statistics interface to provide per-buffer information on production systems. + * + * The interface at ``/sys/kernel/dma-buf/buffers`` exposes information about + * every DMA-BUF when ``CONFIG_DMABUF_SYSFS_STATS`` is enabled. + * + * The following stats are exposed by the interface: + * + * * ``/sys/kernel/dmabuf/buffers/<inode_number>/exporter_name`` + * * ``/sys/kernel/dmabuf/buffers/<inode_number>/size`` + * * ``/sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attach_uid>/device`` + * * ``/sys/kernel/dmabuf/buffers/<inode_number>/attachments/<attach_uid>/map_counter`` + * + * The information in the interface can also be used to derive per-exporter and + * per-device usage statistics. The data from the interface can be gathered + * on error conditions or other important events to provide a snapshot of + * DMA-BUF usage. It can also be collected periodically by telemetry to monitor + * various metrics. + * + * Detailed documentation about the interface is present in + * Documentation/ABI/testing/sysfs-kernel-dmabuf-buffers. + */ + +struct dma_buf_stats_attribute { + struct attribute attr; + ssize_t (*show)(struct dma_buf *dmabuf, + struct dma_buf_stats_attribute *attr, char *buf); +}; +#define to_dma_buf_stats_attr(x) container_of(x, struct dma_buf_stats_attribute, attr) + +static ssize_t dma_buf_stats_attribute_show(struct kobject *kobj, + struct attribute *attr, + char *buf) +{ + struct dma_buf_stats_attribute *attribute; + struct dma_buf_sysfs_entry *sysfs_entry; + struct dma_buf *dmabuf; + + attribute = to_dma_buf_stats_attr(attr); + sysfs_entry = to_dma_buf_entry_from_kobj(kobj); + dmabuf = sysfs_entry->dmabuf; + + if (!dmabuf || !attribute->show) + return -EIO; + + return attribute->show(dmabuf, attribute, buf); +} + +static const struct sysfs_ops dma_buf_stats_sysfs_ops = { + .show = dma_buf_stats_attribute_show, +}; + +static ssize_t exporter_name_show(struct dma_buf *dmabuf, + struct dma_buf_stats_attribute *attr, + char *buf) +{ + return sysfs_emit(buf, "%s\n", dmabuf->exp_name); +} + +static ssize_t size_show(struct dma_buf *dmabuf, + struct dma_buf_stats_attribute *attr, + char *buf) +{ + return sysfs_emit(buf, "%zu\n", dmabuf->size); +} + +static struct dma_buf_stats_attribute exporter_name_attribute = + __ATTR_RO(exporter_name); +static struct dma_buf_stats_attribute size_attribute = __ATTR_RO(size); + +static struct attribute *dma_buf_stats_default_attrs[] = { + &exporter_name_attribute.attr, + &size_attribute.attr, + NULL, +}; +ATTRIBUTE_GROUPS(dma_buf_stats_default); + +static void dma_buf_sysfs_release(struct kobject *kobj) +{ + struct dma_buf_sysfs_entry *sysfs_entry; + + sysfs_entry = to_dma_buf_entry_from_kobj(kobj); + kfree(sysfs_entry); +} + +static struct kobj_type dma_buf_ktype = { + .sysfs_ops = &dma_buf_stats_sysfs_ops, + .release = dma_buf_sysfs_release, + .default_groups = dma_buf_stats_default_groups, +}; + +#define to_dma_buf_attach_entry_from_kobj(x) container_of(x, struct dma_buf_attach_sysfs_entry, kobj) + +struct dma_buf_attach_stats_attribute { + struct attribute attr; + ssize_t (*show)(struct dma_buf_attach_sysfs_entry *sysfs_entry, + struct dma_buf_attach_stats_attribute *attr, char *buf); +}; +#define to_dma_buf_attach_stats_attr(x) container_of(x, struct dma_buf_attach_stats_attribute, attr) + +static ssize_t dma_buf_attach_stats_attribute_show(struct kobject *kobj, + struct attribute *attr, + char *buf) +{ + struct dma_buf_attach_stats_attribute *attribute; + struct dma_buf_attach_sysfs_entry *sysfs_entry; + + attribute = to_dma_buf_attach_stats_attr(attr); + sysfs_entry = to_dma_buf_attach_entry_from_kobj(kobj); + + if (!attribute->show) + return -EIO; + + return attribute->show(sysfs_entry, attribute, buf); +} + +static const struct sysfs_ops dma_buf_attach_stats_sysfs_ops = { + .show = dma_buf_attach_stats_attribute_show, +}; + +static ssize_t map_counter_show(struct dma_buf_attach_sysfs_entry *sysfs_entry, + struct dma_buf_attach_stats_attribute *attr, + char *buf) +{ + return sysfs_emit(buf, "%u\n", sysfs_entry->map_counter); +} + +static struct dma_buf_attach_stats_attribute map_counter_attribute = + __ATTR_RO(map_counter); + +static struct attribute *dma_buf_attach_stats_default_attrs[] = { + &map_counter_attribute.attr, + NULL, +}; +ATTRIBUTE_GROUPS(dma_buf_attach_stats_default); + +static void dma_buf_attach_sysfs_release(struct kobject *kobj) +{ + struct dma_buf_attach_sysfs_entry *sysfs_entry; + + sysfs_entry = to_dma_buf_attach_entry_from_kobj(kobj); + kfree(sysfs_entry); +} + +static struct kobj_type dma_buf_attach_ktype = { + .sysfs_ops = &dma_buf_attach_stats_sysfs_ops, + .release = dma_buf_attach_sysfs_release, + .default_groups = dma_buf_attach_stats_default_groups, +}; + +void dma_buf_attach_stats_teardown(struct dma_buf_attachment *attach) +{ + struct dma_buf_attach_sysfs_entry *sysfs_entry; + + sysfs_entry = attach->sysfs_entry; + if (!sysfs_entry) + return; + + sysfs_delete_link(&sysfs_entry->kobj, &attach->dev->kobj, "device"); + + kobject_del(&sysfs_entry->kobj); + kobject_put(&sysfs_entry->kobj); +} + +int dma_buf_attach_stats_setup(struct dma_buf_attachment *attach, + unsigned int uid) +{ + struct dma_buf_attach_sysfs_entry *sysfs_entry; + int ret; + struct dma_buf *dmabuf; + + if (!attach) + return -EINVAL; + + dmabuf = attach->dmabuf; + + sysfs_entry = kzalloc(sizeof(struct dma_buf_attach_sysfs_entry), + GFP_KERNEL); + if (!sysfs_entry) + return -ENOMEM; + + sysfs_entry->kobj.kset = dmabuf->sysfs_entry->attach_stats_kset; + + attach->sysfs_entry = sysfs_entry; + + ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_attach_ktype, + NULL, "%u", uid); + if (ret) + goto kobj_err; + + ret = sysfs_create_link(&sysfs_entry->kobj, &attach->dev->kobj, + "device"); + if (ret) + goto link_err; + + return 0; + +link_err: + kobject_del(&sysfs_entry->kobj); +kobj_err: + kobject_put(&sysfs_entry->kobj); + attach->sysfs_entry = NULL; + + return ret; +} +void dma_buf_stats_teardown(struct dma_buf *dmabuf) +{ + struct dma_buf_sysfs_entry *sysfs_entry; + + sysfs_entry = dmabuf->sysfs_entry; + if (!sysfs_entry) + return; + + kset_unregister(sysfs_entry->attach_stats_kset); + kobject_del(&sysfs_entry->kobj); + kobject_put(&sysfs_entry->kobj); +} + + +/* Statistics files do not need to send uevents. */ +static int dmabuf_sysfs_uevent_filter(struct kset *kset, struct kobject *kobj) +{ + return 0; +} + +static const struct kset_uevent_ops dmabuf_sysfs_no_uevent_ops = { + .filter = dmabuf_sysfs_uevent_filter, +}; + +static struct kset *dma_buf_stats_kset; +static struct kset *dma_buf_per_buffer_stats_kset; +int dma_buf_init_sysfs_statistics(void) +{ + dma_buf_stats_kset = kset_create_and_add("dmabuf", + &dmabuf_sysfs_no_uevent_ops, + kernel_kobj); + if (!dma_buf_stats_kset) + return -ENOMEM; + + dma_buf_per_buffer_stats_kset = kset_create_and_add("buffers", + &dmabuf_sysfs_no_uevent_ops, + &dma_buf_stats_kset->kobj); + if (!dma_buf_per_buffer_stats_kset) { + kset_unregister(dma_buf_stats_kset); + return -ENOMEM; + } + + return 0; +} + +void dma_buf_uninit_sysfs_statistics(void) +{ + kset_unregister(dma_buf_per_buffer_stats_kset); + kset_unregister(dma_buf_stats_kset); +} + +int dma_buf_stats_setup(struct dma_buf *dmabuf) +{ + struct dma_buf_sysfs_entry *sysfs_entry; + int ret; + struct kset *attach_stats_kset; + + if (!dmabuf || !dmabuf->file) + return -EINVAL; + + if (!dmabuf->exp_name) { + pr_err("exporter name must not be empty if stats needed\n"); + return -EINVAL; + } + + sysfs_entry = kzalloc(sizeof(struct dma_buf_sysfs_entry), GFP_KERNEL); + if (!sysfs_entry) + return -ENOMEM; + + sysfs_entry->kobj.kset = dma_buf_per_buffer_stats_kset; + sysfs_entry->dmabuf = dmabuf; + + dmabuf->sysfs_entry = sysfs_entry; + + /* create the directory for buffer stats */ + ret = kobject_init_and_add(&sysfs_entry->kobj, &dma_buf_ktype, NULL, + "%lu", file_inode(dmabuf->file)->i_ino); + if (ret) + goto err_sysfs_dmabuf; + + /* create the directory for attachment stats */ + attach_stats_kset = kset_create_and_add("attachments", + &dmabuf_sysfs_no_uevent_ops, + &sysfs_entry->kobj); + if (!attach_stats_kset) { + ret = -ENOMEM; + goto err_sysfs_attach; + } + + sysfs_entry->attach_stats_kset = attach_stats_kset; + + return 0; + +err_sysfs_attach: + kobject_del(&sysfs_entry->kobj); +err_sysfs_dmabuf: + kobject_put(&sysfs_entry->kobj); + dmabuf->sysfs_entry = NULL; + return ret; +} diff --git a/drivers/dma-buf/dma-buf-sysfs-stats.h b/drivers/dma-buf/dma-buf-sysfs-stats.h new file mode 100644 index 000000000000..5f4703249117 --- /dev/null +++ b/drivers/dma-buf/dma-buf-sysfs-stats.h @@ -0,0 +1,62 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * DMA-BUF sysfs statistics. + * + * Copyright (C) 2021 Google LLC. + */ + +#ifndef _DMA_BUF_SYSFS_STATS_H +#define _DMA_BUF_SYSFS_STATS_H + +#ifdef CONFIG_DMABUF_SYSFS_STATS + +int dma_buf_init_sysfs_statistics(void); +void dma_buf_uninit_sysfs_statistics(void); + +int dma_buf_stats_setup(struct dma_buf *dmabuf); +int dma_buf_attach_stats_setup(struct dma_buf_attachment *attach, + unsigned int uid); +static inline void dma_buf_update_attachment_map_count(struct dma_buf_attachment *attach, + int delta) +{ + struct dma_buf_attach_sysfs_entry *entry = attach->sysfs_entry; + + entry->map_counter += delta; +} +void dma_buf_stats_teardown(struct dma_buf *dmabuf); +void dma_buf_attach_stats_teardown(struct dma_buf_attachment *attach); +static inline unsigned int dma_buf_update_attach_uid(struct dma_buf *dmabuf) +{ + struct dma_buf_sysfs_entry *entry = dmabuf->sysfs_entry; + + return entry->attachment_uid++; +} +#else + +static inline int dma_buf_init_sysfs_statistics(void) +{ + return 0; +} + +static inline void dma_buf_uninit_sysfs_statistics(void) {} + +static inline int dma_buf_stats_setup(struct dma_buf *dmabuf) +{ + return 0; +} +static inline int dma_buf_attach_stats_setup(struct dma_buf_attachment *attach, + unsigned int uid) +{ + return 0; +} + +static inline void dma_buf_stats_teardown(struct dma_buf *dmabuf) {} +static inline void dma_buf_attach_stats_teardown(struct dma_buf_attachment *attach) {} +static inline void dma_buf_update_attachment_map_count(struct dma_buf_attachment *attach, + int delta) {} +static inline unsigned int dma_buf_update_attach_uid(struct dma_buf *dmabuf) +{ + return 0; +} +#endif +#endif // _DMA_BUF_SYSFS_STATS_H diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index f264b70c383e..184dd7acb1ed 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -29,6 +29,8 @@ #include <uapi/linux/dma-buf.h> #include <uapi/linux/magic.h> +#include "dma-buf-sysfs-stats.h" + static inline int is_dma_buf_file(struct file *); struct dma_buf_list { @@ -79,6 +81,7 @@ static void dma_buf_release(struct dentry *dentry) if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) dma_resv_fini(dmabuf->resv); + dma_buf_stats_teardown(dmabuf); module_put(dmabuf->owner); kfree(dmabuf->name); kfree(dmabuf); @@ -580,6 +583,10 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) file->f_mode |= FMODE_LSEEK; dmabuf->file = file; + ret = dma_buf_stats_setup(dmabuf); + if (ret) + goto err_sysfs; + mutex_init(&dmabuf->lock); INIT_LIST_HEAD(&dmabuf->attachments); @@ -589,6 +596,14 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) return dmabuf; +err_sysfs: + /* + * Set file->f_path.dentry->d_fsdata to NULL so that when + * dma_buf_release() gets invoked by dentry_ops, it exits + * early before calling the release() dma_buf op. + */ + file->f_path.dentry->d_fsdata = NULL; + fput(file); err_dmabuf: kfree(dmabuf); err_module: @@ -723,6 +738,7 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, { struct dma_buf_attachment *attach; int ret; + unsigned int attach_uid; if (WARN_ON(!dmabuf || !dev)) return ERR_PTR(-EINVAL); @@ -748,8 +764,13 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, } dma_resv_lock(dmabuf->resv, NULL); list_add(&attach->node, &dmabuf->attachments); + attach_uid = dma_buf_update_attach_uid(dmabuf); dma_resv_unlock(dmabuf->resv); + ret = dma_buf_attach_stats_setup(attach, attach_uid); + if (ret) + goto err_sysfs; + /* When either the importer or the exporter can't handle dynamic * mappings we cache the mapping here to avoid issues with the * reservation object lock. @@ -776,6 +797,7 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, dma_resv_unlock(attach->dmabuf->resv); attach->sgt = sgt; attach->dir = DMA_BIDIRECTIONAL; + dma_buf_update_attachment_map_count(attach, 1 /* delta */); } return attach; @@ -792,6 +814,7 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, if (dma_buf_is_dynamic(attach->dmabuf)) dma_resv_unlock(attach->dmabuf->resv); +err_sysfs: dma_buf_detach(dmabuf, attach); return ERR_PTR(ret); } @@ -841,6 +864,7 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) dma_resv_lock(attach->dmabuf->resv, NULL); __unmap_dma_buf(attach, attach->sgt, attach->dir); + dma_buf_update_attachment_map_count(attach, -1 /* delta */); if (dma_buf_is_dynamic(attach->dmabuf)) { dma_buf_unpin(attach); @@ -854,6 +878,7 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) if (dmabuf->ops->detach) dmabuf->ops->detach(dmabuf, attach); + dma_buf_attach_stats_teardown(attach); kfree(attach); } EXPORT_SYMBOL_GPL(dma_buf_detach); @@ -993,6 +1018,9 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, } #endif /* CONFIG_DMA_API_DEBUG */ + if (!IS_ERR(sg_table)) + dma_buf_update_attachment_map_count(attach, 1 /* delta */); + return sg_table; } EXPORT_SYMBOL_GPL(dma_buf_map_attachment); @@ -1030,6 +1058,8 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, if (dma_buf_is_dynamic(attach->dmabuf) && !IS_ENABLED(CONFIG_DMABUF_MOVE_NOTIFY)) dma_buf_unpin(attach); + + dma_buf_update_attachment_map_count(attach, -1 /* delta */); } EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment); @@ -1480,6 +1510,12 @@ static inline void dma_buf_uninit_debugfs(void) static int __init dma_buf_init(void) { + int ret; + + ret = dma_buf_init_sysfs_statistics(); + if (ret) + return ret; + dma_buf_mnt = kern_mount(&dma_buf_fs_type); if (IS_ERR(dma_buf_mnt)) return PTR_ERR(dma_buf_mnt); @@ -1495,5 +1531,6 @@ static void __exit dma_buf_deinit(void) { dma_buf_uninit_debugfs(); kern_unmount(dma_buf_mnt); + dma_buf_uninit_sysfs_statistics(); } __exitcall(dma_buf_deinit); diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index efdc56b9d95f..342585bd6dff 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -295,6 +295,9 @@ struct dma_buf_ops { * @poll: for userspace poll support * @cb_excl: for userspace poll support * @cb_shared: for userspace poll support + * @sysfs_entry: for exposing information about this buffer in sysfs. + * The attachment_uid member of @sysfs_entry is protected by dma_resv lock + * and is incremented on each attach. * * This represents a shared buffer, created by calling dma_buf_export(). The * userspace representation is a normal file descriptor, which can be created by @@ -330,6 +333,15 @@ struct dma_buf { __poll_t active; } cb_excl, cb_shared; +#ifdef CONFIG_DMABUF_SYSFS_STATS + /* for sysfs stats */ + struct dma_buf_sysfs_entry { + struct kobject kobj; + struct dma_buf *dmabuf; + unsigned int attachment_uid; + struct kset *attach_stats_kset; + } *sysfs_entry; +#endif }; /** @@ -379,6 +391,7 @@ struct dma_buf_attach_ops { * @importer_ops: importer operations for this attachment, if provided * dma_buf_map/unmap_attachment() must be called with the dma_resv lock held. * @importer_priv: importer specific attachment data. + * @sysfs_entry: For exposing information about this attachment in sysfs. * * This structure holds the attachment information between the dma_buf buffer * and its user device(s). The list contains one attachment struct per device @@ -399,6 +412,13 @@ struct dma_buf_attachment { const struct dma_buf_attach_ops *importer_ops; void *importer_priv; void *priv; +#ifdef CONFIG_DMABUF_SYSFS_STATS + /* for sysfs stats */ + struct dma_buf_attach_sysfs_entry { + struct kobject kobj; + unsigned int map_counter; + } *sysfs_entry; +#endif }; /** -- 2.32.0.rc1.229.g3e70b5a671-goog

3 years, 11 months

2
2
0 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig