On Sat, 25 Jan 2020 at 00:48, Roy Spliet <nouveau@xxxxxxxxxx> wrote: > > Without diving in any of the details, your commit message has me curious > and concerned... In a "manager" kind of way, despite being neither a > manager nor an insider or active contributor. ;-) > > On 24/01/2020 14:30, Christian König wrote: > > From: Christian König <ckoenig.leichtzumerken@xxxxxxxxx> > > > > While working on TTM cleanups I've found that the io_reserve_lru used by > > Nouveau is actually not working at all. > > What made you conclude this mechanism doesn't work? Does this imply that > Nouveau is broken? If so: Does the migration of this code from TTM to > Nouveau mean that Nouveau remains broken, but it's now simply no longer > your problem? And what would it take to fix Nouveau? I believe Christian is referring to the issue that this[1] should fix, where we'd never hit the LRU eviction path for IO mappings. Ben. [1] https://github.com/skeggsb/nouveau/commit/04bc1dd62db05b22265ea0febf199e5967b9eeb2 > Best, > > Roy > > > > > In general we should remove driver specific handling from the memory > > management, so this patch moves the io_reserve_lru handling into Nouveau > > instead. > > > > The patch should be functional correct, but is only compile tested! > > > > v2: don't call ttm_bo_unmap_virtual in nouveau_ttm_io_mem_reserve > > > > Signed-off-by: Christian König <christian.koenig@xxxxxxx> > > Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx> > > --- > > drivers/gpu/drm/nouveau/nouveau_bo.c | 107 ++++++++++++++++++++------ > > drivers/gpu/drm/nouveau/nouveau_bo.h | 3 + > > drivers/gpu/drm/nouveau/nouveau_drv.h | 2 + > > drivers/gpu/drm/nouveau/nouveau_ttm.c | 43 ++++++++++- > > 4 files changed, 131 insertions(+), 24 deletions(-) > > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c > > index 81668104595f..acee054f77ed 100644 > > --- a/drivers/gpu/drm/nouveau/nouveau_bo.c > > +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c > > @@ -137,6 +137,7 @@ nouveau_bo_del_ttm(struct ttm_buffer_object *bo) > > struct nouveau_bo *nvbo = nouveau_bo(bo); > > > > WARN_ON(nvbo->pin_refcnt > 0); > > + nouveau_bo_del_io_reserve_lru(bo); > > nv10_bo_put_tile_region(dev, nvbo->tile, NULL); > > > > /* > > @@ -304,6 +305,7 @@ nouveau_bo_init(struct nouveau_bo *nvbo, u64 size, int align, u32 flags, > > > > nvbo->bo.mem.num_pages = size >> PAGE_SHIFT; > > nouveau_bo_placement_set(nvbo, flags, 0); > > + INIT_LIST_HEAD(&nvbo->io_reserve_lru); > > > > ret = ttm_bo_init(nvbo->bo.bdev, &nvbo->bo, size, type, > > &nvbo->placement, align >> PAGE_SHIFT, false, > > @@ -574,6 +576,26 @@ nouveau_bo_sync_for_cpu(struct nouveau_bo *nvbo) > > PAGE_SIZE, DMA_FROM_DEVICE); > > } > > > > +void nouveau_bo_add_io_reserve_lru(struct ttm_buffer_object *bo) > > +{ > > + struct nouveau_drm *drm = nouveau_bdev(bo->bdev); > > + struct nouveau_bo *nvbo = nouveau_bo(bo); > > + > > + mutex_lock(&drm->ttm.io_reserve_mutex); > > + list_move_tail(&nvbo->io_reserve_lru, &drm->ttm.io_reserve_lru); > > + mutex_unlock(&drm->ttm.io_reserve_mutex); > > +} > > + > > +void nouveau_bo_del_io_reserve_lru(struct ttm_buffer_object *bo) > > +{ > > + struct nouveau_drm *drm = nouveau_bdev(bo->bdev); > > + struct nouveau_bo *nvbo = nouveau_bo(bo); > > + > > + mutex_lock(&drm->ttm.io_reserve_mutex); > > + list_del_init(&nvbo->io_reserve_lru); > > + mutex_unlock(&drm->ttm.io_reserve_mutex); > > +} > > + > > int > > nouveau_bo_validate(struct nouveau_bo *nvbo, bool interruptible, > > bool no_wait_gpu) > > @@ -675,8 +697,6 @@ nouveau_bo_init_mem_type(struct ttm_bo_device *bdev, uint32_t type, > > } > > > > man->func = &nouveau_vram_manager; > > - man->io_reserve_fastpath = false; > > - man->use_io_reserve_lru = true; > > } else { > > man->func = &ttm_bo_manager_func; > > } > > @@ -1305,6 +1325,8 @@ nouveau_bo_move_ntfy(struct ttm_buffer_object *bo, bool evict, > > if (bo->destroy != nouveau_bo_del_ttm) > > return; > > > > + nouveau_bo_del_io_reserve_lru(bo); > > + > > if (mem && new_reg->mem_type != TTM_PL_SYSTEM && > > mem->mem.page == nvbo->page) { > > list_for_each_entry(vma, &nvbo->vma_list, head) { > > @@ -1427,6 +1449,30 @@ nouveau_bo_verify_access(struct ttm_buffer_object *bo, struct file *filp) > > filp->private_data); > > } > > > > +static void > > +nouveau_ttm_io_mem_free_locked(struct nouveau_drm *drm, struct ttm_mem_reg *reg) > > +{ > > + struct nouveau_mem *mem = nouveau_mem(reg); > > + > > + if (!reg->bus.base) > > + return; /* already freed */ > > + > > + if (drm->client.mem->oclass >= NVIF_CLASS_MEM_NV50) { > > + switch (reg->mem_type) { > > + case TTM_PL_TT: > > + if (mem->kind) > > + nvif_object_unmap_handle(&mem->mem.object); > > + break; > > + case TTM_PL_VRAM: > > + nvif_object_unmap_handle(&mem->mem.object); > > + break; > > + default: > > + break; > > + } > > + } > > + reg->bus.base = 0; > > +} > > + > > static int > > nouveau_ttm_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_reg *reg) > > { > > @@ -1434,18 +1480,26 @@ nouveau_ttm_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_reg *reg) > > struct nouveau_drm *drm = nouveau_bdev(bdev); > > struct nvkm_device *device = nvxx_device(&drm->client.device); > > struct nouveau_mem *mem = nouveau_mem(reg); > > + struct nouveau_bo *nvbo; > > + int ret; > > + > > + if (reg->bus.base) > > + return 0; /* already mapped */ > > > > reg->bus.addr = NULL; > > reg->bus.offset = 0; > > reg->bus.size = reg->num_pages << PAGE_SHIFT; > > - reg->bus.base = 0; > > reg->bus.is_iomem = false; > > if (!(man->flags & TTM_MEMTYPE_FLAG_MAPPABLE)) > > return -EINVAL; > > + > > + mutex_lock(&drm->ttm.io_reserve_mutex); > > +retry: > > switch (reg->mem_type) { > > case TTM_PL_SYSTEM: > > /* System memory */ > > - return 0; > > + ret = 0; > > + goto out; > > case TTM_PL_TT: > > #if IS_ENABLED(CONFIG_AGP) > > if (drm->agp.bridge) { > > @@ -1469,7 +1523,6 @@ nouveau_ttm_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_reg *reg) > > } args; > > u64 handle, length; > > u32 argc = 0; > > - int ret; > > > > switch (mem->mem.object.oclass) { > > case NVIF_CLASS_MEM_NV50: > > @@ -1493,38 +1546,46 @@ nouveau_ttm_io_mem_reserve(struct ttm_bo_device *bdev, struct ttm_mem_reg *reg) > > ret = nvif_object_map_handle(&mem->mem.object, > > &args, argc, > > &handle, &length); > > - if (ret != 1) > > - return ret ? ret : -EINVAL; > > + if (ret != 1) { > > + ret = ret ? ret : -EINVAL; > > + goto out; > > + } > > + ret = 0; > > > > reg->bus.base = 0; > > reg->bus.offset = handle; > > } > > break; > > default: > > - return -EINVAL; > > + ret = -EINVAL; > > } > > - return 0; > > + > > +out: > > + if (ret == -EAGAIN) { > > + nvbo = list_first_entry_or_null(&drm->ttm.io_reserve_lru, > > + typeof(*nvbo), > > + io_reserve_lru); > > + if (nvbo) { > > + list_del_init(&nvbo->io_reserve_lru); > > + drm_vma_node_unmap(&nvbo->bo.base.vma_node, > > + bdev->dev_mapping); > > + nouveau_ttm_io_mem_free_locked(drm, &nvbo->bo.mem); > > + goto retry; > > + } > > + > > + } > > + mutex_unlock(&drm->ttm.io_reserve_mutex); > > + return ret; > > } > > > > static void > > nouveau_ttm_io_mem_free(struct ttm_bo_device *bdev, struct ttm_mem_reg *reg) > > { > > struct nouveau_drm *drm = nouveau_bdev(bdev); > > - struct nouveau_mem *mem = nouveau_mem(reg); > > > > - if (drm->client.mem->oclass >= NVIF_CLASS_MEM_NV50) { > > - switch (reg->mem_type) { > > - case TTM_PL_TT: > > - if (mem->kind) > > - nvif_object_unmap_handle(&mem->mem.object); > > - break; > > - case TTM_PL_VRAM: > > - nvif_object_unmap_handle(&mem->mem.object); > > - break; > > - default: > > - break; > > - } > > - } > > + mutex_lock(&drm->ttm.io_reserve_mutex); > > + nouveau_ttm_io_mem_free_locked(drm, reg); > > + mutex_unlock(&drm->ttm.io_reserve_mutex); > > } > > > > static int > > diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.h b/drivers/gpu/drm/nouveau/nouveau_bo.h > > index 38f9d8350963..c47fcdf80ade 100644 > > --- a/drivers/gpu/drm/nouveau/nouveau_bo.h > > +++ b/drivers/gpu/drm/nouveau/nouveau_bo.h > > @@ -17,6 +17,7 @@ struct nouveau_bo { > > bool force_coherent; > > struct ttm_bo_kmap_obj kmap; > > struct list_head head; > > + struct list_head io_reserve_lru; > > > > /* protected by ttm_bo_reserve() */ > > struct drm_file *reserved_by; > > @@ -92,6 +93,8 @@ int nouveau_bo_validate(struct nouveau_bo *, bool interruptible, > > bool no_wait_gpu); > > void nouveau_bo_sync_for_device(struct nouveau_bo *nvbo); > > void nouveau_bo_sync_for_cpu(struct nouveau_bo *nvbo); > > +void nouveau_bo_add_io_reserve_lru(struct ttm_buffer_object *bo); > > +void nouveau_bo_del_io_reserve_lru(struct ttm_buffer_object *bo); > > > > /* TODO: submit equivalent to TTM generic API upstream? */ > > static inline void __iomem * > > diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h > > index da8c46e09943..cd19c8ce5939 100644 > > --- a/drivers/gpu/drm/nouveau/nouveau_drv.h > > +++ b/drivers/gpu/drm/nouveau/nouveau_drv.h > > @@ -158,6 +158,8 @@ struct nouveau_drm { > > int type_vram; > > int type_host[2]; > > int type_ncoh[2]; > > + struct mutex io_reserve_mutex; > > + struct list_head io_reserve_lru; > > } ttm; > > > > /* GEM interface support */ > > diff --git a/drivers/gpu/drm/nouveau/nouveau_ttm.c b/drivers/gpu/drm/nouveau/nouveau_ttm.c > > index 77a0c6ad3cef..50518b48e9b4 100644 > > --- a/drivers/gpu/drm/nouveau/nouveau_ttm.c > > +++ b/drivers/gpu/drm/nouveau/nouveau_ttm.c > > @@ -162,13 +162,51 @@ const struct ttm_mem_type_manager_func nv04_gart_manager = { > > .debug = nouveau_manager_debug > > }; > > > > +static vm_fault_t nouveau_ttm_fault(struct vm_fault *vmf) > > +{ > > + struct vm_area_struct *vma = vmf->vma; > > + struct ttm_buffer_object *bo = vma->vm_private_data; > > + pgprot_t prot; > > + vm_fault_t ret; > > + > > + ret = ttm_bo_vm_reserve(bo, vmf); > > + if (ret) > > + return ret; > > + > > + nouveau_bo_del_io_reserve_lru(bo); > > + > > + prot = vm_get_page_prot(vma->vm_flags); > > + ret = ttm_bo_vm_fault_reserved(vmf, prot, TTM_BO_VM_NUM_PREFAULT); > > + if (ret == VM_FAULT_RETRY && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT)) > > + return ret; > > + > > + nouveau_bo_add_io_reserve_lru(bo); > > + > > + dma_resv_unlock(bo->base.resv); > > + > > + return ret; > > +} > > + > > +static struct vm_operations_struct nouveau_ttm_vm_ops = { > > + .fault = nouveau_ttm_fault, > > + .open = ttm_bo_vm_open, > > + .close = ttm_bo_vm_close, > > + .access = ttm_bo_vm_access > > +}; > > + > > int > > nouveau_ttm_mmap(struct file *filp, struct vm_area_struct *vma) > > { > > struct drm_file *file_priv = filp->private_data; > > struct nouveau_drm *drm = nouveau_drm(file_priv->minor->dev); > > + int ret; > > > > - return ttm_bo_mmap(filp, vma, &drm->ttm.bdev); > > + ret = ttm_bo_mmap(filp, vma, &drm->ttm.bdev); > > + if (ret) > > + return ret; > > + > > + vma->vm_ops = &nouveau_ttm_vm_ops; > > + return 0; > > } > > > > static int > > @@ -273,6 +311,9 @@ nouveau_ttm_init(struct nouveau_drm *drm) > > return ret; > > } > > > > + mutex_init(&drm->ttm.io_reserve_mutex); > > + INIT_LIST_HEAD(&drm->ttm.io_reserve_lru); > > + > > NV_INFO(drm, "VRAM: %d MiB\n", (u32)(drm->gem.vram_available >> 20)); > > NV_INFO(drm, "GART: %d MiB\n", (u32)(drm->gem.gart_available >> 20)); > > return 0; > > > _______________________________________________ > Nouveau mailing list > Nouveau@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/nouveau _______________________________________________ Nouveau mailing list Nouveau@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/nouveau
