Re: [PATCH 6/6] drm/gma500: use common functions for get/put pages

From: Alan Cox <alan-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org>
To: Rob Clark <rob.clark-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Cc: linaro-dev-cunTk1MwBs8s++Sfvej+rw@public.gmane.org,
	dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
Subject: Re: [PATCH 6/6] drm/gma500: use common functions for get/put pages
Date: Mon, 12 Sep 2011 23:25:29 +0100	[thread overview]
Message-ID: <20110912232529.141c8cf6@lxorguk.ukuu.org.uk> (raw)
In-Reply-To: <CAF6AEGva8Ggo8W+f_bfNhs8L27KHZh8a3Gro8M_s1A_p_yALFQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

> > 3. GMA500 used the old way of doing things because last mail conversation
> > I had with Hugh the cleaned up interfaces could not guarantee the page is
> > mapped in the low 32bits and for any of the GMA500/600 series devices.
> >
> > Has that changed ? I think I'd also prefer it if the methods had a
> > BUG_ON() getting a high page when they asked for DMA32.
> 
> Hmm, ok, I found this thread, which is I guess what you are referring to:
> 
>   https://lkml.org/lkml/2011/7/11/269
> 
> I'm not really sure if anything has changed.. But it doesn't look like
> the gma500 driver ever unpins anything.. so I guess it isn't a problem
> (yet).  (Well, either that or I am overlooking something.)

It unpins objects when they are removed from the GTT.

> btw, I could be missing something, but it seems like as long as you
> remove the pages from any userspace mmap'ing before you unpin the
> pages, that you shouldn't hit the case of page getting swapped back in

I don't see how you can guarantee another task won't page it in or have
it referenced (eg via shmem and GEM's FLINK feature). So it's pretty hard
to do.

> >4gb.. if you are always in control of bringing the page back into
> memory, you can ensure that DMA32 is always specified.  Not sure if
> that helps at all.  But at some point in the not too distant future,
> I'll be in the same boat so I am interested in a good way to handle
> this.

Might be worth following up with Hugh if you also need it. Right now we
kind of dodge it on GMA500 as you can't buy a box with that much memory
attached to one so its theoretical for the moment.

We did briefly discuss offlist but didn't chase it down because it was
theoretical for the moment. One possibility would be for the shmem/vfs
code to be smart enough to invalidate and move the page.

> re: BUG_ON():
> I wonder if a check would better belong in
> shmem_read_mapping_page_gfp() or shmem_getpage_gfp()?

Agreed. The BUG_ON is sufficient for GMA500 itself - just so if we ever
forget about it we bug rather than do something very nasty.