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

[Thomas Hellstrom <thomas@shipmail.org>]

On 01/25/2012 10:41 AM, Ben Skeggs wrote:
>
> My main concern is that we blindly and unnecessarily set up GPU bindings and
> end up with unnecessary code in TTM, and furthermore that we communicate
> that bad practice to future driver writers.
> This "unnecessary code" is like 5 lines of cleanup if something fails,
> hardly anything to be jumping up and down about :)

It's just not TTM's business, unless the GPU maps are mappable by the 
CPU as well.
Also, What if the mapping setup in move_notify() fails?

>
>> Thomas, what do you suggest to move forward with this? Both of these
>> bugs are serious regressions that make nouveau unusable with the current
>> 3.3-rc series. Ben.
>>
>> My number one choice would of course be to have the drivers set up their
>> private GPU mappings after a
>> successful validate call, as I originally suggested and you agreed to.
>>
>> If that's not possible (I realize it's late in the release series), I'll
>> ack this patch if you and Jerome agree not to block
>> attempts to move in that direction for future kernel releases.
> I can't say I'm entirely happy with the plan honestly.  To me, it still
> seems more efficient to handle this when a move happens (comparatively
> rare) and "map new backing storage into every vm that has a reference"
> than to (on every single buffer of every single "exec" call) go "is this
> buffer mapped into this channel's vm? yes, ok; no, lets go map it".
>
> I'm not even sure how exactly I plan on storing this mapping efficiently
> yet.. Scanning the BO's linked list of VMs it's mapped into for "if
> (this_vma == chan->vma)" doesn't exactly sound performant.

As previously suggested, in the simplest case a bo could have a 'needs 
remap' flag
that is set on gpu map teardown on move_notify(), and when this flag is 
detected in validate,
go ahead and set up all needed maps and clear that flag.

This is the simplest case and more or less equivalent to the current 
solution, except
maps aren't set up unless needed by at least one channel and there is a 
clear way
to handle errors when GPU maps are set up.

A simple and straightforward fix that leaves the path open (if so 
desired) to
handle finer channel granularity.

Or am I missing something?

/Thomas






>
> Thanks,
> Ben.
>
>> Thanks,
>> Thomas
>>
>>
>>
>>>> Signed-off-by: Ben Skeggs<bskeggs@redhat.com>
>>>> Cc: Jerome Glisse<j.glisse@gmail.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) {
>>>>
>>>>
>>>>
>>>>
>>
>>
>