[Intel-gfx] [PATCH i-g-t] tests/kms_plane_lowres: Plane visibility after atomic modesets
Kahola, Mika
mika.kahola at intel.com
Wed Nov 16 11:37:03 UTC 2016
> -----Original Message-----
> From: Daniel Stone [mailto:daniel at fooishbar.org]
> Sent: Tuesday, November 15, 2016 3:20 PM
> To: Kahola, Mika <mika.kahola at intel.com>
> Cc: intel-gfx <intel-gfx at lists.freedesktop.org>
> Subject: Re: [Intel-gfx] [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 at intel.com> 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
More information about the Intel-gfx
mailing list