Re: [PATCH 02/34] drm/nouveau: store nvkm_device pointer in nouveau_drm

[Danilo Krummrich <dakr@xxxxxxxxxx>]

On 5/27/24 16:19, Ben Skeggs wrote:
> There's various different places in the drm code that get at the
> nvkm_device via various creative (and not very type-safe) means.
>
> One of those being via nvif_device.object.priv.
>
> Another patch series is going to entirely remove the ioctl-like
> interfaces beween the drm code and nvkm, and that field will no
> longer exist.
>
> This provides a safer replacement for accessing the nvkm_device,
> and will used more in upcoming patches to cleanup other cases.
>
> Signed-off-by: Ben Skeggs <bskeggs@xxxxxxxxxx>
> ---
>   drivers/gpu/drm/nouveau/nouveau_drm.c | 13 +++++--------
>   drivers/gpu/drm/nouveau/nouveau_drv.h |  1 +
>   2 files changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index c37798b507ea..e6ed68dcef78 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -579,7 +579,7 @@ nouveau_parent = {
>   };
>   
>   static int
> -nouveau_drm_device_init(struct drm_device *dev)
> +nouveau_drm_device_init(struct drm_device *dev, struct nvkm_device *nvkm)
>   {
>   	struct nouveau_drm *drm;
>   	int ret;
> @@ -588,6 +588,7 @@ nouveau_drm_device_init(struct drm_device *dev)
>   		return -ENOMEM;
>   	dev->dev_private = drm;
>   	drm->dev = dev;
> +	drm->nvkm = nvkm;
>   
>   	nvif_parent_ctor(&nouveau_parent, &drm->parent);
>   	drm->master.base.object.parent = &drm->parent;
> @@ -830,7 +831,7 @@ static int nouveau_drm_probe(struct pci_dev *pdev,
>   
>   	pci_set_drvdata(pdev, drm_dev);
>   
> -	ret = nouveau_drm_device_init(drm_dev);
> +	ret = nouveau_drm_device_init(drm_dev, device);

NIT (and actually not directly related to this patch): It seems that we
randomly switch between 'dev', 'drm_dev' for struct drm_device and
'device', 'nvkm' for nvkm_device. Since this is a cleanup series, mind
adding a patch to get this straight?

>   	if (ret)
>   		goto fail_pci;
>   
> @@ -861,14 +862,10 @@ void
>   nouveau_drm_device_remove(struct drm_device *dev)
>   {
>   	struct nouveau_drm *drm = nouveau_drm(dev);
> -	struct nvkm_client *client;
> -	struct nvkm_device *device;
> +	struct nvkm_device *device = drm->nvkm;
>   
>   	drm_dev_unplug(dev);
>   
> -	client = nvxx_client(&drm->client.base);
> -	device = nvkm_device_find(client->device);
> -
>   	nouveau_drm_device_fini(dev);
>   	drm_dev_put(dev);
>   	nvkm_device_del(&device);
> @@ -1376,7 +1373,7 @@ nouveau_platform_device_create(const struct nvkm_device_tegra_func *func,
>   		goto err_free;
>   	}
>   
> -	err = nouveau_drm_device_init(drm);
> +	err = nouveau_drm_device_init(drm, *pdevice);

Same here, 'pdev' for struct platform_device and 'pdevice' for a ** to a
struct nvkm_device seems confusing.

>   	if (err)
>   		goto err_put;
>   
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h
> index e239c6bf4afa..b711e994407b 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drv.h
> +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
> @@ -201,6 +201,7 @@ u_memcpya(uint64_t user, unsigned int nmemb, unsigned int size)
>   #include <nvif/parent.h>
>   
>   struct nouveau_drm {
> +	struct nvkm_device *nvkm;
>   	struct nvif_parent parent;
>   	struct nouveau_cli master;
>   	struct nouveau_cli client;