[Intel-gfx] [PATCH v6 14/22] dma-buf: Introduce new locking convention

[Dmitry Osipenko]

On 7/1/22 13:43, Dmitry Osipenko wrote:
> On 6/29/22 00:26, Thomas Hellström (Intel) wrote:
>> On 5/30/22 15:57, Dmitry Osipenko wrote:
>>> 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
>>>>>>> now
>>>>>>> on all dma-buf importers are responsible for holding dma-buf
>>>>>>> reservation
>>>>>>> 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
>>>>>>> drivers
>>>>>>>         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
>>>>>> somehow.
>>>>> 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
>>>> drm_gem_map_attach->drm_gem_pin->radeon_gem_prime_pin->radeon_bo_reserve->ttm_bo_reserve->dma_resv_lock.
>>>>
>>> Yeah, I was looking at the both sides, but missed this one.
>> Also i915 will run into trouble with attach. In particular since i915
>> starts a full ww transaction in its attach callback to be able to lock
>> other objects if migration is needed. I think i915 CI would catch this
>> in a selftest.
> Seems it indeed it should deadlock. But i915 selftests apparently
> should've caught it and they didn't, I'll re-check what happened.
> 

The i915 selftests use a separate mock_dmabuf_ops. That's why it works
for the selftests, i.e. there is no deadlock.

Thomas, would i915 CI run a different set of tests or will it be the
default i915 selftests ran by IGT?

-- 
Best regards,
Dmitry