* [PATCH i-g-t] i915/gem_mmap_gtt: Markup a copy of GTT set-domains
@ 2019-04-13 13:27 Chris Wilson
2019-04-15 12:23 ` [igt-dev] " Ville Syrjälä
0 siblings, 1 reply; 4+ messages in thread
From: Chris Wilson @ 2019-04-13 13:27 UTC (permalink / raw)
To: intel-gfx; +Cc: igt-dev
We have to control the cache domains, especially important before first
writing into the object.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
tests/i915/gem_mmap_gtt.c | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/tests/i915/gem_mmap_gtt.c b/tests/i915/gem_mmap_gtt.c
index 58922ee32..ab7d3f2d0 100644
--- a/tests/i915/gem_mmap_gtt.c
+++ b/tests/i915/gem_mmap_gtt.c
@@ -678,27 +678,26 @@ test_huge_copy(int fd, int huge, int tiling_a, int tiling_b, int ncpus)
igt_fork(child, ncpus) {
uint64_t valid_size = huge_object_size;
- uint32_t bo;
+ uint32_t bo[2];
char *a, *b;
- bo = gem_create(fd, huge_object_size);
+ bo[0] = gem_create(fd, huge_object_size);
if (tiling_a) {
- igt_require(__gem_set_tiling(fd, bo, abs(tiling_a), min_tile_width(devid, tiling_a)) == 0);
+ igt_require(__gem_set_tiling(fd, bo[0], abs(tiling_a), min_tile_width(devid, tiling_a)) == 0);
valid_size = rounddown(valid_size, tile_row_size(tiling_a, min_tile_width(devid, tiling_a)));
}
- a = __gem_mmap__gtt(fd, bo, huge_object_size, PROT_READ | PROT_WRITE);
+ a = __gem_mmap__gtt(fd, bo[0], huge_object_size, PROT_READ | PROT_WRITE);
igt_require(a);
- gem_close(fd, bo);
- bo = gem_create(fd, huge_object_size);
+ bo[1] = gem_create(fd, huge_object_size);
if (tiling_b) {
- igt_require(__gem_set_tiling(fd, bo, abs(tiling_b), max_tile_width(devid, tiling_b)) == 0);
+ igt_require(__gem_set_tiling(fd, bo[1], abs(tiling_b), max_tile_width(devid, tiling_b)) == 0);
valid_size = rounddown(valid_size, tile_row_size(tiling_b, max_tile_width(devid, tiling_b)));
}
- b = __gem_mmap__gtt(fd, bo, huge_object_size, PROT_READ | PROT_WRITE);
+ b = __gem_mmap__gtt(fd, bo[1], huge_object_size, PROT_READ | PROT_WRITE);
igt_require(b);
- gem_close(fd, bo);
+ gem_set_domain(fd, bo[0], I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
for (i = 0; i < valid_size / PAGE_SIZE; i++) {
uint32_t *ptr = (uint32_t *)(a + PAGE_SIZE*i);
for (int j = 0; j < PAGE_SIZE/4; j++)
@@ -706,7 +705,7 @@ test_huge_copy(int fd, int huge, int tiling_a, int tiling_b, int ncpus)
igt_progress("Writing a ", i, valid_size / PAGE_SIZE);
}
-
+ gem_set_domain(fd, bo[1], I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
for (i = 0; i < valid_size / PAGE_SIZE; i++) {
uint32_t *ptr = (uint32_t *)(b + PAGE_SIZE*i);
for (int j = 0; j < PAGE_SIZE/4; j++)
@@ -727,12 +726,19 @@ test_huge_copy(int fd, int huge, int tiling_a, int tiling_b, int ncpus)
A_tmp[j] = B_tmp[j];
else
B_tmp[j] = A_tmp[j];
+
+ gem_set_domain(fd, bo[0], I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
memcpy(A, A_tmp, PAGE_SIZE);
+
+ gem_set_domain(fd, bo[1], I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
memcpy(B, B_tmp, PAGE_SIZE);
igt_progress("Copying a<->b ", i, valid_size / PAGE_SIZE);
}
+ gem_close(fd, bo[0]);
+ gem_close(fd, bo[1]);
+
for (i = 0; i < valid_size / PAGE_SIZE; i++) {
uint32_t page[PAGE_SIZE/sizeof(uint32_t)];
copy_wc_page(page, a + PAGE_SIZE*i);
--
2.20.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] i915/gem_mmap_gtt: Markup a copy of GTT set-domains
2019-04-13 13:27 [PATCH i-g-t] i915/gem_mmap_gtt: Markup a copy of GTT set-domains Chris Wilson
@ 2019-04-15 12:23 ` Ville Syrjälä
2019-04-15 13:18 ` Chris Wilson
0 siblings, 1 reply; 4+ messages in thread
From: Ville Syrjälä @ 2019-04-15 12:23 UTC (permalink / raw)
To: Chris Wilson; +Cc: igt-dev, intel-gfx
On Sat, Apr 13, 2019 at 02:27:57PM +0100, Chris Wilson wrote:
> We have to control the cache domains, especially important before first
> writing into the object.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> tests/i915/gem_mmap_gtt.c | 26 ++++++++++++++++----------
> 1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/tests/i915/gem_mmap_gtt.c b/tests/i915/gem_mmap_gtt.c
> index 58922ee32..ab7d3f2d0 100644
> --- a/tests/i915/gem_mmap_gtt.c
> +++ b/tests/i915/gem_mmap_gtt.c
> @@ -678,27 +678,26 @@ test_huge_copy(int fd, int huge, int tiling_a, int tiling_b, int ncpus)
>
> igt_fork(child, ncpus) {
> uint64_t valid_size = huge_object_size;
> - uint32_t bo;
> + uint32_t bo[2];
> char *a, *b;
>
> - bo = gem_create(fd, huge_object_size);
> + bo[0] = gem_create(fd, huge_object_size);
> if (tiling_a) {
> - igt_require(__gem_set_tiling(fd, bo, abs(tiling_a), min_tile_width(devid, tiling_a)) == 0);
> + igt_require(__gem_set_tiling(fd, bo[0], abs(tiling_a), min_tile_width(devid, tiling_a)) == 0);
> valid_size = rounddown(valid_size, tile_row_size(tiling_a, min_tile_width(devid, tiling_a)));
> }
> - a = __gem_mmap__gtt(fd, bo, huge_object_size, PROT_READ | PROT_WRITE);
> + a = __gem_mmap__gtt(fd, bo[0], huge_object_size, PROT_READ | PROT_WRITE);
> igt_require(a);
> - gem_close(fd, bo);
>
> - bo = gem_create(fd, huge_object_size);
> + bo[1] = gem_create(fd, huge_object_size);
> if (tiling_b) {
> - igt_require(__gem_set_tiling(fd, bo, abs(tiling_b), max_tile_width(devid, tiling_b)) == 0);
> + igt_require(__gem_set_tiling(fd, bo[1], abs(tiling_b), max_tile_width(devid, tiling_b)) == 0);
> valid_size = rounddown(valid_size, tile_row_size(tiling_b, max_tile_width(devid, tiling_b)));
> }
> - b = __gem_mmap__gtt(fd, bo, huge_object_size, PROT_READ | PROT_WRITE);
> + b = __gem_mmap__gtt(fd, bo[1], huge_object_size, PROT_READ | PROT_WRITE);
> igt_require(b);
> - gem_close(fd, bo);
>
> + gem_set_domain(fd, bo[0], I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> for (i = 0; i < valid_size / PAGE_SIZE; i++) {
> uint32_t *ptr = (uint32_t *)(a + PAGE_SIZE*i);
> for (int j = 0; j < PAGE_SIZE/4; j++)
> @@ -706,7 +705,7 @@ test_huge_copy(int fd, int huge, int tiling_a, int tiling_b, int ncpus)
> igt_progress("Writing a ", i, valid_size / PAGE_SIZE);
> }
>
> -
> + gem_set_domain(fd, bo[1], I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> for (i = 0; i < valid_size / PAGE_SIZE; i++) {
> uint32_t *ptr = (uint32_t *)(b + PAGE_SIZE*i);
> for (int j = 0; j < PAGE_SIZE/4; j++)
> @@ -727,12 +726,19 @@ test_huge_copy(int fd, int huge, int tiling_a, int tiling_b, int ncpus)
> A_tmp[j] = B_tmp[j];
> else
> B_tmp[j] = A_tmp[j];
> +
> + gem_set_domain(fd, bo[0], I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
why do we repeat this here? I can't see anything that would change the
domains after the first time we set it.
> memcpy(A, A_tmp, PAGE_SIZE);
> +
> + gem_set_domain(fd, bo[1], I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> memcpy(B, B_tmp, PAGE_SIZE);
>
> igt_progress("Copying a<->b ", i, valid_size / PAGE_SIZE);
> }
>
> + gem_close(fd, bo[0]);
> + gem_close(fd, bo[1]);
> +
> for (i = 0; i < valid_size / PAGE_SIZE; i++) {
> uint32_t page[PAGE_SIZE/sizeof(uint32_t)];
> copy_wc_page(page, a + PAGE_SIZE*i);
> --
> 2.20.1
>
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] i915/gem_mmap_gtt: Markup a copy of GTT set-domains
2019-04-15 12:23 ` [igt-dev] " Ville Syrjälä
@ 2019-04-15 13:18 ` Chris Wilson
2019-04-15 13:29 ` Ville Syrjälä
0 siblings, 1 reply; 4+ messages in thread
From: Chris Wilson @ 2019-04-15 13:18 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: igt-dev, intel-gfx
Quoting Ville Syrjälä (2019-04-15 13:23:54)
> On Sat, Apr 13, 2019 at 02:27:57PM +0100, Chris Wilson wrote:
> > We have to control the cache domains, especially important before first
> > writing into the object.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> > tests/i915/gem_mmap_gtt.c | 26 ++++++++++++++++----------
> > 1 file changed, 16 insertions(+), 10 deletions(-)
> >
> > diff --git a/tests/i915/gem_mmap_gtt.c b/tests/i915/gem_mmap_gtt.c
> > index 58922ee32..ab7d3f2d0 100644
> > --- a/tests/i915/gem_mmap_gtt.c
> > +++ b/tests/i915/gem_mmap_gtt.c
> > @@ -678,27 +678,26 @@ test_huge_copy(int fd, int huge, int tiling_a, int tiling_b, int ncpus)
> >
> > igt_fork(child, ncpus) {
> > uint64_t valid_size = huge_object_size;
> > - uint32_t bo;
> > + uint32_t bo[2];
> > char *a, *b;
> >
> > - bo = gem_create(fd, huge_object_size);
> > + bo[0] = gem_create(fd, huge_object_size);
> > if (tiling_a) {
> > - igt_require(__gem_set_tiling(fd, bo, abs(tiling_a), min_tile_width(devid, tiling_a)) == 0);
> > + igt_require(__gem_set_tiling(fd, bo[0], abs(tiling_a), min_tile_width(devid, tiling_a)) == 0);
> > valid_size = rounddown(valid_size, tile_row_size(tiling_a, min_tile_width(devid, tiling_a)));
> > }
> > - a = __gem_mmap__gtt(fd, bo, huge_object_size, PROT_READ | PROT_WRITE);
> > + a = __gem_mmap__gtt(fd, bo[0], huge_object_size, PROT_READ | PROT_WRITE);
> > igt_require(a);
> > - gem_close(fd, bo);
> >
> > - bo = gem_create(fd, huge_object_size);
> > + bo[1] = gem_create(fd, huge_object_size);
> > if (tiling_b) {
> > - igt_require(__gem_set_tiling(fd, bo, abs(tiling_b), max_tile_width(devid, tiling_b)) == 0);
> > + igt_require(__gem_set_tiling(fd, bo[1], abs(tiling_b), max_tile_width(devid, tiling_b)) == 0);
> > valid_size = rounddown(valid_size, tile_row_size(tiling_b, max_tile_width(devid, tiling_b)));
> > }
> > - b = __gem_mmap__gtt(fd, bo, huge_object_size, PROT_READ | PROT_WRITE);
> > + b = __gem_mmap__gtt(fd, bo[1], huge_object_size, PROT_READ | PROT_WRITE);
> > igt_require(b);
> > - gem_close(fd, bo);
> >
> > + gem_set_domain(fd, bo[0], I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> > for (i = 0; i < valid_size / PAGE_SIZE; i++) {
> > uint32_t *ptr = (uint32_t *)(a + PAGE_SIZE*i);
> > for (int j = 0; j < PAGE_SIZE/4; j++)
> > @@ -706,7 +705,7 @@ test_huge_copy(int fd, int huge, int tiling_a, int tiling_b, int ncpus)
> > igt_progress("Writing a ", i, valid_size / PAGE_SIZE);
> > }
> >
> > -
> > + gem_set_domain(fd, bo[1], I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> > for (i = 0; i < valid_size / PAGE_SIZE; i++) {
> > uint32_t *ptr = (uint32_t *)(b + PAGE_SIZE*i);
> > for (int j = 0; j < PAGE_SIZE/4; j++)
> > @@ -727,12 +726,19 @@ test_huge_copy(int fd, int huge, int tiling_a, int tiling_b, int ncpus)
> > A_tmp[j] = B_tmp[j];
> > else
> > B_tmp[j] = A_tmp[j];
> > +
> > + gem_set_domain(fd, bo[0], I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
>
> why do we repeat this here? I can't see anything that would change the
> domains after the first time we set it.
Just nervous; the one aspect I thoroughly dislike about set-domain is
that it is one sided and does not define a critical section over which
its applies. As such, anything else may change the domain in the middle
of our operation, and that unnerves me.
Making it double-sided has the dilemma of how best to lie to userspace
when you steal control, or how to fixup userspace who forgot to release
their lock.
It's easy to say not to repeat this in future api; defining what else
should be done instead is trickier. At present, such sketches all imply
that userspace is entirely responsible for controlling coherency with
consumers and swapping remains an invisible cache flush.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] i915/gem_mmap_gtt: Markup a copy of GTT set-domains
2019-04-15 13:18 ` Chris Wilson
@ 2019-04-15 13:29 ` Ville Syrjälä
0 siblings, 0 replies; 4+ messages in thread
From: Ville Syrjälä @ 2019-04-15 13:29 UTC (permalink / raw)
To: Chris Wilson; +Cc: igt-dev, intel-gfx
On Mon, Apr 15, 2019 at 02:18:28PM +0100, Chris Wilson wrote:
> Quoting Ville Syrjälä (2019-04-15 13:23:54)
> > On Sat, Apr 13, 2019 at 02:27:57PM +0100, Chris Wilson wrote:
> > > We have to control the cache domains, especially important before first
> > > writing into the object.
> > >
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > > tests/i915/gem_mmap_gtt.c | 26 ++++++++++++++++----------
> > > 1 file changed, 16 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/tests/i915/gem_mmap_gtt.c b/tests/i915/gem_mmap_gtt.c
> > > index 58922ee32..ab7d3f2d0 100644
> > > --- a/tests/i915/gem_mmap_gtt.c
> > > +++ b/tests/i915/gem_mmap_gtt.c
> > > @@ -678,27 +678,26 @@ test_huge_copy(int fd, int huge, int tiling_a, int tiling_b, int ncpus)
> > >
> > > igt_fork(child, ncpus) {
> > > uint64_t valid_size = huge_object_size;
> > > - uint32_t bo;
> > > + uint32_t bo[2];
> > > char *a, *b;
> > >
> > > - bo = gem_create(fd, huge_object_size);
> > > + bo[0] = gem_create(fd, huge_object_size);
> > > if (tiling_a) {
> > > - igt_require(__gem_set_tiling(fd, bo, abs(tiling_a), min_tile_width(devid, tiling_a)) == 0);
> > > + igt_require(__gem_set_tiling(fd, bo[0], abs(tiling_a), min_tile_width(devid, tiling_a)) == 0);
> > > valid_size = rounddown(valid_size, tile_row_size(tiling_a, min_tile_width(devid, tiling_a)));
> > > }
> > > - a = __gem_mmap__gtt(fd, bo, huge_object_size, PROT_READ | PROT_WRITE);
> > > + a = __gem_mmap__gtt(fd, bo[0], huge_object_size, PROT_READ | PROT_WRITE);
> > > igt_require(a);
> > > - gem_close(fd, bo);
> > >
> > > - bo = gem_create(fd, huge_object_size);
> > > + bo[1] = gem_create(fd, huge_object_size);
> > > if (tiling_b) {
> > > - igt_require(__gem_set_tiling(fd, bo, abs(tiling_b), max_tile_width(devid, tiling_b)) == 0);
> > > + igt_require(__gem_set_tiling(fd, bo[1], abs(tiling_b), max_tile_width(devid, tiling_b)) == 0);
> > > valid_size = rounddown(valid_size, tile_row_size(tiling_b, max_tile_width(devid, tiling_b)));
> > > }
> > > - b = __gem_mmap__gtt(fd, bo, huge_object_size, PROT_READ | PROT_WRITE);
> > > + b = __gem_mmap__gtt(fd, bo[1], huge_object_size, PROT_READ | PROT_WRITE);
> > > igt_require(b);
> > > - gem_close(fd, bo);
> > >
> > > + gem_set_domain(fd, bo[0], I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> > > for (i = 0; i < valid_size / PAGE_SIZE; i++) {
> > > uint32_t *ptr = (uint32_t *)(a + PAGE_SIZE*i);
> > > for (int j = 0; j < PAGE_SIZE/4; j++)
> > > @@ -706,7 +705,7 @@ test_huge_copy(int fd, int huge, int tiling_a, int tiling_b, int ncpus)
> > > igt_progress("Writing a ", i, valid_size / PAGE_SIZE);
> > > }
> > >
> > > -
> > > + gem_set_domain(fd, bo[1], I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> > > for (i = 0; i < valid_size / PAGE_SIZE; i++) {
> > > uint32_t *ptr = (uint32_t *)(b + PAGE_SIZE*i);
> > > for (int j = 0; j < PAGE_SIZE/4; j++)
> > > @@ -727,12 +726,19 @@ test_huge_copy(int fd, int huge, int tiling_a, int tiling_b, int ncpus)
> > > A_tmp[j] = B_tmp[j];
> > > else
> > > B_tmp[j] = A_tmp[j];
> > > +
> > > + gem_set_domain(fd, bo[0], I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> >
> > why do we repeat this here? I can't see anything that would change the
> > domains after the first time we set it.
>
> Just nervous; the one aspect I thoroughly dislike about set-domain is
> that it is one sided and does not define a critical section over which
> its applies. As such, anything else may change the domain in the middle
> of our operation, and that unnerves me.
>
> Making it double-sided has the dilemma of how best to lie to userspace
> when you steal control, or how to fixup userspace who forgot to release
> their lock.
>
> It's easy to say not to repeat this in future api; defining what else
> should be done instead is trickier. At present, such sketches all imply
> that userspace is entirely responsible for controlling coherency with
> consumers and swapping remains an invisible cache flush.
Fair enough.
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-04-15 13:29 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-04-13 13:27 [PATCH i-g-t] i915/gem_mmap_gtt: Markup a copy of GTT set-domains Chris Wilson
2019-04-15 12:23 ` [igt-dev] " Ville Syrjälä
2019-04-15 13:18 ` Chris Wilson
2019-04-15 13:29 ` Ville Syrjälä
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox