[PATCH i-g-t] tests/gem_mmap_gtt: Use PAGE_SIZE instead of hard coded value

[PATCH i-g-t] tests/gem_mmap_gtt: Use PAGE_SIZE instead of hard coded value

[Joonas Lahtinen]

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(-)

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



_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] tests/gem_mmap_gtt: Use PAGE_SIZE instead of hard coded value

[Thomas Wood]

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
>
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] tests/gem_mmap_gtt: Use PAGE_SIZE instead of hard coded value

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] tests/gem_mmap_gtt: Use PAGE_SIZE instead of hard coded value

[Joonas Lahtinen]

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
> 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] tests/gem_mmap_gtt: Use PAGE_SIZE instead of hard coded value

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] tests/gem_mmap_gtt: Use PAGE_SIZE instead of hard coded value

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] tests/gem_mmap_gtt: Use PAGE_SIZE instead of hard coded value

[Jesse Barnes]

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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx