| Message ID | 1429861138.4424.0.camel@jlahtine-mobl1 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 24 April 2015 at 08:38, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote: > Now that there is PAGE_SIZE define, use it. Thanks, I've pushed this patch. I also noticed PAGE_SIZE gets defined in several tests, so at some point it might be worth moving it into the library. > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > --- > tests/gem_mmap_gtt.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/tests/gem_mmap_gtt.c b/tests/gem_mmap_gtt.c > index e80f52e..92fa644 100644 > --- a/tests/gem_mmap_gtt.c > +++ b/tests/gem_mmap_gtt.c > @@ -148,24 +148,25 @@ test_short(int fd) > igt_assert(drmIoctl(fd, > DRM_IOCTL_I915_GEM_MMAP_GTT, > &mmap_arg) == 0); > - for (pages = 1; pages <= OBJECT_SIZE / 4096; pages <<= 1) { > + for (pages = 1; pages <= OBJECT_SIZE / PAGE_SIZE; pages <<= 1) { > uint8_t *r, *w; > > - w = mmap64(0, pages * 4096, PROT_READ | PROT_WRITE, > + w = mmap64(0, pages * PAGE_SIZE, PROT_READ | PROT_WRITE, > MAP_SHARED, fd, mmap_arg.offset); > igt_assert(w != MAP_FAILED); > > - r = mmap64(0, pages * 4096, PROT_READ, > + r = mmap64(0, pages * PAGE_SIZE, PROT_READ, > MAP_SHARED, fd, mmap_arg.offset); > igt_assert(r != MAP_FAILED); > > for (p = 0; p < pages; p++) { > - w[4096*p] = r[4096*p]; > - w[4096*p+4095] = r[4096*p+4095]; > + w[p*PAGE_SIZE] = r[p*PAGE_SIZE]; > + w[p*PAGE_SIZE+(PAGE_SIZE-1)] = > + r[p*PAGE_SIZE+(PAGE_SIZE-1)]; > } > > - munmap(r, pages * 4096); > - munmap(w, pages * 4096); > + munmap(r, pages * PAGE_SIZE); > + munmap(w, pages * PAGE_SIZE); > } > gem_close(fd, mmap_arg.handle); > } > -- > 1.9.3 > > >
On Mon, Apr 27, 2015 at 06:35:54PM +0100, Thomas Wood wrote: > On 24 April 2015 at 08:38, Joonas Lahtinen > <joonas.lahtinen@linux.intel.com> wrote: > > Now that there is PAGE_SIZE define, use it. > > Thanks, I've pushed this patch. I also noticed PAGE_SIZE gets defined > in several tests, so at some point it might be worth moving it into > the library. Be wary of these though. PAGE_SIZE should only ever be used wrt to struct page and not GPU pages. If you must, please use GTT_PAGE_SIZE instead. -Chris
On ma, 2015-04-27 at 20:43 +0100, Chris Wilson wrote: > On Mon, Apr 27, 2015 at 06:35:54PM +0100, Thomas Wood wrote: > > On 24 April 2015 at 08:38, Joonas Lahtinen > > <joonas.lahtinen@linux.intel.com> wrote: > > > Now that there is PAGE_SIZE define, use it. > > > > Thanks, I've pushed this patch. I also noticed PAGE_SIZE gets defined > > in several tests, so at some point it might be worth moving it into > > the library. > > Be wary of these though. PAGE_SIZE should only ever be used wrt to struct > page and not GPU pages. If you must, please use GTT_PAGE_SIZE instead. Do we have a platform/case where these are different? Just asking out of curiosity :) And the use above was to a mmaped area, which I think should be considered to behave like CPU paged memory? Otherwise kind of defeats mmap purpose. Regards, Joonas > -Chris >
On Thu, Apr 30, 2015 at 01:28:46PM +0300, Joonas Lahtinen wrote: > On ma, 2015-04-27 at 20:43 +0100, Chris Wilson wrote: > > On Mon, Apr 27, 2015 at 06:35:54PM +0100, Thomas Wood wrote: > > > On 24 April 2015 at 08:38, Joonas Lahtinen > > > <joonas.lahtinen@linux.intel.com> wrote: > > > > Now that there is PAGE_SIZE define, use it. > > > > > > Thanks, I've pushed this patch. I also noticed PAGE_SIZE gets defined > > > in several tests, so at some point it might be worth moving it into > > > the library. > > > > Be wary of these though. PAGE_SIZE should only ever be used wrt to struct > > page and not GPU pages. If you must, please use GTT_PAGE_SIZE instead. > > Do we have a platform/case where these are different? Just asking out of > curiosity :) Yes. We just haven't enabled big pages yet. The thought of getting globs of 64k contiguous physical memory isn't too appealing, but like with hugepages there are likely enough tasks that benefit. > And the use above was to a mmaped area, which I think should be > considered to behave like CPU paged memory? Otherwise kind of defeats > mmap purpose. I was making the observation just in case someone wanted to go through the whole code base fixing up the hardcoded 4096 constants. -Chris
On Thu, Apr 30, 2015 at 12:22:56PM +0100, Chris Wilson wrote: > On Thu, Apr 30, 2015 at 01:28:46PM +0300, Joonas Lahtinen wrote: > > On ma, 2015-04-27 at 20:43 +0100, Chris Wilson wrote: > > > On Mon, Apr 27, 2015 at 06:35:54PM +0100, Thomas Wood wrote: > > > > On 24 April 2015 at 08:38, Joonas Lahtinen > > > > <joonas.lahtinen@linux.intel.com> wrote: > > > > > Now that there is PAGE_SIZE define, use it. > > > > > > > > Thanks, I've pushed this patch. I also noticed PAGE_SIZE gets defined > > > > in several tests, so at some point it might be worth moving it into > > > > the library. > > > > > > Be wary of these though. PAGE_SIZE should only ever be used wrt to struct > > > page and not GPU pages. If you must, please use GTT_PAGE_SIZE instead. > > > > Do we have a platform/case where these are different? Just asking out of > > curiosity :) > > Yes. We just haven't enabled big pages yet. The thought of getting globs > of 64k contiguous physical memory isn't too appealing, but like with > hugepages there are likely enough tasks that benefit. I thought the verdict thus far was that hw engineers overspecced tlbs and 64k pages aren't really worth it except in some corner-case video code workloads. Might have changed with the gen8+ pagetables, but I haven't seen any new noises about this. -Daniel
On 05/04/2015 07:16 AM, Daniel Vetter wrote: > On Thu, Apr 30, 2015 at 12:22:56PM +0100, Chris Wilson wrote: >> On Thu, Apr 30, 2015 at 01:28:46PM +0300, Joonas Lahtinen wrote: >>> On ma, 2015-04-27 at 20:43 +0100, Chris Wilson wrote: >>>> On Mon, Apr 27, 2015 at 06:35:54PM +0100, Thomas Wood wrote: >>>>> On 24 April 2015 at 08:38, Joonas Lahtinen >>>>> <joonas.lahtinen@linux.intel.com> wrote: >>>>>> Now that there is PAGE_SIZE define, use it. >>>>> >>>>> Thanks, I've pushed this patch. I also noticed PAGE_SIZE gets defined >>>>> in several tests, so at some point it might be worth moving it into >>>>> the library. >>>> >>>> Be wary of these though. PAGE_SIZE should only ever be used wrt to struct >>>> page and not GPU pages. If you must, please use GTT_PAGE_SIZE instead. >>> >>> Do we have a platform/case where these are different? Just asking out of >>> curiosity :) >> >> Yes. We just haven't enabled big pages yet. The thought of getting globs >> of 64k contiguous physical memory isn't too appealing, but like with >> hugepages there are likely enough tasks that benefit. > > I thought the verdict thus far was that hw engineers overspecced tlbs and > 64k pages aren't really worth it except in some corner-case video code > workloads. Might have changed with the gen8+ pagetables, but I haven't > seen any new noises about this. I hadn't heard that; Damien looked at this awhile back but I'm not sure if he got to the point of getting perf numbers. Those would be nice... there's a lot of added complexity, but if our media processing overhead goes down by 20% it would probably be worth it! Jesse
diff --git a/tests/gem_mmap_gtt.c b/tests/gem_mmap_gtt.c index e80f52e..92fa644 100644 --- a/tests/gem_mmap_gtt.c +++ b/tests/gem_mmap_gtt.c @@ -148,24 +148,25 @@ test_short(int fd) igt_assert(drmIoctl(fd, DRM_IOCTL_I915_GEM_MMAP_GTT, &mmap_arg) == 0); - for (pages = 1; pages <= OBJECT_SIZE / 4096; pages <<= 1) { + for (pages = 1; pages <= OBJECT_SIZE / PAGE_SIZE; pages <<= 1) { uint8_t *r, *w; - w = mmap64(0, pages * 4096, PROT_READ | PROT_WRITE, + w = mmap64(0, pages * PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd, mmap_arg.offset); igt_assert(w != MAP_FAILED); - r = mmap64(0, pages * 4096, PROT_READ, + r = mmap64(0, pages * PAGE_SIZE, PROT_READ, MAP_SHARED, fd, mmap_arg.offset); igt_assert(r != MAP_FAILED); for (p = 0; p < pages; p++) { - w[4096*p] = r[4096*p]; - w[4096*p+4095] = r[4096*p+4095]; + w[p*PAGE_SIZE] = r[p*PAGE_SIZE]; + w[p*PAGE_SIZE+(PAGE_SIZE-1)] = + r[p*PAGE_SIZE+(PAGE_SIZE-1)]; } - munmap(r, pages * 4096); - munmap(w, pages * 4096); + munmap(r, pages * PAGE_SIZE); + munmap(w, pages * PAGE_SIZE); } gem_close(fd, mmap_arg.handle); }
Now that there is PAGE_SIZE define, use it. Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> --- tests/gem_mmap_gtt.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)