On Wed, Apr 13, 2011 at 08:56:26PM +0100, Chris Wilson wrote: > On Wed, 13 Apr 2011 21:26:24 +0200, Daniel Vetter <[email protected]> wrote: > > On Tue, Apr 12, 2011 at 09:31:52PM +0100, Chris Wilson wrote: > > > Replace the three nearly identical copies of the code with a single > > > function. And take advantage of the opportunity to do some > > > micro-optimisation: avoid the vmalloc if at all possible and also avoid > > > dropping the lock unless we are forced to acquire the mm semaphore. > > > > One tiny nitpick: Perhaps put an api comment at the top of > > gem_get_user_pages that this function drops the struct_mutex. That's not > > something we normally do and could cause endless amounts of fun if > > neglected. > > How about: > > /** > * Magically retrieves the pages for the user addr whilst holding the > * dev->struct_mutex. > * > * Since we can not take the mm semaphore whilst holding our > dev->struct_mutex, > * due to the pre-existing lock dependency established by i915_gem_fault(), > * we have to perform some sleight-of-hand. > * > * First, we try the lockless variant of gup whilst continuing to hold the > * mutex. If that fails to get all the user pages, then we no choice but > * to acquire the mm semaphore (thus dropping the lock on dev->struct_mutex > * to do so). The dev->struct_mutex is then re-acquired before we return. > * > * Returns: an error code *and* the number of user pages acquired. Even > * on an error, you must iterate over the return pages and release them. > */
Perfect. And just reminded me that my review wasn't too careful, I've glossed a bit over that num_pages detail ... -Daniel -- Daniel Vetter Mail: [email protected] Mobile: +41 (0)79 365 57 48 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
