Re: [igt-dev] [PATCH i-g-t] tests/kms_available_modes_crc: Fix handling of test comparisons

Juha-Pekka Heikkila
2019-01-29T14:01:40Z

On 23.1.2019 21.50, Pandiyan, Dhinakaran wrote:
> On Wed, 2018-12-19 at 15:41 +0200, Juha-Pekka Heikkila wrote:
>> Fixed handling of single plane modes so used bpp never get to
>> be 0 and ease off crc comparison a bit with gamma tables as
>> idea of this test is to see chosen modes are able to initialize.
>>
>> Release output to PIPE_ANY at end of test per connector.
>>
>> Reduce verboseness if there's no errors.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106641
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106701
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>> ---
>> tests/kms_available_modes_crc.c | 169 ++++++++++++++++++++--------
>> ------------
>> 1 file changed, 86 insertions(+), 83 deletions(-)
>>
>> diff --git a/tests/kms_available_modes_crc.c
>> b/tests/kms_available_modes_crc.c
>> index 45a4775..04472d0 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,42 +134,33 @@ 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},
>> -
>> - /*
>> - * following two are skipped because blending seems to work
>> - * incorrectly with exception of AR24 on cursor plane.
>> - * 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_XRGB2101010, 0, BYTES_PP_4, 0xffffffff},
>> - { DRM_FORMAT_XBGR2101010, 0, 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_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},
>> + { DRM_FORMAT_ARGB8888, BYTES_PP_4, 0xffffffff},
>> + { DRM_FORMAT_ABGR8888, BYTES_PP_4, 0x00ffffff},
>> + { DRM_FORMAT_XRGB2101010, BYTES_PP_4, 0xffffffff},
>> + { DRM_FORMAT_XBGR2101010, BYTES_PP_4, 0xffffffff},
>> + { 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},
>> #endif
>> - { 0, 0, 0, 0 }
>> + { 0, SKIP4, 0 }
>> };
>>
>> /*
>> @@ -221,24 +217,9 @@ static bool fill_in_fb(data_t *data,
>> igt_output_t *output, igt_plane_t *plane,
>> writesize = data->size+data->size/2;
>> break;
>> case SKIP4:
>> - if (fillers[i].fourcc == DRM_FORMAT_ARGB8888 &&
>> - plane->type == DRM_PLANE_TYPE_CURSOR) {
>> - /*
>> - * special for cursor plane where blending works
>> correctly.
>> - */
>> - ptemp_32_buf = (unsigned int*)data->buf;
>> - for (c = 0; c < data->size/4; c++)
>> - ptemp_32_buf[c] = fillers[i].value;
>> - writesize = data->size;
>> - break;
>> - }
>> - igt_info("Format %s CRC comparison skipped by
>> design.\n",
>> - (char*)&fillers[i].fourcc);
>> -
>> - return false;
>> 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;
>> }
>>
>> @@ -261,6 +242,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;
>> @@ -289,6 +272,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 */
>> case BYTES_PP_4:
>> bpp = 32;
>> break;
>> @@ -319,7 +304,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;
>> }
>> @@ -380,13 +365,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*
>> 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)){
>> /*
>> @@ -404,32 +390,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, &current_crc);
>>
>> - if (plane->type != DRM_PLANE_TYPE_CURSOR) {
>> - if (!igt_check_crc_equal(&current_crc,
>> - &data->fullscreen_crc)) {
>> - crccompare[0] =
>> igt_crc_to_string(&current_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(&current_crc,
>> - &data->cursor_crc)) {
>> - crccompare[0] =
>> igt_crc_to_string(&current_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(&current_crc, p_crc))
>> {
>> + crccompare[0] =
>> igt_crc_to_string(&current_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]);
>
> Can you please split this debug print into a separate patch? That way
> we can see which format/plane/pipe combination is failing on ICL while
> you are working on the gamma fixes?

This is what you were hoping for?
https://patchwork.freedesktop.org/series/55727/

/Juha-Pekka

>
>
>
>> +
>> + free(crccompare[0]);
>> + free(crccompare[1]);
>> + if (!data->allow_fail)
>> rVal++;
>> - }
>> }
>> }
>> remove_fb(data, output, plane);
>> return rVal;
>> }
>> - return 1;
>> + return data->allow_fail ? 0 : 1;
>> }
>>
>>
>> @@ -440,14 +426,38 @@ test_available_modes(data_t* data)
>> igt_plane_t *plane;
>> int modeindex;
>> enum pipe pipe;
>> - int invalids = 0;
>> + int failed_crcs = 0, i, lut_size;
>> drmModePlane *modePlane;
>> - char planetype[3][8] = {"OVERLAY\0", "PRIMARY\0", "CURSOR\0" };
>> +
>> + struct {
>> + uint16_t red;
>> + uint16_t green;
>> + uint16_t blue;
>> + uint16_t reserved;
>> + } *lut;
>>
>> 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);
>>
>> + if (igt_pipe_obj_has_prop(&data->display.pipes[pipe],
>> IGT_CRTC_GAMMA_LUT_SIZE)) {
>> + lut_size = igt_pipe_get_prop(&data->display,
>> pipe,
>> + IGT_CRTC_GAMMA_LUT
>> _SIZE);
>> + lut = calloc(sizeof(*lut), lut_size);
>> +
>> + for (i = 0; i < lut_size; i++) {
>> + lut[i].red = (i * 0xffff / (lut_size -
>> 1)) & 0xfc00;
>> + lut[i].green = (i * 0xffff / (lut_size
>> - 1)) & 0xfc00;
>> + lut[i].blue = (i * 0xffff / (lut_size -
>> 1)) & 0xfc00;
>> + }
>> +
>> + igt_pipe_replace_prop_blob(&data->display,
>> pipe,
>> + IGT_CRTC_GAMMA_LUT,
>> + lut, sizeof(*lut) *
>> lut_size);
>> + free(lut);
>> + }
>> +
>> data->pipe_crc = igt_pipe_crc_new(data->gfx_fd, pipe,
>> INTEL_PIPE_CRC_SOURCE
>> _AUTO);
>>
>> @@ -467,25 +477,18 @@ 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);
>> }
>> igt_pipe_crc_stop(data->pipe_crc);
>> igt_pipe_crc_free(data->pipe_crc);
>> - igt_display_commit2(&data->display, data->commit);
>> +
>> + igt_output_set_pipe(output, PIPE_ANY);
>> }
>> - igt_assert(invalids == 0);
>> + igt_assert(failed_crcs == 0);
>> }
>>
>>