Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 9/9] i915: Exercise I915_CONTEXT_PARAM_RINGSIZE
Quoting Janusz Krzysztofik (2020-02-20 15:57:24) > Hi Chris, > > On Monday, December 2, 2019 3:59:19 PM CET Chris Wilson wrote: > > Quoting Janusz Krzysztofik (2019-12-02 14:42:58) > > > Hi Chris, > > > > > > I have a few questions rather than comments. I hope they are worth > > > spending > > > your time. > > > > > > On Wednesday, November 13, 2019 1:52:40 PM CET Chris Wilson wrote: > > > > I915_CONTEXT_PARAM_RINGSIZE specifies how large to create the command > > > > ringbuffer for logical ring contects. This directly affects the number > > > > > > s/contects/contexts/ > > > > > > > of batches userspace can submit before blocking waiting for space. > > > > > > > > Signed-off-by: Chris Wilson > > Have you got this patch still queued somewhere? As UMD has accepted the > solution and are ready with changes on their side, I think we need to merge > it > soon, and the kernel side as well. Link? That's all I need to merge the kernel... -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 9/9] i915: Exercise I915_CONTEXT_PARAM_RINGSIZE
Hi Chris,
On Monday, December 2, 2019 3:59:19 PM CET Chris Wilson wrote:
> Quoting Janusz Krzysztofik (2019-12-02 14:42:58)
> > Hi Chris,
> >
> > I have a few questions rather than comments. I hope they are worth
> > spending
> > your time.
> >
> > On Wednesday, November 13, 2019 1:52:40 PM CET Chris Wilson wrote:
> > > I915_CONTEXT_PARAM_RINGSIZE specifies how large to create the command
> > > ringbuffer for logical ring contects. This directly affects the number
> >
> > s/contects/contexts/
> >
> > > of batches userspace can submit before blocking waiting for space.
> > >
> > > Signed-off-by: Chris Wilson
Have you got this patch still queued somewhere? As UMD has accepted the
solution and are ready with changes on their side, I think we need to merge it
soon, and the kernel side as well.
Thanks,
Janusz
> > > ---
> > > tests/Makefile.sources| 3 +
> > > tests/i915/gem_ctx_ringsize.c | 296 ++
> > > tests/meson.build | 1 +
> > > 3 files changed, 300 insertions(+)
> > > create mode 100644 tests/i915/gem_ctx_ringsize.c
> > >
> > > diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> > > index e17d43155..801fc52f3 100644
> > > --- a/tests/Makefile.sources
> > > +++ b/tests/Makefile.sources
> > > @@ -163,6 +163,9 @@ gem_ctx_param_SOURCES = i915/gem_ctx_param.c
> > > TESTS_progs += gem_ctx_persistence
> > > gem_ctx_persistence_SOURCES = i915/gem_ctx_persistence.c
> > >
> > > +TESTS_progs += gem_ctx_ringsize
> > > +gem_ctx_ringsize_SOURCES = i915/gem_ctx_ringsize.c
> > > +
> > > TESTS_progs += gem_ctx_shared
> > > gem_ctx_shared_SOURCES = i915/gem_ctx_shared.c
> > >
> > > diff --git a/tests/i915/gem_ctx_ringsize.c b/tests/i915/gem_ctx_ringsize.c
> > > new file mode 100644
> > > index 0..1450e8f0d
> > > --- /dev/null
> > > +++ b/tests/i915/gem_ctx_ringsize.c
> > > @@ -0,0 +1,296 @@
> > > +/*
> > > + * Copyright © 2019 Intel Corporation
> > > + *
> > > + * Permission is hereby granted, free of charge, to any person obtaining
> > > a
> > > + * copy of this software and associated documentation files (the
> > > "Software"),
> > > + * to deal in the Software without restriction, including without
> > > limitation
> > > + * the rights to use, copy, modify, merge, publish, distribute,
> > > sublicense,
> > > + * and/or sell copies of the Software, and to permit persons to whom the
> > > + * Software is furnished to do so, subject to the following conditions:
> > > + *
> > > + * The above copyright notice and this permission notice (including the
> > > next
> > > + * paragraph) shall be included in all copies or substantial portions of
> > > the
> > > + * Software.
> > > + *
> > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> > > EXPRESS OR
> > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > > MERCHANTABILITY,
> > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT
> > > SHALL
> > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> > > OTHER
> > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > > ARISING
> > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > > DEALINGS
> > > + * IN THE SOFTWARE.
> > > + */
> > > +
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +#include
> > > +
> > > +#include "drmtest.h" /* gem_quiescent_gpu()! */
> > > +#include "i915/gem_context.h"
> > > +#include "i915/gem_engine_topology.h"
> > > +#include "ioctl_wrappers.h" /* gem_wait()! */
> > > +#include "sw_sync.h"
> > > +
> > > +#define I915_CONTEXT_PARAM_RINGSIZE 0xc
> >
> > How are we going to handle symbol redefinition conflict which arises as
> > soon
> > as this symbol is also included from kernel headers (e.g. via
> > "i915/gem_engine_topology.h")?
>
> Final version we copy the headers form the kernel. Conflicts remind us
> when we forget.
>
> >
> > > +
> > > +static bool has_ringsize(int i915)
> > > +{
> > > + struct drm_i915_gem_context_param p = {
> > > + .param = I915_CONTEXT_PARAM_RINGSIZE,
> > > + };
> > > +
> > > + return __gem_context_get_param(i915, ) == 0;
> > > +}
> > > +
> > > +static void test_idempotent(int i915)
> > > +{
> > > + struct drm_i915_gem_context_param p = {
> > > + .param = I915_CONTEXT_PARAM_RINGSIZE,
> > > + };
> > > + uint32_t saved;
> > > +
> > > + /*
> > > + * Simple test to verify that we are able to read back the same
> > > + * value as we set.
> > > + */
> > > +
> > > + gem_context_get_param(i915, );
> > > + saved = p.value;
> > > +
> > > + for (uint32_t x = 1 << 12; x <= 128 << 12; x <<= 1) {
> >
> > I've noticed you are using two different notations for those
> > minimum/maximum
> > constants. I think that may be confusing. How about defining and using
> > macros?
>
> A range in pages...
>
> > > +
Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 9/9] i915: Exercise I915_CONTEXT_PARAM_RINGSIZE
Quoting Janusz Krzysztofik (2019-12-02 14:42:58)
> Hi Chris,
>
> I have a few questions rather than comments. I hope they are worth spending
> your time.
>
> On Wednesday, November 13, 2019 1:52:40 PM CET Chris Wilson wrote:
> > I915_CONTEXT_PARAM_RINGSIZE specifies how large to create the command
> > ringbuffer for logical ring contects. This directly affects the number
>
> s/contects/contexts/
>
> > of batches userspace can submit before blocking waiting for space.
> >
> > Signed-off-by: Chris Wilson
> > ---
> > tests/Makefile.sources| 3 +
> > tests/i915/gem_ctx_ringsize.c | 296 ++
> > tests/meson.build | 1 +
> > 3 files changed, 300 insertions(+)
> > create mode 100644 tests/i915/gem_ctx_ringsize.c
> >
> > diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> > index e17d43155..801fc52f3 100644
> > --- a/tests/Makefile.sources
> > +++ b/tests/Makefile.sources
> > @@ -163,6 +163,9 @@ gem_ctx_param_SOURCES = i915/gem_ctx_param.c
> > TESTS_progs += gem_ctx_persistence
> > gem_ctx_persistence_SOURCES = i915/gem_ctx_persistence.c
> >
> > +TESTS_progs += gem_ctx_ringsize
> > +gem_ctx_ringsize_SOURCES = i915/gem_ctx_ringsize.c
> > +
> > TESTS_progs += gem_ctx_shared
> > gem_ctx_shared_SOURCES = i915/gem_ctx_shared.c
> >
> > diff --git a/tests/i915/gem_ctx_ringsize.c b/tests/i915/gem_ctx_ringsize.c
> > new file mode 100644
> > index 0..1450e8f0d
> > --- /dev/null
> > +++ b/tests/i915/gem_ctx_ringsize.c
> > @@ -0,0 +1,296 @@
> > +/*
> > + * Copyright © 2019 Intel Corporation
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the
> > "Software"),
> > + * to deal in the Software without restriction, including without
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the
> > next
> > + * paragraph) shall be included in all copies or substantial portions of
> > the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> > OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> > OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> > DEALINGS
> > + * IN THE SOFTWARE.
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include "drmtest.h" /* gem_quiescent_gpu()! */
> > +#include "i915/gem_context.h"
> > +#include "i915/gem_engine_topology.h"
> > +#include "ioctl_wrappers.h" /* gem_wait()! */
> > +#include "sw_sync.h"
> > +
> > +#define I915_CONTEXT_PARAM_RINGSIZE 0xc
>
> How are we going to handle symbol redefinition conflict which arises as soon
> as this symbol is also included from kernel headers (e.g. via
> "i915/gem_engine_topology.h")?
Final version we copy the headers form the kernel. Conflicts remind us
when we forget.
>
> > +
> > +static bool has_ringsize(int i915)
> > +{
> > + struct drm_i915_gem_context_param p = {
> > + .param = I915_CONTEXT_PARAM_RINGSIZE,
> > + };
> > +
> > + return __gem_context_get_param(i915, ) == 0;
> > +}
> > +
> > +static void test_idempotent(int i915)
> > +{
> > + struct drm_i915_gem_context_param p = {
> > + .param = I915_CONTEXT_PARAM_RINGSIZE,
> > + };
> > + uint32_t saved;
> > +
> > + /*
> > + * Simple test to verify that we are able to read back the same
> > + * value as we set.
> > + */
> > +
> > + gem_context_get_param(i915, );
> > + saved = p.value;
> > +
> > + for (uint32_t x = 1 << 12; x <= 128 << 12; x <<= 1) {
>
> I've noticed you are using two different notations for those minimum/maximum
> constants. I think that may be confusing. How about defining and using
> macros?
A range in pages...
> > + p.value = x;
> > + gem_context_set_param(i915, );
> > + gem_context_get_param(i915, );
> > + igt_assert_eq_u32(p.value, x);
> > + }
> > +
> > + p.value = saved;
> > + gem_context_set_param(i915, );
> > +}
> > +
> > +static void test_invalid(int i915)
> > +{
> > + struct drm_i915_gem_context_param p = {
> > + .param = I915_CONTEXT_PARAM_RINGSIZE,
> > + };
> > + uint64_t invalid[] = {
> > + 0, 1, 4095, 4097, 8191, 8193,
> > + /* upper limit may be HW dependent, atm it is 512KiB */
> > +
Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 9/9] i915: Exercise I915_CONTEXT_PARAM_RINGSIZE
Hi Chris,
I have a few questions rather than comments. I hope they are worth spending
your time.
On Wednesday, November 13, 2019 1:52:40 PM CET Chris Wilson wrote:
> I915_CONTEXT_PARAM_RINGSIZE specifies how large to create the command
> ringbuffer for logical ring contects. This directly affects the number
s/contects/contexts/
> of batches userspace can submit before blocking waiting for space.
>
> Signed-off-by: Chris Wilson
> ---
> tests/Makefile.sources| 3 +
> tests/i915/gem_ctx_ringsize.c | 296 ++
> tests/meson.build | 1 +
> 3 files changed, 300 insertions(+)
> create mode 100644 tests/i915/gem_ctx_ringsize.c
>
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index e17d43155..801fc52f3 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -163,6 +163,9 @@ gem_ctx_param_SOURCES = i915/gem_ctx_param.c
> TESTS_progs += gem_ctx_persistence
> gem_ctx_persistence_SOURCES = i915/gem_ctx_persistence.c
>
> +TESTS_progs += gem_ctx_ringsize
> +gem_ctx_ringsize_SOURCES = i915/gem_ctx_ringsize.c
> +
> TESTS_progs += gem_ctx_shared
> gem_ctx_shared_SOURCES = i915/gem_ctx_shared.c
>
> diff --git a/tests/i915/gem_ctx_ringsize.c b/tests/i915/gem_ctx_ringsize.c
> new file mode 100644
> index 0..1450e8f0d
> --- /dev/null
> +++ b/tests/i915/gem_ctx_ringsize.c
> @@ -0,0 +1,296 @@
> +/*
> + * Copyright © 2019 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "drmtest.h" /* gem_quiescent_gpu()! */
> +#include "i915/gem_context.h"
> +#include "i915/gem_engine_topology.h"
> +#include "ioctl_wrappers.h" /* gem_wait()! */
> +#include "sw_sync.h"
> +
> +#define I915_CONTEXT_PARAM_RINGSIZE 0xc
How are we going to handle symbol redefinition conflict which arises as soon
as this symbol is also included from kernel headers (e.g. via
"i915/gem_engine_topology.h")?
> +
> +static bool has_ringsize(int i915)
> +{
> + struct drm_i915_gem_context_param p = {
> + .param = I915_CONTEXT_PARAM_RINGSIZE,
> + };
> +
> + return __gem_context_get_param(i915, ) == 0;
> +}
> +
> +static void test_idempotent(int i915)
> +{
> + struct drm_i915_gem_context_param p = {
> + .param = I915_CONTEXT_PARAM_RINGSIZE,
> + };
> + uint32_t saved;
> +
> + /*
> + * Simple test to verify that we are able to read back the same
> + * value as we set.
> + */
> +
> + gem_context_get_param(i915, );
> + saved = p.value;
> +
> + for (uint32_t x = 1 << 12; x <= 128 << 12; x <<= 1) {
I've noticed you are using two different notations for those minimum/maximum
constants. I think that may be confusing. How about defining and using
macros?
> + p.value = x;
> + gem_context_set_param(i915, );
> + gem_context_get_param(i915, );
> + igt_assert_eq_u32(p.value, x);
> + }
> +
> + p.value = saved;
> + gem_context_set_param(i915, );
> +}
> +
> +static void test_invalid(int i915)
> +{
> + struct drm_i915_gem_context_param p = {
> + .param = I915_CONTEXT_PARAM_RINGSIZE,
> + };
> + uint64_t invalid[] = {
> + 0, 1, 4095, 4097, 8191, 8193,
> + /* upper limit may be HW dependent, atm it is 512KiB */
> + (512 << 10) - 1, (512 << 10) + 1,
Here is an example of that different notation mentioned above.
> + -1, -1u
> + };
> + uint32_t saved;
> +
> + gem_context_get_param(i915, );
> + saved = p.value;
> +
> + for (int i = 0; i < ARRAY_SIZE(invalid); i++) {
> + p.value = invalid[i];
> + igt_assert_eq(__gem_context_set_param(i915, ), -EINVAL);
> + gem_context_get_param(i915, );
> +
