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

[sashiko-bot@kernel.org]

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
- [Medium] drm/xe: verbose pci_p2pdma_distance() check in xe_dma_buf_match_mapping()
--

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