[igt-dev] [PATCH] drm/doc: Make igts for cross-driver stuff mandatory

Mon Jan 21 17:21:12 UTC 2019

On Mon, Jan 21, 2019 at 12:54 PM Brian Starkey <Brian.Starkey at arm.com> wrote:
>
> Hi Daniel,
>
> On Thu, Jan 17, 2019 at 12:52:16PM +0100, Daniel Vetter wrote:
> > On Thu, Jan 17, 2019 at 12:38 PM Liviu Dudau <liviu.dudau at arm.com> wrote:
> > >
> > > On Wed, Jan 16, 2019 at 05:39:03PM +0100, Daniel Vetter wrote:
> > > > Compared to the RFC[1] no changes to the patch itself, but igt moved
> > > > forward a lot:
> > > >
> > > > - gitlab CI builds with: reduced configs/libraries, arm cross build
> > > >   and a sysroot build (should address all the build/cross platform
> > > >   concerns raised in the RFC discussions).
> > > >
> > > > - tests reorganized into subdirectories so that the i915-gem tests
> > > >   don't clog the main/shared tests directory anymore
> > > >
> > > > - quite a few more non-intel people contributing/reviewing/committing
> > > >   igt tests patches.
> > > >
> > > > I think this addresses all the concerns raised in the RFC discussions,
> > > > and assuming there's enough Acks and no new issues that pop up, we can
> > > > go ahead with this.
> > > >
> > > > 1: https://patchwork.kernel.org/patch/10648851/
> > > > Cc: Petri Latvala <petri.latvala at intel.com>
> > > > Cc: Arkadiusz Hiler <arkadiusz.hiler at intel.com>
> > > > Cc: Liviu Dudau <liviu.dudau at arm.com>
> > > > Cc: Sean Paul <sean at poorly.run>
> > > > Cc: Eric Anholt <eric at anholt.net>
> > > > Cc: Alex Deucher <alexander.deucher at amd.com>
> > > > Cc: Dave Airlie <airlied at redhat.com>
> > > > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > > > ---
> > > >  Documentation/gpu/drm-uapi.rst | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> > > > index a752aa561ea4..413915d6b7d2 100644
> > > > --- a/Documentation/gpu/drm-uapi.rst
> > > > +++ b/Documentation/gpu/drm-uapi.rst
> > > > @@ -238,6 +238,13 @@ DRM specific patterns. Note that ENOTTY has the slightly unintuitive meaning of
> > > >  Testing and validation
> > > >  ======================
> > > >
> > > > +Testing Requirements for userspace API
> > > > +--------------------------------------
> > > > +
> > > > +New cross-driver userspace interface extensions, like new IOCTL, new KMS
> > > > +properties, new files in sysfs or anything else that constitutes an API change
> > > > +need to have driver-agnostic testcases in IGT for that feature.
> > >
> > > From an aspirational point of view I am fine with this and you can have
> > > my Acked-by: Liviu Dudau <liviu.dudau at arm.com>.
> > >
> > > From a practical point of view I would like to see a matrix of KMS APIs
> > > that are being validated and the drivers that have been tested. Otherwise,
> > > the next person that comes and tries to add a new IOCTL, KMS property or new
> > > file in sysfs is going to discover that he has subscribed to a much bigger
> > > task of getting enough KMS drivers testable in the first place.
> >
> > This is what the _new_ features is about, no expectation to write
> > tests for all the existing stuff. Although I think there's not really
> > any big gaps in igt anymore, we do have at least some (rather rough
> > and coarse in some case) test coverage for everything I think. Should
> > this be clarified further?
> > -Daniel
> >
>
> I share a similar view to Liviu here. I think this new requirement
> raises the bar more than you intended.
>
> By saying that all new features must be tested by igt, you're also
> implying that a driver must run igt (at some basic level); before the
> developers working on that driver can start trying to implement new
> features. That puts an additional hurdle in the way of adding stuff
> to KMS for people who aren't already using igt.
>
> I'm all for testing, and UAPI being well proven before we merge it,
> and even for a central KMS test suite. However, when we (Arm Mali-DP
> people) have tried to implement things in igt it's been a battle,
> because of various built-in assumptions which it made.
>
> For example, most meaningful igt tests rely on CRC. Much of our HW
> doesn't have CRC. CRC could be implemented in theory using writeback,
> but that currently doesn't exist. That means you're effectively saying
> that we (Arm) can't implement any new cross-device KMS features until
> we've either:
>
>  a) also implemented writeback-based CRC in igt OR
>  b) implemented the new feature in someone else's driver which does
>     support CRC.

We didn't just pick crcs for lols (or because that's all intel
supports), we picked it because it will work for both hw with crc and
hw with writeback. I checked with a pile of driver writers way back
(over irc), and the interface we picked is something pretty much all
display blocks (except the _very_ simple ones) should be able to
support. Same discussion also happened again when made the crc
interfaces in debugfs more generic.

> That seems a bit out of order to me. It would be like me saying "all
> KMS drivers must use Arm's test suite, which uses writeback and pixel
> checking", and you'd be in a pickle because you don't have writeback.
>
> In a similar vein, I remember having to fix igt on devices which
> didn't have cursor planes, before I could even think about writing
> tests.
>
> If you can prove that issues like these are all resolved now in igt,
> then great! Otherwise, I don't think igt is "ready" to be used as a
> requirement for merging KMS kernel API.

I don't think this is realistic (or Liviu's take on this): Expecting
that igt just works on all drivers, and that driver writers don't have
to do some ramp-up on a testsuite code base is just not going to
happen. If we go with a testsuite then there
- will be some ramp up required for people who've never used it
- it won't work on every driver under the sun

The later could perhaps be fixed if we go khronos-style on uapi
design, but pls note that the requirements for ratifying new khr
extensions are _much_ stricter than what we have right now, or what's
proposed here. Plus khr essentially got the testsuite for old gl
versions donated by google to them (they still don't have one for
legacy gl, only for modern gl and all versions of gles).

Also, the fundamental difference between igt and all these other kms
test suites is that they're all not open source. We're not going to
use a closed source to validate i915, nor are we going to pour
engineering resources into that.

So from a theoretical pov I think your argument has some merit, in
practice it boils down to "let's not have a test suite, mkay". Is that
your stance here?
-Daniel

>
> Thanks,
> -Brian
>
> > >
> > > Best regards,
> > > Liviu
> > >
> > >
> > > > +
> > > >  Validating changes with IGT
> > > >  ---------------------------
> > > >
> > > > --
> > > > 2.20.1
> > > >
> > >
> > > --
> > > ====================
> > > | I would like to |
> > > | fix the world,  |
> > > | but they're not |
> > > | giving me the   |
> > >  \ source code!  /
> > >   ---------------
> > >     ¯\_(ツ)_/¯
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch