Re: [Intel-gfx] [I-G-T] igt/gem_mocs_settings: improve RC6 testings
On 22/07/16 10:40, Antoine, Peter wrote:
-Original Message-
From: Chris Wilson [mailto:ch...@chris-wilson.co.uk]
Sent: Friday, July 22, 2016 10:38 AM
To: Antoine, Peter
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [I-G-T] igt/gem_mocs_settings: improve RC6 testings
On Thu, Jul 21, 2016 at 09:49:51PM +, Antoine, Peter wrote:
-Original Message-
From: Chris Wilson [mailto:ch...@chris-wilson.co.uk]
Sent: Thursday, July 21, 2016 9:40 PM
To: Antoine, Peter
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [I-G-T] igt/gem_mocs_settings: improve RC6 testings
On Tue, Jul 19, 2016 at 11:25:29AM +0100, Peter Antoine wrote:
On some platforms the MOCS values are not always saved and restored
on
RC6 enter/exit. The rational is that the context with restore these
values. On these platforms the test will fail as it tests the values
by directly reading the MOCS registers.
But there's nothing wrong with the existing tests per-se? You just
want to add a new one that explicitly tests rc6 save/restore, and in
doing so just need to limit the forcewake. For that you just want to
limit intel_register_access to the critical sections. (You don't need
to find the pci_dev afresh everytime.)
On some platforms (BXT) it does not restore L3CC registers. So that on these
platforms the direct read will fail unless you hold forcewake for the whole
test. It also implies that the registers are valid for the whole power
lifecycle, so if you are using the tests as API documentation then it implies
that the registers are always valid. This is not always true on all platforms.
Right. If those registers are only valid when read from within an active
context, please send the patch to remove the invalid tests first. That means
all the direct register access is undefined, right?
Ok, will do that.
Peter.
So the direct memory reads are removed to give the correct API usage for the
MOCS. That is that the l3CC registers are only valid while in the RCS context.
Ok.
+static void context_rc6_test(void)
+{
+ int fd = drm_open_driver(DRIVER_INTEL);
+ int res_ms;
+ uint32_t ctx_id = gem_context_create(fd);
+
+ igt_debug("RC6 Context Test\n");
+ check_control_registers(fd, I915_EXEC_RENDER, ctx_id, false);
+ check_l3cc_registers(fd, I915_EXEC_RENDER, ctx_id, false);
+
+ res_ms = read_rc6_residency();
+ sleep(3);
And you could spin here until rc6 residency increased.
timeout = 3000 / 2;
while (read_rc6_residency() == initial_res && --timeout)
usleep(2000);
Ok. Decided against that as 3 seconds (I know magic value) should have been
enough. I can change to do the spin with a timeout.
Peter, your email client's quoting is broken in the thread above; it
makes it quite difficult to determine who said what. Different authors
should have different numbers of >> before their contributions, or some
other way of identifying who said what.
.Dave.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [I-G-T] igt/gem_mocs_settings: improve RC6 testings
-Original Message-
From: Chris Wilson [mailto:ch...@chris-wilson.co.uk]
Sent: Friday, July 22, 2016 10:38 AM
To: Antoine, Peter
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [I-G-T] igt/gem_mocs_settings: improve RC6 testings
On Thu, Jul 21, 2016 at 09:49:51PM +, Antoine, Peter wrote:
>
>
> -Original Message-
> From: Chris Wilson [mailto:ch...@chris-wilson.co.uk]
> Sent: Thursday, July 21, 2016 9:40 PM
> To: Antoine, Peter
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [I-G-T] igt/gem_mocs_settings: improve RC6 testings
>
> On Tue, Jul 19, 2016 at 11:25:29AM +0100, Peter Antoine wrote:
> > On some platforms the MOCS values are not always saved and restored
> > on
> > RC6 enter/exit. The rational is that the context with restore these
> > values. On these platforms the test will fail as it tests the values
> > by directly reading the MOCS registers.
>
> But there's nothing wrong with the existing tests per-se? You just
> want to add a new one that explicitly tests rc6 save/restore, and in
> doing so just need to limit the forcewake. For that you just want to
> limit intel_register_access to the critical sections. (You don't need
> to find the pci_dev afresh everytime.)
>
> On some platforms (BXT) it does not restore L3CC registers. So that on these
> platforms the direct read will fail unless you hold forcewake for the whole
> test. It also implies that the registers are valid for the whole power
> lifecycle, so if you are using the tests as API documentation then it implies
> that the registers are always valid. This is not always true on all platforms.
Right. If those registers are only valid when read from within an active
context, please send the patch to remove the invalid tests first. That means
all the direct register access is undefined, right?
Ok, will do that.
Peter.
> So the direct memory reads are removed to give the correct API usage for the
> MOCS. That is that the l3CC registers are only valid while in the RCS context.
Ok.
> > +static void context_rc6_test(void)
> > +{
> > + int fd = drm_open_driver(DRIVER_INTEL);
> > + int res_ms;
> > + uint32_t ctx_id = gem_context_create(fd);
> > +
> > + igt_debug("RC6 Context Test\n");
> > + check_control_registers(fd, I915_EXEC_RENDER, ctx_id, false);
> > + check_l3cc_registers(fd, I915_EXEC_RENDER, ctx_id, false);
> > +
> > + res_ms = read_rc6_residency();
> > + sleep(3);
>
> And you could spin here until rc6 residency increased.
>
> timeout = 3000 / 2;
> while (read_rc6_residency() == initial_res && --timeout)
> usleep(2000);
>
> Ok. Decided against that as 3 seconds (I know magic value) should have been
> enough. I can change to do the spin with a timeout.
It was more about not wasting 3 seconds, if we can get to rc6 within a 100ms.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [I-G-T] igt/gem_mocs_settings: improve RC6 testings
On Thu, Jul 21, 2016 at 09:49:51PM +, Antoine, Peter wrote:
>
>
> -Original Message-
> From: Chris Wilson [mailto:ch...@chris-wilson.co.uk]
> Sent: Thursday, July 21, 2016 9:40 PM
> To: Antoine, Peter
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [I-G-T] igt/gem_mocs_settings: improve RC6 testings
>
> On Tue, Jul 19, 2016 at 11:25:29AM +0100, Peter Antoine wrote:
> > On some platforms the MOCS values are not always saved and restored on
> > RC6 enter/exit. The rational is that the context with restore these
> > values. On these platforms the test will fail as it tests the values
> > by directly reading the MOCS registers.
>
> But there's nothing wrong with the existing tests per-se? You just want to
> add a new one that explicitly tests rc6 save/restore, and in doing so just
> need to limit the forcewake. For that you just want to limit
> intel_register_access to the critical sections. (You don't need to find the
> pci_dev afresh everytime.)
>
> On some platforms (BXT) it does not restore L3CC registers. So that on these
> platforms the direct read will fail unless you hold forcewake for the whole
> test. It also implies that the registers are valid for the whole power
> lifecycle, so if you are using the tests as API documentation then it implies
> that the registers are always valid. This is not always true on all platforms.
Right. If those registers are only valid when read from within an active
context, please send the patch to remove the invalid tests first. That
means all the direct register access is undefined, right?
> So the direct memory reads are removed to give the correct API usage for the
> MOCS. That is that the l3CC registers are only valid while in the RCS context.
Ok.
> > +static void context_rc6_test(void)
> > +{
> > + int fd = drm_open_driver(DRIVER_INTEL);
> > + int res_ms;
> > + uint32_t ctx_id = gem_context_create(fd);
> > +
> > + igt_debug("RC6 Context Test\n");
> > + check_control_registers(fd, I915_EXEC_RENDER, ctx_id, false);
> > + check_l3cc_registers(fd, I915_EXEC_RENDER, ctx_id, false);
> > +
> > + res_ms = read_rc6_residency();
> > + sleep(3);
>
> And you could spin here until rc6 residency increased.
>
> timeout = 3000 / 2;
> while (read_rc6_residency() == initial_res && --timeout)
> usleep(2000);
>
> Ok. Decided against that as 3 seconds (I know magic value) should have been
> enough. I can change to do the spin with a timeout.
It was more about not wasting 3 seconds, if we can get to rc6 within a
100ms.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [I-G-T] igt/gem_mocs_settings: improve RC6 testings
-Original Message-
From: Chris Wilson [mailto:ch...@chris-wilson.co.uk]
Sent: Thursday, July 21, 2016 9:40 PM
To: Antoine, Peter
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [I-G-T] igt/gem_mocs_settings: improve RC6 testings
On Tue, Jul 19, 2016 at 11:25:29AM +0100, Peter Antoine wrote:
> On some platforms the MOCS values are not always saved and restored on
> RC6 enter/exit. The rational is that the context with restore these
> values. On these platforms the test will fail as it tests the values
> by directly reading the MOCS registers.
But there's nothing wrong with the existing tests per-se? You just want to add
a new one that explicitly tests rc6 save/restore, and in doing so just need to
limit the forcewake. For that you just want to limit intel_register_access to
the critical sections. (You don't need to find the pci_dev afresh everytime.)
On some platforms (BXT) it does not restore L3CC registers. So that on these
platforms the direct read will fail unless you hold forcewake for the whole
test. It also implies that the registers are valid for the whole power
lifecycle, so if you are using the tests as API documentation then it implies
that the registers are always valid. This is not always true on all platforms.
So the direct memory reads are removed to give the correct API usage for the
MOCS. That is that the l3CC registers are only valid while in the RCS context.
> +static unsigned int readit(const char *path) {
> + unsigned int ret = 0;
> + int scanned = 0;
> + FILE *file;
> +
> + file = fopen(path, "r");
> + igt_assert(file);
> + scanned = fscanf(file, "%u", &ret);
> + igt_assert_eq(scanned, 1);
> +
> + fclose(file);
> +
> + return ret;
> +}
> +
> +static int read_rc6_residency(void)
> +{
> + unsigned int residency;
> + const int device = drm_get_card();
> + static const char path_format[] =
> + "/sys/class/drm/card%d/power/rc6_residency_ms";
> + char path[sizeof(path_format)];
> + int ret;
> +
> + ret = snprintf(path, sizeof(path)-1, path_format, device);
> +
> + igt_assert_neq(ret, -1);
> + residency = readit(path);
This should be using a sysfs helper. igt_sysfs.c should already be sufficient,
or if not, please improve.
Ok, will look into this.
> + return residency;
> +}
> +
> +static void context_rc6_test(void)
> +{
> + int fd = drm_open_driver(DRIVER_INTEL);
> + int res_ms;
> + uint32_t ctx_id = gem_context_create(fd);
> +
> + igt_debug("RC6 Context Test\n");
> + check_control_registers(fd, I915_EXEC_RENDER, ctx_id, false);
> + check_l3cc_registers(fd, I915_EXEC_RENDER, ctx_id, false);
> +
> + res_ms = read_rc6_residency();
> + sleep(3);
And you could spin here until rc6 residency increased.
timeout = 3000 / 2;
while (read_rc6_residency() == initial_res && --timeout)
usleep(2000);
Ok. Decided against that as 3 seconds (I know magic value) should have been
enough. I can change to do the spin with a timeout.
and importantly: igt_require(read_rc6_residency() != initial_res); Don't assert
for what may be a user configuration.
Ok, that makes a lot of sense. Will check that RC6 is on in kernel and BIOS
settings.
Peter.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [I-G-T] igt/gem_mocs_settings: improve RC6 testings
On Tue, Jul 19, 2016 at 11:25:29AM +0100, Peter Antoine wrote:
> On some platforms the MOCS values are not always saved and restored
> on RC6 enter/exit. The rational is that the context with restore
> these values. On these platforms the test will fail as it tests the
> values by directly reading the MOCS registers.
But there's nothing wrong with the existing tests per-se? You just want
to add a new one that explicitly tests rc6 save/restore, and in doing so
just need to limit the forcewake. For that you just want to limit
intel_register_access to the critical sections. (You don't need to find
the pci_dev afresh everytime.)
> +static unsigned int readit(const char *path)
> +{
> + unsigned int ret = 0;
> + int scanned = 0;
> + FILE *file;
> +
> + file = fopen(path, "r");
> + igt_assert(file);
> + scanned = fscanf(file, "%u", &ret);
> + igt_assert_eq(scanned, 1);
> +
> + fclose(file);
> +
> + return ret;
> +}
> +
> +static int read_rc6_residency(void)
> +{
> + unsigned int residency;
> + const int device = drm_get_card();
> + static const char path_format[] =
> + "/sys/class/drm/card%d/power/rc6_residency_ms";
> + char path[sizeof(path_format)];
> + int ret;
> +
> + ret = snprintf(path, sizeof(path)-1, path_format, device);
> +
> + igt_assert_neq(ret, -1);
> + residency = readit(path);
This should be using a sysfs helper. igt_sysfs.c should already be
sufficient, or if not, please improve.
> + return residency;
> +}
> +
> +static void context_rc6_test(void)
> +{
> + int fd = drm_open_driver(DRIVER_INTEL);
> + int res_ms;
> + uint32_t ctx_id = gem_context_create(fd);
> +
> + igt_debug("RC6 Context Test\n");
> + check_control_registers(fd, I915_EXEC_RENDER, ctx_id, false);
> + check_l3cc_registers(fd, I915_EXEC_RENDER, ctx_id, false);
> +
> + res_ms = read_rc6_residency();
> + sleep(3);
And you could spin here until rc6 residency increased.
timeout = 3000 / 2;
while (read_rc6_residency() == initial_res && --timeout)
usleep(2000);
and importantly: igt_require(read_rc6_residency() != initial_res);
Don't assert for what may be a user configuration.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
