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

Tue Jan 22 08:53:20 UTC 2019

On Mon, Jan 21, 2019 at 6:21 PM Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
>
> 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.

With a night's worth of sleep, this here is what annoys me - you
demand I "prove" that igt works everywhere. That's not realistic.
Intel is not going to pay the bill to get a CI farm for every drm
driver existing up&running, including fixing all the bugs that will
uncover (both in igt and even more so in drivers). Especially not if
mere mortals can't even buy the hardware. Nor is anyone else going to
do that. If there are some fundamental overall issues with igt then
I'm ofc happy to get them addressed (like the build issues raised a
few months ago).

There's also the options to somehow find funding for an igt clone, but
if you just find a bit of fund then realistically it's more effective
to throw it at igt. That's how google funded making igt work on
non-i915 drivers at least. Or you manage to open source one of the
existing internal/proprietary test suites, but I think that's going to
be worse than a fresh clone since on top of finding money you also
have to wait a few years for legal review.
-Danie

> 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

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