> -----Original Message----- > From: Daniel Stone [mailto:daniel@xxxxxxxxxxxxx] > Sent: Tuesday, November 15, 2016 3:20 PM > To: Kahola, Mika <mika.kahola@xxxxxxxxx> > Cc: intel-gfx <intel-gfx@xxxxxxxxxxxxxxxxxxxxx> > Subject: Re: [PATCH i-g-t] tests/kms_plane_lowres: Plane visibility > after atomic modesets > > Hi Mika, > > On 15 November 2016 at 12:38, Mika Kahola <mika.kahola@xxxxxxxxx> wrote: > > Testcase for plane visibility after atomic modesets. The idea of the > > test is the following: > > > > - draw a blue screen with high resolution > > - enable a yellow plane, visible, in lower-left corner > > - set a new lower resolution mode (1024x768) that makes plane > > invisible > > - check from debugfs 'i915_display_info' that the plane is invisible > > - switch back to higher resolution mode > > - check from debugfs 'i915_display_info' that the plane is visible > > again > > - repeat number of iterations, default 64 > > It would be nice to have this for non-Intel drivers as well, with these > suggestions: Thanks for the review and feedback! I think we can modify this to suite non-Intel drivers as well. > > > +static inline uint32_t pipe_select(int pipe) { > > + if (pipe > 1) > > + return pipe << DRM_VBLANK_HIGH_CRTC_SHIFT; > > + else if (pipe > 0) > > + return DRM_VBLANK_SECONDARY; > > + else > > + return 0; > > +} > > + > > +static unsigned int get_vblank(int fd, int pipe, unsigned int flags) > > +{ > > + union drm_wait_vblank vbl; > > + > > + memset(&vbl, 0, sizeof(vbl)); > > + vbl.request.type = DRM_VBLANK_RELATIVE | pipe_select(pipe) | flags; > > + if (drmIoctl(fd, DRM_IOCTL_WAIT_VBLANK, &vbl)) > > + return 0; > > + > > + return vbl.reply.sequence; > > +} > > + > > +static int parse_resolution(res_t *resolution, char *info) { > > + char size[32]; > > + int ret; > > + > > + ret = sscanf(info + 4, "%d%*c %*s %*s %*s %s%*c", > > + &resolution->id, size); > > + > > + if (ret != 2) > > + return -1; > > + > > + ret = sscanf(size + 6, "%d%*c%d%*c", > > + &resolution->width, &resolution->height); > > + > > + if (ret != 2) > > + return -1; > > + > > + return ret + 1; > > +} > > Could all these be helpers? This crossed my mind too. I was thinking to do some cleanup after this one. For example the routine get_vblank() is called from other tests as well so it does make sense to move these as helpers. > > > +static bool get_visibility(FILE *fid, res_t *resolution) { > > + char tmp[256]; > > + res_t plane; > > + int ret; > > + > > + while (fgets(tmp, 256, fid) != NULL) { > > + if (strstr(tmp, "type=OVL") != NULL) { > > + ret = sscanf(tmp + 12, "%d%*c %*s %*s %d%*c%d%*c", > > + &plane.id, &plane.width, > > + &plane.height); > > + > > + igt_assert_eq(ret, 3); > > + > > + if (plane.width > resolution->width) > > + return false; > > + else if (plane.height > resolution->height) > > + return false; > > + else > > + return true; > > + } > > + } > > + > > + return false; > > +} > > + > > +static bool plane_visible(void) > > +{ > > + char tmp[256]; > > + FILE *fid; > > + bool visible = false; > > + res_t resolution; > > + int ret; > > + > > + fid = fopen("/sys/kernel/debug/dri/0/i915_display_info", > > + "r"); > > + > > + if (fid == NULL) > > + return false; > > + > > + while (fgets(tmp, 256, fid) != NULL) { > > + if (strstr(tmp, "active=yes") != NULL) { > > + ret = parse_resolution(&resolution, tmp); > > + igt_assert_eq(ret, 3); > > + visible = get_visibility(fid, &resolution); > > + } > > + } > > + > > + fclose(fid); > > + > > + return visible; > > +} > > Could this be made optional, determined from property state? The idea of the test case is to test plane visibility when switching resolution. Therefore, I wouldn't make this optional. > > > +static drmModeModeInfo * > > +test_setup(data_t *data, enum pipe pipe, uint64_t tiling, int flags, > > + igt_output_t *output) > > +{ > > + drmModeModeInfo *mode; > > + int x, y; > > + > > + igt_output_set_pipe(output, pipe); > > + > > + data->plane[0] = igt_output_get_plane(output, IGT_PLANE_PRIMARY); > > + data->plane[1] = igt_output_get_plane(output, IGT_PLANE_2); > > Could the test skip if these were unavailable? You're right, we need to test that these planes are available. > > > + mode = igt_output_get_mode(output); > > + > > + x = 0; > > + y = mode->vdisplay - SIZE; > > + > > + igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay, > > + DRM_FORMAT_XRGB8888, > > + LOCAL_DRM_FORMAT_MOD_NONE, > > + 0.0, 0.0, 1.0, > > + &data->fb[0]); > > + > > + igt_plane_set_fb(data->plane[0], &data->fb[0]); > > + > > + /* yellow sprite plane in lower left corner */ > > + igt_create_color_fb(data->drm_fd, > > + SIZE, SIZE, > > + DRM_FORMAT_XRGB8888, > > + tiling, > > s/tiling/modifier/ Ok > > > +static void > > +test_plane_position_with_output(data_t *data, enum pipe pipe, > > + igt_output_t *output, uint64_t tiling) > > +{ > > + igt_crc_t *crc_hires1, *crc_hires2; > > + igt_crc_t *crc_lowres; > > + drmModeModeInfo std_1024_mode = { > > + .clock = 65000, > > + .hdisplay = 1024, > > + .hsync_start = 1048, > > + .hsync_end = 1184, > > + .htotal = 1344, > > + .hskew = 0, > > + .vdisplay = 768, > > + .vsync_start = 771, > > + .vsync_end = 777, > > + .vtotal = 806, > > + .vscan = 0, > > + .vrefresh = 60, > > + .flags = 0xA, > > + .type = 0x40, > > + .name = "Custom 1024x768", > > + }; > > Could this mode only be used as a fallback, with the mode list first being scanned > to see if the output natively offers a mode small enough to pass the test? That is an option. I chose to use this one as this mode was applied with other kms test, see for example 'kms_frontbuffer_tracking.c'. It is true, that I should at least test that the original mode is big enough that with this mode the plane has theoretical possibility to be invisible. > > > + i = 0; > > + while (i < iterations || loop_forever) { > > + /* switch to lower resolution */ > > + igt_output_override_mode(output, &std_1024_mode); > > + igt_output_set_pipe(output, pipe); > > + > > + mode2 = igt_output_get_mode(output); > > + > > + igt_assert_eq(std_1024_mode.hdisplay, mode2->hdisplay); > > + igt_assert_eq(std_1024_mode.vdisplay, mode2->vdisplay); > > + igt_assert_eq(std_1024_mode.vrefresh, > > + mode2->vrefresh); > > + > > + display_commit_mode(data, pipe, flags, crc_lowres); > > Could the test skip if this fails? Either because the output does not support the > mode, or because some hardware does not allow planes to be marked as visible > but offscreen. Yes, test could be skipped if mode commit fails. Or would it be better if the test would fail in this case? > > > +static void > > +run_tests_for_pipe(data_t *data, enum pipe pipe) { > > + igt_subtest_f("pipe-%s-tiling-none", > > + kmstest_pipe_name(pipe)) > > + test_plane_position(data, pipe, > > +LOCAL_I915_FORMAT_MOD_X_TILED); > > + > > + igt_subtest_f("pipe-%s-tiling-x", > > + kmstest_pipe_name(pipe)) > > + test_plane_position(data, pipe, > > + LOCAL_I915_FORMAT_MOD_X_TILED); > > + > > + igt_subtest_f("pipe-%s-tiling-y", > > + kmstest_pipe_name(pipe)) > > + test_plane_position(data, pipe, > > + LOCAL_I915_FORMAT_MOD_Y_TILED); > > + > > + igt_subtest_f("pipe-%s-tiling-yf", > > + kmstest_pipe_name(pipe)) > > + test_plane_position(data, pipe, > > +LOCAL_I915_FORMAT_MOD_Yf_TILED); } > > Is there any reason to not test MOD_NONE as well? This is my bad. MODE_NONE should be tested. I will add this test when spinning another round of this. Cheers, Mika > > > + igt_fixture { > > + data.drm_fd = drm_open_driver_master(DRIVER_INTEL); > > With all these, you should at least be very close to making this DRIVER_ANY. > > Cheers, > Daniel _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
