[Intel-gfx] [PATCH i-g-t] i-g-t: kms_plane_scaling: Enhanced scaling tests

[Daniel Vetter]

On Mon, Nov 27, 2017 at 09:20:33AM +0000, Srinivas, Vidya wrote:
> 
> 
> > -----Original Message-----
> > From: daniel.vetter at ffwll.ch [mailto:daniel.vetter at ffwll.ch] On Behalf Of
> > Daniel Vetter
> > Sent: Monday, November 27, 2017 2:47 PM
> > To: Srinivas, Vidya <vidya.srinivas at intel.com>
> > Cc: intel-gfx <intel-gfx at lists.freedesktop.org>
> > Subject: Re: [Intel-gfx] [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 at intel.com> 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