[PATCH v6 14/22] dma-buf: Introduce new locking convention
dmitry.osipenko at collabora.com
Mon May 30 13:57:22 UTC 2022
On 5/30/22 16:41, Christian König wrote:
> Hi Dmitry,
> Am 30.05.22 um 15:26 schrieb Dmitry Osipenko:
>> Hello Christian,
>> On 5/30/22 09:50, Christian König wrote:
>>> Hi Dmitry,
>>> First of all please separate out this patch from the rest of the series,
>>> since this is a complex separate structural change.
>> I assume all the patches will go via the DRM tree in the end since the
>> rest of the DRM patches in this series depend on this dma-buf change.
>> But I see that separation may ease reviewing of the dma-buf changes, so
>> let's try it.
> That sounds like you are underestimating a bit how much trouble this
> will be.
>>> I have tried this before and failed because catching all the locks in
>>> the right code paths are very tricky. So expect some fallout from this
>>> and make sure the kernel test robot and CI systems are clean.
>> Sure, I'll fix up all the reported things in the next iteration.
>> BTW, have you ever posted yours version of the patch? Will be great if
>> we could compare the changed code paths.
> No, I never even finished creating it after realizing how much work it
> would be.
>>>> This patch introduces new locking convention for dma-buf users. From
>>>> on all dma-buf importers are responsible for holding dma-buf
>>>> lock around operations performed over dma-bufs.
>>>> This patch implements the new dma-buf locking convention by:
>>>> 1. Making dma-buf API functions to take the reservation lock.
>>>> 2. Adding new locked variants of the dma-buf API functions for
>>>> that need to manage imported dma-bufs under the held lock.
>>> Instead of adding new locked variants please mark all variants which
>>> expect to be called without a lock with an _unlocked postfix.
>>> This should make it easier to remove those in a follow up patch set and
>>> then fully move the locking into the importer.
>> Do we really want to move all the locks to the importers? Seems the
>> majority of drivers should be happy with the dma-buf helpers handling
>> the locking for them.
> Yes, I clearly think so.
>>>> 3. Converting all drivers to the new locking scheme.
>>> I have strong doubts that you got all of them. At least radeon and
>>> nouveau should grab the reservation lock in their ->attach callbacks
>> Radeon and Nouveau use gem_prime_import_sg_table() and they take resv
>> lock already, seems they should be okay (?)
> You are looking at the wrong side. You need to fix the export code path,
> not the import ones.
> See for example attach on radeon works like this
Yeah, I was looking at the both sides, but missed this one.
> Same for nouveau and probably a few other exporters as well. That will
> certainly cause a deadlock if you don't fix it.
> I strongly suggest to do this step by step, first attach/detach and then
> the rest.
Thank you very much for the suggestions. I'll implement them in the next
More information about the dri-devel