Re: [PATCH IGT v2 4/6] tests/kms_frontbuffer_tracking: Refactor to use IGT PSR library functions

[Jim Bride <jim.bride@linux.intel.com>]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx