Re: [PATCH 2/2] kms_rotation_crc: Adding test for 90/270 rotation

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 04/22/2015 05:51 AM, Jindal, Sonika wrote:
> > 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@xxxxxxxxx>
> > > ---
> > >   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.

I think they should go since, if you look, then don't belong to data at 
all, no code takes data and uses them from there but only the negative 
tests uses them only locally. So I think local igt_fb in 
test_unsupported_tiling_pixel_format would be more appropriate.

> > >       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.

Yes I get what it does :) my point was that it uses the second rotation 
to decide whether to paint a rectangular (full fb) or square (sub-fb) 
image and that it would be more obvious if the parameter said so.

> > > +        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.

To me that's marginal since I think they could be identical with just 
one if/else when it checks what to assert for. But never mind.

Bigger issue I have is negative tests fail on my box. :)

Regards,

Tvrtko



_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx