Received-SPF: pass (google.com: domain of linux-kernel+bounces-162808-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <ba3e7914e816f893bdba9aebe045f9af04ed85a6.camel@redhat.com>
Subject: Re: [PATCH 2/2] drm/nouveau/gsp: Use the sg allocator for level 2
 of radix3
From: Lyude Paul <lyude@redhat.com>
To: Dave Airlie <airlied@gmail.com>
Cc: nouveau@lists.freedesktop.org, dri-devel@lists.freedesktop.org, Karol
 Herbst <kherbst@redhat.com>, Danilo Krummrich <dakr@redhat.com>, Daniel
 Vetter <daniel@ffwll.ch>,  Dave Airlie <airlied@redhat.com>, Ben Skeggs
 <bskeggs@redhat.com>, Timur Tabi <ttabi@nvidia.com>, Dan Carpenter
 <dan.carpenter@linaro.org>, open list <linux-kernel@vger.kernel.org>
Date: Mon, 29 Apr 2024 13:54:25 -0400
In-Reply-To: <CAPM=9txB5Zhyyno-GLD_JShV-GZd8hgYVh=Ok=Skmzs=eTi64A@mail.gmail.com>
References: <20240426154138.64643-1-lyude@redhat.com>
	 <20240426154138.64643-2-lyude@redhat.com>
	 <CAPM=9txB5Zhyyno-GLD_JShV-GZd8hgYVh=Ok=Skmzs=eTi64A@mail.gmail.com>
Organization: Red Hat Inc.
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.52.1 (3.52.1-1.fc40) 
Precedence: bulk
MIME-Version: 1.0

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.
> >=20
> > Signed-off-by: Lyude Paul <lyude@redhat.com>
> > Cc: stable@vger.kernel.org
> > ---
> > =C2=A0.../gpu/drm/nouveau/include/nvkm/subdev/gsp.h |=C2=A0 4 +-
> > =C2=A0.../gpu/drm/nouveau/nvkm/subdev/gsp/r535.c=C2=A0=C2=A0=C2=A0 | 71=
 ++++++++++++---
> > ----
> > =C2=A02 files changed, 47 insertions(+), 28 deletions(-)
> >=20
> > 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 {
> > =C2=A0};
> >=20
> > =C2=A0struct nvkm_gsp_radix3 {
> > -=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct nvkm_gsp_mem mem[3];
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct nvkm_gsp_mem lvl0;
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct nvkm_gsp_mem lvl1;
> > +=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 struct sg_table lvl2;
>=20
> 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.=C2=A0We need the virtua=
l
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.

>=20
> Dave.
>=20

--=20
Cheers, Lyude Paul (she/her) Software Engineer at Red Hat