Re: [igt-dev] [PATCH i-g-t 3/3] tests/kms_plane_scaling: Add test to validate max source size

[Swati Sharma <swati2.sharma@intel.com>]

On 16-Feb-23 9:01 PM, Juha-Pekka Heikkila wrote:
> On 15.2.2023 18.59, Swati Sharma wrote:
>> i915 specific test is added to validate max source size. This
>> test is expected to return -EINVAL for platforms where we have
>> max source width 4096 whereas it will pass on platforms having
>> max source width 5120. We have created fb of size 5120x4320
>> (=size of source) and downscaled to 3840x2400(=size of dest).
>>
>> On MTL(disp_ver=14), in dmesg we can see
>> "[drm:skl_update_scaler [i915]] scaler_user index 0.0: src
>> 5120x4320 dst 3840x2400 size is out of scaler range."
>> since max source width supported is 4096.
>> whereas same test will pass on ADLP(display_ver=13) since max
>> source width  supported is 5120.
>>
>> Signed-off-by: Swati Sharma <swati2.sharma@intel.com>
>> ---
>>   tests/kms_plane_scaling.c | 94 +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 94 insertions(+)
>>
>> diff --git a/tests/kms_plane_scaling.c b/tests/kms_plane_scaling.c
>> index 9737d8645..ae917b202 100644
>> --- a/tests/kms_plane_scaling.c
>> +++ b/tests/kms_plane_scaling.c
>> @@ -890,6 +890,95 @@ static void invalid_parameter_tests(data_t *d)
>>       }
>>   }
>> +/*
>> + *    Max source/destination width/height
>> + *    for i915 driver.
>> + *
>> + *    DISPLAY_VER < 11
>> + *        max_src_w = 4096
>> + *        max_src_h = 4096
>> + *        max_dst_w = 4096
>> + *        max_dst_h = 4096
>> + *
>> + *    DISPLAY_VER = 11
>> + *        max_src_w = 5120
>> + *        max_src_h = 4096
>> + *        max_dst_w = 5120
>> + *        max_dst_h = 4096
>> + *
>> + *    DISPLAY_VER = 12-13
>> + *        max_src_w = 5120
>> + *        max_src_h = 8192
>> + *        max_dst_w = 8192
>> + *        max_dst_h = 8192
>> + *
>> + *    DISPLAY_VER = 14
>> + *        max_src_w = 4096
>> + *        max_src_h = 8192
>> + *        max_dst_w = 8192
>> + *        max_dst_h = 8192
> 
> I think here could be added comment these numbers are coming from 
> drivers/gpu/drm/i915/display/skl_scaler.c in kernel sources
> 
>> + */
>> +
>> +static void i915_max_source_size_test(data_t *d)
>> +{
>> +    enum pipe pipe = PIPE_A;
>> +    igt_output_t *output;
>> +    igt_fb_t fb;
>> +    igt_plane_t *plane;
>> +    int rval;
>> +
>> +    static const struct invalid_paramtests paramtests[1] = {
> 
> "static const struct invalid_paramtests paramtests[]" would be better 
> here, ie. not declaring here how long is the list of tests
> 
>> +        {
>> +            .testname = "max-src-size",
>> +            .planesize = {3840, 2400},
>> +            .params = {{~0}}
>> +        },
>> +    };
>> +
>> +    igt_fixture {
>> +        output = igt_get_single_output_for_pipe(&d->display, pipe);
>> +        igt_require(output);
>> +
>> +        igt_output_set_pipe(output, pipe);
>> +        plane = igt_output_get_plane_type(output, 
>> DRM_PLANE_TYPE_PRIMARY);
>> +
>> +        igt_create_fb(d->drm_fd, 5120, 4320,
>> +            DRM_FORMAT_XRGB8888,
>> +            DRM_FORMAT_MOD_NONE,
>> +            &fb);
> 
> above indentation is off on those lines with parameters.
> 
>> +    }
>> +
>> +    igt_describe("test for validating max source size.");
>> +    igt_subtest_with_dynamic("i915-max-source-size") {
>> +        for (uint32_t i = 0; i < ARRAY_SIZE(paramtests); i++) {
>> +            igt_dynamic(paramtests[i].testname) {
>> +                igt_plane_set_position(plane, 0, 0);
>> +                igt_plane_set_fb(plane, &fb);
>> +                igt_plane_set_size(plane,
>> +                            paramtests[i].planesize[0],
>> +                            paramtests[i].planesize[1]);
>> +
>> +                for (uint32_t j = 0; paramtests[i].params[j].prop != 
>> ~0; j++)
>> +                    igt_plane_set_prop_value(plane,
>> +                                 paramtests[i].params[j].prop,
>> +                                 paramtests[i].params[j].value);
>> +
>> +                rval = igt_display_try_commit2(&d->display, 
>> COMMIT_ATOMIC);
>> +
>> +                if (intel_display_ver(d->devid) < 11 || 
>> intel_display_ver(d->devid) >= 14)
>> +                    igt_assert(rval == -EINVAL || rval == -ERANGE);
>> +                else
>> +                    igt_assert_eq(rval, 0);
>> +            }
>> +        }
>> +    }
>> +
>> +    igt_fixture {
>> +        igt_remove_fb(d->drm_fd, &fb);
>> +        igt_output_set_pipe(output, PIPE_NONE);
>> +    }
>> +}
>> +
>>   static int opt_handler(int opt, int opt_index, void *_data)
>>   {
>>       data_t *data = _data;
>> @@ -1055,6 +1144,11 @@ igt_main_args("", long_opts, help_str, 
>> opt_handler, &data)
>>       invalid_parameter_tests(&data);
>> +    igt_fixture
>> +        igt_require_f(data.devid, "This test is valid only for i915 
>> devices.\n");
>> +
>> +    i915_max_source_size_test(&data);
>> +
> 
> These above changes I think could be wrapped inside igt_subtest_group so 
> that i915 requirement doesn't become inherited if someone come add 
> something here after this test.

Done. Sent new series with the fixes 
https://patchwork.freedesktop.org/series/114059/

> 
>>       igt_fixture {
>>           igt_display_fini(&data.display);
>>           close(data.drm_fd);
> 
-- 
~Swati Sharma