Re: [igt-dev] [PATCH i-g-t v4 3/4] test: Add PSR2 selective update tests

Dhinakaran Pandiyan
2019-01-24T19:28:35Z

On Thu, 2019-01-24 at 10:27 -0800, Souza, Jose wrote:
> On Wed, 2019-01-23 at 18:33 -0800, Dhinakaran Pandiyan wrote:
> > On Wed, 2019-01-23 at 15:56 -0800, José Roberto de Souza wrote:
> > > This tests checks if hardware is able to do selective update when
> > > screen changes.
> > > PSR2 don't trigger interruptions and the 'PSR2 SU status'
> > > register
> > > is not kept loaded all the times, so it is necessary keep polling
> > > PSR status debugfs until those values are loaded.
> > >
> > > Also from DEEP_SLEEP state HW will not do a seletive update, as
> > > most of the memory/context is lost in deep sleep state hardware
> > > will
> > > need to exit PSR mode then wait a configured number of frames to
> > > activate PSR again to then start doing seletive updates, that is
> > > why
> > > just one screen change is not enough to pass this tests.
> > >
> > > When a selective update happens and the values are loaded and
> > > read
> > > from debugfs it is compared with the expected value of seletive
> > > update blocks, if matches the polling is stopped and the test
> > > passed
> > > otherwise it will wait until it reachs a maximum number o screen
> > > changes to fail the test.
> > >
> > > v2: Using new SU blocks debugfs output
> > >
> > > v3:
> > > - removed the timerfd to fail the test, now failing based in a
> > > maximum number of screen changes
> > > - removing thread to read debugfs, read from main thread is
> > > enough
> > > - improved commit message
> > >
> > > v4:
> > > - getting cairo context for frontbuffer test in prepare()
> > > - droppoing poll(), using blocking timerfd instead
> > >
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > > lib/igt_psr.c | 29 ++++
> > > lib/igt_psr.h | 1 +
> > > tests/Makefile.sources | 1 +
> > > tests/kms_psr2_su.c | 292
> > > +++++++++++++++++++++++++++++++++++++++++
> > > tests/meson.build | 1 +
> > > 5 files changed, 324 insertions(+)
> > > create mode 100644 tests/kms_psr2_su.c
> > >
> > > diff --git a/lib/igt_psr.c b/lib/igt_psr.c
> > > index d726fad5..8c0f05e8 100644
> > > --- a/lib/igt_psr.c
> > > +++ b/lib/igt_psr.c
> > > @@ -178,3 +178,32 @@ bool psr_sink_support(int debugfs_fd, enum
> > > psr_mode mode)
> > > */
> > > return strstr(buf, "Sink support: yes [0x03]");
> > > }
> > > +
> > > +#define PSR2_SU_BLOCK_STR_LOOKUP "PSR2 SU blocks:\n0\t"
> > > +
> > > +static bool
> > > +psr2_read_last_num_su_blocks_val(int debugfs_fd, uint16_t
> > > *num_su_blocks)
> > > +{
> > > + char buf[PSR_STATUS_MAX_LEN];
> > > + char *str;
> > > + int ret;
> > > +
> > > + ret = igt_debugfs_simple_read(debugfs_fd,
> > > "i915_edp_psr_status", buf,
> > > + sizeof(buf));
> > > + if (ret < 0)
> > > + return false;
> > > +
> > > + str = strstr(buf, PSR2_SU_BLOCK_STR_LOOKUP);
> > > + if (!str)
> > > + return false;
> > > +
> > > + str = &str[strlen(PSR2_SU_BLOCK_STR_LOOKUP)];
> > > + *num_su_blocks = (uint16_t)strtol(str, NULL, 10);
> > > +
> > > + return true;
> > > +}
> > > +
> > > +bool psr2_wait_su(int debugfs_fd, uint16_t *num_su_blocks)
> > > +{
> > > + return igt_wait(psr2_read_last_num_su_blocks_val(debugfs_fd,
> > > num_su_blocks), 40, 1);
> > > +}
> > > diff --git a/lib/igt_psr.h b/lib/igt_psr.h
> > > index 7e7017bf..49599cf8 100644
> > > --- a/lib/igt_psr.h
> > > +++ b/lib/igt_psr.h
> > > @@ -40,5 +40,6 @@ bool psr_wait_update(int debugfs_fd, enum
> > > psr_mode
> > > mode);
> > > bool psr_enable(int debugfs_fd, enum psr_mode);
> > > bool psr_disable(int debugfs_fd);
> > > bool psr_sink_support(int debugfs_fd, enum psr_mode);
> > > +bool psr2_wait_su(int debugfs_fd, uint16_t *num_su_blocks);
> > >
> > > #endif
> > > diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> > > index 519eac79..9174aecc 100644
> > > --- a/tests/Makefile.sources
> > > +++ b/tests/Makefile.sources
> > > @@ -80,6 +80,7 @@ TESTS_progs = \
> > > kms_plane_scaling \
> > > kms_properties \
> > > kms_psr \
> > > + kms_psr2_su \
> > > kms_pwrite_crc \
> > > kms_rmfb \
> > > kms_rotation_crc \
> > > diff --git a/tests/kms_psr2_su.c b/tests/kms_psr2_su.c
> > > new file mode 100644
> > > index 00000000..344ddffe
> > > --- /dev/null
> > > +++ b/tests/kms_psr2_su.c
> > > @@ -0,0 +1,292 @@
> > > +/*
> > > + * Copyright © 2019 Intel Corporation
> > > + *
> > > + * Permission is hereby granted, free of charge, to any person
> > > obtaining a
> > > + * copy of this software and associated documentation files (the
> > > "Software"),
> > > + * to deal in the Software without restriction, including
> > > without
> > > limitation
> > > + * the rights to use, copy, modify, merge, publish, distribute,
> > > sublicense,
> > > + * and/or sell copies of the Software, and to permit persons to
> > > whom
> > > the
> > > + * Software is furnished to do so, subject to the following
> > > conditions:
> > > + *
> > > + * The above copyright notice and this permission notice
> > > (including
> > > the next
> > > + * paragraph) shall be included in all copies or substantial
> > > portions of the
> > > + * Software.
> > > + *
> > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY
> > > KIND,
> > > EXPRESS OR
> > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> > > MERCHANTABILITY,
> > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO
> > > EVENT SHALL
> > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM,
> > > DAMAGES
> > > OR OTHER
> > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR
> > > OTHERWISE,
> > > ARISING
> > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > > OTHER DEALINGS
> > > + * IN THE SOFTWARE.
> > > + *
> > > + */
> > > +
> > > +#include "igt.h"
> > > +#include "igt_sysfs.h"
> > > +#include "igt_psr.h"
> > > +#include <errno.h>
> > > +#include <stdbool.h>
> > > +#include <stdio.h>
> > > +#include <string.h>
> > > +#include <sys/timerfd.h>
> > > +#include "intel_bufmgr.h"
> > > +
> > > +IGT_TEST_DESCRIPTION("Test PSR2 selective update");
> > > +
> > > +#define SQUARE_SIZE 100
> > > +/* each selective update block is 4 lines tall */
> > > +#define EXPECTED_NUM_SU_BLOCKS ((SQUARE_SIZE / 4) + (SQUARE_SIZE
> > > %
> > > 4
> > > ? 1 : 0))
> > > +
> > > +/*
> > > + * Minimum is 15 as the number of frames to active PSR2 could be
> > > configured
> > > + * to 15 frames plus a few more in case we miss a selective
> > > update
> > > between
> > > + * debugfs reads.
> > > + */
> > > +#define MAX_SCREEN_CHANGES 20
> > > +
> > > +enum operations {
> > > + PAGE_FLIP,
> > > + FRONTBUFFER,
> > > + LAST
> > > +};
> > > +
> > > +static const char *op_str(enum operations op)
> > > +{
> > > + static const char * const name[] = {
> > > + [PAGE_FLIP] = "page_flip",
> > > + [FRONTBUFFER] = "frontbuffer"
> > > + };
> > > +
> > > + return name[op];
> > > +}
> > > +
> > > +typedef struct {
> > > + int drm_fd;
> > > + int debugfs_fd;
> > > + igt_display_t display;
> > > + drm_intel_bufmgr *bufmgr;
> > > + drmModeModeInfo *mode;
> > > + igt_output_t *output;
> > > + struct igt_fb fb[2];
> > > + enum operations op;
> > > + cairo_t *cr;
> > > + int change_screen_timerfd;
> > > + uint32_t screen_changes;
> > > +} data_t;
> > > +
> > > +static void setup_output(data_t *data)
> > > +{
> > > + igt_display_t *display = &data->display;
> > > + igt_output_t *output;
> > > + enum pipe pipe;
> > > +
> > > + for_each_pipe_with_valid_output(display, pipe, output) {
> > > + drmModeConnectorPtr c = output->config.connector;
> > > +
> > > + if (c->connector_type != DRM_MODE_CONNECTOR_eDP)
> > > + continue;
> > > +
> > > + igt_output_set_pipe(output, pipe);
> > > + data->output = output;
> > > + data->mode = igt_output_get_mode(output);
> >
> > Won't this fail the same way as other PSR2 tests on WHL because of
> > the
> > default mode exceeding PSR2 capability?
>
> As the goal of this test is not to test if PSR2 can be enabled, on
> igt_fixture() we have:
>
> igt_require(psr_enable(data.debugfs_fd, PSR_MODE_2));
> igt_require(psr_wait_entry(data.debugfs_fd, PSR_MODE_2));
psr_wait_entry() is the one that would return false, isn't it?

>
> So this test will be skiped and the error will be reported by
> kms_psr@psr2_basic and other tests in kms_psr.
>
Fair enough.

I was about add a Reviewed-by but then noticed that the test skipped on
one of the ICL. Do you know what's happening?

> >