Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] iommu/arm-smmu-qcom: qcom_adreno_smmu_alloc_context_bank() shifts GMU to cb 2 - [Medium] iommu/arm-smmu-qcom: undocumented qcom,lpac DT compatible breaks DT ABI --
commit 89b03a5b08cd9912b4e1abfb59372ce6c00d8186 Author: Anna Maniscalco <[email protected]> iommu: arm-smmu-qcom: Configure lpac device with split address space This commit configures the LPAC device to use a split address space by matching the "qcom,lpac" compatible string, recognizing stream ID 1 in qcom_adreno_smmu_is_gpu_device(), and reserving context banks 0 and 1 for GPU and LPAC devices. > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > index edd41b5a3b6ac..48a590aaeb5eb 100644 > --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c > @@ -35,6 +35,8 @@ > static const struct of_device_id qcom_smmu_actlr_client_of_match[] = { > { .compatible = "qcom,adreno", > .data = (const void *) (PREFETCH_DEEP | CPRE | CMTLB) }, > + { .compatible = "qcom,lpac", > + .data = (const void *) (PREFETCH_DEEP | CPRE | CMTLB) }, [Severity: Medium] Does adding "qcom,lpac" here and in qcom_smmu_client_of_match[] introduce an undocumented Device Tree compatible string and break Device Tree backwards compatibility? Neither this commit nor the follow-up commits in this series document "qcom,lpac" in Documentation/devicetree/bindings/, and Documentation/devicetree/bindings/display/msm/gpu.yaml sets additionalProperties: false: Documentation/devicetree/bindings/display/msm/gpu.yaml: required: - compatible - reg - interrupts additionalProperties: false which causes dtbs_check validation to fail when an lpac child node is added under the GPU node in "arm64: dts: qcom: sm8650: move smmu sid 1 to new lpac device". Furthermore, existing upstream Device Trees (such as sm8350.dtsi, sc8280xp.dtsi, sm8450.dtsi, sm8550.dtsi, sm8650.dtsi, hamoa.dtsi, kodiak.dtsi, lemans.dtsi, and monaco.dtsi) already assign Stream ID 1 directly to the main "qcom,adreno" GPU node: arch/arm64/boot/dts/qcom/sm8650.dtsi: iommus = <&adreno_smmu 0 0x0>, <&adreno_smmu 1 0x0>; When booting with an existing sm8650 DTB that lacks the new "qcom,lpac" child node, gpu->lpac_pdev is NULL and context bank 1 is never allocated, yet the later commits "firmware: qcom: scm: Configure LPAC aperture", "drm/msm: initialize LPAC ring", and "drm/msm: enable LPAC on gen70900" gate LPAC hardware initialization on ADRENO_QUIRK_LPAC rather than gpu->lpac_pdev. [ ... ] > @@ -305,9 +309,9 @@ static int qcom_adreno_smmu_alloc_context_bank(struct > arm_smmu_domain *smmu_doma > */ > if (qcom_adreno_smmu_is_gpu_device(dev)) { > start = 0; > - count = 1; > + count = 2; > } else { > - start = 1; > + start = 2; > count = smmu->num_context_banks; > } [Severity: High] Won't unconditionally changing start from 1 to 2 in the else branch of qcom_adreno_smmu_alloc_context_bank() shift the Adreno GMU ("qcom,adreno-gmu", SID 5) from context bank 1 to context bank 2 across all Adreno platforms, and break non-GPU context bank allocation on qcom,sdm630-smmu-v2? During Adreno GMU initialization on Adreno 6xx/7xx/8xx SoCs, the call chain: a6xx_gpu_init() -> a6xx_gmu_init() -> a6xx_gmu_memory_probe() -> msm_iommu_new(gmu->dev, NULL, 0) -> iommu_attach_device() -> arm_smmu_attach_dev() -> arm_smmu_init_domain_context() -> qcom_adreno_smmu_alloc_context_bank() evaluates gmu->dev (SID 5), which takes the else branch (both here and after the later commit "iommu: arm-smmu-qcom: Fixed mapping between sid and cb for gpu and lpac") and allocates cbndx 2 instead of cbndx 1, leaving cbndx 1 unused on all SoCs or DTBs without a separate "qcom,lpac" device. As noted in commit afc95681c30689 ("iommu/arm-smmu-qcom: Add missing GMU entry to match table"), firmware on platforms such as sc8280xp expects context bank 1 to be assigned to the GMU; assigning context bank 2 causes the SMMU to wedge and the GPU to hang without memory access when a GPU translation fault occurs. Additionally, on qcom,sdm630-smmu-v2, qcom_adreno_smmuv2_cfg_probe() limits smmu->num_context_banks to 2: drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c:qcom_adreno_smmuv2_cfg_probe() { /* TZ protects several last context banks, hide them from Linux */ if (of_device_is_compatible(smmu->dev->of_node, "qcom,sdm630-smmu-v2") && smmu->num_context_banks == 5) smmu->num_context_banks = 2; } With start = 2 and count = 2, __arm_smmu_alloc_bitmap(smmu->context_map, 2, 2) has an empty allocation range and will unconditionally return -ENOSPC for any non-GPU device. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20261029-descriptive-name-lpac-upstream-v2-0-a21a5c850...@gmail.com?part=1
