Maintaining small drm drivers

[Patrik Jakobsson]

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.

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