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 <bske...@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
