Re: [igt-dev] [PATCH i-g-t] tests/kms_scaling_modes: New IGT to validate scaling modes

["Sharma, Swati2" <swati2.sharma@intel.com>]

Thanks JP for the review. Addressed all the review comments
in rev2 https://patchwork.freedesktop.org/patch/470363/?series=98463&rev=3

On 12-Jan-22 12:23 AM, Juha-Pekka Heikkila wrote:
> Hi Swati,
> 
> I think this type scaling happen only on eDP with i915. What this mean 
> is if there's on Intel CI shards boxes some with eDP and others without 
> eDP there will be flip flopping with test skipping or running depending 
> which box test will get to run on. If shards machines in same hw family 
> are similar with respect to eDP then there's no issue on flip flopping 
> for that reason.
> 
> Some comments below
> 
> On 3.1.2022 18.19, Swati Sharma wrote:
>> In this IGT, various scaling modes are validated. Scaling
>> mode is one of the connector properties. This property
>> defines how a non-native mode is upscaled to the native
>> mode of an LCD panel.
>>
>> There are basically 4 types of scaling modes defined:
>>
>>     None:
>>         No upscaling happens, scaling is left to the panel. Not all
>>         drivers expose this mode.
>>     Full:
>>         The output is upscaled to the full resolution of the panel,
>>         ignoring the aspect ratio. It will expand current image to
>>         the size of the monitor.
>>     Center:
>>         No upscaling happens, the output is centered within the native
>>         resolution the panel. As a result, black bars may appear
>>         around the image.
>>     Full aspect:
>>         The output is upscaled to maximize either the width or height
>>         while retaining the aspect ratio. It will fill the screen w/o
>>         stretching the image. Black bars are placed either on top
>>         and bottom or left and right of the picture.
>>
>> Note: This IGT is build upon kms_panel_fitting
>>
>> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
>> ---
>>   tests/kms_scaling_modes.c | 167 ++++++++++++++++++++++++++++++++++++++
>>   tests/meson.build         |   1 +
>>   2 files changed, 168 insertions(+)
>>   create mode 100644 tests/kms_scaling_modes.c
>>
>> diff --git a/tests/kms_scaling_modes.c b/tests/kms_scaling_modes.c
>> new file mode 100644
>> index 00000000..d90783d0
>> --- /dev/null
>> +++ b/tests/kms_scaling_modes.c
>> @@ -0,0 +1,167 @@
>> +/*
>> + * Copyright © 2022 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person 
>> obtaining a
>> + * copy of this software and associated documentation files (the 
>> "Software"),
>> + * to deal in the Software without restriction, including without 
>> limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, 
>> sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including 
>> the next
>> + * paragraph) shall be included in all copies or substantial portions 
>> of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>> EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>> MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT 
>> SHALL
>> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, 
>> DAMAGES OR
>> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>> + *
>> + * Authors:
>> + *     Swati Sharma <swati2.sharma@intel.com>
>> + *
>> + */
>> +
>> +#include "igt.h"
>> +#include <fcntl.h>
>> +#include <termios.h>
>> +#include <unistd.h>
>> +
>> +IGT_TEST_DESCRIPTION("Test display scaling modes");
>> +
>> +/* Test flags */
>> +enum {
>> +    TEST_NONE = 1 << 0,
>> +    TEST_FULL = 1 << 1,
>> +    TEST_CENTER = 1 << 2,
>> +    TEST_FULLASPECT = 1 << 3,
>> +};
> 
> I didn't notice this enum doing anything else than code DRM_MODE_SCALE_* 
> to different numbers which will be translated in test_cycle_flags(..), 
> is this translation layer needed? Quickly looking it seem when calling 
> test_scaling_mode(..) you could give DRM_MODE_SCALE_* as the flag 
> directly and in place of calling test_cycle_flags(..) you'd just call 
> igt_output_set_prop_value(..)
> 
>> +
>> +/* Common test data */
>> +typedef struct data {
>> +    igt_display_t display;
>> +    int drm_fd;
>> +} data_t;
>> +
>> +static void test_cycle_flags(igt_output_t *output, uint32_t test_flags)
>> +{
>> +    if (test_flags & TEST_NONE)
>> +        igt_output_set_prop_value(output, IGT_CONNECTOR_SCALING_MODE, 
>> DRM_MODE_SCALE_NONE);
>> +    if (test_flags & TEST_FULL)
>> +        igt_output_set_prop_value(output, IGT_CONNECTOR_SCALING_MODE, 
>> DRM_MODE_SCALE_FULLSCREEN);
>> +    if (test_flags & TEST_CENTER)
>> +        igt_output_set_prop_value(output, IGT_CONNECTOR_SCALING_MODE, 
>> DRM_MODE_SCALE_CENTER);
>> +    if (test_flags & TEST_FULLASPECT)
>> +        igt_output_set_prop_value(output, IGT_CONNECTOR_SCALING_MODE, 
>> DRM_MODE_SCALE_ASPECT);
>> +}
>> +
>> +static void test_scaling_mode_on_output(igt_display_t *display, const 
>> enum pipe pipe,
>> +                    igt_output_t *output, uint32_t flags)
>> +{
>> +    igt_plane_t *primary, *sprite;
>> +    drmModeModeInfo mode;
>> +    struct igt_fb red, blue;
>> +    int ret;
>> +
>> +    igt_output_set_pipe(output, pipe);
>> +    mode = *igt_output_get_mode(output);
>> +
>> +    primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
>> +    sprite = igt_output_get_plane_type(output, DRM_PLANE_TYPE_OVERLAY);
>> +
>> +    igt_create_color_fb(display->drm_fd, mode.hdisplay, mode.vdisplay,
>> +                DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_NONE,
>> +                0.f, 0.f, 1.f, &blue);
>> +
>> +    igt_create_color_fb(display->drm_fd, 640, 480,
>> +                DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_NONE,
>> +                1.f, 0.f, 0.f, &red);
>> +
>> +    igt_plane_set_fb(primary, &blue);
>> +    igt_plane_set_fb(sprite, &red);
>> +
>> +    igt_display_commit2(display, COMMIT_ATOMIC);
>> +
>> +    mode.hdisplay = 640;
>> +    mode.vdisplay = 480;
>> +    igt_output_override_mode(output, &mode);
>> +
>> +    igt_plane_set_fb(sprite, NULL);
>> +    igt_plane_set_fb(primary, &red);
>> +
>> +    test_cycle_flags(output, flags);
>> +
>> +    /* Don't pass ALLOW_MODESET with overridden mode, force fastset */
>> +    ret = igt_display_try_commit_atomic(display, 0, NULL);
>> +
>> +    if (ret != -EINVAL)
>> +        igt_display_commit_atomic(display, 0, NULL);
> 
> Not sure why this commit? On above igt_display_try_commit_atomic(..) 
> actual commit happened already.
> 
>> +    else
>> +        igt_skip_on_f(ret == -EINVAL, "Scaling mode not supported\n");
>> +
>> +        igt_remove_fb(display->drm_fd, &red);
>> +        igt_remove_fb(display->drm_fd, &blue);
> ^^
> indentation with spaces instead of tab cause it to look misaligned in 
> patch.
> 
>> +}
>> +
>> +/* Returns true if an output supports scaling mode property */
>> +static bool has_scaling_mode(igt_output_t *output)
>> +{
>> +    return igt_output_has_prop(output, IGT_CONNECTOR_SCALING_MODE) &&
>> +           igt_output_get_prop(output, IGT_CONNECTOR_SCALING_MODE);
>> +}
>> +
>> +static void test_scaling_mode(data_t *data, uint32_t flags)
>> +{
>> +    igt_display_t *display = &data->display;
>> +    igt_output_t *output;
>> +    enum pipe pipe;
>> +    int valid_tests = 0;
>> +
>> +    for_each_pipe_with_valid_output(display, pipe, output) {
>> +        if (!has_scaling_mode(output))
>> +            continue;
>> +
>> +        igt_dynamic_f("%s-pipe-%s", output->name, 
>> kmstest_pipe_name(pipe));
>> +        igt_display_reset(display);
>> +        test_scaling_mode_on_output(display, pipe, output, flags);
>> +        valid_tests++;
>> +    }
>> +
>> +    igt_require_f(valid_tests, "No valid crtc/connector combinations 
>> found\n");
>> +}
>> +
>> +igt_main
>> +{
>> +    data_t data = {};
>> +
>> +    igt_fixture {
>> +        data.drm_fd = drm_open_driver_master(DRIVER_ANY);
>> +        igt_require(data.drm_fd >= 0);
>> +
>> +        kmstest_set_vt_graphics_mode();
>> +
>> +        igt_display_require(&data.display, data.drm_fd);
>> +        igt_require(data.display.is_atomic);
>> +
>> +        igt_display_require_output(&data.display);
>> +    }
>> +
>> +    igt_describe("Tests full display scaling mode");
>> +    igt_subtest_with_dynamic("scaling-mode-full")
>> +        test_scaling_mode(&data, TEST_FULL);
>> +    igt_describe("Tests center display scaling mode");
>> +    igt_subtest_with_dynamic("scaling-mode-center")
>> +        test_scaling_mode(&data, TEST_CENTER);
>> +    igt_describe("Tests full aspect display scaling mode");
>> +    igt_subtest_with_dynamic("scaling-mode-full-aspect")
>> +        test_scaling_mode(&data, TEST_FULLASPECT);
>> +    igt_describe("Tests none display scaling mode (no scaling)");
>> +    igt_subtest_with_dynamic("scaling-mode-none")
>> +        test_scaling_mode(&data, TEST_NONE);
>> +
>> +    igt_fixture
>> +        igt_display_fini(&data.display);
>> +}
>> diff --git a/tests/meson.build b/tests/meson.build
>> index c14acf99..7003d064 100644
>> --- a/tests/meson.build
>> +++ b/tests/meson.build
>> @@ -50,6 +50,7 @@ test_progs = [
>>       'kms_properties',
>>       'kms_rmfb',
>>       'kms_rotation_crc',
>> +    'kms_scaling_modes',
>>       'kms_selftest',
>>       'kms_sequence',
>>       'kms_setmode',
> 

-- 
~Swati Sharma