Re: [PATCH 24/30] drm/i915: Refactor pwrite/pread to use single copy of get_user_pages

From: Paul Menzel <paulepanter@users.sourceforge.net>
To: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 24/30] drm/i915: Refactor pwrite/pread to use single copy of get_user_pages
Date: Fri, 15 Apr 2011 11:48:33 +0200	[thread overview]
Message-ID: <1302860913.4203.31.camel@mattotaupa> (raw)
In-Reply-To: <20110414232323.GA4118@lundgren.kumite>

[-- Attachment #1.1: Type: text/plain, Size: 1953 bytes --]

Am Donnerstag, den 14.04.2011, 16:23 -0700 schrieb Ben Widawsky:
> On Wed, Apr 13, 2011 at 08:56:26PM +0100, Chris Wilson wrote:
> > 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

I do not know what »gup« means.

> >  * mutex. If that fails to get all the user pages, then we no choice but

s/then we no/then we have no/

> >  * 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 <ben@bwidawsk.net>

Reviewed-by: Paul Menzel <paulepanter@users.sourceforge.net>

Thanks,

Paul

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx