[PATCH] tests/kms_universal_plane: Re-work expectations for non-intel hw
Kamil Konieczny
kamil.konieczny at linux.intel.com
Tue Jan 16 09:42:51 UTC 2024
Hi Rob,
On 2024-01-11 at 09:30:08 -0800, Rob Clark wrote:
> On Thu, Jan 11, 2024 at 5:37 AM Kamil Konieczny
> <kamil.konieczny at linux.intel.com> wrote:
> >
> > Hi Rob,
> > On 2024-01-10 at 12:04:34 -0800, Rob Clark wrote:
> > > From: Rob Clark <robdclark at chromium.org>
> > >
> > > The expectation assumptions do not fit for non-intel hw. So re-work the
> > > expectations and add msm support.
> > >
> > > Signed-off-by: Rob Clark <robdclark at chromium.org>
> > > ---
> > > tests/kms_universal_plane.c | 102 +++++++++++++++++++++++++-----------
> > > 1 file changed, 72 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/tests/kms_universal_plane.c b/tests/kms_universal_plane.c
> > > index 6a39f93cc5aa..3158ff816046 100644
> > > --- a/tests/kms_universal_plane.c
> > > +++ b/tests/kms_universal_plane.c
> > > @@ -402,8 +402,7 @@ sanity_test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
> > > sanity_test_t test = { .data = data };
> > > igt_plane_t *primary;
> > > drmModeModeInfo *mode;
> > > - int i;
> > > - int expect = 0;
> >
> > Could you keep this var?
> >
> > > + int i, ret;
> > >
> > > igt_output_set_pipe(output, pipe);
> > > mode = igt_output_get_mode(output);
> > > @@ -418,43 +417,86 @@ sanity_test_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
> > >
> > > /*
> > > * Try to use universal plane API to set primary plane that
> > > - * doesn't cover CRTC (should fail on pre-gen9 and succeed on
> > > - * gen9+).
> > > + * doesn't cover CRTC.
> > > */
> > > igt_plane_set_fb(primary, &test.undersized_fb);
> > > - if (is_intel_device(data->drm_fd))
> > > - expect = (data->display_ver < 9) ? -EINVAL : 0;
> > > - igt_assert(igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL) == expect);
> > > + ret = igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL);
> > > +
> > > + if (is_intel_device(data->drm_fd)) {
> > > + /* should fail on pre-gen9 and succeed on gen9+: */
> > > + if (data->display_ver < 9)
> > > + igt_assert_eq(ret, -EINVAL);
> > > + else
> > > + igt_assert_eq(ret, 0);
> > > + } else if (is_msm_device(data->drm_fd)) {
> > > + /* Should be supported on all gens: */
> > > + igt_assert_eq(ret, 0);
> > > + } else {
> > > + igt_assert_eq(ret, 0);
> > > + }
> >
> > The old code above should work on msm, why do you changed it?
>
> The reason for this change, and several other similar but (for msm)
> non-functionally-changing hunks was to document the expectations for
> msm and to give folks working on other drivers a guide about where to
> insert their specific expectations.
>
> The alternative, I think, if we don't want to keep adding driver
> specifics is to just change all cases to allow either success or the
> correct error code.
>
> BR,
> -R
>
imho we should leave it as is and introduce changes when they will be
needed and keep lines of code low. If someone will have other error
it can be instroduced with before:
if (is_intel_device(data->drm_fd))
expect = (data->display_ver < 9) ? -EINVAL : 0;
else if (is_msm_device(data->drm_fd))
As this is display code I will add Juha-Pekka to Cc.
> > > - expect = (data->display_ver < 9) ? -EINVAL : 0;
> > > - igt_assert(igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL) == expect);
> > > + ret = igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL);
> >
> > >
> > > - /* Same as above, but different plane positioning. */
> > > + /* Same as above, but different plane positioning: */
> >
> > Drop this change.
> >
> > > igt_plane_set_position(primary, 100, 100);
> > > - igt_assert(igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL) == expect);
> > > + ret = igt_display_try_commit2(&data->display, COMMIT_UNIVERSAL);
> > > +
> > > + if (is_intel_device(data->drm_fd)) {
> > > + /* should fail on pre-gen9 and succeed on gen9+: */
> > > + if (data->display_ver < 9)
> > > + igt_assert_eq(ret, -EINVAL);
> > > + else
> > > + igt_assert_eq(ret, 0);
> > > + } else if (is_msm_device(data->drm_fd)) {
> > > + /* Should be supported on all gens: */
> > > + igt_assert_eq(ret, 0);
> > > + } else {
> > > + igt_assert_eq(ret, 0);
> > > + }
> >
> > Same here, old code should just work for msm.
> >
> > >
> > > igt_plane_set_position(primary, 0, 0);
> > >
> > > - /* Try to use universal plane API to scale down (should fail on pre-gen9) */
> > > - if (is_intel_device(data->drm_fd))
> > > - expect = (data->display_ver < 9) ? -ERANGE : 0;
> >
> > Please keep this if(...) expect = ...
> > for a reason which follows.
> >
> > > - igt_assert(drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> > > - output->config.crtc->crtc_id,
> > > - test.oversized_fb.fb_id, 0,
> > > - 0, 0,
> > > - mode->hdisplay + 100,
> > > - mode->vdisplay + 100,
> > > - IGT_FIXED(0,0), IGT_FIXED(0,0),
> > > - IGT_FIXED(mode->hdisplay,0),
> > > - IGT_FIXED(mode->vdisplay,0)) == expect);
> > > + /* Try to use universal plane API to scale down */
> > > + ret = drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> > > + output->config.crtc->crtc_id,
> > > + test.oversized_fb.fb_id, 0,
> > > + 0, 0,
> > > + mode->hdisplay + 100,
> > > + mode->vdisplay + 100,
> > > + IGT_FIXED(0,0), IGT_FIXED(0,0),
> > > + IGT_FIXED(mode->hdisplay,0),
> > > + IGT_FIXED(mode->vdisplay,0));
> > > +
> > > + if (is_intel_device(data->drm_fd)) {
> > > + /* should fail on pre-gen9 and succeed on gen9+: */
> > > + if (data->display_ver < 9)
> > > + igt_assert_eq(ret, -ERANGE);
> > > + else
> > > + igt_assert_eq(ret, 0);
> >
> > Or just simple
> > igt_assert_eq(ret, expect);
> >
> > > + } else {
> > > + /* Could succeed or fail with -ERANGE depending on hw limits: */
> > > + igt_assert((ret == 0) || (ret == -ERANGE));
Could you read/discover those limits and check for actual
expected error?
Regards,
Kamil
> > > + }
> >
> > This is a real change, it relaxed checks for non-intel HW.
> > imho we should ask others, for example amd developers if that is ok
> > for them.
> >
> > >
> > > /* Try to use universal plane API to scale up (should fail on pre-gen9) */
> > > - igt_assert(drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> > > - output->config.crtc->crtc_id,
> > > - test.oversized_fb.fb_id, 0,
> > > - 0, 0,
> > > - mode->hdisplay,
> > > - mode->vdisplay,
> > > - IGT_FIXED(0,0), IGT_FIXED(0,0),
> > > - IGT_FIXED(mode->hdisplay - 100,0),
> > > - IGT_FIXED(mode->vdisplay - 100,0)) == expect);
> > > + ret = drmModeSetPlane(data->drm_fd, primary->drm_plane->plane_id,
> > > + output->config.crtc->crtc_id,
> > > + test.oversized_fb.fb_id, 0,
> > > + 0, 0,
> > > + mode->hdisplay,
> > > + mode->vdisplay,
> > > + IGT_FIXED(0,0), IGT_FIXED(0,0),
> > > + IGT_FIXED(mode->hdisplay - 100,0),
> > > + IGT_FIXED(mode->vdisplay - 100,0));
> > > +
> > > + if (is_intel_device(data->drm_fd)) {
> > > + /* should fail on pre-gen9 and succeed on gen9+: */
> > > + if (data->display_ver < 9)
> > > + igt_assert_eq(ret, -ERANGE);
> > > + else
> > > + igt_assert_eq(ret, 0);
> >
> > Same here, just simple:
> > igt_assert_eq(ret, expect);
> >
> > Regards,
> > Kamil
> >
> > > + } else {
> > > + /* Could succeed or fail with -ERANGE depending on hw limits: */
> > > + igt_assert((ret == 0) || (ret == -ERANGE));
> > > + }
> > >
> > > /* Find other crtcs and try to program our primary plane on them */
> > > for (i = 0; i < test.moderes->count_crtcs; i++)
> > > --
> > > 2.43.0
> > >
More information about the igt-dev
mailing list