[Intel-gfx] [PATCH IGT v2 4/6] tests/kms_frontbuffer_tracking: Refactor to use IGT PSR library functions
Paulo Zanoni
paulo.r.zanoni at intel.com
Fri Jun 30 20:46:27 UTC 2017
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 at intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> Signed-off-by: Jim Bride <jim.bride at 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)
More information about the Intel-gfx
mailing list