[Intel-gfx] [PATCH 1/4] dma-buf: change DMA-buf locking convention

[Koenig, Christian]

Am 16.10.19 um 16:23 schrieb Daniel Vetter:
> On Wed, Oct 16, 2019 at 3:46 PM Koenig, Christian
> <Christian.Koenig at amd.com> wrote:
>> Am 08.10.19 um 10:55 schrieb Daniel Vetter:
>>> On Wed, Oct 02, 2019 at 08:37:50AM +0000, Koenig, Christian wrote:
>>>> Hi Daniel,
>>>>
>>>> once more a ping on this. Any more comments or can we get it comitted?
>>> Sorry got a bit smashed past weeks, but should be resurrected now back
>>> from xdc.
>> And any more thoughts on this? I mean we are blocked for month on this
>> now :(
> I replied to both 1 and 2 in this series on 8th Oct. You even replied
> to me in the thread on patch 2 ... but not here (I top posted since
> this detour here just me being confused).

Ok, in this case its my fault. I totally missed your reply on 1 and 
thought that the reply on 2 was actually for a different thread.

I'm going to submit the TTM changes separately, cause that is actually a 
bug fix for a completely different issue which just happens to surface 
because we change the locking.

Thanks,
Christian.

> -Daniel
>
>> Thanks,
>> Christian.
>>
>>> -Daniel
>>>> Thanks,
>>>> Christian.
>>>>
>>>> Am 24.09.19 um 11:50 schrieb Christian König:
>>>>> Am 17.09.19 um 16:56 schrieb Daniel Vetter:
>>>>>> [SNIP]
>>>>>>>>>>>>>>>>         +    /* 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.
>>>>>>>>>>>>>>>> +     */
>>>>>>>>>>>>>>>> +    if (dma_buf_attachment_is_dynamic(attach) !=
>>>>>>>>>>>>>>>> +        dma_buf_is_dynamic(dmabuf)) {
>>>>>>>>>>>>>>>> +        struct sg_table *sgt;
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> +        if (dma_buf_is_dynamic(attach->dmabuf))
>>>>>>>>>>>>>>>> + dma_resv_lock(attach->dmabuf->resv, NULL);
>>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>>> +        sgt = dmabuf->ops->map_dma_buf(attach,
>>>>>>>>>>>>>>>> DMA_BIDIRECTIONAL);
>>>>>>>>>>>>>>> Now we're back to enforcing DMA_BIDI, which works nicely
>>>>>>>>>>>>>>> around the
>>>>>>>>>>>>>>> locking pain, but apparently upsets the arm-soc folks who
>>>>>>>>>>>>>>> want to
>>>>>>>>>>>>>>> control
>>>>>>>>>>>>>>> this better.
>>>>>>>>>>>>>> Take another look at dma_buf_map_attachment(), we still try
>>>>>>>>>>>>>> to get the
>>>>>>>>>>>>>> caching there for ARM.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> What we do here is to bidirectionally map the buffer to avoid
>>>>>>>>>>>>>> the
>>>>>>>>>>>>>> locking hydra when importer and exporter disagree on locking.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> So the ARM folks can easily avoid that by switching to
>>>>>>>>>>>>>> dynamic locking
>>>>>>>>>>>>>> for both.
>>>>>>>>>>>> So you still break the contract between importer and exporter,
>>>>>>>>>>>> except not
>>>>>>>>>>>> for anything that's run in intel-gfx-ci so all is good?
>>>>>>>>>>> No, the contract between importer and exporter stays exactly the
>>>>>>>>>>> same it
>>>>>>>>>>> is currently as long as you don't switch to dynamic dma-buf
>>>>>>>>>>> handling.
>>>>>>>>>>>
>>>>>>>>>>> There is no functional change for the ARM folks here. The only
>>>>>>>>>>> change
>>>>>>>>>>> which takes effect is between i915 and amdgpu and that is perfectly
>>>>>>>>>>> covered by intel-gfx-ci.
>>>>>>>>>> There's people who want to run amdgpu on ARM?
>>>>>>>>> Sure there are, we even recently fixed some bugs for this.
>>>>>>>>>
>>>>>>>>> But as far as I know there is no one currently which is affect by
>>>>>>>>> this
>>>>>>>>> change on ARM with amdgpu.
>>>>>>>> But don't you break them with this now?
>>>>>>> No, we see the bidirectional attachment as compatible with the other
>>>>>>> ones.
>>>>>>>
>>>>>>>> amdgpu will soon set the dynamic flag on exports, which forces the
>>>>>>>> caching
>>>>>>>> at create time (to avoid the locking fun), which will then result in a
>>>>>>>> EBUSY at map_attachment time because we have a cached mapping, but
>>>>>>>> it's
>>>>>>>> the wrong type.
>>>>>>> See the check in dma_buf_map_attachment():
>>>>>>>
>>>>>>>         if (attach->dir != direction && attach->dir != DMA_BIDIRECTIONAL)
>>>>>>>             return ERR_PTR(-EBUSY);
>>>>>> Hm, I misread this. So yeah should work, +/- the issue that we might
>>>>>> not flush enough. But I guess that can be fixed whenever, it's not
>>>>>> like dma-api semantics are a great fit for us. Maybe a fixme comment
>>>>>> would be useful here ... I'll look at this tomorrow or so because atm
>>>>>> brain is slow, I'm down with the usual post-conference cold it seems
>>>>>> :-/
>>>>> Hope your are feeling better now, adding a comment is of course not a
>>>>> problem.
>>>>>
>>>>> With that fixed can I get an reviewed-by or at least and acked-by?
>>>>>
>>>>> I want to land at least some parts of those changes now.
>>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>> -Daniel
>>>>>>
>