From: Jani Nikula <jani.nikula@intel.com>
To: Jeevan B <jeevan.b@intel.com>, igt-dev@lists.freedesktop.org
Cc: Jeevan B <jeevan.b@intel.com>
Subject: Re: [PATCH i-g-t v2 3/3] tests/kms_setmode: Use public igt_display helper APIs
Date: Tue, 23 Jun 2026 11:31:55 +0300 [thread overview]
Message-ID: <2bb1c45a9f509bbb17ba72de436cf0e7e563b2fc@intel.com> (raw)
In-Reply-To: <20260623080927.3216500-4-jeevan.b@intel.com>
On Tue, 23 Jun 2026, Jeevan B <jeevan.b@intel.com> wrote:
> Replace all direct access to igt_display_t internals with public
> helper APIs to maintain proper encapsulation and improve forward
> compatibility with IGT. This refactoring updates code to use
> igt_display and igt_crtc accessors, adjusts function signatures
> to pass objects instead of indices, and removes reliance on internal
> struct fields, ensuring cleaner, more robust, and idiomatic
> usage without changing functionality.
These changes need to be folded into the previous patch instead of
adding a broken intermediate step.
>
> Signed-off-by: Jeevan B <jeevan.b@intel.com>
> ---
> tests/kms_setmode.c | 68 ++++++++++++++++++++++++++++-----------------
> 1 file changed, 43 insertions(+), 25 deletions(-)
>
> diff --git a/tests/kms_setmode.c b/tests/kms_setmode.c
> index 419f5b518..1d5c4dfb0 100644
> --- a/tests/kms_setmode.c
> +++ b/tests/kms_setmode.c
> @@ -334,12 +334,14 @@ static int get_encoder_idx(drmModeRes *resources, drmModeEncoder *encoder)
> }
>
> static void get_crtc_config_str(struct crtc_config *crtc, char *buf,
> - size_t buf_size)
> + size_t buf_size, igt_display_t *disp)
> {
> - igt_crtc_t *igt_crtc = &display.crtcs[crtc->crtc_idx];
> + igt_crtc_t *igt_crtc = igt_crtc_for_crtc_index(disp, crtc->crtc_idx);
> int pos;
> int i;
>
> + igt_assert(igt_crtc);
> +
> pos = snprintf(buf, buf_size,
> "CRTC[%d] [Pipe %s] Mode: %s@%dHz Connectors: ",
> igt_crtc->crtc_id, kmstest_pipe_name(igt_crtc->pipe),
> @@ -374,7 +376,6 @@ static void setup_crtcs(const struct test_config *tconf,
> /* Fetch resources locally only for encoder clone validity checks */
> resources = drmModeGetResources(drm_fd);
> igt_assert(resources);
> -
> encoder_usage_count = calloc(resources->count_encoders,
> sizeof(*encoder_usage_count));
> igt_assert(encoder_usage_count);
> @@ -409,15 +410,16 @@ static void setup_crtcs(const struct test_config *tconf,
> igt_output_t *output;
> drmModeConnector *connector;
> drmModeEncoder *encoder;
> - igt_crtc_t *igt_crtc;
> + igt_crtc_t *crtc_obj;
Please no. The convention everywhere is igt_crtc_t *crtc. Use
it. Everywhere.
>
> crtc->cconfs[j] = cconf[i + j];
> output = cconf[i + j].output;
> connector = output->config.connector;
> - igt_crtc = &disp->crtcs[crtc->crtc_idx];
> + crtc_obj = igt_crtc_for_crtc_index(disp, crtc->crtc_idx);
> + igt_assert(crtc_obj);
>
> /* Check CRTC/output compatibility using IGT helper */
> - config_valid &= igt_crtc_connector_valid(igt_crtc, output);
> + config_valid &= igt_crtc_connector_valid(crtc_obj, output);
>
> /* Intel connectors have only a single encoder */
> if (connector->count_encoders == 1) {
> @@ -491,7 +493,7 @@ static void cleanup_crtcs(struct crtc_config *crtcs, int crtc_count)
> #define frame_time(km) (1000.0 * (km)->htotal * (km)->vtotal / (km)->clock)
> #define line_time(km) (1000.0 * (km)->htotal / (km)->clock)
>
> -static bool check_timings(int crtc_idx, const drmModeModeInfo *kmode)
> +static bool check_timings(igt_crtc_t *crtc, const drmModeModeInfo *kmode)
> {
> #define CALIBRATE_TS_STEPS 120 /* ~2s has to be less than 128! */
> drmVBlank wait;
> @@ -503,10 +505,9 @@ static bool check_timings(int crtc_idx, const drmModeModeInfo *kmode)
> double mean;
> double stddev;
> int n;
> - int pipe_crtc_index = display.crtcs[crtc_idx].crtc_index;
>
> memset(&wait, 0, sizeof(wait));
> - wait.request.type = kmstest_get_vbl_flag(pipe_crtc_index);
> + wait.request.type = igt_crtc_get_vbl_flag(crtc);
> wait.request.type |= DRM_VBLANK_RELATIVE | DRM_VBLANK_NEXTONMISS;
> do_or_die(drmWaitVBlank(drm_fd, &wait));
>
> @@ -516,7 +517,7 @@ static bool check_timings(int crtc_idx, const drmModeModeInfo *kmode)
> last_timestamp += wait.reply.tval_usec;
>
> memset(&wait, 0, sizeof(wait));
> - wait.request.type = kmstest_get_vbl_flag(pipe_crtc_index);
> + wait.request.type = igt_crtc_get_vbl_flag(crtc);
> wait.request.type |= DRM_VBLANK_ABSOLUTE | DRM_VBLANK_EVENT;
> wait.request.sequence = last_seq;
> for (n = 0; n < CALIBRATE_TS_STEPS; n++) {
> @@ -526,7 +527,7 @@ static bool check_timings(int crtc_idx, const drmModeModeInfo *kmode)
> do_or_die(drmWaitVBlank(drm_fd, &wait));
>
> /* Double check that haven't already missed the vblank */
> - check.request.type = kmstest_get_vbl_flag(pipe_crtc_index);
> + check.request.type = igt_crtc_get_vbl_flag(crtc);
> check.request.type |= DRM_VBLANK_RELATIVE;
> do_or_die(drmWaitVBlank(drm_fd, &check));
>
> @@ -649,7 +650,7 @@ retry:
> }
>
> for (i = 0; i < crtc_count; i++) {
> - get_crtc_config_str(&crtcs[i], str_buf[i], sizeof(str_buf[i]));
> + get_crtc_config_str(&crtcs[i], str_buf[i], sizeof(str_buf[i]), tconf->display);
> crtc_strs[i] = &str_buf[i][0];
> }
>
> @@ -661,12 +662,13 @@ retry:
>
> for (i = 0; i < crtc_count; i++) {
> igt_plane_t *primary;
> - igt_crtc_t *igt_crtc;
> + igt_crtc_t *crtc_obj;
> bool invalid_clone;
> int j;
>
> crtc = &crtcs[i];
> - igt_crtc = &display.crtcs[crtc->crtc_idx];
> + crtc_obj = igt_crtc_for_crtc_index(tconf->display, crtc->crtc_idx);
> + igt_assert(crtc_obj);
>
> /* Treat this as expected failure for invalid tests.*/
> invalid_clone = (tconf->flags & TEST_INVALID) &&
> @@ -686,11 +688,11 @@ retry:
> for (j = 0; j < crtc->connector_count; j++) {
> igt_output_t *output = crtc->cconfs[j].output;
>
> - igt_output_set_crtc(output, igt_crtc);
> + igt_output_set_crtc(output, crtc_obj);
> igt_output_override_mode(output, &crtc->mode);
> }
>
> - primary = igt_crtc_get_plane_type(igt_crtc, DRM_PLANE_TYPE_PRIMARY);
> + primary = igt_crtc_get_plane_type(crtc_obj, DRM_PLANE_TYPE_PRIMARY);
> igt_plane_set_fb(primary, &crtc->fb_info);
> }
>
> @@ -698,8 +700,13 @@ retry:
> DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
>
> if (is_intel_device(drm_fd) && ret == 0) {
> - for (i = 0; i < crtc_count; i++)
> - intel_drrs_disable(&display.crtcs[crtcs[i].crtc_idx]);
> + for (i = 0; i < crtc_count; i++) {
> + igt_crtc_t *crtc_obj = igt_crtc_for_crtc_index(tconf->display,
> + crtcs[i].crtc_idx);
> +
> + igt_assert(crtc_obj);
> + intel_drrs_disable(crtc_obj);
> + }
> }
>
> if (ret < 0) {
> @@ -721,9 +728,12 @@ retry:
>
> if (ret == 0 && tconf->flags & TEST_TIMINGS) {
> bool status = false;
> + igt_crtc_t *crtc_obj = igt_crtc_for_crtc_index(tconf->display,
> + crtcs[0].crtc_idx);
> + igt_assert(crtc_obj);
>
> for (int attempt = 1; attempt <= 2; attempt++) {
> - status = check_timings(crtcs[0].crtc_idx, &crtcs[0].mode);
> + status = check_timings(crtc_obj, &crtcs[0].mode);
> if (status)
> break;
> igt_info("Timing check failed on attempt %d, retrying...\n", attempt);
> @@ -736,13 +746,16 @@ retry:
> }
>
> static int get_test_name_str(struct crtc_config *crtc, char *buf,
> - size_t buf_size)
> + size_t buf_size, igt_display_t *disp)
Parameters should be ordered from the highest level context to the
lowest level. igt_display_t *display should be first.
> {
> + igt_crtc_t *crtc_obj = igt_crtc_for_crtc_index(disp, crtc->crtc_idx);
> int pos;
> int i;
>
> + igt_assert(crtc_obj);
> +
> pos = snprintf(buf, buf_size, "pipe-%s-",
> - kmstest_pipe_name(display.crtcs[crtc->crtc_idx].pipe));
> + kmstest_pipe_name(crtc_obj->pipe));
>
> for (i = 0; i < crtc->connector_count; i++) {
> drmModeConnector *connector = crtc->cconfs[i].output->config.connector;
> @@ -774,7 +787,9 @@ static void test_one_combination(const struct test_config *tconf,
> for (i = 0; i < crtc_count; i++) {
> if (i > 0)
> pos += snprintf(&test_name[pos], ARRAY_SIZE(test_name) - pos, "-");
> - pos += get_test_name_str(&crtcs[i], &test_name[pos], ARRAY_SIZE(test_name) - pos);
> + pos += get_test_name_str(&crtcs[i], &test_name[pos],
> + ARRAY_SIZE(test_name) - pos,
> + tconf->display);
> }
>
> if (!is_intel_device(drm_fd))
> @@ -783,7 +798,8 @@ static void test_one_combination(const struct test_config *tconf,
> for (i = 0; i < crtc_count; i++) {
> struct crtc_config *crtc = &crtcs[i];
> char conn_name[64], prev_conn_name[64];
> - int n_valid_crtcs = get_crtc_count(tconf->display->n_crtcs, extended);
> + int n_valid_crtcs = get_crtc_count(igt_display_n_crtcs(tconf->display),
> + extended);
>
> snprintf(conn_name, sizeof(conn_name), "%s",
> igt_output_name(crtc->cconfs->output));
> @@ -933,7 +949,8 @@ static void test_combinations(const struct test_config *tconf,
> struct combination_set crtc_combs;
> struct connector_config *cconfs;
> int i;
> - int crtc_count = get_crtc_count(tconf->display->n_crtcs, extended);
> + int crtc_count = get_crtc_count(igt_display_n_crtcs(tconf->display),
> + extended);
>
> igt_assert(tconf->display);
>
> @@ -986,7 +1003,8 @@ free_cconfs:
> static void run_test(const struct test_config *tconf)
> {
> int connector_num;
> - int crtc_count = get_crtc_count(tconf->display->n_crtcs, extended);
> + int crtc_count = get_crtc_count(igt_display_n_crtcs(tconf->display),
> + extended);
>
> connector_num = tconf->flags & TEST_CLONE ? 2 : 1;
> for (; connector_num <= crtc_count; connector_num++)
--
Jani Nikula, Intel
prev parent reply other threads:[~2026-06-23 8:32 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-06-23 8:09 [PATCH i-g-t v2 0/3] tests/kms_setmode: Convert to igt_display atomic API Jeevan B
2026-06-23 8:09 ` [PATCH i-g-t v2 1/3] tests/kms_setmode: Drop invalid-clone-single-crtc-stealing subtest Jeevan B
2026-06-23 8:09 ` [PATCH i-g-t v2 2/3] tests/kms_setmode: Convert modeset path to atomic API Jeevan B
2026-06-23 8:26 ` Jani Nikula
2026-06-23 8:09 ` [PATCH i-g-t v2 3/3] tests/kms_setmode: Use public igt_display helper APIs Jeevan B
2026-06-23 8:31 ` Jani Nikula [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2bb1c45a9f509bbb17ba72de436cf0e7e563b2fc@intel.com \
--to=jani.nikula@intel.com \
--cc=igt-dev@lists.freedesktop.org \
--cc=jeevan.b@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox