Re: [PATCH 03/24] drm/i915: Pin backing pages for pwrite

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Sep 12, 2012 at 03:13:27PM +0200, Daniel Vetter wrote:
> On Thu, Sep 06, 2012 at 05:07:58PM -0700, Ben Widawsky wrote:
> > On Tue,  4 Sep 2012 21:02:55 +0100
> > > @@ -742,7 +741,6 @@ i915_gem_shmem_pwrite(struct drm_device *dev,
> > >  	int hit_slowpath = 0;
> > >  	int needs_clflush_after = 0;
> > >  	int needs_clflush_before = 0;
> > > -	int release_page;
> > >  
> > >  	user_data = (char __user *) (uintptr_t) args->data_ptr;
> > >  	remain = args->size;
> > > @@ -768,6 +766,12 @@ i915_gem_shmem_pwrite(struct drm_device *dev,
> > >  	    && obj->cache_level == I915_CACHE_NONE)
> > >  		needs_clflush_before = 1;
> > >  
> > > +	ret = i915_gem_object_get_pages(obj);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	i915_gem_object_pin_pages(obj);
> > > +
> > >  	offset = args->offset;
> > >  	obj->dirty = 1;
> > >  
> > > @@ -793,18 +797,7 @@ i915_gem_shmem_pwrite(struct drm_device *dev,
> > >  			((shmem_page_offset | page_length)
> > >  				& (boot_cpu_data.x86_clflush_size - 1));
> > >  
> > > -		if (obj->pages) {
> > > -			page = obj->pages[offset >> PAGE_SHIFT];
> > > -			release_page = 0;
> > > -		} else {
> > > -			page = shmem_read_mapping_page(mapping, offset >> PAGE_SHIFT);
> > > -			if (IS_ERR(page)) {
> > > -				ret = PTR_ERR(page);
> > > -				goto out;
> > > -			}
> > > -			release_page = 1;
> > > -		}
> > > -
> > > +		page = obj->pages[offset >> PAGE_SHIFT];
> > >  		page_do_bit17_swizzling = obj_do_bit17_swizzling &&
> > >  			(page_to_phys(page) & (1 << 17)) != 0;
> > >  
> > 
> > So the obvious question is what about the page caching? Can you add to
> > the commit message for my edification why previously the shmem page is
> > released from the page cache and now it isn't?
> 
> The really old code simply held onto dev->struct_mutex to guarantee that
> the pages (in obj->pages) won't disappear. My pwrite/pread rework drops
> the lock in the slowpath (to avoid deadlocking with our own pagefault
> handler), so I needed to manually grab a reference to the page to avoid it
> disappearing (and then also drop that ref again).
> 
> Chris' new code uses the new pages_pin stuff to ensure that the backing
> storage doesn't vanish, so we can reap this complexity.

I guess I've misunderstood your question: The current code either uses the
obj->pages page array or grabs the page from the backing storage. The
later gives you a page with a reference. But since the obj->pages array
can disapppear when we drop dev->struct_mutex, we need to manually hold a
reference. Since it's a slow-path I didn't bother between whether we've
got the page from obj->pages (where grabbing a ref while dropping the lock
is required) and from shmem_read_mapping_page (where we already hold a
ref) and simply grabbed an additional ref unconditionally.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch