[Intel-gfx] [I-G-T][PATCH] Add panning test for primary plane.
Damien Lespiau
damien.lespiau at intel.com
Mon Jul 7 19:04:13 CEST 2014
On Fri, May 23, 2014 at 08:28:47AM +0800, Yi Sun wrote:
> Get CRCs of a full red and a full blue surface as reference.
>
> Create a big framebuffer that is twice width and twice height as the
> current display mode.
>
> Fill the top left quarter with red, bottom right quarter with blue
> Check the scanned out image with the CRTC at position (0, 0) of the
> framebuffer and it should be the same CRC as the full red fb
> Check the scanned out image with the CRTC at position (hdisplay,
> vdisplay) and it should be the same CRC as the full blue fb
>
> Signed-off-by: Lei Liu <lei.a.liu at intel.com>
> Signed-off-by: Yi Sun <yi.sun at intel.com>
I've very sorry for the time it took to get this patch somewhere on top
of my TODO list. To redeem myself I've rebased it, fixed it up and
send the result to the mailing list.
I went ahead with only testing the primary plane at the moment. I think
instead of set_panning() it would make more sense to have a
igt_plane_set_source_rectangle() so it'd work better with the drm plane
API. That's for another time though.
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index fffad9f..5cdc48b 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -532,6 +532,13 @@ void igt_display_init(igt_display_t *display, int drm_fd)
> display->outputs = calloc(display->n_outputs, sizeof(igt_output_t));
> igt_assert(display->outputs);
>
> + /*
> + * Be default display can scan out from the original position of the frame buffer.
> + * But we can change this position making display scan out with a offset.
> + */
> + display->buffer_x = 0;
> + display->buffer_y = 0;
So, nop. the panning offset is a property of a plane (each plane can
have one), so that's something we need in the plane structure.
> for (i = 0; i < display->n_outputs; i++) {
> igt_output_t *output = &display->outputs[i];
>
> @@ -830,13 +837,13 @@ static int igt_output_commit(igt_output_t *output)
> igt_output_name(output),
> pipe_name(output->config.pipe),
> fb_id,
> - 0, 0,
> + display->buffer_x, display->buffer_y,
> mode->hdisplay, mode->vdisplay);
>
> ret = drmModeSetCrtc(display->drm_fd,
> crtc_id,
> fb_id,
> - 0, 0, /* x, y */
> + display->buffer_x, display->buffer_y, /* x, y */
> &output->id,
> 1,
> mode);
> @@ -849,7 +856,7 @@ static int igt_output_commit(igt_output_t *output)
> ret = drmModeSetCrtc(display->drm_fd,
> crtc_id,
> fb_id,
> - 0, 0, /* x, y */
> + display->buffer_x, display->buffer_y, /* x, y */
We are disabling the pipe here, setting the panning doesn't do anything
in that case.
> NULL, /* connectors */
> 0, /* n_connectors */
> NULL /* mode */);
> @@ -987,6 +994,26 @@ void igt_plane_set_fb(igt_plane_t *plane, struct igt_fb *fb)
> plane->fb_changed = true;
> }
>
> +void igt_plane_set_fb_offset(igt_plane_t *plane, struct igt_fb *fb, int x, int y)
> +{
This function foward declaration needs to be added to the header file.
> + igt_pipe_t *pipe = plane->pipe;
> + igt_display_t *display = pipe->display;
> +
> + (*display).buffer_x = x;
> + (*display).buffer_y = y;
Eek. display->buffer_x!
> +
> + LOG(display, "%c.%d: plane_set_fb(%d)\n", pipe_name(pipe->pipe),
> + plane->index, fb ? fb->fb_id : 0);
Of course, the logging message needs to be updated with set_panning(x, y)
> + plane->fb = fb;
This is against the orthogonality principle of the (any!) API, setting
the fb and setting the panning (x,y) are two different things, each one
with its own entry point.
> +
> + if (plane->is_primary)
> + pipe->need_set_crtc = true;
> + else if (plane->is_cursor)
> + pipe->need_set_cursor = true;
> +
> + plane->fb_changed = true;
For state tracking, we entirely switched to defining what has changed in
the setters (here, the panning (x,y)) and resolve which ioctl() needs to
be called in the commit function (because the idea is to be able to test
the different paths we have to drive the display (legacy, universal
planes + properties, atomic ioctl).
> +}
> void igt_plane_set_position(igt_plane_t *plane, int x, int y)
> {
> igt_pipe_t *pipe = plane->pipe;
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index 439a634..cb9370e 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -139,6 +139,8 @@ struct igt_display {
> int log_shift;
> int n_pipes;
> int n_outputs;
> + int buffer_x;
> + int buffer_y;
> unsigned long pipes_in_use;
> igt_output_t *outputs;
> igt_pipe_t pipes[I915_MAX_PIPES];
> diff --git a/tests/kms_plane.c b/tests/kms_plane.c
> index 6bdbca3..6fcdadb 100644
> --- a/tests/kms_plane.c
> +++ b/tests/kms_plane.c
> @@ -34,6 +34,12 @@
> #include "igt_kms.h"
>
> typedef struct {
> + float red;
> + float green;
> + float blue;
> +} colour_t;
While I do appeciate the british spelling, let's be a bit of a bore and
adapt the american one for type names.
> +typedef struct {
> int drm_fd;
> igt_display_t display;
> } data_t;
> @@ -55,12 +61,19 @@ typedef struct {
> igt_crc_t reference_crc;
> } test_position_t;
>
> +//colour_t colour;
You shouldn't leave this kind of things in patches you send for review.
> +enum {
> + TEST_POSITION_FULLY_COVERED = 1 << 0,
> + TEST_POSITION_BUFFER_ORIGINAL,
> +};
The 1 << 0 means that the original author intended for this enum to be
a bitfield. One should add a = 1 << 1 (in this case, this is strictly
equivalent, but it's good practice).
Also the TEST_POSITION prefix was there because it's a flag for the
position test. I split that in a separate enum for the panning one.
> +
> /*
> * create a green fb with a black rectangle at (rect_x,rect_y) and of size
> * (rect_w,rect_h)
> */
> static void
> -create_fb_for_mode__position(data_t *data, drmModeModeInfo *mode,
> +create_fb_for_mode_position(data_t *data, drmModeModeInfo *mode,
This wasn't a typo. I wanted it like this. Doesn't matter though.
> double rect_x, double rect_y,
> double rect_w, double rect_h,
> struct igt_fb *fb /* out */)
> @@ -84,13 +97,44 @@ create_fb_for_mode__position(data_t *data, drmModeModeInfo *mode,
> }
>
> static void
> -test_position_init(test_position_t *test, igt_output_t *output, enum pipe pipe)
> +create_fb_for_mode_panning(data_t *data, drmModeModeInfo *mode,
> + double rect_x, double rect_y,
> + double rect_w, double rect_h,
> + struct igt_fb *fb /* out */)
> +{
> + unsigned int fb_id;
> + cairo_t *cr;
> +
> + fb_id = igt_create_fb(data->drm_fd,
> + mode->hdisplay * 2, mode->vdisplay * 2,
> + DRM_FORMAT_XRGB8888,
> + false /* tiling */,
> + fb);
> + igt_assert(fb_id);
> +
> + cr = igt_get_cairo_ctx(data->drm_fd, fb);
> +
> + igt_paint_color(cr, 0, 0, mode->hdisplay, mode->vdisplay,
> + 1.0, 0.0, 0.0);
> +
> + igt_paint_color(cr, rect_x, rect_y, rect_w, rect_h,
> + 0.0, 0.0, 1.0);
rect_x, rect_y, rect_w and rect_h were there to define a rectangle in
the position test. With the panning test, we just need to the mode.
> +
> + igt_assert(cairo_status(cr) == 0);
> + cairo_destroy(cr);
> +}
> +
> +static void
> +test_position_init(test_position_t *test, igt_output_t *output, enum pipe pipe, colour_t colour_fb)
> {
* line > 80 chars
* structures are usually passed as references to avoid the whole copy on
the stack
* you are making test_position_init() more general and using it in the
panning test. This is a good sign it needs a rename (the purpose of
this function is to grab the reference CRC, so the name should reflect
that)
* Such a change is a good candate for a patch on its own.
> data_t *data = test->data;
> struct igt_fb green_fb;
> drmModeModeInfo *mode;
> igt_plane_t *primary;
>
> + data->display.buffer_x = 0;
> + data->display.buffer_y = 0;
If you have to temper with the internal states of the display object,
this means you got something wrong with your API.
> test->pipe_crc = igt_pipe_crc_new(pipe, INTEL_PIPE_CRC_SOURCE_AUTO);
>
> igt_output_set_pipe(output, pipe);
> @@ -100,7 +144,7 @@ test_position_init(test_position_t *test, igt_output_t *output, enum pipe pipe)
> igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> DRM_FORMAT_XRGB8888,
> false, /* tiled */
> - 0.0, 1.0, 0.0,
> + colour_fb.red, colour_fb.green, colour_fb.blue,
> &green_fb);
> igt_plane_set_fb(primary, &green_fb);
>
> @@ -115,6 +159,44 @@ test_position_init(test_position_t *test, igt_output_t *output, enum pipe pipe)
> }
>
> static void
> +test_panning_init(test_position_t *test, igt_output_t *output,
> + enum pipe pipe, unsigned int flags)
> +{
This whole function is unused, because you made test_position_init more
general I suppose. It's a lot better to make one function more general
than copy/pasting it and changing it slightly. So I just removed this
one.
> + data_t *data = test->data;
> + struct igt_fb panning_fb;
> + drmModeModeInfo *mode;
> + igt_plane_t *primary;
> + colour_t colour;
> +
> + data->display.buffer_x=0;
> + data->display.buffer_y=0;
> +
> + test->pipe_crc = igt_pipe_crc_new(pipe, INTEL_PIPE_CRC_SOURCE_AUTO);
> +
> + igt_output_set_pipe(output, pipe);
> + primary = igt_output_get_plane(output, 0);
> +
> + if (flags & TEST_POSITION_BUFFER_ORIGINAL)
> + colour = (colour_t){ .red = 1.0, .green = 0.0, .blue = 0.0 };
> + else
> + colour = (colour_t){ .red = 0.0, .green = 0.0, .blue = 1.0 };
> +
> + mode = igt_output_get_mode(output);
> + igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
> + DRM_FORMAT_XRGB8888,
> + false, /* tiled */
> + colour.red, colour.green, colour.blue,
> + &panning_fb);
> + igt_plane_set_fb(primary, &panning_fb);
> +
> + igt_display_commit(&data->display);
> +
> + igt_pipe_crc_collect_crc(test->pipe_crc, &test->reference_crc);
> +
> + igt_remove_fb(data->drm_fd, &panning_fb);
> +}
> +
> +static void
> test_position_fini(test_position_t *test, igt_output_t *output)
> {
> igt_pipe_crc_free(test->pipe_crc);
> @@ -123,10 +205,6 @@ test_position_fini(test_position_t *test, igt_output_t *output)
> igt_display_commit(&test->data->display);
> }
>
> -enum {
> - TEST_POSITION_FULLY_COVERED = 1 << 0,
> -};
> -
> static void
> test_plane_position_with_output(data_t *data,
> enum pipe pipe,
> @@ -139,18 +217,19 @@ test_plane_position_with_output(data_t *data,
> struct igt_fb primary_fb, sprite_fb;
> drmModeModeInfo *mode;
> igt_crc_t crc;
> + colour_t color_green = { .red = 0, .green = 1.0, .blue = 0.0 };
>
> fprintf(stdout, "Testing connector %s using pipe %c plane %d\n",
> igt_output_name(output), pipe_name(pipe), plane);
>
> - test_position_init(&test, output, pipe);
> + test_position_init(&test, output, pipe, color_green);
>
> mode = igt_output_get_mode(output);
> primary = igt_output_get_plane(output, 0);
> sprite = igt_output_get_plane(output, plane);
>
> - create_fb_for_mode__position(data, mode, 100, 100, 64, 64,
> - &primary_fb);
> + create_fb_for_mode_position(data, mode, 100, 100, 64, 64,
> + &primary_fb);
> igt_plane_set_fb(primary, &primary_fb);
>
> igt_create_color_fb(data->drm_fd,
> @@ -182,6 +261,59 @@ test_plane_position_with_output(data_t *data,
> }
>
> static void
> +test_plane_panning_with_output(data_t *data,
> + enum pipe pipe,
> + enum igt_plane plane,
> + igt_output_t *output,
> + unsigned int flags)
> +{
> + test_position_t test = { .data = data };
> + igt_plane_t *primary;
> + struct igt_fb primary_fb;
> + drmModeModeInfo *mode;
> + igt_crc_t crc;
> + colour_t colour;
> +
> + fprintf(stdout, "Testing connector %s using pipe %c plane %d\n",
> + igt_output_name(output), pipe_name(pipe), plane);
> +
> + if (flags & TEST_POSITION_BUFFER_ORIGINAL)
> + colour = (colour_t){ .red = 1.0, .green = 0.0, .blue = 0.0 };
> + else
> + colour = (colour_t){ .red = 0.0, .green = 0.0, .blue = 1.0 };
> +
> + test_position_init(&test, output, pipe, colour);
Right. So there's a split between data_t and test_position_t. The
intention was to be able to split the data that is common for all the
tests and per-test data. That means we need a test_panning_t.
Also, I documented what was the test_position doing, I think that's
helpful for people reading the tests, so we need one here as well.
> +
> + mode = igt_output_get_mode(output);
> + primary = igt_output_get_plane(output, 0);
> +
> + create_fb_for_mode_panning(data,
> + mode, mode->hdisplay, mode->vdisplay,
> + mode->hdisplay, mode->vdisplay,
> + &primary_fb);
> +
> + if (flags & TEST_POSITION_BUFFER_ORIGINAL)
> + igt_plane_set_fb_offset(primary, &primary_fb, 0, 0);
> + else
> + igt_plane_set_fb_offset(primary, &primary_fb, mode->hdisplay, mode->vdisplay);
> +
> + igt_plane_set_position(primary, 0, 0);
> +
> + igt_display_commit(&data->display);
> +
> + igt_pipe_crc_collect_crc(test.pipe_crc, &crc);
> +
> + igt_plane_set_fb(primary, NULL);
> +
> + test_position_fini(&test, output);
> +
> + if (flags & TEST_POSITION_BUFFER_ORIGINAL)
> + igt_assert(igt_crc_equal(&test.reference_crc, &crc));
> + else
> + igt_assert(igt_crc_equal(&test.reference_crc, &crc));
This doesn't look right (even if it is). You don't need an 'if' ...
'else' if the 2 branches are identical.
> +}
> +
> +static void
> test_plane_position(data_t *data, enum pipe pipe, enum igt_plane plane,
> unsigned int flags)
> {
> @@ -196,16 +328,40 @@ test_plane_position(data_t *data, enum pipe pipe, enum igt_plane plane,
> }
>
> static void
> +test_plane_panning(data_t *data, enum pipe pipe, enum igt_plane plane,
> + unsigned int flags)
> +{
> + igt_output_t *output;
> +
> + igt_skip_on(pipe >= data->display.n_pipes);
> + igt_skip_on(plane >= data->display.pipes[pipe].n_planes);
> +
> + for_each_connected_output(&data->display, output)
> + test_plane_panning_with_output(data, pipe, plane, output,
> + flags);
> +}
> +
> +static void
> run_tests_for_pipe_plane(data_t *data, enum pipe pipe, enum igt_plane plane)
> {
> igt_subtest_f("plane-position-covered-pipe-%c-plane-%d",
> - pipe_name(pipe), plane)
> + pipe_name(pipe), plane)
> test_plane_position(data, pipe, plane,
> - TEST_POSITION_FULLY_COVERED);
> + TEST_POSITION_FULLY_COVERED);
>
> igt_subtest_f("plane-position-hole-pipe-%c-plane-%d",
> - pipe_name(pipe), plane)
> + pipe_name(pipe), plane)
> test_plane_position(data, pipe, plane, 0);
For the 3 changes above, please don't re-indent unrelated lines.
> +
> + igt_subtest_f("plane-panning-buffer-position-original-%c-plane-%d",
> + pipe_name(pipe), plane)
> + test_plane_panning(data, pipe, plane,
> + TEST_POSITION_BUFFER_ORIGINAL);
> +
> + igt_subtest_f("plane-panning-set-position-pipe-%c-plane-%d",
> + pipe_name(pipe), plane)
> + test_plane_panning(data, pipe, plane, 0);
Panning is only supported for the primary plane and this function will
be called with plane number from 1 to IGT_MAX_PLANES - 1. So we call it
a number of times and do the same test. A bit useless right now, but
will be needed when we improve the test to support the other planes, so
I left it alone.
--
Damien
More information about the Intel-gfx
mailing list