Danilo Krummrich
2024-Jul-09 15:33 UTC
[PATCH v2 06/37] drm/nouveau: move allocation of root client out of nouveau_cli_init()
On Fri, Jul 05, 2024 at 04:36:50AM +1000, Ben Skeggs wrote:> drm->master isn't really a nouveau_cli, and is mostly just used to get > at an nvif_mmu object to implement a hack around issues with the ioctl > interface to nvkm. > > Later patches in this series will allocate nvif_device/mmu objects in > nouveau_drm directly, removing the need for master.Please don't use future tense.> > Another patch series will remove the need for the above-mentioned hack > entirely.Do you have those patches already?> > The only other member of drm->master that's needed is the nvif_client, > and is a dependency of device/mmu. So the first step is to move its > allocation out of code handling nouveau_cli init. > > v2: > - modified slightly due to the addition of tegra/pci cleanup patches > > Signed-off-by: Ben Skeggs <bskeggs at nvidia.com> > --- > drivers/gpu/drm/nouveau/nouveau_drm.c | 46 ++++++++++++++------------- > 1 file changed, 24 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c > index 140e27af0d64..a942d2c03d44 100644 > --- a/drivers/gpu/drm/nouveau/nouveau_drm.c > +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c > @@ -129,12 +129,12 @@ nouveau_platform_name(struct platform_device *platformdev) > } > > static u64 > -nouveau_name(struct drm_device *dev) > +nouveau_name(struct device *dev) > { > - if (dev_is_pci(dev->dev)) > - return nouveau_pci_name(to_pci_dev(dev->dev)); > + if (dev_is_pci(dev)) > + return nouveau_pci_name(to_pci_dev(dev)); > else > - return nouveau_platform_name(to_platform_device(dev->dev)); > + return nouveau_platform_name(to_platform_device(dev));This looks like it should be a separate patch.> } > > static inline bool > @@ -209,9 +209,11 @@ nouveau_cli_fini(struct nouveau_cli *cli) > nouveau_vmm_fini(&cli->vmm); > nvif_mmu_dtor(&cli->mmu); > nvif_device_dtor(&cli->device); > - mutex_lock(&cli->drm->master.lock); > - nvif_client_dtor(&cli->base); > - mutex_unlock(&cli->drm->master.lock); > + if (cli != &cli->drm->master) { > + mutex_lock(&cli->drm->master.lock); > + nvif_client_dtor(&cli->base); > + mutex_unlock(&cli->drm->master.lock); > + } > } > > static int > @@ -241,7 +243,7 @@ nouveau_cli_init(struct nouveau_drm *drm, const char *sname, > { NVIF_CLASS_VMM_NV04 , -1 }, > {} > }; > - u64 device = nouveau_name(drm->dev); > + u64 device = nouveau_name(drm->dev->dev); > int ret; > > snprintf(cli->name, sizeof(cli->name), "%s", sname); > @@ -253,10 +255,7 @@ nouveau_cli_init(struct nouveau_drm *drm, const char *sname, > INIT_LIST_HEAD(&cli->worker); > mutex_init(&cli->lock); > > - if (cli == &drm->master) { > - ret = nvif_driver_init(NULL, nouveau_config, nouveau_debug, > - cli->name, device, &cli->base); > - } else { > + if (cli != &drm->master) { > mutex_lock(&drm->master.lock); > ret = nvif_client_ctor(&drm->master.base, cli->name, device, > &cli->base); > @@ -626,7 +625,6 @@ nouveau_drm_device_fini(struct nouveau_drm *drm) > nouveau_cli_fini(&drm->client); > nouveau_cli_fini(&drm->master); > destroy_workqueue(drm->sched_wq); > - nvif_parent_dtor(&drm->parent); > mutex_destroy(&drm->clients_lock); > } > > @@ -636,15 +634,10 @@ nouveau_drm_device_init(struct nouveau_drm *drm) > struct drm_device *dev = drm->dev; > int ret; > > - nvif_parent_ctor(&nouveau_parent, &drm->parent); > - drm->master.base.object.parent = &drm->parent;Moving this to nouveau_drm_device_new(), plus the resulting changes in error handling, don't seem to be related to this commit either.> - > drm->sched_wq = alloc_workqueue("nouveau_sched_wq_shared", 0, > WQ_MAX_ACTIVE); > - if (!drm->sched_wq) { > - ret = -ENOMEM; > - goto fail_alloc; > - } > + if (!drm->sched_wq) > + return -ENOMEM; > > ret = nouveau_cli_init(drm, "DRM-master", &drm->master); > if (ret) > @@ -726,8 +719,6 @@ nouveau_drm_device_init(struct nouveau_drm *drm) > nouveau_cli_fini(&drm->master); > fail_wq: > destroy_workqueue(drm->sched_wq); > -fail_alloc: > - nvif_parent_dtor(&drm->parent); > return ret; > } > > @@ -737,6 +728,9 @@ nouveau_drm_device_del(struct nouveau_drm *drm) > if (drm->dev) > drm_dev_put(drm->dev); > > + nvif_client_dtor(&drm->master.base); > + nvif_parent_dtor(&drm->parent); > + > kfree(drm); > } > > @@ -753,6 +747,14 @@ nouveau_drm_device_new(const struct drm_driver *drm_driver, struct device *paren > > drm->nvkm = device; > > + nvif_parent_ctor(&nouveau_parent, &drm->parent); > + drm->master.base.object.parent = &drm->parent; > + > + ret = nvif_driver_init(NULL, nouveau_config, nouveau_debug, "drm", > + nouveau_name(parent), &drm->master.base); > + if (ret) > + goto done; > + > drm->dev = drm_dev_alloc(drm_driver, parent); > if (IS_ERR(drm->dev)) { > ret = PTR_ERR(drm->dev); > -- > 2.45.1 >
Ben Skeggs
2024-Jul-18 07:29 UTC
[PATCH v2 06/37] drm/nouveau: move allocation of root client out of nouveau_cli_init()
On 10/7/24 01:33, Danilo Krummrich wrote:> On Fri, Jul 05, 2024 at 04:36:50AM +1000, Ben Skeggs wrote: >> drm->master isn't really a nouveau_cli, and is mostly just used to get >> at an nvif_mmu object to implement a hack around issues with the ioctl >> interface to nvkm. >> >> Later patches in this series will allocate nvif_device/mmu objects in >> nouveau_drm directly, removing the need for master. > Please don't use future tense. > >> Another patch series will remove the need for the above-mentioned hack >> entirely. > Do you have those patches already?Yes.? It's the "remove-ioctl" series, of which this one used to be a part of.? I've mentioned it in the updated commit message regardless.> >> The only other member of drm->master that's needed is the nvif_client, >> and is a dependency of device/mmu. So the first step is to move its >> allocation out of code handling nouveau_cli init. >> >> v2: >> - modified slightly due to the addition of tegra/pci cleanup patches >> >> Signed-off-by: Ben Skeggs <bskeggs at nvidia.com> >> --- >> drivers/gpu/drm/nouveau/nouveau_drm.c | 46 ++++++++++++++------------- >> 1 file changed, 24 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c >> index 140e27af0d64..a942d2c03d44 100644 >> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c >> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c >> @@ -129,12 +129,12 @@ nouveau_platform_name(struct platform_device *platformdev) >> } >> >> static u64 >> -nouveau_name(struct drm_device *dev) >> +nouveau_name(struct device *dev) >> { >> - if (dev_is_pci(dev->dev)) >> - return nouveau_pci_name(to_pci_dev(dev->dev)); >> + if (dev_is_pci(dev)) >> + return nouveau_pci_name(to_pci_dev(dev)); >> else >> - return nouveau_platform_name(to_platform_device(dev->dev)); >> + return nouveau_platform_name(to_platform_device(dev)); > This looks like it should be a separate patch.No.? One of its callers is now before drm_device.dev is valid. Also, the remove-ioctl series removes these functions entirely.> >> } >> >> static inline bool >> @@ -209,9 +209,11 @@ nouveau_cli_fini(struct nouveau_cli *cli) >> nouveau_vmm_fini(&cli->vmm); >> nvif_mmu_dtor(&cli->mmu); >> nvif_device_dtor(&cli->device); >> - mutex_lock(&cli->drm->master.lock); >> - nvif_client_dtor(&cli->base); >> - mutex_unlock(&cli->drm->master.lock); >> + if (cli != &cli->drm->master) { >> + mutex_lock(&cli->drm->master.lock); >> + nvif_client_dtor(&cli->base); >> + mutex_unlock(&cli->drm->master.lock); >> + } >> } >> >> static int >> @@ -241,7 +243,7 @@ nouveau_cli_init(struct nouveau_drm *drm, const char *sname, >> { NVIF_CLASS_VMM_NV04 , -1 }, >> {} >> }; >> - u64 device = nouveau_name(drm->dev); >> + u64 device = nouveau_name(drm->dev->dev); >> int ret; >> >> snprintf(cli->name, sizeof(cli->name), "%s", sname); >> @@ -253,10 +255,7 @@ nouveau_cli_init(struct nouveau_drm *drm, const char *sname, >> INIT_LIST_HEAD(&cli->worker); >> mutex_init(&cli->lock); >> >> - if (cli == &drm->master) { >> - ret = nvif_driver_init(NULL, nouveau_config, nouveau_debug, >> - cli->name, device, &cli->base); >> - } else { >> + if (cli != &drm->master) { >> mutex_lock(&drm->master.lock); >> ret = nvif_client_ctor(&drm->master.base, cli->name, device, >> &cli->base); >> @@ -626,7 +625,6 @@ nouveau_drm_device_fini(struct nouveau_drm *drm) >> nouveau_cli_fini(&drm->client); >> nouveau_cli_fini(&drm->master); >> destroy_workqueue(drm->sched_wq); >> - nvif_parent_dtor(&drm->parent); >> mutex_destroy(&drm->clients_lock); >> } >> >> @@ -636,15 +634,10 @@ nouveau_drm_device_init(struct nouveau_drm *drm) >> struct drm_device *dev = drm->dev; >> int ret; >> >> - nvif_parent_ctor(&nouveau_parent, &drm->parent); >> - drm->master.base.object.parent = &drm->parent; > Moving this to nouveau_drm_device_new(), plus the resulting changes in error > handling, don't seem to be related to this commit either.They are, because they're needed by nvif printk macros, and as other nvif-related setup moves to this function, they'll oops without it. Yes, the linkage between "parent" and master.base (nvif_client) is clumsy, but, once again, this is fixed in the remove-ioctl series.> >> - >> drm->sched_wq = alloc_workqueue("nouveau_sched_wq_shared", 0, >> WQ_MAX_ACTIVE); >> - if (!drm->sched_wq) { >> - ret = -ENOMEM; >> - goto fail_alloc; >> - } >> + if (!drm->sched_wq) >> + return -ENOMEM; >> >> ret = nouveau_cli_init(drm, "DRM-master", &drm->master); >> if (ret) >> @@ -726,8 +719,6 @@ nouveau_drm_device_init(struct nouveau_drm *drm) >> nouveau_cli_fini(&drm->master); >> fail_wq: >> destroy_workqueue(drm->sched_wq); >> -fail_alloc: >> - nvif_parent_dtor(&drm->parent); >> return ret; >> } >> >> @@ -737,6 +728,9 @@ nouveau_drm_device_del(struct nouveau_drm *drm) >> if (drm->dev) >> drm_dev_put(drm->dev); >> >> + nvif_client_dtor(&drm->master.base); >> + nvif_parent_dtor(&drm->parent); >> + >> kfree(drm); >> } >> >> @@ -753,6 +747,14 @@ nouveau_drm_device_new(const struct drm_driver *drm_driver, struct device *paren >> >> drm->nvkm = device; >> >> + nvif_parent_ctor(&nouveau_parent, &drm->parent); >> + drm->master.base.object.parent = &drm->parent; >> + >> + ret = nvif_driver_init(NULL, nouveau_config, nouveau_debug, "drm", >> + nouveau_name(parent), &drm->master.base); >> + if (ret) >> + goto done; >> + >> drm->dev = drm_dev_alloc(drm_driver, parent); >> if (IS_ERR(drm->dev)) { >> ret = PTR_ERR(drm->dev); >> -- >> 2.45.1 >>