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

Tue Jan 22 14:03:59 UTC 2019

On Tue, Jan 22, 2019 at 2:27 PM Brian Starkey <Brian.Starkey at arm.com> wrote:
>
> Hi,
>
> On Tue, Jan 22, 2019 at 09:53:20AM +0100, Daniel Vetter wrote:
> > 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 doesn't really address my issue, but no matter. I guess I'm
> having a hard time separating the existing igts from _new_ igts for
> new features; so sorry about that.
>
> Please appreciate that there's honestly a whole lot of work needed to
> make existing igts work for CRC using writeback. There's no "nonblock"
> or continuous CRC collection using writeback. You also need to request
> your CRC before you make your commit, so that the FB can be attached
> to the writeback connector. Fixing this up isn't small or trivial,
> it's not just a case of "ramp-up", it's pervasive throughout the igt
> tree.
>
> That shouldn't need to impact your proposal for new tests, if they are
> written with that in mind.

Hm, I've figured the simplest way to make this happen is to enable
writeback from the debugfs crc interface, and instead of just
collecting the crc from the writeback completion interrupt, you first
feed the entire framebuffer into a checksum function. That code
shouldn't need a full rewrite of igt, and is kinda how I envisaged
writeback-for-igt-crc would pan out.

It's at least how we've done it for vkms, and there the
compress-fb-into-crc is a few lines of code. Most important bit is
that you don't sample invisible stuff, i.e. don't sample the X in
XRGB8888 (which is what vkms uses internally right now), and to not
sampel the invisible stuff outside of the fb. We could put this into a
drm_framebuffer_helper.c, Noralf has a bunch of other related fb
helper functions in tinydrm which would also neatly fit in there.

It could be that this isn't fast enough to keep up with vblank, but
for all the single-shot crc tests it should be good enough. Old igt
had the mishabit of asking for multiple crcs (to hack around i915
issues), but I thought we've addressed all that when creating the
generic debugfs crc interfaces. The other issue is that you probably
can't enable writeback in all cases (some chips you can enable
writeback as a sidechannel to any active crtc, some need a dedicated
thing). I do think igt can cope with "sry no crc for this config"
though and should skip these (sub)tests if it's just not possible.

Rewriting all of igt to make writeback-only display blocks work with
them sounds like the wrong approach. Or at least one we should try
really hard to avoid.

> > > > 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).
>
> I'm really not asking you to. I'm sorry that I've annoyed you, and I'm
> not being deliberately awkward.
>
> I genuinely don't know what the current state of "not-i915" testing is
> in igt. Do you really not think it's a good idea to have that known
> and documented before you make igt a mandatory part of the DRM tree?
>
> Sure, there's commits from non-intel folks - heck there's even a few
> from me. But I can tell you right away that doesn't mean that igt
> works in any meaningful way on my platform. Does it work well enough
> for me to add new test cases? Honestly I don't know.
>
> Maybe you can suggest some suites which are expected to already be
> fully generic and should run anywhere which we can use as
> confirmation? To me, having some reasonable subset of the active
> driver devs building and running that on their platforms and reporting
> back before you merge this patch wouldn't be unreasonable or
> outlandish.

So my Grand Plan is to get vkms up to par, and even get that
integrated into igt and drm autobuilds we'll run on gitlab CI. But
vkms is still a few internships away from being useful, atm it has one
primary plane and one non-plane cursor (it's not even a universal
cursor plane yet). Will give you _lots_ of skips.

But I guess we could fairly easily add vkms as one of the machines
we're testing to intel's CI farm, which means every patch and every
commit gets at least tested on non-i915, and you'll get tons of data
of which tests blow up, and even more which will just skip (due to
lack of features on vkms' side). We did have that situation a while
back with amdgpu and kbl-g (the intel+amd gpu's chip), but only by
accident (we do still run i915+amdgpu buffer sharing tests, since we
care about that). Took us a while to realize that igt preferred
running on amdgpu somehow. Running random non-i915 chips in our CI
farm isn't something that'll happen, but vkms should not be hard to
set up and get going.

Would that be useful if we get that done first?

> > 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
>
> That's fair, and I'll grant you that it's hard to push people to do
> that ramp-up before it's mandatory.
>
> > >
> > > 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).
>
> meh. There's both advantages and disadvantages to that. I do think
> there's a lot of scope for more concrete API design docs.
>
> > >
> > > 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.
> > >
>
> Yes, obviously we can't use a closed codebase as the test suite for
> DRM. I don't think anyone has/would/will suggest that. I was making an
> analogy, that if I came to you with an open test-suite, which was
> written by Arm and which didn't run on your hardware, and said it was
> about to become mandatory, you might have some complaints.

Intel invests _lots_ of money into being first with open source. If
they wouldn't, I'd probably be working somewhere else and fighting for
that vendor's test suite :-) So I think it's not really symmetrical
situation. Heck if my mail would be daniel.vetter at arm.com I'd be busy
ramping arm validation up on igt and killing any internal test suites.
Aside: I've already killed 2 such tests suites here at intel, I'm good
at this ...

> > > 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?
>
> I think you're twisting my words a bit far there. That's clearly not
> what I'm saying. I was quite explicit about that point:
>
>         > I'm all for [...] a central KMS test suite.
>
> If I could magically wish for anything I like, it would be for a
> test-suite which was built from the ground up to be *the* generic KMS
> test-suite.
>
> We don't live in a magical world, so igt makes a good starting point.
> I'm not disputing that. I'm just asking for a sanity check of your
> assertions that igt is "ready" today.

I guess it boils down to what a clear goal for "ready" is, which is
useful, concrete and realistic. "Get vkms into igt CI" would be a
useful thing, and something that imo makes tons of sense. It's been on
my plan anyway. "Prove that igt is ready for everyone" otoh is too
vague to be actionable.

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