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

[Ben Skeggs]

On Wed, 2012-01-25 at 15:33 +0100, Thomas Hellstrom wrote:
> 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?
It can't fail, and well, in nouveau's implementation it never will.
It's simply a "fill the ptes for all the vmas currently associated with
a bo".

And well, it's about as much TTM's business as VRAM aperture allocation
is..  I don't see the big deal, if you wan't to do it a different way in
your driver, there's nothing stopping you.  It's a lot of bother for
essentially zero effort in TTM..

> 
> >
> >> 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.
Yes, right.  That can be done, and gives exactly the same functionality
as I *already* achieve with move_notify() but apparently have to change
just because you've decided nouveau/radeon are both doing the
WrongThing(tm).

Anyhow, I care more that 3.3 works than how it works.  So, whatever.  If
I must agree to this in order to get a regression fix in, then I guess I
really have no choice in the matter.

Ben.

> 
> 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 at redhat.com>
> >>>> Cc: Jerome Glisse<j.glisse at 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) {
> >>>>
> >>>>
> >>>>
> >>>>
> >>
> >>
> >
> 
> 
>