[Intel-gfx] [PATCH i-g-t 3/8] kms_frontbuffer_tracking: Allow pipe crc or sink crc individually.

[Paulo Zanoni]

2015-11-05 16:53 GMT-02:00 Rodrigo Vivi <rodrigo.vivi at intel.com>:
> Sink CRC should be enough by itself to validate PSR. Also sometimes
> the error might be on the CRC calculations themselves. So let's give
> the flexibility to use either individually.
>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> ---
>  tests/kms_frontbuffer_tracking.c | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
> index 606d0a9..d879493 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -232,7 +232,8 @@ struct draw_pattern_info pattern4;
>  /* Command line parameters. */
>  struct {
>         bool check_status;
> -       bool check_crc;
> +       bool check_pipe_crc;
> +       bool check_sink_crc;
>         bool fbc_check_compression;
>         bool fbc_check_last_action;
>         bool no_edp;
> @@ -244,7 +245,8 @@ struct {
>         int shared_fb_y_offset;
>  } opt = {
>         .check_status = true,
> -       .check_crc = true,
> +       .check_pipe_crc = true,
> +       .check_sink_crc = true,
>         .fbc_check_compression = true,
>         .fbc_check_last_action = true,
>         .no_edp = false,
> @@ -1578,15 +1580,17 @@ static int adjust_assertion_flags(const struct test_mode *t, int flags)
>         int flags__ = (flags);                                          \
>         struct both_crcs crc_;                                          \
>                                                                         \
> -       if (!opt.check_crc || (flags__ & DONT_ASSERT_CRC))              \
> +       if (flags__ & DONT_ASSERT_CRC)                                  \
>                 break;                                                  \
>                                                                         \
>         collect_crcs(&crc_);                                            \
>         print_crc("Calculated CRC:", &crc_);                            \
>                                                                         \
>         igt_assert(wanted_crc);                                         \
> -       igt_assert_crc_equal(&crc_.pipe, &wanted_crc->pipe);            \
> -       assert_sink_crc_equal(&crc_.sink, &wanted_crc->sink);           \
> +       if (opt.check_pipe_crc)                                         \
> +               igt_assert_crc_equal(&crc_.pipe, &wanted_crc->pipe);    \
> +       if (opt.check_sink_crc)                                         \
> +               assert_sink_crc_equal(&crc_.sink, &wanted_crc->sink);   \

This, combined with patch 2/8, means that we may still get SKIPs in
case reading the sink CRC fails, even if we specify --pipe-crc-only.
We should avoid even collecting the CRCs in case the flags are passed
so we don't risk that SKIP you introduced.

>  } while (0)
>
>  #define do_status_assertions(flags_) do {                              \
> @@ -2926,7 +2930,16 @@ static int opt_handler(int option, int option_index, void *data)
>                 opt.check_status = false;
>                 break;
>         case 'c':
> -               opt.check_crc = false;
> +               opt.check_pipe_crc = false;
> +               opt.check_sink_crc = false;
> +               break;
> +       case 'S':
> +               opt.check_pipe_crc = false;
> +               opt.check_sink_crc = true;
> +               break;
> +       case 'P':
> +               opt.check_pipe_crc = true;
> +               opt.check_sink_crc = false;
>                 break;
>         case 'o':
>                 opt.fbc_check_compression = false;
> @@ -2974,6 +2987,8 @@ static int opt_handler(int option, int option_index, void *data)
>  const char *help_str =
>  "  --no-status-check           Don't check for enable/disable status\n"
>  "  --no-crc-check              Don't check for CRC values\n"
> +"  --sink-crc-only             Check for Sink CRC only. Don't check for Pipe CRC value\n"
> +"  --pipe-crc-only             Check for Pipe CRC only. Don't check for Sink CRC value\n"

I was trying really hard to keep all descriptions under 80 columns
because OCD :)

By the way, the logic behind these flags seems inverted. Since both
CRCs are enabled by default, the flags should be to prevent them. Why
not just make them "--no-sink-crc" and "--no-pipe-crc"? IMHO it's more
intuitive and clear since each flag only touches one of the knobs
instead of mandating the behavior on two different knobs (i.e.,
passing --sink-crc-only --pipe-crc-only doesn't make sense and only
the last flag will be respected, but passing --no-pipe-crc
--no-sink-crc makes sense since each flag controls only one of the
knobs).

Otherwise, I like the idea: many times I wrote small hacks to ignore
just sink CRCs but not pipe CRC but never ended up writing the option.


>  "  --no-fbc-compression-check  Don't check for the FBC compression status\n"
>  "  --no-fbc-action-check       Don't check for the FBC last action\n"
>  "  --no-edp                    Don't use eDP monitors\n"
> @@ -3097,6 +3112,8 @@ int main(int argc, char *argv[])
>         struct option long_options[] = {
>                 { "no-status-check",          0, 0, 's'},
>                 { "no-crc-check",             0, 0, 'c'},
> +               { "sink-crc-only",            0, 0, 'S'},
> +               { "pipe-crc-only",            0, 0, 'P'},
>                 { "no-fbc-compression-check", 0, 0, 'o'},
>                 { "no-fbc-action-check",      0, 0, 'a'},
>                 { "no-edp",                   0, 0, 'e'},
> --
> 2.4.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni