| Message ID | 1493390254-5232-8-git-send-email-praveen.paneri@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Sex, 2017-04-28 às 20:07 +0530, Praveen Paneri escreveu: > Now that we have support for Y-tiled buffers, add another > iteration of tests for Y-tiled buffers. Have you tested this on platforms that don't support Y-tiled buffers? I don't see a check for that, so I wonder if we'll just fail some assertion or correctly hit some igt_skip() call I couldn't find. More below. > > Signed-off-by: Praveen Paneri <praveen.paneri@intel.com> > --- > tests/kms_fbc_crc.c | 71 +++++++++++++++++++++++++++++------------ > ------------ > 1 file changed, 39 insertions(+), 32 deletions(-) > > diff --git a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c > index 7964e05..cdf04c1 100644 > --- a/tests/kms_fbc_crc.c > +++ b/tests/kms_fbc_crc.c > @@ -318,12 +318,10 @@ static void prepare_crtc(data_t *data) > igt_output_set_pipe(output, data->pipe); > } > > -static void create_fbs(data_t *data, bool tiled, struct igt_fb *fbs) > +static void create_fbs(data_t *data, uint64_t tiling, struct igt_fb > *fbs) > { > int rc; > drmModeModeInfo *mode = igt_output_get_mode(data->output); > - uint64_t tiling = tiled ? LOCAL_I915_FORMAT_MOD_X_TILED : > - LOCAL_DRM_FORMAT_MOD_NONE; > > rc = igt_create_color_fb(data->drm_fd, mode->hdisplay, mode- > >vdisplay, > DRM_FORMAT_XRGB8888, tiling, > @@ -344,8 +342,8 @@ static void get_ref_crcs(data_t *data) > struct igt_fb fbs[4]; > int i; > > - create_fbs(data, false, &fbs[0]); > - create_fbs(data, false, &fbs[2]); > + create_fbs(data, LOCAL_DRM_FORMAT_MOD_NONE, &fbs[0]); > + create_fbs(data, LOCAL_DRM_FORMAT_MOD_NONE, &fbs[2]); > > fill_mmap_gtt(data, fbs[2].gem_handle, 0xff); > fill_mmap_gtt(data, fbs[3].gem_handle, 0xff); > @@ -366,7 +364,7 @@ static void get_ref_crcs(data_t *data) > igt_remove_fb(data->drm_fd, &fbs[i]); > } > > -static bool prepare_test(data_t *data, enum test_mode test_mode) > +static bool prepare_test(data_t *data, enum test_mode test_mode, > uint64_t tiling) > { > igt_display_t *display = &data->display; > igt_output_t *output = data->output; > @@ -374,7 +372,7 @@ static bool prepare_test(data_t *data, enum > test_mode test_mode) > > data->primary = igt_output_get_plane_type(data->output, > DRM_PLANE_TYPE_PRIMARY); > > - create_fbs(data, true, data->fb); > + create_fbs(data, tiling, data->fb); > > igt_pipe_crc_free(data->pipe_crc); > data->pipe_crc = NULL; > @@ -484,32 +482,41 @@ static void run_test(data_t *data, enum > test_mode mode) > > reset_display(data); > > - for_each_pipe_with_valid_output(display, data->pipe, data- > >output) { > - prepare_crtc(data); > - > - igt_info("Beginning %s on pipe %s, connector %s\n", > - igt_subtest_name(), > - kmstest_pipe_name(data->pipe), > - igt_output_name(data->output)); > - > - if (!prepare_test(data, mode)) { > - igt_info("%s on pipe %s, connector %s: > SKIPPED\n", > - igt_subtest_name(), > - kmstest_pipe_name(data->pipe), > - igt_output_name(data->output)); > - continue; > + for (int tiling = I915_TILING_X; > + tiling <= I915_TILING_Y; tiling++) { What I don't understand is why this part of the code chooses to go with the tiling constants (I915_TILING_) only to later convert them to modifiers with igt_fb_tiling_to_mod(). If this loop iterated over the modifiers directly we wouldn't need that. The rest of the code only cares about the modifiers. > + for_each_pipe_with_valid_output(display, > + data->pipe, data- > >output) { > + prepare_crtc(data); > + > + igt_info("Beginning %s on pipe %s, connector > " > + "%s, %s-tiled\n", > + igt_subtest_name(), > + kmstest_pipe_name(data- > >pipe), > + igt_output_name(data- > >output), > + (tiling == I915_TILING_X) ? > "x":"y" ); This change is not keeping the indentation style, things should be aligned with the parens (although I see they're actually aligned with the quote, which is also weird). The same can be said for the other two igt_info() calls in this patch. > + > + if (!prepare_test(data, mode, > + igt_fb_tiling_to_mod(tilin > g))) { > + igt_info("%s on pipe %s, connector > %s: SKIPPED\n", > + igt_subtest_name(), > + kmstest_pipe_name(da > ta->pipe), > + igt_output_name(data > ->output)); This one is missing the "%s-tiled" part that was added in the other two messages. And we can probably create a "const char *tiling_name" variable to store the %s part in order to avoid the same ternary operator in the 3 if statements. > + continue; > + } > + > + valid_tests++; > + > + test_crc(data, mode); > + > + igt_info("%s on pipe %s, connector %s" > + "%s-tiled: PASSED\n", > + igt_subtest_name(), > + kmstest_pipe_name(data- > >pipe), > + igt_output_name(data- > >output), > + (tiling == I915_TILING_X) ? > "x":"y" ); > + > + finish_crtc(data, mode); > } > - > - valid_tests++; > - > - test_crc(data, mode); > - > - igt_info("%s on pipe %s, connector %s: PASSED\n", > - igt_subtest_name(), > - kmstest_pipe_name(data->pipe), > - igt_output_name(data->output)); > - > - finish_crtc(data, mode); > } > > igt_require_f(valid_tests, "no valid crtc/connector > combinations found\n");
Hi Paulo, On Thursday 13 July 2017 02:31 AM, Paulo Zanoni wrote: > Em Sex, 2017-04-28 às 20:07 +0530, Praveen Paneri escreveu: >> Now that we have support for Y-tiled buffers, add another >> iteration of tests for Y-tiled buffers. > > Have you tested this on platforms that don't support Y-tiled buffers? I Unfortunately I haven't... > don't see a check for that, so I wonder if we'll just fail some > assertion or correctly hit some igt_skip() call I couldn't find. ...but I will add the check as you have mentioned > > More below. > >> >> Signed-off-by: Praveen Paneri <praveen.paneri@intel.com> >> --- >> tests/kms_fbc_crc.c | 71 +++++++++++++++++++++++++++++------------ >> ------------ >> 1 file changed, 39 insertions(+), 32 deletions(-) >> >> diff --git a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c >> index 7964e05..cdf04c1 100644 >> --- a/tests/kms_fbc_crc.c >> +++ b/tests/kms_fbc_crc.c >> @@ -318,12 +318,10 @@ static void prepare_crtc(data_t *data) >> igt_output_set_pipe(output, data->pipe); >> } >> >> -static void create_fbs(data_t *data, bool tiled, struct igt_fb *fbs) >> +static void create_fbs(data_t *data, uint64_t tiling, struct igt_fb >> *fbs) >> { >> int rc; >> drmModeModeInfo *mode = igt_output_get_mode(data->output); >> - uint64_t tiling = tiled ? LOCAL_I915_FORMAT_MOD_X_TILED : >> - LOCAL_DRM_FORMAT_MOD_NONE; >> >> rc = igt_create_color_fb(data->drm_fd, mode->hdisplay, mode- >>> vdisplay, >> DRM_FORMAT_XRGB8888, tiling, >> @@ -344,8 +342,8 @@ static void get_ref_crcs(data_t *data) >> struct igt_fb fbs[4]; >> int i; >> >> - create_fbs(data, false, &fbs[0]); >> - create_fbs(data, false, &fbs[2]); >> + create_fbs(data, LOCAL_DRM_FORMAT_MOD_NONE, &fbs[0]); >> + create_fbs(data, LOCAL_DRM_FORMAT_MOD_NONE, &fbs[2]); >> >> fill_mmap_gtt(data, fbs[2].gem_handle, 0xff); >> fill_mmap_gtt(data, fbs[3].gem_handle, 0xff); >> @@ -366,7 +364,7 @@ static void get_ref_crcs(data_t *data) >> igt_remove_fb(data->drm_fd, &fbs[i]); >> } >> >> -static bool prepare_test(data_t *data, enum test_mode test_mode) >> +static bool prepare_test(data_t *data, enum test_mode test_mode, >> uint64_t tiling) >> { >> igt_display_t *display = &data->display; >> igt_output_t *output = data->output; >> @@ -374,7 +372,7 @@ static bool prepare_test(data_t *data, enum >> test_mode test_mode) >> >> data->primary = igt_output_get_plane_type(data->output, >> DRM_PLANE_TYPE_PRIMARY); >> >> - create_fbs(data, true, data->fb); >> + create_fbs(data, tiling, data->fb); >> >> igt_pipe_crc_free(data->pipe_crc); >> data->pipe_crc = NULL; >> @@ -484,32 +482,41 @@ static void run_test(data_t *data, enum >> test_mode mode) >> >> reset_display(data); >> >> - for_each_pipe_with_valid_output(display, data->pipe, data- >>> output) { >> - prepare_crtc(data); >> - >> - igt_info("Beginning %s on pipe %s, connector %s\n", >> - igt_subtest_name(), >> - kmstest_pipe_name(data->pipe), >> - igt_output_name(data->output)); >> - >> - if (!prepare_test(data, mode)) { >> - igt_info("%s on pipe %s, connector %s: >> SKIPPED\n", >> - igt_subtest_name(), >> - kmstest_pipe_name(data->pipe), >> - igt_output_name(data->output)); >> - continue; >> + for (int tiling = I915_TILING_X; >> + tiling <= I915_TILING_Y; tiling++) { > > What I don't understand is why this part of the code chooses to go with > the tiling constants (I915_TILING_) only to later convert them to > modifiers with igt_fb_tiling_to_mod(). If this loop iterated over the > modifiers directly we wouldn't need that. The rest of the code only > cares about the modifiers. I chose to loop over tiling constants as they are in a simple arithmetic order. anyhow I will just change that. Also as mentioned above can I just add a check to skip Y-tiling tests for older platforms? igt_skip_on(intel_gen(intel_get_drm_devid(drm_fd)) < 9 && tiling == I915_TILING_Y); > > >> + for_each_pipe_with_valid_output(display, >> + data->pipe, data- >>> output) { >> + prepare_crtc(data); >> + >> + igt_info("Beginning %s on pipe %s, connector >> " >> + "%s, %s-tiled\n", >> + igt_subtest_name(), >> + kmstest_pipe_name(data- >>> pipe), >> + igt_output_name(data- >>> output), >> + (tiling == I915_TILING_X) ? >> "x":"y" ); > > This change is not keeping the indentation style, things should be > aligned with the parens (although I see they're actually aligned with > the quote, which is also weird). The same can be said for the other two > igt_info() calls in this patch. Will fix it > > >> + >> + if (!prepare_test(data, mode, >> + igt_fb_tiling_to_mod(tilin >> g))) { >> + igt_info("%s on pipe %s, connector >> %s: SKIPPED\n", >> + igt_subtest_name(), >> + kmstest_pipe_name(da >> ta->pipe), >> + igt_output_name(data >> ->output)); > > This one is missing the "%s-tiled" part that was added in the other two > messages. > > And we can probably create a "const char *tiling_name" variable to > store the %s part in order to avoid the same ternary operator in the 3 > if statements. make sense, will add. > > >> + continue; >> + } >> + >> + valid_tests++; >> + >> + test_crc(data, mode); >> + >> + igt_info("%s on pipe %s, connector %s" >> + "%s-tiled: PASSED\n", >> + igt_subtest_name(), >> + kmstest_pipe_name(data- >>> pipe), >> + igt_output_name(data- >>> output), >> + (tiling == I915_TILING_X) ? >> "x":"y" ); >> + >> + finish_crtc(data, mode); >> } >> - >> - valid_tests++; >> - >> - test_crc(data, mode); >> - >> - igt_info("%s on pipe %s, connector %s: PASSED\n", >> - igt_subtest_name(), >> - kmstest_pipe_name(data->pipe), >> - igt_output_name(data->output)); >> - >> - finish_crtc(data, mode); >> } >> >> igt_require_f(valid_tests, "no valid crtc/connector >> combinations found\n");
Em Sex, 2017-07-14 às 19:25 +0530, Praveen Paneri escreveu: > Hi Paulo, > > On Thursday 13 July 2017 02:31 AM, Paulo Zanoni wrote: > > Em Sex, 2017-04-28 às 20:07 +0530, Praveen Paneri escreveu: > > > Now that we have support for Y-tiled buffers, add another > > > iteration of tests for Y-tiled buffers. > > > > Have you tested this on platforms that don't support Y-tiled > > buffers? I > > Unfortunately I haven't... > > don't see a check for that, so I wonder if we'll just fail some > > assertion or correctly hit some igt_skip() call I couldn't find. > > ...but I will add the check as you have mentioned > > > > More below. > > > > > > > > Signed-off-by: Praveen Paneri <praveen.paneri@intel.com> > > > --- > > > tests/kms_fbc_crc.c | 71 +++++++++++++++++++++++++++++-------- > > > ---- > > > ------------ > > > 1 file changed, 39 insertions(+), 32 deletions(-) > > > > > > diff --git a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c > > > index 7964e05..cdf04c1 100644 > > > --- a/tests/kms_fbc_crc.c > > > +++ b/tests/kms_fbc_crc.c > > > @@ -318,12 +318,10 @@ static void prepare_crtc(data_t *data) > > > igt_output_set_pipe(output, data->pipe); > > > } > > > > > > -static void create_fbs(data_t *data, bool tiled, struct igt_fb > > > *fbs) > > > +static void create_fbs(data_t *data, uint64_t tiling, struct > > > igt_fb > > > *fbs) > > > { > > > int rc; > > > drmModeModeInfo *mode = igt_output_get_mode(data- > > > >output); > > > - uint64_t tiling = tiled ? LOCAL_I915_FORMAT_MOD_X_TILED > > > : > > > - LOCAL_DRM_FORMAT_MOD_NONE; > > > > > > rc = igt_create_color_fb(data->drm_fd, mode->hdisplay, > > > mode- > > > > vdisplay, > > > > > > DRM_FORMAT_XRGB8888, tiling, > > > @@ -344,8 +342,8 @@ static void get_ref_crcs(data_t *data) > > > struct igt_fb fbs[4]; > > > int i; > > > > > > - create_fbs(data, false, &fbs[0]); > > > - create_fbs(data, false, &fbs[2]); > > > + create_fbs(data, LOCAL_DRM_FORMAT_MOD_NONE, &fbs[0]); > > > + create_fbs(data, LOCAL_DRM_FORMAT_MOD_NONE, &fbs[2]); > > > > > > fill_mmap_gtt(data, fbs[2].gem_handle, 0xff); > > > fill_mmap_gtt(data, fbs[3].gem_handle, 0xff); > > > @@ -366,7 +364,7 @@ static void get_ref_crcs(data_t *data) > > > igt_remove_fb(data->drm_fd, &fbs[i]); > > > } > > > > > > -static bool prepare_test(data_t *data, enum test_mode test_mode) > > > +static bool prepare_test(data_t *data, enum test_mode test_mode, > > > uint64_t tiling) > > > { > > > igt_display_t *display = &data->display; > > > igt_output_t *output = data->output; > > > @@ -374,7 +372,7 @@ static bool prepare_test(data_t *data, enum > > > test_mode test_mode) > > > > > > data->primary = igt_output_get_plane_type(data->output, > > > DRM_PLANE_TYPE_PRIMARY); > > > > > > - create_fbs(data, true, data->fb); > > > + create_fbs(data, tiling, data->fb); > > > > > > igt_pipe_crc_free(data->pipe_crc); > > > data->pipe_crc = NULL; > > > @@ -484,32 +482,41 @@ static void run_test(data_t *data, enum > > > test_mode mode) > > > > > > reset_display(data); > > > > > > - for_each_pipe_with_valid_output(display, data->pipe, > > > data- > > > > output) { > > > > > > - prepare_crtc(data); > > > - > > > - igt_info("Beginning %s on pipe %s, connector > > > %s\n", > > > - igt_subtest_name(), > > > - kmstest_pipe_name(data->pipe), > > > - igt_output_name(data->output)); > > > - > > > - if (!prepare_test(data, mode)) { > > > - igt_info("%s on pipe %s, connector %s: > > > SKIPPED\n", > > > - igt_subtest_name(), > > > - kmstest_pipe_name(data->pipe), > > > - igt_output_name(data- > > > >output)); > > > - continue; > > > + for (int tiling = I915_TILING_X; > > > + tiling <= I915_TILING_Y; tiling++) { > > > > What I don't understand is why this part of the code chooses to go > > with > > the tiling constants (I915_TILING_) only to later convert them to > > modifiers with igt_fb_tiling_to_mod(). If this loop iterated over > > the > > modifiers directly we wouldn't need that. The rest of the code only > > cares about the modifiers. > > I chose to loop over tiling constants as they are in a simple > arithmetic > order. anyhow I will just change that. Just put the two local_format stuff in an array and iterate over it. > > Also as mentioned above can I just add a check to skip Y-tiling > tests > for older platforms? > > igt_skip_on(intel_gen(intel_get_drm_devid(drm_fd)) < 9 && > tiling == I915_TILING_Y); You can't do this here because the same subtest tests X tiling. Perhaps we could make Y tiling be a separate subtest? I'm not a huge fan of single tests that do tons of stuff. > > > > > > > > + for_each_pipe_with_valid_output(display, > > > + data->pipe, > > > data- > > > > output) { > > > > > > + prepare_crtc(data); > > > + > > > + igt_info("Beginning %s on pipe %s, > > > connector > > > " > > > + "%s, %s-tiled\n", > > > + igt_subtest_name(), > > > + kmstest_pipe_name(data- > > > > pipe), > > > > > > + igt_output_name(data- > > > > output), > > > > > > + (tiling == > > > I915_TILING_X) ? > > > "x":"y" ); > > > > This change is not keeping the indentation style, things should be > > aligned with the parens (although I see they're actually aligned > > with > > the quote, which is also weird). The same can be said for the other > > two > > igt_info() calls in this patch. > > Will fix it > > > > > > > + > > > + if (!prepare_test(data, mode, > > > + igt_fb_tiling_to_mod(t > > > ilin > > > g))) { > > > + igt_info("%s on pipe %s, > > > connector > > > %s: SKIPPED\n", > > > + igt_subtest_name > > > (), > > > + kmstest_pipe_nam > > > e(da > > > ta->pipe), > > > + igt_output_name( > > > data > > > ->output)); > > > > This one is missing the "%s-tiled" part that was added in the other > > two > > messages. > > > > And we can probably create a "const char *tiling_name" variable to > > store the %s part in order to avoid the same ternary operator in > > the 3 > > if statements. > > make sense, will add. > > > > > > > + continue; > > > + } > > > + > > > + valid_tests++; > > > + > > > + test_crc(data, mode); > > > + > > > + igt_info("%s on pipe %s, connector %s" > > > + "%s-tiled: PASSED\n", > > > + igt_subtest_name(), > > > + kmstest_pipe_name(data- > > > > pipe), > > > > > > + igt_output_name(data- > > > > output), > > > > > > + (tiling == > > > I915_TILING_X) ? > > > "x":"y" ); > > > + > > > + finish_crtc(data, mode); > > > } > > > - > > > - valid_tests++; > > > - > > > - test_crc(data, mode); > > > - > > > - igt_info("%s on pipe %s, connector %s: > > > PASSED\n", > > > - igt_subtest_name(), > > > - kmstest_pipe_name(data->pipe), > > > - igt_output_name(data->output)); > > > - > > > - finish_crtc(data, mode); > > > } > > > > > > igt_require_f(valid_tests, "no valid crtc/connector > > > combinations found\n");
On Friday 14 July 2017 07:55 PM, Paulo Zanoni wrote: > Em Sex, 2017-07-14 às 19:25 +0530, Praveen Paneri escreveu: >> Hi Paulo, >> >> On Thursday 13 July 2017 02:31 AM, Paulo Zanoni wrote: >>> Em Sex, 2017-04-28 às 20:07 +0530, Praveen Paneri escreveu: >>>> Now that we have support for Y-tiled buffers, add another >>>> iteration of tests for Y-tiled buffers. >>> >>> Have you tested this on platforms that don't support Y-tiled >>> buffers? I >> >> Unfortunately I haven't... >>> don't see a check for that, so I wonder if we'll just fail some >>> assertion or correctly hit some igt_skip() call I couldn't find. >> >> ...but I will add the check as you have mentioned >>> >>> More below. >>> >>>> >>>> Signed-off-by: Praveen Paneri <praveen.paneri@intel.com> >>>> --- >>>> tests/kms_fbc_crc.c | 71 +++++++++++++++++++++++++++++-------- >>>> ---- >>>> ------------ >>>> 1 file changed, 39 insertions(+), 32 deletions(-) >>>> >>>> diff --git a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c >>>> index 7964e05..cdf04c1 100644 >>>> --- a/tests/kms_fbc_crc.c >>>> +++ b/tests/kms_fbc_crc.c >>>> @@ -318,12 +318,10 @@ static void prepare_crtc(data_t *data) >>>> igt_output_set_pipe(output, data->pipe); >>>> } >>>> >>>> -static void create_fbs(data_t *data, bool tiled, struct igt_fb >>>> *fbs) >>>> +static void create_fbs(data_t *data, uint64_t tiling, struct >>>> igt_fb >>>> *fbs) >>>> { >>>> int rc; >>>> drmModeModeInfo *mode = igt_output_get_mode(data- >>>>> output); >>>> - uint64_t tiling = tiled ? LOCAL_I915_FORMAT_MOD_X_TILED >>>> : >>>> - LOCAL_DRM_FORMAT_MOD_NONE; >>>> >>>> rc = igt_create_color_fb(data->drm_fd, mode->hdisplay, >>>> mode- >>>>> vdisplay, >>>> >>>> DRM_FORMAT_XRGB8888, tiling, >>>> @@ -344,8 +342,8 @@ static void get_ref_crcs(data_t *data) >>>> struct igt_fb fbs[4]; >>>> int i; >>>> >>>> - create_fbs(data, false, &fbs[0]); >>>> - create_fbs(data, false, &fbs[2]); >>>> + create_fbs(data, LOCAL_DRM_FORMAT_MOD_NONE, &fbs[0]); >>>> + create_fbs(data, LOCAL_DRM_FORMAT_MOD_NONE, &fbs[2]); >>>> >>>> fill_mmap_gtt(data, fbs[2].gem_handle, 0xff); >>>> fill_mmap_gtt(data, fbs[3].gem_handle, 0xff); >>>> @@ -366,7 +364,7 @@ static void get_ref_crcs(data_t *data) >>>> igt_remove_fb(data->drm_fd, &fbs[i]); >>>> } >>>> >>>> -static bool prepare_test(data_t *data, enum test_mode test_mode) >>>> +static bool prepare_test(data_t *data, enum test_mode test_mode, >>>> uint64_t tiling) >>>> { >>>> igt_display_t *display = &data->display; >>>> igt_output_t *output = data->output; >>>> @@ -374,7 +372,7 @@ static bool prepare_test(data_t *data, enum >>>> test_mode test_mode) >>>> >>>> data->primary = igt_output_get_plane_type(data->output, >>>> DRM_PLANE_TYPE_PRIMARY); >>>> >>>> - create_fbs(data, true, data->fb); >>>> + create_fbs(data, tiling, data->fb); >>>> >>>> igt_pipe_crc_free(data->pipe_crc); >>>> data->pipe_crc = NULL; >>>> @@ -484,32 +482,41 @@ static void run_test(data_t *data, enum >>>> test_mode mode) >>>> >>>> reset_display(data); >>>> >>>> - for_each_pipe_with_valid_output(display, data->pipe, >>>> data- >>>>> output) { >>>> >>>> - prepare_crtc(data); >>>> - >>>> - igt_info("Beginning %s on pipe %s, connector >>>> %s\n", >>>> - igt_subtest_name(), >>>> - kmstest_pipe_name(data->pipe), >>>> - igt_output_name(data->output)); >>>> - >>>> - if (!prepare_test(data, mode)) { >>>> - igt_info("%s on pipe %s, connector %s: >>>> SKIPPED\n", >>>> - igt_subtest_name(), >>>> - kmstest_pipe_name(data->pipe), >>>> - igt_output_name(data- >>>>> output)); >>>> - continue; >>>> + for (int tiling = I915_TILING_X; >>>> + tiling <= I915_TILING_Y; tiling++) { >>> >>> What I don't understand is why this part of the code chooses to go >>> with >>> the tiling constants (I915_TILING_) only to later convert them to >>> modifiers with igt_fb_tiling_to_mod(). If this loop iterated over >>> the >>> modifiers directly we wouldn't need that. The rest of the code only >>> cares about the modifiers. >> >> I chose to loop over tiling constants as they are in a simple >> arithmetic >> order. anyhow I will just change that. > > Just put the two local_format stuff in an array and iterate over it. > >> >> Also as mentioned above can I just add a check to skip Y-tiling >> tests >> for older platforms? >> >> igt_skip_on(intel_gen(intel_get_drm_devid(drm_fd)) < 9 && >> tiling == I915_TILING_Y); > > You can't do this here because the same subtest tests X tiling. Okay! what if I just skip the loop for Y tiling with some message? > > Perhaps we could make Y tiling be a separate subtest? I'm not a huge > fan of single tests that do tons of stuff. Will it be possible in just one subtest. I thought we will have to duplicate all the subtests for Y-tile case? Plz suggest Thanks, Praveen > >> >>> >>> >>>> + for_each_pipe_with_valid_output(display, >>>> + data->pipe, >>>> data- >>>>> output) { >>>> >>>> + prepare_crtc(data); >>>> + >>>> + igt_info("Beginning %s on pipe %s, >>>> connector >>>> " >>>> + "%s, %s-tiled\n", >>>> + igt_subtest_name(), >>>> + kmstest_pipe_name(data- >>>>> pipe), >>>> >>>> + igt_output_name(data- >>>>> output), >>>> >>>> + (tiling == >>>> I915_TILING_X) ? >>>> "x":"y" ); >>> >>> This change is not keeping the indentation style, things should be >>> aligned with the parens (although I see they're actually aligned >>> with >>> the quote, which is also weird). The same can be said for the other >>> two >>> igt_info() calls in this patch. >> >> Will fix it >>> >>> >>>> + >>>> + if (!prepare_test(data, mode, >>>> + igt_fb_tiling_to_mod(t >>>> ilin >>>> g))) { >>>> + igt_info("%s on pipe %s, >>>> connector >>>> %s: SKIPPED\n", >>>> + igt_subtest_name >>>> (), >>>> + kmstest_pipe_nam >>>> e(da >>>> ta->pipe), >>>> + igt_output_name( >>>> data >>>> ->output)); >>> >>> This one is missing the "%s-tiled" part that was added in the other >>> two >>> messages. >>> >>> And we can probably create a "const char *tiling_name" variable to >>> store the %s part in order to avoid the same ternary operator in >>> the 3 >>> if statements. >> >> make sense, will add. >>> >>> >>>> + continue; >>>> + } >>>> + >>>> + valid_tests++; >>>> + >>>> + test_crc(data, mode); >>>> + >>>> + igt_info("%s on pipe %s, connector %s" >>>> + "%s-tiled: PASSED\n", >>>> + igt_subtest_name(), >>>> + kmstest_pipe_name(data- >>>>> pipe), >>>> >>>> + igt_output_name(data- >>>>> output), >>>> >>>> + (tiling == >>>> I915_TILING_X) ? >>>> "x":"y" ); >>>> + >>>> + finish_crtc(data, mode); >>>> } >>>> - >>>> - valid_tests++; >>>> - >>>> - test_crc(data, mode); >>>> - >>>> - igt_info("%s on pipe %s, connector %s: >>>> PASSED\n", >>>> - igt_subtest_name(), >>>> - kmstest_pipe_name(data->pipe), >>>> - igt_output_name(data->output)); >>>> - >>>> - finish_crtc(data, mode); >>>> } >>>> >>>> igt_require_f(valid_tests, "no valid crtc/connector >>>> combinations found\n");
diff --git a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c index 7964e05..cdf04c1 100644 --- a/tests/kms_fbc_crc.c +++ b/tests/kms_fbc_crc.c @@ -318,12 +318,10 @@ static void prepare_crtc(data_t *data) igt_output_set_pipe(output, data->pipe); } -static void create_fbs(data_t *data, bool tiled, struct igt_fb *fbs) +static void create_fbs(data_t *data, uint64_t tiling, struct igt_fb *fbs) { int rc; drmModeModeInfo *mode = igt_output_get_mode(data->output); - uint64_t tiling = tiled ? LOCAL_I915_FORMAT_MOD_X_TILED : - LOCAL_DRM_FORMAT_MOD_NONE; rc = igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay, DRM_FORMAT_XRGB8888, tiling, @@ -344,8 +342,8 @@ static void get_ref_crcs(data_t *data) struct igt_fb fbs[4]; int i; - create_fbs(data, false, &fbs[0]); - create_fbs(data, false, &fbs[2]); + create_fbs(data, LOCAL_DRM_FORMAT_MOD_NONE, &fbs[0]); + create_fbs(data, LOCAL_DRM_FORMAT_MOD_NONE, &fbs[2]); fill_mmap_gtt(data, fbs[2].gem_handle, 0xff); fill_mmap_gtt(data, fbs[3].gem_handle, 0xff); @@ -366,7 +364,7 @@ static void get_ref_crcs(data_t *data) igt_remove_fb(data->drm_fd, &fbs[i]); } -static bool prepare_test(data_t *data, enum test_mode test_mode) +static bool prepare_test(data_t *data, enum test_mode test_mode, uint64_t tiling) { igt_display_t *display = &data->display; igt_output_t *output = data->output; @@ -374,7 +372,7 @@ static bool prepare_test(data_t *data, enum test_mode test_mode) data->primary = igt_output_get_plane_type(data->output, DRM_PLANE_TYPE_PRIMARY); - create_fbs(data, true, data->fb); + create_fbs(data, tiling, data->fb); igt_pipe_crc_free(data->pipe_crc); data->pipe_crc = NULL; @@ -484,32 +482,41 @@ static void run_test(data_t *data, enum test_mode mode) reset_display(data); - for_each_pipe_with_valid_output(display, data->pipe, data->output) { - prepare_crtc(data); - - igt_info("Beginning %s on pipe %s, connector %s\n", - igt_subtest_name(), - kmstest_pipe_name(data->pipe), - igt_output_name(data->output)); - - if (!prepare_test(data, mode)) { - igt_info("%s on pipe %s, connector %s: SKIPPED\n", - igt_subtest_name(), - kmstest_pipe_name(data->pipe), - igt_output_name(data->output)); - continue; + for (int tiling = I915_TILING_X; + tiling <= I915_TILING_Y; tiling++) { + for_each_pipe_with_valid_output(display, + data->pipe, data->output) { + prepare_crtc(data); + + igt_info("Beginning %s on pipe %s, connector " + "%s, %s-tiled\n", + igt_subtest_name(), + kmstest_pipe_name(data->pipe), + igt_output_name(data->output), + (tiling == I915_TILING_X) ? "x":"y" ); + + if (!prepare_test(data, mode, + igt_fb_tiling_to_mod(tiling))) { + igt_info("%s on pipe %s, connector %s: SKIPPED\n", + igt_subtest_name(), + kmstest_pipe_name(data->pipe), + igt_output_name(data->output)); + continue; + } + + valid_tests++; + + test_crc(data, mode); + + igt_info("%s on pipe %s, connector %s" + "%s-tiled: PASSED\n", + igt_subtest_name(), + kmstest_pipe_name(data->pipe), + igt_output_name(data->output), + (tiling == I915_TILING_X) ? "x":"y" ); + + finish_crtc(data, mode); } - - valid_tests++; - - test_crc(data, mode); - - igt_info("%s on pipe %s, connector %s: PASSED\n", - igt_subtest_name(), - kmstest_pipe_name(data->pipe), - igt_output_name(data->output)); - - finish_crtc(data, mode); } igt_require_f(valid_tests, "no valid crtc/connector combinations found\n");
Now that we have support for Y-tiled buffers, add another iteration of tests for Y-tiled buffers. Signed-off-by: Praveen Paneri <praveen.paneri@intel.com> --- tests/kms_fbc_crc.c | 71 +++++++++++++++++++++++++++++------------------------ 1 file changed, 39 insertions(+), 32 deletions(-)