On Fri, May 26, 2017 at 8:49 AM, Daniel Vetter <daniel@xxxxxxxx> wrote: > On Thu, May 25, 2017 at 1:09 AM, Patrik Jakobsson > <patrik.r.jakobsson@xxxxxxxxx> wrote: >> On Wed, May 24, 2017 at 9:52 PM, Daniel Vetter <daniel@xxxxxxxx> wrote: >>> On Wed, May 24, 2017 at 6:57 PM, Patrik Jakobsson >>> <patrik.r.jakobsson@xxxxxxxxx> wrote: >>>> Hi Dave and Daniel, >>>> >>>> We had a little mishap this morning when I had pushed a fix for gma500 >>>> into drm-misc-fixes without first getting someone to review it. The >>>> patch have been on the list for over a month and I don't feel like I >>>> have enough karma to force someone to review it. Since I'm the only >>>> one actively reviewing gma500 stuff I've effectively locked myself out >>>> from submitting patches for the driver. Sure, sometimes others help >>>> out and that is ofc appreciated. >>>> >>>> As you suggested Daniel, I could trade light-weight reviews with >>>> someone else. At first it sounds reasonable but when I think about it >>>> it's rather bad. Why should I sell my r-b tags cheap in order to get >>>> patches into the driver I'm maintaining? This model seems broken. >>>> Doing quick reviews because you trust the author is not a good idea. >>>> It defeats the purpose and soils the value of your r-b tag (learned >>>> from my own mistakes). >>>> >>>> In the case of gma500 I'm exaggerating the problem a bit but others >>>> will run into the same issue at some point. So my question is, can we >>>> scrap the requirements for an r-b tag in drivers with only one >>>> continuously active developer or at least make it more "soft"? Other >>>> ideas are welcome. >>> >>> I had a really long discussion about this topic in private with >>> another maintainer who expressed some unhappiness about the drm-misc >>> review model. Yes it looks a bit silly that you have to trade review, >>> but in the end if you think it's necessary to review other >>> submissions, then imo you also need to get your own patches reviewed >>> or at least sanity-checked. >> >> Thanks for replying Daniel. >> >> I agree with your reasoning but we're looking at the problem from two >> different perspectives. It's not silly to require review per se. My >> patches also deserves review but the problem is that I cannot count on >> getting it and I don't think I should be stealing time from others >> (you included) who's time is better spent elsewhere. > > > Did you ping other drm-misc maintainers on irc? Did you ping > Sean/Jani/me as ultim fallback reviewers? Yes sometimes getting that > review takes a bit of time, especially if the collaboration hasn't > been established yet. I didn't think about the r-b until you actually mentioned it. I figured someone would review it if they felt it was necessary. So the push into misc-fixes without r-b was never intentional. > >> Currently I get to choose between bad (patches without good review) or >> worse (no patches at all). Unfortunately I cannot pick bad because of >> the r-b tag requirement. Also, I review gma500 patches because it is >> expected of me and because I can. Not because I think the quality is >> worse than mine. I'd love to get reviews back but so far people just >> drop their patches and run. >> >>> >>> That's why drm-intel, drm-misc and anything else I'll ever maintain >>> will have a hard&solid rule to never push my own stuff (or anyone else >>> with commit rights) without review. No exceptions. >> >> That works when dealing with i915 and drm core (and other bigger >> drivers). You have a decent group of people who are technically >> qualified with personal and professional incentives to review your >> patches. It's easy to have high standards when they are not being >> challenged. >> >> On the gma500 team there's me and a bunch of frustrated users. What I >> would like to do is to continue shrinking the codebase and fix up the >> most obvious mistakes to make it more maintainable and let it live a >> couple of more years. I will likely have some time to do that again >> soon. Not because it's very important but because it's fun and makes a >> small group of people happy. Adding a bunch of process to this makes >> it less fun and reduces my output. > > Find another smaller driver in need of some cleanup (we can add more > to drm-misc), cross review. Yes it's a bit of work (see above), but at > least from the drm subsystem pov the end result will be worth it, > since more code sharing and more collaboration gives us a much > stronger community. I'm currently at a conference so I figured I'd ask around. So far, people are reluctant to get their hands dirty in a driver they've never looked at before and don't have hardware to test. From a community perspective, would you agree to require review by AMD for all Intel patches and vice versa (a slight exaggeration, I know)? I'd say that would cripple development of both drivers. There is as you say the a-b tag but I honestly doubt it's useful. > >>> In my opinion, as a maintainer of a part of the linux kernel your job >>> is to be the steward of the code and contributors/community around it, >>> not be the lord with special priviledges like being able to push >>> unreviewed patches or nacking contributions just because you're the >>> maintainer. And yes, part of the rules behind drm-misc is to make sure >>> that even single-maintainer drivers do collaborate, because with most >>> drivers sooner or later there will be other contributors. >> >> Right now I'm the lord of a mess but with less privileges than the >> contributors. They at least get their patches reviewed and included. >> Sounds more like I'm the fool ;) >> >> Sure, I can nack peoples patches but where's the fun in that. Nacking >> is never the easy choice since it requires justification and thus more >> work. I certainly don't see it as a privilege. >> >>> So at least from my side, yes there's an agenda behind this all, and >>> its intentional. It also seems to work reasonable well thus far, since >>> worst case drm-misc maintainers serve as reviewers of last resort. >> >> I understand there's an agenda and it makes sense from a "big drivers" >> pov. After some thinking, I would prefer the "pull from layers of >> trust" approach instead of "push through a very tightly tuned >> process". The layers of trust model also works well (as we all know). >> If maintainers feel they are getting overwhelmed with work we should >> look at expanding the subsystem tree instead of inventing new ways to >> handle things. Perhaps the solution to all of this is to just go back >> to sending patches for small drivers as pull-requests to you or Dave >> and let you decide if the sender is trustworthy enough to deserve a >> signed-off-by. > > What I want to achieve is that small drivers together feel&collaborate > like a "big driver". Yes you won't have experts on your specific hw, > but there's lots of good feedback wrt extracting helper functions into > the core, using the existing ones correctly and all the things like > that. See e.g. all the work that has happened around the simple > helpers from Noralf. I agree. That's very useful. > > Also, drm-misc is optional, you can go back to sending pull requests > to Dave (not to me, I don't handle those) if you think that's > easier/better for gma500. I'd like to do what puts the least amount of strain on maintainers. drm-misc scales well for developer but not for maintainers imo. We have a well working model in the kernel. I'd hate to see DRM turn into a special snowflake. > >> Either way, I don't want to turn this into a long discussion. If this >> is the way it needs to work then that is fine by me. Most of the time >> it works and gma500 is the driver with the smaller userbase and should >> not make life difficult for the bigger drivers. > > Very much appreciated, with feedback and discussions we can't improve > the process for everyone. Yes, and I'll find someone to review my upcoming patches and we'll see how it pans out in the gma500 case. Thanks Patrik > > Thanks, Daniel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
