Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] drm/msm/gem: dma_map_sgtable() in get_pages() uses iommu-dma on MDP IOMMUs --
commit e379fa230053c8f948229c260c41caa88a3e753f Author: Rob Clark <[email protected]> drm/msm/gem: dma_map/unmap_sgtable() This commit inlines sync_for_device() and sync_for_cpu() into get_pages() and put_pages(), calling dma_map_sgtable() and dma_unmap_sgtable() on all GEM objects using DMA_ATTR_SKIP_CPU_SYNC for cached buffers to populate the scatterlist DMA addresses and lengths. > diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c > index 181b9ad326729..bcdb57866cf6c 100644 > --- a/drivers/gpu/drm/msm/msm_gem.c > +++ b/drivers/gpu/drm/msm/msm_gem.c [ ... ] > @@ -219,13 +192,20 @@ static struct page **get_pages(struct drm_gem_object > *obj) > goto error; > } > > - update_device_mem(dev->dev_private, obj->size); > - > - /* For non-cached buffers, ensure the new pages are clean > - * because display controller, GPU, etc. are not coherent: > - */ > + /* For non-cached buffers, ensure the new pages are clean. */ > if (msm_obj->flags & MSM_BO_WC) > - sync_for_device(msm_obj); > + map_attrs = 0; > + > + ret = dma_map_sgtable(drm_dev_dma_dev(obj->dev), > + msm_obj->sgt, > + DMA_BIDIRECTIONAL, > + map_attrs); > + if (ret) { > + DRM_DEV_ERROR(dev->dev, "could not map sgtable: %d\n", > ret); > + goto error; > + } [Severity: High] As reported by Sashiko on v3 [1], will calling dma_map_sgtable() on drm_dev_dma_dev(obj->dev) in get_pages() invoke iommu_dma_map_sg() instead of dma_direct_map_sg() on platforms where the display controller device has an IOMMU attached directly to the MDP node? In msm_kms_init_vm(), the IOMMU can be attached directly to mdp_dev (which is obj->dev->dev, returned by drm_dev_dma_dev(obj->dev)): drivers/gpu/drm/msm/msm_kms.c:msm_kms_init_vm() { if (device_iommu_mapped(mdp_dev)) iommu_dev = mdp_dev; else if (mdss_dev && device_iommu_mapped(mdss_dev)) iommu_dev = mdss_dev; } This occurs on qcom_iommu platforms (such as MSM8916, MSM8939, MSM8953, and MSM8976) where qcom_iommu_ops does not implement def_domain_type, as well as on arm-smmu-qcom MDP5 platforms (such as MSM8996, SDM630, and SDM660) where the iommus property is on the qcom,mdp5 node rather than the parent mdss node and is not matched by qcom_smmu_client_of_match[]: drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c: static const struct of_device_id qcom_smmu_client_of_match[] __maybe_unused = { { .compatible = "qcom,adreno" }, { .compatible = "qcom,adreno-gmu" }, { .compatible = "qcom,glymur-mdss" }, { .compatible = "qcom,mdp4" }, { .compatible = "qcom,mdss" }, ... On these platforms, iommu_setup_dma_ops() allocates a DMA default_domain and sets use_dma_iommu(dev) to true on obj->dev->dev. When msm_iommu_new() later attaches an unmanaged domain via iommu_attach_device(), use_dma_iommu(dev) and group->default_domain remain set. Even though get_dma_ops(dev) is NULL, dma_go_direct() checks use_dma_iommu(dev) before checking ops: kernel/dma/mapping.c:dma_go_direct() { if (use_dma_iommu(dev)) return false; if (likely(!ops)) return true; } As a result, get_pages() -> dma_map_sgtable() -> __dma_map_sg_attrs() routes into iommu_dma_map_sg() for every GEM object (now including MSM_BO_CACHED buffers). That maps the buffer into the unused group->default_domain, coalesces sgt->nents, and populates sg_dma_address(sg) with default_domain IOVAs rather than CPU physical addresses. When paired with commit 2ee69d02162f ("drm/msm: Convert map helper to use dma-address") later in this series, where __do_map() in drivers/gpu/drm/msm/msm_iommu.c passes sg_dma_address(sg) as the physical address to iommu_map(), won't this program the GPU and display page tables with default_domain IOVAs instead of physical addresses? [1] https://lore.kernel.org/r/[email protected] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
