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
