Maintaining small drm drivers

Sun May 28 12:10:14 UTC 2017

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.

>
>>> 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