Re: [PATCH v3 24/37] drm/nouveau: always map device

[Zhi Wang <zhiw@nvidia.com>]

On Fri, 26 Jul 2024 14:38:15 +1000
Ben Skeggs <bskeggs@nvidia.com> wrote:

Reviewed-by: Zhi Wang <zhiw@nvidia.com>

I agree that to always map the device so that we can remove extra rd/wr
callbacks in patch 25 and patch 26.

Would you mind to point me the patch you mentioned to clean up this
later as well? I am interested to see the fate of this newly added
function.

> The next commit removes the nvif rd/wr methods from nvif_device, which
> were probably a bad idea, and mostly intended as a fallback if
> ioremap() failed (or wasn't available, as was the case in some tools
> I once used).
> 
> The nv04 KMS driver already mapped the device, because it's mostly
> been kept alive on life-support for many years and still directly
> bashes PRI a lot for modesetting.
> 
> Post-nv50, I tried pretty hard to keep PRI accesses out of the DRM
> code, but there's still a few random places where we do, and those
> were using the rd/wr paths prior to this commit.
> 
> This allocates and maps a new nvif_device (which will replace the
> usage of nouveau_drm.master.device later on), and replicates this
> pointer to all other possible users.
> 
> This will be cleaned up by the end of another patch series, after it's
> been made safe to do so.
> 
> Signed-off-by: Ben Skeggs <bskeggs@nvidia.com>
> ---
>  drivers/gpu/drm/nouveau/dispnv04/disp.c       |  5 -----
>  drivers/gpu/drm/nouveau/include/nvif/device.h |  1 +
>  drivers/gpu/drm/nouveau/nouveau_drm.c         | 16 ++++++++++++++++
>  drivers/gpu/drm/nouveau/nouveau_drv.h         |  2 ++
>  drivers/gpu/drm/nouveau/nvif/device.c         |  6 ++++++
>  5 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/dispnv04/disp.c
> b/drivers/gpu/drm/nouveau/dispnv04/disp.c index
> 13705c5f1497..e8b27bb135e7 100644 ---
> a/drivers/gpu/drm/nouveau/dispnv04/disp.c +++
> b/drivers/gpu/drm/nouveau/dispnv04/disp.c @@ -189,7 +189,6 @@ static
> void nv04_display_destroy(struct drm_device *dev)
>  {
>  	struct nv04_display *disp = nv04_display(dev);
> -	struct nouveau_drm *drm = nouveau_drm(dev);
>  	struct nouveau_encoder *encoder;
>  	struct nouveau_crtc *nv_crtc;
>  
> @@ -206,8 +205,6 @@ nv04_display_destroy(struct drm_device *dev)
>  
>  	nouveau_display(dev)->priv = NULL;
>  	vfree(disp);
> -
> -	nvif_object_unmap(&drm->client.device.object);
>  }
>  
>  int
> @@ -229,8 +226,6 @@ nv04_display_create(struct drm_device *dev)
>  
>  	disp->drm = drm;
>  
> -	nvif_object_map(&drm->client.device.object, NULL, 0);
> -
>  	nouveau_display(dev)->priv = disp;
>  	nouveau_display(dev)->dtor = nv04_display_destroy;
>  	nouveau_display(dev)->init = nv04_display_init;
> diff --git a/drivers/gpu/drm/nouveau/include/nvif/device.h
> b/drivers/gpu/drm/nouveau/include/nvif/device.h index
> f7c1b3a43c84..fec76f4733a4 100644 ---
> a/drivers/gpu/drm/nouveau/include/nvif/device.h +++
> b/drivers/gpu/drm/nouveau/include/nvif/device.h @@ -20,6 +20,7 @@
> struct nvif_device { 
>  int  nvif_device_ctor(struct nvif_client *, const char *name, struct
> nvif_device *); void nvif_device_dtor(struct nvif_device *);
> +int  nvif_device_map(struct nvif_device *);
>  u64  nvif_device_time(struct nvif_device *);
>  
>  /*XXX*/
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
> b/drivers/gpu/drm/nouveau/nouveau_drm.c index
> 22cdd987dd2f..316f7877047d 100644 ---
> a/drivers/gpu/drm/nouveau/nouveau_drm.c +++
> b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -206,6 +206,7 @@
> nouveau_cli_fini(struct nouveau_cli *cli) nouveau_vmm_fini(&cli->svm);
>  	nouveau_vmm_fini(&cli->vmm);
>  	nvif_mmu_dtor(&cli->mmu);
> +	cli->device.object.map.ptr = NULL;
>  	nvif_device_dtor(&cli->device);
>  	if (cli != &cli->drm->master) {
>  		mutex_lock(&cli->drm->master.lock);
> @@ -267,6 +268,8 @@ nouveau_cli_init(struct nouveau_drm *drm, const
> char *sname, goto done;
>  	}
>  
> +	cli->device.object.map.ptr = drm->device.object.map.ptr;
> +
>  	ret = nvif_mclass(&cli->device.object, mmus);
>  	if (ret < 0) {
>  		NV_PRINTK(err, cli, "No supported MMU class\n");
> @@ -715,6 +718,7 @@ nouveau_drm_device_del(struct nouveau_drm *drm)
>  	if (drm->dev)
>  		drm_dev_put(drm->dev);
>  
> +	nvif_device_dtor(&drm->device);
>  	nvif_client_dtor(&drm->master.base);
>  	nvif_parent_dtor(&drm->parent);
>  
> @@ -751,6 +755,18 @@ nouveau_drm_device_new(const struct drm_driver
> *drm_driver, struct device *paren if (ret)
>  		goto done;
>  
> +	ret = nvif_device_ctor(&drm->master.base, "drmDevice",
> &drm->device);
> +	if (ret) {
> +		NV_ERROR(drm, "Device allocation failed: %d\n", ret);
> +		goto done;
> +	}
> +
> +	ret = nvif_device_map(&drm->device);
> +	if (ret) {
> +		NV_ERROR(drm, "Failed to map PRI: %d\n", ret);
> +		goto done;
> +	}
> +
>  done:
>  	if (ret) {
>  		nouveau_drm_device_del(drm);
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h
> b/drivers/gpu/drm/nouveau/nouveau_drv.h index
> e7d072a9a477..80ffe15ba76b 100644 ---
> a/drivers/gpu/drm/nouveau/nouveau_drv.h +++
> b/drivers/gpu/drm/nouveau/nouveau_drv.h @@ -203,6 +203,8 @@
> u_memcpya(uint64_t user, unsigned int nmemb, unsigned int size)
> struct nouveau_drm { struct nvkm_device *nvkm;
>  	struct nvif_parent parent;
> +	struct nvif_device device;
> +
>  	struct nouveau_cli master;
>  	struct nouveau_cli client;
>  	struct drm_device *dev;
> diff --git a/drivers/gpu/drm/nouveau/nvif/device.c
> b/drivers/gpu/drm/nouveau/nvif/device.c index
> b14bccb9a93f..24880931039f 100644 ---
> a/drivers/gpu/drm/nouveau/nvif/device.c +++
> b/drivers/gpu/drm/nouveau/nvif/device.c @@ -38,6 +38,12 @@
> nvif_device_time(struct nvif_device *device) return
> device->user.func->time(&device->user); }
>  
> +int
> +nvif_device_map(struct nvif_device *device)
> +{
> +	return nvif_object_map(&device->object, NULL, 0);
> +}
> +
>  void
>  nvif_device_dtor(struct nvif_device *device)
>  {