Christian König
2022-Aug-24 17:45 UTC
[Linaro-mm-sig] [PATCH v3 6/9] dma-buf: Move dma-buf attachment to dynamic locking specification
Am 24.08.22 um 17:49 schrieb Dmitry Osipenko:> On 8/24/22 18:24, Christian K?nig wrote: >> Am 24.08.22 um 12:22 schrieb Dmitry Osipenko: >>> Move dma-buf attachment API functions to the dynamic locking >>> specification. >>> The strict locking convention prevents deadlock situations for dma-buf >>> importers and exporters. >>> >>> Previously, the "unlocked" versions of the attachment API functions >>> weren't taking the reservation lock and this patch makes them to take >>> the lock. >>> >>> Intel and AMD GPU drivers already were mapping the attached dma-bufs >>> under >>> the held lock during attachment, hence these drivers are updated to use >>> the locked functions. >>> >>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko at collabora.com> >>> --- >>> ? drivers/dma-buf/dma-buf.c????????????????? | 115 ++++++++++++++------- >>> ? drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c??? |?? 4 +- >>> ? drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c |?? 8 +- >>> ? drivers/gpu/drm/i915/gem/i915_gem_object.c |? 12 +++ >>> ? include/linux/dma-buf.h??????????????????? |? 20 ++-- >>> ? 5 files changed, 110 insertions(+), 49 deletions(-) >>> >>> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c >>> index 4556a12bd741..f2a5a122da4a 100644 >>> --- a/drivers/dma-buf/dma-buf.c >>> +++ b/drivers/dma-buf/dma-buf.c >>> @@ -559,7 +559,7 @@ static struct file *dma_buf_getfile(struct dma_buf >>> *dmabuf, int flags) >>> ?? * 2. Userspace passes this file-descriptors to all drivers it wants >>> this buffer >>> ?? *??? to share with: First the file descriptor is converted to a >>> &dma_buf using >>> ?? *??? dma_buf_get(). Then the buffer is attached to the device using >>> - *??? dma_buf_attach(). >>> + *??? dma_buf_attach_unlocked(). >> Now I get why this is confusing me so much. >> >> The _unlocked postfix implies that there is another function which >> should be called with the locks already held, but this is not the case >> for attach/detach (because they always need to grab the lock themselves). > That's correct. The attach/detach ops of exporter can take the lock > (like i915 exporter does it), hence importer must not grab the lock > around dma_buf_attach() invocation. > >> So I suggest to drop the _unlocked postfix for the attach/detach >> functions. Another step would then be to unify attach/detach with >> dynamic_attach/dynamic_detach when both have the same locking convention >> anyway. > It's not a problem to change the name, but it's unclear to me why we > should do it. The _unlocked postfix tells importer that reservation must > be unlocked and it must be unlocked in case of dma_buf_attach(). > > Dropping the postfix will make dma_buf_attach() inconsistent with the > rest of the _unlocked functions(?). Are you sure we need to rename it?The idea of the postfix was to distinguish between two different versions of the same function, e.g. dma_buf_vmap_unlocked() vs normal dma_buf_vmap(). When we don't have those two types of the same function I don't think it makes to much sense to keep that. We should just properly document which functions expect what and that's what your documentation patch does. Regards, Christian.> >> Sorry that this is going so much back and forth, it's really complicated >> to keep all the stuff in my head at the moment :) > Not a problem at all, I expected that it will take some time for this > patchset to settle down. >