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@xxxxxxx> 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@xxxxxxx 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
