Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
