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

[Daniel Vetter]

On Sat, Jul 27, 2013 at 06:52:55PM +1000, Dave Airlie wrote:
> On Sat, Jul 27, 2013 at 10:05 AM, Ben Widawsky <ben at bwidawsk.net> wrote:
> > On Sat, Jul 27, 2013 at 09:13:38AM +1000, Dave Airlie wrote:
> >> >
> >> > Hey, overall it's actually quite a bit of fun.
> >> >
> >> > I do agree that QA is really important for a fastpaced process, but
> >> > it's also not the only peace to get something in. Review (both of the
> >> > patch itself but also of  the test coverage) catches a lot of issues,
> >> > and in many cases not the same ones as QA would. Especially if the
> >> > testcoverage of a new feature is less than stellar, which imo is still
> >> > the case for gem due to the tons of finickle cornercases.
> >>
> >> Just my 2c worth on this topic, since I like the current process, and
> >> I believe making it too formal is probably going to make things suck
> >> too much.
> >>
> >> I'd rather Daniel was slowing you guys down up front more, I don't
> >> give a crap about Intel project management or personal manager relying
> >> on getting features merged when, I do care that you engineers when you
> >> merge something generally get transferred 100% onto something else and
> >> don't react strongly enough to issues on older code you have created
> >> that either have lain dormant since patches merged or are regressions
> >> since patches merged. So I believe the slowing down of merging
> >> features gives a better chance of QA or other random devs of finding
> >> the misc regressions while you are still focused on the code and
> >> hitting the long term bugs that you guys rarely get resourced to fix
> >> unless I threaten to stop pulling stuff.
> >>
> >> So whatever Daniel says goes as far as I'm concerned, if I even
> >> suspect he's taken some internal Intel pressure to merge some feature,
> >> I'm going to stop pulling from him faster than I stopped pulling from
> >> the previous maintainers :-), so yeah engineers should be prepared to
> >> backup what they post even if Daniel is wrong, but on the other hand
> >> they need to demonstrate they understand the code they are pushing and
> >> sometimes with ppgtt and contexts I'm not sure anyone really
> >> understands how the hw works let alone the sw :-P
> >>
> >> Dave.
> >
> > Honestly, I wouldn't have responded if you didn't mention the Intel
> > program management thing...
> >
> > The problem I am trying to emphasize, and let's use contexts/ppgtt as an
> > example, is we have three options:
> > 1. It's complicated, and a big change, so let's not do it.
> > 2. I continue to rebase the massive change on top of the extremely fast
> > paced i915 tree, with no QA coverage.
> > 3. We get decent bits merged ASAP by putting it in a repo that both gets
> > much wider usage than my personal branch, and gets nightly QA coverage.
> >
> > PPGTT + Contexts have existed for a while, and so we went with #1 for
> > quite a while.
> >
> > Now we're at #2. There's two sides to your 'developer needs to
> > defend...' I need Daniel to give succinct feedback, and agree upon steps
> > required to get code merged. My original gripe was that it's hard to
> > deal with the, "that patch is too big" comments almost 2 months after
> > the first version was sent. Equally, "that looks funny" without a real
> > explanation of what looks funny, or sufficient thought up front about
> > what might look better is just as hard to deal with. Inevitably, yes -
> > it's a big scary series of patches - but if we're honest with ourselves,
> > it's almost guaranteed to blow up somewhere regardless of how much we
> > rework it, and who reviews it. Blowing up long before you merge would
> > always be better than the after you merge.
> >
> > My desire is to get to something like #3. I had a really long paragraph
> > on why and how we could do that, but I've redacted it. Let's just leave
> > it as, I think that should be the goal.
> >
> 
> Daniel could start taking topic branches like Ingo does, however he'd
> have a lot of fun merging them,
> he's already getting closer and closer to the extreme stuff -tip does,
> and he'd have to feed the topics to QA and possibly -next separately,
> the question is when to include a branch or not include it.

Yeah, I guess eventually we need to go more crazy with the branching model
for drm/i915. But even getting to the current model was quite some fun, so
I don't want to rock the boat too much if not required ;-)

Also I fear that integrating random developer branches myself will put me
at an ugly spot where I partially maintain (due to the regular merge
conflicts) patches I haven't yet accepted. And since I'm only human I'll
then just merge patches to get rid of the merge pain. So I don't really
want to do that.

Similarly for the internal tree (which just contains hw enabling for
platforms we're not yet allowed to talk about and some related hacks) I've
put down the rule that I won't take patches which are not upstream
material (minus the last bit of polish and no real review requirement).
Otherwise I'll start to bend the upstream rules a bit ... ;-)
 
> Maybe he can schedule a time that QA gets all the branches, and maybe
> not put stuff into -next until we are sure its on its way.

Imo the solution here is for QA to beat the nightly test infrastructure
into a solid enough shape that it can run arbitrary developer branches,
unattended. I think we're slowly getting there (but for obvious reasons
that's no my main aim as the maintainer when working together with our QA
guys).

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