[PATCH 00/11] drm/nouveau: Enable GP10B by default

[PATCH 00/11] drm/nouveau: Enable GP10B by default

[Thierry Reding]

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

-- 
2.23.0

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH 01/11] drm/nouveau: tegra: Avoid pulsing reset twice

[Thierry Reding]

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;
 
-- 
2.23.0

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH 02/11] drm/nouveau: tegra: Set clock rate if not set

[Thierry Reding]

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)) {
-- 
2.23.0

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH 05/11] drm/nouveau: gp10b: Use correct copy engine

[Thierry Reding]

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,
-- 
2.23.0

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH 06/11] drm/nouveau: gk20a: Set IOMMU bit for DMA API if appropriate

[Thierry Reding]

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;
 }
-- 
2.23.0

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH 07/11] drm/nouveau: gk20a: Implement custom MMU class

[Thierry Reding]

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,
-- 
2.23.0

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH 08/11] drm/nouveau: tegra: Skip IOMMU initialization if already attached

[Thierry Reding]

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;
-- 
2.23.0

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 08/11] drm/nouveau: tegra: Skip IOMMU initialization if already attached

[Robin Murphy]

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;
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 08/11] drm/nouveau: tegra: Skip IOMMU initialization if already attached

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2415 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 08/11] drm/nouveau: tegra: Skip IOMMU initialization if already attached

[Robin Murphy]

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.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 08/11] drm/nouveau: tegra: Skip IOMMU initialization if already attached

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3542 bytes --]

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 09/11] drm/nouveau: tegra: Fall back to 32-bit DMA mask without IOMMU

[Thierry Reding]

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)
-- 
2.23.0

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH 10/11] arm64: tegra: Enable GPU on Jetson TX2

[Thierry Reding]

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";
 
-- 
2.23.0

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH 03/11] drm/nouveau: secboot: Read WPR configuration from GPU registers

[Thierry Reding]

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;
 
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 03/11] drm/nouveau: secboot: Read WPR configuration from GPU registers

[Ben Skeggs]

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
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 03/11] drm/nouveau: secboot: Read WPR configuration from GPU registers

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 9737 bytes --]

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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> >
> > 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-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> >  .../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-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

Re: [PATCH 03/11] drm/nouveau: secboot: Read WPR configuration from GPU registers

[Ben Skeggs]

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
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[PATCH 04/11] drm/nouveau: gp10b: Add custom L2 cache implementation

[Thierry Reding]

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

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 04/11] drm/nouveau: gp10b: Add custom L2 cache implementation

[Ben Dooks]

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.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

https://www.codethink.co.uk/privacy.html
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 04/11] drm/nouveau: gp10b: Add custom L2 cache implementation

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 6433 bytes --]

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-thierry.reding@gmail.com/

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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 04/11] drm/nouveau: gp10b: Add custom L2 cache implementation

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 6819 bytes --]

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-thierry.reding@gmail.com/
> 
> 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

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 04/11] drm/nouveau: gp10b: Add custom L2 cache implementation

[Joerg Roedel]

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-thierry.reding@gmail.com/
> > 
> > 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 11/11] arm64: tegra: Enable SMMU for GPU on Tegra186

[Thierry Reding]

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 {
-- 
2.23.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel