From: Thierry Reding treding@nvidia.com
Hi Ben,
I messed up the ordering of patches in my tree a bit, so these two fixes got separated from the others. I don't consider these particularily urgent because the crash that the first one fixes only happens on gp10b which we don't enable by default yet and the second patch fixes a crash that only happens on module unload (or driver unbind, more accurately), which isn't a terribly common thing to do.
I'll be sending out fixes shortly to make the GP10B work more properly on a wider range of Jetson TX2 devices and enable it by default.
One thing to mention is that I'm not exactly sure if the first patch is the right thing to do. I haven't seen any issues after that change, but I'm also not exactly sure I understand what BAR2 is used for, so I don't know if I would've even covered those code paths (other than the one causing the crash at probe time) in my tests.
It'd be great to get Lyude's feedback on the second patch, since that call to pci_disable_device() was rather oddly placed and I'm not sure if that was essential for things to work or whether the slightly different point in time where it's called after this patch is also okay. It looks to me like it should work fine, but I don't currently have a way to test this on desktop GPUs.
Thierry
Thierry Reding (2): drm/nouveau: tegra: Fix NULL pointer dereference drm/nouveau: tegra: Do not try to disable PCI device
drivers/gpu/drm/nouveau/nouveau_drm.c | 3 +- .../drm/nouveau/nvkm/subdev/instmem/gk20a.c | 30 +++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-)
From: Thierry Reding treding@nvidia.com
Fill in BAR2 callbacks for instance memory. There's no BAR2 on Tegra GPUs, but buffers are all in system memory anyway, so just return the plain address.
Signed-off-by: Thierry Reding treding@nvidia.com --- .../drm/nouveau/nvkm/subdev/instmem/gk20a.c | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c index 985f2990ab0d..b0493f8df1fe 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c @@ -261,6 +261,34 @@ gk20a_instobj_release_iommu(struct nvkm_memory *memory) nvkm_ltc_invalidate(ltc); }
+static u64 +gk20a_instobj_bar2_dma(struct nvkm_memory *memory) +{ + struct gk20a_instobj_dma *iobj = gk20a_instobj_dma(memory); + u64 addr = ~0ULL; + + if (gk20a_instobj_acquire_dma(&iobj->base.memory)) + addr = gk20a_instobj_addr(&iobj->base.memory); + + gk20a_instobj_release_dma(&iobj->base.memory); + + return addr; +} + +static u64 +gk20a_instobj_bar2_iommu(struct nvkm_memory *memory) +{ + struct gk20a_instobj_iommu *iobj = gk20a_instobj_iommu(memory); + u64 addr = ~0ULL; + + if (gk20a_instobj_acquire_iommu(&iobj->base.memory)) + addr = gk20a_instobj_addr(&iobj->base.memory); + + gk20a_instobj_release_iommu(&iobj->base.memory); + + return addr; +} + static u32 gk20a_instobj_rd32(struct nvkm_memory *memory, u64 offset) { @@ -353,6 +381,7 @@ static const struct nvkm_memory_func gk20a_instobj_func_dma = { .dtor = gk20a_instobj_dtor_dma, .target = gk20a_instobj_target, + .bar2 = gk20a_instobj_bar2_dma, .page = gk20a_instobj_page, .addr = gk20a_instobj_addr, .size = gk20a_instobj_size, @@ -365,6 +394,7 @@ static const struct nvkm_memory_func gk20a_instobj_func_iommu = { .dtor = gk20a_instobj_dtor_iommu, .target = gk20a_instobj_target, + .bar2 = gk20a_instobj_bar2_iommu, .page = gk20a_instobj_page, .addr = gk20a_instobj_addr, .size = gk20a_instobj_size,
Not sure I understand enough about why BAR2 is needed or what it's used for to give a proper review on this one, probably better to wait for skeggsb to take a look at this
On Mon, 2019-09-16 at 16:36 +0200, Thierry Reding wrote:
From: Thierry Reding treding@nvidia.com
Fill in BAR2 callbacks for instance memory. There's no BAR2 on Tegra GPUs, but buffers are all in system memory anyway, so just return the plain address.
Signed-off-by: Thierry Reding treding@nvidia.com
.../drm/nouveau/nvkm/subdev/instmem/gk20a.c | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c index 985f2990ab0d..b0493f8df1fe 100644 --- a/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/instmem/gk20a.c @@ -261,6 +261,34 @@ gk20a_instobj_release_iommu(struct nvkm_memory *memory) nvkm_ltc_invalidate(ltc); }
+static u64 +gk20a_instobj_bar2_dma(struct nvkm_memory *memory) +{
- struct gk20a_instobj_dma *iobj = gk20a_instobj_dma(memory);
- u64 addr = ~0ULL;
- if (gk20a_instobj_acquire_dma(&iobj->base.memory))
addr = gk20a_instobj_addr(&iobj->base.memory);- gk20a_instobj_release_dma(&iobj->base.memory);
- return addr;
+}
+static u64 +gk20a_instobj_bar2_iommu(struct nvkm_memory *memory) +{
- struct gk20a_instobj_iommu *iobj = gk20a_instobj_iommu(memory);
- u64 addr = ~0ULL;
- if (gk20a_instobj_acquire_iommu(&iobj->base.memory))
addr = gk20a_instobj_addr(&iobj->base.memory);- gk20a_instobj_release_iommu(&iobj->base.memory);
- return addr;
+}
static u32 gk20a_instobj_rd32(struct nvkm_memory *memory, u64 offset) { @@ -353,6 +381,7 @@ static const struct nvkm_memory_func gk20a_instobj_func_dma = { .dtor = gk20a_instobj_dtor_dma, .target = gk20a_instobj_target,
- .bar2 = gk20a_instobj_bar2_dma, .page = gk20a_instobj_page, .addr = gk20a_instobj_addr, .size = gk20a_instobj_size,
@@ -365,6 +394,7 @@ static const struct nvkm_memory_func gk20a_instobj_func_iommu = { .dtor = gk20a_instobj_dtor_iommu, .target = gk20a_instobj_target,
- .bar2 = gk20a_instobj_bar2_iommu, .page = gk20a_instobj_page, .addr = gk20a_instobj_addr, .size = gk20a_instobj_size,
From: Thierry Reding treding@nvidia.com
When Nouveau is instantiated on top of a platform device, the dev->pdev field will be NULL and calling pci_disable_device() will crash. Move the PCI disabling code to the PCI specific driver removal code.
Signed-off-by: Thierry Reding treding@nvidia.com --- drivers/gpu/drm/nouveau/nouveau_drm.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index 2cd83849600f..b65ae817eabf 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -715,7 +715,6 @@ static int nouveau_drm_probe(struct pci_dev *pdev, void nouveau_drm_device_remove(struct drm_device *dev) { - struct pci_dev *pdev = dev->pdev; struct nouveau_drm *drm = nouveau_drm(dev); struct nvkm_client *client; struct nvkm_device *device; @@ -727,7 +726,6 @@ nouveau_drm_device_remove(struct drm_device *dev) device = nvkm_device_find(client->device);
nouveau_drm_device_fini(dev); - pci_disable_device(pdev); drm_dev_put(dev); nvkm_device_del(&device); } @@ -738,6 +736,7 @@ nouveau_drm_remove(struct pci_dev *pdev) struct drm_device *dev = pci_get_drvdata(pdev);
nouveau_drm_device_remove(dev); + pci_disable_device(pdev); }
static int
I don't see any problems with this, since nvkm_device_del() is mainly in charge of just releasing memory allocations as is drm_dev_put().
Reviewed-by: Lyude Paul lyude@redhat.com
On Mon, 2019-09-16 at 16:36 +0200, Thierry Reding wrote:
From: Thierry Reding treding@nvidia.com
When Nouveau is instantiated on top of a platform device, the dev->pdev field will be NULL and calling pci_disable_device() will crash. Move the PCI disabling code to the PCI specific driver removal code.
Signed-off-by: Thierry Reding treding@nvidia.com
drivers/gpu/drm/nouveau/nouveau_drm.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index 2cd83849600f..b65ae817eabf 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -715,7 +715,6 @@ static int nouveau_drm_probe(struct pci_dev *pdev, void nouveau_drm_device_remove(struct drm_device *dev) {
- struct pci_dev *pdev = dev->pdev; struct nouveau_drm *drm = nouveau_drm(dev); struct nvkm_client *client; struct nvkm_device *device;
@@ -727,7 +726,6 @@ nouveau_drm_device_remove(struct drm_device *dev) device = nvkm_device_find(client->device);
nouveau_drm_device_fini(dev);
- pci_disable_device(pdev); drm_dev_put(dev); nvkm_device_del(&device);
} @@ -738,6 +736,7 @@ nouveau_drm_remove(struct pci_dev *pdev) struct drm_device *dev = pci_get_drvdata(pdev);
nouveau_drm_device_remove(dev);
- pci_disable_device(pdev);
}
static int
On Tue, 17 Sep 2019 at 00:36, Thierry Reding thierry.reding@gmail.com wrote:
From: Thierry Reding treding@nvidia.com
Hi Ben,
I messed up the ordering of patches in my tree a bit, so these two fixes got separated from the others. I don't consider these particularily urgent because the crash that the first one fixes only happens on gp10b which we don't enable by default yet and the second patch fixes a crash that only happens on module unload (or driver unbind, more accurately), which isn't a terribly common thing to do.
I'll be sending out fixes shortly to make the GP10B work more properly on a wider range of Jetson TX2 devices and enable it by default.
One thing to mention is that I'm not exactly sure if the first patch is the right thing to do. I haven't seen any issues after that change, but I'm also not exactly sure I understand what BAR2 is used for, so I don't know if I would've even covered those code paths (other than the one causing the crash at probe time) in my tests.
BAR2 on dGPUs is used to map kernel-level GPU objects in VRAM so they can be accessed by the driver. It's pretty much a smaller version of BAR1, but intended for a different purpose.
On dGPUs, there's a couple of places (fault buffer address, and fault method buffer on volta) where the GPU wants PRI regs to be poked with an offset within BAR2 rather than an aperture+offset combination. I'm not 100% sure what Tegra parts do here, but presumably if it's working for you, they're happy to just accept a system memory address instead.
I guess this would be the right thing to do here in that situation. The more obvious (from a "reading the code" POV) thing to do would be to write Tegra-specific versions of the functions that use nvkm_memory_bar2() to perform this mapping, and use nvkm_memory_addr() instead but I'm not sure if we need/want to go to that effort. It's conceivable it could be required at some point.
Ben.
It'd be great to get Lyude's feedback on the second patch, since that call to pci_disable_device() was rather oddly placed and I'm not sure if that was essential for things to work or whether the slightly different point in time where it's called after this patch is also okay. It looks to me like it should work fine, but I don't currently have a way to test this on desktop GPUs.
Thierry
Thierry Reding (2): drm/nouveau: tegra: Fix NULL pointer dereference drm/nouveau: tegra: Do not try to disable PCI device
drivers/gpu/drm/nouveau/nouveau_drm.c | 3 +- .../drm/nouveau/nvkm/subdev/instmem/gk20a.c | 30 +++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-)
-- 2.23.0
Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
On Tue, Sep 17, 2019 at 04:07:54PM +1000, Ben Skeggs wrote:
On Tue, 17 Sep 2019 at 00:36, Thierry Reding thierry.reding@gmail.com wrote:
From: Thierry Reding treding@nvidia.com
Hi Ben,
I messed up the ordering of patches in my tree a bit, so these two fixes got separated from the others. I don't consider these particularily urgent because the crash that the first one fixes only happens on gp10b which we don't enable by default yet and the second patch fixes a crash that only happens on module unload (or driver unbind, more accurately), which isn't a terribly common thing to do.
I'll be sending out fixes shortly to make the GP10B work more properly on a wider range of Jetson TX2 devices and enable it by default.
One thing to mention is that I'm not exactly sure if the first patch is the right thing to do. I haven't seen any issues after that change, but I'm also not exactly sure I understand what BAR2 is used for, so I don't know if I would've even covered those code paths (other than the one causing the crash at probe time) in my tests.
BAR2 on dGPUs is used to map kernel-level GPU objects in VRAM so they can be accessed by the driver. It's pretty much a smaller version of BAR1, but intended for a different purpose.
On dGPUs, there's a couple of places (fault buffer address, and fault method buffer on volta) where the GPU wants PRI regs to be poked with an offset within BAR2 rather than an aperture+offset combination. I'm not 100% sure what Tegra parts do here, but presumably if it's working for you, they're happy to just accept a system memory address instead.
I guess this would be the right thing to do here in that situation. The more obvious (from a "reading the code" POV) thing to do would be to write Tegra-specific versions of the functions that use nvkm_memory_bar2() to perform this mapping, and use nvkm_memory_addr() instead but I'm not sure if we need/want to go to that effort. It's conceivable it could be required at some point.
Yeah, that sounds slightly more correct. I'll look into it and see if I can come up with something.
Thierry
Ben.
It'd be great to get Lyude's feedback on the second patch, since that call to pci_disable_device() was rather oddly placed and I'm not sure if that was essential for things to work or whether the slightly different point in time where it's called after this patch is also okay. It looks to me like it should work fine, but I don't currently have a way to test this on desktop GPUs.
Thierry
Thierry Reding (2): drm/nouveau: tegra: Fix NULL pointer dereference drm/nouveau: tegra: Do not try to disable PCI device
drivers/gpu/drm/nouveau/nouveau_drm.c | 3 +- .../drm/nouveau/nvkm/subdev/instmem/gk20a.c | 30 +++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-)
-- 2.23.0
Nouveau mailing list Nouveau@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/nouveau
dri-devel@lists.freedesktop.org
-
Ben Skeggs -
Lyude Paul -
Thierry Reding