From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mgamail.intel.com (mgamail.intel.com [192.55.52.43]) by gabe.freedesktop.org (Postfix) with ESMTPS id 584B910E19C for ; Tue, 10 Oct 2023 05:47:20 +0000 (UTC) Message-ID: <029d9a3b-58aa-4a66-a8af-9cfeae4b5a72@intel.com> Date: Tue, 10 Oct 2023 11:17:15 +0530 MIME-Version: 1.0 Content-Language: en-US To: Karthik B S , igt-dev@lists.freedesktop.org References: <20230918075411.247122-1-swati2.sharma@intel.com> <20230918075411.247122-4-swati2.sharma@intel.com> From: "Sharma, Swati2" In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [igt-dev] [PATCH i-g-t 3/6] tests/kms_atomic: test cleanup List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: Hi Karthik, On 27-Sep-23 9:02 AM, Karthik B S wrote: > > On 9/18/2023 1:24 PM, Swati Sharma wrote: >> Use atomic_clear() for all subtests. > > Hi, > > Overall the patch looks good to me. But as we're doing a little more > than only using atomic_clear() in all subtests, could you could split > the patch into two? Or if you see that the other changes are too trivial > for a separate patch, please update the commit message mentioning that > few checks are moved around in the functions. I'm fine with either > approach. > > Thanks, > Karthik.B.S Updated commit message since other changes were trivial. >> >> Signed-off-by: Swati Sharma >> --- >>   tests/kms_atomic.c | 49 ++++++++++++++++++++++++++-------------------- >>   1 file changed, 28 insertions(+), 21 deletions(-) >> >> diff --git a/tests/kms_atomic.c b/tests/kms_atomic.c >> index f984d34f0..c55cb5b2f 100644 >> --- a/tests/kms_atomic.c >> +++ b/tests/kms_atomic.c >> @@ -393,27 +393,23 @@ plane_primary_overlay_mutable_zpos(igt_pipe_t >> *pipe, igt_output_t *output, >>    */ >>   static void >>   plane_immutable_zpos(igt_display_t *display, igt_pipe_t *pipe, >> -             igt_output_t *output) >> +             igt_output_t *output, igt_plane_t *primary, int n_planes) >>   { >>       cairo_t *cr; >>       struct igt_fb fb_ref; >> -    igt_plane_t *primary; >>       drmModeModeInfo *mode; >>       igt_pipe_crc_t *pipe_crc; >>       igt_crc_t ref_crc, new_crc; >>       int fb_id_lower, fb_id_upper; >> -    int n_planes = pipe->n_planes; >>       igt_plane_t *plane_ptr[n_planes]; >>       struct igt_fb fb_lower, fb_upper; >>       uint32_t w_lower, h_lower, w_upper, h_upper; >>       memset(plane_ptr, 0, n_planes * sizeof(igt_plane_t *)); >> -    igt_require(n_planes >= 2); >>       igt_require_pipe_crc(display->drm_fd); >>       mode = igt_output_get_mode(output); >> -    primary = igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY); >>       /* for lower plane */ >>       w_lower = mode->hdisplay; >> @@ -643,17 +639,13 @@ static void plane_primary(igt_pipe_t *pipe, >> igt_plane_t *plane, struct igt_fb *f >>    * Test to ensure that DRM_MODE_ATOMIC_TEST_ONLY really only touches >> the >>    * free-standing state objects and nothing else. >>    */ >> -static void test_only(igt_pipe_t *pipe_obj, >> -              igt_plane_t *primary, >> -              igt_output_t *output) >> +static void test_only(igt_pipe_t *pipe_obj, igt_plane_t *primary, >> +              igt_output_t *output, uint32_t format) >>   { >>       drmModeModeInfo *mode = igt_output_get_mode(output); >> -    uint32_t format = plane_get_igt_format(primary); >>       struct igt_fb fb; >>       uint64_t old_plane_values[IGT_NUM_PLANE_PROPS], >> old_crtc_values[IGT_NUM_CRTC_PROPS]; >> -    igt_require(format != 0); >> - >>       plane_get_current_state(primary, old_plane_values); >>       crtc_get_current_state(pipe_obj, old_crtc_values); >> @@ -1388,7 +1380,8 @@ static void atomic_setup(igt_display_t *display, >> enum pipe pipe, igt_output_t *o >>       crtc_commit(primary->pipe, primary, COMMIT_ATOMIC, >> ATOMIC_RELAX_NONE); >>   } >> -static void atomic_clear(igt_display_t *display, enum pipe pipe, >> igt_plane_t *primary, igt_output_t *output) >> +static void atomic_clear(igt_display_t *display, enum pipe pipe, >> igt_plane_t *primary, >> +             igt_output_t *output, struct igt_fb *fb) >>   { >>       igt_plane_t *plane; >> @@ -1399,6 +1392,7 @@ static void atomic_clear(igt_display_t *display, >> enum pipe pipe, igt_plane_t *pr >>       igt_output_set_pipe(output, PIPE_NONE); >>       crtc_commit(primary->pipe, primary, COMMIT_ATOMIC, >> ATOMIC_RELAX_NONE); >> +    igt_remove_fb(display->drm_fd, fb); >>   } >>   igt_main >> @@ -1450,6 +1444,7 @@ igt_main >>           atomic_setup(&display, pipe, output, primary, &fb); >>           plane_overlay(pipe_obj, output, overlay); >> +        atomic_clear(&display, pipe, primary, output, &fb); >>       } >>       igt_describe("Test for KMS atomic modesetting on primary plane >> and ensure coherency between " >> @@ -1457,6 +1452,7 @@ igt_main >>       igt_subtest("plane-primary-legacy") { >>           atomic_setup(&display, pipe, output, primary, &fb); >>           plane_primary(pipe_obj, primary, &fb); >> +        atomic_clear(&display, pipe, primary, output, &fb); >>       } >>       igt_describe("Verify that the overlay plane can cover the >> primary one (and "\ >> @@ -1464,14 +1460,14 @@ igt_main >>       igt_subtest("plane-primary-overlay-mutable-zpos") { >>           uint32_t format_primary = DRM_FORMAT_ARGB8888; >>           uint32_t format_overlay = DRM_FORMAT_ARGB1555; >> -        igt_plane_t *overlay; >> +        igt_plane_t *overlay = >> +            igt_pipe_get_plane_type(pipe_obj, DRM_PLANE_TYPE_OVERLAY); >> +        igt_require(overlay); >>           igt_info("Using (pipe %s + %s) to run the subtest.\n", >>                kmstest_pipe_name(pipe), igt_output_name(output)); >>           igt_display_reset(&display); >> -        overlay = igt_pipe_get_plane_type(pipe_obj, >> DRM_PLANE_TYPE_OVERLAY); >> -        igt_require(overlay); >>           igt_require(igt_plane_has_prop(primary, IGT_PLANE_ZPOS)); >>           igt_require(igt_plane_has_prop(overlay, IGT_PLANE_ZPOS)); >> @@ -1482,28 +1478,35 @@ igt_main >>           igt_output_set_pipe(output, pipe); >>           plane_primary_overlay_mutable_zpos(pipe_obj, output, >> primary, overlay, >>                              format_primary, format_overlay); >> +        atomic_clear(&display, pipe, primary, output, &fb); >>       } >>       igt_describe("Verify the reported zpos property of planes by >> making sure "\ >>                "only higher zpos planes cover the lower zpos ones."); >>       igt_subtest("plane-immutable-zpos") { >> +        int n_planes = pipe_obj->n_planes; >> +        igt_require(n_planes >= 2); >> + >>           igt_info("Using (pipe %s + %s) to run the subtest.\n", >>                kmstest_pipe_name(pipe), igt_output_name(output)); >>           igt_display_reset(&display); >>           igt_output_set_pipe(output, pipe); >> -        plane_immutable_zpos(&display, pipe_obj, output); >> +        plane_immutable_zpos(&display, pipe_obj, output, primary, >> n_planes); >> +        atomic_clear(&display, pipe, primary, output, &fb); >>       } >>       igt_describe("Test to ensure that DRM_MODE_ATOMIC_TEST_ONLY >> really only touches " >>                "the free-standing state objects and nothing else."); >>       igt_subtest("test-only") { >> -        atomic_clear(&display, pipe, primary, output); >> +        uint32_t format = plane_get_igt_format(primary); >> +        igt_require(format != 0); >>           igt_info("Using (pipe %s + %s) to run the subtest.\n", >>                kmstest_pipe_name(pipe), igt_output_name(output)); >> -        test_only(pipe_obj, primary, output); >> +        atomic_clear(&display, pipe, primary, output, &fb); >> +        test_only(pipe_obj, primary, output, format); >>       } >>       igt_describe("Test for KMS atomic modesetting on cursor plane >> and ensure coherency between " >> @@ -1515,30 +1518,35 @@ igt_main >>           atomic_setup(&display, pipe, output, primary, &fb); >>           plane_cursor(pipe_obj, output, cursor); >> +        atomic_clear(&display, pipe, primary, output, &fb); >>       } >>       igt_describe("Test error handling when invalid plane parameters >> are passed"); >>       igt_subtest("plane-invalid-params") { >>           atomic_setup(&display, pipe, output, primary, &fb); >>           plane_invalid_params(pipe_obj, output, primary, &fb); >> +        atomic_clear(&display, pipe, primary, output, &fb); >>       } >>       igt_describe("Test error handling when invalid plane fence >> parameters are passed"); >>       igt_subtest("plane-invalid-params-fence") { >>           atomic_setup(&display, pipe, output, primary, &fb); >>           plane_invalid_params_fence(pipe_obj, output, primary); >> +        atomic_clear(&display, pipe, primary, output, &fb); >>       } >>       igt_describe("Test error handling when invalid crtc parameters >> are passed"); >>       igt_subtest("crtc-invalid-params") { >>           atomic_setup(&display, pipe, output, primary, &fb); >>           crtc_invalid_params(pipe_obj, output, primary, &fb); >> +        atomic_clear(&display, pipe, primary, output, &fb); >>       } >>       igt_describe("Test error handling when invalid crtc fence >> parameters are passed"); >>       igt_subtest("crtc-invalid-params-fence") { >>           atomic_setup(&display, pipe, output, primary, &fb); >>           crtc_invalid_params_fence(pipe_obj, output, primary, &fb); >> +        atomic_clear(&display, pipe, primary, output, &fb); >>       } >>       igt_describe("Test abuse the atomic ioctl directly in order to >> test " >> @@ -1547,6 +1555,7 @@ igt_main >>       igt_subtest("atomic-invalid-params") { >>           atomic_setup(&display, pipe, output, primary, &fb); >>           atomic_invalid_params(pipe_obj, primary, output, &fb); >> +        atomic_clear(&display, pipe, primary, output, &fb); >>       } >>       igt_describe("Simple test cases to use FB_DAMAGE_CLIPS plane >> property"); >> @@ -1555,12 +1564,10 @@ igt_main >>           atomic_setup(&display, pipe, output, primary, &fb); >>           atomic_plane_damage(pipe_obj, primary, &fb); >> +        atomic_clear(&display, pipe, primary, output, &fb); >>       } >>       igt_fixture { >> -        atomic_clear(&display, pipe, primary, output); >> -        igt_remove_fb(display.drm_fd, &fb); >> - >>           igt_display_fini(&display); >>           drm_close_driver(display.drm_fd); >>       }