On Sun, May 28, 2017 at 2:10 PM, Patrik Jakobsson <patrik.r.jakobsson@xxxxxxxxx> wrote: > 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. Small driver = only 1 regular contributor. AMD and intel are anything but small. And yes if I'd maintain a small driver I'd welcome to have a regular exchange with other maintainers to make sure my driver is up to date with drm best practices. Again gma500 is a bit special since it's not moving anymore. >>>> 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. I don't understand your concern here - the traditional kernel model still exists in drm, if you opt to go with that. And being a special snowflake in a large community like the kernel is sometimes necessary, since if everyone always does it the same, then the overall community doesn't learn anymore. Other maintainers/subsystems do things different in other areas. >>> 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. Yeah, let's see and adjust as we go ... Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
