* Maintaining small drm drivers @ 2017-05-24 16:57 Patrik Jakobsson 2017-05-24 19:52 ` Daniel Vetter 0 siblings, 1 reply; 12+ messages in thread From: Patrik Jakobsson @ 2017-05-24 16:57 UTC (permalink / raw) To: dri-devel, Daniel Vetter, Dave Airlie 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. Cheers Patrik _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Maintaining small drm drivers 2017-05-24 16:57 Maintaining small drm drivers Patrik Jakobsson @ 2017-05-24 19:52 ` Daniel Vetter 2017-05-24 19:56 ` Daniel Vetter 2017-05-24 23:09 ` Patrik Jakobsson 0 siblings, 2 replies; 12+ messages in thread From: Daniel Vetter @ 2017-05-24 19:52 UTC (permalink / raw) To: Patrik Jakobsson; +Cc: dri-devel On Wed, May 24, 2017 at 6:57 PM, Patrik Jakobsson <patrik.r.jakobsson@gmail.com> 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. 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. 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. 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. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Maintaining small drm drivers 2017-05-24 19:52 ` Daniel Vetter @ 2017-05-24 19:56 ` Daniel Vetter 2017-05-24 23:09 ` Patrik Jakobsson 1 sibling, 0 replies; 12+ messages in thread From: Daniel Vetter @ 2017-05-24 19:56 UTC (permalink / raw) To: Patrik Jakobsson; +Cc: dri-devel Quick clarification. On Wed, May 24, 2017 at 9:52 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, May 24, 2017 at 6:57 PM, Patrik Jakobsson > <patrik.r.jakobsson@gmail.com> 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). That's why we only require an acked-by tag for small drivers, it's not a full review. So can't soil the value of your r-b tags. All the same reasons still apply. -Daniel >> 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. > > 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. > > 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. > > 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. > > Thanks, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Maintaining small drm drivers 2017-05-24 19:52 ` Daniel Vetter 2017-05-24 19:56 ` Daniel Vetter @ 2017-05-24 23:09 ` Patrik Jakobsson 2017-05-26 6:49 ` Daniel Vetter 1 sibling, 1 reply; 12+ messages in thread From: Patrik Jakobsson @ 2017-05-24 23:09 UTC (permalink / raw) To: Daniel Vetter; +Cc: dri-devel On Wed, May 24, 2017 at 9:52 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Wed, May 24, 2017 at 6:57 PM, Patrik Jakobsson > <patrik.r.jakobsson@gmail.com> 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. 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. > > 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. 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. Thanks Patrik > Thanks, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Maintaining small drm drivers 2017-05-24 23:09 ` Patrik Jakobsson @ 2017-05-26 6:49 ` Daniel Vetter 2017-05-26 11:42 ` Jani Nikula 2017-05-28 12:10 ` Patrik Jakobsson 0 siblings, 2 replies; 12+ messages in thread From: Daniel Vetter @ 2017-05-26 6:49 UTC (permalink / raw) To: Patrik Jakobsson; +Cc: dri-devel On Thu, May 25, 2017 at 1:09 AM, Patrik Jakobsson <patrik.r.jakobsson@gmail.com> wrote: > On Wed, May 24, 2017 at 9:52 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Wed, May 24, 2017 at 6:57 PM, Patrik Jakobsson >> <patrik.r.jakobsson@gmail.com> 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. > 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. >> 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. 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. > 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. Thanks, Daniel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Maintaining small drm drivers 2017-05-26 6:49 ` Daniel Vetter @ 2017-05-26 11:42 ` Jani Nikula 2017-05-26 11:44 ` Daniel Vetter 2017-05-28 12:10 ` Patrik Jakobsson 1 sibling, 1 reply; 12+ messages in thread From: Jani Nikula @ 2017-05-26 11:42 UTC (permalink / raw) To: Daniel Vetter, Patrik Jakobsson; +Cc: dri-devel On Fri, 26 May 2017, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, May 25, 2017 at 1:09 AM, Patrik Jakobsson > <patrik.r.jakobsson@gmail.com> wrote: >> 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. I think review is extremely important, even for trivial things. Case in point, ITYM s/with/without/. ;) BR, Jani. -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Maintaining small drm drivers 2017-05-26 11:42 ` Jani Nikula @ 2017-05-26 11:44 ` Daniel Vetter 2017-05-28 12:11 ` Patrik Jakobsson 0 siblings, 1 reply; 12+ messages in thread From: Daniel Vetter @ 2017-05-26 11:44 UTC (permalink / raw) To: Jani Nikula; +Cc: dri-devel On Fri, May 26, 2017 at 1:42 PM, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Fri, 26 May 2017, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Thu, May 25, 2017 at 1:09 AM, Patrik Jakobsson >> <patrik.r.jakobsson@gmail.com> wrote: >>> 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. > > I think review is extremely important, even for trivial things. Case in > point, ITYM s/with/without/. ;) Oops, indeed :-) -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Maintaining small drm drivers 2017-05-26 11:44 ` Daniel Vetter @ 2017-05-28 12:11 ` Patrik Jakobsson 0 siblings, 0 replies; 12+ messages in thread From: Patrik Jakobsson @ 2017-05-28 12:11 UTC (permalink / raw) To: Daniel Vetter; +Cc: dri-devel On Fri, May 26, 2017 at 1:44 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, May 26, 2017 at 1:42 PM, Jani Nikula > <jani.nikula@linux.intel.com> wrote: >> On Fri, 26 May 2017, Daniel Vetter <daniel@ffwll.ch> wrote: >>> On Thu, May 25, 2017 at 1:09 AM, Patrik Jakobsson >>> <patrik.r.jakobsson@gmail.com> wrote: >>>> 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. >> >> I think review is extremely important, even for trivial things. Case in >> point, ITYM s/with/without/. ;) > > Oops, indeed :-) Touché :) -Patrik > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Maintaining small drm drivers 2017-05-26 6:49 ` Daniel Vetter 2017-05-26 11:42 ` Jani Nikula @ 2017-05-28 12:10 ` Patrik Jakobsson 2017-05-29 6:53 ` Daniel Vetter 1 sibling, 1 reply; 12+ messages in thread From: Patrik Jakobsson @ 2017-05-28 12:10 UTC (permalink / raw) To: Daniel Vetter; +Cc: dri-devel On Fri, May 26, 2017 at 8:49 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, May 25, 2017 at 1:09 AM, Patrik Jakobsson > <patrik.r.jakobsson@gmail.com> wrote: >> On Wed, May 24, 2017 at 9:52 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >>> On Wed, May 24, 2017 at 6:57 PM, Patrik Jakobsson >>> <patrik.r.jakobsson@gmail.com> 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Maintaining small drm drivers 2017-05-28 12:10 ` Patrik Jakobsson @ 2017-05-29 6:53 ` Daniel Vetter 2017-05-29 7:35 ` Daniel Vetter 0 siblings, 1 reply; 12+ messages in thread From: Daniel Vetter @ 2017-05-29 6:53 UTC (permalink / raw) To: Patrik Jakobsson; +Cc: dri-devel On Sun, May 28, 2017 at 2:10 PM, Patrik Jakobsson <patrik.r.jakobsson@gmail.com> wrote: > On Fri, May 26, 2017 at 8:49 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >> On Thu, May 25, 2017 at 1:09 AM, Patrik Jakobsson >> <patrik.r.jakobsson@gmail.com> wrote: >>> On Wed, May 24, 2017 at 9:52 PM, Daniel Vetter <daniel@ffwll.ch> wrote: >>>> On Wed, May 24, 2017 at 6:57 PM, Patrik Jakobsson >>>> <patrik.r.jakobsson@gmail.com> 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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Maintaining small drm drivers 2017-05-29 6:53 ` Daniel Vetter @ 2017-05-29 7:35 ` Daniel Vetter 2017-06-01 15:19 ` Patrik Jakobsson 0 siblings, 1 reply; 12+ messages in thread From: Daniel Vetter @ 2017-05-29 7:35 UTC (permalink / raw) To: Patrik Jakobsson; +Cc: dri-devel On Mon, May 29, 2017 at 8:53 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >>> 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. To make it clearer what I meant to say: It's of course better if your reviewer has domain knowledge about your code, but if there's only 1 regular contributor a bit of review is imo still good. As soon as there's a few regular contributors then a review sub-group in drm-misc forms (e.g. like what's happened with bridge drivers, and we documented that in MAINTAINERS). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: Maintaining small drm drivers 2017-05-29 7:35 ` Daniel Vetter @ 2017-06-01 15:19 ` Patrik Jakobsson 0 siblings, 0 replies; 12+ messages in thread From: Patrik Jakobsson @ 2017-06-01 15:19 UTC (permalink / raw) To: Daniel Vetter; +Cc: dri-devel On Mon, May 29, 2017 at 9:35 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, May 29, 2017 at 8:53 AM, Daniel Vetter <daniel@ffwll.ch> wrote: >>>> 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. > > To make it clearer what I meant to say: It's of course better if your > reviewer has domain knowledge about your code, but if there's only 1 > regular contributor a bit of review is imo still good. As soon as > there's a few regular contributors then a review sub-group in drm-misc > forms (e.g. like what's happened with bridge drivers, and we > documented that in MAINTAINERS). Yes, a reviewer pool would be a good idea. Not just for drm-misc but for patches on dri-devel in general. Handling of "unsorted" patches with no direct maintainer can be improved. Basically what I get from this is that we sacrifice some convenience for small drivers on behalf of the bigger ones. I think that's fair during the circumstances. Not optimal, but fair. I'll be using drm-misc for smaller stuff and send larger series as p-r to Dave (with r-b if I can get them). I think that makes most sense from a gma500 pov. Cheers Patrik > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-06-01 15:19 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-05-24 16:57 Maintaining small drm drivers Patrik Jakobsson 2017-05-24 19:52 ` Daniel Vetter 2017-05-24 19:56 ` Daniel Vetter 2017-05-24 23:09 ` Patrik Jakobsson 2017-05-26 6:49 ` Daniel Vetter 2017-05-26 11:42 ` Jani Nikula 2017-05-26 11:44 ` Daniel Vetter 2017-05-28 12:11 ` Patrik Jakobsson 2017-05-28 12:10 ` Patrik Jakobsson 2017-05-29 6:53 ` Daniel Vetter 2017-05-29 7:35 ` Daniel Vetter 2017-06-01 15:19 ` Patrik Jakobsson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.