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@xxxxxxxxxxxxxxx> wrote:
> > 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>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
