| Message ID | 1346788996-19080-4-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, 4 Sep 2012 21:02:55 +0100 Chris Wilson <chris@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@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. > @@ -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? > @@ -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)
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@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@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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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
On Tue, 4 Sep 2012 21:02:55 +0100 Chris Wilson <chris@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@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; > @@ -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; > > @@ -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) I'll leave the pread/pwrite reviewing to Daniel...
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; @@ -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; @@ -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)
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@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_gem.c | 37 +++++++++++++------------------------ 1 file changed, 13 insertions(+), 24 deletions(-)