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

["Souza, Jose" <jose.souza@intel.com>]

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