From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 24/30] drm/i915: Refactor pwrite/pread to use single copy of get_user_pages Date: Thu, 14 Apr 2011 16:23:23 -0700 Message-ID: <20110414232323.GA4118@lundgren.kumite> References: <1302640318-23165-1-git-send-email-chris@chris-wilson.co.uk> <1302640318-23165-25-git-send-email-chris@chris-wilson.co.uk> <20110413192623.GG3660@viiv.ffwll.ch> <1bdc18$k77tdj@fmsmga002.fm.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from cloud01.chad-versace.us (184-106-247-128.static.cloud-ips.com [184.106.247.128]) by gabe.freedesktop.org (Postfix) with ESMTP id D883EA02E1 for ; Thu, 14 Apr 2011 16:23:26 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1bdc18$k77tdj@fmsmga002.fm.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Wed, Apr 13, 2011 at 08:56:26PM +0100, Chris Wilson wrote: > On Wed, 13 Apr 2011 21:26:24 +0200, Daniel Vetter 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. > */ > > ? > -Chris I like this patch... Reviewed-by: Ben Widawsky > > -- > Chris Wilson, Intel Open Source Technology Centre > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx