Messages in this thread | | | Date | Wed, 30 Aug 2023 12:14:04 -0300 | Subject | Re: [PATCH v11] drm: Add initial ci/ subdirectory | From | Helen Koike <> |
| |
On 30/08/2023 11:57, Maxime Ripard wrote: > On Wed, Aug 30, 2023 at 10:24:49AM -0300, Helen Koike wrote: >> Hi all, >> >> Thanks for you comments. >> >> On 30/08/2023 08:37, Maxime Ripard wrote: >>> On Wed, Aug 30, 2023 at 01:58:31PM +0300, Jani Nikula wrote: >>>> On Wed, 30 Aug 2023, Maxime Ripard <mripard@kernel.org> wrote: >>>>> On Tue, Aug 22, 2023 at 04:26:06PM +0200, Daniel Vetter wrote: >>>>>> On Fri, Aug 11, 2023 at 02:19:53PM -0300, Helen Koike wrote: >>>>>>> From: Tomeu Vizoso <tomeu.vizoso@collabora.com> >>>>>>> >>>>>>> Developers can easily execute several tests on different devices >>>>>>> by just pushing their branch to their fork in a repository hosted >>>>>>> on gitlab.freedesktop.org which has an infrastructure to run jobs >>>>>>> in several runners and farms with different devices. >>>>>>> >>>>>>> There are also other automated tools that uprev dependencies, >>>>>>> monitor the infra, and so on that are already used by the Mesa >>>>>>> project, and we can reuse them too. >>>>>>> >>>>>>> Also, store expectations about what the DRM drivers are supposed >>>>>>> to pass in the IGT test suite. By storing the test expectations >>>>>>> along with the code, we can make sure both stay in sync with each >>>>>>> other so we can know when a code change breaks those expectations. >>>>>>> >>>>>>> Also, include a configuration file that points to the out-of-tree >>>>>>> CI scripts. >>>>>>> >>>>>>> This will allow all contributors to drm to reuse the infrastructure >>>>>>> already in gitlab.freedesktop.org to test the driver on several >>>>>>> generations of the hardware. >>>>>>> >>>>>>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com> >>>>>>> Signed-off-by: Helen Koike <helen.koike@collabora.com> >>>>>>> Acked-by: Daniel Stone <daniels@collabora.com> >>>>>>> Acked-by: Rob Clark <robdclark@gmail.com> >>>>>>> Tested-by: Rob Clark <robdclark@gmail.com> >>>>>> >>>>>> Ok I pushed this into a topic/drm-ci branch in drm.git and asked sfr to >>>>>> include that branch in linux-next. >>>>>> >>>>>> But also I'd like to see a lot more acks here, we should be able to at >>>>>> least pile up a bunch of (driver) maintainers from drm-misc in support of >>>>>> this. Also maybe media, at least I've heard noises that they're maybe >>>>>> interested too? Plus anyone else, the more the better. >>>>> >>>>> I'm not really convinced by that approach at all, and most of the issues >>>>> I see are shown by the follow-up series here: >>>> >>>> I'm not fully convinced either, more like "let's see". In that narrow >>>> sense, ack. I don't see harm in trying, if you're also open to backing >>>> off in case it does not pan out. >>>> >>>>> https://lore.kernel.org/dri-devel/20230825122435.316272-1-vignesh.raman@collabora.com/ >>>>> >>>>> * We hardcode a CI farm setup into the kernel >> >> >> These could be out of tree. >> >> There is a version outside the kernel tree where you just point the CI >> configuration to a url: >> https://gitlab.freedesktop.org/gfx-ci/drm-ci/-/merge_requests/1 >> >> We were discussing it here https://www.linuxtv.org/cgi-bin/mailman/private/linuxtv-ci/2023-August/000027.html > > It looks like it's private > >> (I guess Sima's reply didn't got into the mailing list) but the argument of >> not having out of tree repo is due to historical bad experience of having to >> sync the kernel with the code and it can become messy. > > My point is that even though the test strategy might be considered a > "property" of the kernel, how you execute it is definitely not and you > will have as many setups as you have CI farms. You can't put that into > the kernel, just like we don't put the kernel command line in it for > example. > >>>>> >>>>> * We cannot trust that the code being run is actually the one being >>>>> pushed into gitlab >> >> >> We can improve this if this is a requirement. >> For DTS configuration we can work with overlays (which is the current >> modification on that patchset). For other changes that are not suitable to >> upstream (and should be rare) we can see if we work with the >> `-external-fixes` approach or another approach, we can check it case by case >> to understand why it is not suitable for upstream. > > The existence of that branch in itself is an issue to me. Again, it's a > matter of trust. How can I trust a branch I barely know about, of which > the development is not clear and isn't reviewed by any of the > maintainers of the code that might affect the test outcomes. > > Or put another way, if I run the tests on my machine, it won't work. Why > should it work on the CI farm? The branch itself is broken. It might not > be due to any of the work I did, but it's broken still. > >>>>> >>>>> * IMO, and I know we disagree here, any IGT test we enable for a given >>>>> platform should work, period. Allowing failures and flaky tests just >>>>> sweeps whatever issue is there under the rug. If the test is at >>>>> fault, we should fix the test, if the driver / kernel is at fault, >>>>> then I certainly want to know about it. >> >> I believe we need a baseline and understand the current status of tests. If >> you check the xfails folder in the patch you can see that I had to add a few >> tests on *-skips.txt since those tests crashes the system and other on >> *-fails.txt that are consistently not passing. > > I agree that we need a baseline, but that baseline should be defined by > the tests own merits, not their outcome on a particular platform. > > In other words, I want all drivers to follow that baseline, and if they > don't it's a bug we should fix, and we should be vocal about it. We > shouldn't ignore the test because it's broken. > > Going back to the example I used previously, kms_hdmi_inject@inject-4k > shouldn't fail on mt8173, ever. That's a bug. Ignoring it and reporting > that "all tests are good" isn't ok. There's something wrong with that > driver and we should fix it. > > Or at the very least, explain in much details what is the breakage, how > we noticed it, why we can't fix it, and how to reproduce it. > > Because in its current state, there's no chance we'll ever go over that > test list and remove some of them. Or even know if, if we ever fix a bug > somewhere, we should remove a flaky or failing test. > >> Since the "any IGT test we enable for a given platform should work" is not a >> reality atm, > > Thanks for the reality check, but it's very much doable: we're in > control of the test suite. > >> we need to have a clear view about which tests are not corresponding >> to it, so we can start fixing. First we need to be aware of the issues >> so we can start fixing them, otherwise we will stay in the "no tests >> no failures" ground :) > > I think we have somewhat contradicting goals. You want to make > regression testing, so whatever test used to work in the past should > keep working. That's fine, but it's different from "expectations about > what the DRM drivers are supposed to pass in the IGT test suite" which > is about validation, ie "all KMS drivers must behave this way".
I see. Indeed, for me it is more about regression testing.
We could have a configuration where developers could choose to run regression tests or overall validation (but I understand this is not the point, but just saying we could have both somehow).
We could have some policy: if you want to enable a certain device in the CI, you need to make sure it passes all tests first to force people to go fix the issues, but maybe it would be a big barrier.
I'm afraid that, if a test fail (and it is a clear bug), people would just say "work for most of the cases, this is not a priority to fix" and just start ignoring the CI, this is why I think regression tests is a good way to start with.
Anyway, just brain storming :) I really hope we can find a nice useful solution for the community.
Regards, Helen
> > I guess for regression you very much would like that all-green > dashboard, and it's understandable. For validation, we don't care and we > should be as vocal as possible to report broken drivers. > > Eventually, we should have regression testing over the validation test > suite. > > It's not about reality. We should be clear what we expect from those > test suites, and not claim that it's something it's not. > >>>> At least for display, where this also depends on peripheral hardware, >>>> it's not an easy problem, really. >>> >>> Aside from the Chamelium tests, which tests actually rely on peripheral >>> hardware? On EDID and hotplug, sure, but that can easily be set up from >>> the userspace, or something like >>> >>> https://www.lindy-international.com/HDMI-2-0-EDID-Emulator.htm?websale8=ld0101.ld021102&pi=32115 >>> >>>> How reliable do you need it to be? How many nines? Who is going to >>>> debug the issues that need hundreds or thousands of runs to reproduce? >>>> If a commit makes some test less reliable, how long is it going to >>>> take to even see that or pinpoint that? >>> >>> I mean, that's also true for failures or success then. How many times do >>> you need a test to run properly to qualify it as a meaningful test? How >>> do you know that it's not a flaky test? >>> >>> Ultimately, it's about trust. If, for a given test that just failed, I >>> can't be certain that it's because of the branch I just submitted, I >>> will just ignore the tests results after a while. >>> >>> This is already what plagues kernelci, and we should do better. >> >> This is something that is really nice on Mesa3D, a patch only gets merged if >> tests passes, which forces people to not ignore it, which forces the code to >> be fixed and the CI to be constantly maintained. >> >> Of course there are bad days there, but there is real value. Nice thread to >> check: https://gitlab.freedesktop.org/mesa/mesa/-/issues/8635 (thanks Alyssa >> for the feedback). > > I'm sure it works great for Mesa, but I'm also sure it doesn't ignore > CTS reports that a particular device isn't a valid OpenGL or Vulkan > implementation anymore. > >>> And I'm sorry, but if some part of the kernel or driver just isn't >>> reliable, then we shouldn't claim it is (except for all the times it >>> isn't). If no-one has the time to look into it, fine, but flagging it >>> under a flaky test doesn't help anyone. >> >> At least we would know what is there that isn't reliable. > > We would too if the test was reported as failed. But our preferred > approach to do so diverge. > > Maxime
| |