Re: [igt-dev] [PATCH] [i-g-t, v2] tests/kms_cursor_crc: skip pipe on invalid connector in cursor size test

[Petri Latvala <petri.latvala@intel.com>]

On Mon, Jan 23, 2023 at 11:58:44AM +0200, Bhadane, Dnyaneshwar wrote:
> Hello Petri,
> > -----Original Message-----
> > From: Latvala, Petri <petri.latvala@intel.com>
> > Sent: Monday, January 16, 2023 8:12 PM
> > To: Bhadane, Dnyaneshwar <dnyaneshwar.bhadane@intel.com>
> > Cc: igt-dev@lists.freedesktop.org; Kurmi, Suresh Kumar
> > <suresh.kumar.kurmi@intel.com>; Heikkila, Juha-pekka <juha-
> > pekka.heikkila@intel.com>
> > Subject: Re: [igt-dev] [PATCH] [i-g-t, v2] tests/kms_cursor_crc: skip pipe on
> > invalid connector in cursor size test
> > 
> > On Mon, Jan 16, 2023 at 08:02:19PM +0530, Dnyaneshwar Bhadane wrote:
> > > Only the valid pipe connector combination reach to the igt commit.
> > > Cursor max-size test will not affect existing flow as only skip for
> > > invalid connector.
> > > For cursor-dpms and cursor-suspend not require to check
> > > require_cursor_size becuase the cursor height and width used from drm
> > capablities.
> > >
> > > --v2
> > > - Used for_each_pipe_with_single_output() to iterate for valid pipe and
> > output.
> > >
> > > Signed-off-by: Dnyaneshwar Bhadane <dnyaneshwar.bhadane@intel.com>
> > > ---
> > >  tests/kms_cursor_crc.c | 16 +++-------------
> > >  1 file changed, 3 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c index
> > > d5a4b30b..a4afcff3 100644
> > > --- a/tests/kms_cursor_crc.c
> > > +++ b/tests/kms_cursor_crc.c
> > > @@ -749,7 +749,7 @@ static void run_size_tests(data_t *data, int w, int h)
> > >  				}
> > >  			}
> > >
> > > -			for_each_pipe(&data->display, pipe) {
> > > +			for_each_pipe_with_single_output(&data->display,
> > pipe,
> > > +data->output) {
> > >  				data->pipe = pipe;
> > >
> > >  				if (require_cursor_size(data, w, h)) { @@ -
> > 850,15 +850,10 @@
> > > static void run_tests_on_pipe(data_t *data)
> > >
> > >  	igt_describe("Check random placement of a cursor with DPMS.");
> > >  	igt_subtest_with_dynamic("cursor-dpms") {
> > > -		for_each_pipe(&data->display, pipe) {
> > > +		for_each_pipe_with_single_output(&data->display, pipe,
> > > +data->output) {
> > >  			data->pipe = pipe;
> > >  			data->flags = TEST_DPMS;
> > >
> > > -			if (require_cursor_size(data, data->cursor_max_w,
> > data->cursor_max_h)) {
> > > -				igt_debug("Cursor size %dx%d not supported by
> > driver\n",
> > > -					  data->cursor_max_w, data-
> > >cursor_max_h);
> > > -				continue;
> > > -			}
> > >
> > >  			igt_dynamic_f("pipe-%s-%s",
> > >  				      kmstest_pipe_name(pipe),
> > > @@ -871,15 +866,10 @@ static void run_tests_on_pipe(data_t *data)
> > >
> > >  	igt_describe("Check random placement of a cursor with suspend.");
> > >  	igt_subtest_with_dynamic("cursor-suspend") {
> > > -		for_each_pipe(&data->display, pipe) {
> > > +		for_each_pipe_with_single_output(&data->display, pipe,
> > > +data->output) {
> > >  			data->pipe = pipe;
> > >  			data->flags = TEST_SUSPEND;
> > >
> > > -			if (require_cursor_size(data, data->cursor_max_w,
> > data->cursor_max_h)) {
> > > -				igt_debug("Cursor size %dx%d not supported by
> > driver\n",
> > > -					  data->cursor_max_w, data-
> > >cursor_max_h);
> > > -				continue;
> > > -			}
> > 
> > 
> > These will effectively overwrite whatever data->output was set to in the fixture
> > in run_tests_on_pipe. But, how did that fixture ever work?
> > That fixture leaves 'pipe' variable uninitialized and then finds an output for that
> > pipe?
> > 
> The pipe variable is default taking value 0 as it enum.

It's accidentally 0, because CI builds with the default of
buildtype=debugoptimized. It's an automatic variable, not a global.


-- 
Petri Latvala