[igt-dev] [PATCH i-g-t 1/2] tests/frontbuffer_tracking: Do not assert FBC state after a page flip changing stride

[igt-dev] [PATCH i-g-t 1/2] tests/frontbuffer_tracking: Do not assert FBC state after a page flip changing stride

[José Roberto de Souza]

When the stridechange subtest was introduced f23ea58f1fbb
("kms_frontbuffer_tracking: expand badstride and stridechange")
atomic was not around and change the stride using drmModePageFlip()
was not allowed so it was expected that it would return -EINVAL and
kernel would keep the old framebuffer with the smaller plane that is
know to fit on CFB(if it don't fit the test will skip on the first
full-modeset because "not enough stolen memory" is set).

But after the introduction of atomic the subtest was updated by
f63e070b469d ("kms_frontbuffer_tracking: Fix tests with the new
atomic reality.") to accept a no error return from drmModePageFlip()
but the do_assertions() that follows it was not updated.
As the subtest function comment states, kernel will do fastsets in
this scenario and the allocated CFB could not be enough to keep FBC
enabled over the new framebuffer, so here adding the missing
DONT_ASSERT_FEATURE_STATUS to ignore the FBC state and just test if
CRC match and if kernel do not misbehave.

Other way to solve this issue would be make the kernel do a
full-modeset when CFB is not enough for the new plane so FBC is
disabled with the CRC freeing the actual CFB and then after enable
CRTC again it will try to enable FBC again if it can allocate the
required CFB but by the subtest comment this is not intended.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105683
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 tests/kms_frontbuffer_tracking.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index 4d15ce1c..087fc473 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -2950,7 +2950,7 @@ static void stridechange_subtest(const struct test_mode *t)
 	 */
 	rc = drmModePageFlip(drm.fd, drm.display.pipes[params->pipe].crtc_id, new_fb->fb_id, 0, NULL);
 	igt_assert(rc == -EINVAL || rc == 0);
-	do_assertions(0);
+	do_assertions(DONT_ASSERT_FEATURE_STATUS);
 }
 
 /**
-- 
2.21.0

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

[igt-dev] [PATCH i-g-t 2/2] tests/frontbuffer_tracking: Assert the status of other features on stridechange subtest

[José Roberto de Souza]

As explained in c1edee186d18 ("tests/frontbuffer_tracking: Do not
assert FBC state after a page flip changing stride") after changing
the plane stride there is the possibility that CFB will not be big
enough to keep FBC enabled, that is why do_assertions() is called
with DONT_ASSERT_FEATURE_STATUS but DONT_ASSERT_FEATURE_STATUS is
overkill and will not check the status of the other features like PSR
and DRRS when running combined feature tests and possibly hiding
bugs.

So lets add a new flag that will only not assert FBC.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 tests/kms_frontbuffer_tracking.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
index 087fc473..0dcde097 100644
--- a/tests/kms_frontbuffer_tracking.c
+++ b/tests/kms_frontbuffer_tracking.c
@@ -1527,6 +1527,7 @@ static void do_flush(const struct test_mode *t)
 
 #define DONT_ASSERT_CRC			(1 << 0)
 #define DONT_ASSERT_FEATURE_STATUS	(1 << 1)
+#define DONT_ASSERT_FBC_STATUS		(1 << 12)
 
 #define FBC_ASSERT_FLAGS		(0xF << 2)
 #define ASSERT_FBC_ENABLED		(1 << 2)
@@ -1557,7 +1558,7 @@ static int adjust_assertion_flags(const struct test_mode *t, int flags)
 			flags |= ASSERT_DRRS_HIGH;
 	}
 
-	if ((t->feature & FEATURE_FBC) == 0)
+	if ((t->feature & FEATURE_FBC) == 0 || (flags & DONT_ASSERT_FBC_STATUS))
 		flags &= ~FBC_ASSERT_FLAGS;
 	if ((t->feature & FEATURE_PSR) == 0)
 		flags &= ~PSR_ASSERT_FLAGS;
@@ -2928,7 +2929,7 @@ static void stridechange_subtest(const struct test_mode *t)
 	fill_fb_region(&params->primary, COLOR_PRIM_BG);
 
 	set_mode_for_params(params);
-	do_assertions(DONT_ASSERT_FEATURE_STATUS);
+	do_assertions(DONT_ASSERT_FBC_STATUS);
 
 	/* Go back to the fb that can have FBC. */
 	params->primary.fb = old_fb;
@@ -2938,7 +2939,7 @@ static void stridechange_subtest(const struct test_mode *t)
 	/* This operation is the same as above, but with the planes API. */
 	params->primary.fb = new_fb;
 	set_prim_plane_for_params(params);
-	do_assertions(DONT_ASSERT_FEATURE_STATUS);
+	do_assertions(DONT_ASSERT_FBC_STATUS);
 
 	params->primary.fb = old_fb;
 	set_prim_plane_for_params(params);
@@ -2950,7 +2951,7 @@ static void stridechange_subtest(const struct test_mode *t)
 	 */
 	rc = drmModePageFlip(drm.fd, drm.display.pipes[params->pipe].crtc_id, new_fb->fb_id, 0, NULL);
 	igt_assert(rc == -EINVAL || rc == 0);
-	do_assertions(DONT_ASSERT_FEATURE_STATUS);
+	do_assertions(DONT_ASSERT_FBC_STATUS);
 }
 
 /**
-- 
2.21.0

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

[igt-dev] ✓ Fi.CI.BAT: success for series starting with [i-g-t,1/2] tests/frontbuffer_tracking: Do not assert FBC state after a page flip changing stride

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t,1/2] tests/frontbuffer_tracking: Do not assert FBC state after a page flip changing stride
URL   : https://patchwork.freedesktop.org/series/58701/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5832 -> IGTPW_2733
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/58701/revisions/1/mbox/

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s3:
    - fi-icl-u3:          PASS -> FAIL [fdo#103375]

  * igt@kms_frontbuffer_tracking@basic:
    - fi-byt-clapper:     PASS -> FAIL [fdo#103167]

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

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

  
#### Possible fixes ####

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

  
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718


Participating hosts (43 -> 39)
------------------------------

  Missing    (4): fi-kbl-soraka fi-ilk-m540 fi-bsw-cyan fi-hsw-4200u 


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

    * IGT: IGT_4911 -> IGTPW_2733

  CI_DRM_5832: f1fc30ad3723a8b6265c2edf50a7f637ecd75a23 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2733: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2733/
  IGT_4911: d9fe699ea45406e279b78d1afdb4d57a205a3c99 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools

== Logs ==

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

[igt-dev] ✓ Fi.CI.IGT: success for series starting with [i-g-t,1/2] tests/frontbuffer_tracking: Do not assert FBC state after a page flip changing stride

[Patchwork]

== Series Details ==

Series: series starting with [i-g-t,1/2] tests/frontbuffer_tracking: Do not assert FBC state after a page flip changing stride
URL   : https://patchwork.freedesktop.org/series/58701/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_5832_full -> IGTPW_2733_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/58701/revisions/1/mbox/

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

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

### IGT changes ###

#### Issues hit ####

  * igt@kms_atomic_transition@3x-modeset-transitions:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +12

  * igt@kms_atomic_transition@6x-modeset-transitions-fencing:
    - shard-glk:          NOTRUN -> SKIP [fdo#109271] / [fdo#109278]

  * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a:
    - shard-glk:          PASS -> DMESG-WARN [fdo#110222]
    - shard-hsw:          PASS -> DMESG-WARN [fdo#110222]
    - shard-kbl:          PASS -> DMESG-WARN [fdo#110222]

  * igt@kms_fbcon_fbt@fbc:
    - shard-glk:          NOTRUN -> SKIP [fdo#109271] +14

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

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

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-shrfb-pgflip-blt:
    - shard-snb:          NOTRUN -> SKIP [fdo#109271] +100

  * igt@kms_rotation_crc@multiplane-rotation:
    - shard-kbl:          PASS -> INCOMPLETE [fdo#103665]

  * igt@kms_rotation_crc@multiplane-rotation-cropping-top:
    - shard-kbl:          PASS -> FAIL [fdo#109016]

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

  * igt@kms_vblank@pipe-c-ts-continuation-dpms-suspend:
    - shard-kbl:          PASS -> FAIL [fdo#104894]

  * igt@kms_vblank@pipe-c-ts-continuation-suspend:
    - shard-apl:          PASS -> FAIL [fdo#104894] +1

  * igt@perf_pmu@busy-hang-vcs1:
    - shard-apl:          NOTRUN -> SKIP [fdo#109271]

  
#### Possible fixes ####

  * igt@gem_create@create-clear:
    - shard-snb:          INCOMPLETE [fdo#105411] -> PASS

  * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a:
    - shard-snb:          DMESG-WARN [fdo#110222] -> PASS +2

  * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-b:
    - shard-kbl:          DMESG-WARN [fdo#110222] -> PASS +1
    - shard-hsw:          DMESG-WARN [fdo#110222] -> PASS

  * igt@kms_cursor_crc@cursor-128x128-dpms:
    - shard-kbl:          FAIL [fdo#103232] -> PASS
    - shard-apl:          FAIL [fdo#103232] -> PASS

  * igt@kms_draw_crc@draw-method-xrgb8888-render-xtiled:
    - shard-glk:          FAIL [fdo#107791] -> PASS

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

  * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-shrfb-plflip-blt:
    - shard-glk:          FAIL [fdo#103167] -> PASS

  * igt@kms_frontbuffer_tracking@fbc-stridechange:
    - shard-hsw:          FAIL [fdo#105682] -> PASS

  * igt@kms_plane_scaling@pipe-a-scaler-with-pixel-format:
    - shard-glk:          SKIP [fdo#109271] / [fdo#109278] -> PASS +2

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

  * igt@kms_vblank@pipe-a-ts-continuation-modeset:
    - shard-apl:          FAIL [fdo#104894] -> PASS
    - shard-kbl:          FAIL [fdo#104894] -> PASS

  
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#104894]: https://bugs.freedesktop.org/show_bug.cgi?id=104894
  [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#107791]: https://bugs.freedesktop.org/show_bug.cgi?id=107791
  [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#110222]: https://bugs.freedesktop.org/show_bug.cgi?id=110222
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (10 -> 5)
------------------------------

  Missing    (5): shard-skl pig-hsw-4770r pig-glk-j5005 shard-iclb pig-skl-6260u 


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

    * IGT: IGT_4911 -> IGTPW_2733
    * Piglit: piglit_4509 -> None

  CI_DRM_5832: f1fc30ad3723a8b6265c2edf50a7f637ecd75a23 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_2733: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2733/
  IGT_4911: d9fe699ea45406e279b78d1afdb4d57a205a3c99 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

Re: [igt-dev] [PATCH i-g-t 1/2] tests/frontbuffer_tracking: Do not assert FBC state after a page flip changing stride

[Dhinakaran Pandiyan]

On Thu, 2019-03-28 at 15:28 -0700, José Roberto de Souza wrote:
> When the stridechange subtest was introduced f23ea58f1fbb
> ("kms_frontbuffer_tracking: expand badstride and stridechange")
> atomic was not around and change the stride using drmModePageFlip()
> was not allowed so it was expected that it would return -EINVAL and
> kernel would keep the old framebuffer with the smaller plane that is
> know to fit on CFB(if it don't fit the test will skip on the first
> full-modeset because "not enough stolen memory" is set).
> 
> But after the introduction of atomic the subtest was updated by
> f63e070b469d ("kms_frontbuffer_tracking: Fix tests with the new
> atomic reality.") to accept a no error return from drmModePageFlip()
> but the do_assertions() that follows it was not updated.
> As the subtest function comment states, kernel will do fastsets in
> this scenario and the allocated CFB could not be enough to keep FBC
> enabled over the new framebuffer, so here adding the missing
> DONT_ASSERT_FEATURE_STATUS to ignore the FBC state and just test if
> CRC match and if kernel do not misbehave.
> 
> Other way to solve this issue would be make the kernel do a
> full-modeset when CFB is not enough for the new plane so FBC is
> disabled with the CRC freeing the actual CFB and then after enable
> CRTC again it will try to enable FBC again if it can allocate the
> required CFB but by the subtest comment this is not intended.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105683
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  tests/kms_frontbuffer_tracking.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/kms_frontbuffer_tracking.c
> b/tests/kms_frontbuffer_tracking.c
> index 4d15ce1c..087fc473 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -2950,7 +2950,7 @@ static void stridechange_subtest(const struct test_mode
> *t)
>  	 */
>  	rc = drmModePageFlip(drm.fd, drm.display.pipes[params->pipe].crtc_id,
> new_fb->fb_id, 0, NULL);
>  	igt_assert(rc == -EINVAL || rc == 0);
> -	do_assertions(0);
> +	do_assertions(DONT_ASSERT_FEATURE_STATUS);
The assertion is still valid if the page flip ioctl failed, isn't it?

-DK

>  }
>  
>  /**

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

Re: [igt-dev] [PATCH i-g-t 2/2] tests/frontbuffer_tracking: Assert the status of other features on stridechange subtest

[Dhinakaran Pandiyan]

On Thu, 2019-03-28 at 15:28 -0700, José Roberto de Souza wrote:
> As explained in c1edee186d18 ("tests/frontbuffer_tracking: Do not
> assert FBC state after a page flip changing stride") after changing
> the plane stride there is the possibility that CFB will not be big
> enough to keep FBC enabled, that is why do_assertions() is called
> with DONT_ASSERT_FEATURE_STATUS but DONT_ASSERT_FEATURE_STATUS is
> overkill and will not check the status of the other features like PSR
> and DRRS when running combined feature tests and possibly hiding
> bugs.
> 
> So lets add a new flag that will only not assert FBC.
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  tests/kms_frontbuffer_tracking.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/kms_frontbuffer_tracking.c
> b/tests/kms_frontbuffer_tracking.c
> index 087fc473..0dcde097 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -1527,6 +1527,7 @@ static void do_flush(const struct test_mode *t)
>  
>  #define DONT_ASSERT_CRC			(1 << 0)
>  #define DONT_ASSERT_FEATURE_STATUS	(1 << 1)
> +#define DONT_ASSERT_FBC_STATUS		(1 << 12)
>  
>  #define FBC_ASSERT_FLAGS		(0xF << 2)
>  #define ASSERT_FBC_ENABLED		(1 << 2)
> @@ -1557,7 +1558,7 @@ static int adjust_assertion_flags(const struct test_mode
> *t, int flags)
>  			flags |= ASSERT_DRRS_HIGH;
>  	}
>  
> -	if ((t->feature & FEATURE_FBC) == 0)
> +	if ((t->feature & FEATURE_FBC) == 0 || (flags & DONT_ASSERT_FBC_STATUS))
>  		flags &= ~FBC_ASSERT_FLAGS;
>  	if ((t->feature & FEATURE_PSR) == 0)
>  		flags &= ~PSR_ASSERT_FLAGS;
> @@ -2928,7 +2929,7 @@ static void stridechange_subtest(const struct test_mode
> *t)
>  	fill_fb_region(&params->primary, COLOR_PRIM_BG);
>  
>  	set_mode_for_params(params);
> -	do_assertions(DONT_ASSERT_FEATURE_STATUS);
> +	do_assertions(DONT_ASSERT_FBC_STATUS);
>  
>  	/* Go back to the fb that can have FBC. */
>  	params->primary.fb = old_fb;
> @@ -2938,7 +2939,7 @@ static void stridechange_subtest(const struct test_mode
> *t)
>  	/* This operation is the same as above, but with the planes API. */
>  	params->primary.fb = new_fb;
>  	set_prim_plane_for_params(params);
> -	do_assertions(DONT_ASSERT_FEATURE_STATUS);
> +	do_assertions(DONT_ASSERT_FBC_STATUS);
>  
>  	params->primary.fb = old_fb;
>  	set_prim_plane_for_params(params);
> @@ -2950,7 +2951,7 @@ static void stridechange_subtest(const struct test_mode
> *t)
>  	 */
>  	rc = drmModePageFlip(drm.fd, drm.display.pipes[params->pipe].crtc_id,
> new_fb->fb_id, 0, NULL);
>  	igt_assert(rc == -EINVAL || rc == 0);
> -	do_assertions(DONT_ASSERT_FEATURE_STATUS);
> +	do_assertions(DONT_ASSERT_FBC_STATUS);

Isn't this the same as
(DONT_ASSERT_FEATURE_STATUS || ASSERT_PSR_ENABLED || ASSERT_DRRS_HIGH)?

Explicit about what features are asserted but is verbose.

>  
>  /**

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

Re: [igt-dev] [PATCH i-g-t 1/2] tests/frontbuffer_tracking: Do not assert FBC state after a page flip changing stride

[Souza, Jose]

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

On Tue, 2019-04-02 at 11:49 -0700, Dhinakaran Pandiyan wrote:
> On Thu, 2019-03-28 at 15:28 -0700, José Roberto de Souza wrote:
> > When the stridechange subtest was introduced f23ea58f1fbb
> > ("kms_frontbuffer_tracking: expand badstride and stridechange")
> > atomic was not around and change the stride using drmModePageFlip()
> > was not allowed so it was expected that it would return -EINVAL and
> > kernel would keep the old framebuffer with the smaller plane that
> > is
> > know to fit on CFB(if it don't fit the test will skip on the first
> > full-modeset because "not enough stolen memory" is set).
> > 
> > But after the introduction of atomic the subtest was updated by
> > f63e070b469d ("kms_frontbuffer_tracking: Fix tests with the new
> > atomic reality.") to accept a no error return from
> > drmModePageFlip()
> > but the do_assertions() that follows it was not updated.
> > As the subtest function comment states, kernel will do fastsets in
> > this scenario and the allocated CFB could not be enough to keep FBC
> > enabled over the new framebuffer, so here adding the missing
> > DONT_ASSERT_FEATURE_STATUS to ignore the FBC state and just test if
> > CRC match and if kernel do not misbehave.
> > 
> > Other way to solve this issue would be make the kernel do a
> > full-modeset when CFB is not enough for the new plane so FBC is
> > disabled with the CRC freeing the actual CFB and then after enable
> > CRTC again it will try to enable FBC again if it can allocate the
> > required CFB but by the subtest comment this is not intended.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105683
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  tests/kms_frontbuffer_tracking.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tests/kms_frontbuffer_tracking.c
> > b/tests/kms_frontbuffer_tracking.c
> > index 4d15ce1c..087fc473 100644
> > --- a/tests/kms_frontbuffer_tracking.c
> > +++ b/tests/kms_frontbuffer_tracking.c
> > @@ -2950,7 +2950,7 @@ static void stridechange_subtest(const struct
> > test_mode
> > *t)
> >  	 */
> >  	rc = drmModePageFlip(drm.fd, drm.display.pipes[params-
> > >pipe].crtc_id,
> > new_fb->fb_id, 0, NULL);
> >  	igt_assert(rc == -EINVAL || rc == 0);
> > -	do_assertions(0);
> > +	do_assertions(DONT_ASSERT_FEATURE_STATUS);
> The assertion is still valid if the page flip ioctl failed, isn't it?

Huum yeah, I guess non-atomic drivers would also return a error before
changing the hardware state.

So change to:

do_assertions(rc ? 0 : DONT_ASSERT_FEATURE_STATUS);


> 
> -DK
> 
> >  }
> >  
> >  /**

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 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 2/2] tests/frontbuffer_tracking: Assert the status of other features on stridechange subtest

[Souza, Jose]

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

On Tue, 2019-04-02 at 12:26 -0700, Dhinakaran Pandiyan wrote:
> On Thu, 2019-03-28 at 15:28 -0700, José Roberto de Souza wrote:
> > As explained in c1edee186d18 ("tests/frontbuffer_tracking: Do not
> > assert FBC state after a page flip changing stride") after changing
> > the plane stride there is the possibility that CFB will not be big
> > enough to keep FBC enabled, that is why do_assertions() is called
> > with DONT_ASSERT_FEATURE_STATUS but DONT_ASSERT_FEATURE_STATUS is
> > overkill and will not check the status of the other features like
> > PSR
> > and DRRS when running combined feature tests and possibly hiding
> > bugs.
> > 
> > So lets add a new flag that will only not assert FBC.
> > 
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  tests/kms_frontbuffer_tracking.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tests/kms_frontbuffer_tracking.c
> > b/tests/kms_frontbuffer_tracking.c
> > index 087fc473..0dcde097 100644
> > --- a/tests/kms_frontbuffer_tracking.c
> > +++ b/tests/kms_frontbuffer_tracking.c
> > @@ -1527,6 +1527,7 @@ static void do_flush(const struct test_mode
> > *t)
> >  
> >  #define DONT_ASSERT_CRC			(1 << 0)
> >  #define DONT_ASSERT_FEATURE_STATUS	(1 << 1)
> > +#define DONT_ASSERT_FBC_STATUS		(1 << 12)
> >  
> >  #define FBC_ASSERT_FLAGS		(0xF << 2)
> >  #define ASSERT_FBC_ENABLED		(1 << 2)
> > @@ -1557,7 +1558,7 @@ static int adjust_assertion_flags(const
> > struct test_mode
> > *t, int flags)
> >  			flags |= ASSERT_DRRS_HIGH;
> >  	}
> >  
> > -	if ((t->feature & FEATURE_FBC) == 0)
> > +	if ((t->feature & FEATURE_FBC) == 0 || (flags &
> > DONT_ASSERT_FBC_STATUS))
> >  		flags &= ~FBC_ASSERT_FLAGS;
> >  	if ((t->feature & FEATURE_PSR) == 0)
> >  		flags &= ~PSR_ASSERT_FLAGS;
> > @@ -2928,7 +2929,7 @@ static void stridechange_subtest(const struct
> > test_mode
> > *t)
> >  	fill_fb_region(&params->primary, COLOR_PRIM_BG);
> >  
> >  	set_mode_for_params(params);
> > -	do_assertions(DONT_ASSERT_FEATURE_STATUS);
> > +	do_assertions(DONT_ASSERT_FBC_STATUS);
> >  
> >  	/* Go back to the fb that can have FBC. */
> >  	params->primary.fb = old_fb;
> > @@ -2938,7 +2939,7 @@ static void stridechange_subtest(const struct
> > test_mode
> > *t)
> >  	/* This operation is the same as above, but with the planes
> > API. */
> >  	params->primary.fb = new_fb;
> >  	set_prim_plane_for_params(params);
> > -	do_assertions(DONT_ASSERT_FEATURE_STATUS);
> > +	do_assertions(DONT_ASSERT_FBC_STATUS);
> >  
> >  	params->primary.fb = old_fb;
> >  	set_prim_plane_for_params(params);
> > @@ -2950,7 +2951,7 @@ static void stridechange_subtest(const struct
> > test_mode
> > *t)
> >  	 */
> >  	rc = drmModePageFlip(drm.fd, drm.display.pipes[params-
> > >pipe].crtc_id,
> > new_fb->fb_id, 0, NULL);
> >  	igt_assert(rc == -EINVAL || rc == 0);
> > -	do_assertions(DONT_ASSERT_FEATURE_STATUS);
> > +	do_assertions(DONT_ASSERT_FBC_STATUS);
> 
> Isn't this the same as
> (DONT_ASSERT_FEATURE_STATUS || ASSERT_PSR_ENABLED ||
> ASSERT_DRRS_HIGH)?

This would work as ASSERT_PSR_ENABLED and ASSERT_DRRS_HIGH would be
cleared when running this test without this features but have
DONT_ASSERT_FEATURE_STATUS with ASSERT_PSR_ENABLED and ASSERT_DRRS_HIGH
is contradictory to each other.

> 
> Explicit about what features are asserted but is verbose.
> 
> >  
> >  /**

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 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 1/2] tests/frontbuffer_tracking: Do not assert FBC state after a page flip changing stride

[Dhinakaran Pandiyan]

On Tue, 2019-04-02 at 12:46 -0700, Souza, Jose wrote:
> On Tue, 2019-04-02 at 11:49 -0700, Dhinakaran Pandiyan wrote:
> > On Thu, 2019-03-28 at 15:28 -0700, José Roberto de Souza wrote:
> > > When the stridechange subtest was introduced f23ea58f1fbb
> > > ("kms_frontbuffer_tracking: expand badstride and stridechange")
> > > atomic was not around and change the stride using drmModePageFlip()
> > > was not allowed so it was expected that it would return -EINVAL and
> > > kernel would keep the old framebuffer with the smaller plane that
> > > is
> > > know to fit on CFB(if it don't fit the test will skip on the first
> > > full-modeset because "not enough stolen memory" is set).
> > > 
> > > But after the introduction of atomic the subtest was updated by
> > > f63e070b469d ("kms_frontbuffer_tracking: Fix tests with the new
> > > atomic reality.") to accept a no error return from
> > > drmModePageFlip()
> > > but the do_assertions() that follows it was not updated.
> > > As the subtest function comment states, kernel will do fastsets in
> > > this scenario and the allocated CFB could not be enough to keep FBC
> > > enabled over the new framebuffer, so here adding the missing
> > > DONT_ASSERT_FEATURE_STATUS to ignore the FBC state and just test if
> > > CRC match and if kernel do not misbehave.
> > > 
> > > Other way to solve this issue would be make the kernel do a
> > > full-modeset when CFB is not enough for the new plane so FBC is
> > > disabled with the CRC freeing the actual CFB and then after enable
> > > CRTC again it will try to enable FBC again if it can allocate the
> > > required CFB but by the subtest comment this is not intended.
> > > 
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105683
> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  tests/kms_frontbuffer_tracking.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/kms_frontbuffer_tracking.c
> > > b/tests/kms_frontbuffer_tracking.c
> > > index 4d15ce1c..087fc473 100644
> > > --- a/tests/kms_frontbuffer_tracking.c
> > > +++ b/tests/kms_frontbuffer_tracking.c
> > > @@ -2950,7 +2950,7 @@ static void stridechange_subtest(const struct
> > > test_mode
> > > *t)
> > >  	 */
> > >  	rc = drmModePageFlip(drm.fd, drm.display.pipes[params-
> > > > pipe].crtc_id,
> > > 
> > > new_fb->fb_id, 0, NULL);
> > >  	igt_assert(rc == -EINVAL || rc == 0);
> > > -	do_assertions(0);
> > > +	do_assertions(DONT_ASSERT_FEATURE_STATUS);
> > 
> > The assertion is still valid if the page flip ioctl failed, isn't it?
> 
> Huum yeah, I guess non-atomic drivers would also return a error before
> changing the hardware state.
> 
> So change to:
> 
> do_assertions(rc ? 0 : DONT_ASSERT_FEATURE_STATUS);
> 
Yeah, that should work.

-DK
> 
> > 
> > -DK
> > 
> > >  }
> > >  
> > >  /**

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