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

[Alistair Popple <apopple@nvidia.com>]

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.

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