[igt-dev] [PATCH i-g-t] tests/kms_setmode: Fix mode selection for Nx tests

[Modem, Bhanuprakash]

> From: Nautiyal, Ankit K <ankit.k.nautiyal at intel.com>
> Sent: Wednesday, July 28, 2021 5:01 PM
> To: Modem, Bhanuprakash <bhanuprakash.modem at intel.com>; igt-
> dev at lists.freedesktop.org; Deak, Imre <imre.deak at intel.com>
> Subject: Re: [PATCH i-g-t] tests/kms_setmode: Fix mode selection for Nx tests
> 
> Hi Bhanu,
> 
> Overall the change looks good to me.
> 
> Only one thing that we might want to change is:
> 
> We are currently, trying all modes of Display 2 with first mode of
> Display 1.
> 
> If we exhaust all the modes of Display2, we should go with the 2nd mode
> of Display1 and try again.
> 
> Instead we just return from that point.
> 
> Consider a hypothetical case: Say we have DP and eDP combination mapped
> to pipe A and pipe B.
> 
> In that case, first we will try first for DP (pipe A) and then try all
> modes of eDP.
> 
> In case of eDP is having fixed mode, we get out of recursion after
> trying the one fixed mode of Display2 and just fail the test, instead of
> trying with lower modes of DP.
> 
> Please find inline suggestion to fix this:
> 
> 
> On 5/26/2021 1:35 AM, Bhanuprakash Modem wrote:
> > This patch will find the connector/mode combination that fits
> > into the bandwidth when more than one monitor is connected.
> >
> > Example:
> >    When two monitors connected through MST, the second monitor
> >    also tries to use the same mode. So two such modes may not
> >    fit into the link bandwidth. So, iterate through connected
> >    outputs & modes and find a combination of modes those fit
> >    into the link BW.
> >
> > Cc: Imre Deak <imre.deak at intel.com>
> > Cc: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
> > Signed-off-by: Bhanuprakash Modem <bhanuprakash.modem at intel.com>
> > ---
> >   tests/kms_setmode.c | 95 ++++++++++++++++++++++++++++++++-------------
> >   1 file changed, 68 insertions(+), 27 deletions(-)
> >
> > diff --git a/tests/kms_setmode.c b/tests/kms_setmode.c
> > index eb9ac18964..93ccb829e3 100644
> > --- a/tests/kms_setmode.c
> > +++ b/tests/kms_setmode.c
> > @@ -46,6 +46,9 @@ static int filter_test_id;
> >   static bool dry_run;
> >   static bool all_pipes = false;
> >
> > +static char str_buf[MAX_CRTCS][1024];
> > +static const char *crtc_strs[MAX_CRTCS];
> > +
> >   const drmModeModeInfo mode_640_480 = {
> >   	.name		= "640x480",
> >   	.vrefresh	= 60,
> > @@ -540,44 +543,46 @@ static void check_timings(int crtc_idx, const
> drmModeModeInfo *kmode)
> >   		     fabs(mean - expected) / line_time(kmode));
> >   }
> >
> > -static void test_crtc_config(const struct test_config *tconf,
> > -			     struct crtc_config *crtcs, int crtc_count)
> > +static int sort_drm_modes(const void *a, const void *b)
> >   {
> > -	char str_buf[MAX_CRTCS][1024];
> > -	const char *crtc_strs[MAX_CRTCS];
> > -	struct crtc_config *crtc;
> > -	static int test_id;
> > -	bool config_failed = false;
> > -	int ret = 0;
> > -	int i;
> > +	const drmModeModeInfo *mode1 = a, *mode2 = b;
> >
> > -	test_id++;
> > +	return (mode1->clock < mode2->clock) - (mode2->clock < mode1->clock);
> > +}
> >
> > -	if (filter_test_id && filter_test_id != test_id)
> > -		return;
> > +static
> > +int __test_crtc_config(struct crtc_config *crtcs, int crtc_count,
> > +		       const struct test_config *tconf, bool *config_failed,
> > +		       int base)
> > +{
> > +	struct crtc_config *crtc = NULL;
> > +	int ret = 0;
> >
> > -	igt_info("  Test id#%d CRTC count %d\n", test_id, crtc_count);
> > +	if (base >= crtc_count)
> > +		return ret;
> >
> > -	for (i = 0; i < crtc_count; i++) {
> > -		get_crtc_config_str(&crtcs[i], str_buf[i], sizeof(str_buf[i]));
> > -		crtc_strs[i] = &str_buf[i][0];
> > -	}
> > +	crtc = &crtcs[base];
> >
> > -	if (dry_run) {
> > -		for (i = 0; i < crtc_count; i++)
> > -			igt_info("    %s\n", crtc_strs[i]);
> > -		return;
> > -	}
> > +	/* Sort the modes in descending order by clock freq. */
> > +	qsort(crtc->cconfs->connector->modes,
> > +		crtc->cconfs->connector->count_modes,
> > +		sizeof(drmModeModeInfo),
> > +		sort_drm_modes);
> >
> > -	for (i = 0; i < crtc_count; i++) {
> > +	for (int i = 0; i < crtc->cconfs->connector->count_modes; i++) {
> >   		uint32_t *ids;
> >
> > -		crtc = &crtcs[i];
> > +		if (!crtc_supports_mode(crtc, &crtc->cconfs->connector->modes[i]))
> > +			continue;
> > +
> > +		crtc->mode = crtc->cconfs->connector->modes[i];
> >
> > -		igt_info("    %s\n", crtc_strs[i]);
> > +		get_crtc_config_str(crtc, str_buf[base], sizeof(str_buf[base]));
> > +		crtc_strs[base] = &str_buf[base][0];
> > +		igt_info("    %s\n", crtc_strs[base]);
> >
> >   		create_fb_for_crtc(crtc, &crtc->fb_info);
> > -		paint_fb(&crtc->fb_info, tconf->name, crtc_strs, crtc_count, i);
> > +		paint_fb(&crtc->fb_info, tconf->name, crtc_strs, crtc_count, base);
> >
> >   		ids = get_connector_ids(crtc);
> >   		if (tconf->flags & TEST_STEALING)
> > @@ -589,12 +594,48 @@ static void test_crtc_config(const struct test_config
> *tconf,
> >
> >   		free(ids);
> >
> > +		if (ret < 0 && errno == ENOSPC)
> > +			continue;
> > +
> >   		if (ret < 0) {
> >   			igt_assert_eq(errno, EINVAL);
> > -			config_failed = true;
> > +			*config_failed = true;
> >   		}
> > +
> > +		return __test_crtc_config(crtcs, crtc_count, tconf, config_failed,
> base + 1);
> 
> IMHO this should be:
> 
> ret = __test_crtc_config(crtcs, crtc_count, tconf, config_failed, base + 1);

Thanks for the review Ankit, I have gone through the logic again.

As recursion calling is inside the loop (list of display modes), It can iterate
over the all modes on each connected display.

And yes, there is a problem with this logic. Even though we find the valid
connector & mode combination, recursion will continue & endup with the minimum
modes. I am trying to find a way to break the recursion when we find the valid
combination. If possible, can you please suggest any solution?

- Bhanu

> 
> if (ret < 0 && errno == ENOSPC)
> 
>        continue; //continue with next mode of current CRTC
> 
> else
> 
>        return ret;
> 
> 
> Regards,
> 
> Ankit
> 
> 
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static void test_crtc_config(const struct test_config *tconf,
> > +			     struct crtc_config *crtcs, int crtc_count)
> > +{
> > +	static int test_id;
> > +	bool config_failed = false;
> > +	int ret = 0;
> > +	int i;
> > +
> > +	test_id++;
> > +
> > +	if (filter_test_id && filter_test_id != test_id)
> > +		return;
> > +
> > +	igt_info("  Test id#%d CRTC count %d\n", test_id, crtc_count);
> > +
> > +	for (i = 0; i < crtc_count; i++) {
> > +		get_crtc_config_str(&crtcs[i], str_buf[i], sizeof(str_buf[i]));
> > +		crtc_strs[i] = &str_buf[i][0];
> >   	}
> >
> > +	if (dry_run) {
> > +		for (i = 0; i < crtc_count; i++)
> > +			igt_info("    %s\n", crtc_strs[i]);
> > +		return;
> > +	}
> > +
> > +	ret = __test_crtc_config(crtcs, crtc_count, tconf, &config_failed, 0);
> > +
> >   	igt_assert(config_failed == !!(tconf->flags & TEST_INVALID));
> >
> >   	if (ret == 0 && tconf->flags & TEST_TIMINGS)