[igt-dev] [PATCH i-g-t] tests/kms_available_modes_crc limit tested fb formats
Dhinakaran Pandiyan
dhinakaran.pandiyan at intel.com
Thu Feb 21 22:04:19 UTC 2019
On Thu, 2019-02-21 at 13:28 +0200, Juha-Pekka Heikkila wrote:
> On 21.2.2019 2.01, Dhinakaran Pandiyan wrote:
> > On Mon, 2019-02-18 at 16:22 +0200, Juha-Pekka Heikkila wrote:
> > > This test is causing too much useless noise. Limit tested
> > > fb formats to DRM_FORMAT_C8 and DRM_FORMAT_XBGR2101010 for now.
> > > These two formats are currently not tested otherwise thus
> > > they're left here for now. DRM_FORMAT_XBGR2101010 need to be
> > > included into IGT supported formats and DRM_FORMAT_C8 test need
> > > to be moved elsewhere, maybe into kms_plane.
> >
> > Thanks for doing this.
> >
> > >
> > > Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
> > > ---
> > > tests/kms_available_modes_crc.c | 217 +++++++++++++++--------
> > > -------
> > > ----------
> > > 1 file changed, 83 insertions(+), 134 deletions(-)
> > >
> > > diff --git a/tests/kms_available_modes_crc.c
> > > b/tests/kms_available_modes_crc.c
> > > index 7ff385f..a51006b 100644
> > > --- a/tests/kms_available_modes_crc.c
> > > +++ b/tests/kms_available_modes_crc.c
> > > @@ -101,17 +101,17 @@ static void
> > > generate_comparison_crc_list(data_t
> > > *data, igt_output_t *output)
> > > igt_plane_set_fb(primary, &data->primary_fb);
> > > igt_display_commit2(&data->display, data->commit);
> > >
> > > + igt_pipe_crc_drain(data->pipe_crc);
> > > igt_pipe_crc_get_current(data->gfx_fd, data->pipe_crc,
> > > &data-
> > > > cursor_crc);
> > >
> > > igt_plane_set_fb(primary, NULL);
> > > igt_display_commit2(&data->display, data->commit);
> > >
> > > - intel_gen(intel_get_drm_devid(data->gfx_fd)) < 9 ?
> > > - igt_paint_color(cr, 0, 0, mode->hdisplay, mode-
> > > > vdisplay, 1.0, 1.0, 1.0) :
> > >
> > > - igt_paint_color_alpha(cr, 0, 0, mode->hdisplay, mode-
> > > > vdisplay, 1.0, 1.0, 1.0, 1.0);
> > >
> > > + igt_paint_color(cr, 0, 0, mode->hdisplay, mode->vdisplay, 1.0,
> > > 1.0, 1.0);
> > >
> > > igt_plane_set_fb(primary, &data->primary_fb);
> > > igt_display_commit2(&data->display, data->commit);
> > >
> > > + igt_pipe_crc_drain(data->pipe_crc);
> > > igt_pipe_crc_get_current(data->gfx_fd, data->pipe_crc,
> > > &data-
> > > > fullscreen_crc);
> > >
> > >
> > > cairo_destroy(cr);
> > > @@ -122,48 +122,11 @@ static const struct {
> > > uint32_t fourcc;
> > > char zeropadding;
> > > enum { BYTES_PP_1=1,
> > > - BYTES_PP_2=2,
> > > - BYTES_PP_4=4,
> > > - NV12,
> > > - P010,
> > > - SKIP4 } bpp;
> > > + BYTES_PP_4=4} 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},
> > > -
> > > - /*
> > > - * (semi-)planar formats
> > > - */
> > > - { DRM_FORMAT_NV12, 0, NV12, 0x80eb},
> > > -#ifdef DRM_FORMAT_P010
> > > - { DRM_FORMAT_P010, 0, P010, 0x8000eb00},
> > > -#endif
> > > -#ifdef DRM_FORMAT_P012
> > > - { DRM_FORMAT_P012, 0, P010, 0x8000eb00},
> > > -#endif
> > > -#ifdef DRM_FORMAT_P016
> > > - { DRM_FORMAT_P016, 0, P010, 0x8000eb00},
> > > -#endif
> > > { 0, 0, 0, 0 }
> > > };
> > >
> > > @@ -175,10 +138,9 @@ static bool fill_in_fb(data_t *data,
> > > igt_output_t *output, igt_plane_t *plane,
> > > uint32_t format)
> > > {
> > > signed i, c, writesize;
> > > - unsigned short* ptemp_16_buf;
> > > unsigned int* ptemp_32_buf;
> > >
> > > - for( i = 0; fillers[i].fourcc != 0; i++ ) {
> > > + for( i = 0; i < ARRAY_SIZE(fillers)-1; i++ ) {
> > > if( fillers[i].fourcc == format )
> > > break;
> > > }
> > > @@ -190,53 +152,10 @@ static bool fill_in_fb(data_t *data,
> > > igt_output_t *output, igt_plane_t *plane,
> > > ptemp_32_buf[c] = fillers[i].value;
> > > writesize = data->size;
> > > break;
> > > - case BYTES_PP_2:
> > > - ptemp_16_buf = (unsigned short*)data->buf;
> > > - for (c = 0; c < data->size/2; c++)
> > > - ptemp_16_buf[c] = (unsigned
> > > short)fillers[i].value;
> > > - writesize = data->size;
> > > - break;
> > > case BYTES_PP_1:
> > > memset((void *)data->buf, fillers[i].value,
> > > data-
> > > > size);
> > >
> > > writesize = data->size;
> > > break;
> > > - case NV12:
> > > - memset((void *)data->buf, fillers[i].value&0xff,
> > > - data->fb.offsets[1]);
> > > -
> > > - memset((void *)(data->buf+data->fb.offsets[1]),
> > > - (fillers[i].value>>8)&0xff,
> > > - data->size - data->fb.offsets[1]);
> > > -
> > > - writesize = data->size;
> > > - break;
> > > - case P010:
> > > - ptemp_16_buf = (unsigned short*)data->buf;
> > > - for (c = 0; c < data->size/2; c++)
> > > - ptemp_16_buf[c] = (unsigned
> > > short)fillers[i].value&0xffff;
> > > -
> > > - ptemp_16_buf = (unsigned short*)(data->buf+data->size);
> > > - for (c = 0; c < data->size/2; c++)
> > > - ptemp_16_buf[c] = (unsigned
> > > short)(fillers[i].value>>16)&0xffff;
> > > -
> > > - 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);
> > > @@ -271,26 +190,20 @@ static bool setup_fb(data_t *data,
> > > igt_output_t
> > > *output, igt_plane_t *plane,
> > > tiling = LOCAL_DRM_FORMAT_MOD_NONE;
> > > }
> > >
> > > - for (i = 0; fillers[i].fourcc != 0; i++) {
> > > - if (fillers[i].fourcc == format)
> > > + for( i = 0; i < ARRAY_SIZE(fillers)-1; i++ ) {
> > > + if( fillers[i].fourcc == format )
> > > break;
> > > }
> > >
> > > switch (fillers[i].bpp) {
> > > - case NV12:
> > > case BYTES_PP_1:
> > > bpp = 8;
> > > break;
> > > -
> > > - case P010:
> > > - case BYTES_PP_2:
> > > - bpp = 16;
> > > - break;
> > > -
> > > - case SKIP4:
> > > case BYTES_PP_4:
> > > bpp = 32;
> > > break;
> > > + default:
> > > + return false;
> >
> > Hmm, we should never hit this condition. Mark this as a failure so
> > that
> > the test gets fixed?
>
> Yes, this is good place for assert.
>
> >
> > > }
> > >
> > > igt_get_fb_tile_size(data->gfx_fd, tiling, bpp,
> > > @@ -299,17 +212,6 @@ static bool setup_fb(data_t *data,
> > > igt_output_t
> > > *output, igt_plane_t *plane,
> > > data->fb.strides[0] = ALIGN(w * bpp / 8, tile_width);
> > > gemsize = data->size = data->fb.strides[0] * ALIGN(h,
> > > tile_height);
> > >
> > > - if (fillers[i].bpp == P010 || fillers[i].bpp == NV12) {
> > > - data->fb.offsets[1] = data->size;
> > > - data->fb.strides[1] = data->fb.strides[0];
> > > - gemsize = data->size * 2;
> > > -
> > > - if (fillers[i].bpp == NV12)
> > > - data->size += data->fb.strides[1] * ALIGN(h/2,
> > > tile_height);
> > > -
> > > - num_planes = 2;
> > > - }
> > > -
> > > data->gem_handle = gem_create(data->gfx_fd, gemsize);
> > > ret = __gem_set_tiling(data->gfx_fd, data->gem_handle,
> > > igt_fb_mod_to_tiling(tiling),
> > > @@ -386,12 +288,23 @@ 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)
> > > + int mode, enum pipe pipe)
> >
> > s/mode/format
> > Comment applies to other places too.
>
> While I agree I'm so-so about doing this change since it will follow
> into massive patch for test which I'm anyway wanting to throw away
> in
> the end.
Okay.
>
> >
> >
> > > {
> > > igt_crc_t current_crc;
> > > signed rVal = 0;
> > > bool do_crc;
> > > - char* crccompare[2];
> > > + int i;
> > > +
> > > + /*
> > > + * Limit tests only to those fb formats listed in fillers table
> > > + */
> > > + for( i = 0; i < ARRAY_SIZE(fillers)-1; i++ ) {
> > > + if( fillers[i].fourcc == mode )
> > > + break;
> > > + }
> > > +
> > > + if(fillers[i].bpp == 0)
> > > + return false;
s/return false/return 0 since the function returns 'number' of crc
failures.
> >
> > .bpp is always valid for the two formats that are tested, isn't it?
> > In
> > that case .bpp == 0 is a test failure.
> >
>
> No, here will arrive query for all fb formats which kernel talk
> about.
> If bpp become zero this will go out from testing mode which is not
> supported. Ie. C8 or XBGR2101010 will go forward and here other
> formats
> are ditched.
You are right, my brain automatically filled in a
if (i == ARRAY_SIZE(fillers) - 1)
return 0;
I feel checking for the loop index to have reached the limit seems more
natural since only two valid formats are tested. Also, we don't really
need the { 0, 0, 0, 0 } element in the fillers array anymore.
>
> > >
> > > if (prepare_crtc(data, output, plane, mode)){
> > > /*
> > > @@ -412,29 +325,30 @@ test_one_mode(data_t* data, igt_output_t
> > > *output, igt_plane_t* plane,
> > > 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]);
> > > + igt_warn("crc mismatch.
> > > connector %s using pipe %s" \
> > > + " plane index %d mode
> > > %.4s\n",
> > > + igt_output_name(output)
> > > ,
> > > + kmstest_pipe_name(pipe)
> > > ,
> > > + plane->index,
> > > + (char*)&mode);
> > > 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]);
> > > + igt_warn("crc mismatch.
> > > connector %s using pipe %s" \
> > > + " plane index %d mode
> > > %.4s\n",
> > > + igt_output_name(output)
> > > ,
> > > + kmstest_pipe_name(pipe)
> > > ,
> > > + plane->index,
> > > + (char*)&mode);
> > > rVal++;
> > > }
> > > }
> > > }
> > > - remove_fb(data, output, plane);
> > > - return rVal;
> > > }
> > > - return 1;
> > > + remove_fb(data, output, plane);
> > > + return rVal;
> > > }
> > >
> > >
> > > @@ -445,14 +359,44 @@ test_available_modes(data_t* data)
> > > igt_plane_t *plane;
> > > int modeindex;
> > > enum pipe pipe;
> > > - int invalids = 0;
> > > + int invalids = 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 = NULL;
> > >
> > > for_each_pipe_with_valid_output(&data->display, pipe,
> > > output) {
> > > 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)) & 0x8000;
> > > + lut[i].green = (i * 0xffff / (lut_size
> > > - 1)) & 0x8000;
> > > + lut[i].blue = (i * 0xffff / (lut_size -
> > > 1)) & 0x8000;
> > > + }
> > > +
> > > + igt_pipe_replace_prop_blob(&data->display,
> > > pipe,
> > > + IGT_CRTC_GAMMA_LUT,
> > > + lut, sizeof(*lut) *
> > > lut_size);
> > > + igt_display_commit2(&data->display, data-
> > > > commit);
> > >
> > > +
> > > + for (i = 0; i < lut_size; i++) {
> > > + lut[i].red = i * 0xffff / (lut_size -
> > > 1);
> > > + lut[i].green = i * 0xffff / (lut_size -
> > > 1);
> > > + lut[i].blue = i * 0xffff / (lut_size -
> > > 1);
> > > + }
> > > + }
> > > +
> > > data->pipe_crc = igt_pipe_crc_new(data->gfx_fd,
> > > pipe,
> > > INTEL_PIPE_CR
> > > C_SOURCE
> > > _AUTO);
> > >
> > > @@ -467,29 +411,34 @@ test_available_modes(data_t* data)
> > > modePlane = drmModeGetPlane(data-
> > > >gfx_fd,
> > > plane-
> > > >drm_plane-
> > > > plane_id);
> > >
> > >
> > > + if (plane->type == DRM_PLANE_TYPE_CURSOR)
> > > + continue;
> > > +
> > > for (modeindex = 0;
> > > modeindex < modePlane-
> > > >count_formats;
> > > 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);
> >
> > Not sure why this was removed, perhaps convert it to igt_debug()?
> > It's
> > hard to tell what is being tested now even with the --debug option.
>
> I did consider this as too verbose. Above where crcs are compared
> there's printed what failed with more detail than here.
>
> >
> > If you want to address the comments in a separate patch, feel free
> > to
> > add
> > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
>
> I'm going to have few CI runs with evolving this patch still.
>
> I found timing error on this test, it is most likely what is causing
> the
> noise from this test in CI runs. I somehow never see it on any of my
> own
> computers but realized it from the logic and first attempt to
> mitigate
> it changed CI results for the better so I'll get that fixed.
>
> /Juha-Pekka
>
> >
> >
> > > -
> > > invalids += test_one_mode(data,
> > > output,
> > > plane
> > > ,
> > > - modePlane-
> > > > formats[modeindex]);
> > >
> > > + modePlane-
> > > > formats[modeindex],
> > >
> > > + pipe);
> > > }
> > > drmModeFreePlane(modePlane);
> > > }
> > > igt_pipe_crc_stop(data->pipe_crc);
> > > igt_pipe_crc_free(data->pipe_crc);
> > > - igt_display_commit2(&data->display, data->commit);
> > > +
> > > + if (lut != NULL) {
> > > + igt_pipe_replace_prop_blob(&data->display,
> > > pipe,
> > > + IGT_CRTC_GAMMA_LUT,
> > > + lut, sizeof(*lut) *
> > > lut_size);
> > > + free(lut);
> > > + lut = NULL;
> > > + }
> > > +
> > > igt_output_set_pipe(output, PIPE_NONE);
> > > + igt_display_commit2(&data->display, data->commit);
> > > }
> > > igt_assert(invalids == 0);
> > > }
>
>
More information about the igt-dev
mailing list