Re: [PATCH 2/2] drm/nouveau/gsp: Use the sg allocator for level 2 of radix3
On Mon, 2024-04-29 at 16:03 +1000, Dave Airlie wrote:
> > Currently, this can result in runtime PM issues on systems where
> > memory
> > Luckily, we don't actually need to allocate coherent memory for the
> > page
> > table thanks to being able to pass the GPU a radix3 page table for
> > suspend/resume data. So, let's rewrite nvkm_gsp_radix3_sg() to use
> > the sg
> > allocator for level 2. We continue using coherent allocations for
> > lvl0 and
> > 1, since they only take a single page.
> >
> > Signed-off-by: Lyude Paul
> > Cc: sta...@vger.kernel.org
> > ---
> > .../gpu/drm/nouveau/include/nvkm/subdev/gsp.h | 4 +-
> > .../gpu/drm/nouveau/nvkm/subdev/gsp/r535.c | 71 ---
> >
> > 2 files changed, 47 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> > b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> > index 6f5d376d8fcc1..a11d16a16c3b2 100644
> > --- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> > +++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> > @@ -15,7 +15,9 @@ struct nvkm_gsp_mem {
> > };
> >
> > struct nvkm_gsp_radix3 {
> > - struct nvkm_gsp_mem mem[3];
> > + struct nvkm_gsp_mem lvl0;
> > + struct nvkm_gsp_mem lvl1;
> > + struct sg_table lvl2;
>
> This looks great, could we go a step further and combine lvl0 and
> lvl1
> into a 2 page allocation, I thought we could combine lvl0/lvl1 into a
> 2 page alloc, but that actually might be a bad idea under memory
> pressure.
I'm not sure I understand :P, do we want to go for that or not? TBH -
I'm not sure there's any hardware reason we wouldn't be able to do the
whole radix3 table as an sg allocation with two additional memory pages
added on for level 0 and 1 - since both of those can only be the size
of a single page anyway it probably doesn't make much of a difference.
The main reason I didn't end up doing that though is because it would
make the codepath in nvkm_radix3_sg() a lot uglier. We need the virtual
addresses of level 0-2's first/only pages to populate them, and we also
need the DMA addresses of level 1-2. There isn't an iterator that lets
you go through both DMA/virtual addresses as far as I can tell - and
even if there was we'd start having to keep track of when we reach the
end of a page in the loop and make sure that we always set pte to the
address of the third sg page on the first iteration of the loop. IMO,
scatterlist could definitely benefit from having an iterator that does
both and can be stepped through both in and out of for loop macros
(like Iterator in rust).
So - it's definitely possible, but considering:
* nvkm_gsp_mem isn't a very big struct
* We're only allocating a single page for level 0 and 1, so at least
according to the advice I got from Sima this should be a safe amount
to allocate coherently under memory pressure.
* It's just a lot easier code-wise having direct address to the
DMA/virt addresses for the first two levels
I decided to stay with nvkm_gsp_mem_ctor() for the first two pages and
just use nvkm_gsp_sg() for the rest. I can definitely convert the whole
thing to using nvkm_gsp_sg() if we really want though - but I don't
think it'll give us much benefit.
I'll send out the new version of the patch without these changes and a
fix for one of the issues with this patch I already mentioned to Timur,
just let me know what you end up deciding and I can revise the patch if
you want.
>
> Dave.
>
--
Cheers, Lyude Paul (she/her) Software Engineer at Red Hat
Re: [PATCH 2/2] drm/nouveau/gsp: Use the sg allocator for level 2 of radix3
> Currently, this can result in runtime PM issues on systems where memory
> Luckily, we don't actually need to allocate coherent memory for the page
> table thanks to being able to pass the GPU a radix3 page table for
> suspend/resume data. So, let's rewrite nvkm_gsp_radix3_sg() to use the sg
> allocator for level 2. We continue using coherent allocations for lvl0 and
> 1, since they only take a single page.
>
> Signed-off-by: Lyude Paul
> Cc: sta...@vger.kernel.org
> ---
> .../gpu/drm/nouveau/include/nvkm/subdev/gsp.h | 4 +-
> .../gpu/drm/nouveau/nvkm/subdev/gsp/r535.c| 71 ---
> 2 files changed, 47 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> index 6f5d376d8fcc1..a11d16a16c3b2 100644
> --- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> +++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> @@ -15,7 +15,9 @@ struct nvkm_gsp_mem {
> };
>
> struct nvkm_gsp_radix3 {
> - struct nvkm_gsp_mem mem[3];
> + struct nvkm_gsp_mem lvl0;
> + struct nvkm_gsp_mem lvl1;
> + struct sg_table lvl2;
This looks great, could we go a step further and combine lvl0 and lvl1
into a 2 page allocation, I thought we could combine lvl0/lvl1 into a
2 page alloc, but that actually might be a bad idea under memory
pressure.
Dave.
[PATCH 2/2] drm/nouveau/gsp: Use the sg allocator for level 2 of radix3
Currently we allocate all 3 levels of radix3 page tables using
nvkm_gsp_mem_ctor(), which uses dma_alloc_coherent() for allocating all of
the relevant memory. This can end up failing in scenarios where the system
has very high memory fragmentation, and we can't find enough contiguous
memory to allocate level 2 of the page table.
Currently, this can result in runtime PM issues on systems where memory
fragmentation is high - as we'll fail to allocate the page table for our
suspend/resume buffer:
kworker/10:2: page allocation failure: order:7, mode:0xcc0(GFP_KERNEL),
nodemask=(null),cpuset=/,mems_allowed=0
CPU: 10 PID: 479809 Comm: kworker/10:2 Not tainted
6.8.6-201.ChopperV6.fc39.x86_64 #1
Hardware name: SLIMBOOK Executive/Executive, BIOS N.1.10GRU06 02/02/2024
Workqueue: pm pm_runtime_work
Call Trace:
dump_stack_lvl+0x64/0x80
warn_alloc+0x165/0x1e0
? __alloc_pages_direct_compact+0xb3/0x2b0
__alloc_pages_slowpath.constprop.0+0xd7d/0xde0
__alloc_pages+0x32d/0x350
__dma_direct_alloc_pages.isra.0+0x16a/0x2b0
dma_direct_alloc+0x70/0x270
nvkm_gsp_radix3_sg+0x5e/0x130 [nouveau]
r535_gsp_fini+0x1d4/0x350 [nouveau]
nvkm_subdev_fini+0x67/0x150 [nouveau]
nvkm_device_fini+0x95/0x1e0 [nouveau]
nvkm_udevice_fini+0x53/0x70 [nouveau]
nvkm_object_fini+0xb9/0x240 [nouveau]
nvkm_object_fini+0x75/0x240 [nouveau]
nouveau_do_suspend+0xf5/0x280 [nouveau]
nouveau_pmops_runtime_suspend+0x3e/0xb0 [nouveau]
pci_pm_runtime_suspend+0x67/0x1e0
? __pfx_pci_pm_runtime_suspend+0x10/0x10
__rpm_callback+0x41/0x170
? __pfx_pci_pm_runtime_suspend+0x10/0x10
rpm_callback+0x5d/0x70
? __pfx_pci_pm_runtime_suspend+0x10/0x10
rpm_suspend+0x120/0x6a0
pm_runtime_work+0x98/0xb0
process_one_work+0x171/0x340
worker_thread+0x27b/0x3a0
? __pfx_worker_thread+0x10/0x10
kthread+0xe5/0x120
? __pfx_kthread+0x10/0x10
ret_from_fork+0x31/0x50
? __pfx_kthread+0x10/0x10
ret_from_fork_asm+0x1b/0x30
Luckily, we don't actually need to allocate coherent memory for the page
table thanks to being able to pass the GPU a radix3 page table for
suspend/resume data. So, let's rewrite nvkm_gsp_radix3_sg() to use the sg
allocator for level 2. We continue using coherent allocations for lvl0 and
1, since they only take a single page.
Signed-off-by: Lyude Paul
Cc: sta...@vger.kernel.org
---
.../gpu/drm/nouveau/include/nvkm/subdev/gsp.h | 4 +-
.../gpu/drm/nouveau/nvkm/subdev/gsp/r535.c| 71 ---
2 files changed, 47 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
index 6f5d376d8fcc1..a11d16a16c3b2 100644
--- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
+++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
@@ -15,7 +15,9 @@ struct nvkm_gsp_mem {
};
struct nvkm_gsp_radix3 {
- struct nvkm_gsp_mem mem[3];
+ struct nvkm_gsp_mem lvl0;
+ struct nvkm_gsp_mem lvl1;
+ struct sg_table lvl2;
};
int nvkm_gsp_sg(struct nvkm_device *, u64 size, struct sg_table *);
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
index 9858c1438aa7f..2bf9077d37118 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/r535.c
@@ -1624,7 +1624,7 @@ r535_gsp_wpr_meta_init(struct nvkm_gsp *gsp)
meta->magic = GSP_FW_WPR_META_MAGIC;
meta->revision = GSP_FW_WPR_META_REVISION;
- meta->sysmemAddrOfRadix3Elf = gsp->radix3.mem[0].addr;
+ meta->sysmemAddrOfRadix3Elf = gsp->radix3.lvl0.addr;
meta->sizeOfRadix3Elf = gsp->fb.wpr2.elf.size;
meta->sysmemAddrOfBootloader = gsp->boot.fw.addr;
@@ -1919,8 +1919,9 @@ nvkm_gsp_sg(struct nvkm_device *device, u64 size, struct
sg_table *sgt)
static void
nvkm_gsp_radix3_dtor(struct nvkm_gsp *gsp, struct nvkm_gsp_radix3 *rx3)
{
- for (int i = ARRAY_SIZE(rx3->mem) - 1; i >= 0; i--)
- nvkm_gsp_mem_dtor(gsp, >mem[i]);
+ nvkm_gsp_sg_free(gsp->subdev.device, >lvl2);
+ nvkm_gsp_mem_dtor(gsp, >lvl1);
+ nvkm_gsp_mem_dtor(gsp, >lvl0);
}
/**
@@ -1960,36 +1961,52 @@ static int
nvkm_gsp_radix3_sg(struct nvkm_gsp *gsp, struct sg_table *sgt, u64 size,
struct nvkm_gsp_radix3 *rx3)
{
- u64 addr;
+ struct sg_dma_page_iter sg_dma_iter;
+ struct scatterlist *sg;
+ size_t bufsize;
+ u64 *pte;
+ int ret, i, page_idx = 0;
- for (int i = ARRAY_SIZE(rx3->mem) - 1; i >= 0; i--) {
- u64 *ptes;
- size_t bufsize;
- int ret, idx;
-
- bufsize = ALIGN((size / GSP_PAGE_SIZE) * sizeof(u64),
GSP_PAGE_SIZE);
- ret = nvkm_gsp_mem_ctor(gsp, bufsize, >mem[i]);
- if (ret)
- return ret;
+ ret = nvkm_gsp_mem_ctor(gsp, GSP_PAGE_SIZE, >lvl0);
+ if (ret)
+ return
