From mboxrd@z Thu Jan  1 00:00:00 1970
From: Chris Wilson <chris@chris-wilson.co.uk>
Subject: Re: [PATCH 24/30] drm/i915: Refactor pwrite/pread to
	use single copy of get_user_pages
Date: Wed, 13 Apr 2011 20:56:26 +0100
Message-ID: <1bdc18$k77tdj@fmsmga002.fm.intel.com>
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>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org>
Received: from mga11.intel.com (mga11.intel.com [192.55.52.93])
	by gabe.freedesktop.org (Postfix) with ESMTP id 3919F9F367
	for <intel-gfx@lists.freedesktop.org>;
	Wed, 13 Apr 2011 12:56:29 -0700 (PDT)
In-Reply-To: <20110413192623.GG3660@viiv.ffwll.ch>
List-Unsubscribe: <http://lists.freedesktop.org/mailman/options/intel-gfx>,
	<mailto:intel-gfx-request@lists.freedesktop.org?subject=unsubscribe>
List-Archive: <http://lists.freedesktop.org/archives/intel-gfx>
List-Post: <mailto:intel-gfx@lists.freedesktop.org>
List-Help: <mailto:intel-gfx-request@lists.freedesktop.org?subject=help>
List-Subscribe: <http://lists.freedesktop.org/mailman/listinfo/intel-gfx>,
	<mailto:intel-gfx-request@lists.freedesktop.org?subject=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: Daniel Vetter <daniel@ffwll.ch>
Cc: intel-gfx@lists.freedesktop.org
List-Id: intel-gfx@lists.freedesktop.org

On Wed, 13 Apr 2011 21:26:24 +0200, Daniel Vetter <daniel@ffwll.ch> 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

-- 
Chris Wilson, Intel Open Source Technology Centre