[igt-dev] [PATCH v5 1/2] tests/kms_lease: Terminate lease fd before subtests and remove lease_t from all sub-tests
Nautiyal, Ankit K
ankit.k.nautiyal at intel.com
Mon Oct 23 12:42:29 UTC 2023
Hi Thasleem,
Patch looks better now.
Small nitpciks inline in commit message:
On 10/23/2023 9:15 AM, Mohammed Thasleem wrote:
> Failures happen at drmModeSetCursor and prepare_crtc_master or
> prepare_crtc_lease which exit tests without termination of lease fd
> which resulting in resource busy for sub-sequence execution, when
typo: subsequent executions
> new lessor request for lease.
>
> Terminate the lease fd when any assert happens without termination
> of lease fd and on any drmModeSetCrtc or drmModeGetCrtc exit.
> Terminate lease fd on every display dependent subtests execution.
>
> Remove lease_t from all sub-tests and move it to data_t and
> send data struct instead lease struct and avoid duplicating
As mentioned by Karthik in previous rev:
"Also better to have this "avoid duplicating of prepare_crtc in to
master crtc and lease crtc." as a separate sentence IMHO."
With these fixed, this is:
Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
> of prepare_crtc in to master crtc and lease crtc.
>
> v2: Separated prepare crtc for master and lease.
> Replace close with terminate_lease.
> Removed lease_t from all sub-tests. (Ankit)
> v3: Updated commit message. (Ankit)
> v4: Avoid duplicating prepare_crtc. (Ankit)
> v5: Send fd to close instead structure address.
> v6: Use lease fd instead mcl.fd for display dependent tests.
> Remove terminate_lease and close fd from display dependent subtests.
> v7: Update commit message. (Karthik)
>
> Signed-off-by: Mohammed Thasleem <mohammed.thasleem at intel.com>
> ---
> tests/kms_lease.c | 168 +++++++++++++++++++++-------------------------
> 1 file changed, 76 insertions(+), 92 deletions(-)
>
> diff --git a/tests/kms_lease.c b/tests/kms_lease.c
> index 343cdb812..44aa51362 100644
> --- a/tests/kms_lease.c
> +++ b/tests/kms_lease.c
> @@ -211,6 +211,7 @@ typedef struct {
> } lease_t;
>
> typedef struct {
> + lease_t lease;
> lease_t master;
> enum pipe pipe;
> uint32_t crtc_id;
> @@ -226,9 +227,10 @@ static igt_output_t *connector_id_to_output(igt_display_t *display, uint32_t con
> return igt_output_from_connector(display, &connector);
> }
>
> -static int prepare_crtc(lease_t *lease, data_t *data)
> +static int prepare_crtc(data_t *data, bool is_master)
> {
> drmModeModeInfo *mode;
> + lease_t *lease = is_master ? &data->master : &data->lease;
> igt_display_t *display = &lease->display;
> igt_output_t *output = connector_id_to_output(display, data->connector_id);
> enum pipe pipe = display->pipes[data->pipe].pipe;
> @@ -314,7 +316,7 @@ static int get_lease(int fd, struct drm_mode_get_lease *mgl)
> return err;
> }
>
> -static int make_lease(data_t *data, lease_t *lease)
> +static int make_lease(data_t *data)
> {
> uint32_t object_ids[3];
> struct drm_mode_create_lease mcl;
> @@ -334,14 +336,14 @@ static int make_lease(data_t *data, lease_t *lease)
> if (ret)
> return ret;
>
> - lease->fd = mcl.fd;
> - lease->lessee_id = mcl.lessee_id;
> + data->lease.fd = mcl.fd;
> + data->lease.lessee_id = mcl.lessee_id;
> return 0;
> }
>
> -static void terminate_lease(lease_t *lease)
> +static void terminate_lease(int lease_fd)
> {
> - close(lease->fd);
> + close(lease_fd);
> }
>
> static int paint_fb(int drm_fd, struct igt_fb *fb, const char *test_name,
> @@ -370,24 +372,21 @@ static void simple_lease(data_t *data)
> {
> enum pipe pipe = data->pipe;
>
> - lease_t lease;
> -
> /* Create a valid lease */
> - igt_assert_eq(make_lease(data, &lease), 0);
> + igt_assert_eq(make_lease(data), 0);
>
> - igt_display_require(&lease.display, lease.fd);
> + igt_display_require(&data->lease.display, data->lease.fd);
>
> /* Set a mode on the leased output */
> - igt_assert_eq(0, prepare_crtc(&lease, data));
> + igt_assert_eq(0, prepare_crtc(data, false));
>
> /* Paint something attractive */
> - paint_fb(lease.fd, &lease.primary_fb, "simple-lease",
> - lease.mode->name, igt_output_name(lease.output), kmstest_pipe_name(pipe));
> + paint_fb(data->lease.fd, &data->lease.primary_fb, "simple-lease",
> + data->lease.mode->name, igt_output_name(data->lease.output),
> + kmstest_pipe_name(pipe));
> igt_debug_wait_for_keypress("lease");
> - cleanup_crtc(&lease,
> - connector_id_to_output(&lease.display, data->connector_id));
> -
> - terminate_lease(&lease);
> + cleanup_crtc(&data->lease,
> + connector_id_to_output(&data->lease.display, data->connector_id));
> }
>
> static void empty_lease(data_t *data)
> @@ -395,8 +394,7 @@ static void empty_lease(data_t *data)
> struct drm_mode_create_lease mcl = {0};
>
> igt_assert_eq(create_lease(data->master.fd, &mcl), 0);
> -
> - close(mcl.fd);
> + data->lease.fd = mcl.fd;
> }
>
> static void page_flip_implicit_plane(data_t *data)
> @@ -429,10 +427,11 @@ static void page_flip_implicit_plane(data_t *data)
>
> drmSetClientCap(data->master.fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 0);
> do_or_die(create_lease(data->master.fd, &mcl));
> + data->lease.fd = mcl.fd;
> drmSetClientCap(data->master.fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
>
> /* Set a mode on the leased output */
> - igt_assert_eq(0, prepare_crtc(&data->master, data));
> + igt_assert_eq(0, prepare_crtc(data, true));
>
> /* sanity check */
> do_or_die(drmModePageFlip(data->master.fd, data->crtc_id,
> @@ -444,22 +443,22 @@ static void page_flip_implicit_plane(data_t *data)
> igt_wait_for_vblank(data->master.fd,
> display->pipes[pipe].crtc_offset);
>
> - do_or_die(drmModePageFlip(mcl.fd, data->crtc_id,
> + do_or_die(drmModePageFlip(data->lease.fd, data->crtc_id,
> data->master.primary_fb.fb_id,
> 0, NULL));
> - close(mcl.fd);
> + close(data->lease.fd);
>
> object_ids[mcl.object_count++] = wrong_plane_id;
> do_or_die(create_lease(data->master.fd, &mcl));
> + data->lease.fd = mcl.fd;
>
> igt_wait_for_vblank(data->master.fd,
> display->pipes[pipe].crtc_offset);
>
> - igt_assert_eq(drmModePageFlip(mcl.fd, data->crtc_id,
> + igt_assert_eq(drmModePageFlip(data->lease.fd, data->crtc_id,
> data->master.primary_fb.fb_id,
> 0, NULL),
> -EACCES);
> - close(mcl.fd);
>
> cleanup_crtc(&data->master,
> connector_id_to_output(&data->master.display, data->connector_id));
> @@ -498,6 +497,7 @@ static void setcrtc_implicit_plane(data_t *data)
>
> drmSetClientCap(data->master.fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 0);
> do_or_die(create_lease(data->master.fd, &mcl));
> + data->lease.fd = mcl.fd;
> drmSetClientCap(data->master.fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
>
> /*
> @@ -505,31 +505,31 @@ static void setcrtc_implicit_plane(data_t *data)
> * then the client cap for aspect-ratio bits must be set.
> */
> if (mode->flags & DRM_MODE_FLAG_PIC_AR_MASK) {
> - drmSetClientCap(mcl.fd, DRM_CLIENT_CAP_ASPECT_RATIO, 1);
> + drmSetClientCap(data->lease.fd, DRM_CLIENT_CAP_ASPECT_RATIO, 1);
> }
>
> /* Set a mode on the leased output */
> - igt_assert_eq(0, prepare_crtc(&data->master, data));
> + igt_assert_eq(0, prepare_crtc(data, true));
>
> /* sanity check */
> ret = drmModeSetCrtc(data->master.fd, data->crtc_id, -1,
> 0, 0, object_ids, 1, mode);
> - ret_mcl = drmModeSetCrtc(mcl.fd, data->crtc_id, -1,
> + ret_mcl = drmModeSetCrtc(data->lease.fd, data->crtc_id, -1,
> 0, 0, object_ids, 1, mode);
> - close(mcl.fd);
> + close(data->lease.fd);
> igt_assert_eq(ret, 0);
> igt_assert_eq(ret_mcl, 0);
>
> object_ids[mcl.object_count++] = wrong_plane_id;
> do_or_die(create_lease(data->master.fd, &mcl));
> + data->lease.fd = mcl.fd;
>
> - igt_assert_eq(drmModeSetCrtc(mcl.fd, data->crtc_id, -1,
> + igt_assert_eq(drmModeSetCrtc(data->lease.fd, data->crtc_id, -1,
> 0, 0, object_ids, 1, mode),
> -EACCES);
> /* make sure we are allowed to turn the CRTC off */
> - do_or_die(drmModeSetCrtc(mcl.fd, data->crtc_id,
> + do_or_die(drmModeSetCrtc(data->lease.fd, data->crtc_id,
> 0, 0, 0, NULL, 0, NULL));
> - close(mcl.fd);
>
> cleanup_crtc(&data->master,
> connector_id_to_output(&data->master.display, data->connector_id));
> @@ -549,23 +549,24 @@ static void cursor_implicit_plane(data_t *data)
>
> drmSetClientCap(data->master.fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 0);
> do_or_die(create_lease(data->master.fd, &mcl));
> + data->lease.fd = mcl.fd;
> drmSetClientCap(data->master.fd, DRM_CLIENT_CAP_UNIVERSAL_PLANES, 1);
>
> /* Set a mode on the leased output */
> - igt_assert_eq(0, prepare_crtc(&data->master, data));
> + igt_assert_eq(0, prepare_crtc(data, true));
>
> /* sanity check */
> do_or_die(drmModeSetCursor(data->master.fd, data->crtc_id, 0, 0, 0));
> - do_or_die(drmModeSetCursor(mcl.fd, data->crtc_id, 0, 0, 0));
> - close(mcl.fd);
> + do_or_die(drmModeSetCursor(data->lease.fd, data->crtc_id, 0, 0, 0));
> + close(data->lease.fd);
>
> /* primary plane is never the cursor */
> object_ids[mcl.object_count++] = data->plane_id;
> do_or_die(create_lease(data->master.fd, &mcl));
> + data->lease.fd = mcl.fd;
>
> - igt_assert_eq(drmModeSetCursor(mcl.fd, data->crtc_id, 0, 0, 0),
> + igt_assert_eq(drmModeSetCursor(data->lease.fd, data->crtc_id, 0, 0, 0),
> -EACCES);
> - close(mcl.fd);
>
> cleanup_crtc(&data->master,
> connector_id_to_output(&data->master.display, data->connector_id));
> @@ -624,7 +625,8 @@ static void atomic_implicit_crtc(data_t *data)
> igt_assert(crtc_id_prop);
>
> do_or_die(create_lease(data->master.fd, &mcl));
> - do_or_die(drmSetClientCap(mcl.fd, DRM_CLIENT_CAP_ATOMIC, 1));
> + data->lease.fd = mcl.fd;
> + do_or_die(drmSetClientCap(data->lease.fd, DRM_CLIENT_CAP_ATOMIC, 1));
>
> /* check CRTC_ID property on the plane */
> req = drmModeAtomicAlloc();
> @@ -637,7 +639,7 @@ static void atomic_implicit_crtc(data_t *data)
> ret = drmModeAtomicCommit(data->master.fd, req, DRM_MODE_ATOMIC_TEST_ONLY, NULL);
> igt_assert(ret == 0 || ret == -EINVAL);
>
> - ret = drmModeAtomicCommit(mcl.fd, req, DRM_MODE_ATOMIC_TEST_ONLY, NULL);
> + ret = drmModeAtomicCommit(data->lease.fd, req, DRM_MODE_ATOMIC_TEST_ONLY, NULL);
> igt_assert(ret == -EACCES);
> drmModeAtomicFree(req);
>
> @@ -652,29 +654,26 @@ static void atomic_implicit_crtc(data_t *data)
> ret = drmModeAtomicCommit(data->master.fd, req, DRM_MODE_ATOMIC_TEST_ONLY, NULL);
> igt_assert(ret == 0 || ret == -EINVAL);
>
> - ret = drmModeAtomicCommit(mcl.fd, req, DRM_MODE_ATOMIC_TEST_ONLY, NULL);
> + ret = drmModeAtomicCommit(data->lease.fd, req, DRM_MODE_ATOMIC_TEST_ONLY, NULL);
> igt_assert(ret == -EACCES);
> drmModeAtomicFree(req);
> -
> - close(mcl.fd);
> }
>
> /* Test listing lessees */
> static void lessee_list(data_t *data)
> {
> - lease_t lease;
> struct drm_mode_list_lessees mll;
> uint32_t lessees[1];
>
> mll.pad = 0;
>
> /* Create a valid lease */
> - igt_assert_eq(make_lease(data, &lease), 0);
> + igt_assert_eq(make_lease(data), 0);
>
> /* check for nested leases */
> mll.count_lessees = 0;
> mll.lessees_ptr = 0;
> - igt_assert_eq(list_lessees(lease.fd, &mll), 0);
> + igt_assert_eq(list_lessees(data->lease.fd, &mll), 0);
> igt_assert_eq(mll.count_lessees, 0);
>
> /* Get the number of lessees */
> @@ -696,14 +695,14 @@ static void lessee_list(data_t *data)
> igt_assert_eq(mll.count_lessees, 1);
>
> /* Make sure the listed lease is the same as the one we created */
> - igt_assert_eq(lessees[0], lease.lessee_id);
> + igt_assert_eq(lessees[0], data->lease.lessee_id);
>
> /* invalid pad */
> mll.pad = -1;
> igt_assert_eq(list_lessees(data->master.fd, &mll), -EINVAL);
> mll.pad = 0;
>
> - terminate_lease(&lease);
> + terminate_lease(data->lease.fd);
>
> /* Make sure the lease is gone */
> igt_assert_eq(list_lessees(data->master.fd, &mll), 0);
> @@ -713,7 +712,6 @@ static void lessee_list(data_t *data)
> /* Test getting the contents of a lease */
> static void lease_get(data_t *data)
> {
> - lease_t lease;
> struct drm_mode_get_lease mgl;
> int num_leased_obj = 3;
> uint32_t objects[num_leased_obj];
> @@ -722,12 +720,12 @@ static void lease_get(data_t *data)
> mgl.pad = 0;
>
> /* Create a valid lease */
> - igt_assert_eq(make_lease(data, &lease), 0);
> + igt_assert_eq(make_lease(data), 0);
>
> /* Get the number of objects */
> mgl.count_objects = 0;
> mgl.objects_ptr = 0;
> - igt_assert_eq(get_lease(lease.fd, &mgl), 0);
> + igt_assert_eq(get_lease(data->lease.fd, &mgl), 0);
>
> /* Make sure it's 2 */
> igt_assert_eq(mgl.count_objects, num_leased_obj);
> @@ -735,7 +733,7 @@ static void lease_get(data_t *data)
> /* Get the objects */
> mgl.objects_ptr = (uint64_t) (uintptr_t) objects;
>
> - igt_assert_eq(get_lease(lease.fd, &mgl), 0);
> + igt_assert_eq(get_lease(data->lease.fd, &mgl), 0);
>
> /* Make sure it's 2 */
> igt_assert_eq(mgl.count_objects, num_leased_obj);
> @@ -761,28 +759,25 @@ static void lease_get(data_t *data)
>
> /* invalid pad */
> mgl.pad = -1;
> - igt_assert_eq(get_lease(lease.fd, &mgl), -EINVAL);
> + igt_assert_eq(get_lease(data->lease.fd, &mgl), -EINVAL);
> mgl.pad = 0;
>
> /* invalid pointer */
> mgl.objects_ptr = 0;
> - igt_assert_eq(get_lease(lease.fd, &mgl), -EFAULT);
> -
> - terminate_lease(&lease);
> + igt_assert_eq(get_lease(data->lease.fd, &mgl), -EFAULT);
> }
>
> static void lease_unleased_crtc(data_t *data)
> {
> - lease_t lease;
> enum pipe p;
> uint32_t bad_crtc_id;
> drmModeCrtc *crtc;
> int ret;
>
> /* Create a valid lease */
> - igt_assert_eq(make_lease(data, &lease), 0);
> + igt_assert_eq(make_lease(data), 0);
>
> - igt_display_require(&lease.display, lease.fd);
> + igt_display_require(&data->lease.display, data->lease.fd);
>
> /* Find another CRTC that we don't control */
> bad_crtc_id = 0;
> @@ -798,36 +793,33 @@ static void lease_unleased_crtc(data_t *data)
> igt_skip_on(bad_crtc_id == 0);
>
> /* sanity check */
> - ret = drmModeSetCrtc(lease.fd, data->crtc_id, 0, 0, 0, NULL, 0, NULL);
> + ret = drmModeSetCrtc(data->lease.fd, data->crtc_id, 0, 0, 0, NULL, 0, NULL);
> igt_assert_eq(ret, 0);
> - crtc = drmModeGetCrtc(lease.fd, data->crtc_id);
> + crtc = drmModeGetCrtc(data->lease.fd, data->crtc_id);
> igt_assert(crtc);
> drmModeFreeCrtc(crtc);
>
> /* Attempt to use the unleased crtc id. We need raw ioctl to bypass the
> * igt_kms helpers.
> */
> - ret = drmModeSetCrtc(lease.fd, bad_crtc_id, 0, 0, 0, NULL, 0, NULL);
> + ret = drmModeSetCrtc(data->lease.fd, bad_crtc_id, 0, 0, 0, NULL, 0, NULL);
> igt_assert_eq(ret, -ENOENT);
> - crtc = drmModeGetCrtc(lease.fd, bad_crtc_id);
> + crtc = drmModeGetCrtc(data->lease.fd, bad_crtc_id);
> igt_assert(!crtc);
> igt_assert_eq(errno, ENOENT);
> drmModeFreeCrtc(crtc);
> -
> - terminate_lease(&lease);
> }
>
> static void lease_unleased_connector(data_t *data)
> {
> - lease_t lease;
> int o;
> uint32_t bad_connector_id;
> drmModeConnector *c;
>
> /* Create a valid lease */
> - igt_assert_eq(make_lease(data, &lease), 0);
> + igt_assert_eq(make_lease(data), 0);
>
> - igt_display_require(&lease.display, lease.fd);
> + igt_display_require(&data->lease.display, data->lease.fd);
>
> /* Find another connector that we don't control */
> bad_connector_id = 0;
> @@ -840,72 +832,66 @@ static void lease_unleased_connector(data_t *data)
> igt_skip_on(bad_connector_id == 0);
>
> /* sanity check */
> - c = drmModeGetConnector(lease.fd, data->connector_id);
> + c = drmModeGetConnector(data->lease.fd, data->connector_id);
> igt_assert(c);
>
> /* Attempt to use the unleased connector id. Note that the
> */
> - c = drmModeGetConnector(lease.fd, bad_connector_id);
> + c = drmModeGetConnector(data->lease.fd, bad_connector_id);
> igt_assert(!c);
> igt_assert_eq(errno, ENOENT);
> -
> - terminate_lease(&lease);
> }
>
> /* Test revocation of lease */
> static void lease_revoke(data_t *data)
> {
> - lease_t lease;
> struct drm_mode_revoke_lease mrl;
> int ret;
>
> /* Create a valid lease */
> - igt_assert_eq(make_lease(data, &lease), 0);
> + igt_assert_eq(make_lease(data), 0);
>
> - igt_display_require(&lease.display, lease.fd);
> + igt_display_require(&data->lease.display, data->lease.fd);
>
> /* try to revoke an invalid lease */
> mrl.lessee_id = 0;
> igt_assert_eq(revoke_lease(data->master.fd, &mrl), -ENOENT);
>
> /* try to revoke with the wrong fd */
> - mrl.lessee_id = lease.lessee_id;
> - igt_assert_eq(revoke_lease(lease.fd, &mrl), -EACCES);
> + mrl.lessee_id = data->lease.lessee_id;
> + igt_assert_eq(revoke_lease(data->lease.fd, &mrl), -EACCES);
>
> /* Revoke the lease using the master fd */
> - mrl.lessee_id = lease.lessee_id;
> + mrl.lessee_id = data->lease.lessee_id;
> igt_assert_eq(revoke_lease(data->master.fd, &mrl), 0);
>
> /* Try to use the leased objects */
> - ret = prepare_crtc(&lease, data);
> + ret = prepare_crtc(data, false);
>
> /* Ensure that the expected error is returned */
> igt_assert_eq(ret, -ENOENT);
>
> - terminate_lease(&lease);
> + terminate_lease(data->lease.fd);
>
> /* make sure the lease is gone */
> - mrl.lessee_id = lease.lessee_id;
> + mrl.lessee_id = data->lease.lessee_id;
> igt_assert_eq(revoke_lease(data->master.fd, &mrl), -ENOENT);
> }
>
> /* Test leasing objects more than once */
> static void lease_again(data_t *data)
> {
> - lease_t lease_a, lease_b;
>
> /* Create a valid lease */
> - igt_assert_eq(make_lease(data, &lease_a), 0);
> + igt_assert_eq(make_lease(data), 0);
>
> /* Attempt to re-lease the same objects */
> - igt_assert_eq(make_lease(data, &lease_b), -EBUSY);
> + igt_assert_eq(make_lease(data), -EBUSY);
>
> - terminate_lease(&lease_a);
> + terminate_lease(data->lease.fd);
>
> /* Now attempt to lease the same objects */
> - igt_assert_eq(make_lease(data, &lease_b), 0);
> -
> - terminate_lease(&lease_b);
> + igt_assert_eq(make_lease(data), 0);
> }
>
> #define assert_unleased(ret) \
> @@ -915,14 +901,13 @@ static void lease_again(data_t *data)
> /* Test leasing an invalid connector */
> static void lease_invalid_connector(data_t *data)
> {
> - lease_t lease;
> uint32_t save_connector_id;
> int ret;
>
> /* Create an invalid lease */
> save_connector_id = data->connector_id;
> data->connector_id = 0xbaadf00d;
> - ret = make_lease(data, &lease);
> + ret = make_lease(data);
> data->connector_id = save_connector_id;
> assert_unleased(ret);
> }
> @@ -930,28 +915,26 @@ static void lease_invalid_connector(data_t *data)
> /* Test leasing an invalid crtc */
> static void lease_invalid_crtc(data_t *data)
> {
> - lease_t lease;
> uint32_t save_crtc_id;
> int ret;
>
> /* Create an invalid lease */
> save_crtc_id = data->crtc_id;
> data->crtc_id = 0xbaadf00d;
> - ret = make_lease(data, &lease);
> + ret = make_lease(data);
> data->crtc_id = save_crtc_id;
> assert_unleased(ret);
> }
>
> static void lease_invalid_plane(data_t *data)
> {
> - lease_t lease;
> uint32_t save_plane_id;
> int ret;
>
> /* Create an invalid lease */
> save_plane_id = data->plane_id;
> data->plane_id = 0xbaadf00d;
> - ret = make_lease(data, &lease);
> + ret = make_lease(data);
> data->plane_id = save_plane_id;
> assert_unleased(ret);
> }
> @@ -1393,6 +1376,7 @@ igt_main
> DRM_PLANE_TYPE_PRIMARY)->drm_plane->plane_id;
> f->func(&data);
> }
> + terminate_lease(data.lease.fd);
> }
> }
> }
More information about the igt-dev
mailing list