[igt-dev] [PATCH i-g-t] tests/kms_available_modes_crc: Fix handling of test comparisons
Juha-Pekka Heikkila
juhapekka.heikkila at gmail.com
Wed Jan 30 11:25:31 UTC 2019
On 30.1.2019 8.40, Dhinakaran Pandiyan wrote:
> On Fri, 2019-01-25 at 14:30 +0200, Juha-Pekka Heikkila wrote:
>> Fixed handling of single plane modes so used bpp never get to
>> be 0.
>>
>> Reduce verboseness if there's no errors.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106701
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
>> ---
>> tests/kms_available_modes_crc.c | 123 ++++++++++++++++++++--------
>> ------------
>> 1 file changed, 61 insertions(+), 62 deletions(-)
>>
>> diff --git a/tests/kms_available_modes_crc.c
>> b/tests/kms_available_modes_crc.c
>> index 7ff385f..432c084 100644
>> --- a/tests/kms_available_modes_crc.c
>> +++ b/tests/kms_available_modes_crc.c
>> @@ -54,6 +54,12 @@ typedef struct {
>>
>> igt_crc_t cursor_crc;
>> igt_crc_t fullscreen_crc;
>> +
>> + /*
>> + * If there's unknown format setting up mode can fail
>> + * without failing entire test.
>> + */
>> + bool allow_fail;
>> } data_t;
>>
>>
>> @@ -120,7 +126,6 @@ static void generate_comparison_crc_list(data_t
>> *data, igt_output_t *output)
>>
>> static const struct {
>> uint32_t fourcc;
>> - char zeropadding;
>> enum { BYTES_PP_1=1,
>> BYTES_PP_2=2,
>> BYTES_PP_4=4,
>> @@ -129,10 +134,10 @@ static const struct {
>> SKIP4 } bpp;
>> uint32_t value;
>> } fillers[] = {
>> - { DRM_FORMAT_C8, 0, BYTES_PP_1, 0xff},
>> - { DRM_FORMAT_RGB565, 0, BYTES_PP_2, 0xffff},
>> - { DRM_FORMAT_XRGB8888, 0, BYTES_PP_4, 0xffffffff},
>> - { DRM_FORMAT_XBGR8888, 0, BYTES_PP_4, 0xffffffff},
>> + { DRM_FORMAT_C8, BYTES_PP_1, 0xff},
>> + { DRM_FORMAT_RGB565, BYTES_PP_2, 0xffff},
>> + { DRM_FORMAT_XRGB8888, BYTES_PP_4, 0xffffffff},
>> + { DRM_FORMAT_XBGR8888, BYTES_PP_4, 0xffffffff},
>>
>> /*
>> * following two are skipped because blending seems to work
>> @@ -140,31 +145,31 @@ static const struct {
>> * Test still creates the planes, just filling plane
>> * and getting crc is skipped.
>> */
>> - { DRM_FORMAT_ARGB8888, 0, SKIP4, 0xffffffff},
>> - { DRM_FORMAT_ABGR8888, 0, SKIP4, 0x00ffffff},
>> + { DRM_FORMAT_ARGB8888, SKIP4, 0xffffffff},
>> + { DRM_FORMAT_ABGR8888, SKIP4, 0x00ffffff},
>>
>> - { DRM_FORMAT_XRGB2101010, 0, BYTES_PP_4, 0xffffffff},
>> - { DRM_FORMAT_XBGR2101010, 0, BYTES_PP_4, 0xffffffff},
>> + { DRM_FORMAT_XRGB2101010, BYTES_PP_4, 0xffffffff},
>> + { DRM_FORMAT_XBGR2101010, BYTES_PP_4, 0xffffffff},
>>
>> - { DRM_FORMAT_YUYV, 0, BYTES_PP_4, 0x80eb80eb},
>> - { DRM_FORMAT_YVYU, 0, BYTES_PP_4, 0x80eb80eb},
>> - { DRM_FORMAT_VYUY, 0, BYTES_PP_4, 0xeb80eb80},
>> - { DRM_FORMAT_UYVY, 0, BYTES_PP_4, 0xeb80eb80},
>> + { DRM_FORMAT_YUYV, BYTES_PP_4, 0x80eb80eb},
>> + { DRM_FORMAT_YVYU, BYTES_PP_4, 0x80eb80eb},
>> + { DRM_FORMAT_VYUY, BYTES_PP_4, 0xeb80eb80},
>> + { DRM_FORMAT_UYVY, BYTES_PP_4, 0xeb80eb80},
>>
>> /*
>> * (semi-)planar formats
>> */
>> - { DRM_FORMAT_NV12, 0, NV12, 0x80eb},
>> + { DRM_FORMAT_NV12, NV12, 0x80eb},
>> #ifdef DRM_FORMAT_P010
>> - { DRM_FORMAT_P010, 0, P010, 0x8000eb00},
>> + { DRM_FORMAT_P010, P010, 0x8000eb00},
>> #endif
>> #ifdef DRM_FORMAT_P012
>> - { DRM_FORMAT_P012, 0, P010, 0x8000eb00},
>> + { DRM_FORMAT_P012, P010, 0x8000eb00},
>> #endif
>> #ifdef DRM_FORMAT_P016
>> - { DRM_FORMAT_P016, 0, P010, 0x8000eb00},
>> + { DRM_FORMAT_P016, P010, 0x8000eb00},
>
> Doesn't the library implement this stuff already? I see similar code in
> create_bo_for_fb(). Even if new formats are needed, I think it's best
> to put that in the lib/igt_fb.
These days igt_fb does implement most of these modes I agree. This test
was written back in the day when igt support was limited to RGB888 and
RGB565. Idea of this test was just to try all fb formats kernel
advertised could actually initialize.
If you feel this test is obsolete you could ask for votes to get it
removed, I tried that before xmas but nobody was interested in removing
this so I started to fix problems seen in this test.
In any case these days one cannot include new fb formats into i915
without IGT support so kms_plane test does what this test does.
>
>
>> #endif
>> - { 0, 0, 0, 0 }
>> + { 0, SKIP4, 0 }
>> };
>>
>> /*
>> @@ -233,13 +238,10 @@ static bool fill_in_fb(data_t *data,
>> igt_output_t *output, igt_plane_t *plane,
>> writesize = data->size;
>> break;
>> }
>> - igt_info("Format %s CRC comparison skipped by
>> design.\n",
>> - (char*)&fillers[i].fourcc);
>> -
>> - return false;
>> + /* Fall through */
>> default:
>> - igt_info("Unsupported mode for test %s\n",
>> - (char*)&fillers[i].fourcc);
>> + igt_info("Unsupported mode for testing CRC %.4s\n",
>> + (char *)&format);
>> return false;
>> }
>>
>> @@ -260,6 +262,8 @@ static bool setup_fb(data_t *data, igt_output_t
>> *output, igt_plane_t *plane,
>> int bpp = 0;
>> int i;
>>
>> + data->allow_fail = false;
>> +
>> mode = igt_output_get_mode(output);
>> if (plane->type != DRM_PLANE_TYPE_CURSOR) {
>> w = mode->hdisplay;
>> @@ -288,6 +292,8 @@ static bool setup_fb(data_t *data, igt_output_t
>> *output, igt_plane_t *plane,
>> break;
>>
>> case SKIP4:
>> + data->allow_fail = true;
>> + /* fall through */
> Wouldn't this prevent CRC testing for
> (fillers[i].fourcc == DRM_FORMAT_ARGB8888 && plane->type ==
> DRM_PLANE_TYPE_CURSOR) ?
There was later patch which fixed this. This version didn't pass CI test
because I had forgotten "igt_display_reset(&data->display)" call bit
below, it killed test on KBL.
>
>
>> case BYTES_PP_4:
>> bpp = 32;
>> break;
>> @@ -324,7 +330,7 @@ static bool setup_fb(data_t *data, igt_output_t
>> *output, igt_plane_t *plane,
>>
>> if(ret < 0) {
>> igt_info("Creating fb for format %s failed, return code
>> %d\n",
>> - (char*)&data->format.name, ret);
>> + (char *)&data->format.name, ret);
>>
>> return false;
>> }
>> @@ -385,13 +391,14 @@ static bool prepare_crtc(data_t *data,
>> igt_output_t *output,
>>
>>
>> static int
>> -test_one_mode(data_t* data, igt_output_t *output, igt_plane_t*
> Side note not related to the changes you are making, "mode" is quite
> misleading. Shouldn't the test be called kms_pixel_formats_crc or
> something? I thought this was about testing different connector modes.
>
>
>> plane,
>> - int mode)
>> +test_one_mode(data_t *data, igt_output_t *output, igt_plane_t
>> *plane,
>> + enum pipe pipe, int mode)
>> {
>> igt_crc_t current_crc;
>> signed rVal = 0;
>> bool do_crc;
>> - char* crccompare[2];
>> + char *crccompare[2];
>> + igt_crc_t *p_crc;
>>
>> if (prepare_crtc(data, output, plane, mode)){
>> /*
>> @@ -409,32 +416,32 @@ test_one_mode(data_t* data, igt_output_t
>> *output, igt_plane_t* plane,
>> if (do_crc) {
>> igt_pipe_crc_get_current(data->gfx_fd, data-
>>> pipe_crc, ¤t_crc);
>>
>> - if (plane->type != DRM_PLANE_TYPE_CURSOR) {
>> - if (!igt_check_crc_equal(¤t_crc,
>> - &data->fullscreen_crc)) {
>> - crccompare[0] =
>> igt_crc_to_string(¤t_crc);
>> - crccompare[1] =
>> igt_crc_to_string(&data->fullscreen_crc);
>> - igt_warn("crc mismatch. target
>> %.8s, result %.8s.\n", crccompare[0], crccompare[1]);
>> - free(crccompare[0]);
>> - free(crccompare[1]);
>> - rVal++;
>> - }
>> - } else {
>> - if (!igt_check_crc_equal(¤t_crc,
>> - &data->cursor_crc)) {
>> - crccompare[0] =
>> igt_crc_to_string(¤t_crc);
>> - crccompare[1] =
>> igt_crc_to_string(&data->cursor_crc);
>> - igt_warn("crc mismatch. target
>> %.8s, result %.8s.\n", crccompare[0], crccompare[1]);
>> - free(crccompare[0]);
>> - free(crccompare[1]);
>> + if (plane->type != DRM_PLANE_TYPE_CURSOR)
>> + p_crc = &data->fullscreen_crc;
>> + else
>> + p_crc = &data->cursor_crc;
>> +
>> + if (!igt_check_crc_equal(¤t_crc, p_crc))
>> {
>> + crccompare[0] =
>> igt_crc_to_string(¤t_crc);
>> + crccompare[1] =
>> igt_crc_to_string(p_crc);
>> + igt_warn("crc mismatch on %s PIPE %s
>> plane %d format %.4s. target %.8s, result %.8s.\n",
>> + igt_output_name(output),
>> + kmstest_pipe_name(pipe),
>> + plane->index,
>> + (char *)&data->format.name,
>> + crccompare[0],
>> + crccompare[1]);
> This is the only hunk I was hoping you would be able to split into a
> separate patch.
>
>> +
>> + free(crccompare[0]);
>> + free(crccompare[1]);
>> + if (!data->allow_fail)
> Is there a point of reading CRCs if failures are allowed?
It is marked as mode which is not listed in this test but initialization
worked so default paths are followed. It will produce warning which I
think is useful for indicating this test need updating but will not
produce failure to mark newly added features caused problems.
>
>> rVal++;
>> - }
>> }
>> }
>> remove_fb(data, output, plane);
>> return rVal;
>> }
>> - return 1;
>> + return data->allow_fail ? 0 : 1;
>> }
>>
>>
>> @@ -445,11 +452,11 @@ test_available_modes(data_t* data)
>> igt_plane_t *plane;
>> int modeindex;
>> enum pipe pipe;
>> - int invalids = 0;
>> + int failed_crcs = 0;
>> drmModePlane *modePlane;
>> - char planetype[3][8] = {"OVERLAY\0", "PRIMARY\0", "CURSOR\0" };
>>
>> for_each_pipe_with_valid_output(&data->display, pipe, output) {
>> + igt_display_reset(&data->display);
>> igt_output_set_pipe(output, pipe);
>> igt_display_commit2(&data->display, data->commit);
>>
>> @@ -472,17 +479,9 @@ test_available_modes(data_t* data)
>> modeindex++) {
>> data->format.dword = modePlane-
>>> formats[modeindex];
>>
>> - igt_info("Testing connector %s using
>> pipe %s" \
>> - " plane index %d type %s mode
>> %s\n",
>> - igt_output_name(output),
>> - kmstest_pipe_name(pipe),
>> - plane->index,
>> - planetype[plane->type],
>> - (char*)&data->format.name);
>> -
>> - invalids += test_one_mode(data, output,
>> - plane,
>> - modePlane-
>>> formats[modeindex]);
>> + failed_crcs += test_one_mode(data,
>> output,
>> + plane,
>> pipe,
>> + modePlane-
>>> formats[modeindex]);
>> }
>> drmModeFreePlane(modePlane);
>> }
>> @@ -491,7 +490,7 @@ test_available_modes(data_t* data)
>> igt_display_commit2(&data->display, data->commit);
>> igt_output_set_pipe(output, PIPE_NONE);
>> }
>> - igt_assert(invalids == 0);
>> + igt_assert_f((failed_crcs == 0), "There was %d modes with
>> invalid crc\n", failed_crcs);
>> }
>>
>>
>
More information about the igt-dev
mailing list