[Intel-gfx] [PATCH 03/24] drm/i915: Pin backing pages for pwrite

Daniel Vetter daniel at ffwll.ch
Wed Sep 12 15:13:27 CEST 2012


On Thu, Sep 06, 2012 at 05:07:58PM -0700, Ben Widawsky wrote:
> On Tue,  4 Sep 2012 21:02:55 +0100
> Chris Wilson <chris at chris-wilson.co.uk> wrote:
> 
> > By using the recently introduced pinning of pages, we can safely drop
> > the mutex in the knowledge that the pages are not going to disappear
> > beneath us, and so we can simplify the code for iterating over the pages.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c |   37 +++++++++++++------------------------
> >  1 file changed, 13 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index aa088ef..8a4eac0 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -690,7 +690,7 @@ shmem_pwrite_fast(struct page *page, int shmem_page_offset, int page_length,
> >  				       page_length);
> >  	kunmap_atomic(vaddr);
> >  
> > -	return ret;
> > +	return ret ? -EFAULT : 0;
> >  }
> >  
> >  /* Only difference to the fast-path function is that this can handle bit17
> > @@ -724,7 +724,7 @@ shmem_pwrite_slow(struct page *page, int shmem_page_offset, int page_length,
> >  					     page_do_bit17_swizzling);
> >  	kunmap(page);
> >  
> > -	return ret;
> > +	return ret ? -EFAULT : 0;
> >  }
> >  
> >  static int
> > @@ -733,7 +733,6 @@ i915_gem_shmem_pwrite(struct drm_device *dev,
> >  		      struct drm_i915_gem_pwrite *args,
> >  		      struct drm_file *file)
> >  {
> > -	struct address_space *mapping = obj->base.filp->f_path.dentry->d_inode->i_mapping;
> >  	ssize_t remain;
> >  	loff_t offset;
> >  	char __user *user_data;
> 
> Without digging to deep to see if you looked already. It would be nice
> if we can get a DRM_INFO or something for cases where return isn't
> actually EFAULT.

If I understand your question correctly, the answer is that ret is never
-EFAULT; the copy functions return the amount of uncopied data in bytes.
This simply aligns the revalue with our usualy -errno stuff. Since these
two functions are not pure wrappers around the copy helpers, I agree that
-errno is a better fit for the return semantics.

> 
> > @@ -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.
-Daniel

> 
> > @@ -816,26 +809,20 @@ i915_gem_shmem_pwrite(struct drm_device *dev,
> >  			goto next_page;
> >  
> >  		hit_slowpath = 1;
> > -		page_cache_get(page);
> >  		mutex_unlock(&dev->struct_mutex);
> > -
> >  		ret = shmem_pwrite_slow(page, shmem_page_offset, page_length,
> >  					user_data, page_do_bit17_swizzling,
> >  					partial_cacheline_write,
> >  					needs_clflush_after);
> >  
> >  		mutex_lock(&dev->struct_mutex);
> > -		page_cache_release(page);
> > +
> >  next_page:
> >  		set_page_dirty(page);
> >  		mark_page_accessed(page);
> > -		if (release_page)
> > -			page_cache_release(page);
> >  
> > -		if (ret) {
> > -			ret = -EFAULT;
> > +		if (ret)
> >  			goto out;
> > -		}
> >  
> >  		remain -= page_length;
> >  		user_data += page_length;
> > @@ -843,6 +830,8 @@ next_page:
> >  	}
> >  
> >  out:
> > +	i915_gem_object_unpin_pages(obj);
> > +
> >  	if (hit_slowpath) {
> >  		/* Fixup: Kill any reinstated backing storage pages */
> >  		if (obj->madv == __I915_MADV_PURGED)
> 
> 
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch



More information about the Intel-gfx mailing list