Re: [PATCH] drm/ttm: fix two regressions since move_notify changes

[Thomas Hellstrom <thomas@shipmail.org>]

On 01/25/2012 06:34 AM, Ben Skeggs wrote:
> From: Ben Skeggs<bskeggs@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@redhat.com>
> Cc: Jerome Glisse<j.glisse@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@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) {