[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 at linux.intel.com>
> > > Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza at 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
> > 
> > >  }
> > >  
> > >  /**