Re: [PATCH v2 28/37] drm/nouveau: add nvif_mmu to nouveau_drm

[Danilo Krummrich <dakr@redhat.com>]

On Thu, Jul 18, 2024 at 06:10:29PM +1000, Ben Skeggs wrote:
> On 10/7/24 02:34, Danilo Krummrich wrote:
> 
> > On Fri, Jul 05, 2024 at 04:37:12AM +1000, Ben Skeggs wrote:
> > > This allocates a new nvif_mmu in nouveau_drm, and uses it for TTM
> > > backend memory allocations instead of nouveau_drm.master.mmu,
> > > which will be removed in a later commit.
> > It would be good to make clear that this is part of a couple of commits that aim
> > at removing nouveau_drm::master.
> 
> Nearly the entire series relates to that in some manner. Nevertheless, I've
> slightly reworded the commit message.

I think there are quite some unrelated ones. Otherwise I wonder why this series
is titled "misc. cleanups and removal of unused apis" and the cover letter does
not mention the removal of nouveau_drm::master at all.

Anyway, next time it would probably be good to partition things even more.
For instance, have one series that only does the nouveau_drm::master removal and
another one that only does the other misc cleanups.

> 
> > 
> > Also, can we get all related commits a bit closer to each other?
> 
> They basically are.  Only a handful of commits could likely be moved around
> safely, and not in any way that'd result in any kind of perfect ordering
> like you're asking for.  It also invalidates the testing I've done to ensure
> things are bisectable.

It's not about perfect ordering, it's about making it easier to review and hence
make the process less error prone. It's also less annoying for the author; when
things are obvious, reviewers tend to ask less stupid questions. :)

> 
> > 
> > > Signed-off-by: Ben Skeggs <bskeggs@nvidia.com>
> > > ---
> > >   drivers/gpu/drm/nouveau/nouveau_drm.c | 35 ++++++++++++++++-----------
> > >   drivers/gpu/drm/nouveau/nouveau_drv.h |  1 +
> > >   drivers/gpu/drm/nouveau/nouveau_mem.c | 12 ++++-----
> > >   3 files changed, 28 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > > index 5ff116bcbabf..07748cfab233 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> > > @@ -227,13 +227,6 @@ nouveau_cli_init(struct nouveau_drm *drm, const char *sname,
> > >   		{}
> > >   	};
> > >   	static const struct nvif_mclass
> > > -	mmus[] = {
> > > -		{ NVIF_CLASS_MMU_GF100, -1 },
> > > -		{ NVIF_CLASS_MMU_NV50 , -1 },
> > > -		{ NVIF_CLASS_MMU_NV04 , -1 },
> > > -		{}
> > > -	};
> > > -	static const struct nvif_mclass
> > >   	vmms[] = {
> > >   		{ NVIF_CLASS_VMM_GP100, -1 },
> > >   		{ NVIF_CLASS_VMM_GM200, -1 },
> > > @@ -270,13 +263,7 @@ nouveau_cli_init(struct nouveau_drm *drm, const char *sname,
> > >   	cli->device.object.map.ptr = drm->device.object.map.ptr;
> > > -	ret = nvif_mclass(&cli->device.object, mmus);
> > > -	if (ret < 0) {
> > > -		NV_PRINTK(err, cli, "No supported MMU class\n");
> > > -		goto done;
> > > -	}
> > > -
> > > -	ret = nvif_mmu_ctor(&cli->device.object, "drmMmu", mmus[ret].oclass,
> > > +	ret = nvif_mmu_ctor(&cli->device.object, "drmMmu", drm->mmu.object.oclass,
> > >   			    &cli->mmu);
> > >   	if (ret) {
> > >   		NV_PRINTK(err, cli, "MMU allocation failed: %d\n", ret);
> > > @@ -717,6 +704,7 @@ nouveau_drm_device_del(struct nouveau_drm *drm)
> > >   	if (drm->dev)
> > >   		drm_dev_put(drm->dev);
> > > +	nvif_mmu_dtor(&drm->mmu);
> > >   	nvif_device_dtor(&drm->device);
> > >   	nvif_client_dtor(&drm->master.base);
> > >   	nvif_parent_dtor(&drm->parent);
> > > @@ -728,6 +716,13 @@ static struct nouveau_drm *
> > >   nouveau_drm_device_new(const struct drm_driver *drm_driver, struct device *parent,
> > >   		       struct nvkm_device *device)
> > >   {
> > > +	static const struct nvif_mclass
> > > +	mmus[] = {
> > > +		{ NVIF_CLASS_MMU_GF100, -1 },
> > > +		{ NVIF_CLASS_MMU_NV50 , -1 },
> > > +		{ NVIF_CLASS_MMU_NV04 , -1 },
> > > +		{}
> > > +	};
> > >   	struct nouveau_drm *drm;
> > >   	int ret;
> > > @@ -757,6 +752,18 @@ nouveau_drm_device_new(const struct drm_driver *drm_driver, struct device *paren
> > >   		goto done;
> > >   	}
> > > +	ret = nvif_mclass(&drm->device.object, mmus);
> > > +	if (ret < 0) {
> > > +		NV_ERROR(drm, "No supported MMU class\n");
> > > +		goto done;
> > > +	}
> > > +
> > > +	ret = nvif_mmu_ctor(&drm->device.object, "drmMmu", mmus[ret].oclass, &drm->mmu);
> > > +	if (ret) {
> > > +		NV_ERROR(drm, "MMU allocation failed: %d\n", ret);
> > > +		goto done;
> > > +	}
> > > +
> > >   	drm->dev = drm_dev_alloc(drm_driver, parent);
> > >   	if (IS_ERR(drm->dev)) {
> > >   		ret = PTR_ERR(drm->dev);
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h
> > > index a9e0a63c772e..2535a50b99f3 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
> > > @@ -204,6 +204,7 @@ struct nouveau_drm {
> > >   	struct nvkm_device *nvkm;
> > >   	struct nvif_parent parent;
> > >   	struct nvif_device device;
> > > +	struct nvif_mmu mmu;
> > >   	struct nouveau_cli master;
> > >   	struct nouveau_cli client;
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_mem.c b/drivers/gpu/drm/nouveau/nouveau_mem.c
> > > index 25f31d5169e5..67f93cf753ba 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_mem.c
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_mem.c
> > > @@ -91,7 +91,7 @@ nouveau_mem_host(struct ttm_resource *reg, struct ttm_tt *tt)
> > >   	struct nouveau_mem *mem = nouveau_mem(reg);
> > >   	struct nouveau_cli *cli = mem->cli;
> > >   	struct nouveau_drm *drm = cli->drm;
> > > -	struct nvif_mmu *mmu = &cli->mmu;
> > > +	struct nvif_mmu *mmu = &drm->mmu;
> > >   	struct nvif_mem_ram_v0 args = {};
> > >   	u8 type;
> > >   	int ret;
> > > @@ -115,7 +115,7 @@ nouveau_mem_host(struct ttm_resource *reg, struct ttm_tt *tt)
> > >   		args.dma = tt->dma_address;
> > >   	mutex_lock(&drm->master.lock);
> > > -	ret = nvif_mem_ctor_type(mmu, "ttmHostMem", cli->mem->oclass, type, PAGE_SHIFT,
> > > +	ret = nvif_mem_ctor_type(mmu, "ttmHostMem", mmu->mem, type, PAGE_SHIFT,
> > >   				 reg->size,
> > >   				 &args, sizeof(args), &mem->mem);
> > >   	mutex_unlock(&drm->master.lock);
> > > @@ -128,14 +128,14 @@ nouveau_mem_vram(struct ttm_resource *reg, bool contig, u8 page)
> > >   	struct nouveau_mem *mem = nouveau_mem(reg);
> > >   	struct nouveau_cli *cli = mem->cli;
> > >   	struct nouveau_drm *drm = cli->drm;
> > > -	struct nvif_mmu *mmu = &cli->mmu;
> > > +	struct nvif_mmu *mmu = &drm->mmu;
> > >   	u64 size = ALIGN(reg->size, 1 << page);
> > >   	int ret;
> > >   	mutex_lock(&drm->master.lock);
> > > -	switch (cli->mem->oclass) {
> > > +	switch (mmu->mem) {
> > >   	case NVIF_CLASS_MEM_GF100:
> > > -		ret = nvif_mem_ctor_type(mmu, "ttmVram", cli->mem->oclass,
> > > +		ret = nvif_mem_ctor_type(mmu, "ttmVram", mmu->mem,
> > >   					 drm->ttm.type_vram, page, size,
> > >   					 &(struct gf100_mem_v0) {
> > >   						.contig = contig,
> > > @@ -143,7 +143,7 @@ nouveau_mem_vram(struct ttm_resource *reg, bool contig, u8 page)
> > >   					 &mem->mem);
> > >   		break;
> > >   	case NVIF_CLASS_MEM_NV50:
> > > -		ret = nvif_mem_ctor_type(mmu, "ttmVram", cli->mem->oclass,
> > > +		ret = nvif_mem_ctor_type(mmu, "ttmVram", mmu->mem,
> > >   					 drm->ttm.type_vram, page, size,
> > >   					 &(struct nv50_mem_v0) {
> > >   						.bankswz = mmu->kind[mem->kind] == 2,
> > > -- 
> > > 2.45.1
> > > 
>