| Message ID | 1468923929-22731-1-git-send-email-peter.antoine@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
-----Original Message----- From: Chris Wilson [mailto:chris@chris-wilson.co.uk] Sent: Thursday, July 21, 2016 9:40 PM To: Antoine, Peter <peter.antoine@intel.com> 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
On Thu, Jul 21, 2016 at 09:49:51PM +0000, Antoine, Peter wrote: > > > -----Original Message----- > From: Chris Wilson [mailto:chris@chris-wilson.co.uk] > Sent: Thursday, July 21, 2016 9:40 PM > To: Antoine, Peter <peter.antoine@intel.com> > 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
-----Original Message----- From: Chris Wilson [mailto:chris@chris-wilson.co.uk] Sent: Friday, July 22, 2016 10:38 AM To: Antoine, Peter <peter.antoine@intel.com> 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 +0000, Antoine, Peter wrote: > > > -----Original Message----- > From: Chris Wilson [mailto:chris@chris-wilson.co.uk] > Sent: Thursday, July 21, 2016 9:40 PM > To: Antoine, Peter <peter.antoine@intel.com> > 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
On 22/07/16 10:40, Antoine, Peter wrote: > > -----Original Message----- > From: Chris Wilson [mailto:chris@chris-wilson.co.uk] > Sent: Friday, July 22, 2016 10:38 AM > To: Antoine, Peter <peter.antoine@intel.com> > 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 +0000, Antoine, Peter wrote: >> >> -----Original Message----- >> From: Chris Wilson [mailto:chris@chris-wilson.co.uk] >> Sent: Thursday, July 21, 2016 9:40 PM >> To: Antoine, Peter <peter.antoine@intel.com> >> 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.
diff --git a/tests/gem_mocs_settings.c b/tests/gem_mocs_settings.c index bc553e9..e85dcda 100644 --- a/tests/gem_mocs_settings.c +++ b/tests/gem_mocs_settings.c @@ -132,65 +132,6 @@ static uint32_t get_engine_base(uint32_t engine) } } -static uint32_t get_mocs_register_value(int fd, uint64_t offset, uint32_t index) -{ - igt_assert(index < MAX_NUMBER_MOCS_REGISTERS); - return intel_register_read(offset + index * 4); -} - -static void test_mocs_control_values(int fd, uint32_t engine) -{ - const uint32_t engine_base = get_engine_base(engine); - struct mocs_table table; - int local_fd; - int i; - - local_fd = fd; - if (local_fd == -1) - local_fd = drm_open_driver_master(DRIVER_INTEL); - - igt_assert(get_mocs_settings(local_fd, &table, false)); - - for (i = 0; i < table.size; i++) - igt_assert_eq_u32(get_mocs_register_value(local_fd, - engine_base, i), - table.table[i].control_value); - - if (local_fd != fd) - close(local_fd); -} - -static void test_mocs_l3cc_values(int fd) -{ - uint32_t reg_values[MAX_NUMBER_MOCS_REGISTERS/2]; - struct mocs_table table; - int local_fd; - int i; - - local_fd = fd; - if (local_fd == -1) - local_fd = drm_open_driver_master(DRIVER_INTEL); - - for (i = 0; i < MAX_NUMBER_MOCS_REGISTERS / 2; i++) - reg_values[i] = intel_register_read(GEN9_LNCFCMOCS0 + (i * 4)); - - igt_assert(get_mocs_settings(local_fd, &table, false)); - - for (i = 0; i < table.size / 2; i++) { - igt_assert_eq_u32((reg_values[i] & 0xffff), - table.table[i * 2].l3cc_value); - igt_assert_eq_u32((reg_values[i] >> 16), - table.table[i * 2 + 1].l3cc_value); - } - - if (table.size & 1) - igt_assert_eq_u32((reg_values[i] & 0xffff), - table.table[i * 2].l3cc_value); - - if (local_fd != fd) - close(local_fd); -} - #define MI_STORE_REGISTER_MEM_64_BIT_ADDR ((0x24 << 23) | 2) static int create_read_batch(struct drm_i915_gem_relocation_entry *reloc, @@ -428,11 +369,8 @@ static void test_mocs_values(int fd) continue; igt_debug("Testing %s\n", e->name); - test_mocs_control_values(fd, engine); test_context_mocs_values(fd, engine); } - - test_mocs_l3cc_values(fd); } static void default_context_tests(unsigned mode) @@ -566,12 +504,73 @@ static void context_dirty_test(unsigned mode) static void run_tests(unsigned mode) { + struct pci_device *pci_dev; + + pci_dev = intel_get_pci_device(); + igt_require(pci_dev); + intel_register_access_init(pci_dev, 0); + default_context_tests(mode); default_dirty_tests(mode); context_save_restore_test(mode); context_dirty_test(mode); + + intel_register_access_fini(); +} + +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); + + 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); + igt_assert_neq(res_ms, read_rc6_residency()); + + check_control_registers(fd, I915_EXEC_RENDER, ctx_id, false); + check_l3cc_registers(fd, I915_EXEC_RENDER, ctx_id, false); + close(fd); } + static void test_requirements(void) { int fd = drm_open_driver_master(DRIVER_INTEL); @@ -584,19 +583,16 @@ static void test_requirements(void) igt_main { - struct pci_device *pci_dev; - igt_fixture { test_requirements(); - - pci_dev = intel_get_pci_device(); - igt_require(pci_dev); - intel_register_access_init(pci_dev, 0); } igt_subtest("mocs-settings") run_tests(NONE); + igt_subtest("mocs-rc6") + context_rc6_test(); + igt_subtest("mocs-reset") run_tests(RESET); @@ -605,8 +601,4 @@ igt_main igt_subtest("mocs-hibernate") run_tests(HIBERNATE); - - igt_fixture { - intel_register_access_fini(); - } }
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. So this change removes the direct testing of the values and adds a test that specifically waits for the GPU to enter and then leave RC6 and then tests the MOCS values. Signed-off-by: Peter Antoine <peter.antoine@intel.com> --- tests/gem_mocs_settings.c | 136 ++++++++++++++++++++++------------------------ 1 file changed, 64 insertions(+), 72 deletions(-)