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
