[PATCH 1/4] dma-buf: add optional invalidate_mappings callback

[Christian König]

Am 12.03.2018 um 18:07 schrieb Daniel Vetter:
> On Fri, Mar 09, 2018 at 08:11:41PM +0100, Christian K??nig wrote:
>> [SNIP]
>>   
>> +/**
>> + * dma_buf_invalidate_mappings - invalidate all mappings of this dma_buf
>> + *
>> + * @dmabuf:	[in]	buffer which mappings should be invalidated
>> + *
>> + * Informs all attachmenst that they need to destroy and recreated all their
>> + * mappings.
>> + */
>> +void dma_buf_invalidate_mappings(struct dma_buf *dmabuf)
>> +{
>> +	struct dma_buf_attachment *attach;
>> +
>> +	reservation_object_assert_held(dmabuf->resv);
>> +
>> +	list_for_each_entry(attach, &dmabuf->attachments, node)
>> +		attach->invalidate_mappings(attach);
> To make the locking work I think we also need to require importers to hold
> the reservation object while attaching/detaching. Otherwise the list walk
> above could go boom.

Oh, good point. Going, to fix this.

> [SNIP]
>> +	/**
>> +	 * @supports_mapping_invalidation:
>> +	 *
>> +	 * True for exporters which supports unpinned DMA-buf operation using
>> +	 * the reservation lock.
>> +	 *
>> +	 * When attachment->invalidate_mappings is set the @map_dma_buf and
>> +	 * @unmap_dma_buf callbacks can be called with the reservation lock
>> +	 * held.
>> +	 */
>> +	bool supports_mapping_invalidation;
> Why do we need this? Importer could simply always register with the
> invalidate_mapping hook registered, and exporters could use it when they
> see fit. That gives us more lockdep coverage to make sure importers use
> their attachment callbacks correctly (aka they hold the reservation
> object).

One sole reason: Backward compability.

I didn't wanted to audit all those different drivers if they can handle 
being called with the reservation lock held.

>
>> +
>>   	/**
>>   	 * @map_dma_buf:
>>   	 *
>> @@ -326,6 +338,29 @@ struct dma_buf_attachment {
>>   	struct device *dev;
>>   	struct list_head node;
>>   	void *priv;
>> +
>> +	/**
>> +	 * @invalidate_mappings:
>> +	 *
>> +	 * Optional callback provided by the importer of the attachment which
>> +	 * must be set before mappings are created.
> This doesn't work, it must be set before the attachment is created,
> otherwise you race with your invalidate callback.

Another good point.

>
> I think the simplest option would be to add a new dma_buf_attach_dynamic
> (well except a less crappy name).

Well how about adding an optional invalidate_mappings parameter to the 
existing dma_buf_attach?

>
>> +	 *
>> +	 * If provided the exporter can avoid pinning the backing store while
>> +	 * mappings exists.
>> +	 *
>> +	 * The function is called with the lock of the reservation object
>> +	 * associated with the dma_buf held and the mapping function must be
>> +	 * called with this lock held as well. This makes sure that no mapping
>> +	 * is created concurrently with an ongoing invalidation.
>> +	 *
>> +	 * After the callback all existing mappings are still valid until all
>> +	 * fences in the dma_bufs reservation object are signaled, but should be
>> +	 * destroyed by the importer as soon as possible.
> Do we guarantee that the importer will attach a fence, after which the
> mapping will be gone? What about re-trying? Or just best effort (i.e. only
> useful for evicting to try to make room).

The importer should attach fences for all it's operations with the DMA-buf.

> I think a helper which both unmaps _and_ waits for all the fences to clear
> would be best, with some guarantees that it'll either fail or all the
> mappings _will_ be gone. The locking for that one will be hilarious, since
> we need to figure out dmabuf->lock vs. the reservation. I kinda prefer we
> throw away the dmabuf->lock and superseed it entirely by the reservation
> lock.

Big NAK on that. The whole API is asynchronously, e.g. we never block 
for any operation to finish.

Otherwise you run into big trouble with cross device GPU resets and 
stuff like that.

>> +	 *
>> +	 * New mappings can be created immediately, but can't be used before the
>> +	 * exclusive fence in the dma_bufs reservation object is signaled.
>> +	 */
>> +	void (*invalidate_mappings)(struct dma_buf_attachment *attach);
> Bunch of questions about exact semantics, but I very much like this. And I
> think besides those technical details, the overall approach seems sound.

Yeah this initial implementation was buggy like hell. Just wanted to 
confirm that the idea is going in the right direction.

Thanks for the comments,
Christian.

> -Daniel
>