On Thu, Jul 18, 2024 at 05:29:20PM +1000, Ben Skeggs wrote:
> 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@xxxxxxxxxx>
> > > ---
> > > 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
Which doesn't seem necessary. You could call drm_dev_alloc() before
nvif_driver_init() in nouveau_drm_device_new(), can't you?
> remove-ioctl series removes these functions entirely.
Then why bother changing?
>
>
> >
> > > }
> > > 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.
Such things definitely deserve a comment, this is not obvious.
>
> Yes, the linkage between "parent" and master.base (nvif_client) is clumsy,
> but, once again, this is fixed in the remove-ioctl series.
Less important if removed later on, but you still want to make it obvious to the
reviewers that what you are doing is required and the "right" thing to do.
>
> >
> > > -
> > > 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
> > >
>
