[Intel-gfx] [PATCH i-g-t 4/6] i-g-t kms_plane_scaling: test scaling with tiling rotation and pixel formats

[Srinivas, Vidya]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Thursday, December 14, 2017 4:25 PM
> To: Srinivas, Vidya <vidya.srinivas at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; Vetter, Daniel <daniel.vetter at intel.com>
> Subject: Re: [Intel-gfx] [PATCH i-g-t 4/6] i-g-t kms_plane_scaling: test
> scaling with tiling rotation and pixel formats
> 
> Just a quick comment at the bottom.
> 
> On Wed, Dec 13, 2017 at 03:20:50PM +0530, Vidya Srinivas wrote:
> > @@ -312,23 +480,40 @@ static void test_plane_scaling(data_t *d, enum
> pipe pipe)
> >  	igt_require_f(valid_tests, "no valid crtc/connector combinations
> > found\n");  }
> >
> > -igt_simple_main
> > +igt_main
> >  {
> >  	data_t data = {};
> >  	enum pipe pipe;
> >
> >  	igt_skip_on_simulation();
> >
> > -
> > -	data.drm_fd = drm_open_driver(DRIVER_INTEL);
> > -	igt_require_pipe_crc(data.drm_fd);
> > -	igt_display_init(&data.display, data.drm_fd);
> > -	data.devid = intel_get_drm_devid(data.drm_fd);
> > +	igt_fixture {
> > +		data.drm_fd = drm_open_driver(DRIVER_INTEL);
> > +		kmstest_set_vt_graphics_mode();
> > +		igt_require_pipe_crc(data.drm_fd);
> > +		igt_display_init(&data.display, data.drm_fd);
> > +		data.devid = intel_get_drm_devid(data.drm_fd);
> > +		igt_require_gem(data.drm_fd);
> > +	}
> >
> >  	data.num_scalers = intel_gen(data.devid) >= 9 ? 2 : 0;
> 
> Hm, would be nice if we could get rid of this hard-coded platform
> knowledge and autodiscover. But atm the kernel doesn't expose a
> "can_scale" property unfortunately. Maybe add a FIXME comment?
> 
> Real reason I'm commenting here: You probably need to put this into the
> igt_fixture too, since otherwise you're test won't run correctly when just
> enumerating tests.
> 
> >
> > -	for_each_pipe_static(pipe)
> > -		test_plane_scaling(&data, pipe);
> > +	for_each_pipe_static(pipe) {
> > +
> > +		igt_subtest_f("scaler_basic_test") {
> > +			test_plane_scaling(&data, pipe);
> > +		}
> > +
> > +		igt_subtest_f("scaler_with_pixel_format") {
> > +			test_scaler_with_pixel_format(&data, pipe);
> > +		}
> >
> > -	igt_display_fini(&data.display);
> > +		igt_subtest_f("scaler_with_rotation") {
> > +			test_scaler_with_rotation(&data, pipe);
> > +		}
> > +
> > +		igt_fixture {
> > +			igt_display_fini(&data.display);
> > +		}
> > +	}
> 
> Just a quick drive-by: You probably want to convert to subtest-based testing
> as the very first patch (without any functional tests). In your current series
> you add more subtests while still using igt_simple_main, which will blow
> up.
> 
> The goal of a split up patch series isn't just to make review easier, but also
> testing: Every single step in your series is supposed to be a fully functional
> codebase. When you're not much experienced with building up such a patch
> series, it's good practice to double-check that before sending. I tend to use
> git rebase -x $test-script to automate that if possible. That way you can
> make sure that every single step in your patch series builds (cleanly!) and
> results in a working testcases (which should only change results if your patch
> aims to do that, not as some side-effect).

Thank you. Sorry if there is any mistake here. But when we have added subtests we have changed it to igt_main and not simple.
Before that we have not added subtests. We followed whatever you suggested - as in, first patch would correct just whatever is
Not working as of now. Then in the later tests we added new subtests with enhancements. We ran all the tests once (I am sorry,
Not the way you mentioned) fully and it ran on our APL board.

Regards
Vidya

> 
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch