[igt-dev] [PATCH i-g-t 7/7] tests/kms_ccs: Larger fb to fully cover up the primary plane

[Dhinakaran Pandiyan]

On Sat, 2019-02-23 at 15:43 +0200, Juha-Pekka Heikkilä wrote:
> 
> Clinton Taylor kirjoitti 22.2.2019 klo 18.18:
> > 
> > On 2/22/19 6:19 AM, Juha-Pekka Heikkila wrote:
> > > I wouldn't go reverting this limitation. Idea of this test is to
> > > see 
> > > if ccs work but not how big planes can be used with ccs. Matt's
> > > patch 
> > > was to fix that two planes 1080p problem on APL but if you go see
> > > the 
> > > machine https://intel-gfx-ci.01.org/hardware.html#fi-apl-guc
> > > you'll 
> > > notice it support 4096x2160 at 60Hz at highest. After removing this 
> > > limitation all you need to do to fail this test is to change the
> > > monitor.
> > > 
> > 
> > The test has been changed to the current active size of the pipe
> > and not 
> > the maximum size supported by the hardware.
> > 
> 
> I didn't understand the reasoning. This test did fail on different 
> machines because of plane sizes which required more bandwith than
> was 
> available in budget.

There is just 
 one primary plane scanning out a fb sized to match connector's default
mode
 an overlay scanning out a fb of size 256x256. 

This to me is a reasonable configuration that should not exceeding any
hardware limits. The commit that changes the primary fb size does not
specifically call out which platforms the problem was on and what exact
hardware limit the test exceeds. 

>  Watermark calculation was adjusted but it doesn't 
> change limitations on hardware.

Looking at the bug, it does seem like the real problem was the second
plane not meeting the minimum watermark limits due to a buggy watermark
algorithm. And that problem has been fixed. If you see the original
list of failures, it is across all platforms and the test fails only
when both primary and sprite plane are enabled. 

> 
> Is there benefit in reindroducing this point of failure?
There is not much point in testing an artificial configuration (2000 *
vdisplay) in order to make the test pass. We should know when a sane
configuration fails so that it can be debugged.

> 
> /Juha-Pekka
> 
> > 
> > 
> > > This igt test was originally limited due to this machine 
> > > https://intel-gfx-ci.01.org/hardware.html#fi-kbl-soraka which is
> > > meant 
> > > to be Chromebook with 2400x1600 size display. Quickly looking it
> > > seems 
> > > Soraka has not been recently running so much CI runs though.
> > > 
> > > /Juha-Pekka
> > > 
> > > On 21.2.2019 16.41, Dhinakaran Pandiyan wrote:
> > > > This is a revert of commit 27fa97d16294 ("tests/kms_ccs: Avoid
> > > > using 
> > > > plane
> > > > sizes which exceed hw capability"). The watermarks issue that
> > > > the patch
> > > > refers to has been fixed as per the bugzilla, so let's use an
> > > > fb that
> > > > covers the whole screen.
> > > > 
> > > > Cc: Clinton Taylor <clinton.a.taylor at intel.com>
> > > > Cc: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
> > > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > References: https://bugs.freedesktop.org/show_bug.cgi?id=105458
> > > > Signed-off-by: Dhinakaran Pandiyan <
> > > > dhinakaran.pandiyan at intel.com>
> > > > ---
> > > >   tests/kms_ccs.c | 16 +++-------------
> > > >   1 file changed, 3 insertions(+), 13 deletions(-)
> > > > 
> > > > diff --git a/tests/kms_ccs.c b/tests/kms_ccs.c
> > > > index 8cbf100f..48f10af0 100644
> > > > --- a/tests/kms_ccs.c
> > > > +++ b/tests/kms_ccs.c
> > > > @@ -69,15 +69,6 @@ const struct {
> > > >       {0.0, 1.0, 0.0}
> > > >   };
> > > >   -/*
> > > > - * Limit maximum used sprite plane width so this test will
> > > > not 
> > > > mistakenly
> > > > - * fail on hardware limitations which are not interesting to
> > > > this test.
> > > > - * On this test too wide sprite plane may fail during creation
> > > > with 
> > > > dmesg
> > > > - * comment saying:
> > > > - * "Requested display configuration exceeds system watermark 
> > > > limitations"
> > > > - */
> > > > -#define MAX_SPRITE_PLANE_WIDTH 2000
> > > > -
> > > >   struct local_drm_format_modifier {
> > > >          /* Bitmask of formats in get_plane format list this
> > > > info 
> > > > applies to. The
> > > >       * offset allows a sliding window of which 64 formats
> > > > (bits).
> > > > @@ -313,13 +304,12 @@ static bool try_config(data_t *data,
> > > > enum 
> > > > test_fb_flags fb_flags,
> > > >       if (data->plane && fb_flags & FB_COMPRESSED) {
> > > >           if (!plane_has_format_with_ccs(data, data->plane, 
> > > > DRM_FORMAT_XRGB8888))
> > > >               return false;
> > > > -        generate_fb(data, &fb, min(MAX_SPRITE_PLANE_WIDTH, 
> > > > drm_mode->hdisplay),
> > > > -                drm_mode->vdisplay,
> > > > +        generate_fb(data, &fb, drm_mode->hdisplay, drm_mode-
> > > > >vdisplay,
> > > >                   (fb_flags & ~FB_COMPRESSED) | FB_HAS_PLANE);
> > > >           generate_fb(data, &fb_sprite, 256, 256, fb_flags);
> > > >       } else {
> > > > -        generate_fb(data, &fb, min(MAX_SPRITE_PLANE_WIDTH, 
> > > > drm_mode->hdisplay),
> > > > -                drm_mode->vdisplay, fb_flags);
> > > > +        generate_fb(data, &fb, drm_mode->hdisplay, drm_mode-
> > > > >vdisplay,
> > > > +                fb_flags);
> > > >       }
> > > >         if (data->flags & TEST_FAIL_ON_ADDFB2)
> > > >