| Message ID | 1498849944-26404-5-git-send-email-jim.bride@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
I believe it would be better to create the psr lib with only one function at time and on every patch that adds the new function already removes that from here and from frontbuffer tracking test as well... easier to review and to make sure that we are not changing the behaviour. also... On Fri, Jun 30, 2017 at 12:12 PM, Jim Bride <jim.bride@linux.intel.com> wrote: > v2: * Minor functional tweaks and bug fixes > * Rebase > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: Jim Bride <jim.bride@linux.intel.com> > --- > 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 I believe this deserves a separated patch... > 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)) > 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); this doesn't fit anymore > 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"); > 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; > - } > + > > 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) > -- > 2.7.4 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: Jim Bride <jim.bride@linux.intel.com> 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)
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@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: Jim Bride <jim.bride@linux.intel.com> > --- > 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 > 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)) > 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); Changing the timeout from 5s to 10s is going to significantly increase the runtime for kms_frontbuffer_tracking. > -} > - > #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"); > 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; > - } > + > > 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)) { There's a difference between "wait until disabled" and "wait until enabled". If we're expecting PSR to be disabled and we call wait_until_enabled(), we guarantee that PSR is going to be disabled during the whole timeout. And that's the point: guarantee that PSR stays disabled during the entire time, i.e., nothing enables it at any point in the next $timeout seconds. It could be the case that PSR got disabled and then re-enabled due to some delayed work function, and we want to prevent that by staying the whole timeout re-checking its status. So here, if we just assert that PSR got disabled at some point during the timeout, we're changing the behavior of the test and we're losing the original purpose of the assertion. > \ > + 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)
On Fri, Jun 30, 2017 at 01:19:57PM -0700, Rodrigo Vivi wrote: > I believe it would be better to create the psr lib with only one > function at time and on every patch that adds the new function already > removes that from here and from frontbuffer tracking test as well... > > easier to review and to make sure that we are not changing the behaviour. I'm testing a new series with the requested structural changes and review feedback to-date. I hope to send them out on Monday (testing takes a while.) Jim > also... > > On Fri, Jun 30, 2017 at 12:12 PM, Jim Bride <jim.bride@linux.intel.com> wrote: > > v2: * Minor functional tweaks and bug fixes > > * Rebase > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Signed-off-by: Jim Bride <jim.bride@linux.intel.com> > > --- > > 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 > > I believe this deserves a separated patch... > > > 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)) > > 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); > > this doesn't fit anymore > > > 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"); > > 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; > > - } > > + > > > > 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) > > -- > > 2.7.4 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Rodrigo Vivi > Blog: http://blog.vivi.eng.br
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 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)) 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"); 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; - } + 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)
v2: * Minor functional tweaks and bug fixes * Rebase Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Signed-off-by: Jim Bride <jim.bride@linux.intel.com> --- tests/kms_frontbuffer_tracking.c | 119 +++++++-------------------------------- 1 file changed, 19 insertions(+), 100 deletions(-)