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

[Brian Starkey]

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.

> > > > > 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 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?

-Brian

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