[PATCH v2 06/37] drm/nouveau: move allocation of root client out of nouveau_cli_init()

[Ben Skeggs]

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
>>