Em Sex, 2017-06-30 às 12:12 -0700, Jim Bride escreveu: > v2: * Minor functional tweaks and bug fixes > * Rebase > > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx> > Signed-off-by: Jim Bride <jim.bride@xxxxxxxxxxxxxxx> This patch is not just a refactor. It changes a how the code behaves. Please split it into many patches: one patch replaces code with equivalent code from the PSR library, and the other patches should each do one of the actual code changes that this patch does. > --- > tests/kms_frontbuffer_tracking.c | 119 +++++++-------------------- > ------------ > 1 file changed, 19 insertions(+), 100 deletions(-) > > diff --git a/tests/kms_frontbuffer_tracking.c > b/tests/kms_frontbuffer_tracking.c > index c24e4a8..3a8b754 100644 > --- a/tests/kms_frontbuffer_tracking.c > +++ b/tests/kms_frontbuffer_tracking.c > @@ -183,7 +183,7 @@ struct { > }; > > > -#define SINK_CRC_SIZE 12 > +#define SINK_CRC_SIZE 6 As Rodrigo mentioned, this is going to break. > typedef struct { > char data[SINK_CRC_SIZE]; > } sink_crc_t; > @@ -327,28 +327,6 @@ drmModeModeInfo std_1024_mode = { > .name = "Custom 1024x768", > }; > > -static drmModeModeInfoPtr > get_connector_smallest_mode(drmModeConnectorPtr c) > -{ > - int i; > - drmModeModeInfoPtr smallest = NULL; > - > - for (i = 0; i < c->count_modes; i++) { > - drmModeModeInfoPtr mode = &c->modes[i]; > - > - if (!smallest) > - smallest = mode; > - > - if (mode->hdisplay * mode->vdisplay < > - smallest->hdisplay * smallest->vdisplay) > - smallest = mode; > - } > - > - if (c->connector_type == DRM_MODE_CONNECTOR_eDP) > - smallest = &std_1024_mode; > - > - return smallest; > -} > - > static drmModeConnectorPtr get_connector(uint32_t id) > { > int i; > @@ -421,30 +399,6 @@ static void init_mode_params(struct > modeset_params *params, uint32_t crtc_id, > params->sprite.h = 64; > } > > -static bool connector_get_mode(drmModeConnectorPtr c, > drmModeModeInfoPtr *mode) > -{ > - *mode = NULL; > - > - if (c->connection != DRM_MODE_CONNECTED || !c->count_modes) > - return false; > - > - if (c->connector_type == DRM_MODE_CONNECTOR_eDP && > opt.no_edp) > - return false; > - > - if (opt.small_modes) > - *mode = get_connector_smallest_mode(c); > - else > - *mode = &c->modes[0]; > - > - /* On HSW the CRC WA is so awful that it makes you think > everything is > - * bugged. */ > - if (IS_HASWELL(intel_get_drm_devid(drm.fd)) && > - c->connector_type == DRM_MODE_CONNECTOR_eDP) > - *mode = &std_1024_mode; > - > - return true; > -} > - > static bool connector_supports_pipe_a(drmModeConnectorPtr connector) > { > int i; > @@ -473,7 +427,7 @@ static bool find_connector(bool edp_only, bool > pipe_a, uint32_t forbidden_id, > continue; > if (c->connector_id == forbidden_id) > continue; > - if (!connector_get_mode(c, &mode)) > + if (!igt_psr_find_good_mode(c, &mode)) igt_psr_find_good_mode() doesn't seem to do what connector_get_mode() does, we can't use it. This is going to introduce bugs. > continue; > > *ret_connector = c; > @@ -804,23 +758,6 @@ static void fbc_print_status(void) > igt_info("FBC status:\n%s\n", buf); > } > > -static bool psr_is_enabled(void) > -{ > - char buf[256]; > - > - debugfs_read("i915_edp_psr_status", buf); > - return strstr(buf, "\nActive: yes\n") && > - strstr(buf, "\nHW Enabled & Active bit: yes\n"); > -} > - > -static void psr_print_status(void) > -{ > - char buf[256]; > - > - debugfs_read("i915_edp_psr_status", buf); > - igt_info("PSR status:\n%s\n", buf); > -} > - > static struct timespec fbc_get_last_action(void) > { > struct timespec ret = { 0, 0 }; > @@ -926,44 +863,31 @@ static bool fbc_wait_until_enabled(void) > return igt_wait(fbc_is_enabled(), 2000, 1); > } > > -static bool psr_wait_until_enabled(void) > -{ > - return igt_wait(psr_is_enabled(), 5000, 1); > -} > - > #define fbc_enable() igt_set_module_param_int("enable_fbc", 1) > #define fbc_disable() igt_set_module_param_int("enable_fbc", 0) > -#define psr_enable() igt_set_module_param_int("enable_psr", 1) > -#define psr_disable() igt_set_module_param_int("enable_psr", 0) > > static void get_sink_crc(sink_crc_t *crc, bool mandatory) > { > - int rc, errno_; > + int rc; > > if (!sink_crc.supported) { > memcpy(crc, "unsupported!", SINK_CRC_SIZE); > return; > } > > - lseek(sink_crc.fd, 0, SEEK_SET); > - > - rc = read(sink_crc.fd, crc->data, SINK_CRC_SIZE); > - errno_ = errno; > - > - if (rc == -1 && errno_ == ENOTTY) { > + rc = igt_psr_get_sink_crc(sink_crc.fd, crc->data); > + if (rc == ENOTTY) { > igt_info("Sink CRC not supported: panel doesn't > support it\n"); We'll print this twice now. > sink_crc.supported = false; > - } else if (rc == -1 && errno_ == ETIMEDOUT) { > - if (sink_crc.reliable) { > - igt_info("Sink CRC is unreliable on this > machine.\n"); > + } else if (rc == ETIMEDOUT) { > + if (sink_crc.reliable) > sink_crc.reliable = false; > - } > + > There are some problems with white spaces in the lines above: a double blank line and trailing spaces at the end of lines. > if (mandatory) > igt_skip("Sink CRC is unreliable on this > machine.\n"); > } else { > - igt_assert_f(rc != -1, "Unexpected error: %d\n", > errno_); > - igt_assert(rc == SINK_CRC_SIZE); > + igt_assert_f(rc == 0, "Unexpected error: %d\n", rc); > } > } > > @@ -1180,7 +1104,7 @@ static void disable_features(const struct > test_mode *t) > return; > > fbc_disable(); > - psr_disable(); > + igt_psr_disable(); > } > > static void *busy_thread_func(void *data) > @@ -1432,7 +1356,7 @@ static void setup_sink_crc(void) > fill_fb_region(&prim_mode_params.fb, COLOR_PRIM_BG); > set_mode_for_params(&prim_mode_params); > > - sink_crc.fd = openat(drm.debugfs, "i915_sink_crc_eDP1", > O_RDONLY); > + sink_crc.fd = igt_psr_open_sink_crc(drm.debugfs); > igt_assert_lte(0, sink_crc.fd); > > /* Do a first read to try to detect if it's supported. */ > @@ -1547,14 +1471,6 @@ static void teardown_fbc(void) > { > } > > -static bool psr_sink_has_support(void) > -{ > - char buf[256]; > - > - debugfs_read("i915_edp_psr_status", buf); > - return strstr(buf, "Sink_Support: yes\n"); > -} > - > static void setup_psr(void) > { > if (get_connector(prim_mode_params.connector_id)- > >connector_type != > @@ -1563,7 +1479,7 @@ static void setup_psr(void) > return; > } > > - if (!psr_sink_has_support()) { > + if (!igt_psr_sink_support(drm.fd)) { > igt_info("Can't test PSR: not supported by > sink.\n"); > return; > } > @@ -1717,12 +1633,15 @@ static int adjust_assertion_flags(const > struct test_mode *t, int flags) > } > \ > > \ > if (flags_ & ASSERT_PSR_ENABLED) { > \ > - if (!psr_wait_until_enabled()) { > \ > - psr_print_status(); > \ > + if (!igt_psr_await_status(drm.fd, true)) { > \ > + igt_psr_print_status(drm.fd); > \ > igt_assert_f(false, "PSR disabled\n"); > \ > } > \ > } else if (flags_ & ASSERT_PSR_DISABLED) { > \ > - igt_assert(!psr_wait_until_enabled()); > \ > + if (!igt_psr_await_status(drm.fd, false)) { > \ > + igt_psr_print_status(drm.fd); > \ > + igt_assert_f(false, "PSR enabled\n"); > \ > + } > \ > } > \ > } while (0) > > @@ -1822,7 +1741,7 @@ static void enable_features_for_test(const > struct test_mode *t) > if (t->feature & FEATURE_FBC) > fbc_enable(); > if (t->feature & FEATURE_PSR) > - psr_enable(); > + igt_psr_enable(); > } > > static void check_test_requirements(const struct test_mode *t) _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx