[Intel-gfx] [PATCH 2/2] kms_rotation_crc: Adding test for 90/270 rotation
Jindal, Sonika
sonika.jindal at intel.com
Tue Apr 21 21:51:58 PDT 2015
On 4/21/2015 5:24 PM, Tvrtko Ursulin wrote:
>
> Hi,
>
> On 04/07/2015 09:29 AM, Sonika Jindal wrote:
>> Adding 90/270 rotation testcase for primary and sprite planes.
>>
>> v2: Added position test for sprite. Checking for gen > 9 for 90/270.
>> Some cleanup and rebase.
>> v3: Added test for unsupported tiling and unsupported pixel format for
>> 90/270
>>
>> Signed-off-by: Sonika Jindal <sonika.jindal at intel.com>
>> ---
>> tests/kms_rotation_crc.c | 248
>> ++++++++++++++++++++++++++++++++++++++++------
>> 1 file changed, 215 insertions(+), 33 deletions(-)
>>
>> diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
>> index e11d7a9..48afaa1 100644
>> --- a/tests/kms_rotation_crc.c
>> +++ b/tests/kms_rotation_crc.c
>> @@ -28,14 +28,21 @@
>> #include "igt_debugfs.h"
>> #include "igt_kms.h"
>> #include "igt_core.h"
>> +#include "intel_chipset.h"
>>
>> typedef struct {
>> int gfx_fd;
>> igt_display_t display;
>> struct igt_fb fb;
>> struct igt_fb fb_cursor;
>> + struct igt_fb fb_full;
>> + struct igt_fb fb_565;
>> + struct igt_fb fb_tiling;
>
> Could you do with just fb and not having fb_565 and fb_tiling since I
> think you only use one at a time?
>
Yes I can, but then I'l have to create and remove fb during the test.
Since this is just one negative test (for 565 format and tiling) I
wanted to add, I thought it will look better this way.
>> igt_crc_t ref_crc;
>> igt_pipe_crc_t *pipe_crc;
>> + igt_rotation_t rotation;
>> + int pos_x;
>> + int pos_y;
>> } data_t;
>>
>> static void
>> @@ -63,18 +70,46 @@ paint_squares(data_t *data, struct igt_fb *fb,
>> drmModeModeInfo *mode,
>> w = mode->hdisplay;
>> h = mode->vdisplay;
>>
>> - cr = igt_get_cairo_ctx(data->gfx_fd, &data->fb);
>> + cr = igt_get_cairo_ctx(data->gfx_fd, fb);
>>
>> if (rotation == IGT_ROTATION_180) {
>> cairo_translate(cr, w, h);
>> cairo_rotate(cr, M_PI);
>> }
>>
>> - /* Paint with 4 squares of Red, Green, White, Blue Clockwise */
>> - igt_paint_color(cr, 0, 0, w / 2, h / 2, 1.0, 0.0, 0.0);
>> - igt_paint_color(cr, w / 2, 0, w / 2, h / 2, 0.0, 1.0, 0.0);
>> - igt_paint_color(cr, 0, h / 2, w / 2, h / 2, 0.0, 0.0, 1.0);
>> - igt_paint_color(cr, w / 2, h / 2, w / 2, h / 2, 1.0, 1.0, 1.0);
>> + /*
>> + * "rotation" is used for creating ref rotated fb and
>> + * "data->rotation" is used to determine the required size
>> + * while creating unrotated fb.
>> + */
>
> This comment and the setup below is not the most straightforward to
> understand.
>
> If I got it right you want rotation to draw a rotated image, and
> effectively a flag to say whether the drawn image is to be over full fb
> or in a square.
>
> Then what about having something like a "square_image" boolean flag
> instead of two rotation parameters?
data->rotation has the rotation value which we want our test to rotate
the fb with.
Now paint_squares gets called two times in a test
* First to paint the rotated reference fb to collect the sw rotated crc.
For this we set the appropriate rotation in the rotation parameter to
paint_squares. (Step 1 in the test)
* Next it is called to paint an unrotated fb for the HW to rotate it
using set_property. To draw unrotated fb, we pass IGT_ROTATION_0 in
rotation parameter to paint_squares.(Step 2 in the test).
Only for 90/270, I started using data->rotation inside this function.
Otherwise we were not using it.
This function is just to paint the fb based upon the rotation.
>
>> + if (rotation == IGT_ROTATION_90) {
>> + /* Paint 4 squares with width == height in Blue, Red,
>> + Green, White Clockwise order to look like 90 degree
>> rotated*/
>> + w = h = mode->vdisplay;
>> + igt_paint_color(cr, 0, 0, w / 2, h / 2, 0.0, 0.0, 1.0);
>> + igt_paint_color(cr, w / 2, 0, w / 2, h / 2, 1.0, 0.0, 0.0);
>> + igt_paint_color(cr, 0, h / 2, w / 2, h / 2, 1.0, 1.0, 1.0);
>> + igt_paint_color(cr, w / 2, h / 2, w / 2, h / 2, 0.0, 1.0,
>> 0.0);
>> +
>> + } else if (rotation == IGT_ROTATION_270) {
>> + /* Paint 4 squares with width == height in Green, White,
>> + Blue, Red Clockwise order to look like 270 degree rotated*/
>> + w = h = mode->vdisplay;
>> + igt_paint_color(cr, 0, 0, w / 2, h / 2, 0.0, 1.0, 0.0);
>> + igt_paint_color(cr, w / 2, 0, w / 2, h / 2, 1.0, 1.0, 1.0);
>> + igt_paint_color(cr, 0, h / 2, w / 2, h / 2, 1.0, 0.0, 0.0);
>> + igt_paint_color(cr, w / 2, h / 2, w / 2, h / 2, 0.0, 0.0,
>> 1.0);
>> +
>> + } else {
>> + if (data->rotation == IGT_ROTATION_90 ||
>> + data->rotation == IGT_ROTATION_270)
>> + w = h = mode->vdisplay;
>> + /* Paint with 4 squares of Red, Green, White, Blue
>> Clockwise */
>> + igt_paint_color(cr, 0, 0, w / 2, h / 2, 1.0, 0.0, 0.0);
>> + igt_paint_color(cr, w / 2, 0, w / 2, h / 2, 0.0, 1.0, 0.0);
>> + igt_paint_color(cr, 0, h / 2, w / 2, h / 2, 0.0, 0.0, 1.0);
>> + igt_paint_color(cr, w / 2, h / 2, w / 2, h / 2, 1.0, 1.0,
>> 1.0);
>> + }
>> }
>> cairo_destroy(cr);
>> }
>> @@ -84,7 +119,12 @@ static void prepare_crtc(data_t *data,
>> igt_output_t *output, enum pipe pipe,
>> {
>> drmModeModeInfo *mode;
>> igt_display_t *display = &data->display;
>> - int fb_id, fb_cursor_id;
>> + int fb_id, fb_cursor_id, fb_full_id;
>> + int w, h;
>> + uint64_t tiling = LOCAL_DRM_FORMAT_MOD_NONE;
>> + enum igt_commit_style commit = COMMIT_LEGACY;
>> + int old_rotation;
>> + igt_plane_t *primary;
>>
>> igt_output_set_pipe(output, pipe);
>>
>> @@ -94,10 +134,45 @@ static void prepare_crtc(data_t *data,
>> igt_output_t *output, enum pipe pipe,
>>
>> mode = igt_output_get_mode(output);
>>
>> + w = mode->hdisplay;
>> + h = mode->vdisplay;
>> +
>> + fb_full_id = igt_create_fb(data->gfx_fd,
>> + w, h,
>> + DRM_FORMAT_XRGB8888,
>> + tiling,
>> + &data->fb_full);
>> + igt_assert(fb_full_id);
>> +
>> + /*
>> + * With igt_display_commit2 and COMMIT_UNIVERSAL, we call just the
>> + * setplane without a modeset. So, to be able to call
>> + * igt_display_commit and ultimately setcrtc to do the first
>> modeset,
>> + * we create an fb covering the crtc and call commit
>> + */
>> +
>> + old_rotation = data->rotation;
>> + data->rotation = IGT_ROTATION_0;
>> + primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
>> + paint_squares(data, &data->fb_full, mode, IGT_ROTATION_0, primary);
>> + igt_plane_set_fb(primary, &data->fb_full);
>> + igt_display_commit(display);
>> + data->rotation = old_rotation;
>> +
>> + /*
>> + * For 90/270, we will use create smaller fb so that the rotated
>> + * frame can fit in
>> + */
>> + if (data->rotation == IGT_ROTATION_90 ||
>> + data->rotation == IGT_ROTATION_270) {
>> + tiling = LOCAL_I915_FORMAT_MOD_Y_TILED;
>> + w = h = mode->vdisplay;
>> + }
>> +
>> fb_id = igt_create_fb(data->gfx_fd,
>> - mode->hdisplay, mode->vdisplay,
>> + w, h,
>> DRM_FORMAT_XRGB8888,
>> - LOCAL_DRM_FORMAT_MOD_NONE,
>> + tiling,
>> &data->fb);
>> igt_assert(fb_id);
>>
>> @@ -110,27 +185,20 @@ static void prepare_crtc(data_t *data,
>> igt_output_t *output, enum pipe pipe,
>>
>> /* Step 1: create a reference CRC for a software-rotated fb */
>>
>> - /*
>> - * XXX: We always set the primary plane to actually enable the
>> pipe as
>> - * there's no way (that works) to light up a pipe with only a sprite
>> - * plane enabled at the moment.
>> - */
>> - if (!plane->is_primary) {
>> - igt_plane_t *primary;
>> -
>> - primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
>> - paint_squares(data, &data->fb, mode, IGT_ROTATION_180, primary);
>> - igt_plane_set_fb(primary, &data->fb);
>> - }
>> -
>> if (plane->is_cursor) {
>> - paint_squares(data, &data->fb_cursor, mode, IGT_ROTATION_180,
>> plane);
>> + paint_squares(data, &data->fb_cursor, mode, data->rotation,
>> plane);
>> igt_plane_set_fb(plane, &data->fb_cursor);
>> } else {
>> - paint_squares(data, &data->fb, mode, IGT_ROTATION_180, plane);
>> + paint_squares(data, &data->fb, mode, data->rotation, plane);
>> igt_plane_set_fb(plane, &data->fb);
>> + igt_plane_set_position(plane, data->pos_x, data->pos_y);
>> }
>> - igt_display_commit(display);
>> + if (plane->is_primary || plane->is_cursor) {
>> + igt_require(data->display.has_universal_planes);
>> + commit = COMMIT_UNIVERSAL;
>> + }
>> +
>> + igt_display_commit2(display, commit);
>> igt_pipe_crc_collect_crc(data->pipe_crc, &data->ref_crc);
>>
>> /*
>> @@ -155,6 +223,7 @@ static void cleanup_crtc(data_t *data,
>> igt_output_t *output, igt_plane_t *plane)
>>
>> igt_remove_fb(data->gfx_fd, &data->fb);
>> igt_remove_fb(data->gfx_fd, &data->fb_cursor);
>> + igt_remove_fb(data->gfx_fd, &data->fb_full);
>>
>> /* XXX: see the note in prepare_crtc() */
>> if (!plane->is_primary) {
>> @@ -170,6 +239,69 @@ static void cleanup_crtc(data_t *data,
>> igt_output_t *output, igt_plane_t *plane)
>> igt_display_commit(display);
>> }
>>
>> +static void test_unsupported_tiling_pixel_format(data_t *data, enum
>> igt_plane plane_type)
>> +{
>
> If the function body here is essentially the same as
> test_plane_rotation, could you avoid adding duplicating code by passing
> in tiling and pixel format to test_plane_rotation (maybe via data)? With
> a "expect fail" flag, or even just embedding the knowledge would work.
>
They are not same. We don't need few things from test_plane_rotation in
this one like prepare_crtc. So, instead of using if/else at multiple
places, I find it cleaner to have a different function for negative tests.
Regards,
Sonika
> Regards,
>
> Tvrtko
More information about the Intel-gfx
mailing list