03:25 Venemo: zzyiwei: I don't think we need to "punish" anyone, but I do think it needs to be clarified if it's OK to merge things without a review
10:08 stefan11111: If anyone has the time to take a look, I opened https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/38959 , without it, tiled gbm bo's allocated with the dri backend cannot be unmapped
11:41 daniels: Venemo: yeah, I agree with you there - I think it's just a reminder/clarification of what our requirements are, and don't do it again next time :)
13:58 robclark: yeah, probably would be good to require review on core things touching more than one driver
14:01 robclark: (side note, there are some experiments w/ ai review for kernel patches.. which actually shows a bit of promise with the right prompt tuning.. which could maybe help with reviews in corners of mesa+drm with low bus factor)
15:22 Venemo: robclark: speaking of the kernel, thus far I've only seen the AI output for their backports, and that one gets a lot of things wrong. i wouldn't trust that for a review
15:27 robclark: I am a bit of an AI skeptic, but also reviews don't have to be perfect to point out things that humans should have a 2nd look at.. seems like, at least w/ some tuning false positives are down to ~30% which is low enough to be useful IMHO.. I think I'd trust AI review before I trust AI patches or backport decisions
15:27 robclark: https://github.com/masoncl/review-prompts fwiw
15:35 karolherbst: robclark: how much of that could be covered with static analysis tho? It doesn't have to be control flow stuff trying to figure out possible values, but even trivial things like what clippy provides are probably better things too look at, because we don't really have any of that besides clippy
15:36 karolherbst: but not sure how many useful tools there are in the C/C++ world
15:36 karolherbst: but even "30%" false positive rate is quite high
15:37 karolherbst: like that's almost evey second suggestion being wrong :P
15:37 karolherbst: (1 out of 3, but you get the point)
15:38 karolherbst: like those tools are useful if they offload mental work, but if I can't trust the output then it's adding too much overhead. Like if it's 30% "suggestions" but not really relevant, that's a lot better than nonsense suggestions.
15:38 karolherbst: mistrust has a cost, and 30% nonsense so I can't trust is is a looot worse than 30% "sure, but it doesn't amtter"
15:39 robclark: I've looked at a couple reviews.. one in drm/msm.. it caught things that you wouldn't see by only looking at the patch.. and that I don't think static analysis would catch. It turns out the patch was correct, but it would have been a case where it would have been useful for the patch author to explain why that is
15:40 robclark: like I said, I am an AI skeptic, but that convinced me that there is some utility in the are of patch review.. esp in corners of drivers where there aren't a lot of experts to go around and existing human reviews are in short supply
15:41 karolherbst: sure, all I'm saying is, that if such a tool is introduced and it has a too high rate of "nonsense" replies, it adds a cost to many developers
15:41 robclark: I'm not ready to say "hey lets just blindly accept AI patches" or anything like that.. but reviews, with some prompt tuning, is an area that shows some potential
15:42 karolherbst: like if you can't trust a person or a tool, it lowers your mental capacity for the actual work to be done
15:43 pac85: One danger with ai is that it can make up very good arguments for completely wrong things
15:43 karolherbst: like yeah, I'm sure it can spot problems, but just because it can, doesn't mean it's a net benefit overall
15:44 robclark: sure.. but what I've seen is approaching the "good enough" threshold.. maybe less of a problem in areas that get enough review already.. but there are areas that have a low bus factor and don't get enough review..
15:44 danylo: even the false positives could be useful, I expect in many cases them to be instance of not so clear code, that worth either explaining or making more clear.
15:44 karolherbst: but that wouldn't really solve the "not enough review" problem, because somebody still has to review it anyway
15:44 robclark: anyways, worth taking a look at that github project.. it is kinda interesting
15:44 robclark: danylo: exactly
15:45 karolherbst: but even if we agree to use a model, we'd need to find the one model that is actually ethically created, which none are atm :P
15:45 karolherbst: not gonna accept anything from OpenAI or any of the other dumpster fire companies
15:46 robclark: yeah, not really taking a stand on that (and there are questions about licenses vs training data, etc).. but those are less problematic for reviews than for patches generated by AI
15:47 robclark: I do see a need in some areas (not enough qualified reviewers in some areas) and so far there are some promising looking development in that area
15:48 karolherbst: oh sure, I mean we can all agree that potentially it's useful and time will tell how much
15:49 karolherbst: but I'd rather do enjoying things instead of having that discussion, especially around this time of the year 🙃
15:49 karolherbst: Maybe something to bring up like next month and we can have a discussion on gitlab there? Not really sure how that would go tho
15:50 karolherbst: especially once we start to discuss it, there will be 5 phoronix articles about what each of us said on the matter
15:51 karolherbst: but I think we should tackle the topic in a more generic sense. Like automated code reviews is helpful and we can toy around with different tools and neater integration than "look at the CI pipelines output"
15:52 robclark: at this point, I'm just following the topic on the kernel side.. it came up recently because it was a topic at maintainers summit. Like I said, I'm not ready to accept AI patches / bug reports / etc, and I think mesa's position on that is reasonable. But I think patch/MR review has some potential as a useful area for AI. In particular because it doesn't have to be perfect to be useful.
15:52 robclark: so the review aspect is, IMHO, an interesting area to follow
15:52 karolherbst: for sure
16:14 soreau: wouldn't AI add a certain amount of randomness depending on which LLM you query? and then it reviews, but does it have permission to (n)ack? :P
16:15 karolherbst: the point isn't to give it any privileges tho
16:17 karolherbst: but there are for sure a lot of hidden costs to actually doing it: there are enough people around who just won't bother contributing or getting involved if you have an AI chatbot doing code reviews, which is understandable
16:19 karolherbst: but I don't think that randomness is an issue as long as it's generally pointing out the right things, but I also kinda doubt it's a net benefit all things considered even if there would be an ethical way of doing it
16:20 karolherbst: and I don't mean just ethical concerns in terms of training
18:54 rpavlik: (I also daily drive an rx580 - GFX8....)
20:39 airlied: karolherbst: the feedback on kernel reviews with AI has been they consistently catch things human reviewers forget, so although they might have 30% false positives, for the things they know about they seem to find it almost 100% which human reviewers don't achieve
21:21 dcbaker: gfxstrand: I pulled "2bd282a968 pan/bi: Run nir_lower_all_phis_to_scalar() late" out of the staging/25.3 branch. It applies okay, but there's some testing regressions I'm not sure about
21:22 dcbaker: bbrezillon: I've got "bbc8ce2704 pan/cs: Don't leak builder resources" for the staging/25.3 branch, and that one patch doesn't apply cleanly. The diff doesn't look too complicated, but it seems like the kind of change that a bad backport could introduce some hard to reproduce bugs, would you mind making a PR for that?