Re: [PATCH 34/49] gma500: the GEM and GTT code is deviceindependant

[Hugh Dickins]

On Sun, 9 Oct 2011, Patrik Jakobsson wrote:
> On Mon, Jul 11, 2011 at 7:49 PM, Hugh Dickins wrote:
> > On Mon, 11 Jul 2011, Alan Cox wrote:
> >> > Your <4GB pages won't get swapped out while they're pinned.  But can
> >> > it happen that they'd be unpinned, swapped out, swapped back in >4GB
> >> > pages, then cause trouble for you when needed again?
> >>
> >> It does look that way, in which case that will eventually need fixing. At
> >> the moment you can't put enough memory into a device using these chips
> >> but that won't always be true I imagine.
> >
> > Thanks, I won't worry about it at this moment, but we'd better not forget.
> >
> > If it's easy for you to include a WARN_ON_ONCE check (perhaps
> > on page_to_pfn(page)), that may be worth doing to remind us.
> >
> > It's a bit sad to learn this requirement just after I'd completed
> > removing the readpage copying code, and a bit strange to have shmem
> > confined by hardware constraints; but I guess that's what we took on
> > when we opened it up to GEM.
> >
> > It will probably make sense for me to add synchronous migration when
> > a shmem swap page is found not to match the contraints wanted by the
> > mapping it goes into: mainly for NUMA, but covering your case too.
> 
> I think we need to revisit this problem. On 3.1-rc4 with some of my own changes
> I've just triggered read_cache_page_gfp in psb_gtt_attach_pages when trying to
> set a resolution that doesn't fit in stolen memory. Replacing it with
> shmem_read_mapping_page seems to work but how do we go about solving the >4GB
> issue? Is it ok for now to just use shmem_read_mapping_page or did any of you
> have a better solution?

I was surprised to see drivers/staging/gma500 appearing still to use
read_cache_page_gfp().  I assumed that since nobody was complaining,
it must be on a currently unusable path.  But you have code coming up,
that now enables that path?

Am I right to think that your immediate problem is just the oops in
__read_cache_page(), that you're not yet about to hit the 4GB issue?

I haven't rushed to address the 4GB issue, but what I have in mind is
killing two-and-a-half birds with one stone, by putting a little cookie
into the swapper_space radix_tree when we free a swapcache page, that
specifies node/zone and hashes object/offset.

NUMA mempolicies are too complex to be encapsulated in a sizeof(long)
cookie, but it should improve the common case after swapin; while
solving your 4GB GEM case, and vastly speeding up swapoff.

Here's the kind of patch I imagined would be going in for gma500, that
specifies __GFP_DMA32 on the mapping, so even swapoff can know that
this object needs its pages below 4GB (even before my recent changes,
swapoff would have broken you by inserting higher pages in the cache)
- once I implement that.  But I've not tested this patch at all...


[PATCH] gma500: use shmem_read_mapping_page

In 3.1 onwards, read_cache_page_gfp() just oopses on GEM objects:
switch gma500 over to shmem_read_mapping_page() like i915.  But when
larger devices arrive, gma500 will need to keep its pages below 4GB, so
specify __GFP_DMA32 (though that limit is not yet enforced in shmem.c).

Signed-off-by: Hugh Dicking <hughd@xxxxxxxxxx>
---

 drivers/staging/gma500/framebuffer.c |    7 +++++++
 drivers/staging/gma500/gem.c         |    4 ++++
 drivers/staging/gma500/gtt.c         |    5 ++---
 3 files changed, 13 insertions(+), 3 deletions(-)

--- 3.1-rc9/drivers/staging/gma500/framebuffer.c	2011-08-07 23:44:38.587914954 -0700
+++ linux/drivers/staging/gma500/framebuffer.c	2011-10-10 10:40:06.422389114 -0700
@@ -317,7 +317,9 @@ static struct drm_framebuffer *psb_frame
  */
 static struct gtt_range *psbfb_alloc(struct drm_device *dev, int aligned_size)
 {
+	struct address_space *mapping;
 	struct gtt_range *backing;
+
 	/* Begin by trying to use stolen memory backing */
 	backing = psb_gtt_alloc_range(dev, aligned_size, "fb", 1);
 	if (backing) {
@@ -336,6 +338,11 @@ static struct gtt_range *psbfb_alloc(str
 		psb_gtt_free_range(dev, backing);
 		return NULL;
 	}
+
+	/* Specify that its pages be allocated below 4GB */
+	mapping = backing->gem.filp->f_path.dentry->d_inode->i_mapping;
+	mapping_set_gfp_mask(mapping, GFP_USER | __GFP_DMA32);
+
 	return backing;
 }
 
--- 3.1-rc9/drivers/staging/gma500/gem.c	2011-08-07 23:44:38.587914954 -0700
+++ linux/drivers/staging/gma500/gem.c	2011-10-10 10:39:31.974219007 -0700
@@ -104,6 +104,7 @@ unlock:
 static int psb_gem_create(struct drm_file *file,
 	struct drm_device *dev, uint64_t size, uint32_t *handlep)
 {
+	struct address_space *mapping;
 	struct gtt_range *r;
 	int ret;
 	u32 handle;
@@ -125,6 +126,9 @@ static int psb_gem_create(struct drm_fil
 		dev_err(dev->dev, "GEM init failed for %lld\n", size);
 		return -ENOMEM;
 	}
+	/* Specify that its pages be allocated below 4GB */
+	mapping = r->gem.filp->f_path.dentry->d_inode->i_mapping;
+	mapping_set_gfp_mask(mapping, GFP_USER | __GFP_DMA32);
 	/* Give the object a handle so we can carry it more easily */
 	ret = drm_gem_handle_create(file, &r->gem, &handle);
 	if (ret) {
--- 3.1-rc9/drivers/staging/gma500/gtt.c	2011-08-07 23:44:38.591914970 -0700
+++ linux/drivers/staging/gma500/gtt.c	2011-10-10 10:19:31.424265313 -0700
@@ -20,6 +20,7 @@
  */
 
 #include <drm/drmP.h>
+#include <linux/shmem_fs.h>
 #include "psb_drv.h"
 
 
@@ -158,9 +159,7 @@ static int psb_gtt_attach_pages(struct g
 	gt->npage = pages;
 
 	for (i = 0; i < pages; i++) {
-		/* FIXME: review flags later */
-		p = read_cache_page_gfp(mapping, i,
-					__GFP_COLD | GFP_KERNEL);
+		p = shmem_read_mapping_page(mapping, i);
 		if (IS_ERR(p))
 			goto err;
 		gt->pages[i] = p;