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

[Daniel Vetter]

On Tue, Jan 22, 2019 at 4:08 PM Brian Starkey <Brian.Starkey at arm.com> wrote:
>
> On Tue, Jan 22, 2019 at 03:03:59PM +0100, Daniel Vetter wrote:
> > On Tue, Jan 22, 2019 at 2:27 PM Brian Starkey <Brian.Starkey at arm.com> wrote:
>
> [snip]
>
> > >
> > > 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.
> >
>
> It's an option. I wasn't super comfortable with the kernel doing so
> much "stuff" under-the-hood, in terms of allocating another fb,
> having some bandwidth available, having a datapath available...
>
> Then I wasn't sure how to make debugfs hook in to the atomic commit
> pathway, triggering a new commit with the framebuffer when userspace
> turns CRC on and trying not to tread on the toes of the thing calling
> the actual commit ioctl(). I think it would be pretty invasive.
>
> Even after that, the semantics may be different enough (e.g. writeback
> is oneshot) that tests would need to be changed anyway.

You'd need to rearm it somehow ofc, which probably needs some hacks to
bypass atomic. Wrt just doing an atomic commit: i915 already does that
to apply some workarounds for our crc generation. igt seems to cope
with an atomic commit happening in the background when setting up the
igt, so that's not a problem.

> > > > > > 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?
>
> Sure, that would be useful. I'm not even saying I want it in CI
> though, just for some people who aren't @intel.com to try it on their
> HW and say "works for me".

There's a pretty huge difference between "I want acks from other
people" and "you need to prove this works everywhere" ... The prove
thing pretty much requires CI, or it's imo not really anywhere near
proving.

> > > > 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 ...
> >
>
> As you say, we clearly aren't in a symmetrical situation. I can only
> tell you how it looks from my side.
>
> > > > > 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.
>
> Actionable is good, this would be my suggestion for actionable:
>
> > > 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.
>
> The fuzzy bit is "reasonable subset": maybe amdgpu, vc4, msm, mali-dp,
> vkms? Perhaps one suite with CRC and one without?

With this we're back to "Daniel, pls make sure igt works on all
drivers". In theory that sounds reasonable, in practice it means no
testsuite. vkms I can make happen I think, no problem. Also
vkms-without-some-features (like no crc - would need to be added, or
no vblank - already there), if people think that's useful. More, you
need to hire me :-)

If you just mean that I should make sure there's plenty of acks from
all these people, that's already a given.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch