On Mon, Aug 05, 2013 at 10:42:08AM -0300, Paulo Zanoni wrote:
> 2013/8/5 Daniel Vetter <daniel@xxxxxxxx>:
> > On Mon, Jul 29, 2013 at 05:53:27PM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> >>
> >> This test chekcs our code that enables Package C8+. The environment
> >> requirements for this test are quite complicated:
> >> - The machine needs to be properly configured to reach PC8+ when
> >> possible, which means all the power management policies and device
> >> drivers must be properly configured.
> >> - Before running the test, the machine needs to have 0 PC8+
> >> residency. If you run the test after the machine already has PC8+
> >> residency, it means you'll be comparing post-PC8+ with post-PC8+,
> >> so you won't be able to get things lost after the first time we
> >> enter PC8+.
> >> - This means that after you run the test once, you have to
> >> reboot, unless you use the "--skip-zero-pc8-check" option, but
> >> you really need to know what you're doing.
> >
> > This needs to be made more robust since I want to include this test into
> > the normal -nightly testruns.
>
> Notice I added the test to noinst_PROGRAMS, so QA won't be running it
> for now, even if we merge it. Why? Because of these restrictions
> mentioned above.
Yep I've spotted that, but that needs to be fixed ;-)
>
> > Does comparing the difference of the
> > registers not work instead of checking for 0?
>
> Can you please elaborate more on this sentence? I'm not sure what
> you're suggesting here.
Atm you check that the pc8 residency counters are 0, so we need to reboot
after having run the test once. For other similar stuff (e.g. the rc6
residency test) we take a snapshot before/after and check the differences
only. That way the test should work always.
> The problem is: let's imagine there's some important register that we
> initialize when starting the driver, but then we don't touch it
> anymore. And this register is one of the registers that get reset when
> we enter PC8, but our code doesn't fix it after leaving PC8. So if you
> boot your machine, do something to allow PC8+ and then disallow it,
> the register will go back to the "reset" state (which isn't guaranteed
> to be 0x0, especially on display registers). Then you run tests/pc8:
> it reads the register (which contains the "reset" value instead of the
> real value set by our driver when it got loaded, because we already
> entered PC8+ once), enters PC8+, leaves PC8+, reads the register
> again, notices the value is the same and then gives us a "PASS". On
> the other hand, if you didn't reach PC8+ before running tests/pc8,
> then it would have noticed the value of the register changes.
>
> In other words: if some register gets initialized by our driver to a
> non-default value, but this value gets lost after we enter/leave PC8+,
> we will *only* be able to notice the difference when comparing a state
> where we *never* entered PC8+ against a state where we "already
> entered PC8+ at least once", because after the first time you
> enter/leave PC8+ you'll already have reset the register, so you'll be
> comparing bugged state against bugged state.
Ok, I see your point. But imo igt testcases shouldn't (at least with the
testcases run by default) have such detailed knowledge of what the kernel
actually does with the registers. So a depency like "this test needs to be
run after a fresh boot when we've never entered pc8+ yet" isn't good since
it'll make the testcase fragile.
Instead the test should check whether everything still works after we've
been in pc8+ for a bit. One exception could be w/a bits if we have a
common igt testcase to check for all of them. Then we could just rerun
that testcase.
But if e.g. the swizzling settings get lost over pc8+ it's better to add
an explicit functional swizzle test here instead of checking registers.
>
> >
> >> - You need at least one output connected.
> >
> > The test should skip (retun 77;) if that's the case.
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> >> ---
> >> tests/.gitignore | 1 +
> >> tests/Makefile.am | 1 +
> >> tests/pc8.c | 555 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 557 insertions(+)
> >> create mode 100644 tests/pc8.c
> >>
> >> diff --git a/tests/.gitignore b/tests/.gitignore
> >> index 451ab2d..c70fdb7 100644
> >> --- a/tests/.gitignore
> >> +++ b/tests/.gitignore
> >> @@ -90,6 +90,7 @@ getstats
> >> getversion
> >> kms_flip
> >> kms_render
> >> +pc8
> >> prime_nv_api
> >> prime_nv_pcopy
> >> prime_nv_test
> >> diff --git a/tests/Makefile.am b/tests/Makefile.am
> >> index a59c25f..3507716 100644
> >> --- a/tests/Makefile.am
> >> +++ b/tests/Makefile.am
> >> @@ -2,6 +2,7 @@ if BUILD_TESTS
> >> noinst_PROGRAMS = \
> >> gem_stress \
> >> ddi_compute_wrpll \
> >> + pc8 \
> >> $(TESTS_progs) \
> >> $(TESTS_progs_M) \
> >> $(HANG) \
> >> diff --git a/tests/pc8.c b/tests/pc8.c
> >> new file mode 100644
> >> index 0000000..7fdfeb5
> >> --- /dev/null
> >> +++ b/tests/pc8.c
> >> @@ -0,0 +1,555 @@
> >> +/*
> >> + * Copyright © 2013 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.
> >> + *
> >> + * Authors:
> >> + * Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> >> + *
> >> + */
> >> +
> >> +#include <stdio.h>
> >> +#include <assert.h>
> >> +#include <stdint.h>
> >> +#include <stdbool.h>
> >> +#include <string.h>
> >> +
> >> +#include <unistd.h>
> >> +#include <sys/types.h>
> >> +#include <sys/stat.h>
> >> +#include <fcntl.h>
> >> +
> >> +#include "drm.h"
> >> +#include "drmtest.h"
> >> +#include "intel_batchbuffer.h"
> >> +#include "intel_gpu_tools.h"
> >> +
> >> +#define MSR_PC8_RES 0x630
> >> +#define MSR_PC9_RES 0x631
> >> +#define MSR_PC10_RES 0x632
> >> +
> >> +#define MAX_CONNECTORS 32
> >> +#define MAX_ENCODERS 32
> >> +#define MAX_CRTCS 16
> >> +
> >> +int drm_fd;
> >> +
> >> +/* Stuff used when creating FBs and mode setting. */
> >> +struct mode_set_data {
> >> + drmModeResPtr res;
> >> + drmModeConnectorPtr connectors[MAX_CONNECTORS];
> >> +
> >> + drm_intel_bufmgr *bufmgr;
> >> + uint32_t devid;
> >> +};
> >> +
> >> +enum compare_step {
> >> + STEP_RESET,
> >> + STEP_RESTORED,
> >> +};
> >> +
> >> +/* Stuff we query at different times so we can compare. */
> >> +struct compare_data {
> >> + drmModeResPtr res;
> >> + drmModeEncoderPtr encoders[MAX_ENCODERS];
> >> + drmModeConnectorPtr connectors[MAX_CONNECTORS];
> >> + drmModeCrtcPtr crtcs[MAX_CRTCS];
> >> + drmModePropertyBlobPtr edids[MAX_CONNECTORS];
> >> +
> >> + struct {
> >> + /* We know these are lost */
> >> + uint32_t arb_mode;
> >> + uint32_t tilectl;
> >> +
> >> + /* Stuff touched at init_clock_gating, so we can make sure we
> >> + * don't need to call it when reiniting. */
> >> + uint32_t gen6_ucgctl2;
> >> + uint32_t gen7_l3cntlreg1;
> >> + uint32_t transa_chicken1;
> >> +
> >> + uint32_t deier;
> >> + uint32_t gtier;
> >> +
> >> + uint32_t ddi_buf_trans_a_1;
> >> + uint32_t ddi_buf_trans_b_5;
> >> + uint32_t ddi_buf_trans_c_10;
> >> + uint32_t ddi_buf_trans_d_15;
> >> + uint32_t ddi_buf_trans_e_20;
> >> + } regs;
> >> +};
> >> +
> >> +static uint64_t get_residency(int fd, uint32_t type)
> >> +{
> >> + int rc;
> >> + uint64_t ret;
> >> +
> >> + rc = pread(fd, &ret, sizeof(uint64_t), type);
> >> + assert(rc == sizeof(uint64_t));
> >> +
> >> + return ret;
> >> +}
> >> +
> >> +static bool pc8_plus_residency_is_zero(int fd)
> >> +{
> >> + uint64_t res_pc8, res_pc9, res_pc10;
> >> +
> >> + res_pc8 = get_residency(fd, MSR_PC8_RES);
> >> + res_pc9 = get_residency(fd, MSR_PC9_RES);
> >> + res_pc10 = get_residency(fd, MSR_PC10_RES);
> >> +
> >> + if (res_pc8 != 0 || res_pc9 != 0 || res_pc10 != 0)
> >> + return false;
> >> +
> >> + return true;
> >> +}
> >> +
> >> +static bool pc8_plus_residency_changed(int fd, unsigned int timeout_sec)
> >> +{
> >> + unsigned int i;
> >> + uint64_t res_pc8, res_pc9, res_pc10;
> >> + int to_sleep = 100 * 1000;
> >> +
> >> + res_pc8 = get_residency(fd, MSR_PC8_RES);
> >> + res_pc9 = get_residency(fd, MSR_PC9_RES);
> >> + res_pc10 = get_residency(fd, MSR_PC10_RES);
> >> +
> >> + for (i = 0; i < timeout_sec * 1000 * 1000; i += to_sleep) {
> >> + if (res_pc8 != get_residency(fd, MSR_PC8_RES) ||
> >> + res_pc9 != get_residency(fd, MSR_PC9_RES) ||
> >> + res_pc10 != get_residency(fd, MSR_PC10_RES)) {
> >> + return true;
> >> + }
> >> + usleep(to_sleep);
> >> + }
> >> +
> >> + return false;
> >> +}
> >> +
> >> +static void disable_all_screens(struct mode_set_data *data)
> >> +{
> >> + int i, rc;
> >> +
> >> + for (i = 0; i < data->res->count_crtcs; i++) {
> >> + rc = drmModeSetCrtc(drm_fd, data->res->crtcs[i], -1, 0, 0,
> >> + NULL, 0, NULL);
> >> + assert(rc == 0);
> >> + }
> >> +}
> >> +
> >> +static uint32_t create_fb(struct mode_set_data *data, int width, int height)
> >> +{
> >> + struct kmstest_fb fb;
> >> + cairo_t *cr;
> >> + uint32_t buffer_id;
> >> +
> >> + buffer_id = kmstest_create_fb(drm_fd, width, height, 32, 24, false,
> >> + &fb);
> >> + cr = kmstest_get_cairo_ctx(drm_fd, &fb);
> >> + kmstest_paint_test_pattern(cr, width, height);
> >> + return buffer_id;
> >> +}
> >> +
> >> +static void enable_one_screen(struct mode_set_data *data)
> >> +{
> >> + uint32_t crtc_id = 0, buffer_id = 0, connector_id = 0;
> >> + drmModeModeInfoPtr mode = NULL;
> >> + int i, rc;
> >> +
> >> + for (i = 0; i < data->res->count_connectors; i++) {
> >> + drmModeConnectorPtr c = data->connectors[i];
> >> +
> >> + if (c->connection == DRM_MODE_CONNECTED && c->count_modes) {
> >> + connector_id = c->connector_id;
> >> + mode = &c->modes[0];
> >> + break;
> >> + }
> >> + }
> >> +
> >> + crtc_id = data->res->crtcs[0];
> >> + buffer_id = create_fb(data, mode->hdisplay, mode->vdisplay);
> >> +
> >> + assert(crtc_id);
> >> + assert(buffer_id);
> >> + assert(connector_id);
> >> + assert(mode);
> >> +
> >> + rc = drmModeSetCrtc(drm_fd, crtc_id, buffer_id, 0, 0, &connector_id,
> >> + 1, mode);
> >> + assert(rc == 0);
> >> +}
> >> +
> >> +static void get_connector_edid(struct compare_data *data, int index)
> >> +{
> >> + unsigned int i;
> >> + drmModeConnectorPtr connector = data->connectors[index];
> >> + drmModeObjectPropertiesPtr props;
> >> +
> >> + props = drmModeObjectGetProperties(drm_fd, connector->connector_id,
> >> + DRM_MODE_OBJECT_CONNECTOR);
> >> +
> >> + for (i = 0; i < props->count_props; i++) {
> >> + drmModePropertyPtr prop = drmModeGetProperty(drm_fd,
> >> + props->props[i]);
> >> +
> >> + if (strcmp(prop->name, "EDID") == 0) {
> >> + assert(prop->flags & DRM_MODE_PROP_BLOB);
> >> + assert(prop->count_blobs == 0);
> >> + data->edids[index] = drmModeGetPropertyBlob(drm_fd,
> >> + props->prop_values[i]);
> >> + }
> >> +
> >> + drmModeFreeProperty(prop);
> >> + }
> >> +
> >> + drmModeFreeObjectProperties(props);
> >> +}
> >> +
> >> +static void init_mode_set_data(struct mode_set_data *data)
> >> +{
> >> + int i;
> >> +
> >> + data->res = drmModeGetResources(drm_fd);
> >> + assert(data->res);
> >> + assert(data->res->count_connectors <= MAX_CONNECTORS);
> >> +
> >> + for (i = 0; i < data->res->count_connectors; i++)
> >> + data->connectors[i] = drmModeGetConnector(drm_fd,
> >> + data->res->connectors[i]);
> >> +
> >> + data->bufmgr = drm_intel_bufmgr_gem_init(drm_fd, 4096);
> >> + data->devid = intel_get_drm_devid(drm_fd);
> >> +
> >> + do_or_die(drmtest_set_vt_graphics_mode());
> >> + drm_intel_bufmgr_gem_enable_reuse(data->bufmgr);
> >> +}
> >> +
> >> +static void fini_mode_set_data(struct mode_set_data *data)
> >> +{
> >> + int i;
> >> +
> >> + drm_intel_bufmgr_destroy(data->bufmgr);
> >> +
> >> + for (i = 0; i < data->res->count_connectors; i++)
> >> + drmModeFreeConnector(data->connectors[i]);
> >> + drmModeFreeResources(data->res);
> >> +}
> >> +
> >> +static void get_drm_info(struct compare_data *data)
> >> +{
> >> + int i;
> >> +
> >> + data->res = drmModeGetResources(drm_fd);
> >> + assert(data->res);
> >> +
> >> + assert(data->res->count_connectors <= MAX_CONNECTORS);
> >> + assert(data->res->count_encoders <= MAX_ENCODERS);
> >> + assert(data->res->count_crtcs <= MAX_CRTCS);
> >> +
> >> + for (i = 0; i < data->res->count_connectors; i++) {
> >> + data->connectors[i] = drmModeGetConnector(drm_fd,
> >> + data->res->connectors[i]);
> >> + get_connector_edid(data, i);
> >> + }
> >> + for (i = 0; i < data->res->count_encoders; i++)
> >> + data->encoders[i] = drmModeGetEncoder(drm_fd,
> >> + data->res->encoders[i]);
> >> + for (i = 0; i < data->res->count_crtcs; i++)
> >> + data->crtcs[i] = drmModeGetCrtc(drm_fd, data->res->crtcs[i]);
> >> +
> >> + intel_register_access_init(intel_get_pci_device(), 0);
> >> + data->regs.arb_mode = INREG(0x4030);
> >> + data->regs.tilectl = INREG(0x101000);
> >> + data->regs.gen6_ucgctl2 = INREG(0x9404);
> >> + data->regs.gen7_l3cntlreg1 = INREG(0xB0C1);
> >> + data->regs.transa_chicken1 = INREG(0xF0060);
> >> + data->regs.deier = INREG(0x4400C);
> >> + data->regs.gtier = INREG(0x4401C);
> >> + data->regs.ddi_buf_trans_a_1 = INREG(0x64E00);
> >> + data->regs.ddi_buf_trans_b_5 = INREG(0x64E70);
> >> + data->regs.ddi_buf_trans_c_10 = INREG(0x64EE0);
> >> + data->regs.ddi_buf_trans_d_15 = INREG(0x64F58);
> >> + data->regs.ddi_buf_trans_e_20 = INREG(0x64FCC);
> >> + intel_register_access_fini();
> >> +}
> >> +
> >> +static void free_drm_info(struct compare_data *data)
> >> +{
> >> + int i;
> >> +
> >> + for (i = 0; i < data->res->count_connectors; i++) {
> >> + drmModeFreeConnector(data->connectors[i]);
> >> + drmModeFreePropertyBlob(data->edids[i]);
> >> + }
> >> + for (i = 0; i < data->res->count_encoders; i++)
> >> + drmModeFreeEncoder(data->encoders[i]);
> >> + for (i = 0; i < data->res->count_crtcs; i++)
> >> + drmModeFreeCrtc(data->crtcs[i]);
> >> +
> >> + drmModeFreeResources(data->res);
> >> +}
> >> +
> >> +#define COMPARE(d1, d2, data) assert(d1->data == d2->data)
> >> +#define COMPARE_ARRAY(d1, d2, size, data) do { \
> >> + for (i = 0; i < size; i++) \
> >> + assert(d1->data[i] == d2->data[i]); \
> >> +} while (0)
> >> +
> >> +static void assert_drm_resources_equal(struct compare_data *d1,
> >> + struct compare_data *d2)
> >> +{
> >> + COMPARE(d1, d2, res->count_connectors);
> >> + COMPARE(d1, d2, res->count_encoders);
> >> + COMPARE(d1, d2, res->count_crtcs);
> >> + COMPARE(d1, d2, res->min_width);
> >> + COMPARE(d1, d2, res->max_width);
> >> + COMPARE(d1, d2, res->min_height);
> >> + COMPARE(d1, d2, res->max_height);
> >> +}
> >> +
> >> +static void assert_modes_equal(drmModeModeInfoPtr m1, drmModeModeInfoPtr m2)
> >> +{
> >> + COMPARE(m1, m2, clock);
> >> + COMPARE(m1, m2, hdisplay);
> >> + COMPARE(m1, m2, hsync_start);
> >> + COMPARE(m1, m2, hsync_end);
> >> + COMPARE(m1, m2, htotal);
> >> + COMPARE(m1, m2, hskew);
> >> + COMPARE(m1, m2, vdisplay);
> >> + COMPARE(m1, m2, vsync_start);
> >> + COMPARE(m1, m2, vsync_end);
> >> + COMPARE(m1, m2, vtotal);
> >> + COMPARE(m1, m2, vscan);
> >> + COMPARE(m1, m2, vrefresh);
> >> + COMPARE(m1, m2, flags);
> >> + COMPARE(m1, m2, type);
> >> + assert(strcmp(m1->name, m2->name) == 0);
> >> +}
> >> +
> >> +static void assert_drm_connectors_equal(drmModeConnectorPtr c1,
> >> + drmModeConnectorPtr c2)
> >> +{
> >> + int i;
> >> +
> >> + COMPARE(c1, c2, connector_id);
> >> + COMPARE(c1, c2, connector_type);
> >> + COMPARE(c1, c2, connector_type_id);
> >> + COMPARE(c1, c2, mmWidth);
> >> + COMPARE(c1, c2, mmHeight);
> >> + COMPARE(c1, c2, count_modes);
> >> + COMPARE(c1, c2, count_props);
> >> + COMPARE(c1, c2, count_encoders);
> >> + COMPARE_ARRAY(c1, c2, c1->count_props, props);
> >> + COMPARE_ARRAY(c1, c2, c1->count_encoders, encoders);
> >> +
> >> + for (i = 0; i < c1->count_modes; i++)
> >> + assert_modes_equal(&c1->modes[0], &c2->modes[0]);
> >> +}
> >> +
> >> +static void assert_drm_encoders_equal(drmModeEncoderPtr e1,
> >> + drmModeEncoderPtr e2)
> >> +{
> >> + COMPARE(e1, e2, encoder_id);
> >> + COMPARE(e1, e2, encoder_type);
> >> + COMPARE(e1, e2, possible_crtcs);
> >> + COMPARE(e1, e2, possible_clones);
> >> +}
> >> +
> >> +static void assert_drm_crtcs_equal(drmModeCrtcPtr c1, drmModeCrtcPtr c2)
> >> +{
> >> + COMPARE(c1, c2, crtc_id);
> >> +}
> >> +
> >> +static void assert_drm_edids_equal(drmModePropertyBlobPtr e1,
> >> + drmModePropertyBlobPtr e2)
> >> +{
> >> + if (!e1 && !e2)
> >> + return;
> >> + assert(e1 && e2);
> >> +
> >> + COMPARE(e1, e2, id);
> >> + COMPARE(e1, e2, length);
> >> +
> >> + assert(memcmp(e1->data, e2->data, e1->length) == 0);
> >> +}
> >
> > Imo the important part is just checking whether the EDID is still the
> > same, but I guess we can keep all the other checks, too.
> >
> >> +
> >> +static void compare_registers(struct compare_data *d1, struct compare_data *d2,
> >> + enum compare_step step)
> >> +{
> >> + /* Things that shouldn't change at all. */
> >> + COMPARE(d1, d2, regs.gen6_ucgctl2);
> >> + COMPARE(d1, d2, regs.gen7_l3cntlreg1);
> >> + COMPARE(d1, d2, regs.transa_chicken1);
> >> + COMPARE(d1, d2, regs.arb_mode);
> >> + COMPARE(d1, d2, regs.tilectl);
> >> + assert(d1->regs.deier & (1 << 31));
> >> + assert(d2->regs.deier & (1 << 31));
> >> +
> >> + switch (step) {
> >> + case STEP_RESET:
> >> + assert(d2->regs.gtier == 0);
> >> + assert(d2->regs.ddi_buf_trans_a_1 == 0);
> >> + assert(d2->regs.ddi_buf_trans_b_5 == 0);
> >> + assert(d2->regs.ddi_buf_trans_c_10 == 0);
> >> + assert(d2->regs.ddi_buf_trans_d_15 == 0);
> >> + assert(d2->regs.ddi_buf_trans_e_20 == 0);
> >> + break;
> >> + case STEP_RESTORED:
> >> + COMPARE(d1, d2, regs.arb_mode);
> >> + COMPARE(d1, d2, regs.tilectl);
> >> + COMPARE(d1, d2, regs.gtier);
> >> + COMPARE(d1, d2, regs.ddi_buf_trans_a_1);
> >> + COMPARE(d1, d2, regs.ddi_buf_trans_b_5);
> >> + COMPARE(d1, d2, regs.ddi_buf_trans_c_10);
> >> + COMPARE(d1, d2, regs.ddi_buf_trans_d_15);
> >> + COMPARE(d1, d2, regs.ddi_buf_trans_e_20);
> >> + break;
> >> + default:
> >> + assert(0);
> >> + }
> >> +}
> >
> > This is really risky since we've just demonstrated that we can hard-hang
> > hsw if someone concurrently accesses registers. I guess hiding this behind
> > a debug switch would be good.
>
> Shouldn't we add some sort of debugfs interface to read/write
> registers then? We don't want to hang the machine while running
> intel_reg_dumper and all our other tools... These tests are important
> since they catch bugs that actually *happened* while I was developing
> this feature. Most of the other tests are for things people have
> suggested, not bugs that really happened...
On a quick look we have
- arb_mode: This could be caught by a generic "have we lost w/a bits"
test. Similar issues apply for suspend/resume (we have a little helper
for that now) and after gpu resets.
- tilectl: we have tons of swizzling tests. One gaping hole is currently
what happens with the display swizzling, but if we get that wrong people
will notice _really_ fast ;-) And I think we should be able to plug this
hole with crc checksums. But as long as we don't yet have crc checksum
infrastructure I'm willing to live with the gaps.
- gtier: I guess that results in seqno waits not really working. Like I've
mentioned below I think your current test doesn't cover that properly
yet.
- ddi_buf_trans control registers: I guess we could subsume that into the
w/a bit checker simply as another pile of registers which need to have a
special value.
For the register checker Eric started to work on one, but unfornately it's
not yet integrated anywhere into all relevant igt tests.
>
>
> >
> >> +
> >> +static void assert_drm_infos_equal(struct compare_data *d1,
> >> + struct compare_data *d2,
> >> + enum compare_step step)
> >> +{
> >> + int i;
> >> +
> >> + assert_drm_resources_equal(d1, d2);
> >> +
> >> + for (i = 0; i < d1->res->count_connectors; i++) {
> >> + assert_drm_connectors_equal(d1->connectors[i],
> >> + d2->connectors[i]);
> >> + assert_drm_edids_equal(d1->edids[i], d2->edids[i]);
> >> + }
> >> +
> >> + for (i = 0; i < d1->res->count_encoders; i++)
> >> + assert_drm_encoders_equal(d1->encoders[i], d2->encoders[i]);
> >> +
> >> + for (i = 0; i < d1->res->count_crtcs; i++)
> >> + assert_drm_crtcs_equal(d1->crtcs[i], d2->crtcs[i]);
> >> +
> >> + compare_registers(d1, d2, step);
> >> +}
> >> +
> >> +#define BUF_SIZE (8 << 20)
> >> +
> >> +/* to avoid stupid depencies on libdrm, copy&paste */
> >> +struct local_drm_i915_gem_wait {
> >> + /** Handle of BO we shall wait on */
> >> + __u32 bo_handle;
> >> + __u32 flags;
> >> + /** Number of nanoseconds to wait, Returns time remaining. */
> >> + __u64 timeout_ns;
> >> +};
> >> +#define WAIT_IOCTL DRM_IOWR(DRM_COMMAND_BASE + 0x2c, struct local_drm_i915_gem_wait)
> >> +
> >> +static int gem_bo_wait_timeout(int fd, uint32_t handle, uint64_t *timeout_ns)
> >> +{
> >> + struct local_drm_i915_gem_wait wait;
> >> + int ret;
> >> +
> >> + assert(timeout_ns);
> >> +
> >> + wait.bo_handle = handle;
> >> + wait.timeout_ns = *timeout_ns;
> >> + wait.flags = 0;
> >> + ret = drmIoctl(fd, WAIT_IOCTL, &wait);
> >> + *timeout_ns = wait.timeout_ns;
> >> +
> >> + return ret ? -errno : 0;
> >> +}
> >
> > This should be extracted to lib/drmtest.c for all testcases.
> >
> >> +
> >> +static void test_batch(struct mode_set_data *data)
> >> +{
> >> + uint64_t timeout = 1000 * 1000 * 1000 * 2;
> >> + drm_intel_bo *dst;
> >> +
> >> + dst = drm_intel_bo_alloc(data->bufmgr, "dst", BUF_SIZE, 4096);
> >> + if (gem_bo_wait_timeout(drm_fd, dst->handle, &timeout) == -EINVAL)
> >> + printf("Kernel doesn't support wait_timeout\n");
> >> +}
> >
> > Since I don't submit a "submit a batch" anywhere this won't actually test
> > much ...
> >
> >> +
> >> +int main(int argc, char *argv[])
> >> +{
> >> + struct mode_set_data ms_data;
> >> + struct compare_data pre_pc8, during_pc8, post_pc8;
> >> + int msr_fd;
> >> + bool skip_zero_pc8_check = false;
> >> +
> >> + if (argc > 1) {
> >> + /* With this option we can test/debug/develop the tool without
> >> + * needing to reboot every time. */
> >> + if (strcmp(argv[1], "--skip-zero-pc8-check") == 0)
> >> + skip_zero_pc8_check = true;
> >> + }
> >> +
> >> + drm_fd = drm_open_any();
> >> + assert(drm_fd > 0);
> >> +
> >> + init_mode_set_data(&ms_data);
> >> +
> >> + if (!IS_HASWELL(ms_data.devid)) {
> >> + printf("PC8+ feature only supported on Haswell.\n");
> >> + return 77;
> >> + }
> >> +
> >> + msr_fd = open("/dev/cpu/0/msr", O_RDONLY);
> >> + assert(msr_fd != -1);
> >> +
> >> + /* Make sure the machine still didn't enter PC8+, otherwise we won't be
> >> + * able to really compare pre-PC8+ with post-PC8+. */
> >> + printf("Pre-PC8+\n");
> >> + assert(skip_zero_pc8_check || pc8_plus_residency_is_zero(msr_fd));
> >> + get_drm_info(&pre_pc8);
> >> + test_batch(&ms_data);
> >> + assert(skip_zero_pc8_check || pc8_plus_residency_is_zero(msr_fd));
> >> +
> >> + printf("PC8+\n");
> >> + disable_all_screens(&ms_data);
> >> + sleep(1);
> >> +
> >> + assert(pc8_plus_residency_changed(msr_fd, 10));
> >> + get_drm_info(&during_pc8);
> >> + test_batch(&ms_data);
> >> + assert(pc8_plus_residency_changed(msr_fd, 10));
> >
> > Imo we should test each functional piece seperately in it's own loop, i.e.
> >
> > for each foo in test_batch, test_edid_connector1, test_edid_connector2,
> > ... ; do
> > enter pc8+
> > sleep 1
> > run $foo
> > exit pc8+
> > done
>
> This is not possible since the whole goal is to compare the state
> "before we have ever entered PC8+" with "after we have entered PC8+".
> See the big explanation above. But I could try to split the "collect
> data" with the "compare data" steps, so the "compare data" could
> become the individual tests you requested.
I don't think you can make the test robust enough with the current
approach, see above for a few ideas how this could be remedied.
>
>
> >
> > In the current implementation this doesn't matter, but if we change it
> > so that some of the tests will exit pc8+ and if we furthermore add a
> > slight delay for reentering pc8+ then the test won't test a lot any more.
>
> That's why we have functions that sleep for up to 10 seconds waiting
> for the PC8+ residency to move. If we take too much time to go back to
> PC8, then we should increase those 10second timeouts. In fact we could
> make those timeouts become even more than 1 minute since we expect to
> return before that (i.e., when the PC8+ residency moves).
Hm, I guess I've missed that part of the test. But my point was that we
should check that we've reentered pc8+ between each subtest, i.e. every
time we've tried to read and edid, called test_batch.
But as long as we don't wake upthe chip out of pc8+ for these tests I
guess the current test is paranoid enough, but it needs to be changed
before we can frob the code. And since we currently have some aux power
domain get/put lingering that is essentially what can happen I think.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
