[igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition

[igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition

[Stanislav Lisovskiy]

From: Stanislav Lisovskiy <stanislav.lisovskiy@gmail.com>

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.

v2: Added macro to make check look a bit nicer.
v3: Removed redundant debug line.
v4: Added assertion if error is not EINVAL as expected,
    other errors except EINVAL are considered now a failures.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109225
Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
---
 tests/kms_atomic_transition.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/tests/kms_atomic_transition.c b/tests/kms_atomic_transition.c
index 12bafe87..e1a2d4a0 100644
--- a/tests/kms_atomic_transition.c
+++ b/tests/kms_atomic_transition.c
@@ -187,6 +187,12 @@ static void set_sprite_wh(igt_display_t *display, enum pipe pipe,
 		      LOCAL_DRM_FORMAT_MOD_NONE, sprite_fb);
 }
 
+#define is_atomic_check_failure_errno(errno) \
+		(errno != -EINVAL && errno != 0)
+
+#define is_atomic_check_plane_size_errno(errno) \
+		(errno == -EINVAL)
+
 static void setup_parms(igt_display_t *display, enum pipe pipe,
 			const drmModeModeInfo *mode,
 			struct igt_fb *primary_fb,
@@ -251,8 +257,9 @@ 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);
+		igt_assert(!is_atomic_check_failure_errno(ret));
 
-		if (ret == -EINVAL) {
+		if (is_atomic_check_plane_size_errno(ret)) {
 			if (cursor_width == sprite_width &&
 			    cursor_height == sprite_height) {
 				igt_assert_f(alpha,
@@ -442,7 +449,9 @@ 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)
+		igt_assert(!is_atomic_check_failure_errno(ret));
+
+		if (!is_atomic_check_plane_size_errno(ret) || 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

[igt-dev] ✓ Fi.CI.BAT: success for igt/tests: Fix error checking in kms_atomic_transition (rev4)

[Patchwork]

== Series Details ==

Series: igt/tests: Fix error checking in kms_atomic_transition (rev4)
URL   : https://patchwork.freedesktop.org/series/56617/
State : success

== Summary ==

CI Bug Log - changes from IGT_4837 -> IGTPW_2442
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/56617/revisions/4/mbox/

Known issues
------------

  Here are the changes found in IGTPW_2442 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103191] / [fdo#107362]

  
#### Possible fixes ####

  * igt@i915_selftest@live_evict:
    - fi-bsw-kefka:       DMESG-WARN [fdo#107709] -> PASS

  * igt@i915_selftest@live_execlists:
    - {fi-icl-y}:         INCOMPLETE [fdo#109567] -> PASS

  * igt@kms_chamelium@hdmi-hpd-fast:
    - fi-kbl-7500u:       FAIL [fdo#109485] -> PASS

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a:
    - fi-byt-clapper:     FAIL [fdo#103191] / [fdo#107362] -> PASS +1

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - fi-blb-e6850:       INCOMPLETE [fdo#107718] -> PASS

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#107709]: https://bugs.freedesktop.org/show_bug.cgi?id=107709
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [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 (43 -> 39)
------------------------------

  Additional (2): fi-kbl-7560u fi-snb-2600 
  Missing    (6): fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-apl-guc fi-whl-u fi-gdg-551 


Build changes
-------------

    * IGT: IGT_4837 -> IGTPW_2442

  CI_DRM_5629: e5bd5364ce58650dc664ee250629aa902dfe64ac @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2442: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2442/
  IGT_4837: 368e76156f752e6ed6ac32ed9f400567aef7d3fc @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2442/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

[igt-dev] ✓ Fi.CI.IGT: success for igt/tests: Fix error checking in kms_atomic_transition (rev4)

[Patchwork]

== Series Details ==

Series: igt/tests: Fix error checking in kms_atomic_transition (rev4)
URL   : https://patchwork.freedesktop.org/series/56617/
State : success

== Summary ==

CI Bug Log - changes from IGT_4837_full -> IGTPW_2442_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/56617/revisions/4/mbox/

Known issues
------------

  Here are the changes found in IGTPW_2442_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_param@invalid-param-get:
    - shard-snb:          NOTRUN -> FAIL [fdo#109559]

  * igt@gem_exec_schedule@out-order-vebox:
    - shard-glk:          PASS -> FAIL [fdo#109662]

  * igt@kms_busy@extended-modeset-hang-newfb-render-b:
    - shard-glk:          NOTRUN -> DMESG-WARN [fdo#107956]

  * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a:
    - shard-snb:          NOTRUN -> DMESG-WARN [fdo#107956]

  * igt@kms_cursor_crc@cursor-128x128-suspend:
    - shard-apl:          PASS -> FAIL [fdo#103191] / [fdo#103232]

  * igt@kms_cursor_crc@cursor-256x256-suspend:
    - shard-kbl:          PASS -> FAIL [fdo#103191] / [fdo#103232]

  * igt@kms_cursor_crc@cursor-64x21-random:
    - shard-apl:          PASS -> FAIL [fdo#103232] +5
    - shard-kbl:          PASS -> FAIL [fdo#103232] +2

  * igt@kms_flip@flip-vs-expired-vblank:
    - shard-glk:          PASS -> FAIL [fdo#102887] / [fdo#105363]

  * igt@kms_flip@flip-vs-expired-vblank-interruptible:
    - shard-glk:          PASS -> FAIL [fdo#105363]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-cur-indfb-draw-pwrite:
    - shard-kbl:          PASS -> FAIL [fdo#103167]

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-render:
    - shard-apl:          PASS -> FAIL [fdo#103167] +1

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-draw-mmap-cpu:
    - shard-glk:          PASS -> FAIL [fdo#103167] +8

  * igt@kms_plane_alpha_blend@pipe-a-alpha-opaque-fb:
    - shard-glk:          PASS -> FAIL [fdo#108145]

  * igt@kms_plane_alpha_blend@pipe-c-alpha-basic:
    - shard-glk:          NOTRUN -> FAIL [fdo#108145]

  * igt@kms_plane_multiple@atomic-pipe-a-tiling-y:
    - shard-glk:          PASS -> FAIL [fdo#103166] +4

  * igt@kms_setmode@basic:
    - shard-apl:          PASS -> FAIL [fdo#99912]

  * igt@kms_universal_plane@universal-plane-pipe-a-functional:
    - shard-apl:          PASS -> FAIL [fdo#103166]

  * igt@kms_vblank@pipe-b-ts-continuation-suspend:
    - shard-kbl:          PASS -> INCOMPLETE [fdo#103665]

  * igt@pm_rpm@gem-execbuf-stress:
    - shard-kbl:          PASS -> INCOMPLETE [fdo#103665] / [fdo#107803] / [fdo#107807]

  * igt@testdisplay:
    - shard-apl:          PASS -> INCOMPLETE [fdo#103927]

  
#### Possible fixes ####

  * igt@kms_busy@extended-modeset-hang-newfb-render-b:
    - shard-hsw:          DMESG-WARN [fdo#107956] -> PASS

  * igt@kms_ccs@pipe-a-crc-sprite-planes-basic:
    - shard-apl:          FAIL [fdo#106510] / [fdo#108145] -> PASS +1
    - shard-glk:          FAIL [fdo#108145] -> PASS

  * igt@kms_ccs@pipe-b-crc-sprite-planes-basic:
    - shard-kbl:          FAIL [fdo#107725] / [fdo#108145] -> PASS +1

  * igt@kms_cursor_crc@cursor-128x42-onscreen:
    - shard-apl:          FAIL [fdo#103232] -> PASS +5

  * igt@kms_cursor_crc@cursor-64x21-onscreen:
    - shard-kbl:          FAIL [fdo#103232] -> PASS

  * igt@kms_cursor_crc@cursor-alpha-opaque:
    - shard-apl:          FAIL [fdo#109350] -> PASS

  * igt@kms_flip@flip-vs-dpms-interruptible:
    - shard-hsw:          INCOMPLETE [fdo#103540] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-onoff:
    - shard-apl:          FAIL [fdo#103167] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-1p-rte:
    - shard-glk:          FAIL [fdo#103167] / [fdo#105682] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-spr-indfb-onoff:
    - shard-glk:          FAIL [fdo#103167] -> PASS +5

  * igt@kms_plane@pixel-format-pipe-c-planes-source-clamping:
    - shard-apl:          FAIL [fdo#108948] -> PASS

  * igt@kms_plane_multiple@atomic-pipe-c-tiling-x:
    - shard-glk:          FAIL [fdo#103166] -> PASS +2

  * igt@kms_plane_multiple@atomic-pipe-c-tiling-yf:
    - shard-apl:          FAIL [fdo#103166] -> PASS +5

  * igt@kms_setmode@basic:
    - shard-hsw:          FAIL [fdo#99912] -> PASS

  * igt@pm_rc6_residency@rc6-accuracy:
    - shard-snb:          {SKIP} [fdo#109271] -> PASS

  
#### Warnings ####

  * igt@kms_dp_dsc@basic-dsc-enable-edp:
    - shard-snb:          {SKIP} [fdo#109271] -> INCOMPLETE [fdo#105411] / [fdo#107469]

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#102887]: https://bugs.freedesktop.org/show_bug.cgi?id=102887
  [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#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363
  [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
  [fdo#105682]: https://bugs.freedesktop.org/show_bug.cgi?id=105682
  [fdo#106510]: https://bugs.freedesktop.org/show_bug.cgi?id=106510
  [fdo#107469]: https://bugs.freedesktop.org/show_bug.cgi?id=107469
  [fdo#107725]: https://bugs.freedesktop.org/show_bug.cgi?id=107725
  [fdo#107803]: https://bugs.freedesktop.org/show_bug.cgi?id=107803
  [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
  [fdo#107956]: https://bugs.freedesktop.org/show_bug.cgi?id=107956
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108948]: https://bugs.freedesktop.org/show_bug.cgi?id=108948
  [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#109559]: https://bugs.freedesktop.org/show_bug.cgi?id=109559
  [fdo#109662]: https://bugs.freedesktop.org/show_bug.cgi?id=109662
  [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_4837 -> IGTPW_2442

  CI_DRM_5629: e5bd5364ce58650dc664ee250629aa902dfe64ac @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2442: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2442/
  IGT_4837: 368e76156f752e6ed6ac32ed9f400567aef7d3fc @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2442/
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition

[Summers, Stuart]

[-- Attachment #1.1: Type: text/plain, Size: 2576 bytes --]

On Tue, 2019-02-19 at 11:38 +0200, Stanislav Lisovskiy wrote:
> From: Stanislav Lisovskiy <stanislav.lisovskiy@gmail.com>
> 
> 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.
> 
> v2: Added macro to make check look a bit nicer.
> v3: Removed redundant debug line.
> v4: Added assertion if error is not EINVAL as expected,
>     other errors except EINVAL are considered now a failures.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109225
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  tests/kms_atomic_transition.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/kms_atomic_transition.c
> b/tests/kms_atomic_transition.c
> index 12bafe87..e1a2d4a0 100644
> --- a/tests/kms_atomic_transition.c
> +++ b/tests/kms_atomic_transition.c
> @@ -187,6 +187,12 @@ static void set_sprite_wh(igt_display_t
> *display, enum pipe pipe,
>  		      LOCAL_DRM_FORMAT_MOD_NONE, sprite_fb);
>  }
>  
> +#define is_atomic_check_failure_errno(errno) \
> +		(errno != -EINVAL && errno != 0)
> +
> +#define is_atomic_check_plane_size_errno(errno) \
> +		(errno == -EINVAL)
> +
>  static void setup_parms(igt_display_t *display, enum pipe pipe,
>  			const drmModeModeInfo *mode,
>  			struct igt_fb *primary_fb,
> @@ -251,8 +257,9 @@ 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);
> +		igt_assert(!is_atomic_check_failure_errno(ret));
>  
> -		if (ret == -EINVAL) {
> +		if (is_atomic_check_plane_size_errno(ret)) {
>  			if (cursor_width == sprite_width &&
>  			    cursor_height == sprite_height) {
>  				igt_assert_f(alpha,
> @@ -442,7 +449,9 @@ 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)
> +		igt_assert(!is_atomic_check_failure_errno(ret));
> +
> +		if (!is_atomic_check_plane_size_errno(ret) || pipe_obj-
> >n_planes < 3)
>  			break;
>  
>  		ret = 0;

LGTM

Reviewed-by: Stuart Summers <stuart.summers@intel.com>

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3270 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition

[Petri Latvala]

On Tue, Feb 19, 2019 at 09:05:12PM +0000, Summers, Stuart wrote:
> 
> Reviewed-by: Stuart Summers <stuart.summers@intel.com>


Merged, thanks for the patch and review.

Stan, do you have IGT commit access yet?


-- 
Petri Latvala
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition

[Lisovskiy, Stanislav]

On Wed, 2019-03-06 at 14:56 +0200, Petri Latvala wrote:
> On Tue, Feb 19, 2019 at 09:05:12PM +0000, Summers, Stuart wrote:
> > 
> > Reviewed-by: Stuart Summers <stuart.summers@intel.com>
> 
> 
> Merged, thanks for the patch and review.
> 
> Stan, do you have IGT commit access yet?

I didn't try to commit anything, but I guess not.

Best Regards,

Lisovskiy Stanislav
> 
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition

[Daniel Vetter]

On Tue, Feb 19, 2019 at 09:05:12PM +0000, Summers, Stuart wrote:
> On Tue, 2019-02-19 at 11:38 +0200, Stanislav Lisovskiy wrote:
> > From: Stanislav Lisovskiy <stanislav.lisovskiy@gmail.com>
> > 
> > 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.
> > 
> > v2: Added macro to make check look a bit nicer.
> > v3: Removed redundant debug line.
> > v4: Added assertion if error is not EINVAL as expected,
> >     other errors except EINVAL are considered now a failures.

Uh, commit message says to make the error message more general, and then
proceeds to make it not more general. This kind of inconsistencies should
be caught in review.

> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109225
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

And I also don't understand how this patch fixed anything really, we still
just check for EINVAL.
-Daniel

> > ---
> >  tests/kms_atomic_transition.c | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tests/kms_atomic_transition.c
> > b/tests/kms_atomic_transition.c
> > index 12bafe87..e1a2d4a0 100644
> > --- a/tests/kms_atomic_transition.c
> > +++ b/tests/kms_atomic_transition.c
> > @@ -187,6 +187,12 @@ static void set_sprite_wh(igt_display_t
> > *display, enum pipe pipe,
> >  		      LOCAL_DRM_FORMAT_MOD_NONE, sprite_fb);
> >  }
> >  
> > +#define is_atomic_check_failure_errno(errno) \
> > +		(errno != -EINVAL && errno != 0)
> > +
> > +#define is_atomic_check_plane_size_errno(errno) \
> > +		(errno == -EINVAL)
> > +
> >  static void setup_parms(igt_display_t *display, enum pipe pipe,
> >  			const drmModeModeInfo *mode,
> >  			struct igt_fb *primary_fb,
> > @@ -251,8 +257,9 @@ 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);
> > +		igt_assert(!is_atomic_check_failure_errno(ret));
> >  
> > -		if (ret == -EINVAL) {
> > +		if (is_atomic_check_plane_size_errno(ret)) {
> >  			if (cursor_width == sprite_width &&
> >  			    cursor_height == sprite_height) {
> >  				igt_assert_f(alpha,
> > @@ -442,7 +449,9 @@ 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)
> > +		igt_assert(!is_atomic_check_failure_errno(ret));
> > +
> > +		if (!is_atomic_check_plane_size_errno(ret) || pipe_obj-
> > >n_planes < 3)
> >  			break;
> >  
> >  		ret = 0;
> 
> LGTM
> 
> Reviewed-by: Stuart Summers <stuart.summers@intel.com>



> _______________________________________________
> 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

Re: [igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition

[Lisovskiy, Stanislav]

On Wed, 2019-04-03 at 16:23 +0200, Daniel Vetter wrote:
> On Tue, Feb 19, 2019 at 09:05:12PM +0000, Summers, Stuart wrote:
> > On Tue, 2019-02-19 at 11:38 +0200, Stanislav Lisovskiy wrote:
> > > From: Stanislav Lisovskiy <stanislav.lisovskiy@gmail.com>
> > > 
> > > 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.
> > > 
> > > v2: Added macro to make check look a bit nicer.
> > > v3: Removed redundant debug line.
> > > v4: Added assertion if error is not EINVAL as expected,
> > >     other errors except EINVAL are considered now a failures.
> 
> Uh, commit message says to make the error message more general, and
> then
> proceeds to make it not more general. This kind of inconsistencies
> should
> be caught in review.

Hi Daniel,

As the commit message says, previsouly we were expecting only
EINVAL(which indicates for
us that we had reached max size of a plane) or 0.

Problem was that in some circumstances, if we get something else
test case did not catch that and proceeded as if everything is
great, failing afterwards, when trying already to commit or other
place.
The thing which this patch does is as said "Added assertion if error is
not EINVAL as expected, other errors except EINVAL are considered now a
failures". Otherwise if we got EINVAL, we proceed normally.

There was actually no purpose to "fix" something by that, but rather
make it fail in a proper place and proper way, otherwise error just
went further. 

My initial idea actually was to catch all kind of problems here, so
that we always get some kind of working plane configuration here.
However it was not accepted and currently only EINVAL is treated as
"normal" error.

If you have any proposals, how this can be improved sure I can do this.

> 
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109225
> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com
> > > >
> 
> And I also don't understand how this patch fixed anything really, we
> still
> just check for EINVAL.
> -Daniel
> 
> > > ---
> > >  tests/kms_atomic_transition.c | 13 +++++++++++--
> > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tests/kms_atomic_transition.c
> > > b/tests/kms_atomic_transition.c
> > > index 12bafe87..e1a2d4a0 100644
> > > --- a/tests/kms_atomic_transition.c
> > > +++ b/tests/kms_atomic_transition.c
> > > @@ -187,6 +187,12 @@ static void set_sprite_wh(igt_display_t
> > > *display, enum pipe pipe,
> > >  		      LOCAL_DRM_FORMAT_MOD_NONE, sprite_fb);
> > >  }
> > >  
> > > +#define is_atomic_check_failure_errno(errno) \
> > > +		(errno != -EINVAL && errno != 0)
> > > +
> > > +#define is_atomic_check_plane_size_errno(errno) \
> > > +		(errno == -EINVAL)
> > > +
> > >  static void setup_parms(igt_display_t *display, enum pipe pipe,
> > >  			const drmModeModeInfo *mode,
> > >  			struct igt_fb *primary_fb,
> > > @@ -251,8 +257,9 @@ 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);
> > > +		igt_assert(!is_atomic_check_failure_errno(ret));
> > >  
> > > -		if (ret == -EINVAL) {
> > > +		if (is_atomic_check_plane_size_errno(ret)) {
> > >  			if (cursor_width == sprite_width &&
> > >  			    cursor_height == sprite_height) {
> > >  				igt_assert_f(alpha,
> > > @@ -442,7 +449,9 @@ 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)
> > > +		igt_assert(!is_atomic_check_failure_errno(ret));
> > > +
> > > +		if (!is_atomic_check_plane_size_errno(ret) || pipe_obj-
> > > > n_planes < 3)
> > > 
> > >  			break;
> > >  
> > >  		ret = 0;
> > 
> > LGTM
> > 
> > Reviewed-by: Stuart Summers <stuart.summers@intel.com>
> 
> 
> 
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> 
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev

Re: [igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition

[Daniel Vetter]

On Thu, Apr 04, 2019 at 07:33:31AM +0000, Lisovskiy, Stanislav wrote:
> On Wed, 2019-04-03 at 16:23 +0200, Daniel Vetter wrote:
> > On Tue, Feb 19, 2019 at 09:05:12PM +0000, Summers, Stuart wrote:
> > > On Tue, 2019-02-19 at 11:38 +0200, Stanislav Lisovskiy wrote:
> > > > From: Stanislav Lisovskiy <stanislav.lisovskiy@gmail.com>
> > > > 
> > > > 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.
> > > > 
> > > > v2: Added macro to make check look a bit nicer.
> > > > v3: Removed redundant debug line.
> > > > v4: Added assertion if error is not EINVAL as expected,
> > > >     other errors except EINVAL are considered now a failures.
> > 
> > Uh, commit message says to make the error message more general, and
> > then
> > proceeds to make it not more general. This kind of inconsistencies
> > should
> > be caught in review.
> 
> Hi Daniel,
> 
> As the commit message says, previsouly we were expecting only
> EINVAL(which indicates for
> us that we had reached max size of a plane) or 0.
> 
> Problem was that in some circumstances, if we get something else
> test case did not catch that and proceeded as if everything is
> great, failing afterwards, when trying already to commit or other
> place.
> The thing which this patch does is as said "Added assertion if error is
> not EINVAL as expected, other errors except EINVAL are considered now a
> failures". Otherwise if we got EINVAL, we proceed normally.
> 
> There was actually no purpose to "fix" something by that, but rather
> make it fail in a proper place and proper way, otherwise error just
> went further. 
> 
> My initial idea actually was to catch all kind of problems here, so
> that we always get some kind of working plane configuration here.
> However it was not accepted and currently only EINVAL is treated as
> "normal" error.
> 
> If you have any proposals, how this can be improved sure I can do this.

Yeah I think the patch is fine, but the commit message isn't the clearest,
since it explains what exactly an interim version did, and then amends
that. Which is confusing.

You're explanation above would have made a much better commit message, but
oh well the patch is committed so can't fix that up anymore. But good to
heed for next time around, review should also make sure the commit message
captures everything discussed as well as possible.

Cheers, Daniel
> 
> > 
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109225
> > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com
> > > > >
> > 
> > And I also don't understand how this patch fixed anything really, we
> > still
> > just check for EINVAL.
> > -Daniel
> > 
> > > > ---
> > > >  tests/kms_atomic_transition.c | 13 +++++++++++--
> > > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/tests/kms_atomic_transition.c
> > > > b/tests/kms_atomic_transition.c
> > > > index 12bafe87..e1a2d4a0 100644
> > > > --- a/tests/kms_atomic_transition.c
> > > > +++ b/tests/kms_atomic_transition.c
> > > > @@ -187,6 +187,12 @@ static void set_sprite_wh(igt_display_t
> > > > *display, enum pipe pipe,
> > > >  		      LOCAL_DRM_FORMAT_MOD_NONE, sprite_fb);
> > > >  }
> > > >  
> > > > +#define is_atomic_check_failure_errno(errno) \
> > > > +		(errno != -EINVAL && errno != 0)
> > > > +
> > > > +#define is_atomic_check_plane_size_errno(errno) \
> > > > +		(errno == -EINVAL)
> > > > +
> > > >  static void setup_parms(igt_display_t *display, enum pipe pipe,
> > > >  			const drmModeModeInfo *mode,
> > > >  			struct igt_fb *primary_fb,
> > > > @@ -251,8 +257,9 @@ 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);
> > > > +		igt_assert(!is_atomic_check_failure_errno(ret));
> > > >  
> > > > -		if (ret == -EINVAL) {
> > > > +		if (is_atomic_check_plane_size_errno(ret)) {
> > > >  			if (cursor_width == sprite_width &&
> > > >  			    cursor_height == sprite_height) {
> > > >  				igt_assert_f(alpha,
> > > > @@ -442,7 +449,9 @@ 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)
> > > > +		igt_assert(!is_atomic_check_failure_errno(ret));
> > > > +
> > > > +		if (!is_atomic_check_plane_size_errno(ret) || pipe_obj-
> > > > > n_planes < 3)
> > > > 
> > > >  			break;
> > > >  
> > > >  		ret = 0;
> > > 
> > > LGTM
> > > 
> > > Reviewed-by: Stuart Summers <stuart.summers@intel.com>
> > 
> > 
> > 
> > > _______________________________________________
> > > 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

Re: [igt-dev] [PATCH i-g-t v4] igt/tests: Fix error checking in kms_atomic_transition

[Lisovskiy, Stanislav]

On Thu, 2019-04-04 at 10:10 +0200, Daniel Vetter wrote:
> On Thu, Apr 04, 2019 at 07:33:31AM +0000, Lisovskiy, Stanislav wrote:
> > On Wed, 2019-04-03 at 16:23 +0200, Daniel Vetter wrote:
> > > On Tue, Feb 19, 2019 at 09:05:12PM +0000, Summers, Stuart wrote:
> > > > On Tue, 2019-02-19 at 11:38 +0200, Stanislav Lisovskiy wrote:
> > > > > From: Stanislav Lisovskiy <stanislav.lisovskiy@gmail.com>
> > > > > 
> > > > > 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.
> > > > > 
> > > > > v2: Added macro to make check look a bit nicer.
> > > > > v3: Removed redundant debug line.
> > > > > v4: Added assertion if error is not EINVAL as expected,
> > > > >     other errors except EINVAL are considered now a failures.
> > > 
> > > Uh, commit message says to make the error message more general,
> > > and
> > > then
> > > proceeds to make it not more general. This kind of
> > > inconsistencies
> > > should
> > > be caught in review.
> > 
> > Hi Daniel,
> > 
> > As the commit message says, previsouly we were expecting only
> > EINVAL(which indicates for
> > us that we had reached max size of a plane) or 0.
> > 
> > Problem was that in some circumstances, if we get something else
> > test case did not catch that and proceeded as if everything is
> > great, failing afterwards, when trying already to commit or other
> > place.
> > The thing which this patch does is as said "Added assertion if
> > error is
> > not EINVAL as expected, other errors except EINVAL are considered
> > now a
> > failures". Otherwise if we got EINVAL, we proceed normally.
> > 
> > There was actually no purpose to "fix" something by that, but
> > rather
> > make it fail in a proper place and proper way, otherwise error just
> > went further. 
> > 
> > My initial idea actually was to catch all kind of problems here, so
> > that we always get some kind of working plane configuration here.
> > However it was not accepted and currently only EINVAL is treated as
> > "normal" error.
> > 
> > If you have any proposals, how this can be improved sure I can do
> > this.
> 
> Yeah I think the patch is fine, but the commit message isn't the
> clearest,
> since it explains what exactly an interim version did, and then
> amends
> that. Which is confusing.
> 
> You're explanation above would have made a much better commit
> message, but
> oh well the patch is committed so can't fix that up anymore. But good
> to
> heed for next time around, review should also make sure the commit
> message
> captures everything discussed as well as possible.
> 
> Cheers, Daniel

Well, yeah v4 revision says something different from initial commit
message :) I just thought that I need to keep it for history.

To me it is still at least arguable, if we really need to fail in
setup_parms. Anyway, if the function purpose was to find a maximum size
for sprite plane, should it really matter what was the 
error( ENOSPC, ERANGE, EINVAL) then if we are still able to find max
plane size which works?

> > 
> > > 
> > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109225
> > > > > Signed-off-by: Stanislav Lisovskiy <
> > > > > stanislav.lisovskiy@intel.com
> > > > > > 
> > > 
> > > And I also don't understand how this patch fixed anything really,
> > > we
> > > still
> > > just check for EINVAL.
> > > -Daniel
> > > 
> > > > > ---
> > > > >  tests/kms_atomic_transition.c | 13 +++++++++++--
> > > > >  1 file changed, 11 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/tests/kms_atomic_transition.c
> > > > > b/tests/kms_atomic_transition.c
> > > > > index 12bafe87..e1a2d4a0 100644
> > > > > --- a/tests/kms_atomic_transition.c
> > > > > +++ b/tests/kms_atomic_transition.c
> > > > > @@ -187,6 +187,12 @@ static void set_sprite_wh(igt_display_t
> > > > > *display, enum pipe pipe,
> > > > >  		      LOCAL_DRM_FORMAT_MOD_NONE, sprite_fb);
> > > > >  }
> > > > >  
> > > > > +#define is_atomic_check_failure_errno(errno) \
> > > > > +		(errno != -EINVAL && errno != 0)
> > > > > +
> > > > > +#define is_atomic_check_plane_size_errno(errno) \
> > > > > +		(errno == -EINVAL)
> > > > > +
> > > > >  static void setup_parms(igt_display_t *display, enum pipe
> > > > > pipe,
> > > > >  			const drmModeModeInfo *mode,
> > > > >  			struct igt_fb *primary_fb,
> > > > > @@ -251,8 +257,9 @@ 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);
> > > > > +		igt_assert(!is_atomic_check_failure_errno(ret))
> > > > > ;
> > > > >  
> > > > > -		if (ret == -EINVAL) {
> > > > > +		if (is_atomic_check_plane_size_errno(ret)) {
> > > > >  			if (cursor_width == sprite_width &&
> > > > >  			    cursor_height == sprite_height) {
> > > > >  				igt_assert_f(alpha,
> > > > > @@ -442,7 +449,9 @@ 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)
> > > > > +		igt_assert(!is_atomic_check_failure_errno(ret))
> > > > > ;
> > > > > +
> > > > > +		if (!is_atomic_check_plane_size_errno(ret) ||
> > > > > pipe_obj-
> > > > > > n_planes < 3)
> > > > > 
> > > > >  			break;
> > > > >  
> > > > >  		ret = 0;
> > > > 
> > > > LGTM
> > > > 
> > > > Reviewed-by: Stuart Summers <stuart.summers@intel.com>
> > > 
> > > 
> > > 
> > > > _______________________________________________
> > > > igt-dev mailing list
> > > > igt-dev@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> > > 
> > > 
> 
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev