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

[Tvrtko Ursulin]

On 19/01/2018 11:45, Joonas Lahtinen wrote:
> + 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.

I agree with some points but also think it is not desirable to just lose 
any record of potentially significant review effort that went in before 
first public posting.

The only idea I can think of at the moment, if we don't want to use a 
separate tag, is to, as you say, squash meaningful change log entries 
into the commit, convert the r-b to r-b # internal (similar to r-b # v1 
notation), and add the reviewer as cc explicitly:

   drm/i915: Some patch title

   Some commit text.

   Signed-of-by: A
   Reviewed-by: B # internal
   Cc: B

B then follows up with upgrading the r-b.

?

Regards,

Tvrtko