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@xxxxxxxxxx>
> >>>> Cc: Jerome Glisse<j.glisse@xxxxxxxxx>
> >>>> ---
> >>>> 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) {
> >>>>
> >>>>
> >>>>
> >>>>
> >>
> >>
> >
>
>
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
