On Fri, Jan 27, 2017 at 11:50:42AM -0500, Alex Deucher wrote: > On Fri, Jan 27, 2017 at 1:52 AM, Daniel Vetter <daniel.vetter@xxxxxxxx> wrote: > > On Thu, Jan 26, 2017 at 8:48 PM, Eric Anholt <eric@xxxxxxxxxx> wrote: > >>> - Should we require review or at least acks for patches committed by > >>> the author? We have a bunch of drivers with effectively just 1 person > >>> working on it, where getting real review is hard. But otoh a few of > >>> those 1-person drivers will become popular, and then it's good to > >>> start with establishing peer-review early on. I also think that > >>> requiring peer-review is good to share best practices and knowledge > >>> between different people in our community, not just to make sure the > >>> code is correct. For all these reasons I'm leaning towards not making > >>> an exception for drivers, and requiring the same amount of review for > >>> them if they go in through drm-misc as for any other patch. > >> > >> This is the only part I'm concerned about. Would anyone review vc4 > >> patches? Voluntarily? Actually thinking about the > >> functionality/structure of the code, not just style? > >> > >> It sucks today that as part of the kernel process I send out patches > >> "for review", knowing that I won't ever get review, and I just have to > >> wait a while until I think people won't complain at me for sending a PR > >> too quickly. But if the change is to just start blocking my patches on > >> review, I'm concerned I won't be able to get them in at all. > >> > >> I think there's a middle ground, where you graduate to waiting for > >> review once you have multiple involved in that area of the code. This > >> is what we do in Mesa -- vc4 and freedreno push directly, but on the > >> code we share (tgsi_to_nir, for example), we do review. > >> > >> (This is also the point at which I'll offer: Any other ARM drivers that > >> want to do review exchange with me, let me know and I'll start paying > >> attention to your stuff) > > > > So part of the evil goal here is really to kickstart a tit-for-tat > > review economy. That'd would mean we'd need at least 2-3 drivers to > > volunteer for starters, and in many cases a full review is not going > > to happen (or would just unduly delay things for no gain at all). What > > I think would be great though is something much less formal along the > > lines of "read your patch, looks all reasonable, seems to use core drm > > code&concepts how it's supposed, ack". Maybe a few recommendations how > > code can be simplified/clarified, but definitely no multi-round > > bikeshedding tour. So not review to ensure code correctness, but just > > as an information and best practice sharing tool. Also this should be > > a good way to catch good opportunities for subsystem wide refactorings > > when someone copypastes the same few lines for the umpteenth time > > (that's how we e.g. got rid of all the dummy callback > > implementations). And it shouldn't be much work, when I review a new > > driver submission it's about 15 minutes to scroll through patches and > > drop a few comments/suggestions on it. > > > > So much less review than for drm core, and I think somehow getting > > this off the ground would be really good for the community overall. > > But if it doesn't work out, then I'm ok with dropping it, but I think > > we really should try first. I think drm core had similar troubles with > > review, and with drm-misc collaborations seems to improve already. > > I like the idea, but I'm concerned it would just be work for the sake > of work. I'm not sure how much value it provides. I'm worried it'll > just turn into acking for the sake of acking. For core driver changes > that are independent of anything shared, I'm not really sure what the > ack provides other than a way to slap an ack label on your patch > unless the reviewer really digs into the driver. I guess an extra set > of eyes never hurts. Yeah, it would be a bit of a checkbox step, but then there's not that many drivers which really only have 1 contributor. I think as soon as you have 2-3, group maintainership with real peer review within the group should be the expectation, whether that's within drm-misc or in a separate driver repo doesn't matter. And for single-contributor drivers I still think it's useful, both to share best practices (display hw all deals with the same problems in the end, so even when the register don't work the same, the overall logic often does), and to make sure that when there's more contributors there's no process change in switching to a real group maintainership model. And I'm happy to help out kickstarting a small-driver review market by reviewing patches in exchange for review on my cleanup/core/doc stuff :-) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
