On 2026-10-26 at 19:34 +1100, [email protected] 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 <[email protected]> > > 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/[email protected]?part=13
