[igt-dev] [PATCH i-g-t 5/8] tests/i915/kms_flip_tiling: Generalize away copy-pasta
Ville Syrjälä
ville.syrjala at linux.intel.com
Mon Oct 18 07:28:30 UTC 2021
On Mon, Oct 18, 2021 at 12:32:18PM +0530, Karthik B S wrote:
> On 10/14/2021 3:47 AM, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >
> > Replace the huge swaths of copypasta by just looping over the
> > set of modifiers reported by the plane, and testing each against
> > the others (and itself).
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> > tests/i915/kms_flip_tiling.c | 209 +++++------------------------------
> > 1 file changed, 30 insertions(+), 179 deletions(-)
> >
> > diff --git a/tests/i915/kms_flip_tiling.c b/tests/i915/kms_flip_tiling.c
> > index 63e0b8b9648f..e2e6619baeca 100644
> > --- a/tests/i915/kms_flip_tiling.c
> > +++ b/tests/i915/kms_flip_tiling.c
> > @@ -161,195 +161,46 @@ igt_main
> > igt_display_require(&data.display, data.drm_fd);
> > }
> >
> > - /*
> > - * Test that a page flip from a tiled buffer to a linear one works
> > - * correctly. First, it sets the crtc with the linear buffer and
> > - * generates a reference crc for the pipe. Then, the crtc is set with
> > - * the tiled one and page flip to the linear one issued. A new crc is
> > - * generated and compared to the reference one.
> > - */
> > -
> > - igt_describe("Check pageflip from tiled buffer to linear one works correctly with x tiling");
> > - igt_subtest_with_dynamic("flip-changes-tiling") {
> > - uint64_t modifier[2] = { I915_FORMAT_MOD_X_TILED,
> > - DRM_FORMAT_MOD_LINEAR };
> > + igt_describe("Check pageflip between modifiers");
> > + igt_subtest_with_dynamic("flip-change-tiling") {
> > enum pipe pipe;
> >
> > - for (int i = 0; i < ARRAY_SIZE(modifier); i++)
> > - igt_require(igt_display_has_format_mod(&data.display, data.testformat, modifier[i]));
> > -
> > for_each_pipe_with_valid_output(&data.display, pipe, output) {
> > - igt_dynamic_f("%s-pipe-%s", igt_output_name(output), kmstest_pipe_name(pipe))
> > - test_flip_tiling(&data, pipe, output, modifier);
> > - test_cleanup(&data, pipe, output);
> > - }
> > - }
> > -
> > - igt_describe("Check pageflip from tiled buffer to linear one works correctly with y tiling");
> > - igt_subtest_with_dynamic("flip-changes-tiling-Y") {
> > - uint64_t modifier[2] = { I915_FORMAT_MOD_Y_TILED,
> > - DRM_FORMAT_MOD_LINEAR };
> > - enum pipe pipe;
> > + igt_plane_t *plane;
> >
> > - igt_require_fb_modifiers(data.drm_fd);
> > + igt_output_set_pipe(output, pipe);
> >
> > - for (int i = 0; i < ARRAY_SIZE(modifier); i++)
> > - igt_require(igt_display_has_format_mod(&data.display, data.testformat, modifier[i]));
> > + plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
> >
> > - igt_require(data.gen >= 9);
> > + for (int i = 0; i < plane->format_mod_count; i++) {
> > + if (plane->formats[i] != data.testformat)
> > + continue;
> >
> > - for_each_pipe_with_valid_output(&data.display, pipe, output) {
> > - igt_dynamic_f("%s-pipe-%s", igt_output_name(output), kmstest_pipe_name(pipe))
> > - test_flip_tiling(&data, pipe, output, modifier);
> > - test_cleanup(&data, pipe, output);
> > - }
> > - }
> > -
> > - igt_describe("Check pageflip from tiled buffer to linear one works correctly with yf tiling");
> > - igt_subtest_with_dynamic("flip-changes-tiling-Yf") {
> > - uint64_t modifier[2] = { I915_FORMAT_MOD_Yf_TILED,
> > - DRM_FORMAT_MOD_LINEAR };
> > - enum pipe pipe;
> > + for (int j = 0; j < plane->format_mod_count; j++) {
> > + uint64_t modifier[2] = {
> > + plane->modifiers[i],
> > + plane->modifiers[j],
> > + };
> >
> > - igt_require_fb_modifiers(data.drm_fd);
> > + if (plane->formats[j] != data.testformat)
> > + continue;
> Move this check before assigning modifier[]?
Then I'd have to split the declaration and assignment.
> >
> > - for (int i = 0; i < ARRAY_SIZE(modifier); i++)
> > - igt_require(igt_display_has_format_mod(&data.display, data.testformat, modifier[i]));
> > -
> > - igt_require(data.gen >= 9);
> > -
> > - for_each_pipe_with_valid_output(&data.display, pipe, output) {
> > - igt_dynamic_f("%s-pipe-%s", igt_output_name(output), kmstest_pipe_name(pipe))
> > - test_flip_tiling(&data, pipe, output, modifier);
> > - test_cleanup(&data, pipe, output);
> > - }
> > - }
> > -
> > - /*
> > - * Test that a page flip from a tiled buffer to another tiled one works
> > - * correctly. First, it sets the crtc with the tiled buffer and
> > - * generates a reference crc for the pipe. Then a page flip to second
> > - * tiled buffer is issued. A new crc is generated and compared to the
> > - * reference one.
> > - */
> > -
> > - igt_describe("Check pageflip from tiled buffer to another tiled one works correctly with x tiling");
> > - igt_subtest_with_dynamic("flip-X-tiled") {
> > - uint64_t modifier[2] = { I915_FORMAT_MOD_X_TILED,
> > - I915_FORMAT_MOD_X_TILED };
> > - enum pipe pipe;
> > -
> > - for (int i = 0; i < ARRAY_SIZE(modifier); i++)
> > - igt_require(igt_display_has_format_mod(&data.display, data.testformat, modifier[i]));
> > -
> > - for_each_pipe_with_valid_output(&data.display, pipe, output) {
> > - igt_dynamic_f("%s-pipe-%s", igt_output_name(output), kmstest_pipe_name(pipe))
> > - test_flip_tiling(&data, pipe, output, modifier);
> > - test_cleanup(&data, pipe, output);
> > - }
> > - }
> > -
> > - igt_describe("Check pageflip from tiled buffer to another tiled one works correctly with y tiling");
> > - igt_subtest_with_dynamic("flip-Y-tiled") {
> > - uint64_t modifier[2] = { I915_FORMAT_MOD_Y_TILED,
> > - I915_FORMAT_MOD_Y_TILED };
> > - enum pipe pipe;
> > -
> > - igt_require_fb_modifiers(data.drm_fd);
> > -
> > - for (int i = 0; i < ARRAY_SIZE(modifier); i++)
> > - igt_require(igt_display_has_format_mod(&data.display, data.testformat, modifier[i]));
> > -
> > - igt_require(data.gen >= 9);
> > -
> > - for_each_pipe_with_valid_output(&data.display, pipe, output) {
> > - igt_dynamic_f("%s-pipe-%s", igt_output_name(output), kmstest_pipe_name(pipe))
> > - test_flip_tiling(&data, pipe, output, modifier);
> > - test_cleanup(&data, pipe, output);
> > - }
> > - }
> > -
> > - igt_describe("Check pageflip from tiled buffer to another tiled one works correctly with yf tiling");
> > - igt_subtest_with_dynamic("flip-Yf-tiled") {
> > - uint64_t modifier[2] = { I915_FORMAT_MOD_Yf_TILED,
> > - I915_FORMAT_MOD_Yf_TILED };
> > - enum pipe pipe;
> > + igt_require(igt_plane_has_format_mod(plane,
> > + data.testformat,
> > + modifier[0]));
> > + igt_require(igt_plane_has_format_mod(plane,
> > + data.testformat,
> > + modifier[1]));
>
> Is this still required? We are already only taking modifiers which are
> supported by this format? Please correct me if I'm missing something here.
Not missing anything, these can go. We already checked for this by hand.
Originally I forgot that plane->modifiers[]+plane->formats[] actually
form a SoA tuple, so I just iterated plane->modifiers[] without skipping
anything, thinking that would just iterate each modifier once. And with
that apporach I would have still needed the igt_plane_has_format_mod().
However as I rediscovered the truth I then added the manual
plane->format[] checks which make these redundant.
>
> With these minor updates, the patch looks good to me.
>
> Reviewed-by: Karthik B S <karthik.b.s at intel.com>
Thanks.
--
Ville Syrjälä
Intel
More information about the igt-dev
mailing list