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

[Daniel Vetter]

Like I've said in my previous mail I expect such discussions to be
hard and I also think stopping now and giving up is the wrong
approach. So another round.

On Tue, Aug 6, 2013 at 1:34 AM, Jesse Barnes <jbarnes at virtuousgeek.org> wrote:
> On Tue, 6 Aug 2013 00:19:33 +0200
> Daniel Vetter <daniel at 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).

I'll cover your process suggestions below, since some of your
clarifications below shine a different light onto them. But overall I
agree, even that we seem to spin sometimes awfully long.

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

I often throw in a refactoring suggestion when people work on a
feature, that's right. Often it is also a crappy idea, but imo for
long-term maintainance a neat&tidy codebase is really important. So
I'll just throw them out and see what sticks with people.

I realize that pretty much all of the quality standard discussion here
is really fluffy, but like I explained I get to bear a large part of
the "keep it going" workload. And as long as that's the case I frankly
think my standards carry more weight. Furthermore in the cases where
other people from our team chip in with bugfixing that's mostly in
cases where a self-check or testcase clearly puts the blame on them.
So if that is the only way to volunteer people I'll keep asking for
those things (and delay patches indefinitely like e.g. your fastboot
stuff).

And like I've said I'm open to discuss those requirements, but I
freely admit that I have a rather solid ground resolve on this topic.

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

I added it since I've just read through some of the patches on the
android internal branch yesterday and a lot of those patches fall
through on the "good enough commit message" criterion (mostly by
failing to explain why the patch is needed). I've figured that's
relevant since on internal irc you've said even pushing fixes to
upstream is a PITA since they require 2-3 rounds to get in.

To keep things concrete one such example is Kamal's recent rc6 fix
where I've asked for a different approach and he sounded rather pissed
that I don't just take his patch as-is. But after I've explained my
reasoning he seemed to agree, at least he sent out a revised version.
And the changes have all been what I guess you'd call bikesheds, since
it was just shuffling the code logic around a bit and pimping the
commit message. I claim that this is worth it and I think your stance
is that we shouldn't delay patches like this. Or is this a bad example
for a patch which you think was unduly delayed? Please bring another
one up in this case, I really think process discussions are easier
with concrete examples.

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

I'd like to see an example here for something that I blocked, really.
One I could think up is the ips feature from Paulo where I've asked to
convert it over to the pipe config tracking. But I asked for that
specifically so that one of our giant long-term feature goals (atomic
modeset) doesn't move further away, so I think for our long-term aims
this request was justified.

Otherwise I have a hard time coming up with features that had r-b tags
from one of the domain expert you've listed (i.e. where understood
well) and I blocked them. It's true that I often spot something small
when applying a patch, but I also often fix it up while applying
(mostly adding notes to the commit message) or asking for a quick
follow-up fixup patch.

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

Your above clarification that the 2 r-b tags (one from the domain
expert) should overrule my concern imo makes your original proposal a
bit different - my impression was that you've asked for 2 r-b tags,
period. Which would be more than what we currently have, and since we
have a hard time doing even that would imo I think asking for 2 r-b
tags is completely unrealistic.

One prime example is Ville's watermark patches, which have been ready
(he only did a very few v2 versions for bikesheds) since over a month
ago. But stuck since no one bothered to review them.

So your suggestions (points 1) thru 4) in your original mail in this
thread) haven't fallen on deaf ears. Specifically wrt review from
domain experts I'm ok with just an informal ack and letting someone
else do the detailed review. That way the 2nd function of reviewing of
diffusing knowledge in our distributed team works better when I pick
non-domain-experts.

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

I disagree that I'm a bottleneck. Just yesterday I've merged roughly
50 patches because they where all nicely reviewed. And like I've said
some of those patches have been stuck for a month in
no-one-bothers-to-review-them limbo land.

If we drag out another example and look at the ppgtt stuff from Ben
which I've asked to be reworked quite a bit. Now one mistake I've done
is to be way too optimistic about how much time this will take when
hashing out a merge plan with Ben. I've committed the mistake of
trying to fit the work that I think needs to be done into the
available time Ben has and so done the same wishful thinking planning
I complain about all the time. Next time around I'll try to make an
honest plan first and then try to fit it into the time we have instead
of the other way round.

But I really think the rework was required since with the original
patch series I was often left with the nagging feeling that I just
don't understand what's going on, and whether I'd really be able to
track down a regression if it bisected to one of the patches. So I
couldn't slap an honset r-b tag onto it. The new series is imo great
and a joy to review.

So again please bring up an example where I've failed and we can look
at it and figure out what needs to change to improve the process. Imo
those little patches and adjustements to our process are the way
forward. At least that approach worked really well for beating our
kernel QA process into shape. And yes, it's tedious and results will
take time to show up.

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

I'm not letting you off the hook that easily ;-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch