Re: [PATCH i-g-t 2/4] tests/intel/kms_sharpness_filter: refactor test_sharpness_filter function

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

On 15-04-2026 05:13 pm, Juha-Pekka Heikkilä wrote:
> On Thu, Apr 9, 2026 at 3:04 PM Sharma, Swati2 <swati2.sharma@intel.com> wrote:
>> Hi JP,
>>
>> Overall patch LGTM. One small nit.
>>
>> On 01-04-2026 12:34 am, Juha-Pekka Heikkila wrote:
>>> refactor to make it look more nice
>>>
>>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>>> ---
>>>    tests/intel/kms_sharpness_filter.c | 108 ++++++++++++++++-------------
>>>    1 file changed, 59 insertions(+), 49 deletions(-)
>>>
>>> diff --git a/tests/intel/kms_sharpness_filter.c b/tests/intel/kms_sharpness_filter.c
>>> index 029b735bd..ed8d2e48c 100644
>>> --- a/tests/intel/kms_sharpness_filter.c
>>> +++ b/tests/intel/kms_sharpness_filter.c
>>> @@ -88,15 +88,6 @@ IGT_TEST_DESCRIPTION("Test to validate content adaptive sharpness filter");
>>>    #define MAX_PIXELS_FOR_3_TAP_FILTER (1920 * 1080)
>>>    #define MAX_PIXELS_FOR_5_TAP_FILTER (3840 * 2160)
>>>    #define NROUNDS                             10
>>> -#define INVALID_TEST ((type == TEST_INVALID_FILTER_WITH_SCALER) \
>>> -                || (type == TEST_INVALID_FILTER_WITH_PLANE) \
>>> -                || (type == TEST_INVALID_PLANE_WITH_FILTER) \
>>> -                || (type == TEST_INVALID_FILTER_WITH_SCALING_MODE))
>>> -#define SET_PLANES ((type == TEST_FILTER_UPSCALE) \
>>> -             ||  (type == TEST_FILTER_DOWNSCALE) \
>>> -             ||  (type == TEST_INVALID_FILTER_WITH_SCALER) \
>>> -             ||  (type == TEST_INVALID_FILTER_WITH_PLANE) \
>>> -             ||  (type == TEST_INVALID_FILTER_WITH_SCALING_MODE))
>>>
>>>    enum test_type {
>>>        TEST_FILTER_BASIC,
>>> @@ -327,12 +318,37 @@ static int test_filter_toggle(data_t *data)
>>>        return ret;
>>>    }
>>>
>>> -static void test_sharpness_filter(data_t *data,  enum test_type type)
>>> +static bool is_invalid_test(enum test_type type)
>>> +{
>>> +     switch (type) {
>>> +     case TEST_INVALID_FILTER_WITH_SCALER:
>>> +     case TEST_INVALID_FILTER_WITH_PLANE:
>>> +     case TEST_INVALID_PLANE_WITH_FILTER:
>>> +     case TEST_INVALID_FILTER_WITH_SCALING_MODE:
>>> +             return true;
>>> +     default:
>>> +             return false;
>>> +     }
>>> +}
>>> +
>>> +static bool needs_extra_planes(enum test_type type)
>>> +{
>>> +     switch (type) {
>>> +     case TEST_FILTER_UPSCALE:
>>> +     case TEST_FILTER_DOWNSCALE:
>>> +     case TEST_INVALID_FILTER_WITH_SCALER:
>>> +     case TEST_INVALID_FILTER_WITH_PLANE:
>>> +     case TEST_INVALID_FILTER_WITH_SCALING_MODE:
>>> +             return true;
>>> +     default:
>>> +             return false;
>>> +     }
>>> +}
>>> +
>>> +static void test_sharpness_filter(data_t *data, enum test_type type)
>>>    {
>>>        igt_output_t *output = data->output;
>>>        drmModeModeInfo *mode = data->mode;
>>> -     int height = mode->hdisplay;
>>> -     int width =  mode->vdisplay;
>>>        igt_crc_t ref_crc, crc;
>>>        igt_pipe_crc_t *pipe_crc = NULL;
>>>        int ret;
>>> @@ -343,57 +359,31 @@ static void test_sharpness_filter(data_t *data,  enum test_type type)
>>>                      "No requested format/modifier on pipe %s\n",
>>>                      igt_crtc_name(data->crtc));
>>>
>>> -     setup_fb(data->drm_fd, height, width, data->format, data->modifier, &data->fb[0]);
>>> +     setup_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
>>> +              data->format, data->modifier, &data->fb[0]);
>>>        igt_plane_set_fb(data->plane[0], &data->fb[0]);
>>>
>>>        if (type == TEST_FILTER_ROTATION) {
>>> -             if (igt_plane_has_rotation(data->plane[0], data->rotation))
>>> -                     igt_plane_set_rotation(data->plane[0], data->rotation);
>>> -             else
>>> -                     igt_skip("No requested rotation on pipe %s\n",
>>> -                              igt_crtc_name(data->crtc));
>>> +             igt_skip_on_f(!igt_plane_has_rotation(data->plane[0],
>>> +                                                   data->rotation),
>>> +                           "No requested rotation on pipe %s\n",
>>> +                           igt_crtc_name(data->crtc));
>>> +             igt_plane_set_rotation(data->plane[0], data->rotation);
>>>        }
>>>
>>>        if (type == TEST_INVALID_FILTER_WITH_SCALING_MODE)
>>>                igt_require_f(has_scaling_mode(output), "No connecter scaling mode found on %s\n", output->name);
>>>
>>> -     if (SET_PLANES)
>>> +     if (needs_extra_planes(type))
>>>                set_planes(data, type);
>>>
>>>        set_filter_strength_on_pipe(data);
>>>
>>> -     if (!INVALID_TEST && data->filter_strength != 0)
>>> -             igt_debug("Sharpened image should be observed for filter strength > 0\n");
>> Since this in NON-CRC based test, should we keep this debug msg?
> I don't think this debug message help here. Maybe these tests should
> be moved behind command line flag so they wouldn't be run on CI?

Ok. No, we need to run these tests on CI; else we won't be able to catch 
regression.

>
>>> -
>>>        if (type == TEST_INVALID_FILTER_WITH_SCALING_MODE)
>>>                ret = igt_display_try_commit_atomic(&data->display, 0, NULL);
>>>        else
>>>                ret = igt_display_try_commit2(&data->display, COMMIT_ATOMIC);
>>>
>>> -     if (type == TEST_FILTER_DPMS || type == TEST_FILTER_SUSPEND) {
>>> -             pipe_crc = igt_crtc_crc_new(data->crtc,
>>> -                                         IGT_PIPE_CRC_SOURCE_AUTO);
>>> -             igt_pipe_crc_collect_crc(pipe_crc, &ref_crc);
>>> -     }
>>> -
>>> -     if (type == TEST_FILTER_DPMS) {
>>> -             kmstest_set_connector_dpms(data->drm_fd,
>>> -                                        output->config.connector,
>>> -                                        DRM_MODE_DPMS_OFF);
>>> -             kmstest_set_connector_dpms(data->drm_fd,
>>> -                                        output->config.connector,
>>> -                                        DRM_MODE_DPMS_ON);
>>> -     }
>>> -
>>> -     if (type == TEST_FILTER_SUSPEND)
>>> -             igt_system_suspend_autoresume(SUSPEND_STATE_MEM,
>>> -                                           SUSPEND_TEST_NONE);
>>> -
>>> -     if (type == TEST_FILTER_DPMS || type == TEST_FILTER_SUSPEND) {
>>> -             igt_pipe_crc_collect_crc(pipe_crc, &crc);
>>> -             igt_assert_crc_equal(&crc, &ref_crc);
>>> -     }
>>> -
>>>        if (type == TEST_FILTER_TOGGLE)
>>>                ret |= test_filter_toggle(data);
>>>
>>> @@ -409,13 +399,33 @@ static void test_sharpness_filter(data_t *data,  enum test_type type)
>>>                ret = igt_display_try_commit2(&data->display, COMMIT_ATOMIC);
>>>        }
>>>
>>> -     if (INVALID_TEST)
>>> +     if (type == TEST_FILTER_DPMS || type == TEST_FILTER_SUSPEND) {
>>> +             pipe_crc = igt_crtc_crc_new(data->crtc,
>>> +                                         IGT_PIPE_CRC_SOURCE_AUTO);
>>> +             igt_pipe_crc_collect_crc(pipe_crc, &ref_crc);
>>> +
>>> +             if (type == TEST_FILTER_DPMS) {
>>> +                     kmstest_set_connector_dpms(data->drm_fd,
>>> +                                                output->config.connector,
>>> +                                                DRM_MODE_DPMS_OFF);
>>> +                     kmstest_set_connector_dpms(data->drm_fd,
>>> +                                                output->config.connector,
>>> +                                                DRM_MODE_DPMS_ON);
>>> +             } else {
>>> +                     igt_system_suspend_autoresume(SUSPEND_STATE_MEM,
>>> +                                                   SUSPEND_TEST_NONE);
>>> +             }
>>> +
>>> +             igt_pipe_crc_collect_crc(pipe_crc, &crc);
>>> +             igt_assert_crc_equal(&crc, &ref_crc);
>>> +             igt_pipe_crc_free(pipe_crc);
>>> +     }
>>> +
>>> +     if (is_invalid_test(type))
>>>                igt_assert_eq(ret, -EINVAL);
>>>        else
>>>                igt_assert_eq(ret, 0);
>>>
>>> -     /* clean-up */
>>> -     igt_pipe_crc_free(pipe_crc);
>>>        cleanup(data);
>>>    }
>>>