[PATCH v2 06/37] drm/nouveau: move allocation of root client out of nouveau_cli_init()
Ben Skeggs
bskeggs at nvidia.com
Fri Jul 26 04:29:18 UTC 2024
On 19/7/24 21:37, Danilo Krummrich wrote:
> 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 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
> Which doesn't seem necessary. You could call drm_dev_alloc() before
> nvif_driver_init() in nouveau_drm_device_new(), can't you?
True. I've made this change.
>
>> 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
>>>>
More information about the Nouveau
mailing list