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

["Koenig, Christian" <Christian.Koenig@amd.com>]

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 :(

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
>>>>