[igt-dev] [PATCH i-g-t 1/2] tests/kms_invalid_mode: Convert tests to dynamic

[Petri Latvala]

On Thu, Jun 23, 2022 at 11:43:01AM +0530, Modem, Bhanuprakash wrote:
> On Thu-23-06-2022 11:25 am, Gupta, Nidhi1 wrote:
> > Hi Bhanu,
> > 
> > In many tests we are using drm calls, like drmModePageFlip, drmModeSetCrtc, if I will replace it with igt wrapper it will increase the code lines, should I still change it?
> 
> It's fine to increase the number of lines of code. The idea is to use IGT
> wrappers wherever it's possible, since IGT helpers were developed
> purposefully and more handy.
> 
> Ofcourse, in some tests it is not possible to use these IGT helpers, ex:
> kms_setmode.
> 
> @Petri, any thoughts?

When possible, igt wrappers should be preferred, yes.


-- 
Petri Latvala


> 
> - Bhanu
> 
> > 
> > -----Original Message-----
> > From: Modem, Bhanuprakash <bhanuprakash.modem at intel.com>
> > Sent: Thursday, June 23, 2022 8:11 AM
> > To: Gupta, Nidhi1 <nidhi1.gupta at intel.com>; igt-dev at lists.freedesktop.org
> > Cc: Latvala, Petri <petri.latvala at intel.com>
> > Subject: Re: [PATCH i-g-t 1/2] tests/kms_invalid_mode: Convert tests to dynamic
> > 
> > On Wed-22-06-2022 09:18 pm, Nidhi Gupta wrote:
> > > Convert the existing subtests to dynamic subtests at pipe level.
> > > 
> > > Signed-off-by: Nidhi Gupta <nidhi1.gupta at intel.com>
> > > ---
> > >    tests/kms_invalid_mode.c | 53 +++++++++++++++++-----------------------
> > >    1 file changed, 23 insertions(+), 30 deletions(-)
> > > 
> > > diff --git a/tests/kms_invalid_mode.c b/tests/kms_invalid_mode.c index
> > > 630798d8..7e1f683b 100644
> > > --- a/tests/kms_invalid_mode.c
> > > +++ b/tests/kms_invalid_mode.c
> > > @@ -32,6 +32,7 @@ typedef struct _data data_t;
> > >    struct _data {
> > >    	int drm_fd;
> > > +	enum pipe pipe;
> > >    	igt_display_t display;
> > >    	igt_output_t *output;
> > >    	drmModeResPtr res;
> > > @@ -177,21 +178,21 @@ adjust_mode_bad_vtotal(data_t *data, drmModeModeInfoPtr mode)
> > >    	return true;
> > >    }
> > > -static int
> > > +static void
> > >    test_output(data_t *data)
> > >    {
> > >    	igt_output_t *output = data->output;
> > >    	drmModeModeInfo mode;
> > >    	struct igt_fb fb;
> > > -	int i;
> > > +	int ret;
> > > +	uint32_t crtc_id;
> > >    	/*
> > >    	 * FIXME test every mode we have to be more
> > >    	 * sure everything is really getting rejected?
> > >    	 */
> > >    	mode = *igt_output_get_mode(output);
> > > -	if (!data->adjust_mode(data, &mode))
> > > -		return 0;
> > > +	igt_require(data->adjust_mode(data, &mode));
> > >    	igt_create_fb(data->drm_fd,
> > >    		      max_t(uint16_t, mode.hdisplay, 64), @@ -202,32 +203,14 @@
> > > test_output(data_t *data)
> > >    	kmstest_unset_all_crtcs(data->drm_fd, data->res);
> > > -	for (i = 0; i < data->res->count_crtcs; i++) {
> > > -		int ret;
> > > -
> > > -		igt_info("Checking pipe %c connector %s with mode %s\n",
> > > -			 'A'+i, output->name, mode.name);
> > > +	crtc_id = data->display.pipes[data->pipe].crtc_id;
> > > -		ret = drmModeSetCrtc(data->drm_fd, data->res->crtcs[i],
> > > -				     fb.fb_id, 0, 0,
> > > -				     &output->id, 1, &mode);
> > > -		igt_assert_lt(ret, 0);
> > > -	}
> > > +	ret = drmModeSetCrtc(data->drm_fd, crtc_id,
> > > +			     fb.fb_id, 0, 0,
> > > +			     &output->id, 1, &mode);
> > 
> > Instead of using drm calls, is it possible to use igt kms wrappers?
> > 
> > igt_output_set_pipe();
> > try_commit();
> > 
> > - Bhanu
> > 
> > > +	igt_assert_lt(ret, 0);
> > >    	igt_remove_fb(data->drm_fd, &fb);
> > > -
> > > -	return 1;
> > > -}
> > > -
> > > -static void test(data_t *data)
> > > -{
> > > -	int valid_connectors = 0;
> > > -
> > > -	for_each_connected_output(&data->display, data->output) {
> > > -		valid_connectors += test_output(data);
> > > -	}
> > > -
> > > -	igt_require_f(valid_connectors, "No suitable connectors found\n");
> > >    }
> > >    static int i915_max_dotclock(data_t *data) @@ -297,6 +280,10 @@
> > > static data_t data;
> > >    igt_main
> > >    {
> > > +
> > > +	enum pipe pipe;
> > > +	igt_output_t *output;
> > > +
> > >    	igt_fixture {
> > >    		data.drm_fd = drm_open_driver_master(DRIVER_ANY);
> > > @@ -311,9 +298,15 @@ igt_main
> > >    	}
> > >    	for (int i = 0; i < ARRAY_SIZE(subtests); i++) {
> > > -		igt_subtest(subtests[i].name) {
> > > -			data.adjust_mode = subtests[i].adjust_mode;
> > > -			test(&data);
> > > +		igt_subtest_with_dynamic(subtests[i].name) {
> > > +			for_each_pipe_with_valid_output(&data.display, pipe, output) {
> > > +				igt_dynamic_f("%s-pipe-%s", igt_output_name(output), kmstest_pipe_name(pipe)) {
> > > +					data.output = output;
> > > +					data.pipe = pipe;
> > > +					data.adjust_mode = subtests[i].adjust_mode;
> > > +					test_output(&data);
> > > +				}
> > > +			}
> > >    		}
> > >    	}
> > 
>