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 <[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
