On Mon, Nov 27, 2017 at 09:20:33AM +0000, Srinivas, Vidya wrote: > > > > -----Original Message----- > > From: daniel.vetter@xxxxxxxx [mailto:daniel.vetter@xxxxxxxx] On Behalf Of > > Daniel Vetter > > Sent: Monday, November 27, 2017 2:47 PM > > To: Srinivas, Vidya <vidya.srinivas@xxxxxxxxx> > > Cc: intel-gfx <intel-gfx@xxxxxxxxxxxxxxxxxxxxx> > > Subject: Re: [PATCH i-g-t] i-g-t: kms_plane_scaling: Enhanced > > scaling tests > > > > Somehow I forgot to send out my irc feedback to the m-l. > > > > On Wed, Nov 22, 2017 at 9:15 AM, Vidya Srinivas > > <vidya.srinivas@xxxxxxxxx> wrote: > > > +igt_main > > > { > > > data_t data = {}; > > > > > > @@ -308,11 +765,26 @@ igt_simple_main > > > data.drm_fd = drm_open_driver(DRIVER_INTEL); > > > igt_require_pipe_crc(data.drm_fd); > > > igt_display_init(&data.display, data.drm_fd); > > > + igt_require(data.display.is_atomic); > > > data.devid = intel_get_drm_devid(data.drm_fd); > > > > > > data.num_scalers = intel_gen(data.devid) >= 9 ? 2 : 0; > > > > > > - test_plane_scaling(&data); > > > - > > > + //test_plane_scaling(&data); You're commenting out the existing testcase ... > > > + igt_subtest_f("scaler_with_pixel_format") { > > > + test_scaler_with_pixel_format(&data); > > > + } > > > + igt_subtest_f("scaler_with_rotation") { > > > + test_scaler_with_rotation(&data); > > > + } > > > + igt_subtest_f("scaler_with_multiple_planes") { > > > + test_scaler_with_multiple_planes(&data); > > > + } > > > + igt_subtest_f("scaler_with_clipping_clamping") { > > > + test_scaler_with_clipping_clamping_scenario(&data); > > > + } > > > + igt_subtest_f("scaler_with_multi_pipe_plane") { > > > + test_scaler_with_multi_pipe_plane(&data); > > > + } > > > > Commenting out the existing testcase and replacing it with new ones > > entirely, without explaining what's wrong with the old one, or removing it > > (we have git for source control, not comments), and how new tests are > > better isn't how we do things. Presumably the existing tests once worked, so > > the first step should be to fix that up first and explain why the changes are > > necessary. When this code was typed 2 years ago someone put some > > thought into it, throwing all that work away isn't good. > > > > And _then_ (in follow-up patches) fix the gaps in test coverage. > > > > Thanks, Daniel > > Thank you. I think the explanation was missing. This test with format > will enhance the existing cases. I will work on these and re-submit the same. Which means it's not enhancing it, it's outright remove it and replacing with something new. Either you need to show that the existing testcase is totally broken (with a patch that removes it and explain why it's unfixable). Or (and that's really the preferred thing), patch 1 in this series fixes up the existing test as the first step. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
