Re: [PATCH 13/26] drm/xe/dma-buf: Use the SGT mapping type

[Alistair Popple <apopple@nvidia.com>]

On 2026-10-26 at 19:34 +1100, sashiko-bot@kernel.org wrote...
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] drm/xe/dma-buf: xe_dma_buf_match_mapping() changes attach error to -EINVAL

I don't have access to Xe compatible hardware which is required to run
the kunit test, but this comment seems reasonable. Will fix by having
dma_buf_match_mapping() return -EOPNOTSUPP in the first patch.

> - [Medium] drm/xe: verbose pci_p2pdma_distance() check in xe_dma_buf_match_mapping()

This seems valid - fix is to make pci_p2pdma_distance() non-verbose when
introducing dma_buf_sgt_match() in "dma-buf: Add the SGT DMA mapping type".

Will fix this for v2.

 - Alistair

> --
> 
> commit d3c9a19b80db02efc0ef1143d9205eb0a3c2194e
> Author: Jason Gunthorpe <jgg@ziepe.ca>
> 
> drm/xe/dma-buf: Use the SGT mapping type
> 
> This commit converts the Xe dma-buf exporter to use the new SGT mapping type
> infrastructure by providing P2P and non-P2P SGT exporter matches in
> xe_dma_buf_match_mapping() and querying attachment mapping helpers during pin,
> map, and unmap.
> 
> > diff --git a/drivers/gpu/drm/xe/xe_dma_buf.c b/drivers/gpu/drm/xe/xe_dma_buf.c
> > index bf0728838ead5..3ca198da6ee87 100644
> > --- a/drivers/gpu/drm/xe/xe_dma_buf.c
> > +++ b/drivers/gpu/drm/xe/xe_dma_buf.c
> > @@ -27,13 +27,6 @@ static int xe_dma_buf_attach(struct dma_buf *dmabuf,
> >  {
> >  	struct drm_gem_object *obj = attach->dmabuf->priv;
> >  
> > -	if (attach->peer2peer &&
> > -	    pci_p2pdma_distance(to_pci_dev(obj->dev->dev), attach->dev, false) < 0)
> > -		attach->peer2peer = false;
> > -
> > -	if (!attach->peer2peer && !xe_bo_can_migrate(gem_to_xe_bo(obj), XE_PL_TT))
> > -		return -EOPNOTSUPP;
> > -
> >  	xe_pm_runtime_get(to_xe_device(obj->dev));
> >  	return 0;
> >  }
> 
> [ ... ]
> 
> > @@ -205,18 +205,38 @@ static void xe_dma_buf_release(struct dma_buf *dmabuf)
> >  	drm_gem_dmabuf_release(dmabuf);
> >  }
> >  
> > +static const struct dma_buf_mapping_sgt_exp_ops xe_dma_buf_sgt_ops = {
> > +	.map_dma_buf = xe_dma_buf_map,
> > +	.unmap_dma_buf = xe_dma_buf_unmap,
> > +};
> > +
> > +static int xe_dma_buf_match_mapping(struct dma_buf_match_args *args)
> > +{
> > +	struct drm_gem_object *obj = args->dmabuf->priv;
> > +	struct dma_buf_mapping_match sgt_match[2];
> > +	unsigned int num_match = 0;
> > +
> > +	sgt_match[num_match++] = DMA_BUF_EMAPPING_SGT_P2P(
> > +		&xe_dma_buf_sgt_ops, to_pci_dev(obj->dev->dev));
> 
> [Severity: Medium]
> Will offering DMA_BUF_EMAPPING_SGT_P2P() unconditionally first cause spurious
> pci_warn() and dev_warn() messages in dmesg when attaching migratable or
> system-memory-only BOs on systems without PCIe P2P support?
> 
> Previously, xe_dma_buf_attach() called pci_p2pdma_distance() with verbose set
> to false because P2P is optional when a BO resides in or can migrate to
> XE_PL_TT (similar to commit d0fa84f17477 ("drm/amdgpu: Disable verbose for p2p
> dist calc")).
> 
> With this change, dma_buf_match_mapping() evaluates sgt_match[0] first via
> dma_buf_sgt_match(), which calls pci_p2pdma_distance() with verbose set to
> true:
> 
> drivers/dma-buf/dma-buf-mapping.c:dma_buf_sgt_match() {
> 	if (exp->sgt_data.exporter_requires_p2p ==
> 	    DMA_SGT_EXPORTER_REQUIRES_P2P_DISTANCE) {
> 		if (!imp->sgt_data.importer_allows_p2p)
> 			return -EOPNOTSUPP;
> 		if (pci_p2pdma_distance(exp->sgt_data.exporting_p2p_device,
> 					imp->sgt_data.importing_dma_device,
> 					true) < 0)
> 			return -EOPNOTSUPP;
> 	}
> }
> 
> Whenever a P2P-capable importer attaches to an exported Xe dma-buf on a
> platform where P2P is unavailable (such as a non-PCI importer, non-whitelisted
> host bridge, or ACS redirect on PCIe switches), calc_map_type_and_dist() will
> log a warning even though sgt_match[1] (DMA_BUF_EMAPPING_SGT) immediately
> succeeds using system memory, or even when the BO was created as
> system-memory-only.
> 
> > +
> > +	if (xe_bo_can_migrate(gem_to_xe_bo(obj), XE_PL_TT))
> > +		sgt_match[num_match++] =
> > +			DMA_BUF_EMAPPING_SGT(&xe_dma_buf_sgt_ops);
> > +
> > +	return dma_buf_match_mapping(args, sgt_match, num_match);
> > +}
> 
> [Severity: Medium]
> Does replacing the check in xe_dma_buf_attach() with dma_buf_match_mapping()
> change the attach error code from -EOPNOTSUPP to -EINVAL and break the Xe
> dma-buf KUnit test suite?
> 
> When exporting a non-migratable VRAM-only BO (where xe_bo_can_migrate(bo,
> XE_PL_TT) is false) and attaching or importing it from an importer that does
> not support P2P (or where pci_p2pdma_distance() < 0), xe_dma_buf_attach()
> previously returned -EOPNOTSUPP.
> 
> Now, xe_dma_buf_match_mapping() only populates sgt_match[0] with
> DMA_BUF_EMAPPING_SGT_P2P() and calls dma_buf_match_mapping(). In
> drivers/dma-buf/dma-buf-mapping.c:dma_buf_match_mapping(), -EOPNOTSUPP from
> exp->type->match() is skipped, and -EINVAL is returned when no matches remain:
> 
> drivers/dma-buf/dma-buf-mapping.c:dma_buf_match_mapping() {
> 			if (exp->type->match) {
> 				ret = exp->type->match(args->dmabuf, exp, imp);
> 				if (ret == -EOPNOTSUPP)
> 					continue;
> 				if (ret != 0)
> 					return ret;
> 			}
> ...
> 	if (!nr)
> 		return -EINVAL;
> }
> 
> In drivers/gpu/drm/xe/tests/xe_dma_buf.c:xe_test_dmabuf_import_same_driver(),
> the KUnit test explicitly checks for -EOPNOTSUPP on import failure of a
> non-migratable VRAM BO without P2P:
> 
> 	} else if (PTR_ERR(import) != -EOPNOTSUPP) {
> 		/* Unexpected error code. */
> 		KUNIT_FAIL(test,
> 			   "xe_gem_prime_import failed with the wrong err=%ld\n",
> 			   PTR_ERR(import));
> 
> Should this path still return -EOPNOTSUPP when no mapping matches?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261026053623.202642-1-apopple@nvidia.com?part=13