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

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

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
>