Re: [PATCH] drm/doc: Make igts for cross-driver stuff mandatory
Hi Liviu, On Mon, Jan 28, 2019 at 3:03 PM Liviu Dudau wrote: > > On Tue, Jan 22, 2019 at 05:28:19PM +0100, Daniel Vetter wrote: > > On Tue, Jan 22, 2019 at 5:11 PM Sean Paul wrote: > > > On Tue, Jan 22, 2019 at 04:17:25PM +0100, Daniel Vetter wrote: > > > > On Tue, Jan 22, 2019 at 4:08 PM Brian Starkey > > > > 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 > > > > > > wrote: > > > > > > > > > > > /snip > > > > > > > > > > > > > > > > > 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, 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 >
Re: [PATCH] drm/doc: Make igts for cross-driver stuff mandatory
On Tue, Jan 22, 2019 at 05:28:19PM +0100, Daniel Vetter wrote: > On Tue, Jan 22, 2019 at 5:11 PM Sean Paul wrote: > > On Tue, Jan 22, 2019 at 04:17:25PM +0100, Daniel Vetter wrote: > > > On Tue, Jan 22, 2019 at 4:08 PM Brian Starkey > > > 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 > > > > > wrote: > > > > > > > > /snip > > > > > > > > > > > > > > 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, 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. > > > > My $0.02 since my interests are: > > - Make sure all of our (CrOS)
Re: [PATCH] drm/doc: Make igts for cross-driver stuff mandatory
On Tue, Jan 22, 2019 at 5:11 PM Sean Paul wrote: > On Tue, Jan 22, 2019 at 04:17:25PM +0100, Daniel Vetter wrote: > > On Tue, Jan 22, 2019 at 4:08 PM Brian Starkey 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 > > > > wrote: > > > > > /snip > > > > > > > > > > > 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, 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. > > My $0.02 since my interests are: > - Make sure all of our (CrOS) platforms are well-tested, and > - Ensure new features can be merged upstream without too much overhead > > My initial reaction was the same as Brian's. I've "brought up" igt on a > non-i915 > device and it was a bit of a horror show, but I managed to get the test I > wanted > running and lit the device on fire afterwards. Since then at least the > toolchain > has been
Re: [PATCH] drm/doc: Make igts for cross-driver stuff mandatory
On Tue, Jan 22, 2019 at 04:17:25PM +0100, Daniel Vetter wrote: > On Tue, Jan 22, 2019 at 4:08 PM Brian Starkey 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 > > > wrote: > > /snip > > > > > > > > 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, 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. My $0.02 since my interests are: - Make sure all of our (CrOS) platforms are well-tested, and - Ensure new features can be merged upstream without too much overhead My initial reaction was the same as Brian's. I've "brought up" igt on a non-i915 device and it was a bit of a horror show, but I managed to get the test I wanted running and lit the device on fire afterwards. Since then at least the toolchain has been fixed and igt builds, so that's nice. That said, the counterarguments presented have been pretty hypothetical. I get that crc is a pain for ARM (Arm? arm? aRm?), but it seems like a worthwhile investment to get it working so that they can leverage igt. Even if crc
Re: [PATCH] drm/doc: Make igts for cross-driver stuff mandatory
On Tue, Jan 22, 2019 at 4:08 PM Brian Starkey 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 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 > > XRGB (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, 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
Re: [PATCH] drm/doc: Make igts for cross-driver stuff mandatory
On Tue, Jan 22, 2019 at 03:03:59PM +0100, Daniel Vetter wrote: > On Tue, Jan 22, 2019 at 2:27 PM Brian Starkey 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 > XRGB (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, 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. > > > >
Re: [PATCH] drm/doc: Make igts for cross-driver stuff mandatory
On Tue, Jan 22, 2019 at 2:27 PM Brian Starkey 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 > > wrote: > > > > > > On Mon, Jan 21, 2019 at 12:54 PM Brian Starkey > > > 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 > > > > > 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 > > > > > > > Cc: Arkadiusz Hiler > > > > > > > Cc: Liviu Dudau > > > > > > > Cc: Sean Paul > > > > > > > Cc: Eric Anholt > > > > > > > Cc: Alex Deucher > > > > > > > Cc: Dave Airlie > > > > > > > Signed-off-by: Daniel Vetter > > > > > > > --- > > > > > > > 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 . > > > > > > > > > > > > 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
Re: [PATCH] drm/doc: Make igts for cross-driver stuff mandatory
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 wrote: > > > > On Mon, Jan 21, 2019 at 12:54 PM Brian Starkey > > 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 > > > > 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 > > > > > > Cc: Arkadiusz Hiler > > > > > > Cc: Liviu Dudau > > > > > > Cc: Sean Paul > > > > > > Cc: Eric Anholt > > > > > > Cc: Alex Deucher > > > > > > Cc: Dave Airlie > > > > > > Signed-off-by: Daniel Vetter > > > > > > --- > > > > > > 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 . > > > > > > > > > > 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
Re: [PATCH] drm/doc: Make igts for cross-driver stuff mandatory
On Mon, Jan 21, 2019 at 6:21 PM Daniel Vetter wrote: > > On Mon, Jan 21, 2019 at 12:54 PM Brian Starkey 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 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 > > > > > Cc: Arkadiusz Hiler > > > > > Cc: Liviu Dudau > > > > > Cc: Sean Paul > > > > > Cc: Eric Anholt > > > > > Cc: Alex Deucher > > > > > Cc: Dave Airlie > > > > > Signed-off-by: Daniel Vetter > > > > > --- > > > > > 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 . > > > > > > > > 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
Re: [PATCH] drm/doc: Make igts for cross-driver stuff mandatory
On Mon, Jan 21, 2019 at 12:54 PM Brian Starkey 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 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 > > > > Cc: Arkadiusz Hiler > > > > Cc: Liviu Dudau > > > > Cc: Sean Paul > > > > Cc: Eric Anholt > > > > Cc: Alex Deucher > > > > Cc: Dave Airlie > > > > Signed-off-by: Daniel Vetter > > > > --- > > > > 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 . > > > > > > 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
Re: [PATCH] drm/doc: Make igts for cross-driver stuff mandatory
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 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 > > > Cc: Arkadiusz Hiler > > > Cc: Liviu Dudau > > > Cc: Sean Paul > > > Cc: Eric Anholt > > > Cc: Alex Deucher > > > Cc: Dave Airlie > > > Signed-off-by: Daniel Vetter > > > --- > > > 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 . > > > > 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. 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. 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@lists.freedesktop.org > >
Re: [PATCH] drm/doc: Make igts for cross-driver stuff mandatory
On Thu, Jan 17, 2019 at 04:59:49PM +0100, Daniel Vetter wrote: > On Thu, Jan 17, 2019 at 3:54 PM Liviu Dudau wrote: > > > > On Thu, Jan 17, 2019 at 01:32:15PM +0100, Daniel Vetter wrote: > > > On Thu, Jan 17, 2019 at 1:26 PM Liviu Dudau wrote: > > > > > > > > On Thu, Jan 17, 2019 at 12:52:16PM +0100, Daniel Vetter wrote: > > > > > On Thu, Jan 17, 2019 at 12:38 PM Liviu Dudau > > > > > 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 > > > > > > > Cc: Arkadiusz Hiler > > > > > > > Cc: Liviu Dudau > > > > > > > Cc: Sean Paul > > > > > > > Cc: Eric Anholt > > > > > > > Cc: Alex Deucher > > > > > > > Cc: Dave Airlie > > > > > > > Signed-off-by: Daniel Vetter > > > > > > > --- > > > > > > > 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 . > > > > > > > > > > > > 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. > > > > > > > > Yeah, but if the "tests for all the existing stuff" doesn't exist, your > > > > _new_ feature tests are not going to mean much, doesn't it? > > > > > > Why? There's still good test coverage for the new stuff at least. And > > > eventually someone gets bored and fills in the gaps. This is how we've > > > created all the current igt testcases, so it works - mandatory igt for > > > new features is the rule for anything intel does since well over 5 > > > years. > > > > Because if you can't test basic stuff with igt how did you test your new > > feature? What I'm saying is: if developer X is having to use igt for > > a new feature then he is either: a) expecting that basic testing works for > > his driver (or he has bootstrapping isssues) or that b) if the feature > > doesn't get tested on driver A because that driver doesn't even have basic > > tests passing for it then the feature is acceptable. And it will be helpful > > to publish the list of igt tests that pass for each driver in KMS. > > I think this is an entirely different discussion, something along the > lines of "new drivers/new features implemented on existing drivers" > must come with igt test results to prove it's working. Not what I'm > proposing here at all. It would be a good discussion to have, but a > different one I think. New features implemented on existing drivers is what I'm expecting to apply most of the time when we add the
Re: [PATCH] drm/doc: Make igts for cross-driver stuff mandatory
On Thu, Jan 17, 2019 at 3:54 PM Liviu Dudau wrote: > > On Thu, Jan 17, 2019 at 01:32:15PM +0100, Daniel Vetter wrote: > > On Thu, Jan 17, 2019 at 1:26 PM Liviu Dudau wrote: > > > > > > On Thu, Jan 17, 2019 at 12:52:16PM +0100, Daniel Vetter wrote: > > > > On Thu, Jan 17, 2019 at 12:38 PM Liviu Dudau > > > > 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 > > > > > > Cc: Arkadiusz Hiler > > > > > > Cc: Liviu Dudau > > > > > > Cc: Sean Paul > > > > > > Cc: Eric Anholt > > > > > > Cc: Alex Deucher > > > > > > Cc: Dave Airlie > > > > > > Signed-off-by: Daniel Vetter > > > > > > --- > > > > > > 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 . > > > > > > > > > > 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. > > > > > > Yeah, but if the "tests for all the existing stuff" doesn't exist, your > > > _new_ feature tests are not going to mean much, doesn't it? > > > > Why? There's still good test coverage for the new stuff at least. And > > eventually someone gets bored and fills in the gaps. This is how we've > > created all the current igt testcases, so it works - mandatory igt for > > new features is the rule for anything intel does since well over 5 > > years. > > Because if you can't test basic stuff with igt how did you test your new > feature? What I'm saying is: if developer X is having to use igt for > a new feature then he is either: a) expecting that basic testing works for > his driver (or he has bootstrapping isssues) or that b) if the feature > doesn't get tested on driver A because that driver doesn't even have basic > tests passing for it then the feature is acceptable. And it will be helpful > to publish the list of igt tests that pass for each driver in KMS. I think this is an entirely different discussion, something along the lines of "new drivers/new features implemented on existing drivers" must come with igt test results to prove it's working. Not what I'm proposing here at all. It would be a good discussion to have, but a different one I think. The proposal here is only for new uapi: Thus far the requirement is "driver + userspace", not it would be "driver + userspace + igt, if generic feature". > Alternatively, you need to explain better what "driver-agnostic testcase" is > (my reading: a testcase that can be run on any driver). Or point to some igt > testcases that are considered driver-agnostic and known to run on different > (read:
Re: [PATCH] drm/doc: Make igts for cross-driver stuff mandatory
On Thu, Jan 17, 2019 at 01:32:15PM +0100, Daniel Vetter wrote: > On Thu, Jan 17, 2019 at 1:26 PM Liviu Dudau wrote: > > > > On Thu, Jan 17, 2019 at 12:52:16PM +0100, Daniel Vetter wrote: > > > On Thu, Jan 17, 2019 at 12:38 PM Liviu Dudau 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 > > > > > Cc: Arkadiusz Hiler > > > > > Cc: Liviu Dudau > > > > > Cc: Sean Paul > > > > > Cc: Eric Anholt > > > > > Cc: Alex Deucher > > > > > Cc: Dave Airlie > > > > > Signed-off-by: Daniel Vetter > > > > > --- > > > > > 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 . > > > > > > > > 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. > > > > Yeah, but if the "tests for all the existing stuff" doesn't exist, your > > _new_ feature tests are not going to mean much, doesn't it? > > Why? There's still good test coverage for the new stuff at least. And > eventually someone gets bored and fills in the gaps. This is how we've > created all the current igt testcases, so it works - mandatory igt for > new features is the rule for anything intel does since well over 5 > years. Because if you can't test basic stuff with igt how did you test your new feature? What I'm saying is: if developer X is having to use igt for a new feature then he is either: a) expecting that basic testing works for his driver (or he has bootstrapping isssues) or that b) if the feature doesn't get tested on driver A because that driver doesn't even have basic tests passing for it then the feature is acceptable. And it will be helpful to publish the list of igt tests that pass for each driver in KMS. Alternatively, you need to explain better what "driver-agnostic testcase" is (my reading: a testcase that can be run on any driver). Or point to some igt testcases that are considered driver-agnostic and known to run on different (read: non-x86) platforms. > > > > 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. > > > > I would like to see some proof of that in the form of > > > > > > > Should this be clarified further? > > > > an URL that points to a page similar to Mesa's GL supported features > > would be nice to add here, from my point of view. > > There isn't even a list of upstream drm features ... Yeah, I know that! :) > how exactly should that look like? What about a list of igt tests then? OR we could put in the igt TODO file the kind of tests we would like to have at some moment? Best regards, Liviu > Only thing I can tell you is
Re: [PATCH] drm/doc: Make igts for cross-driver stuff mandatory
On Thu, Jan 17, 2019 at 1:26 PM Liviu Dudau wrote: > > On Thu, Jan 17, 2019 at 12:52:16PM +0100, Daniel Vetter wrote: > > On Thu, Jan 17, 2019 at 12:38 PM Liviu Dudau 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 > > > > Cc: Arkadiusz Hiler > > > > Cc: Liviu Dudau > > > > Cc: Sean Paul > > > > Cc: Eric Anholt > > > > Cc: Alex Deucher > > > > Cc: Dave Airlie > > > > Signed-off-by: Daniel Vetter > > > > --- > > > > 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 . > > > > > > 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. > > Yeah, but if the "tests for all the existing stuff" doesn't exist, your > _new_ feature tests are not going to mean much, doesn't it? Why? There's still good test coverage for the new stuff at least. And eventually someone gets bored and fills in the gaps. This is how we've created all the current igt testcases, so it works - mandatory igt for new features is the rule for anything intel does since well over 5 years. > > 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. > > I would like to see some proof of that in the form of > > > > Should this be clarified further? > > an URL that points to a page similar to Mesa's GL supported features > would be nice to add here, from my point of view. There isn't even a list of upstream drm features ... how exactly should that look like? Only thing I can tell you is that we had huge amounts of todo for missing igt tests, and afaik we've worked them all down. We = intel here. It's not perfect, but good enough that we've essentially stopped doing any validation except by running igt. Of course you can always write more tests, same way you can always bikeshed a patch a few more times. Eventually there's dimishing returns. -Daniel > Best regards, > Liviu > > > -Daniel > > > > > > > > Best regards, > > > Liviu > > > > > > > > > > + > > > > Validating changes with IGT > > > > --- > > > > > > > > -- > > > > 2.20.1 > > > > > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > > -- > > | I would like to | > | fix the world, | > | but they're not | > | giving me the | > \ source code! / > --- > ¯\_(ツ)_/¯ -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/doc: Make igts for cross-driver stuff mandatory
On Thu, Jan 17, 2019 at 12:52:16PM +0100, Daniel Vetter wrote: > On Thu, Jan 17, 2019 at 12:38 PM Liviu Dudau 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 > > > Cc: Arkadiusz Hiler > > > Cc: Liviu Dudau > > > Cc: Sean Paul > > > Cc: Eric Anholt > > > Cc: Alex Deucher > > > Cc: Dave Airlie > > > Signed-off-by: Daniel Vetter > > > --- > > > 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 . > > > > 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. Yeah, but if the "tests for all the existing stuff" doesn't exist, your _new_ feature tests are not going to mean much, doesn't it? > 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. I would like to see some proof of that in the form of > Should this be clarified further? an URL that points to a page similar to Mesa's GL supported features would be nice to add here, from my point of view. Best regards, Liviu > -Daniel > > > > > Best regards, > > Liviu > > > > > > > + > > > Validating changes with IGT > > > --- > > > > > > -- > > > 2.20.1 > > > > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --- ¯\_(ツ)_/¯ ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/doc: Make igts for cross-driver stuff mandatory
On Thu, Jan 17, 2019 at 12:38 PM Liviu Dudau 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 > > Cc: Arkadiusz Hiler > > Cc: Liviu Dudau > > Cc: Sean Paul > > Cc: Eric Anholt > > Cc: Alex Deucher > > Cc: Dave Airlie > > Signed-off-by: Daniel Vetter > > --- > > 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 . > > 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 > > 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@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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/doc: Make igts for cross-driver stuff mandatory
On Wed, Jan 16, 2019 at 11:41 PM Eric Anholt wrote: > > Daniel Vetter writes: > > > 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 > > Cc: Arkadiusz Hiler > > Cc: Liviu Dudau > > Cc: Sean Paul > > Cc: Eric Anholt > > Cc: Alex Deucher > > Cc: Dave Airlie > > Signed-off-by: Daniel Vetter > > igt is a bit awkward to work in (the mailing list is very noisy with the > Intel CI being email-based instead of gitlab-based and most of the > traffic being Intel), but it's the right place to be putting shared > tests and hopefully that pain point goes away eventually using gitlab > MRs. I have a filter to mark all CI mails as read, to avoid the spam a bit. And then check it only when I merge a series. But yeah, it's pain. > I think there are going to be some interesting questions on how to deal > with things like KMS properties that aren't amenable to > chamelium/writeback-based testing. However, we should default to > requiring tests and only skip that when we agree collectively that > something isn't testable currently. Should I add a sentence clarifying this? We don't want to block features on a new unproven/unwritten testing approach (e.g. "write an entire vkms to test-drive that feature"), that doesn't make sense. -Daniel > > Reviewed-by: Eric Anholt -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/doc: Make igts for cross-driver stuff mandatory
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 > Cc: Arkadiusz Hiler > Cc: Liviu Dudau > Cc: Sean Paul > Cc: Eric Anholt > Cc: Alex Deucher > Cc: Dave Airlie > Signed-off-by: Daniel Vetter > --- > 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 . 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. 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/doc: Make igts for cross-driver stuff mandatory
On Thu, Jan 17, 2019 at 01:01:00PM +0200, Arkadiusz Hiler 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 > > Cc: Arkadiusz Hiler > > Cc: Liviu Dudau > > Cc: Sean Paul > > Cc: Eric Anholt > > Cc: Alex Deucher > > Cc: Dave Airlie > > Signed-off-by: Daniel Vetter > > Thanks for the trust! I hope we will serve the subsystem well. > > I am happy about the influx of people from outside of i915 that we have > got in 2018 and I hope it will only get better this year. > > There are still some cleanup tasks left when it comes driver-specific > test compartmentalization, but we are getting there :-) > > Acked-by: Arkadiusz Hiler +1 Acked-by: Petri Latvala ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/doc: Make igts for cross-driver stuff mandatory
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 > Cc: Arkadiusz Hiler > Cc: Liviu Dudau > Cc: Sean Paul > Cc: Eric Anholt > Cc: Alex Deucher > Cc: Dave Airlie > Signed-off-by: Daniel Vetter Thanks for the trust! I hope we will serve the subsystem well. I am happy about the influx of people from outside of i915 that we have got in 2018 and I hope it will only get better this year. There are still some cleanup tasks left when it comes driver-specific test compartmentalization, but we are getting there :-) Acked-by: Arkadiusz Hiler -- Cheers, Arek ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/doc: Make igts for cross-driver stuff mandatory
Daniel Vetter writes: > 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 > Cc: Arkadiusz Hiler > Cc: Liviu Dudau > Cc: Sean Paul > Cc: Eric Anholt > Cc: Alex Deucher > Cc: Dave Airlie > Signed-off-by: Daniel Vetter igt is a bit awkward to work in (the mailing list is very noisy with the Intel CI being email-based instead of gitlab-based and most of the traffic being Intel), but it's the right place to be putting shared tests and hopefully that pain point goes away eventually using gitlab MRs. I think there are going to be some interesting questions on how to deal with things like KMS properties that aren't amenable to chamelium/writeback-based testing. However, we should default to requiring tests and only skip that when we agree collectively that something isn't testable currently. Reviewed-by: Eric Anholt signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
