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

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> writes:

> 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

or optionally

Reviewed-by: B # internal v3

-Mika

>    Cc: B
>
> B then follows up with upgrading the r-b.
>
> ?
>
> Regards,
>
> Tvrtko
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx