On 01/25/2012 06:34 AM, Ben Skeggs wrote:
> From: Ben Skeggs<bskeggs at redhat.com>
>
> Both changes in dc97b3409a790d2a21aac6e5cdb99558b5944119 cause serious
> regressions in the nouveau driver.
>
> move_notify() was originally able to presume that bo->mem is the old node,
> and new_mem is the new node. The above commit moves the call to
> move_notify() to after move() has been done, which means that now, sometimes,
> new_mem isn't the new node at all, bo->mem is, and new_mem points at a
> stale, possibly-just-been-killed-by-move node.
>
> This is clearly not a good situation. This patch reverts this change, and
> replaces it with a cleanup in the move() failure path instead.
>
> The second issue is that the call to move_notify() from cleanup_memtype_use()
> causes the TTM ghost objects to get passed into the driver. This is clearly
> bad as the driver knows nothing about these "fake" TTM BOs, and ends up
> accessing uninitialised memory.
>
> I worked around this in nouveau's move_notify() hook by ensuring the BO
> destructor was nouveau's. I don't particularly like this solution, and
> would rather TTM never pass the driver these objects. However, I don't
> clearly understand the reason why we're calling move_notify() here anyway
> and am happy to work around the problem in nouveau instead of breaking the
> behaviour expected by other drivers.
>
> Signed-off-by: Ben Skeggs<bskeggs at redhat.com>
> Cc: Jerome Glisse<j.glisse at gmail.com>
As mentioned in the lengthy email discussion, I don't like the ttm change,
but since we don't have time for anything better,
Reviewed-by: Thomas Hellstrom <thellstrom at vmware.com>
> ---
> drivers/gpu/drm/nouveau/nouveau_bo.c | 4 ++++
> drivers/gpu/drm/ttm/ttm_bo.c | 17 +++++++++++++----
> 2 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c
> b/drivers/gpu/drm/nouveau/nouveau_bo.c
> index 724b41a..ec54364 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_bo.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
> @@ -812,6 +812,10 @@ nouveau_bo_move_ntfy(struct ttm_buffer_object *bo,
> struct ttm_mem_reg *new_mem)
> struct nouveau_bo *nvbo = nouveau_bo(bo);
> struct nouveau_vma *vma;
>
> + /* ttm can now (stupidly) pass the driver bos it didn't create... */
> + if (bo->destroy != nouveau_bo_del_ttm)
> + return;
> +
> list_for_each_entry(vma,&nvbo->vma_list, head) {
> if (new_mem&& new_mem->mem_type == TTM_PL_VRAM) {
> nouveau_vm_map(vma, new_mem->mm_node);
> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
> index 2f0eab6..7c3a57d 100644
> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> @@ -404,6 +404,9 @@ static int ttm_bo_handle_move_mem(struct
> ttm_buffer_object *bo,
> }
> }
>
> + if (bdev->driver->move_notify)
> + bdev->driver->move_notify(bo, mem);
> +
> if (!(old_man->flags& TTM_MEMTYPE_FLAG_FIXED)&&
> !(new_man->flags& TTM_MEMTYPE_FLAG_FIXED))
> ret = ttm_bo_move_ttm(bo, evict, no_wait_reserve, no_wait_gpu,
> mem);
> @@ -413,11 +416,17 @@ static int ttm_bo_handle_move_mem(struct
> ttm_buffer_object *bo,
> else
> ret = ttm_bo_move_memcpy(bo, evict, no_wait_reserve,
> no_wait_gpu, mem);
>
> - if (ret)
> - goto out_err;
> + if (ret) {
> + if (bdev->driver->move_notify) {
> + struct ttm_mem_reg tmp_mem = *mem;
> + *mem = bo->mem;
> + bo->mem = tmp_mem;
> + bdev->driver->move_notify(bo, mem);
> + bo->mem = *mem;
> + }
>
> - if (bdev->driver->move_notify)
> - bdev->driver->move_notify(bo, mem);
> + goto out_err;
> + }
>
> moved:
> if (bo->evicted) {
