[igt-dev] [PATCH i-g-t 6/6] tests/kms_async_flips: Test all modifiers
Murthy, Arun R
arun.r.murthy at intel.com
Mon May 15 09:07:55 UTC 2023
> -----Original Message-----
> From: Murthy, Arun R
> Sent: Tuesday, February 7, 2023 11:19 AM
> To: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: igt-dev at lists.freedesktop.org
> Subject: RE: [PATCH i-g-t 6/6] tests/kms_async_flips: Test all modifiers
>
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Sent: Friday, February 3, 2023 6:02 PM
> > To: Murthy, Arun R <arun.r.murthy at intel.com>
> > Cc: igt-dev at lists.freedesktop.org
> > Subject: Re: [PATCH i-g-t 6/6] tests/kms_async_flips: Test all
> > modifiers
> >
> > On Thu, Feb 02, 2023 at 12:24:32PM +0000, Murthy, Arun R wrote:
> > > > -----Original Message-----
> > > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > Sent: Thursday, February 2, 2023 5:46 PM
> > > > To: Murthy, Arun R <arun.r.murthy at intel.com>
> > > > Cc: igt-dev at lists.freedesktop.org
> > > > Subject: Re: [PATCH i-g-t 6/6] tests/kms_async_flips: Test all
> > > > modifiers
> > > >
> > > > On Thu, Feb 02, 2023 at 11:23:09AM +0000, Murthy, Arun R wrote:
> > > > > > -----Original Message-----
> > > > > > From: Ville Syrjala <ville.syrjala at linux.intel.com>
> > > > > > Sent: Tuesday, January 31, 2023 5:37 PM
> > > > > > To: igt-dev at lists.freedesktop.org
> > > > > > Cc: Murthy, Arun R <arun.r.murthy at intel.com>
> > > > > > Subject: [PATCH i-g-t 6/6] tests/kms_async_flips: Test all
> > > > > > modifiers
> > > > > >
> > > > > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > > >
> > > > > > Run the basic async flip test for all modifiers reported by
> > > > > > the plane (for the
> > > > > > XRGB8888 format). The driver may not support async flip with
> > > > > > all modifiers so we allow this to fail.
> > > > >
> > > > > Support for adding X-tile and linear for all platforms is being
> > > > > added as sub-test in
> > > > > https://patchwork.freedesktop.org/series/103137/
> > > > >
> > > > > >
> > > > > > TODO: probably shuould add an even more basic subtest that
> > > > > > makes sure the driver accepts async flips with a least
> > > > > > one format/modifier combo, if it advertises async flip
> > > > > > support
> > > > > >
> > > > > > Cc: Arun R Murthy <arun.r.murthy at intel.com>
> > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > > > ---
> > > > > > tests/kms_async_flips.c | 54
> > > > > > +++++++++++++++++++++++++++++++++--------
> > > > > > 1 file changed, 44 insertions(+), 10 deletions(-)
> > > > > >
> > > > > > diff --git a/tests/kms_async_flips.c b/tests/kms_async_flips.c
> > > > > > index 58740f6ab702..387fcff2f32c 100644
> > > > > > --- a/tests/kms_async_flips.c
> > > > > > +++ b/tests/kms_async_flips.c
> > > > > > @@ -53,6 +53,8 @@ typedef struct {
> > > > > > igt_output_t *output;
> > > > > > unsigned long flip_timestamp_us;
> > > > > > double flip_interval;
> > > > > > + uint64_t modifier;
> > > > > > + igt_plane_t *plane;
> > > > > > igt_pipe_crc_t *pipe_crc;
> > > > > > igt_crc_t ref_crc;
> > > > > > int flip_count;
> > > > > > @@ -61,6 +63,7 @@ typedef struct {
> > > > > > bool extended;
> > > > > > enum pipe pipe;
> > > > > > bool alternate_sync_async;
> > > > > > + bool allow_fail;
> > > > > > } data_t;
> > > > > >
> > > > > > static void flip_handler(int fd_, unsigned int sequence,
> > > > > > unsigned int tv_sec, @@ -133,7 +136,7 @@ static void
> > > > > > make_fb(data_t *data,
> > > > struct igt_fb *fb,
> > > > > > rec_width = width / (ARRAY_SIZE(data->bufs) * 2);
> > > > > >
> > > > > > igt_create_color_fb(data->drm_fd, width, height,
> > > > > > DRM_FORMAT_XRGB8888,
> > > > > > - default_modifier(data), 0.0, 0.0, 0.5, fb);
> > > > > > + data->modifier, 0.0, 0.0, 0.5, fb);
> > > > > >
> > > > > > cr = igt_get_cairo_ctx(data->drm_fd, fb);
> > > > > > igt_paint_color_rand(cr, rec_width * 2 + rec_width * index,
> > > > > > 0, rec_width, height); @@ -159,24 +162,26 @@ static void
> > > > > > test_init(data_t
> > > > > > *data)
> > > > > > data->refresh_rate = mode->vrefresh;
> > > > > >
> > > > > > igt_output_set_pipe(data->output, data->pipe);
> > > > > > +
> > > > > > + data->plane = igt_output_get_plane_type(data->output,
> > > > > > +DRM_PLANE_TYPE_PRIMARY);
> > > > > > }
> > > > > >
> > > > > > static void test_init_fbs(data_t *data) {
> > > > > > int i;
> > > > > > uint32_t width, height;
> > > > > > - igt_plane_t *plane;
> > > > > > static uint32_t prev_output_id;
> > > > > > + static uint64_t prev_modifier;
> > > > > > drmModeModeInfo *mode;
> > > > > >
> > > > > > mode = igt_output_get_mode(data->output);
> > > > > > width = mode->hdisplay;
> > > > > > height = mode->vdisplay;
> > > > > >
> > > > > > - plane = igt_output_get_plane_type(data->output,
> > > > > > DRM_PLANE_TYPE_PRIMARY);
> > > > > > -
> > > > > > - if (prev_output_id != data->output->id) {
> > > > > > + if (prev_output_id != data->output->id ||
> > > > > > + prev_modifier != data->modifier) {
> > > > > > prev_output_id = data->output->id;
> > > > > > + prev_modifier = data->modifier;
> > > > > >
> > > > > > if (data->bufs[0].fb_id) {
> > > > > > for (i = 0; i < ARRAY_SIZE(data->bufs); i++)
> @@ -187,8
> > > > > > +192,8 @@ static void test_init_fbs(data_t *data)
> > > > > > make_fb(data, &data->bufs[i], width, height,
> i);
> > > > > > }
> > > > > >
> > > > > > - igt_plane_set_fb(plane, &data->bufs[0]);
> > > > > > - igt_plane_set_size(plane, width, height);
> > > > > > + igt_plane_set_fb(data->plane, &data->bufs[0]);
> > > > > > + igt_plane_set_size(data->plane, width, height);
> > > > > >
> > > > > > igt_display_commit2(&data->display, data->display.is_atomic
> ?
> > > > > > COMMIT_ATOMIC : COMMIT_LEGACY); } @@ -243,8 +248,10 @@
> > static
> > > > void
> > > > > > test_async_flip(data_t *data)
> > > > > > ret = drmModePageFlip(data->drm_fd, data->crtc_id,
> > > > > > data->bufs[frame % 4].fb_id,
> > > > > > flags, data);
> > > > > > -
> > > > > > - igt_assert(ret == 0);
> > > > > > + if (frame == 1 && data->allow_fail)
> > > > > > + igt_skip_on(ret == -EINVAL);
> > > > > > + else
> > > > > > + igt_assert(ret == 0);
> > > > > >
> > > > > > wait_flip_event(data);
> > > > > >
> > > > > > @@ -545,6 +552,9 @@ static void run_test(data_t *data, void
> > > > > > (*test)(data_t
> > > > > > *))
> > > > > > for_each_pipe_with_valid_output(&data->display, data-
> >pipe,
> > > > > > data-
> > > > > > >output) {
> > > > > > test_init(data);
> > > > > >
> > > > > > + data->allow_fail = false;
> > > > > > + data->modifier = default_modifier(data);
> > > > > > +
> > > > > > igt_dynamic_f("pipe-%s", kmstest_pipe_name(data-
> >pipe)) {
> > > > > > test_init_fbs(data);
> > > > > > test(data);
> > > > > > @@ -555,6 +565,30 @@ static void run_test(data_t *data, void
> > > > > > (*test)(data_t *))
> > > > > > }
> > > > > > }
> > > > > >
> > > > > > +static void run_test_with_modifiers(data_t *data, void
> > > > > > +(*test)(data_t
> > > > > > +*)) {
> > > > > > + for_each_pipe_with_valid_output(&data->display, data-
> > >pipe,
> > > > > > +data-
> > > > > > >output) {
> > > > > > + test_init(data);
> > > > > > +
> > > > > > + for (int i = 0; i < data->plane->format_mod_count;
> > i++) {
> > > > > > + if (data->plane->formats[i] !=
> > > > > > DRM_FORMAT_XRGB8888)
> > > > > > + continue;
> > > > > > +
> > > > > > + data->allow_fail = true;
> > > > > If the modifier is supported by the platform and there is a
> > > > > failure with a
> > > > bug in driver/hardware, aren't we falsely reporting here?
> > > >
> > > > There is no way to query what is supported by the hw/driver, so we
> > > > just have to try them all and accept that not all may succeed.
> > > Yes that right!
> > > Failing for particular modifier not supported would be better than
> > > allowing
> > to escape a failure.
> >
> > It will skip, which if fine IMO.
> I feel its better to add platform related stuff in config so as to include a
> particular modifier(Linear) rather than accepting that all may not succeed.
> Just because a real failure may also loose traction with this.
> I will leave it for others to comment on this.
>
> Thanks and Regards,
> Arun R Murthy
> -------------------
Will take this change in a separate patch.
Reviewed-by: Arun R Murthy <arun.r.murthy at intel.com>
Thanks and Regards,
Arun R Murthy
--------------------
> >
> > > Moreover exceptions can be added for platforms not supporting a
> > particular modifier.
> > > Maybe a proper way of handling the error might be required.
> >
> > Adding i915 speicifc platform checks into the test means we'll have to
> > potentially touch this test for every new platform. Which isn't a good thing.
> >
> > --
> > Ville Syrjälä
> > Intel
More information about the igt-dev
mailing list