[Intel-gfx] [PATCH 00/27] ICL basic enabling + GEM

[Joonas Lahtinen]

+ Jani

On Wed, 2018-01-10 at 17:32 -0800, Rodrigo Vivi wrote:
> On Tue, Jan 09, 2018 at 11:23:09PM +0000, Paulo Zanoni wrote:
> > Hello
> > 
> > This is the first series of patches for the Icelake platform. Unlike the other
> > series that introduced new platforms, this one is very small and only contains
> > patches for very basic enabling, interrupts and some GEM code. No patches for
> > display or other subsystems yet and GEM is not complete either. I'm hoping that
> > by splitting Icelake enabling into many small series progress will be better
> > tracked and people only interested in one area of the code will be able to
> > ignore everything else more easily. In addition, except for the first very few
> > patches of this series, many of the sub-series that will follow are independent
> > from each other and can be merged in any order. And on top of everything,
> > tracking down any possible issues identified by the CI system will be easier if
> > the problem is in a series with 20 patches instead of 160 patches.
> 
> good idea.
> 
> > 
> > Another point worth mentioning is that some patches already have Reviewed-by
> > tags. It is important to remind everybody that those tags were often given to
> > some early versions of those patches, and rebasing happened since then due to
> > the fast development pacing of our driver. Reworks may have landed on the
> > upstream driver that we missed while rebasing, so it is likely that some reworks
> > need to be applied to these patches now. I considered just removing the R-B tags
> > before submitting the patches here, but I think it's probably better if we give
> > credit to people who already spent time trying to check for problems in earlier
> > versions of the patches. So, those patches that already have R-B tags need to be
> > re-reviewed now, and special consideration should be given to any rebasing
> > problems. I'd love to see some "R-b tag still stands" emails.
> 
> I'm glad you didn't removed the rv-b tags. The review process that
> happened so far was very productive. Let's keep the right credits in place and
> take extra care when merging to dinq. Let's only merge what we are confident
> that review is still valid or ask for re-reviews and extra acks.
> 
> One idea that I heard this morning was to use on internal some custom tag
> like "Internally-Reviewed-by:" but I don't like this idea of adding custom
> tags and I trust our commiters to differentiate between valid internal reviews
> and risky ones. Agree?
> 
> Thoughts?

I've been all favour of converting R-b's to Cc:s and embedding any
meaningful changelog entries into the commit text. Because it'll be the
first revision sent to public, you can't trace any of the previous
review comments back by searching mailing lists. It'll only add
confusion.

I don't see the value added by leaving just the changelog entries to
the commit messages. Quite contrary, they are a potentialcause of
confusion when somebody tries to track down non-existent review history
in public.

And sending pre-reviewed patches to community mailing lists also
doesn't feel quite right. Even for IRC review the BKM is to respond to
the mailing list and note that the patch received a R-b in IRC, for
documentation purposes.

And when you add the fact that there is high chance of not invalidating
the reviews when they should be (due to the urgency and amount of
patches there's related to new product development), I see it more as a
problem maker than a solver.

It has little to give but the trade has much to lose.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation