Re: [igt-dev] [PATCH i-g-t] tests/kms_rotation_crc: Add multi plane tests.

["Juha-Pekka Heikkilä" <juhapekka.heikkila@gmail.com>]

Nautiyal, Ankit K kirjoitti 22.11.2018 klo 9.04:
> Hi Juha-Pekka,
> 
> Thanks for considering the earlier suggestions. The solution looks 
> pretty good, nicely avoids hard-coding of fixed width and height and 
> making it dependent on the resolution.
> 
> I agree with the test as a whole. However I think it requires a slight 
> change in case of 90/270 rotation for determining the x,y for right and 
> bottom rectangles.
> 
> The suggested change can avoid cropped rectangles, for few resolutions.
> 
> Also minute styling issues in a couple of places.
> 
> Please find my suggestions inline:
> 
> 
> On 11/19/2018 5:29 PM, Juha-Pekka Heikkila wrote:
>> Add three new tests which try primary and sprite planes
>> next to each other with different plane formats, rotations
>> and tiling modes.
>>
>> multiplane-rotation subtest run test through with both planes
>> fully visible.
>>
>> multiplane-rotation-cropping-top will crop primary plane to
>> left/top corner and sprite plane to right/top corner while running
>> rotation tests.
>>
>> multiplane-rotation-cropping-bottom will crop primary plane to
>> left/bottom corner and sprite plane to right/bottom corner while
>> running rotation tests.
>>
>> v2 (Nautiyal, Ankit K): Use rectangular figures instead of
>> square figures and fix associated code to perform correctly.
>> Adjust plane positions according to screen size to avoid
>> putting planes outside screen.
>>
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>> ---
>>   tests/kms_rotation_crc.c | 246 
>> +++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 246 insertions(+)
>>
>> diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
>> index 366c254..32814fe 100644
>> --- a/tests/kms_rotation_crc.c
>> +++ b/tests/kms_rotation_crc.c
>> @@ -26,6 +26,25 @@
>>   #include <math.h>
>>   #define MAX_FENCES 32
>> +#define MAXMULTIPLANESAMOUNT 2
>> +
>> +struct p_struct {
>> +    igt_plane_t *plane;
>> +    struct igt_fb fb;
>> +};
>> +
>> +enum p_pointorigo {
>> +    p_top = 1 << 0,
>> +    p_bottom = 1 << 1,
>> +    p_left = 1 << 2,
>> +    p_right = 1 << 3
>> +};
>> +
>> +struct p_point{
>> +    enum p_pointorigo origo;
>> +    float_t x;
>> +    float_t y;
>> +};
>>   typedef struct {
>>       int gfx_fd;
>> @@ -43,6 +62,9 @@ typedef struct {
>>       uint32_t override_fmt;
>>       uint64_t override_tiling;
>>       int devid;
>> +
>> +    struct p_struct *multiplaneoldview;
>> +    struct p_point planepos[MAXMULTIPLANESAMOUNT];
>>   } data_t;
>>   typedef struct {
>> @@ -395,6 +417,194 @@ static void test_plane_rotation(data_t *data, 
>> int plane_type, bool test_bad_form
>>       }
>>   }
>> +typedef struct {
>> +    int32_t x1, y1;
>> +    uint64_t width, height, tiling, planetype, format;
>> +    igt_rotation_t rotation_sw, rotation_hw;
>> +} planeinfos;
>> +
>> +static void get_multiplane_crc(data_t *data, igt_output_t *output,
>> +                   igt_crc_t *crc_output, planeinfos* planeinfo,
> 
> Space before '*'
> 
>> +                   int numplanes)
>> +{
>> +    uint32_t w, h;
>> +    igt_display_t *display = &data->display;
>> +    struct p_struct *planes, *oldplanes;
>> +    int c, ret;
>> +
>> +    oldplanes = data->multiplaneoldview;
>> +    planes = malloc(sizeof(*planes)*numplanes);
> 
> space around multiplication '*'
>> +
>> +    for (c = 0; c < numplanes; c++) {
>> +        planes[c].plane = igt_output_get_plane_type(output,
>> +                                planeinfo[c].planetype);
>> +
>> +        w = planeinfo[c].width;
>> +        h = planeinfo[c].height;
>> +
>> +        if (planeinfo[c].rotation_sw & (IGT_ROTATION_90 | 
>> IGT_ROTATION_270))
>> +            igt_swap(w, h);
>> +
> 
> There can be a minor problem of cropping for rectangles right side of 
> screen, in case of some squarish resolutions.
> We know for our multi-plane rotation test for Top Right rectangle:
> width = 0.2 hdisplay
> height = 0.4 vdisplay
> x1 = hdisplay - 0.2hdisplay - 0.1hdisplay (margin)= 0.7hdisplay
> y1 = vdisplay - 0.4vdisplay - 0.1vdisplay (margin) = 0.5vdisplay
> 
> 
> After rotation 90/270 width and height gets swapped.
> 
> Since x1 = 0.7 hdisplay, so maximum width of top-right rectangle without 
> cropping = 0.3hdisplay.
> So if the actual rectangle width after rotation of 90/270 is more than 
> the maximum available width, we'll get cropped rectangle.
> 
> Now due to swap, the width of the rectangle after rotation = 0.4vdisplay.
> We will always get cropped rectangle if:
>   0.4 vdisplay > 0.3hdisplay.
> 
> Or in other words we'll get cropped rectangle if hdisplay/vdisplay < 4/3.
> Now I am not sure whether we should even think about those resolutions 
> which have hdisplay:vdisplay < 4:3
> But to name a few 1280x1024,  720x576 resolution will get cropped 
> rectangle.

I was wondering if you have monitor which actually have such resolutions 
as those you mentioned? I was looking but didn't find any such. If you 
are thinking about rotating monitor it will not change the scanning 
direction and the resolution flip is done via rotation -- that feature 
which is under test here. I don't know if it's with modern day monitors 
still but earlier scanning order of display was dictated by the lowest 
of x-res vs y-res because it caused less components.

I anyway moved those planes a bit but so far I'm sticking to 4/3 being 
'worst case', I'll probably tomorrow send new version of the patch.

> 
> The problem can be solved, if we change the x1 and y1 in case of 
> rotation 90/270 before swapping.
> 
> if (planeinfo[c].rotation_sw & (IGT_ROTATION_90 | IGT_ROTATION_270)) {
> 
>      x1 = x1 + w - h;
>      y1 = y1 + h - w;
> 
>      igt_swap(w, h);
> }
> so x1 will become 0.7 hdisplay + 0.2hdisplay - 0.4 vdisplay = 0.9 
> hdisplay + 0.4 vdisplay [ a border/margin of 0.1 hdisplay from right side]
> and y1 will become 0.5 vdisplay +0.4 vdisplay - 0.2hdisplay = 0.9 
> vdisplay + 0.2 hdisplay [a border/margin of 0.1 vdisplay from bottom ]
> 
>> +        igt_create_fb(data->gfx_fd, w, h, planeinfo[c].format,
>> +                  planeinfo[c].tiling, &planes[c].fb);
>> +
>> +        paint_squares(data, planeinfo[c].rotation_sw, &planes[c].fb, 
>> 1.0f);
>> +        igt_plane_set_fb(planes[c].plane, &planes[c].fb);
>> +
>> +        if (planeinfo[c].rotation_hw & (IGT_ROTATION_90 | 
>> IGT_ROTATION_270))
>> +            igt_plane_set_size(planes[c].plane, h, w);
>> +
>> +        igt_plane_set_position(planes[c].plane, planeinfo[c].x1, 
>> planeinfo[c].y1);
>> +        igt_plane_set_rotation(planes[c].plane, 
>> planeinfo[c].rotation_hw);
>> +    }
>> +
>> +    ret = igt_display_try_commit2(display, COMMIT_ATOMIC);
>> +    igt_assert_eq(ret, 0);
>> +
>> +    igt_pipe_crc_get_current(data->gfx_fd, data->pipe_crc, crc_output);
>> +
>> +    for (c = 0; c < numplanes && oldplanes; c++)
>> +        igt_remove_fb(data->gfx_fd, &oldplanes[c].fb);
>> +
>> +    free(oldplanes);
>> +    data->multiplaneoldview = (void*)planes;
>> +}
>> +
>> +static void pointlocation(data_t *data, planeinfos* p, 
>> drmModeModeInfo *mode,
> 
> planeinfos *p
> 
>> +              int c)
>> +{
>> +    p[c].x1 = (int32_t)(data->planepos[c].x*mode->hdisplay)
>> +            +((data->planepos[c].origo & p_right) ? mode->hdisplay : 0);
>> +    p[c].y1 = (int32_t)(data->planepos[c].y*mode->vdisplay)
>> +            +((data->planepos[c].origo & p_bottom) ? mode->vdisplay : 
>> 0);
>> +}
>> +
> 
> Space around '*'
> 
>> +/*
>> + * Here is pipe parameter which is now used only for first pipe.
>> + * It is left here if this test ever was wanted to be run on
>> + * different pipes.
>> + */
>> +static void test_multi_plane_rotation(data_t *data, enum pipe pipe)
>> +{
>> +    igt_display_t *display = &data->display;
>> +    igt_output_t *output;
>> +    igt_crc_t retcrc_sw, retcrc_hw;
>> +    planeinfos p[2];
>> +    int c;
>> +    struct p_struct *oldplanes;
>> +    drmModeModeInfo *mode;
>> +
>> +    const static struct {
>> +        igt_rotation_t rotation;
>> +        float_t width;
>> +        float_t height;
>> +        uint64_t tiling;
>> +    } planeconfigs[] = {
>> +    {IGT_ROTATION_0, .2f, .4f, LOCAL_DRM_FORMAT_MOD_NONE },
>> +    {IGT_ROTATION_0, .2f, .4f, LOCAL_I915_FORMAT_MOD_X_TILED },
>> +    {IGT_ROTATION_0, .2f, .4f, LOCAL_I915_FORMAT_MOD_Y_TILED },
>> +    {IGT_ROTATION_0, .2f, .4f, LOCAL_I915_FORMAT_MOD_Yf_TILED },
>> +    {IGT_ROTATION_90, .2f, .4f, LOCAL_I915_FORMAT_MOD_Y_TILED },
>> +    {IGT_ROTATION_90, .2f, .4f, LOCAL_I915_FORMAT_MOD_Yf_TILED },
>> +    {IGT_ROTATION_180, .2f, .4f, LOCAL_DRM_FORMAT_MOD_NONE },
>> +    {IGT_ROTATION_180, .2f, .4f, LOCAL_I915_FORMAT_MOD_X_TILED },
>> +    {IGT_ROTATION_180, .2f, .4f, LOCAL_I915_FORMAT_MOD_Y_TILED },
>> +    {IGT_ROTATION_180, .2f, .4f, LOCAL_I915_FORMAT_MOD_Yf_TILED },
>> +    {IGT_ROTATION_270, .2f, .4f, LOCAL_I915_FORMAT_MOD_Y_TILED },
>> +    {IGT_ROTATION_270, .2f, .4f, LOCAL_I915_FORMAT_MOD_Yf_TILED },
>> +    };
>> +
>> +    /*
>> +    * These are those modes which are tested. For testing feel 
>> interesting
>> +    * case with tiling are 2 byte wide and 4 byte wide.
>> +    *
>> +    * TODO:
>> +    * Built support for NV12 here.
>> +    */
>> +    const static uint32_t  formatlist[] = {DRM_FORMAT_RGB565,
>> +                           DRM_FORMAT_XRGB8888};
>> +
>> +    for_each_valid_output_on_pipe(display, pipe, output) {
>> +        int i, j, k, l;
>> +        igt_output_set_pipe(output, pipe);
>> +        mode = igt_output_get_mode(output);
>> +        igt_display_require_output(display);
>> +        igt_display_commit2(display, COMMIT_ATOMIC);
>> +
>> +        data->pipe_crc = igt_pipe_crc_new(data->gfx_fd, pipe,
>> +                          INTEL_PIPE_CRC_SOURCE_AUTO);
>> +        igt_pipe_crc_start(data->pipe_crc);
>> +
>> +        for (i = 0; i < ARRAY_SIZE(planeconfigs); i++) {
>> +            p[0].planetype = DRM_PLANE_TYPE_PRIMARY;
>> +            p[0].width = 
>> (uint64_t)(planeconfigs[i].width*mode->hdisplay);
>> +            p[0].height = 
>> (uint64_t)(planeconfigs[i].height*mode->vdisplay);
> 
> Space around '*'
> 
>> +            p[0].tiling = planeconfigs[i].tiling;
>> +            pointlocation(data, (planeinfos*)&p, mode, 0);
> 
> Space before '*'
> 
>> +
>> +            for (k = 0; k < ARRAY_SIZE(formatlist); k++) {
>> +                p[0].format = formatlist[k];
>> +
>> +                for (j = 0; j < ARRAY_SIZE(planeconfigs); j++) {
>> +                    p[1].planetype = DRM_PLANE_TYPE_OVERLAY;
>> +                    p[1].width = 
>> (uint64_t)(planeconfigs[j].width*mode->hdisplay);
>> +                    p[1].height = 
>> (uint64_t)(planeconfigs[j].height*mode->vdisplay);
> 
> space around '*'
> 
>> +                    p[1].tiling = planeconfigs[j].tiling;
>> +                    pointlocation(data, (planeinfos*)&p,
>> +                              mode, 1);
>> +
> 
> Space before '*'
>> +                    for (l = 0; l < ARRAY_SIZE(formatlist); l++) {
>> +                        p[1].format = formatlist[l];
>> +
>> +                        /*
>> +                         * RGB565 90/270 degrees rotation is supported
>> +                         * from gen11 onwards.
>> +                         */
>> +                        if (p[0].format == DRM_FORMAT_RGB565 &&
>> +                             (planeconfigs[i].rotation & 
>> (IGT_ROTATION_90 | IGT_ROTATION_270))
>> +                             && intel_gen(data->devid) < 11)
>> +                            continue;
>> +
>> +                        if (p[1].format == DRM_FORMAT_RGB565 &&
>> +                             (planeconfigs[j].rotation & 
>> (IGT_ROTATION_90 | IGT_ROTATION_270))
>> +                             && intel_gen(data->devid) < 11)
>> +                            continue;
>> +
>> +                        p[0].rotation_sw = planeconfigs[i].rotation;
>> +                        p[0].rotation_hw = IGT_ROTATION_0;
>> +                        p[1].rotation_sw = planeconfigs[j].rotation;
>> +                        p[1].rotation_hw = IGT_ROTATION_0;
>> +                        get_multiplane_crc(data, output, &retcrc_sw,
>> +                                   (planeinfos*)&p, 
>> MAXMULTIPLANESAMOUNT);
> 
> Space before '*'
>> +
>> +                        igt_swap(p[0].rotation_sw, p[0].rotation_hw);
>> +                        igt_swap(p[1].rotation_sw, p[1].rotation_hw);
>> +                        get_multiplane_crc(data, output, &retcrc_hw,
>> +                                   (planeinfos*)&p, 
>> MAXMULTIPLANESAMOUNT);
>> +
> 
> Space before '*'
> 
>> +                        igt_assert_crc_equal(&retcrc_sw, &retcrc_hw);
>> +                    }
>> +                }
>> +            }
>> +        }
>> +        igt_pipe_crc_stop(data->pipe_crc);
>> +        igt_pipe_crc_free(data->pipe_crc);
>> +        igt_output_set_pipe(output, PIPE_ANY);
>> +    }
>> +
>> +    /*
>> +    * Old fbs are deleted only after new ones are set on planes.
>> +    * This is done to speed up the test
>> +    */
>> +    oldplanes = data->multiplaneoldview;
>> +    for (c = 0; c < MAXMULTIPLANESAMOUNT && oldplanes; c++)
>> +        igt_remove_fb(data->gfx_fd, &oldplanes[c].fb);
>> +
>> +    free(oldplanes);
>> +    data->multiplaneoldview = NULL;
>> +    data->pipe_crc = NULL;
>> +}
>> +
>>   static void test_plane_rotation_exhaust_fences(data_t *data,
>>                              enum pipe pipe,
>>                              igt_output_t *output,
>> @@ -595,6 +805,42 @@ igt_main
>>           }
>>       }
>> +    igt_subtest_f("multiplane-rotation") {
>> +        igt_require(gen >= 9);
>> +        cleanup_crtc(&data);
>> +        data.planepos[0].origo = p_top | p_left;
>> +        data.planepos[0].x = .1f;
>> +        data.planepos[0].y = .1f;
>> +        data.planepos[1].origo = p_top | p_right;
>> +        data.planepos[1].x = -.3f;
>> +        data.planepos[1].y = .1f;
>> +        test_multi_plane_rotation(&data, 0);
>> +    }
>> +
>> +    igt_subtest_f("multiplane-rotation-cropping-top") {
>> +        igt_require(gen >= 9);
>> +        cleanup_crtc(&data);
>> +        data.planepos[0].origo = p_top | p_left;
>> +        data.planepos[0].x = -.05f;
>> +        data.planepos[0].y = -.15f;
>> +        data.planepos[1].origo = p_top | p_right;
>> +        data.planepos[1].x = -.15f;
>> +        data.planepos[1].y = -.15f;
>> +        test_multi_plane_rotation(&data, 0);
>> +    }
>> +
>> +    igt_subtest_f("multiplane-rotation-cropping-bottom") {
>> +        igt_require(gen >= 9);
>> +        cleanup_crtc(&data);
>> +        data.planepos[0].origo = p_bottom | p_left;
>> +        data.planepos[0].x = -.05f;
>> +        data.planepos[0].y = -.20f;
>> +        data.planepos[1].origo = p_bottom | p_right;
>> +        data.planepos[1].x = -.15f;
>> +        data.planepos[1].y = -.20f;
>> +        test_multi_plane_rotation(&data, 0);
>> +    }
>> +
>>       /*
>>        * exhaust-fences should be last test, if it fails we may OOM in
>>        * the following subtests otherwise.
> 
> Sorry for lengthy details for a minor corner case, couldnt make it more 
> concise, and clear at the same time.
> 
> Regards,
> Ankit
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev