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

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

Hi Daniel,

once more a ping on this. Any more comments or can we get it comitted?

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