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

[Dave Airlie]

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.

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.

Dave.