Maintaining small drm drivers

[Daniel Vetter]

On Sun, May 28, 2017 at 2:10 PM, Patrik Jakobsson
<patrik.r.jakobsson at gmail.com> wrote:
> On Fri, May 26, 2017 at 8:49 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
>> On Thu, May 25, 2017 at 1:09 AM, Patrik Jakobsson
>> <patrik.r.jakobsson at gmail.com> wrote:
>>> On Wed, May 24, 2017 at 9:52 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
>>>> On Wed, May 24, 2017 at 6:57 PM, Patrik Jakobsson
>>>> <patrik.r.jakobsson at 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