[PATCH 1/6] dma-buf: add dynamic DMA-buf handling v8

[Christian König]

Am 22.05.19 um 10:19 schrieb Daniel Vetter:
> On Wed, May 15, 2019 at 04:38:26PM +0200, Christian König wrote:
> [SNAP]
> Just this functional comment, since I think api detail polishing is
> premature if we're not yet aware of how this works.
>
>> +	/* When the importer is dynamic but the exporter isn't we need to cache
>> +	 * the mapping or otherwise would run into issues with the reservation
>> +	 * object lock.
>> +	 */
>> +	if (dma_buf_attachment_is_dynamic(attach) &&
>> +	    !dma_buf_is_dynamic(dmabuf)) {
> Isn't this the wrong way round? dynamic importers should be perfectly fine
> with the reservation locks in their map/unmap paths, it's importers
> calling exporters there.
>
> The real problem is a not-dynamic importer, which hasn't be adjusted to
> allow the reservation lock in their paths where they map/unmap a buffer,
> with a dynamic exporter. That's where we need to cache the mapping to
> avoid the deadlock (or having to change everyone)

Well could be that this is also a problem, but I actually don't think so.

The case I'm describing here certainly is the more obvious problem 
because the importer is already holding the lock the exporter wants to take.

On the other hand we could rather easily change that check to 
dma_buf_attachment_is_dynamic() != dma_buf_is_dynamic() if that is 
indeed a problem.

>> +		struct sg_table *sgt;
>> +
>> +		sgt = dmabuf->ops->map_dma_buf(attach, DMA_BIDIRECTIONAL);
> And unfortunately the non-dynamic, i.e. legacy/current code importer is
> also the one which uses other flags than DMA_BIDIRECTIONAL. At least on
> ARM, and that's the only place where this matters because there the dma
> api might do cache flushing.

Well the only implementer for now is amdgpu, and amdgpu always requires 
a coherent bidirectional mapping.

So this won't be a problem unless the ARM drivers start to implement 
dynamic DMA-buf handling themselves or start to talk to amdgpu (which 
wouldn't have worked before anyway).

Christian.

> Cheers, Daniel