* [igt-dev] [PATCH i-g-t] igt/tests: Fix error checking in kms_atomic_transition
@ 2019-02-13 16:18 Stanislav Lisovskiy via igt-dev
2019-02-13 17:45 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Stanislav Lisovskiy via igt-dev @ 2019-02-13 16:18 UTC (permalink / raw)
To: igt-dev; +Cc: ville.syrjala, martin.peres, juhapekka.heikkila
There is no guarantee that error return value will be
always EINVAL, made a check more general as it can be
ERANGE, ENOSPC, EINVAL and probably others, which all
mean the same in context of this test case: i.e this sprite
size is not valid.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109225
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
tests/kms_atomic_transition.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/kms_atomic_transition.c b/tests/kms_atomic_transition.c
index ec5d25de..58bf6280 100644
--- a/tests/kms_atomic_transition.c
+++ b/tests/kms_atomic_transition.c
@@ -282,7 +282,7 @@ retry:
wm_setup_plane(display, pipe, (1 << n_planes) - 1, parms, false);
ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
- if (ret == -EINVAL) {
+ if (ret != 0) {
if (cursor_width == sprite_width &&
cursor_height == sprite_height) {
igt_assert_f(alpha,
@@ -472,7 +472,7 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
igt_pipe_request_out_fence(pipe_obj);
ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
- if (ret != -EINVAL || pipe_obj->n_planes < 3)
+ if (ret == 0 || pipe_obj->n_planes < 3)
break;
ret = 0;
--
2.17.1
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [igt-dev] ✓ Fi.CI.BAT: success for igt/tests: Fix error checking in kms_atomic_transition
2019-02-13 16:18 [igt-dev] [PATCH i-g-t] igt/tests: Fix error checking in kms_atomic_transition Stanislav Lisovskiy via igt-dev
@ 2019-02-13 17:45 ` Patchwork
2019-02-13 20:41 ` [igt-dev] [PATCH i-g-t] " Daniel Vetter
2019-02-14 9:28 ` [igt-dev] ✓ Fi.CI.IGT: success for " Patchwork
2 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2019-02-13 17:45 UTC (permalink / raw)
To: igt-dev
== Series Details ==
Series: igt/tests: Fix error checking in kms_atomic_transition
URL : https://patchwork.freedesktop.org/series/56617/
State : success
== Summary ==
CI Bug Log - changes from IGT_4824 -> IGTPW_2396
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/56617/revisions/1/mbox/
Possible new issues
-------------------
Here are the unknown changes that may have been introduced in IGTPW_2396:
### IGT changes ###
#### Suppressed ####
The following results come from untrusted machines, tests, or statuses.
They do not affect the overall result.
* igt@i915_selftest@live_workarounds:
- {fi-icl-u3}: PASS -> INCOMPLETE
Known issues
------------
Here are the changes found in IGTPW_2396 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_exec_suspend@basic-s4-devices:
- fi-blb-e6850: NOTRUN -> INCOMPLETE [fdo#107718]
* igt@i915_selftest@live_execlists:
- fi-apl-guc: PASS -> INCOMPLETE [fdo#103927]
* igt@kms_flip@basic-flip-vs-modeset:
- fi-skl-6700hq: PASS -> DMESG-WARN [fdo#105998] +1
#### Possible fixes ####
* igt@gem_exec_suspend@basic-s3:
- fi-blb-e6850: INCOMPLETE [fdo#107718] -> PASS
* igt@i915_selftest@live_workarounds:
- {fi-icl-u2}: INCOMPLETE -> PASS
* igt@kms_busy@basic-flip-c:
- fi-kbl-7500u: {SKIP} [fdo#109271] / [fdo#109278] -> PASS +2
* igt@kms_chamelium@dp-hpd-fast:
- fi-kbl-7500u: DMESG-WARN [fdo#102505] / [fdo#103558] / [fdo#105602] -> PASS
* igt@kms_chamelium@hdmi-hpd-fast:
- fi-kbl-7500u: FAIL [fdo#109485] -> PASS
* igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
- fi-hsw-4770: {SKIP} [fdo#109271] -> PASS +3
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#102505]: https://bugs.freedesktop.org/show_bug.cgi?id=102505
[fdo#103558]: https://bugs.freedesktop.org/show_bug.cgi?id=103558
[fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
[fdo#105602]: https://bugs.freedesktop.org/show_bug.cgi?id=105602
[fdo#105998]: https://bugs.freedesktop.org/show_bug.cgi?id=105998
[fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
[fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
[fdo#108622]: https://bugs.freedesktop.org/show_bug.cgi?id=108622
[fdo#109226]: https://bugs.freedesktop.org/show_bug.cgi?id=109226
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
[fdo#109485]: https://bugs.freedesktop.org/show_bug.cgi?id=109485
[fdo#109567]: https://bugs.freedesktop.org/show_bug.cgi?id=109567
Participating hosts (45 -> 40)
------------------------------
Missing (5): fi-kbl-soraka fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-bdw-samus
Build changes
-------------
* IGT: IGT_4824 -> IGTPW_2396
CI_DRM_5598: 0d4420ccb7b8a3386fefc3719e71b5e8e69d3abb @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_2396: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2396/
IGT_4824: e55d439a9ba744227fb4c9d727338276b78871d4 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2396/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] igt/tests: Fix error checking in kms_atomic_transition
2019-02-13 16:18 [igt-dev] [PATCH i-g-t] igt/tests: Fix error checking in kms_atomic_transition Stanislav Lisovskiy via igt-dev
2019-02-13 17:45 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
@ 2019-02-13 20:41 ` Daniel Vetter
2019-02-14 8:22 ` Lisovskiy, Stanislav via igt-dev
2019-02-14 9:28 ` [igt-dev] ✓ Fi.CI.IGT: success for " Patchwork
2 siblings, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2019-02-13 20:41 UTC (permalink / raw)
To: Stanislav Lisovskiy
Cc: igt-dev, juhapekka.heikkila, ville.syrjala, martin.peres
On Wed, Feb 13, 2019 at 06:18:17PM +0200, Stanislav Lisovskiy via igt-dev wrote:
> There is no guarantee that error return value will be
> always EINVAL, made a check more general as it can be
> ERANGE, ENOSPC, EINVAL and probably others, which all
> mean the same in context of this test case: i.e this sprite
> size is not valid.
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109225
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Kernel atomic spec is pretty clear that it's either EINVAL or ERANGE, and
nothing else. In the docs we even limit to EINVAL (scroll down to
atomic_check):
https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html?highlight=drm_mode_config_funcs#c.drm_mode_config_funcs
Would be great if you can submit a kernel patch to add the ERANGE. And
change this one here to only accept ERANGE and EINVAL as "sprite doesn't
work".
-Daniel
> ---
> tests/kms_atomic_transition.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tests/kms_atomic_transition.c b/tests/kms_atomic_transition.c
> index ec5d25de..58bf6280 100644
> --- a/tests/kms_atomic_transition.c
> +++ b/tests/kms_atomic_transition.c
> @@ -282,7 +282,7 @@ retry:
> wm_setup_plane(display, pipe, (1 << n_planes) - 1, parms, false);
> ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
>
> - if (ret == -EINVAL) {
> + if (ret != 0) {
> if (cursor_width == sprite_width &&
> cursor_height == sprite_height) {
> igt_assert_f(alpha,
> @@ -472,7 +472,7 @@ run_transition_test(igt_display_t *display, enum pipe pipe, igt_output_t *output
> igt_pipe_request_out_fence(pipe_obj);
>
> ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> - if (ret != -EINVAL || pipe_obj->n_planes < 3)
> + if (ret == 0 || pipe_obj->n_planes < 3)
> break;
>
> ret = 0;
> --
> 2.17.1
>
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] igt/tests: Fix error checking in kms_atomic_transition
2019-02-13 20:41 ` [igt-dev] [PATCH i-g-t] " Daniel Vetter
@ 2019-02-14 8:22 ` Lisovskiy, Stanislav via igt-dev
2019-02-14 8:25 ` Daniel Vetter
0 siblings, 1 reply; 10+ messages in thread
From: Lisovskiy, Stanislav via igt-dev @ 2019-02-14 8:22 UTC (permalink / raw)
To: daniel@ffwll.ch
Cc: igt-dev@lists.freedesktop.org, juhapekka.heikkila@intel.com,
Syrjala, Ville, Peres, Martin
On Wed, 2019-02-13 at 21:41 +0100, Daniel Vetter wrote:
> On Wed, Feb 13, 2019 at 06:18:17PM +0200, Stanislav Lisovskiy via
> igt-dev wrote:
> > There is no guarantee that error return value will be
> > always EINVAL, made a check more general as it can be
> > ERANGE, ENOSPC, EINVAL and probably others, which all
> > mean the same in context of this test case: i.e this sprite
> > size is not valid.
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109225
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
>
> Kernel atomic spec is pretty clear that it's either EINVAL or ERANGE,
> and
> nothing else. In the docs we even limit to EINVAL (scroll down to
> atomic_check):
>
> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html?highlight=drm_m
> ode_config_funcs#c.drm_mode_config_funcs
>
> Would be great if you can submit a kernel patch to add the ERANGE.
> And
> change this one here to only accept ERANGE and EINVAL as "sprite
> doesn't
> work".
Unfortunately, in reality it is not like that. We have a bug
fdo#109225, where it returns ENOSPC. DRM does this when plane size
exceeds the framebuffer size(check drm_atomic.c,
drm_atomic_plane_check).
We hit this issue when we happen to have resolution lower than expected
by IGT, so we actually create a framebuffer of smaller size than it
sets for the plane and then we
hit ENOSPC instead EINVAL, which is treated now as OK by the test case.
So adding ERANGE will not help for this bug.
Either I should add all three to IGT(ERANGE, ENOSPC, EINVAL) or fix it
in the kernel, so that it returns EINVAL instead of ENOSPC.
> -Daniel
>
> > ---
> > tests/kms_atomic_transition.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/tests/kms_atomic_transition.c
> > b/tests/kms_atomic_transition.c
> > index ec5d25de..58bf6280 100644
> > --- a/tests/kms_atomic_transition.c
> > +++ b/tests/kms_atomic_transition.c
> > @@ -282,7 +282,7 @@ retry:
> > wm_setup_plane(display, pipe, (1 << n_planes) - 1,
> > parms, false);
> > ret = igt_display_try_commit_atomic(display,
> > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> >
> > - if (ret == -EINVAL) {
> > + if (ret != 0) {
> > if (cursor_width == sprite_width &&
> > cursor_height == sprite_height) {
> > igt_assert_f(alpha,
> > @@ -472,7 +472,7 @@ run_transition_test(igt_display_t *display,
> > enum pipe pipe, igt_output_t *output
> > igt_pipe_request_out_fence(pipe_obj);
> >
> > ret = igt_display_try_commit_atomic(display,
> > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> > - if (ret != -EINVAL || pipe_obj->n_planes < 3)
> > + if (ret == 0 || pipe_obj->n_planes < 3)
> > break;
> >
> > ret = 0;
> > --
> > 2.17.1
> >
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
>
>
--
Best Regards,
Lisovskiy Stanislav
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] igt/tests: Fix error checking in kms_atomic_transition
2019-02-14 8:22 ` Lisovskiy, Stanislav via igt-dev
@ 2019-02-14 8:25 ` Daniel Vetter
2019-02-14 8:31 ` Lisovskiy, Stanislav via igt-dev
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2019-02-14 8:25 UTC (permalink / raw)
To: Lisovskiy, Stanislav
Cc: igt-dev@lists.freedesktop.org, juhapekka.heikkila@intel.com,
Syrjala, Ville, Peres, Martin, daniel@ffwll.ch
On Thu, Feb 14, 2019 at 08:22:29AM +0000, Lisovskiy, Stanislav wrote:
> On Wed, 2019-02-13 at 21:41 +0100, Daniel Vetter wrote:
> > On Wed, Feb 13, 2019 at 06:18:17PM +0200, Stanislav Lisovskiy via
> > igt-dev wrote:
> > > There is no guarantee that error return value will be
> > > always EINVAL, made a check more general as it can be
> > > ERANGE, ENOSPC, EINVAL and probably others, which all
> > > mean the same in context of this test case: i.e this sprite
> > > size is not valid.
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109225
> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> >
> > Kernel atomic spec is pretty clear that it's either EINVAL or ERANGE,
> > and
> > nothing else. In the docs we even limit to EINVAL (scroll down to
> > atomic_check):
> >
> > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html?highlight=drm_m
> > ode_config_funcs#c.drm_mode_config_funcs
> >
> > Would be great if you can submit a kernel patch to add the ERANGE.
> > And
> > change this one here to only accept ERANGE and EINVAL as "sprite
> > doesn't
> > work".
>
> Unfortunately, in reality it is not like that. We have a bug
> fdo#109225, where it returns ENOSPC. DRM does this when plane size
> exceeds the framebuffer size(check drm_atomic.c,
> drm_atomic_plane_check).
> We hit this issue when we happen to have resolution lower than expected
> by IGT, so we actually create a framebuffer of smaller size than it
> sets for the plane and then we
> hit ENOSPC instead EINVAL, which is treated now as OK by the test case.
> So adding ERANGE will not help for this bug.
> Either I should add all three to IGT(ERANGE, ENOSPC, EINVAL) or fix it
> in the kernel, so that it returns EINVAL instead of ENOSPC.
Hm yeah, pls do, and pls do update the kernel documentation. I'm also
wondering whether we shouldn't wrap some checks around calls to
atomic_check in the kernel for this.
-Daniel
>
> > -Daniel
> >
> > > ---
> > > tests/kms_atomic_transition.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tests/kms_atomic_transition.c
> > > b/tests/kms_atomic_transition.c
> > > index ec5d25de..58bf6280 100644
> > > --- a/tests/kms_atomic_transition.c
> > > +++ b/tests/kms_atomic_transition.c
> > > @@ -282,7 +282,7 @@ retry:
> > > wm_setup_plane(display, pipe, (1 << n_planes) - 1,
> > > parms, false);
> > > ret = igt_display_try_commit_atomic(display,
> > > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> > >
> > > - if (ret == -EINVAL) {
> > > + if (ret != 0) {
> > > if (cursor_width == sprite_width &&
> > > cursor_height == sprite_height) {
> > > igt_assert_f(alpha,
> > > @@ -472,7 +472,7 @@ run_transition_test(igt_display_t *display,
> > > enum pipe pipe, igt_output_t *output
> > > igt_pipe_request_out_fence(pipe_obj);
> > >
> > > ret = igt_display_try_commit_atomic(display,
> > > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> > > - if (ret != -EINVAL || pipe_obj->n_planes < 3)
> > > + if (ret == 0 || pipe_obj->n_planes < 3)
> > > break;
> > >
> > > ret = 0;
> > > --
> > > 2.17.1
> > >
> > > _______________________________________________
> > > igt-dev mailing list
> > > igt-dev@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> >
> >
> --
> Best Regards,
>
> Lisovskiy Stanislav
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] igt/tests: Fix error checking in kms_atomic_transition
2019-02-14 8:25 ` Daniel Vetter
@ 2019-02-14 8:31 ` Lisovskiy, Stanislav via igt-dev
2019-02-14 8:33 ` Daniel Vetter
0 siblings, 1 reply; 10+ messages in thread
From: Lisovskiy, Stanislav via igt-dev @ 2019-02-14 8:31 UTC (permalink / raw)
To: daniel@ffwll.ch
Cc: igt-dev@lists.freedesktop.org, juhapekka.heikkila@intel.com,
Syrjala, Ville, Peres, Martin
On Thu, 2019-02-14 at 09:25 +0100, Daniel Vetter wrote:
> On Thu, Feb 14, 2019 at 08:22:29AM +0000, Lisovskiy, Stanislav wrote:
> > On Wed, 2019-02-13 at 21:41 +0100, Daniel Vetter wrote:
> > > On Wed, Feb 13, 2019 at 06:18:17PM +0200, Stanislav Lisovskiy via
> > > igt-dev wrote:
> > > > There is no guarantee that error return value will be
> > > > always EINVAL, made a check more general as it can be
> > > > ERANGE, ENOSPC, EINVAL and probably others, which all
> > > > mean the same in context of this test case: i.e this sprite
> > > > size is not valid.
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109225
> > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.c
> > > > om>
> > >
> > > Kernel atomic spec is pretty clear that it's either EINVAL or
> > > ERANGE,
> > > and
> > > nothing else. In the docs we even limit to EINVAL (scroll down to
> > > atomic_check):
> > >
> > > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html?highlight=d
> > > rm_m
> > > ode_config_funcs#c.drm_mode_config_funcs
> > >
> > > Would be great if you can submit a kernel patch to add the
> > > ERANGE.
> > > And
> > > change this one here to only accept ERANGE and EINVAL as "sprite
> > > doesn't
> > > work".
> >
> > Unfortunately, in reality it is not like that. We have a bug
> > fdo#109225, where it returns ENOSPC. DRM does this when plane size
> > exceeds the framebuffer size(check drm_atomic.c,
> > drm_atomic_plane_check).
> > We hit this issue when we happen to have resolution lower than
> > expected
> > by IGT, so we actually create a framebuffer of smaller size than it
> > sets for the plane and then we
> > hit ENOSPC instead EINVAL, which is treated now as OK by the test
> > case.
> > So adding ERANGE will not help for this bug.
> > Either I should add all three to IGT(ERANGE, ENOSPC, EINVAL) or fix
> > it
> > in the kernel, so that it returns EINVAL instead of ENOSPC.
>
> Hm yeah, pls do, and pls do update the kernel documentation. I'm also
> wondering whether we shouldn't wrap some checks around calls to
> atomic_check in the kernel for this.
> -Daniel
So do you mean I should fix the kernel or igt? :)
>
> >
> > > -Daniel
> > >
> > > > ---
> > > > tests/kms_atomic_transition.c | 4 ++--
> > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/tests/kms_atomic_transition.c
> > > > b/tests/kms_atomic_transition.c
> > > > index ec5d25de..58bf6280 100644
> > > > --- a/tests/kms_atomic_transition.c
> > > > +++ b/tests/kms_atomic_transition.c
> > > > @@ -282,7 +282,7 @@ retry:
> > > > wm_setup_plane(display, pipe, (1 << n_planes)
> > > > - 1,
> > > > parms, false);
> > > > ret = igt_display_try_commit_atomic(display,
> > > > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET,
> > > > NULL);
> > > >
> > > > - if (ret == -EINVAL) {
> > > > + if (ret != 0) {
> > > > if (cursor_width == sprite_width &&
> > > > cursor_height == sprite_height) {
> > > > igt_assert_f(alpha,
> > > > @@ -472,7 +472,7 @@ run_transition_test(igt_display_t *display,
> > > > enum pipe pipe, igt_output_t *output
> > > > igt_pipe_request_out_fence(pipe_obj);
> > > >
> > > > ret = igt_display_try_commit_atomic(display,
> > > > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET,
> > > > NULL);
> > > > - if (ret != -EINVAL || pipe_obj->n_planes < 3)
> > > > + if (ret == 0 || pipe_obj->n_planes < 3)
> > > > break;
> > > >
> > > > ret = 0;
> > > > --
> > > > 2.17.1
> > > >
> > > > _______________________________________________
> > > > igt-dev mailing list
> > > > igt-dev@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> > >
> > >
> >
> > --
> > Best Regards,
> >
> > Lisovskiy Stanislav
>
>
--
Best Regards,
Lisovskiy Stanislav
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] igt/tests: Fix error checking in kms_atomic_transition
2019-02-14 8:31 ` Lisovskiy, Stanislav via igt-dev
@ 2019-02-14 8:33 ` Daniel Vetter
2019-02-14 12:14 ` Lisovskiy, Stanislav via igt-dev
0 siblings, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2019-02-14 8:33 UTC (permalink / raw)
To: Lisovskiy, Stanislav
Cc: igt-dev@lists.freedesktop.org, juhapekka.heikkila@intel.com,
Syrjala, Ville, Peres, Martin, daniel@ffwll.ch
On Thu, Feb 14, 2019 at 08:31:52AM +0000, Lisovskiy, Stanislav wrote:
> On Thu, 2019-02-14 at 09:25 +0100, Daniel Vetter wrote:
> > On Thu, Feb 14, 2019 at 08:22:29AM +0000, Lisovskiy, Stanislav wrote:
> > > On Wed, 2019-02-13 at 21:41 +0100, Daniel Vetter wrote:
> > > > On Wed, Feb 13, 2019 at 06:18:17PM +0200, Stanislav Lisovskiy via
> > > > igt-dev wrote:
> > > > > There is no guarantee that error return value will be
> > > > > always EINVAL, made a check more general as it can be
> > > > > ERANGE, ENOSPC, EINVAL and probably others, which all
> > > > > mean the same in context of this test case: i.e this sprite
> > > > > size is not valid.
> > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109225
> > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.c
> > > > > om>
> > > >
> > > > Kernel atomic spec is pretty clear that it's either EINVAL or
> > > > ERANGE,
> > > > and
> > > > nothing else. In the docs we even limit to EINVAL (scroll down to
> > > > atomic_check):
> > > >
> > > > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html?highlight=d
> > > > rm_m
> > > > ode_config_funcs#c.drm_mode_config_funcs
> > > >
> > > > Would be great if you can submit a kernel patch to add the
> > > > ERANGE.
> > > > And
> > > > change this one here to only accept ERANGE and EINVAL as "sprite
> > > > doesn't
> > > > work".
> > >
> > > Unfortunately, in reality it is not like that. We have a bug
> > > fdo#109225, where it returns ENOSPC. DRM does this when plane size
> > > exceeds the framebuffer size(check drm_atomic.c,
> > > drm_atomic_plane_check).
> > > We hit this issue when we happen to have resolution lower than
> > > expected
> > > by IGT, so we actually create a framebuffer of smaller size than it
> > > sets for the plane and then we
> > > hit ENOSPC instead EINVAL, which is treated now as OK by the test
> > > case.
> > > So adding ERANGE will not help for this bug.
> > > Either I should add all three to IGT(ERANGE, ENOSPC, EINVAL) or fix
> > > it
> > > in the kernel, so that it returns EINVAL instead of ENOSPC.
> >
> > Hm yeah, pls do, and pls do update the kernel documentation. I'm also
> > wondering whether we shouldn't wrap some checks around calls to
> > atomic_check in the kernel for this.
> > -Daniel
>
> So do you mean I should fix the kernel or igt? :)
Oops :-) I meant to fix the igt logic to also accept ENOSPC and EINVAL,
plus update the kernel's documentation so that docs reflect reality.
-Daniel
>
> >
> > >
> > > > -Daniel
> > > >
> > > > > ---
> > > > > tests/kms_atomic_transition.c | 4 ++--
> > > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/tests/kms_atomic_transition.c
> > > > > b/tests/kms_atomic_transition.c
> > > > > index ec5d25de..58bf6280 100644
> > > > > --- a/tests/kms_atomic_transition.c
> > > > > +++ b/tests/kms_atomic_transition.c
> > > > > @@ -282,7 +282,7 @@ retry:
> > > > > wm_setup_plane(display, pipe, (1 << n_planes)
> > > > > - 1,
> > > > > parms, false);
> > > > > ret = igt_display_try_commit_atomic(display,
> > > > > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET,
> > > > > NULL);
> > > > >
> > > > > - if (ret == -EINVAL) {
> > > > > + if (ret != 0) {
> > > > > if (cursor_width == sprite_width &&
> > > > > cursor_height == sprite_height) {
> > > > > igt_assert_f(alpha,
> > > > > @@ -472,7 +472,7 @@ run_transition_test(igt_display_t *display,
> > > > > enum pipe pipe, igt_output_t *output
> > > > > igt_pipe_request_out_fence(pipe_obj);
> > > > >
> > > > > ret = igt_display_try_commit_atomic(display,
> > > > > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET,
> > > > > NULL);
> > > > > - if (ret != -EINVAL || pipe_obj->n_planes < 3)
> > > > > + if (ret == 0 || pipe_obj->n_planes < 3)
> > > > > break;
> > > > >
> > > > > ret = 0;
> > > > > --
> > > > > 2.17.1
> > > > >
> > > > > _______________________________________________
> > > > > igt-dev mailing list
> > > > > igt-dev@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> > > >
> > > >
> > >
> > > --
> > > Best Regards,
> > >
> > > Lisovskiy Stanislav
> >
> >
> --
> Best Regards,
>
> Lisovskiy Stanislav
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 10+ messages in thread
* [igt-dev] ✓ Fi.CI.IGT: success for igt/tests: Fix error checking in kms_atomic_transition
2019-02-13 16:18 [igt-dev] [PATCH i-g-t] igt/tests: Fix error checking in kms_atomic_transition Stanislav Lisovskiy via igt-dev
2019-02-13 17:45 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
2019-02-13 20:41 ` [igt-dev] [PATCH i-g-t] " Daniel Vetter
@ 2019-02-14 9:28 ` Patchwork
2 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2019-02-14 9:28 UTC (permalink / raw)
To: igt-dev
== Series Details ==
Series: igt/tests: Fix error checking in kms_atomic_transition
URL : https://patchwork.freedesktop.org/series/56617/
State : success
== Summary ==
CI Bug Log - changes from IGT_4824_full -> IGTPW_2396_full
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/56617/revisions/1/mbox/
Known issues
------------
Here are the changes found in IGTPW_2396_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@kms_atomic_transition@plane-all-modeset-transition:
- shard-apl: PASS -> INCOMPLETE [fdo#103927]
* igt@kms_color@pipe-a-legacy-gamma:
- shard-kbl: PASS -> FAIL [fdo#104782] / [fdo#108145]
- shard-apl: PASS -> FAIL [fdo#104782] / [fdo#108145]
* igt@kms_color@pipe-c-ctm-max:
- shard-apl: PASS -> FAIL [fdo#108147]
* igt@kms_cursor_crc@cursor-128x128-onscreen:
- shard-kbl: PASS -> FAIL [fdo#103232] +2
* igt@kms_cursor_crc@cursor-256x85-onscreen:
- shard-glk: NOTRUN -> FAIL [fdo#103232]
* igt@kms_cursor_crc@cursor-64x21-random:
- shard-apl: PASS -> FAIL [fdo#103232] +3
* igt@kms_cursor_crc@cursor-alpha-opaque:
- shard-kbl: PASS -> FAIL [fdo#109350]
- shard-apl: PASS -> FAIL [fdo#109350]
- shard-glk: PASS -> FAIL [fdo#109350]
* igt@kms_cursor_legacy@cursor-vs-flip-toggle:
- shard-hsw: PASS -> FAIL [fdo#103355]
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-render:
- shard-apl: PASS -> FAIL [fdo#103167] +3
- shard-kbl: PASS -> FAIL [fdo#103167] +2
* igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-onoff:
- shard-glk: PASS -> FAIL [fdo#103167] +7
* igt@kms_plane@pixel-format-pipe-a-planes-source-clamping:
- shard-apl: PASS -> FAIL [fdo#108948]
* igt@kms_plane@plane-position-covered-pipe-a-planes:
- shard-glk: PASS -> FAIL [fdo#103166] +3
- shard-kbl: PASS -> FAIL [fdo#103166] +1
* igt@kms_plane@plane-position-covered-pipe-c-planes:
- shard-apl: PASS -> FAIL [fdo#103166] +4
* igt@kms_plane_alpha_blend@pipe-b-alpha-transparant-fb:
- shard-kbl: NOTRUN -> FAIL [fdo#108145]
- shard-glk: NOTRUN -> FAIL [fdo#108145]
* igt@kms_setmode@basic:
- shard-apl: PASS -> FAIL [fdo#99912]
* igt@perf@oa-exponents:
- shard-glk: PASS -> FAIL [fdo#105483]
#### Possible fixes ####
* igt@kms_color@pipe-a-degamma:
- shard-apl: FAIL [fdo#104782] / [fdo#108145] -> PASS
- shard-kbl: FAIL [fdo#104782] / [fdo#108145] -> PASS
* igt@kms_color@pipe-c-legacy-gamma:
- shard-apl: FAIL [fdo#104782] -> PASS
* igt@kms_cursor_crc@cursor-128x128-suspend:
- shard-apl: FAIL [fdo#103191] / [fdo#103232] -> PASS
* igt@kms_cursor_crc@cursor-64x21-sliding:
- shard-apl: FAIL [fdo#103232] -> PASS
* igt@kms_cursor_crc@cursor-64x64-suspend:
- shard-apl: DMESG-WARN [fdo#108566] -> PASS
* igt@kms_cursor_legacy@2x-long-flip-vs-cursor-legacy:
- shard-glk: FAIL [fdo#104873] -> PASS
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-wc:
- shard-apl: FAIL [fdo#103167] -> PASS +1
* igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-pwrite:
- shard-kbl: FAIL [fdo#103167] -> PASS
* igt@kms_frontbuffer_tracking@fbc-1p-rte:
- shard-apl: FAIL [fdo#103167] / [fdo#105682] -> PASS
* igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-mmap-wc:
- shard-glk: FAIL [fdo#103167] -> PASS +5
* igt@kms_plane@pixel-format-pipe-a-planes-source-clamping:
- shard-glk: FAIL [fdo#108948] -> PASS
* igt@kms_plane@pixel-format-pipe-c-planes:
- shard-apl: FAIL [fdo#103166] -> PASS +5
* igt@kms_plane_alpha_blend@pipe-c-constant-alpha-max:
- shard-glk: FAIL [fdo#108145] -> PASS +1
- shard-kbl: FAIL [fdo#108145] -> PASS
- shard-apl: FAIL [fdo#108145] -> PASS
* igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
- shard-glk: FAIL [fdo#103166] -> PASS +3
- shard-kbl: FAIL [fdo#103166] -> PASS +2
* igt@kms_rotation_crc@multiplane-rotation-cropping-top:
- shard-kbl: FAIL [fdo#109016] -> PASS
* igt@kms_setmode@basic:
- shard-hsw: FAIL [fdo#99912] -> PASS
- shard-kbl: FAIL [fdo#99912] -> PASS
#### Warnings ####
* igt@kms_frontbuffer_tracking@psr-2p-scndscrn-pri-indfb-draw-render:
- shard-apl: {SKIP} [fdo#109271] -> INCOMPLETE [fdo#103927]
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
[fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
[fdo#103355]: https://bugs.freedesktop.org/show_bug.cgi?id=103355
[fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
[fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
[fdo#104782]: https://bugs.freedesktop.org/show_bug.cgi?id=104782
[fdo#104873]: https://bugs.freedesktop.org/show_bug.cgi?id=104873
[fdo#105483]: https://bugs.freedesktop.org/show_bug.cgi?id=105483
[fdo#105682]: https://bugs.freedesktop.org/show_bug.cgi?id=105682
[fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
[fdo#108147]: https://bugs.freedesktop.org/show_bug.cgi?id=108147
[fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
[fdo#108948]: https://bugs.freedesktop.org/show_bug.cgi?id=108948
[fdo#109016]: https://bugs.freedesktop.org/show_bug.cgi?id=109016
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278
[fdo#109350]: https://bugs.freedesktop.org/show_bug.cgi?id=109350
[fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
Participating hosts (7 -> 5)
------------------------------
Missing (2): shard-skl shard-iclb
Build changes
-------------
* IGT: IGT_4824 -> IGTPW_2396
CI_DRM_5598: 0d4420ccb7b8a3386fefc3719e71b5e8e69d3abb @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_2396: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2396/
IGT_4824: e55d439a9ba744227fb4c9d727338276b78871d4 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2396/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] igt/tests: Fix error checking in kms_atomic_transition
2019-02-14 8:33 ` Daniel Vetter
@ 2019-02-14 12:14 ` Lisovskiy, Stanislav via igt-dev
2019-02-14 15:30 ` Daniel Vetter
0 siblings, 1 reply; 10+ messages in thread
From: Lisovskiy, Stanislav via igt-dev @ 2019-02-14 12:14 UTC (permalink / raw)
To: daniel@ffwll.ch
Cc: igt-dev@lists.freedesktop.org, Syrjala, Ville,
Heikkila, Juha-pekka, Peres, Martin
On Thu, 2019-02-14 at 09:33 +0100, Daniel Vetter wrote:
> On Thu, Feb 14, 2019 at 08:31:52AM +0000, Lisovskiy, Stanislav wrote:
> > On Thu, 2019-02-14 at 09:25 +0100, Daniel Vetter wrote:
> > > On Thu, Feb 14, 2019 at 08:22:29AM +0000, Lisovskiy, Stanislav
> > > wrote:
> > > > On Wed, 2019-02-13 at 21:41 +0100, Daniel Vetter wrote:
> > > > > On Wed, Feb 13, 2019 at 06:18:17PM +0200, Stanislav Lisovskiy
> > > > > via
> > > > > igt-dev wrote:
> > > > > > There is no guarantee that error return value will be
> > > > > > always EINVAL, made a check more general as it can be
> > > > > > ERANGE, ENOSPC, EINVAL and probably others, which all
> > > > > > mean the same in context of this test case: i.e this sprite
> > > > > > size is not valid.
> > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=1092
> > > > > > 25
> > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@int
> > > > > > el.c
> > > > > > om>
> > > > >
> > > > > Kernel atomic spec is pretty clear that it's either EINVAL or
> > > > > ERANGE,
> > > > > and
> > > > > nothing else. In the docs we even limit to EINVAL (scroll
> > > > > down to
> > > > > atomic_check):
> > > > >
> > > > > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html?highlig
> > > > > ht=d
> > > > > rm_m
> > > > > ode_config_funcs#c.drm_mode_config_funcs
> > > > >
> > > > > Would be great if you can submit a kernel patch to add the
> > > > > ERANGE.
> > > > > And
> > > > > change this one here to only accept ERANGE and EINVAL as
> > > > > "sprite
> > > > > doesn't
> > > > > work".
> > > >
> > > > Unfortunately, in reality it is not like that. We have a bug
> > > > fdo#109225, where it returns ENOSPC. DRM does this when plane
> > > > size
> > > > exceeds the framebuffer size(check drm_atomic.c,
> > > > drm_atomic_plane_check).
> > > > We hit this issue when we happen to have resolution lower than
> > > > expected
> > > > by IGT, so we actually create a framebuffer of smaller size
> > > > than it
> > > > sets for the plane and then we
> > > > hit ENOSPC instead EINVAL, which is treated now as OK by the
> > > > test
> > > > case.
> > > > So adding ERANGE will not help for this bug.
> > > > Either I should add all three to IGT(ERANGE, ENOSPC, EINVAL) or
> > > > fix
> > > > it
> > > > in the kernel, so that it returns EINVAL instead of ENOSPC.
> > >
> > > Hm yeah, pls do, and pls do update the kernel documentation. I'm
> > > also
> > > wondering whether we shouldn't wrap some checks around calls to
> > > atomic_check in the kernel for this.
> > > -Daniel
> >
> > So do you mean I should fix the kernel or igt? :)
>
> Oops :-) I meant to fix the igt logic to also accept ENOSPC and
> EINVAL,
> plus update the kernel's documentation so that docs reflect reality.
> -Daniel
Ok, I will fix the kernel docs also. However regarding this patch,
should I still simply just compare to 0 in a check rather than adding
all three, as conditions like "if ((ret == -EINVAL) || (ret == -ERANGE)
|| (ret == -ENOSPC))" would look a bit weird, I guess..
>
> >
> > >
> > > >
> > > > > -Daniel
> > > > >
> > > > > > ---
> > > > > > tests/kms_atomic_transition.c | 4 ++--
> > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/tests/kms_atomic_transition.c
> > > > > > b/tests/kms_atomic_transition.c
> > > > > > index ec5d25de..58bf6280 100644
> > > > > > --- a/tests/kms_atomic_transition.c
> > > > > > +++ b/tests/kms_atomic_transition.c
> > > > > > @@ -282,7 +282,7 @@ retry:
> > > > > > wm_setup_plane(display, pipe, (1 <<
> > > > > > n_planes)
> > > > > > - 1,
> > > > > > parms, false);
> > > > > > ret =
> > > > > > igt_display_try_commit_atomic(display,
> > > > > > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET,
> > > > > > NULL);
> > > > > >
> > > > > > - if (ret == -EINVAL) {
> > > > > > + if (ret != 0) {
> > > > > > if (cursor_width == sprite_width
> > > > > > &&
> > > > > > cursor_height ==
> > > > > > sprite_height) {
> > > > > > igt_assert_f(alpha,
> > > > > > @@ -472,7 +472,7 @@ run_transition_test(igt_display_t
> > > > > > *display,
> > > > > > enum pipe pipe, igt_output_t *output
> > > > > > igt_pipe_request_out_fence(pipe_ob
> > > > > > j);
> > > > > >
> > > > > > ret =
> > > > > > igt_display_try_commit_atomic(display,
> > > > > > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET,
> > > > > > NULL);
> > > > > > - if (ret != -EINVAL || pipe_obj->n_planes <
> > > > > > 3)
> > > > > > + if (ret == 0 || pipe_obj->n_planes < 3)
> > > > > > break;
> > > > > >
> > > > > > ret = 0;
> > > > > > --
> > > > > > 2.17.1
> > > > > >
> > > > > > _______________________________________________
> > > > > > igt-dev mailing list
> > > > > > igt-dev@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> > > > >
> > > > >
> > > >
> > > > --
> > > > Best Regards,
> > > >
> > > > Lisovskiy Stanislav
> > >
> > >
> >
> > --
> > Best Regards,
> >
> > Lisovskiy Stanislav
>
>
--
Best Regards,
Lisovskiy Stanislav
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [igt-dev] [PATCH i-g-t] igt/tests: Fix error checking in kms_atomic_transition
2019-02-14 12:14 ` Lisovskiy, Stanislav via igt-dev
@ 2019-02-14 15:30 ` Daniel Vetter
0 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2019-02-14 15:30 UTC (permalink / raw)
To: Lisovskiy, Stanislav
Cc: igt-dev@lists.freedesktop.org, Syrjala, Ville,
Heikkila, Juha-pekka, daniel@ffwll.ch, Peres, Martin
On Thu, Feb 14, 2019 at 12:14:22PM +0000, Lisovskiy, Stanislav wrote:
> On Thu, 2019-02-14 at 09:33 +0100, Daniel Vetter wrote:
> > On Thu, Feb 14, 2019 at 08:31:52AM +0000, Lisovskiy, Stanislav wrote:
> > > On Thu, 2019-02-14 at 09:25 +0100, Daniel Vetter wrote:
> > > > On Thu, Feb 14, 2019 at 08:22:29AM +0000, Lisovskiy, Stanislav
> > > > wrote:
> > > > > On Wed, 2019-02-13 at 21:41 +0100, Daniel Vetter wrote:
> > > > > > On Wed, Feb 13, 2019 at 06:18:17PM +0200, Stanislav Lisovskiy
> > > > > > via
> > > > > > igt-dev wrote:
> > > > > > > There is no guarantee that error return value will be
> > > > > > > always EINVAL, made a check more general as it can be
> > > > > > > ERANGE, ENOSPC, EINVAL and probably others, which all
> > > > > > > mean the same in context of this test case: i.e this sprite
> > > > > > > size is not valid.
> > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=1092
> > > > > > > 25
> > > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@int
> > > > > > > el.c
> > > > > > > om>
> > > > > >
> > > > > > Kernel atomic spec is pretty clear that it's either EINVAL or
> > > > > > ERANGE,
> > > > > > and
> > > > > > nothing else. In the docs we even limit to EINVAL (scroll
> > > > > > down to
> > > > > > atomic_check):
> > > > > >
> > > > > > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html?highlig
> > > > > > ht=d
> > > > > > rm_m
> > > > > > ode_config_funcs#c.drm_mode_config_funcs
> > > > > >
> > > > > > Would be great if you can submit a kernel patch to add the
> > > > > > ERANGE.
> > > > > > And
> > > > > > change this one here to only accept ERANGE and EINVAL as
> > > > > > "sprite
> > > > > > doesn't
> > > > > > work".
> > > > >
> > > > > Unfortunately, in reality it is not like that. We have a bug
> > > > > fdo#109225, where it returns ENOSPC. DRM does this when plane
> > > > > size
> > > > > exceeds the framebuffer size(check drm_atomic.c,
> > > > > drm_atomic_plane_check).
> > > > > We hit this issue when we happen to have resolution lower than
> > > > > expected
> > > > > by IGT, so we actually create a framebuffer of smaller size
> > > > > than it
> > > > > sets for the plane and then we
> > > > > hit ENOSPC instead EINVAL, which is treated now as OK by the
> > > > > test
> > > > > case.
> > > > > So adding ERANGE will not help for this bug.
> > > > > Either I should add all three to IGT(ERANGE, ENOSPC, EINVAL) or
> > > > > fix
> > > > > it
> > > > > in the kernel, so that it returns EINVAL instead of ENOSPC.
> > > >
> > > > Hm yeah, pls do, and pls do update the kernel documentation. I'm
> > > > also
> > > > wondering whether we shouldn't wrap some checks around calls to
> > > > atomic_check in the kernel for this.
> > > > -Daniel
> > >
> > > So do you mean I should fix the kernel or igt? :)
> >
> > Oops :-) I meant to fix the igt logic to also accept ENOSPC and
> > EINVAL,
> > plus update the kernel's documentation so that docs reflect reality.
> > -Daniel
>
> Ok, I will fix the kernel docs also. However regarding this patch,
> should I still simply just compare to 0 in a check rather than adding
> all three, as conditions like "if ((ret == -EINVAL) || (ret == -ERANGE)
> || (ret == -ENOSPC))" would look a bit weird, I guess..
If you want it less wierd, make it a static inline, e.g.
is_atomic_check_failure_errno(int errno)
{
return errno == -EINVAL || errno == -ERANGE || errno == -ENOSPC;
}
Cheers, Daniel
>
> >
> > >
> > > >
> > > > >
> > > > > > -Daniel
> > > > > >
> > > > > > > ---
> > > > > > > tests/kms_atomic_transition.c | 4 ++--
> > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/tests/kms_atomic_transition.c
> > > > > > > b/tests/kms_atomic_transition.c
> > > > > > > index ec5d25de..58bf6280 100644
> > > > > > > --- a/tests/kms_atomic_transition.c
> > > > > > > +++ b/tests/kms_atomic_transition.c
> > > > > > > @@ -282,7 +282,7 @@ retry:
> > > > > > > wm_setup_plane(display, pipe, (1 <<
> > > > > > > n_planes)
> > > > > > > - 1,
> > > > > > > parms, false);
> > > > > > > ret =
> > > > > > > igt_display_try_commit_atomic(display,
> > > > > > > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET,
> > > > > > > NULL);
> > > > > > >
> > > > > > > - if (ret == -EINVAL) {
> > > > > > > + if (ret != 0) {
> > > > > > > if (cursor_width == sprite_width
> > > > > > > &&
> > > > > > > cursor_height ==
> > > > > > > sprite_height) {
> > > > > > > igt_assert_f(alpha,
> > > > > > > @@ -472,7 +472,7 @@ run_transition_test(igt_display_t
> > > > > > > *display,
> > > > > > > enum pipe pipe, igt_output_t *output
> > > > > > > igt_pipe_request_out_fence(pipe_ob
> > > > > > > j);
> > > > > > >
> > > > > > > ret =
> > > > > > > igt_display_try_commit_atomic(display,
> > > > > > > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET,
> > > > > > > NULL);
> > > > > > > - if (ret != -EINVAL || pipe_obj->n_planes <
> > > > > > > 3)
> > > > > > > + if (ret == 0 || pipe_obj->n_planes < 3)
> > > > > > > break;
> > > > > > >
> > > > > > > ret = 0;
> > > > > > > --
> > > > > > > 2.17.1
> > > > > > >
> > > > > > > _______________________________________________
> > > > > > > igt-dev mailing list
> > > > > > > igt-dev@lists.freedesktop.org
> > > > > > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> > > > > >
> > > > > >
> > > > >
> > > > > --
> > > > > Best Regards,
> > > > >
> > > > > Lisovskiy Stanislav
> > > >
> > > >
> > >
> > > --
> > > Best Regards,
> > >
> > > Lisovskiy Stanislav
> >
> >
> --
> Best Regards,
>
> Lisovskiy Stanislav
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-02-14 15:30 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-02-13 16:18 [igt-dev] [PATCH i-g-t] igt/tests: Fix error checking in kms_atomic_transition Stanislav Lisovskiy via igt-dev
2019-02-13 17:45 ` [igt-dev] ✓ Fi.CI.BAT: success for " Patchwork
2019-02-13 20:41 ` [igt-dev] [PATCH i-g-t] " Daniel Vetter
2019-02-14 8:22 ` Lisovskiy, Stanislav via igt-dev
2019-02-14 8:25 ` Daniel Vetter
2019-02-14 8:31 ` Lisovskiy, Stanislav via igt-dev
2019-02-14 8:33 ` Daniel Vetter
2019-02-14 12:14 ` Lisovskiy, Stanislav via igt-dev
2019-02-14 15:30 ` Daniel Vetter
2019-02-14 9:28 ` [igt-dev] ✓ Fi.CI.IGT: success for " Patchwork
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox