On Wed, Jan 25, 2012 at 5:19 PM, Thomas Hellstrom <thomas@xxxxxxxxxxxx> wrote: > On 01/25/2012 04:37 PM, Jerome Glisse wrote: >> >> On Wed, Jan 25, 2012 at 10:21 AM, Ben Skeggs<skeggsb@xxxxxxxxx> wrote: >>> >>> 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? >>>> >> I went over the code and Ben fix is ok with me, i need to test it a >> bit on radeon side. >> >> For long term solution why not just move most of the >> ttm_bo_handle_move_mem to the driver. It would obsolete the >> move_notify callback. move notify callback was introduced because in >> some case the driver never knew directly that a bo moved. It's obvious >> that driver need to know every time. So instead of having an ha-doc >> function for that. Let just move the handle move stuff into the >> driver. Yes there will be some code duplication but it will avoid >> anykind of weird error path and driver will be able to perform what >> ever make sense. > > > Yes, this is a solution that eliminates the need for TTM to support private > GPU map setup. Code duplication can largely be avoided if we > collect common code in a small utility function. Please this, its reduces one more place TTM suffers from midlayer effects. I'd much prefer the drivers be in control and call into help common code instead of the driver calling in and then lots of callbacks out. (and yes the drm suffers from this a lot as well). Dave. _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
