Re: [PATCH 00/27] ICL basic enabling + GEM

[Jani Nikula <jani.nikula@intel.com>]

On Fri, 19 Jan 2018, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> 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 don't want to devalue internal review, it's good stuff for the most
part. However, there are a few issues. Most patches have seen a bunch of
rebases and fixups since the review. Sometimes the review means, good
enough for merging internally. And finally, I don't want us to give the
impression of internal rubber stamp review. Basically I want every
internal Reviewed-by confirmed on the public list. I think this pretty
much aligns with what Paulo said in the cover letter.

It's a matter of taste whether you require the confirmation of reviews
or change them to Cc's and ask for the same.

For the changelogs, I agree we should start scrubbing them for v1 posted
on the public list.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx