Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/tegra: use_direct_dma not set in host1x_client_iommu_attach() - [High] drm/tegra: missing chunk count in tegra_bo_pin() for shmem BOs --
commit 12a809963b5e12b284858a4f0ff8bc1a58e8b048 Author: Mikko Perttunen <[email protected]> drm/tegra: Use shmem for DMA IOMMU buffer allocations This commit records whether host1x clients are attached to a DMA IOMMU domain during initialization and uses drm_gem_get_pages() via tegra_bo_get_pages() for buffer allocations when DMA IOMMU is active. In mixed IOMMU and direct DMA configurations, it attempts to fall back to contiguous DMA allocations. > diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c > index 324480c87fb1f..1013bbd662f84 100644 > --- a/drivers/gpu/drm/tegra/drm.c > +++ b/drivers/gpu/drm/tegra/drm.c > @@ -958,14 +958,10 @@ int host1x_client_iommu_attach(struct host1x_client > *client) > } > #endif > > - /* > - * If the host1x client is already attached to an IOMMU domain that is > - * not the shared IOMMU domain, don't try to attach it to a different > - * domain. This allows using the IOMMU-backed DMA API. > - */ > - if (domain && domain->type != IOMMU_DOMAIN_IDENTITY && > - domain != tegra->domain) > + if (domain && iommu_is_dma_domain(domain)) { > + tegra->use_dma_iommu = true; > return 0; > + } > > if (tegra->domain) { > group = iommu_group_get(client->dev); > @@ -981,6 +977,8 @@ int host1x_client_iommu_attach(struct host1x_client > *client) > } > > tegra->use_explicit_iommu = true; > + } else { > + tegra->use_direct_dma = true; > } [Severity: High] Can tegra->use_direct_dma remain unset when a direct-DMA client is initialized while tegra->domain is speculatively allocated? In host1x_drm_probe(), tegra->domain is allocated before host1x_device_init() runs whenever the parent host1x device is IOMMU-mapped, and is only freed after host1x_device_init() completes: drivers/gpu/drm/tegra/drm.c:host1x_drm_probe() { if (host1x_drm_wants_iommu(dev) && device_iommu_mapped(dma_dev)) { tegra->domain = iommu_paging_domain_alloc(dma_dev); ... } ... err = host1x_device_init(dev); ... if (tegra->use_explicit_iommu) { ... } else if (tegra->domain) { iommu_domain_free(tegra->domain); tegra->domain = NULL; iova_cache_put(); } } When a client without an IOMMU group calls host1x_client_iommu_attach() during host1x_device_init(), tegra->domain is non-NULL, so execution enters the if (tegra->domain) branch instead of the else branch: drivers/gpu/drm/tegra/drm.c:host1x_client_iommu_attach() { if (tegra->domain) { group = iommu_group_get(client->dev); if (!group) return -ENODEV; } Client initialization callbacks such as tegra_dc_init() ignore -ENODEV and continue operating in direct-DMA mode: drivers/gpu/drm/tegra/dc.c:tegra_dc_init() { err = host1x_client_iommu_attach(client); if (err < 0 && err != -ENODEV) { dev_err(client->dev, "failed to attach to domain: %d\n", err); return err; } } Because host1x_client_iommu_attach() returned -ENODEV early without setting tegra->use_direct_dma = true, the mixed-configuration check in host1x_drm_probe() (if (tegra->use_dma_iommu && tegra->use_direct_dma)) will not trigger, leaving tegra->use_dma_iommu enabled and allocating discontiguous shmem buffers that direct-DMA clients cannot use. Should tegra->use_direct_dma = true also be set before returning -ENODEV when !group? [ ... ] > diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c > index f76af733ea793..bb8d9f9e64534 100644 > --- a/drivers/gpu/drm/tegra/gem.c > +++ b/drivers/gpu/drm/tegra/gem.c > @@ -111,10 +111,6 @@ static struct host1x_bo_mapping *tegra_bo_pin(struct > device *dev, struct host1x_ > } > > if (obj->pages) { > - /* > - * If the buffer object was allocated from the explicit IOMMU > - * API code paths, construct an SG table from the pages. > - */ > err = sg_alloc_table_from_pages(map->sgt, obj->pages, > obj->num_pages, 0, gem->size, > GFP_KERNEL); > if (err < 0) [Severity: High] With shmem-backed buffers now taking the obj->pages path in tegra_bo_pin() when tegra->use_dma_iommu is enabled (where obj->mm is NULL), does map->chunks get set to 0 instead of the mapped scatterlist chunk count? Looking at tegra_bo_pin() in drivers/gpu/drm/tegra/gem.c: if (obj->pages) { err = sg_alloc_table_from_pages(map->sgt, obj->pages, obj->num_pages, 0, gem->size, GFP_KERNEL); if (err < 0) goto free; } else { ... } err = dma_map_sgtable(dev, map->sgt, direction, 0); if (err) goto free_sgt; out: if (!obj->mm) { map->phys = sg_dma_address(map->sgt->sgl); map->chunks = err; } else { map->phys = obj->iova; map->chunks = 1; } Unlike the obj->dma_buf path above it which sets err = sgt_dma_count_chunks(map->sgt), the non-imported path leaves err as 0 (the return value of dma_map_sgtable()). When !obj->mm is true, map->chunks is assigned 0, which bypasses the if (map->chunks > 1) contiguity checks in tegra_dc_pin() and host1x's pin_job() if the DMA mapping is not coalesced into a single contiguous chunk. Should err = sgt_dma_count_chunks(map->sgt) be called after dma_map_sgtable() succeeds? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
