From: Thierry Reding treding@nvidia.com
Hi,
the GPU on Jetson TX2 (GP10B) does not work properly on all devices. Why exactly is not clear, but there are slight differences between the SKUs that were tested. It turns out that the biggest issue is that on some devices (e.g. the one that I have), pulsing the GPU reset twice as is done in the current code (once as part of the power-ungate operation and then again in the driver) causes the GPU to go into a bad state on some devices. Conditionally doing the reset in the driver only if it isn't already done by the power domain code fixes this issue.
Another issue is that the clock may be running at a rate of 0 Hz. This is unlikely to happen because it internally actually can't run that slow, but explicitly setting the clock rate at probe time does seem to help in some cases.
Patch three in this series unifies reading the WPR configuration by getting it from GPU register rather than reaching into the memory controller's register space. This is slightly better because it better separates the two drivers and doesn't require an update everytime the memory controller moves to another register aperture.
Patch 4 ensures the L2 cache makes memory requests with the proper stream ID, which is required when the GPU is behind an IOMMU.
Patch 5 changes the GP10B device initialization to use the correct copy engine. GP10B is a Pascal generation GPU and the way that engines are described changes how the copy engines are enumerated compared to earlier generations.
Patches 6 through 9 allow Nouveau to work on Tegra GPUs if the DMA API is backed by an IOMMU. This is different from current assumptions because mappings for all buffers mapped through the DMA API will need to have the special IOMMU bit set in their page tables. Note that this technically makes it possible to support big pages on Tegra because from the GPU's point of view all memory is now contiguous. However, these patches only make sure that buffers are mapped properly and don't try to enable big pages. Also note that mapping through the IOMMU comes at a slight cost, so this may not always be desirable. However, with Tegra186 and later it's currently not possible (from a DMA API point of view) to map only a subset of buffers through the IOMMU, so any such optimization is deferred. Furthermore, the ARM SMMU driver currently enforces the use of the SMMU by default, so there not much of a choice at the moment.
Finally patches 10 and 11 enable the GPU on Jetson TX2 and make it use the SMMU. I can pick up patches 10 and 11 into the Tegra tree once the other patches have been merged into Nouveau.
Thierry
Alexandre Courbot (1): arm64: tegra: Enable GPU on Jetson TX2
Thierry Reding (10): drm/nouveau: tegra: Avoid pulsing reset twice drm/nouveau: tegra: Set clock rate if not set drm/nouveau: secboot: Read WPR configuration from GPU registers drm/nouveau: gp10b: Add custom L2 cache implementation drm/nouveau: gp10b: Use correct copy engine drm/nouveau: gk20a: Set IOMMU bit for DMA API if appropriate drm/nouveau: gk20a: Implement custom MMU class drm/nouveau: tegra: Skip IOMMU initialization if already attached drm/nouveau: tegra: Fall back to 32-bit DMA mask without IOMMU arm64: tegra: Enable SMMU for GPU on Tegra186
.../boot/dts/nvidia/tegra186-p2771-0000.dts | 4 + arch/arm64/boot/dts/nvidia/tegra186.dtsi | 1 + .../gpu/drm/nouveau/include/nvkm/subdev/ltc.h | 1 + .../gpu/drm/nouveau/nvkm/engine/device/base.c | 4 +- .../drm/nouveau/nvkm/engine/device/tegra.c | 152 +++++++++++------- .../drm/nouveau/nvkm/subdev/instmem/gk20a.c | 35 ++-- .../gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild | 1 + .../gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c | 69 ++++++++ .../gpu/drm/nouveau/nvkm/subdev/ltc/priv.h | 2 + .../gpu/drm/nouveau/nvkm/subdev/mmu/gk20a.c | 50 +++++- .../gpu/drm/nouveau/nvkm/subdev/mmu/gk20a.h | 44 +++++ .../gpu/drm/nouveau/nvkm/subdev/mmu/gm20b.c | 6 +- .../gpu/drm/nouveau/nvkm/subdev/mmu/gp10b.c | 4 +- drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h | 1 + .../drm/nouveau/nvkm/subdev/mmu/vmmgk20a.c | 22 ++- .../drm/nouveau/nvkm/subdev/mmu/vmmgm20b.c | 4 +- .../drm/nouveau/nvkm/subdev/mmu/vmmgp10b.c | 20 ++- .../drm/nouveau/nvkm/subdev/secboot/gm200.h | 2 +- .../drm/nouveau/nvkm/subdev/secboot/gm20b.c | 81 ++++++---- .../drm/nouveau/nvkm/subdev/secboot/gp10b.c | 4 +- 20 files changed, 394 insertions(+), 113 deletions(-) create mode 100644 drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c create mode 100644 drivers/gpu/drm/nouveau/nvkm/subdev/mmu/gk20a.h
From: Thierry Reding treding@nvidia.com
When the GPU powergate is controlled by a generic power domain provider, the reset will automatically be asserted and deasserted as part of the power-ungating procedure.
On some Jetson TX2 boards, doing an additional assert and deassert of the GPU outside of the power-ungate procedure can cause the GPU to go into a bad state where the memory interface can no longer access system memory.
Signed-off-by: Thierry Reding treding@nvidia.com --- drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c index 0e372a190d3f..747a775121cf 100644 --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c @@ -52,18 +52,18 @@ nvkm_device_tegra_power_up(struct nvkm_device_tegra *tdev) clk_set_rate(tdev->clk_pwr, 204000000); udelay(10);
- reset_control_assert(tdev->rst); - udelay(10); - if (!tdev->pdev->dev.pm_domain) { + reset_control_assert(tdev->rst); + udelay(10); + ret = tegra_powergate_remove_clamping(TEGRA_POWERGATE_3D); if (ret) goto err_clamp; udelay(10); - }
- reset_control_deassert(tdev->rst); - udelay(10); + reset_control_deassert(tdev->rst); + udelay(10); + }
return 0;
From: Thierry Reding treding@nvidia.com
If the GPU clock has not had a rate set, initialize it to the maximum clock rate to make sure it does run.
Signed-off-by: Thierry Reding treding@nvidia.com --- drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c index 747a775121cf..d0d52c1d4aee 100644 --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c @@ -279,6 +279,7 @@ nvkm_device_tegra_new(const struct nvkm_device_tegra_func *func, struct nvkm_device **pdevice) { struct nvkm_device_tegra *tdev; + unsigned long rate; int ret;
if (!(tdev = kzalloc(sizeof(*tdev), GFP_KERNEL))) @@ -307,6 +308,17 @@ nvkm_device_tegra_new(const struct nvkm_device_tegra_func *func, goto free; }
+ rate = clk_get_rate(tdev->clk); + if (rate == 0) { + ret = clk_set_rate(tdev->clk, ULONG_MAX); + if (ret < 0) + goto free; + + rate = clk_get_rate(tdev->clk); + + dev_dbg(&pdev->dev, "GPU clock set to %lu\n", rate); + } + if (func->require_ref_clk) tdev->clk_ref = devm_clk_get(&pdev->dev, "ref"); if (IS_ERR(tdev->clk_ref)) {
From: Thierry Reding treding@nvidia.com
The GPUs found on Tegra SoCs have registers that can be used to read the WPR configuration. Use these registers instead of reaching into the memory controller's register space to read the same information.
Signed-off-by: Thierry Reding treding@nvidia.com --- .../drm/nouveau/nvkm/subdev/secboot/gm200.h | 2 +- .../drm/nouveau/nvkm/subdev/secboot/gm20b.c | 81 ++++++++++++------- .../drm/nouveau/nvkm/subdev/secboot/gp10b.c | 4 +- 3 files changed, 53 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h index 62c5e162099a..280b1448df88 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h @@ -41,6 +41,6 @@ int gm200_secboot_run_blob(struct nvkm_secboot *, struct nvkm_gpuobj *, struct nvkm_falcon *);
/* Tegra-only */ -int gm20b_secboot_tegra_read_wpr(struct gm200_secboot *, u32); +int gm20b_secboot_tegra_read_wpr(struct gm200_secboot *);
#endif diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c index df8b919dcf09..f8a543122219 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c @@ -23,39 +23,65 @@ #include "acr.h" #include "gm200.h"
-#define TEGRA210_MC_BASE 0x70019000 - #ifdef CONFIG_ARCH_TEGRA -#define MC_SECURITY_CARVEOUT2_CFG0 0xc58 -#define MC_SECURITY_CARVEOUT2_BOM_0 0xc5c -#define MC_SECURITY_CARVEOUT2_BOM_HI_0 0xc60 -#define MC_SECURITY_CARVEOUT2_SIZE_128K 0xc64 -#define TEGRA_MC_SECURITY_CARVEOUT_CFG_LOCKED (1 << 1) /** * gm20b_secboot_tegra_read_wpr() - read the WPR registers on Tegra * - * On dGPU, we can manage the WPR region ourselves, but on Tegra the WPR region - * is reserved from system memory by the bootloader and irreversibly locked. - * This function reads the address and size of the pre-configured WPR region. + * On dGPU, we can manage the WPR region ourselves, but on Tegra this region + * is allocated from system memory by the secure firmware. The region is then + * marked as a "secure carveout" and irreversibly locked. Furthermore, the WPR + * secure carveout is also configured to be sent to the GPU via a dedicated + * serial bus between the memory controller and the GPU. The GPU requests this + * information upon leaving reset and exposes it through a FIFO register at + * offset 0x100cd4. + * + * The FIFO register's lower 4 bits can be used to set the read index into the + * FIFO. After each read of the FIFO register, the read index is incremented. + * + * Indices 2 and 3 contain the lower and upper addresses of the WPR. These are + * stored in units of 256 B. The WPR is inclusive of both addresses. + * + * Unfortunately, for some reason the WPR info register doesn't contain the + * correct values for the secure carveout. It seems like the upper address is + * always too small by 128 KiB - 1. Given that the secure carvout size in the + * memory controller configuration is specified in units of 128 KiB, it's + * possible that the computation of the upper address of the WPR is wrong and + * causes this difference. */ int -gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb, u32 mc_base) +gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb) { + struct nvkm_device *device = gsb->base.subdev.device; struct nvkm_secboot *sb = &gsb->base; - void __iomem *mc; - u32 cfg; + u64 base, limit; + u32 value;
- mc = ioremap(mc_base, 0xd00); - if (!mc) { - nvkm_error(&sb->subdev, "Cannot map Tegra MC registers\n"); - return -ENOMEM; - } - sb->wpr_addr = ioread32_native(mc + MC_SECURITY_CARVEOUT2_BOM_0) | - ((u64)ioread32_native(mc + MC_SECURITY_CARVEOUT2_BOM_HI_0) << 32); - sb->wpr_size = ioread32_native(mc + MC_SECURITY_CARVEOUT2_SIZE_128K) - << 17; - cfg = ioread32_native(mc + MC_SECURITY_CARVEOUT2_CFG0); - iounmap(mc); + /* set WPR info register to point at WPR base address register */ + value = nvkm_rd32(device, 0x100cd4); + value &= ~0xf; + value |= 0x2; + nvkm_wr32(device, 0x100cd4, value); + + /* read base address */ + value = nvkm_rd32(device, 0x100cd4); + base = (u64)(value >> 4) << 12; + + /* read limit */ + value = nvkm_rd32(device, 0x100cd4); + limit = (u64)(value >> 4) << 12; + + /* + * The upper address of the WPR seems to be computed wrongly and is + * actually SZ_128K - 1 bytes lower than it should be. Adjust the + * value accordingly. + */ + limit += SZ_128K - 1; + + sb->wpr_size = limit - base + 1; + sb->wpr_addr = base; + + nvkm_info(&sb->subdev, "WPR: %016llx-%016llx\n", sb->wpr_addr, + sb->wpr_addr + sb->wpr_size - 1);
/* Check that WPR settings are valid */ if (sb->wpr_size == 0) { @@ -63,11 +89,6 @@ gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb, u32 mc_base) return -EINVAL; }
- if (!(cfg & TEGRA_MC_SECURITY_CARVEOUT_CFG_LOCKED)) { - nvkm_error(&sb->subdev, "WPR region not locked\n"); - return -EINVAL; - } - return 0; } #else @@ -85,7 +106,7 @@ gm20b_secboot_oneinit(struct nvkm_secboot *sb) struct gm200_secboot *gsb = gm200_secboot(sb); int ret;
- ret = gm20b_secboot_tegra_read_wpr(gsb, TEGRA210_MC_BASE); + ret = gm20b_secboot_tegra_read_wpr(gsb); if (ret) return ret;
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c index 28ca29d0eeee..d84e85825995 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c @@ -23,15 +23,13 @@ #include "acr.h" #include "gm200.h"
-#define TEGRA186_MC_BASE 0x02c10000 - static int gp10b_secboot_oneinit(struct nvkm_secboot *sb) { struct gm200_secboot *gsb = gm200_secboot(sb); int ret;
- ret = gm20b_secboot_tegra_read_wpr(gsb, TEGRA186_MC_BASE); + ret = gm20b_secboot_tegra_read_wpr(gsb); if (ret) return ret;
On Tue, 17 Sep 2019 at 01:04, Thierry Reding thierry.reding@gmail.com wrote:
From: Thierry Reding treding@nvidia.com
The GPUs found on Tegra SoCs have registers that can be used to read the WPR configuration. Use these registers instead of reaching into the memory controller's register space to read the same information.
Signed-off-by: Thierry Reding treding@nvidia.com
.../drm/nouveau/nvkm/subdev/secboot/gm200.h | 2 +- .../drm/nouveau/nvkm/subdev/secboot/gm20b.c | 81 ++++++++++++------- .../drm/nouveau/nvkm/subdev/secboot/gp10b.c | 4 +- 3 files changed, 53 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h index 62c5e162099a..280b1448df88 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h @@ -41,6 +41,6 @@ int gm200_secboot_run_blob(struct nvkm_secboot *, struct nvkm_gpuobj *, struct nvkm_falcon *);
/* Tegra-only */ -int gm20b_secboot_tegra_read_wpr(struct gm200_secboot *, u32); +int gm20b_secboot_tegra_read_wpr(struct gm200_secboot *);
#endif diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c index df8b919dcf09..f8a543122219 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c @@ -23,39 +23,65 @@ #include "acr.h" #include "gm200.h"
-#define TEGRA210_MC_BASE 0x70019000
#ifdef CONFIG_ARCH_TEGRA -#define MC_SECURITY_CARVEOUT2_CFG0 0xc58 -#define MC_SECURITY_CARVEOUT2_BOM_0 0xc5c -#define MC_SECURITY_CARVEOUT2_BOM_HI_0 0xc60 -#define MC_SECURITY_CARVEOUT2_SIZE_128K 0xc64 -#define TEGRA_MC_SECURITY_CARVEOUT_CFG_LOCKED (1 << 1) /**
- gm20b_secboot_tegra_read_wpr() - read the WPR registers on Tegra
- On dGPU, we can manage the WPR region ourselves, but on Tegra the WPR region
- is reserved from system memory by the bootloader and irreversibly locked.
- This function reads the address and size of the pre-configured WPR region.
- On dGPU, we can manage the WPR region ourselves, but on Tegra this region
- is allocated from system memory by the secure firmware. The region is then
- marked as a "secure carveout" and irreversibly locked. Furthermore, the WPR
- secure carveout is also configured to be sent to the GPU via a dedicated
- serial bus between the memory controller and the GPU. The GPU requests this
- information upon leaving reset and exposes it through a FIFO register at
- offset 0x100cd4.
- The FIFO register's lower 4 bits can be used to set the read index into the
- FIFO. After each read of the FIFO register, the read index is incremented.
- Indices 2 and 3 contain the lower and upper addresses of the WPR. These are
- stored in units of 256 B. The WPR is inclusive of both addresses.
- Unfortunately, for some reason the WPR info register doesn't contain the
- correct values for the secure carveout. It seems like the upper address is
- always too small by 128 KiB - 1. Given that the secure carvout size in the
- memory controller configuration is specified in units of 128 KiB, it's
- possible that the computation of the upper address of the WPR is wrong and
*/
- causes this difference.
int -gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb, u32 mc_base) +gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb) {
struct nvkm_device *device = gsb->base.subdev.device; struct nvkm_secboot *sb = &gsb->base;
void __iomem *mc;u32 cfg;
u64 base, limit;u32 value;
mc = ioremap(mc_base, 0xd00);if (!mc) {nvkm_error(&sb->subdev, "Cannot map Tegra MC registers\n");return -ENOMEM;}sb->wpr_addr = ioread32_native(mc + MC_SECURITY_CARVEOUT2_BOM_0) |((u64)ioread32_native(mc + MC_SECURITY_CARVEOUT2_BOM_HI_0) << 32);sb->wpr_size = ioread32_native(mc + MC_SECURITY_CARVEOUT2_SIZE_128K)<< 17;cfg = ioread32_native(mc + MC_SECURITY_CARVEOUT2_CFG0);iounmap(mc);
/* set WPR info register to point at WPR base address register */value = nvkm_rd32(device, 0x100cd4);value &= ~0xf;value |= 0x2;nvkm_wr32(device, 0x100cd4, value);/* read base address */value = nvkm_rd32(device, 0x100cd4);base = (u64)(value >> 4) << 12;/* read limit */value = nvkm_rd32(device, 0x100cd4);limit = (u64)(value >> 4) << 12;
acr_r352_wpr_is_set() does a similar readout to confirm the HS firmware did its job on dGPU, perhaps this part of it could be factored out into a function that could be used in both places?
/** The upper address of the WPR seems to be computed wrongly and is* actually SZ_128K - 1 bytes lower than it should be. Adjust the* value accordingly.*/limit += SZ_128K - 1;sb->wpr_size = limit - base + 1;sb->wpr_addr = base;nvkm_info(&sb->subdev, "WPR: %016llx-%016llx\n", sb->wpr_addr,sb->wpr_addr + sb->wpr_size - 1); /* Check that WPR settings are valid */ if (sb->wpr_size == 0) {@@ -63,11 +89,6 @@ gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb, u32 mc_base) return -EINVAL; }
if (!(cfg & TEGRA_MC_SECURITY_CARVEOUT_CFG_LOCKED)) {nvkm_error(&sb->subdev, "WPR region not locked\n");return -EINVAL;}return 0;} #else @@ -85,7 +106,7 @@ gm20b_secboot_oneinit(struct nvkm_secboot *sb) struct gm200_secboot *gsb = gm200_secboot(sb); int ret;
ret = gm20b_secboot_tegra_read_wpr(gsb, TEGRA210_MC_BASE);
ret = gm20b_secboot_tegra_read_wpr(gsb); if (ret) return ret;diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c index 28ca29d0eeee..d84e85825995 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c @@ -23,15 +23,13 @@ #include "acr.h" #include "gm200.h"
-#define TEGRA186_MC_BASE 0x02c10000
static int gp10b_secboot_oneinit(struct nvkm_secboot *sb) { struct gm200_secboot *gsb = gm200_secboot(sb); int ret;
ret = gm20b_secboot_tegra_read_wpr(gsb, TEGRA186_MC_BASE);
ret = gm20b_secboot_tegra_read_wpr(gsb); if (ret) return ret;-- 2.23.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Sep 17, 2019 at 01:49:57PM +1000, Ben Skeggs wrote:
On Tue, 17 Sep 2019 at 01:04, Thierry Reding thierry.reding@gmail.com wrote:
From: Thierry Reding treding@nvidia.com
The GPUs found on Tegra SoCs have registers that can be used to read the WPR configuration. Use these registers instead of reaching into the memory controller's register space to read the same information.
Signed-off-by: Thierry Reding treding@nvidia.com
.../drm/nouveau/nvkm/subdev/secboot/gm200.h | 2 +- .../drm/nouveau/nvkm/subdev/secboot/gm20b.c | 81 ++++++++++++------- .../drm/nouveau/nvkm/subdev/secboot/gp10b.c | 4 +- 3 files changed, 53 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h index 62c5e162099a..280b1448df88 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h @@ -41,6 +41,6 @@ int gm200_secboot_run_blob(struct nvkm_secboot *, struct nvkm_gpuobj *, struct nvkm_falcon *);
/* Tegra-only */ -int gm20b_secboot_tegra_read_wpr(struct gm200_secboot *, u32); +int gm20b_secboot_tegra_read_wpr(struct gm200_secboot *);
#endif diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c index df8b919dcf09..f8a543122219 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c @@ -23,39 +23,65 @@ #include "acr.h" #include "gm200.h"
-#define TEGRA210_MC_BASE 0x70019000
#ifdef CONFIG_ARCH_TEGRA -#define MC_SECURITY_CARVEOUT2_CFG0 0xc58 -#define MC_SECURITY_CARVEOUT2_BOM_0 0xc5c -#define MC_SECURITY_CARVEOUT2_BOM_HI_0 0xc60 -#define MC_SECURITY_CARVEOUT2_SIZE_128K 0xc64 -#define TEGRA_MC_SECURITY_CARVEOUT_CFG_LOCKED (1 << 1) /**
- gm20b_secboot_tegra_read_wpr() - read the WPR registers on Tegra
- On dGPU, we can manage the WPR region ourselves, but on Tegra the WPR region
- is reserved from system memory by the bootloader and irreversibly locked.
- This function reads the address and size of the pre-configured WPR region.
- On dGPU, we can manage the WPR region ourselves, but on Tegra this region
- is allocated from system memory by the secure firmware. The region is then
- marked as a "secure carveout" and irreversibly locked. Furthermore, the WPR
- secure carveout is also configured to be sent to the GPU via a dedicated
- serial bus between the memory controller and the GPU. The GPU requests this
- information upon leaving reset and exposes it through a FIFO register at
- offset 0x100cd4.
- The FIFO register's lower 4 bits can be used to set the read index into the
- FIFO. After each read of the FIFO register, the read index is incremented.
- Indices 2 and 3 contain the lower and upper addresses of the WPR. These are
- stored in units of 256 B. The WPR is inclusive of both addresses.
- Unfortunately, for some reason the WPR info register doesn't contain the
- correct values for the secure carveout. It seems like the upper address is
- always too small by 128 KiB - 1. Given that the secure carvout size in the
- memory controller configuration is specified in units of 128 KiB, it's
- possible that the computation of the upper address of the WPR is wrong and
*/
- causes this difference.
int -gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb, u32 mc_base) +gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb) {
struct nvkm_device *device = gsb->base.subdev.device; struct nvkm_secboot *sb = &gsb->base;
void __iomem *mc;u32 cfg;
u64 base, limit;u32 value;
mc = ioremap(mc_base, 0xd00);if (!mc) {nvkm_error(&sb->subdev, "Cannot map Tegra MC registers\n");return -ENOMEM;}sb->wpr_addr = ioread32_native(mc + MC_SECURITY_CARVEOUT2_BOM_0) |((u64)ioread32_native(mc + MC_SECURITY_CARVEOUT2_BOM_HI_0) << 32);sb->wpr_size = ioread32_native(mc + MC_SECURITY_CARVEOUT2_SIZE_128K)<< 17;cfg = ioread32_native(mc + MC_SECURITY_CARVEOUT2_CFG0);iounmap(mc);
/* set WPR info register to point at WPR base address register */value = nvkm_rd32(device, 0x100cd4);value &= ~0xf;value |= 0x2;nvkm_wr32(device, 0x100cd4, value);/* read base address */value = nvkm_rd32(device, 0x100cd4);base = (u64)(value >> 4) << 12;/* read limit */value = nvkm_rd32(device, 0x100cd4);limit = (u64)(value >> 4) << 12;acr_r352_wpr_is_set() does a similar readout to confirm the HS firmware did its job on dGPU, perhaps this part of it could be factored out into a function that could be used in both places?
I hadn't seen that function. It looks to me like these are now both doing exactly the same thing. The acr_r352_wpr_is_set() also seems to serve only to check that what's read from these WPR info registers matches what was configured as the WPR region previously. This is now rather pointless because, unless the computations differ, the result must be true.
Honestly, I'm not sure we really need to check this. My understanding is that these WPR info registers are the canonical way of obtaining the WPR region base and limit. The way that this works is that the GPU has a dedicated bus to the memory controller and it fetches these parameters from the MC when it leaves reset.
One oddity here, though, is that the code in acr_r352_wpr_is_set() does not account for the strange way that the limit is encoded in these registers. I suspect that this is some weird hardware bug that happens during the transfer of the WPR information to the GPU. I came across some documentation that specifically defines the limit register to contain the upper limit of the WPR in units of 256 B with the WPR being inclusive of both the base and the limit. I'm not exactly sure why this has gone unnoticed, but I think nvgpu doesn't actually test for the WPR size when it loads the firmware. I only ran into this when implementing the WPR info register readout because Nouveau would refuse to load the firmware because it didn't fit into what it thought was the WPR.
Anyway, I've tested this on all of gm20b, gp10b and gv11b and the above computations give me the same values that the (SoC) firmware claims that it configured the WPR with.
Given the above, do you see any further use for acr_r352_wpr_is_set()? Should I follow up with a patch removing it?
Thierry
/** The upper address of the WPR seems to be computed wrongly and is* actually SZ_128K - 1 bytes lower than it should be. Adjust the* value accordingly.*/limit += SZ_128K - 1;sb->wpr_size = limit - base + 1;sb->wpr_addr = base;nvkm_info(&sb->subdev, "WPR: %016llx-%016llx\n", sb->wpr_addr,sb->wpr_addr + sb->wpr_size - 1); /* Check that WPR settings are valid */ if (sb->wpr_size == 0) {@@ -63,11 +89,6 @@ gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb, u32 mc_base) return -EINVAL; }
if (!(cfg & TEGRA_MC_SECURITY_CARVEOUT_CFG_LOCKED)) {nvkm_error(&sb->subdev, "WPR region not locked\n");return -EINVAL;}return 0;} #else @@ -85,7 +106,7 @@ gm20b_secboot_oneinit(struct nvkm_secboot *sb) struct gm200_secboot *gsb = gm200_secboot(sb); int ret;
ret = gm20b_secboot_tegra_read_wpr(gsb, TEGRA210_MC_BASE);
ret = gm20b_secboot_tegra_read_wpr(gsb); if (ret) return ret;diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c index 28ca29d0eeee..d84e85825995 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c @@ -23,15 +23,13 @@ #include "acr.h" #include "gm200.h"
-#define TEGRA186_MC_BASE 0x02c10000
static int gp10b_secboot_oneinit(struct nvkm_secboot *sb) { struct gm200_secboot *gsb = gm200_secboot(sb); int ret;
ret = gm20b_secboot_tegra_read_wpr(gsb, TEGRA186_MC_BASE);
ret = gm20b_secboot_tegra_read_wpr(gsb); if (ret) return ret;-- 2.23.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, 17 Sep 2019 at 18:40, Thierry Reding thierry.reding@gmail.com wrote:
On Tue, Sep 17, 2019 at 01:49:57PM +1000, Ben Skeggs wrote:
On Tue, 17 Sep 2019 at 01:04, Thierry Reding thierry.reding@gmail.com wrote:
From: Thierry Reding treding@nvidia.com
The GPUs found on Tegra SoCs have registers that can be used to read the WPR configuration. Use these registers instead of reaching into the memory controller's register space to read the same information.
Signed-off-by: Thierry Reding treding@nvidia.com
.../drm/nouveau/nvkm/subdev/secboot/gm200.h | 2 +- .../drm/nouveau/nvkm/subdev/secboot/gm20b.c | 81 ++++++++++++------- .../drm/nouveau/nvkm/subdev/secboot/gp10b.c | 4 +- 3 files changed, 53 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h index 62c5e162099a..280b1448df88 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm200.h @@ -41,6 +41,6 @@ int gm200_secboot_run_blob(struct nvkm_secboot *, struct nvkm_gpuobj *, struct nvkm_falcon *);
/* Tegra-only */ -int gm20b_secboot_tegra_read_wpr(struct gm200_secboot *, u32); +int gm20b_secboot_tegra_read_wpr(struct gm200_secboot *);
#endif diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c index df8b919dcf09..f8a543122219 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gm20b.c @@ -23,39 +23,65 @@ #include "acr.h" #include "gm200.h"
-#define TEGRA210_MC_BASE 0x70019000
#ifdef CONFIG_ARCH_TEGRA -#define MC_SECURITY_CARVEOUT2_CFG0 0xc58 -#define MC_SECURITY_CARVEOUT2_BOM_0 0xc5c -#define MC_SECURITY_CARVEOUT2_BOM_HI_0 0xc60 -#define MC_SECURITY_CARVEOUT2_SIZE_128K 0xc64 -#define TEGRA_MC_SECURITY_CARVEOUT_CFG_LOCKED (1 << 1) /**
- gm20b_secboot_tegra_read_wpr() - read the WPR registers on Tegra
- On dGPU, we can manage the WPR region ourselves, but on Tegra the WPR region
- is reserved from system memory by the bootloader and irreversibly locked.
- This function reads the address and size of the pre-configured WPR region.
- On dGPU, we can manage the WPR region ourselves, but on Tegra this region
- is allocated from system memory by the secure firmware. The region is then
- marked as a "secure carveout" and irreversibly locked. Furthermore, the WPR
- secure carveout is also configured to be sent to the GPU via a dedicated
- serial bus between the memory controller and the GPU. The GPU requests this
- information upon leaving reset and exposes it through a FIFO register at
- offset 0x100cd4.
- The FIFO register's lower 4 bits can be used to set the read index into the
- FIFO. After each read of the FIFO register, the read index is incremented.
- Indices 2 and 3 contain the lower and upper addresses of the WPR. These are
- stored in units of 256 B. The WPR is inclusive of both addresses.
- Unfortunately, for some reason the WPR info register doesn't contain the
- correct values for the secure carveout. It seems like the upper address is
- always too small by 128 KiB - 1. Given that the secure carvout size in the
- memory controller configuration is specified in units of 128 KiB, it's
- possible that the computation of the upper address of the WPR is wrong and
*/
- causes this difference.
int -gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb, u32 mc_base) +gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb) {
struct nvkm_device *device = gsb->base.subdev.device; struct nvkm_secboot *sb = &gsb->base;
void __iomem *mc;u32 cfg;
u64 base, limit;u32 value;
mc = ioremap(mc_base, 0xd00);if (!mc) {nvkm_error(&sb->subdev, "Cannot map Tegra MC registers\n");return -ENOMEM;}sb->wpr_addr = ioread32_native(mc + MC_SECURITY_CARVEOUT2_BOM_0) |((u64)ioread32_native(mc + MC_SECURITY_CARVEOUT2_BOM_HI_0) << 32);sb->wpr_size = ioread32_native(mc + MC_SECURITY_CARVEOUT2_SIZE_128K)<< 17;cfg = ioread32_native(mc + MC_SECURITY_CARVEOUT2_CFG0);iounmap(mc);
/* set WPR info register to point at WPR base address register */value = nvkm_rd32(device, 0x100cd4);value &= ~0xf;value |= 0x2;nvkm_wr32(device, 0x100cd4, value);/* read base address */value = nvkm_rd32(device, 0x100cd4);base = (u64)(value >> 4) << 12;/* read limit */value = nvkm_rd32(device, 0x100cd4);limit = (u64)(value >> 4) << 12;acr_r352_wpr_is_set() does a similar readout to confirm the HS firmware did its job on dGPU, perhaps this part of it could be factored out into a function that could be used in both places?
I hadn't seen that function. It looks to me like these are now both doing exactly the same thing. The acr_r352_wpr_is_set() also seems to serve only to check that what's read from these WPR info registers matches what was configured as the WPR region previously. This is now rather pointless because, unless the computations differ, the result must be true.
Yeah, I believe its purpose is simply to confirm the HS firmware executed correctly.
Honestly, I'm not sure we really need to check this. My understanding is that these WPR info registers are the canonical way of obtaining the WPR region base and limit. The way that this works is that the GPU has a dedicated bus to the memory controller and it fetches these parameters from the MC when it leaves reset.
One oddity here, though, is that the code in acr_r352_wpr_is_set() does not account for the strange way that the limit is encoded in these registers. I suspect that this is some weird hardware bug that happens during the transfer of the WPR information to the GPU. I came across some documentation that specifically defines the limit register to contain the upper limit of the WPR in units of 256 B with the WPR being inclusive of both the base and the limit. I'm not exactly sure why this has gone unnoticed, but I think nvgpu doesn't actually test for the WPR size when it loads the firmware. I only ran into this when implementing the WPR info register readout because Nouveau would refuse to load the firmware because it didn't fit into what it thought was the WPR.
Anyway, I've tested this on all of gm20b, gp10b and gv11b and the above computations give me the same values that the (SoC) firmware claims that it configured the WPR with.
Given the above, do you see any further use for acr_r352_wpr_is_set()? Should I follow up with a patch removing it?
You can leave it for now if you like, I'm reworking that entire subsystem already anyway and can nuke it there.
Ben.
Thierry
/** The upper address of the WPR seems to be computed wrongly and is* actually SZ_128K - 1 bytes lower than it should be. Adjust the* value accordingly.*/limit += SZ_128K - 1;sb->wpr_size = limit - base + 1;sb->wpr_addr = base;nvkm_info(&sb->subdev, "WPR: %016llx-%016llx\n", sb->wpr_addr,sb->wpr_addr + sb->wpr_size - 1); /* Check that WPR settings are valid */ if (sb->wpr_size == 0) {@@ -63,11 +89,6 @@ gm20b_secboot_tegra_read_wpr(struct gm200_secboot *gsb, u32 mc_base) return -EINVAL; }
if (!(cfg & TEGRA_MC_SECURITY_CARVEOUT_CFG_LOCKED)) {nvkm_error(&sb->subdev, "WPR region not locked\n");return -EINVAL;}return 0;} #else @@ -85,7 +106,7 @@ gm20b_secboot_oneinit(struct nvkm_secboot *sb) struct gm200_secboot *gsb = gm200_secboot(sb); int ret;
ret = gm20b_secboot_tegra_read_wpr(gsb, TEGRA210_MC_BASE);
ret = gm20b_secboot_tegra_read_wpr(gsb); if (ret) return ret;diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c index 28ca29d0eeee..d84e85825995 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/secboot/gp10b.c @@ -23,15 +23,13 @@ #include "acr.h" #include "gm200.h"
-#define TEGRA186_MC_BASE 0x02c10000
static int gp10b_secboot_oneinit(struct nvkm_secboot *sb) { struct gm200_secboot *gsb = gm200_secboot(sb); int ret;
ret = gm20b_secboot_tegra_read_wpr(gsb, TEGRA186_MC_BASE);
ret = gm20b_secboot_tegra_read_wpr(gsb); if (ret) return ret;-- 2.23.0
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
From: Thierry Reding treding@nvidia.com
There are extra registers that need to be programmed to make the level 2 cache work on GP10B, such as the stream ID register that is used when an SMMU is used to translate memory addresses.
Signed-off-by: Thierry Reding treding@nvidia.com --- .../gpu/drm/nouveau/include/nvkm/subdev/ltc.h | 1 + .../gpu/drm/nouveau/nvkm/engine/device/base.c | 2 +- .../gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild | 1 + .../gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c | 69 +++++++++++++++++++ .../gpu/drm/nouveau/nvkm/subdev/ltc/priv.h | 2 + 5 files changed, 74 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c
diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/ltc.h b/drivers/gpu/drm/nouveau/include/nvkm/subdev/ltc.h index 644d527c3b96..d76f60d7d29a 100644 --- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/ltc.h +++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/ltc.h @@ -40,4 +40,5 @@ int gm107_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **); int gm200_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **); int gp100_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **); int gp102_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **); +int gp10b_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **); #endif diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c index c3c7159f3411..d2d6d5f4028a 100644 --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c @@ -2380,7 +2380,7 @@ nv13b_chipset = { .fuse = gm107_fuse_new, .ibus = gp10b_ibus_new, .imem = gk20a_instmem_new, - .ltc = gp102_ltc_new, + .ltc = gp10b_ltc_new, .mc = gp10b_mc_new, .mmu = gp10b_mmu_new, .secboot = gp10b_secboot_new, diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild b/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild index 2b6d36ea7067..728d75010847 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild @@ -6,3 +6,4 @@ nvkm-y += nvkm/subdev/ltc/gm107.o nvkm-y += nvkm/subdev/ltc/gm200.o nvkm-y += nvkm/subdev/ltc/gp100.o nvkm-y += nvkm/subdev/ltc/gp102.o +nvkm-y += nvkm/subdev/ltc/gp10b.o diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c b/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c new file mode 100644 index 000000000000..4d27c6ea1552 --- /dev/null +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c @@ -0,0 +1,69 @@ +/* + * Copyright (c) 2019 NVIDIA Corporation. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR + * OTHER DEALINGS IN THE SOFTWARE. + * + * Authors: Thierry Reding + */ + +#include "priv.h" + +static void +gp10b_ltc_init(struct nvkm_ltc *ltc) +{ + struct nvkm_device *device = ltc->subdev.device; +#ifdef CONFIG_IOMMU_API + struct iommu_fwspec *spec; +#endif + + nvkm_wr32(device, 0x17e27c, ltc->ltc_nr); + nvkm_wr32(device, 0x17e000, ltc->ltc_nr); + nvkm_wr32(device, 0x100800, ltc->ltc_nr); + +#ifdef CONFIG_IOMMU_API + spec = dev_iommu_fwspec_get(device->dev); + if (spec) { + u32 sid = spec->ids[0] & 0xffff; + + /* stream ID */ + nvkm_wr32(device, 0x160000, sid << 2); + } +#endif +} + +static const struct nvkm_ltc_func +gp10b_ltc = { + .oneinit = gp100_ltc_oneinit, + .init = gp10b_ltc_init, + .intr = gp100_ltc_intr, + .cbc_clear = gm107_ltc_cbc_clear, + .cbc_wait = gm107_ltc_cbc_wait, + .zbc = 16, + .zbc_clear_color = gm107_ltc_zbc_clear_color, + .zbc_clear_depth = gm107_ltc_zbc_clear_depth, + .zbc_clear_stencil = gp102_ltc_zbc_clear_stencil, + .invalidate = gf100_ltc_invalidate, + .flush = gf100_ltc_flush, +}; + +int +gp10b_ltc_new(struct nvkm_device *device, int index, struct nvkm_ltc **pltc) +{ + return nvkm_ltc_new_(&gp10b_ltc, device, index, pltc); +} diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/priv.h b/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/priv.h index 2fcf18e46ce3..eca5a711b1b8 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/priv.h +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/priv.h @@ -46,4 +46,6 @@ void gm107_ltc_zbc_clear_depth(struct nvkm_ltc *, int, const u32); int gp100_ltc_oneinit(struct nvkm_ltc *); void gp100_ltc_init(struct nvkm_ltc *); void gp100_ltc_intr(struct nvkm_ltc *); + +void gp102_ltc_zbc_clear_stencil(struct nvkm_ltc *, int, const u32); #endif
On 16/09/2019 16:04, Thierry Reding wrote:
From: Thierry Reding treding@nvidia.com
There are extra registers that need to be programmed to make the level 2 cache work on GP10B, such as the stream ID register that is used when an SMMU is used to translate memory addresses.
Signed-off-by: Thierry Reding treding@nvidia.com
.../gpu/drm/nouveau/include/nvkm/subdev/ltc.h | 1 + .../gpu/drm/nouveau/nvkm/engine/device/base.c | 2 +- .../gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild | 1 + .../gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c | 69 +++++++++++++++++++ .../gpu/drm/nouveau/nvkm/subdev/ltc/priv.h | 2 + 5 files changed, 74 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c
diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/ltc.h b/drivers/gpu/drm/nouveau/include/nvkm/subdev/ltc.h index 644d527c3b96..d76f60d7d29a 100644 --- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/ltc.h +++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/ltc.h @@ -40,4 +40,5 @@ int gm107_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **); int gm200_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **); int gp100_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **); int gp102_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **); +int gp10b_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **); #endif diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c index c3c7159f3411..d2d6d5f4028a 100644 --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c @@ -2380,7 +2380,7 @@ nv13b_chipset = { .fuse = gm107_fuse_new, .ibus = gp10b_ibus_new, .imem = gk20a_instmem_new,
- .ltc = gp102_ltc_new,
- .ltc = gp10b_ltc_new, .mc = gp10b_mc_new, .mmu = gp10b_mmu_new, .secboot = gp10b_secboot_new,
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild b/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild index 2b6d36ea7067..728d75010847 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild @@ -6,3 +6,4 @@ nvkm-y += nvkm/subdev/ltc/gm107.o nvkm-y += nvkm/subdev/ltc/gm200.o nvkm-y += nvkm/subdev/ltc/gp100.o nvkm-y += nvkm/subdev/ltc/gp102.o +nvkm-y += nvkm/subdev/ltc/gp10b.o diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c b/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c new file mode 100644 index 000000000000..4d27c6ea1552 --- /dev/null +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c @@ -0,0 +1,69 @@ +/*
- Copyright (c) 2019 NVIDIA Corporation.
- Permission is hereby granted, free of charge, to any person obtaining a
- copy of this software and associated documentation files (the "Software"),
- to deal in the Software without restriction, including without limitation
- the rights to use, copy, modify, merge, publish, distribute, sublicense,
- and/or sell copies of the Software, and to permit persons to whom the
- Software is furnished to do so, subject to the following conditions:
- The above copyright notice and this permission notice shall be included in
- all copies or substantial portions of the Software.
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
- THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
- OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
- ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
- OTHER DEALINGS IN THE SOFTWARE.
- Authors: Thierry Reding
- */
+#include "priv.h"
+static void +gp10b_ltc_init(struct nvkm_ltc *ltc) +{
- struct nvkm_device *device = ltc->subdev.device;
+#ifdef CONFIG_IOMMU_API
- struct iommu_fwspec *spec;
+#endif
- nvkm_wr32(device, 0x17e27c, ltc->ltc_nr);
- nvkm_wr32(device, 0x17e000, ltc->ltc_nr);
- nvkm_wr32(device, 0x100800, ltc->ltc_nr);
+#ifdef CONFIG_IOMMU_API
- spec = dev_iommu_fwspec_get(device->dev);
- if (spec) {
u32 sid = spec->ids[0] & 0xffff;/* stream ID */nvkm_wr32(device, 0x160000, sid << 2);- }
+#endif
Could we get rid of the #ifdef blocks here if there was a NULL inline version of dev_iommu_fwspec_get() in the include/linux/iommu.h header? The compiler should then optimise the lot out if the config is not set.
On Mon, Sep 16, 2019 at 04:35:30PM +0100, Ben Dooks wrote:
On 16/09/2019 16:04, Thierry Reding wrote:
From: Thierry Reding treding@nvidia.com
There are extra registers that need to be programmed to make the level 2 cache work on GP10B, such as the stream ID register that is used when an SMMU is used to translate memory addresses.
Signed-off-by: Thierry Reding treding@nvidia.com
.../gpu/drm/nouveau/include/nvkm/subdev/ltc.h | 1 + .../gpu/drm/nouveau/nvkm/engine/device/base.c | 2 +- .../gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild | 1 + .../gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c | 69 +++++++++++++++++++ .../gpu/drm/nouveau/nvkm/subdev/ltc/priv.h | 2 + 5 files changed, 74 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c
diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/ltc.h b/drivers/gpu/drm/nouveau/include/nvkm/subdev/ltc.h index 644d527c3b96..d76f60d7d29a 100644 --- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/ltc.h +++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/ltc.h @@ -40,4 +40,5 @@ int gm107_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **); int gm200_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **); int gp100_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **); int gp102_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **); +int gp10b_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **); #endif diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c index c3c7159f3411..d2d6d5f4028a 100644 --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c @@ -2380,7 +2380,7 @@ nv13b_chipset = { .fuse = gm107_fuse_new, .ibus = gp10b_ibus_new, .imem = gk20a_instmem_new,
- .ltc = gp102_ltc_new,
- .ltc = gp10b_ltc_new, .mc = gp10b_mc_new, .mmu = gp10b_mmu_new, .secboot = gp10b_secboot_new,
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild b/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild index 2b6d36ea7067..728d75010847 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild @@ -6,3 +6,4 @@ nvkm-y += nvkm/subdev/ltc/gm107.o nvkm-y += nvkm/subdev/ltc/gm200.o nvkm-y += nvkm/subdev/ltc/gp100.o nvkm-y += nvkm/subdev/ltc/gp102.o +nvkm-y += nvkm/subdev/ltc/gp10b.o diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c b/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c new file mode 100644 index 000000000000..4d27c6ea1552 --- /dev/null +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c @@ -0,0 +1,69 @@ +/*
- Copyright (c) 2019 NVIDIA Corporation.
- Permission is hereby granted, free of charge, to any person obtaining a
- copy of this software and associated documentation files (the "Software"),
- to deal in the Software without restriction, including without limitation
- the rights to use, copy, modify, merge, publish, distribute, sublicense,
- and/or sell copies of the Software, and to permit persons to whom the
- Software is furnished to do so, subject to the following conditions:
- The above copyright notice and this permission notice shall be included in
- all copies or substantial portions of the Software.
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
- THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
- OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
- ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
- OTHER DEALINGS IN THE SOFTWARE.
- Authors: Thierry Reding
- */
+#include "priv.h"
+static void +gp10b_ltc_init(struct nvkm_ltc *ltc) +{
- struct nvkm_device *device = ltc->subdev.device;
+#ifdef CONFIG_IOMMU_API
- struct iommu_fwspec *spec;
+#endif
- nvkm_wr32(device, 0x17e27c, ltc->ltc_nr);
- nvkm_wr32(device, 0x17e000, ltc->ltc_nr);
- nvkm_wr32(device, 0x100800, ltc->ltc_nr);
+#ifdef CONFIG_IOMMU_API
- spec = dev_iommu_fwspec_get(device->dev);
- if (spec) {
u32 sid = spec->ids[0] & 0xffff;/* stream ID */nvkm_wr32(device, 0x160000, sid << 2);- }
+#endif
Could we get rid of the #ifdef blocks here if there was a NULL inline version of dev_iommu_fwspec_get() in the include/linux/iommu.h header? The compiler should then optimise the lot out if the config is not set.
I had thought the same thing and had even typed up the patch to add a dummy for dev_iommu_fwspec_get(), but then when I tested some builds, I got build errors nevertheless because struct iommu_fwspec is only declared under IOMMU_API.
The obvious thing would have been to move struct iommu_fwspec outside of the IOMMU_API protection, but then I remembered having an earlier discussion with Joerg about something similar. I guess the issue here is that we need to reach into struct iommu_fwspec, so it has to be available in full.
Anyway, I can add a patch to do this and remove the IOMMU_API guards, but let's see what Joerg thinks about that first.
Joerg, to summarize: the proposal here is to move the declaration of the iommu_fwspec outside of the IOMMU_API guard and provide a dummy implementation of dev_iommu_fwspec_get() to allow this code to be built without the #ifdef guards. We had discussed something similar about 5 years back and at the time you had been opposed:
https://lore.kernel.org/linux-iommu/1406897113-20099-1-git-send-email-thierr...
The case here is slightly different and a lot of time has passed since, so just wanted to ask if your opinion here is the same, or whether you would accept a patch to make this buildable without resorting to #ifdef'ery.
Thierry
On Mon, Sep 16, 2019 at 05:49:46PM +0200, Thierry Reding wrote:
On Mon, Sep 16, 2019 at 04:35:30PM +0100, Ben Dooks wrote:
On 16/09/2019 16:04, Thierry Reding wrote:
From: Thierry Reding treding@nvidia.com
There are extra registers that need to be programmed to make the level 2 cache work on GP10B, such as the stream ID register that is used when an SMMU is used to translate memory addresses.
Signed-off-by: Thierry Reding treding@nvidia.com
.../gpu/drm/nouveau/include/nvkm/subdev/ltc.h | 1 + .../gpu/drm/nouveau/nvkm/engine/device/base.c | 2 +- .../gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild | 1 + .../gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c | 69 +++++++++++++++++++ .../gpu/drm/nouveau/nvkm/subdev/ltc/priv.h | 2 + 5 files changed, 74 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c
diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/ltc.h b/drivers/gpu/drm/nouveau/include/nvkm/subdev/ltc.h index 644d527c3b96..d76f60d7d29a 100644 --- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/ltc.h +++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/ltc.h @@ -40,4 +40,5 @@ int gm107_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **); int gm200_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **); int gp100_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **); int gp102_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **); +int gp10b_ltc_new(struct nvkm_device *, int, struct nvkm_ltc **); #endif diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c index c3c7159f3411..d2d6d5f4028a 100644 --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c @@ -2380,7 +2380,7 @@ nv13b_chipset = { .fuse = gm107_fuse_new, .ibus = gp10b_ibus_new, .imem = gk20a_instmem_new,
- .ltc = gp102_ltc_new,
- .ltc = gp10b_ltc_new, .mc = gp10b_mc_new, .mmu = gp10b_mmu_new, .secboot = gp10b_secboot_new,
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild b/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild index 2b6d36ea7067..728d75010847 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/Kbuild @@ -6,3 +6,4 @@ nvkm-y += nvkm/subdev/ltc/gm107.o nvkm-y += nvkm/subdev/ltc/gm200.o nvkm-y += nvkm/subdev/ltc/gp100.o nvkm-y += nvkm/subdev/ltc/gp102.o +nvkm-y += nvkm/subdev/ltc/gp10b.o diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c b/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c new file mode 100644 index 000000000000..4d27c6ea1552 --- /dev/null +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/ltc/gp10b.c @@ -0,0 +1,69 @@ +/*
- Copyright (c) 2019 NVIDIA Corporation.
- Permission is hereby granted, free of charge, to any person obtaining a
- copy of this software and associated documentation files (the "Software"),
- to deal in the Software without restriction, including without limitation
- the rights to use, copy, modify, merge, publish, distribute, sublicense,
- and/or sell copies of the Software, and to permit persons to whom the
- Software is furnished to do so, subject to the following conditions:
- The above copyright notice and this permission notice shall be included in
- all copies or substantial portions of the Software.
- THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
- THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
- OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
- ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
- OTHER DEALINGS IN THE SOFTWARE.
- Authors: Thierry Reding
- */
+#include "priv.h"
+static void +gp10b_ltc_init(struct nvkm_ltc *ltc) +{
- struct nvkm_device *device = ltc->subdev.device;
+#ifdef CONFIG_IOMMU_API
- struct iommu_fwspec *spec;
+#endif
- nvkm_wr32(device, 0x17e27c, ltc->ltc_nr);
- nvkm_wr32(device, 0x17e000, ltc->ltc_nr);
- nvkm_wr32(device, 0x100800, ltc->ltc_nr);
+#ifdef CONFIG_IOMMU_API
- spec = dev_iommu_fwspec_get(device->dev);
- if (spec) {
u32 sid = spec->ids[0] & 0xffff;/* stream ID */nvkm_wr32(device, 0x160000, sid << 2);- }
+#endif
Could we get rid of the #ifdef blocks here if there was a NULL inline version of dev_iommu_fwspec_get() in the include/linux/iommu.h header? The compiler should then optimise the lot out if the config is not set.
I had thought the same thing and had even typed up the patch to add a dummy for dev_iommu_fwspec_get(), but then when I tested some builds, I got build errors nevertheless because struct iommu_fwspec is only declared under IOMMU_API.
The obvious thing would have been to move struct iommu_fwspec outside of the IOMMU_API protection, but then I remembered having an earlier discussion with Joerg about something similar. I guess the issue here is that we need to reach into struct iommu_fwspec, so it has to be available in full.
Anyway, I can add a patch to do this and remove the IOMMU_API guards, but let's see what Joerg thinks about that first.
Joerg, to summarize: the proposal here is to move the declaration of the iommu_fwspec outside of the IOMMU_API guard and provide a dummy implementation of dev_iommu_fwspec_get() to allow this code to be built without the #ifdef guards. We had discussed something similar about 5 years back and at the time you had been opposed:
https://lore.kernel.org/linux-iommu/1406897113-20099-1-git-send-email-thierr...
The case here is slightly different and a lot of time has passed since, so just wanted to ask if your opinion here is the same, or whether you would accept a patch to make this buildable without resorting to #ifdef'ery.
Thierry
Actually adding Joerg this time.
Thierry
Hi Thierry,
On Mon, Sep 16, 2019 at 05:54:43PM +0200, Thierry Reding wrote:
Joerg, to summarize: the proposal here is to move the declaration of the iommu_fwspec outside of the IOMMU_API guard and provide a dummy implementation of dev_iommu_fwspec_get() to allow this code to be built without the #ifdef guards. We had discussed something similar about 5 years back and at the time you had been opposed:
https://lore.kernel.org/linux-iommu/1406897113-20099-1-git-send-email-thierr...
The case here is slightly different and a lot of time has passed since, so just wanted to ask if your opinion here is the same, or whether you would accept a patch to make this buildable without resorting to #ifdef'ery.
This is about DRM, so if it is fine to build drm/nouveau without IOMMU-API I am also fine with dummys for dev_iommu_fwspec_get() and a dummy 'struct iommu_fwspec'.
Regards,
Joerg
From: Thierry Reding treding@nvidia.com
gp10b uses the new engine enumeration mechanism introduced in the Pascal architecture. As a result, the copy engine, which used to be at index 2 for prior Tegra GPU instantiations, has now moved to index 0. Fix up the index and also use the gp100 variant of the copy engine class because on gp10b the PASCAL_DMA_COPY_B class is not supported.
Signed-off-by: Thierry Reding treding@nvidia.com --- drivers/gpu/drm/nouveau/nvkm/engine/device/base.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c index d2d6d5f4028a..99d3fa3fad89 100644 --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/base.c @@ -2387,7 +2387,7 @@ nv13b_chipset = { .pmu = gm20b_pmu_new, .timer = gk20a_timer_new, .top = gk104_top_new, - .ce[2] = gp102_ce_new, + .ce[0] = gp100_ce_new, .dma = gf119_dma_new, .fifo = gp10b_fifo_new, .gr = gp10b_gr_new,
From: Thierry Reding treding@nvidia.com
Detect if the DMA API is backed by an IOMMU and set the IOMMU bit if so. This is needed to make sure IOMMU addresses are properly translated even the explicit IOMMU API is not used.
Signed-off-by: Thierry Reding treding@nvidia.com --- .../drm/nouveau/nvkm/subdev/instmem/gk20a.c | 35 +++++++++++++------ 1 file changed, 25 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c index b0493f8df1fe..1120a2a7d5f1 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c @@ -100,12 +100,14 @@ struct gk20a_instmem { unsigned int vaddr_max; struct list_head vaddr_lru;
+ /* IOMMU mapping */ + unsigned int page_shift; + u64 iommu_mask; + /* Only used if IOMMU if present */ struct mutex *mm_mutex; struct nvkm_mm *mm; struct iommu_domain *domain; - unsigned long iommu_pgshift; - u16 iommu_bit;
/* Only used by DMA API */ unsigned long attrs; @@ -357,12 +359,12 @@ gk20a_instobj_dtor_iommu(struct nvkm_memory *memory) mutex_unlock(&imem->lock);
/* clear IOMMU bit to unmap pages */ - r->offset &= ~BIT(imem->iommu_bit - imem->iommu_pgshift); + r->offset &= ~imem->iommu_mask;
/* Unmap pages from GPU address space and free them */ for (i = 0; i < node->base.mn->length; i++) { iommu_unmap(imem->domain, - (r->offset + i) << imem->iommu_pgshift, PAGE_SIZE); + (r->offset + i) << imem->page_shift, PAGE_SIZE); dma_unmap_page(dev, node->dma_addrs[i], PAGE_SIZE, DMA_BIDIRECTIONAL); __free_page(node->pages[i]); @@ -440,7 +442,7 @@ gk20a_instobj_ctor_dma(struct gk20a_instmem *imem, u32 npages, u32 align,
/* present memory for being mapped using small pages */ node->r.type = 12; - node->r.offset = node->handle >> 12; + node->r.offset = imem->iommu_mask | node->handle >> 12; node->r.length = (npages << PAGE_SHIFT) >> 12;
node->base.mn = &node->r; @@ -493,7 +495,7 @@ gk20a_instobj_ctor_iommu(struct gk20a_instmem *imem, u32 npages, u32 align, mutex_lock(imem->mm_mutex); /* Reserve area from GPU address space */ ret = nvkm_mm_head(imem->mm, 0, 1, npages, npages, - align >> imem->iommu_pgshift, &r); + align >> imem->page_shift, &r); mutex_unlock(imem->mm_mutex); if (ret) { nvkm_error(subdev, "IOMMU space is full!\n"); @@ -502,7 +504,7 @@ gk20a_instobj_ctor_iommu(struct gk20a_instmem *imem, u32 npages, u32 align,
/* Map into GPU address space */ for (i = 0; i < npages; i++) { - u32 offset = (r->offset + i) << imem->iommu_pgshift; + u32 offset = (r->offset + i) << imem->page_shift;
ret = iommu_map(imem->domain, offset, node->dma_addrs[i], PAGE_SIZE, IOMMU_READ | IOMMU_WRITE); @@ -518,7 +520,7 @@ gk20a_instobj_ctor_iommu(struct gk20a_instmem *imem, u32 npages, u32 align, }
/* IOMMU bit tells that an address is to be resolved through the IOMMU */ - r->offset |= BIT(imem->iommu_bit - imem->iommu_pgshift); + r->offset |= imem->iommu_mask;
node->base.mn = r; return 0; @@ -619,11 +621,12 @@ gk20a_instmem_new(struct nvkm_device *device, int index, imem->mm_mutex = &tdev->iommu.mutex; imem->mm = &tdev->iommu.mm; imem->domain = tdev->iommu.domain; - imem->iommu_pgshift = tdev->iommu.pgshift; - imem->iommu_bit = tdev->func->iommu_bit; + imem->page_shift = tdev->iommu.pgshift;
nvkm_info(&imem->base.subdev, "using IOMMU\n"); } else { + imem->page_shift = PAGE_SHIFT; + imem->attrs = DMA_ATTR_NON_CONSISTENT | DMA_ATTR_WEAK_ORDERING | DMA_ATTR_WRITE_COMBINE; @@ -631,5 +634,17 @@ gk20a_instmem_new(struct nvkm_device *device, int index, nvkm_info(&imem->base.subdev, "using DMA API\n"); }
+ /* + * The IOMMU mask needs to be set if an IOMMU is used explicitly (via + * direct IOMMU API usage) or implicitly (via the DMA API). In both + * cases the device will have been attached to an IOMMU domain. + */ + if (iommu_get_domain_for_dev(device->dev)) { + imem->iommu_mask = BIT_ULL(tdev->func->iommu_bit - + imem->page_shift); + nvkm_debug(&imem->base.subdev, "IOMMU mask: %016llx\n", + imem->iommu_mask); + } + return 0; }
From: Thierry Reding treding@nvidia.com
The GPU integrated in NVIDIA Tegra SoCs is connected to system memory via two paths: one direct path to the memory controller and another path that goes through a system MMU first. It's not typically necessary to go through the system MMU because the GPU's MMU can already map buffers so that they appear contiguous to the GPU.
However, in order to support big pages, the system MMU has to be used to combine multiple small pages into one virtually contiguous chunk so that the GPU can then treat that as a single big page.
In order to prepare for big page support, implement a custom MMU class that takes care of setting the IOMMU bit when writing page tables and when appropriate.
This is also necessary to make sure that Nouveau works correctly on Tegra devices where the GPU is connected to a system MMU and that IOMMU is used to back the DMA API. Currently Nouveau assumes that the DMA API is never backed by an IOMMU, so access to DMA-mapped buffers fault when suddenly this assumption is no longer true.
One situation where this can happen is on 32-bit Tegra SoCs where the ARM architecture code automatically attaches the GPU with a DMA/IOMMU domain. This is currently worked around by detaching the GPU from the IOMMU domain at probe time. However, with Tegra186 and later this can now also happen, but unfortunately no mechanism exists to detach from the domain in the 64-bit ARM architecture code.
Using this Tegra-specific MMU class ensures that DMA-mapped buffers are properly mapped (with the IOMMU bit set) if the DMA API is backed by an IOMMU domain.
Signed-off-by: Thierry Reding treding@nvidia.com --- .../gpu/drm/nouveau/nvkm/subdev/mmu/gk20a.c | 50 ++++++++++++++++++- .../gpu/drm/nouveau/nvkm/subdev/mmu/gk20a.h | 44 ++++++++++++++++ .../gpu/drm/nouveau/nvkm/subdev/mmu/gm20b.c | 6 ++- .../gpu/drm/nouveau/nvkm/subdev/mmu/gp10b.c | 4 +- drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h | 1 + .../drm/nouveau/nvkm/subdev/mmu/vmmgk20a.c | 22 +++++++- .../drm/nouveau/nvkm/subdev/mmu/vmmgm20b.c | 4 +- .../drm/nouveau/nvkm/subdev/mmu/vmmgp10b.c | 20 +++++++- 8 files changed, 142 insertions(+), 9 deletions(-) create mode 100644 drivers/gpu/drm/nouveau/nvkm/subdev/mmu/gk20a.h
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/gk20a.c b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/gk20a.c index ac74965a60d4..d9a5e05b7dc7 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/gk20a.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/gk20a.c @@ -19,11 +19,59 @@ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR * OTHER DEALINGS IN THE SOFTWARE. */ + +#include "gk20a.h" #include "mem.h" #include "vmm.h"
+#include <nvkm/core/tegra.h> #include <nvif/class.h>
+static void +gk20a_mmu_ctor(const struct nvkm_mmu_func *func, struct nvkm_device *device, + int index, struct gk20a_mmu *mmu) +{ + struct iommu_domain *domain = iommu_get_domain_for_dev(device->dev); + struct nvkm_device_tegra *tegra = device->func->tegra(device); + + nvkm_mmu_ctor(func, device, index, &mmu->base); + + /* + * If the DMA API is backed by an IOMMU, make sure the IOMMU bit is + * set for all buffer accesses. If the IOMMU is explicitly used, it + * is only used for instance blocks and the MMU doesn't care, since + * buffer objects are only mapped through the MMU, not through the + * IOMMU. + * + * Big page support could be implemented using explicit IOMMU usage, + * but the DMA API already provides that for free, so we don't worry + * about it for now. + */ + if (domain && !tegra->iommu.domain) { + mmu->iommu_mask = BIT_ULL(tegra->func->iommu_bit); + nvkm_debug(&mmu->base.subdev, "IOMMU mask: %llx\n", + mmu->iommu_mask); + } +} + +int +gk20a_mmu_new_(const struct nvkm_mmu_func *func, struct nvkm_device *device, + int index, struct nvkm_mmu **pmmu) +{ + struct gk20a_mmu *mmu; + + mmu = kzalloc(sizeof(*mmu), GFP_KERNEL); + if (!mmu) + return -ENOMEM; + + gk20a_mmu_ctor(func, device, index, mmu); + + if (pmmu) + *pmmu = &mmu->base; + + return 0; +} + static const struct nvkm_mmu_func gk20a_mmu = { .dma_bits = 40, @@ -37,5 +85,5 @@ gk20a_mmu = { int gk20a_mmu_new(struct nvkm_device *device, int index, struct nvkm_mmu **pmmu) { - return nvkm_mmu_new_(&gk20a_mmu, device, index, pmmu); + return gk20a_mmu_new_(&gk20a_mmu, device, index, pmmu); } diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/gk20a.h b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/gk20a.h new file mode 100644 index 000000000000..bb81fc62509c --- /dev/null +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/gk20a.h @@ -0,0 +1,44 @@ +/* + * Copyright (c) 2019 NVIDIA Corporation. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR + * OTHER DEALINGS IN THE SOFTWARE. + */ + +#ifndef __NVKM_MMU_GK20A_H__ +#define __NVKM_MMU_GK20A_H__ + +#include "priv.h" + +struct gk20a_mmu { + struct nvkm_mmu base; + + /* + * If an IOMMU is used, indicates which address bit will trigger an + * IOMMU translation when set (when this bit is not set, the IOMMU is + * bypassed). A value of 0 means an IOMMU is never used. + */ + u64 iommu_mask; +}; + +#define gk20a_mmu(mmu) container_of(mmu, struct gk20a_mmu, base) + +int gk20a_mmu_new_(const struct nvkm_mmu_func *, struct nvkm_device *, + int index, struct nvkm_mmu **); + +#endif diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/gm20b.c b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/gm20b.c index 7353a94b4091..7fccd4df52a8 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/gm20b.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/gm20b.c @@ -19,6 +19,8 @@ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR * OTHER DEALINGS IN THE SOFTWARE. */ + +#include "gk20a.h" #include "mem.h" #include "vmm.h"
@@ -50,6 +52,6 @@ int gm20b_mmu_new(struct nvkm_device *device, int index, struct nvkm_mmu **pmmu) { if (device->fb->page) - return nvkm_mmu_new_(&gm20b_mmu_fixed, device, index, pmmu); - return nvkm_mmu_new_(&gm20b_mmu, device, index, pmmu); + return gk20a_mmu_new_(&gm20b_mmu_fixed, device, index, pmmu); + return gk20a_mmu_new_(&gm20b_mmu, device, index, pmmu); } diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/gp10b.c b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/gp10b.c index 0a50be9a785a..ae3cb47be3d8 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/gp10b.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/gp10b.c @@ -19,6 +19,8 @@ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR * OTHER DEALINGS IN THE SOFTWARE. */ + +#include "gk20a.h" #include "mem.h" #include "vmm.h"
@@ -41,5 +43,5 @@ gp10b_mmu_new(struct nvkm_device *device, int index, struct nvkm_mmu **pmmu) { if (!nvkm_boolopt(device->cfgopt, "GP100MmuLayout", true)) return gm20b_mmu_new(device, index, pmmu); - return nvkm_mmu_new_(&gp10b_mmu, device, index, pmmu); + return gk20a_mmu_new_(&gp10b_mmu, device, index, pmmu); } diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h index 5e55ecbd8005..fb3a9e8bb9cd 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.h @@ -213,6 +213,7 @@ void gf100_vmm_invalidate(struct nvkm_vmm *, u32 type); void gf100_vmm_invalidate_pdb(struct nvkm_vmm *, u64 addr);
int gk20a_vmm_aper(enum nvkm_memory_target); +int gk20a_vmm_valid(struct nvkm_vmm *, void *, u32, struct nvkm_vmm_map *);
int gm200_vmm_new_(const struct nvkm_vmm_func *, const struct nvkm_vmm_func *, struct nvkm_mmu *, bool, u64, u64, void *, u32, diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgk20a.c b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgk20a.c index 5a9582dce970..16d7bf727292 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgk20a.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgk20a.c @@ -19,6 +19,8 @@ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR * OTHER DEALINGS IN THE SOFTWARE. */ + +#include "gk20a.h" #include "vmm.h"
#include <core/memory.h> @@ -33,12 +35,28 @@ gk20a_vmm_aper(enum nvkm_memory_target target) } }
+int +gk20a_vmm_valid(struct nvkm_vmm *vmm, void *argv, u32 argc, + struct nvkm_vmm_map *map) +{ + struct gk20a_mmu *mmu = gk20a_mmu(vmm->mmu); + int ret; + + ret = gf100_vmm_valid(vmm, argv, argc, map); + if (ret < 0) + return ret; + + map->type |= mmu->iommu_mask >> 8; + + return 0; +} + static const struct nvkm_vmm_func gk20a_vmm_17 = { .join = gf100_vmm_join, .part = gf100_vmm_part, .aper = gf100_vmm_aper, - .valid = gf100_vmm_valid, + .valid = gk20a_vmm_valid, .flush = gf100_vmm_flush, .invalidate_pdb = gf100_vmm_invalidate_pdb, .page = { @@ -53,7 +71,7 @@ gk20a_vmm_16 = { .join = gf100_vmm_join, .part = gf100_vmm_part, .aper = gf100_vmm_aper, - .valid = gf100_vmm_valid, + .valid = gk20a_vmm_valid, .flush = gf100_vmm_flush, .invalidate_pdb = gf100_vmm_invalidate_pdb, .page = { diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgm20b.c b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgm20b.c index 96b759695dd8..7a6066d886cd 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgm20b.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgm20b.c @@ -26,7 +26,7 @@ gm20b_vmm_17 = { .join = gm200_vmm_join, .part = gf100_vmm_part, .aper = gk20a_vmm_aper, - .valid = gf100_vmm_valid, + .valid = gk20a_vmm_valid, .flush = gf100_vmm_flush, .invalidate_pdb = gf100_vmm_invalidate_pdb, .page = { @@ -42,7 +42,7 @@ gm20b_vmm_16 = { .join = gm200_vmm_join, .part = gf100_vmm_part, .aper = gk20a_vmm_aper, - .valid = gf100_vmm_valid, + .valid = gk20a_vmm_valid, .flush = gf100_vmm_flush, .invalidate_pdb = gf100_vmm_invalidate_pdb, .page = { diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp10b.c b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp10b.c index e081239afe58..180c8f006e32 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp10b.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmmgp10b.c @@ -19,14 +19,32 @@ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR * OTHER DEALINGS IN THE SOFTWARE. */ + +#include "gk20a.h" #include "vmm.h"
+static int +gp10b_vmm_valid(struct nvkm_vmm *vmm, void *argv, u32 argc, + struct nvkm_vmm_map *map) +{ + struct gk20a_mmu *mmu = gk20a_mmu(vmm->mmu); + int ret; + + ret = gp100_vmm_valid(vmm, argv, argc, map); + if (ret < 0) + return ret; + + map->type |= mmu->iommu_mask >> 4; + + return 0; +} + static const struct nvkm_vmm_func gp10b_vmm = { .join = gp100_vmm_join, .part = gf100_vmm_part, .aper = gk20a_vmm_aper, - .valid = gp100_vmm_valid, + .valid = gp10b_vmm_valid, .flush = gp100_vmm_flush, .mthd = gp100_vmm_mthd, .invalidate_pdb = gp100_vmm_invalidate_pdb,
From: Thierry Reding treding@nvidia.com
If the GPU is already attached to an IOMMU, don't detach it and setup an explicit IOMMU domain. Since Nouveau can now properly handle the case of the DMA API being backed by an IOMMU, just continue using the DMA API.
Signed-off-by: Thierry Reding treding@nvidia.com --- .../drm/nouveau/nvkm/engine/device/tegra.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c index d0d52c1d4aee..fc652aaa41c7 100644 --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c @@ -23,10 +23,6 @@ #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER #include "priv.h"
-#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) -#include <asm/dma-iommu.h> -#endif - static int nvkm_device_tegra_power_up(struct nvkm_device_tegra *tdev) { @@ -109,14 +105,13 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra *tdev) unsigned long pgsize_bitmap; int ret;
-#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) - if (dev->archdata.mapping) { - struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev); - - arm_iommu_detach_device(dev); - arm_iommu_release_mapping(mapping); - } -#endif + /* + * Skip explicit IOMMU initialization if the GPU is already attached + * to an IOMMU domain. This can happen if the DMA API is backed by an + * IOMMU. + */ + if (iommu_get_domain_for_dev(dev)) + return;
if (!tdev->func->iommu_bit) return;
Hi Thierry,
On 16/09/2019 16:04, Thierry Reding wrote:
From: Thierry Reding treding@nvidia.com
If the GPU is already attached to an IOMMU, don't detach it and setup an explicit IOMMU domain. Since Nouveau can now properly handle the case of the DMA API being backed by an IOMMU, just continue using the DMA API.
Signed-off-by: Thierry Reding treding@nvidia.com
.../drm/nouveau/nvkm/engine/device/tegra.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c index d0d52c1d4aee..fc652aaa41c7 100644 --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c @@ -23,10 +23,6 @@ #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER #include "priv.h"
-#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) -#include <asm/dma-iommu.h> -#endif
- static int nvkm_device_tegra_power_up(struct nvkm_device_tegra *tdev) {
@@ -109,14 +105,13 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra *tdev) unsigned long pgsize_bitmap; int ret;
-#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
- if (dev->archdata.mapping) {
struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);arm_iommu_detach_device(dev);arm_iommu_release_mapping(mapping);- }
-#endif
- /*
* Skip explicit IOMMU initialization if the GPU is already attached* to an IOMMU domain. This can happen if the DMA API is backed by an* IOMMU.*/- if (iommu_get_domain_for_dev(dev))
return;
Beware of "iommu.passthrough=1" - you could get a valid default domain here yet still have direct/SWIOTLB DMA ops. I guess you probably want to double-check the domain type as well.
Robin.
if (!tdev->func->iommu_bit) return;
On Mon, Sep 16, 2019 at 04:29:18PM +0100, Robin Murphy wrote:
Hi Thierry,
On 16/09/2019 16:04, Thierry Reding wrote:
From: Thierry Reding treding@nvidia.com
If the GPU is already attached to an IOMMU, don't detach it and setup an explicit IOMMU domain. Since Nouveau can now properly handle the case of the DMA API being backed by an IOMMU, just continue using the DMA API.
Signed-off-by: Thierry Reding treding@nvidia.com
.../drm/nouveau/nvkm/engine/device/tegra.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c index d0d52c1d4aee..fc652aaa41c7 100644 --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c @@ -23,10 +23,6 @@ #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER #include "priv.h" -#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) -#include <asm/dma-iommu.h> -#endif
- static int nvkm_device_tegra_power_up(struct nvkm_device_tegra *tdev) {
@@ -109,14 +105,13 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra *tdev) unsigned long pgsize_bitmap; int ret; -#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
- if (dev->archdata.mapping) {
struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);arm_iommu_detach_device(dev);arm_iommu_release_mapping(mapping);- }
-#endif
- /*
* Skip explicit IOMMU initialization if the GPU is already attached* to an IOMMU domain. This can happen if the DMA API is backed by an* IOMMU.*/- if (iommu_get_domain_for_dev(dev))
return;Beware of "iommu.passthrough=1" - you could get a valid default domain here yet still have direct/SWIOTLB DMA ops. I guess you probably want to double-check the domain type as well.
Good point. An earlier version of this patch had an additional check for IOMMU_DOMAIN_DMA, but then that failed on 32-bit ARM because there the DMA API can also use IOMMU_DOMAIN_UNMANAGED type domains. Checking for IOMMU_DOMAIN_IDENTIFY should be safe, though. That doesn't seem to appear in arch/arm, arch/arm64 or drivers/iommu/dma-iommu.c.
Thierry
On 16/09/2019 16:57, Thierry Reding wrote:
On Mon, Sep 16, 2019 at 04:29:18PM +0100, Robin Murphy wrote:
Hi Thierry,
On 16/09/2019 16:04, Thierry Reding wrote:
From: Thierry Reding treding@nvidia.com
If the GPU is already attached to an IOMMU, don't detach it and setup an explicit IOMMU domain. Since Nouveau can now properly handle the case of the DMA API being backed by an IOMMU, just continue using the DMA API.
Signed-off-by: Thierry Reding treding@nvidia.com
.../drm/nouveau/nvkm/engine/device/tegra.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c index d0d52c1d4aee..fc652aaa41c7 100644 --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c @@ -23,10 +23,6 @@ #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER #include "priv.h" -#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) -#include <asm/dma-iommu.h> -#endif
- static int nvkm_device_tegra_power_up(struct nvkm_device_tegra *tdev) {
@@ -109,14 +105,13 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra *tdev) unsigned long pgsize_bitmap; int ret; -#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
- if (dev->archdata.mapping) {
struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);arm_iommu_detach_device(dev);arm_iommu_release_mapping(mapping);- }
-#endif
- /*
* Skip explicit IOMMU initialization if the GPU is already attached* to an IOMMU domain. This can happen if the DMA API is backed by an* IOMMU.*/- if (iommu_get_domain_for_dev(dev))
return;Beware of "iommu.passthrough=1" - you could get a valid default domain here yet still have direct/SWIOTLB DMA ops. I guess you probably want to double-check the domain type as well.
Good point. An earlier version of this patch had an additional check for IOMMU_DOMAIN_DMA, but then that failed on 32-bit ARM because there the DMA API can also use IOMMU_DOMAIN_UNMANAGED type domains. Checking for IOMMU_DOMAIN_IDENTIFY should be safe, though. That doesn't seem to appear in arch/arm, arch/arm64 or drivers/iommu/dma-iommu.c.
Right, "domain && domain->type != IOMMU_DOMAIN_IDENTITY" should be sufficient to answer "is the DMA layer managing my address space for me?" unless and until some massive API change happens (which I certainly don't foresee).
Robin.
On Mon, Sep 16, 2019 at 05:15:25PM +0100, Robin Murphy wrote:
On 16/09/2019 16:57, Thierry Reding wrote:
On Mon, Sep 16, 2019 at 04:29:18PM +0100, Robin Murphy wrote:
Hi Thierry,
On 16/09/2019 16:04, Thierry Reding wrote:
From: Thierry Reding treding@nvidia.com
If the GPU is already attached to an IOMMU, don't detach it and setup an explicit IOMMU domain. Since Nouveau can now properly handle the case of the DMA API being backed by an IOMMU, just continue using the DMA API.
Signed-off-by: Thierry Reding treding@nvidia.com
.../drm/nouveau/nvkm/engine/device/tegra.c | 19 +++++++------------ 1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c index d0d52c1d4aee..fc652aaa41c7 100644 --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c @@ -23,10 +23,6 @@ #ifdef CONFIG_NOUVEAU_PLATFORM_DRIVER #include "priv.h" -#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) -#include <asm/dma-iommu.h> -#endif
- static int nvkm_device_tegra_power_up(struct nvkm_device_tegra *tdev) {
@@ -109,14 +105,13 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra *tdev) unsigned long pgsize_bitmap; int ret; -#if IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)
- if (dev->archdata.mapping) {
struct dma_iommu_mapping *mapping = to_dma_iommu_mapping(dev);arm_iommu_detach_device(dev);arm_iommu_release_mapping(mapping);- }
-#endif
- /*
* Skip explicit IOMMU initialization if the GPU is already attached* to an IOMMU domain. This can happen if the DMA API is backed by an* IOMMU.*/- if (iommu_get_domain_for_dev(dev))
return;Beware of "iommu.passthrough=1" - you could get a valid default domain here yet still have direct/SWIOTLB DMA ops. I guess you probably want to double-check the domain type as well.
Good point. An earlier version of this patch had an additional check for IOMMU_DOMAIN_DMA, but then that failed on 32-bit ARM because there the DMA API can also use IOMMU_DOMAIN_UNMANAGED type domains. Checking for IOMMU_DOMAIN_IDENTIFY should be safe, though. That doesn't seem to appear in arch/arm, arch/arm64 or drivers/iommu/dma-iommu.c.
Right, "domain && domain->type != IOMMU_DOMAIN_IDENTITY" should be sufficient to answer "is the DMA layer managing my address space for me?" unless and until some massive API change happens (which I certainly don't foresee).
Might be a good idea to roll that up into a function to have a standard way for drivers to check for this rather than open-coding the same condition everywhere (and maybe get things wrong). As an additional advantage, if that massive API change ever does happen we don't have to go and update all the callers.
Something like this perhaps?
static inline bool iommu_managed(struct device *dev) { struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
return domain && domain->type != IOMMU_DOMAIN_UNMANAGED; }
Thierry
From: Thierry Reding treding@nvidia.com
The GPU can usually address more than 32-bit, even without being attached to an IOMMU. However, if the GPU is not attached to an IOMMU, it's likely that there is no IOMMU in the system, in which case any buffers allocated by Nouveau will likely end up in a region of memory that cannot be accessed by host1x.
Signed-off-by: Thierry Reding treding@nvidia.com --- .../drm/nouveau/nvkm/engine/device/tegra.c | 111 +++++++++++------- 1 file changed, 70 insertions(+), 41 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c index fc652aaa41c7..221238a2cf53 100644 --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/tegra.c @@ -97,7 +97,7 @@ nvkm_device_tegra_power_down(struct nvkm_device_tegra *tdev) return 0; }
-static void +static int nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra *tdev) { #if IS_ENABLED(CONFIG_IOMMU_API) @@ -111,47 +111,65 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra *tdev) * IOMMU. */ if (iommu_get_domain_for_dev(dev)) - return; + return -ENODEV;
if (!tdev->func->iommu_bit) - return; + return -ENODEV; + + if (!iommu_present(&platform_bus_type)) + return -ENODEV;
mutex_init(&tdev->iommu.mutex);
- if (iommu_present(&platform_bus_type)) { - tdev->iommu.domain = iommu_domain_alloc(&platform_bus_type); - if (!tdev->iommu.domain) - goto error; + tdev->iommu.domain = iommu_domain_alloc(&platform_bus_type); + if (!tdev->iommu.domain) + return -ENOMEM;
- /* - * A IOMMU is only usable if it supports page sizes smaller - * or equal to the system's PAGE_SIZE, with a preference if - * both are equal. - */ - pgsize_bitmap = tdev->iommu.domain->ops->pgsize_bitmap; - if (pgsize_bitmap & PAGE_SIZE) { - tdev->iommu.pgshift = PAGE_SHIFT; - } else { - tdev->iommu.pgshift = fls(pgsize_bitmap & ~PAGE_MASK); - if (tdev->iommu.pgshift == 0) { - dev_warn(dev, "unsupported IOMMU page size\n"); - goto free_domain; - } - tdev->iommu.pgshift -= 1; + /* + * An IOMMU is only usable if it supports page sizes smaller or equal + * to the system's PAGE_SIZE, with a preference if both are equal. + */ + pgsize_bitmap = tdev->iommu.domain->ops->pgsize_bitmap; + if (pgsize_bitmap & PAGE_SIZE) { + tdev->iommu.pgshift = PAGE_SHIFT; + } else { + tdev->iommu.pgshift = fls(pgsize_bitmap & ~PAGE_MASK); + if (tdev->iommu.pgshift == 0) { + dev_warn(dev, "unsupported IOMMU page size\n"); + ret = -ENOTSUPP; + goto free_domain; }
- ret = iommu_attach_device(tdev->iommu.domain, dev); - if (ret) - goto free_domain; + tdev->iommu.pgshift -= 1; + }
- ret = nvkm_mm_init(&tdev->iommu.mm, 0, 0, - (1ULL << tdev->func->iommu_bit) >> - tdev->iommu.pgshift, 1); - if (ret) - goto detach_device; + ret = iommu_attach_device(tdev->iommu.domain, dev); + if (ret) { + dev_warn(dev, "failed to attach to IOMMU: %d\n", ret); + goto free_domain; + } + + ret = nvkm_mm_init(&tdev->iommu.mm, 0, 0, + (1ULL << tdev->func->iommu_bit) >> + tdev->iommu.pgshift, 1); + if (ret) { + dev_warn(dev, "failed to initialize IOVA space: %d\n", ret); + goto detach_device; + } + + /* + * The IOMMU bit defines the upper limit of the GPU-addressable space. + */ + ret = dma_set_mask(dev, DMA_BIT_MASK(tdev->func->iommu_bit)); + if (ret) { + dev_warn(dev, "failed to set DMA mask: %d\n", ret); + goto fini_mm; }
- return; + return 0; + +fini_mm: + nvkm_mm_fini(&tdev->iommu.mm);
detach_device: iommu_detach_device(tdev->iommu.domain, dev); @@ -159,10 +177,15 @@ nvkm_device_tegra_probe_iommu(struct nvkm_device_tegra *tdev) free_domain: iommu_domain_free(tdev->iommu.domain);
-error: + /* reset these so that the DMA API code paths are executed */ tdev->iommu.domain = NULL; tdev->iommu.pgshift = 0; - dev_err(dev, "cannot initialize IOMMU MM\n"); + + dev_warn(dev, "cannot initialize IOMMU MM\n"); + + return ret; +#else + return -ENOTSUPP; #endif }
@@ -327,14 +350,20 @@ nvkm_device_tegra_new(const struct nvkm_device_tegra_func *func, goto free; }
- /** - * The IOMMU bit defines the upper limit of the GPU-addressable space. - */ - ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(tdev->func->iommu_bit)); - if (ret) - goto free; - - nvkm_device_tegra_probe_iommu(tdev); + ret = nvkm_device_tegra_probe_iommu(tdev); + if (ret) { + /* + * If we fail to set up an IOMMU, fall back to a 32-bit DMA + * mask. This is not necessary for the GPU to work because it + * can usually address all of system memory. However, if the + * buffers allocated by Nouveau are meant to be shared with + * the display controller, we need to restrict where they can + * come from. + */ + ret = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32)); + if (ret) + goto free; + }
ret = nvkm_device_tegra_power_up(tdev); if (ret)
From: Alexandre Courbot acourbot@nvidia.com
Enable the GPU node for the Jetson TX2 board.
Signed-off-by: Alexandre Courbot acourbot@nvidia.com Signed-off-by: Thierry Reding treding@nvidia.com --- arch/arm64/boot/dts/nvidia/tegra186-p2771-0000.dts | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/arch/arm64/boot/dts/nvidia/tegra186-p2771-0000.dts b/arch/arm64/boot/dts/nvidia/tegra186-p2771-0000.dts index bdace01561ba..6f7c7c4c5c29 100644 --- a/arch/arm64/boot/dts/nvidia/tegra186-p2771-0000.dts +++ b/arch/arm64/boot/dts/nvidia/tegra186-p2771-0000.dts @@ -276,6 +276,10 @@ }; };
+ gpu@17000000 { + status = "okay"; + }; + gpio-keys { compatible = "gpio-keys";
From: Thierry Reding treding@nvidia.com
The GPU has a connection to the ARM SMMU found on Tegra186, which can be used to support large pages. Make sure the GPU is attached to the SMMU to take advantage of its capabilities.
Signed-off-by: Thierry Reding treding@nvidia.com --- arch/arm64/boot/dts/nvidia/tegra186.dtsi | 1 + 1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi index 47cd831fcf44..171fd4dfa58d 100644 --- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi +++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi @@ -1172,6 +1172,7 @@ status = "disabled";
power-domains = <&bpmp TEGRA186_POWER_DOMAIN_GPU>; + iommus = <&smmu TEGRA186_SID_GPU>; };
sysram@30000000 {
dri-devel@lists.freedesktop.org
-
Ben Dooks -
Ben Skeggs -
Joerg Roedel -
Robin Murphy -
Thierry Reding