Re: [PATCH v2 06/23] dma-buf: Let the exporter select mapping type at pin/map

["Christian König" <christian.koenig@amd.com>]

On 11/9/26 03:58, Alistair Popple wrote:
> On 2026-11-07 at 19:57 +1100, Karl Mehltretter <kmehltretter@gmail.com> wrote...
>> On Thu, Nov 05, 2026 at 05:40:41PM +0100, Alistair Popple wrote:
>>> + * A pin or map that fails after dma_buf_mapping_select() may reopen the
>>> + * choice only if it was the one that made it. A selection that predates the
>>> + * call belongs to a live pin or mapping and must survive, otherwise the next
>>> + * pin or map would reselect under it without an invalidate.
>>> + */
>>> +void dma_buf_mapping_unselect(struct dma_buf_attachment *attach,
>>> +			      bool was_selected)
>>> +{
>>> +	dma_resv_assert_held(attach->dmabuf->resv);
>>> +
>>> +	if (was_selected)
>>> +		return;
>>> +
>>> +	attach->selected = false;
>>> +	attach->selected_idx = 0;
>>> +}
>>> +
>>
>> After dma_buf_invalidate_mappings(), attach->selected is false while
>> attach->selected_idx still holds that mapping's entry for a pending
>> unmap. 
>>
>> If a pin/map fails before that unmap runs, was_selected is false and
>> dma_buf_mapping_unselect() resets attach->selected_idx to 0.
>>
>> A subsequent dma_buf_unmap_sgt() then uses matches[0].
> 
> Actually there's a bigger issue here. The contract at the moment states that
> mapping type may change after invalidation. However that doesn't account for the
> fact mappings can outlive the invalidation.
> 
> This could be dealt with by switching everything over to the generic
> dma_buf_mapping_map/unmap() path, but that requires an allocation per mapping
> just to track the mapping type.

You need an allocation per mapping anyway to identify the mapping anyway.

We currently abuse the sg_table for that, but essentially the idea of having a structure/object to represent the mapping needs to stay around as far as I can see.

Regards,
Christian.

> 
> The alternate is we update the contract to say mapping type cannot change until
> after all importers have unmapped. Enforcing this would require tracking the
> number of live mappings/pins per-attachment, but I think this is the better
> solution so will add that for the next version.
> 
>>>  int dma_buf_pin(struct dma_buf_attachment *attach)
>>>  {
>>>  	struct dma_buf *dmabuf = attach->dmabuf;
>>> +	bool selected = attach->selected;
>>>  	int ret = 0;
>>>  
>>>  	WARN_ON(!attach->importer_ops);
>>>  
>>>  	dma_resv_assert_held(dmabuf->resv);
>>>  
>>> -	if (dmabuf->ops->pin)
>>> +	ret = dma_buf_mapping_select(attach);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (dmabuf->ops->pin) {
>>>  		ret = dmabuf->ops->pin(attach);
>>> +		if (ret)
>>> +			dma_buf_mapping_unselect(attach, selected);
>>> +	}
>>
>> dma_buf_mapping_select() sets attach->selected = true before ops->pin()
>> and the map callbacks run.
>>
>> In patches 14/15, xe/amdgpu migrate a VRAM buffer to system memory
>> during pin and map when the attachment does not accept P2P.
>>
>> That migration calls dma_buf_invalidate_mappings() and clears
>> attach->selected on every attachment, also the one being pinned/mapped. 
>>
>> When the callback succeeds, the attachment stays pinned or mapped with
>> attach->selected == false.
>>
>> A later map/pin on the same attachment can then reselect without an
>> invalidation.
> 
> Argh. Thanks. Will fix.
> 
>  - Alistair
> 
>> Thanks,
>> Karl