Re: Maintainer-review fluff (was: Re: [PATCH 01/12] drm/i915: plumb VM into object operations)

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Tue, 6 Aug 2013 00:19:33 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:
> The only thing I read here, both in the paragraph above and in the
> rant is that we suck. I agree. My opinion is that this is because
> we've started late, had too few resources and didn't seriously
> estimate how much work is actually involved to enable something for
> real.

No, it's more than that, we suck in very specific ways:
  1) large (and sometimes even small) features take waaay too long to
     land upstream, taking valuable developer time away from other
     things like bug fixing, regressions, etc
  2) hw support lands late, which makes it harder to get debug traction
     with tough bugs (e.g. RC6)

> 
> The only reason I could distill from the above two paragraphs among
> the ranting for way we are so much late is "So while our code might
> look pretty, it's late and buggy". That's imo a farily shallow stab at
> preceived bikesheds, but not a useful angle to improve process.

No, I suggested improvements to our process earlier, and it sounded
like you mostly agreed, though seemed to deny point that we spin for
too long on things (point #1 above).

> Now I agree that I uphold a fairly high quality standard for
> upstreaming, but not an unreasonable one:
> - drm/i915 transformed from the undisputed shittiest driver in the
> kernel to one that mostly just works, while picking up development
> pace. So I don't think I'm fully wrong on insisting on this level of
> quality.
> - we do ship the driver essentially continously, which means we can
> implement features only by small refactoring steps. That clearly
> involves more work than just stitching something together for a
> product.

<sarcasm>
You're way off base here.  We should ship a shitty driver and just land
everything without review or testing.  That way we can go really fast.
Your quality standards are too high (in that they exist at all).
</sarcasm>

More seriously, quality should be measured by the end result in terms
of bugs and how users actually use our stuff.  I'm not sure if that's
what you mean by a "high quality standard".  Sometimes it seems you
care more about refactoring things ad-infinitum than tested code.

> Also note that Chris&me still bear the brute of fixing the random
> fallout all over (it's getting better). So if any proposed changes
> involves me blowing through even more time to track down issues I'm
> strongly not in favour. Same holds for Chris often-heard comment that
> a patch needs an improved commit message or a comment somewhere. Yes
> it's annoying that you need to resend it (this often bugs me myself)
> just to paint the bikeshed a bit different. But imo Chris is pretty
> much throughout spot-on with his requests and a high-quality git
> history has, in my experience at least, been extremely valueable to
> track down the really ugly issues and legalese around all the
> established precendence.

Again, no one is suggesting that we have shitty changelogs or that we
add comments.  Not sure why you brought that up.

> - Rebase hell due to ongoing other work. Thus far I've only tried to
> help here by rechecking/delaying refactoring patches while big
> features are pending. I think we need to try new approaches here and
> imo better planing should help. E.g. the initial modeset refactor was
> way too big and a monolithic junk that I've just wrestled in by
> exorting r-b tags from you. In contrast the pipe config rework was
> about equally big, but at any given time only about 30-50 patches
> where outstanding (in extreme cases), and mutliple people contributed
> different parts of the overall beast. Of course that means that
> occasional, for really big stuff, we need to plan to write a first
> proof of concept as a landmark where we need to go to, which pretty
> much will be thrown away completely.

This is the real issue.  We don't have enough people to burn on
single features for 6 months each so they can be rewritten 3 times until
they look how you would have done it.  If we keep doing that, you
may as well write all of it, and we'll be stuck in my <rant> from
the previous message.  That's why I suggested the two reviewed-by tags
ought to be sufficient as a merge criteria.  Sure, there may be room
for refactoring, but if things are understandable by other developers
and well tested, why block them?

> One meta-comment on top of the actual discussion: I really appreciate
> critique and I've grown a good maintainer-skin to also deal with
> really harsh critique. But I prefer less ranting and more concrete
> examples where I've botched the job (there are plentiful to pick from
> imo) and concrete suggestion for how to improve our overall process.

I've suggested some already, but they've fallen on deaf ears afaict.  I
don't know what more I can do to convince you that you acting as a
review/refactor bottleneck actively undermines the goals I think we
share.

But I'm done with this thread.  Maybe others want to comment on things
they might think improve the situation.

-- 
Jesse Barnes, Intel Open Source Technology Center