RFC: drm-misc for small drivers?

[Daniel Vetter]

On Thu, Jan 26, 2017 at 8:48 PM, Eric Anholt <eric at anholt.net> wrote:
>> - Should we require review or at least acks for patches committed by
>> the author? We have a bunch of drivers with effectively just 1 person
>> working on it, where getting real review is hard. But otoh a few of
>> those 1-person drivers will become popular, and then it's good to
>> start with establishing peer-review early on. I also think that
>> requiring peer-review is good to share best practices and knowledge
>> between different people in our community, not just to make sure the
>> code is correct. For all these reasons I'm leaning towards not making
>> an exception for drivers, and requiring the same amount of review for
>> them if they go in through drm-misc as for any other patch.
>
> This is the only part I'm concerned about.  Would anyone review vc4
> patches?  Voluntarily?  Actually thinking about the
> functionality/structure of the code, not just style?
>
> It sucks today that as part of the kernel process I send out patches
> "for review", knowing that I won't ever get review, and I just have to
> wait a while until I think people won't complain at me for sending a PR
> too quickly.  But if the change is to just start blocking my patches on
> review, I'm concerned I won't be able to get them in at all.
>
> I think there's a middle ground, where you graduate to waiting for
> review once you have multiple involved in that area of the code.  This
> is what we do in Mesa -- vc4 and freedreno push directly, but on the
> code we share (tgsi_to_nir, for example), we do review.
>
> (This is also the point at which I'll offer: Any other ARM drivers that
> want to do review exchange with me, let me know and I'll start paying
> attention to your stuff)

So part of the evil goal here is really to kickstart a tit-for-tat
review economy. That'd would mean we'd need at least 2-3 drivers to
volunteer for starters, and in many cases a full review is not going
to happen (or would just unduly delay things for no gain at all). What
I think would be great though is something much less formal along the
lines of "read your patch, looks all reasonable, seems to use core drm
code&concepts how it's supposed, ack". Maybe a few recommendations how
code can be simplified/clarified, but definitely no multi-round
bikeshedding tour. So not review to ensure code correctness, but just
as an information and best practice sharing tool. Also this should be
a good way to catch good opportunities for subsystem wide refactorings
when someone copypastes the same few lines for the umpteenth time
(that's how we e.g. got rid of all the dummy callback
implementations). And it shouldn't be much work, when I review a new
driver submission it's about 15 minutes to scroll through patches and
drop a few comments/suggestions on it.

So much less review than for drm core, and I think somehow getting
this off the ground would be really good for the community overall.
But if it doesn't work out, then I'm ok with dropping it, but I think
we really should try first. I think drm core had similar troubles with
review, and with drm-misc collaborations seems to improve already.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch