Re: [Intel-gfx] [PATCH 00/27] ICL basic enabling + GEM
Tvrtko Ursulin 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 +, 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
Re: [Intel-gfx] [PATCH 00/27] ICL basic enabling + GEM
On Fri, 19 Jan 2018, 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 +, 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
Re: [Intel-gfx] [PATCH 00/27] ICL basic enabling + GEM
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 +, 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/27] ICL basic enabling + GEM
+ Jani On Wed, 2018-01-10 at 17:32 -0800, Rodrigo Vivi wrote: > On Tue, Jan 09, 2018 at 11:23:09PM +, 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/27] ICL basic enabling + GEM
On Tue, Jan 09, 2018 at 11:23:09PM +, 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? > > Display-related patches and other series should arrive soon. > > Thanks, > Paulo > > Ceraolo Spurio, Daniele (1): > drm/i915/icl: new context descriptor support > > Daniele Ceraolo Spurio (1): > drm/i915/icl: Gen11 forcewake support > > Kelvin Gardiner (2): > drm/i915/icl: Update subslice define for ICL 11 > drm/i915/icl: Added ICL 11 slice, subslice and EU fuse detection > > Michel Thierry (2): > drm/i915/icl: Add Indirect Context Offset for Gen11 > drm/i915/icl: Add reset control register changes > > Oscar Mateo (7): > drm/i915/icl: Correctly initialize the Gen11 engines > drm/i915/icl: Check for fused-off VDBOX and VEBOX instances > drm/i915/icl: Enable the extra video decode and enhancement boxes for > Icelake 11 > drm/i915/icl: Make use of the SW counter field in the new context > descriptor > drm/i915/icl: Split out the servicing of the Selector and Shared IIR > registers > drm/i915/icl: Handle RPS interrupts correctly for Gen11 > drm/i915/icl: Enable RC6 and RPS in Gen11 > > Paulo Zanoni (4): > drm/i915/icl: Add the ICL PCI IDs > drm/i915/icl: add icelake_init_clock_gating() > drm/i915/icl: allow the reg_read ioctl to read the RCS TIMESTAMP > register > drm/i915/gen11: add support for reading the timestamp frequency > > Rodrigo Vivi (1): > drm/i915/icl: Add initial Icelake definitions. > > Thomas Daniel (1): > drm/i915/icl: Enhanced execution list support > > Tomasz Lis (1): > drm/i915/icl: Add configuring MOCS in new Icelake engines > > Tvrtko Ursulin (6): > drm/i915/icl: Icelake interrupt register addresses and bits > drm/i915/icl: Show interrupt registers in debugfs > drm/i915/icl: Prepare for more rings > drm/i915/icl: Interrupt handling > drm/i915/icl: Ringbuffer interrupt handling > drm/i915/icl: Gen11 render context size > > kgardine (1): > drm/i915/icl: Set graphics mode register for gen11 > > drivers/gpu/drm/i915/i915_debugfs.c | 92 - > drivers/gpu/drm/i915/i915_drv.c | 2 + > drivers/gpu/drm/i915/i915_drv.h | 16 +- > drivers/gpu/drm/i915/i915_gem.h | 2 +- > drivers/gpu/drm/i915/i915_gem_context.c | 14 +- > drivers/gpu/drm/i915/i915_gem_context.h | 2 + > drivers/gpu/drm/i915/i915_irq.c | 274 > +- > drivers/gpu/drm/i915/i915_pci.c | 15 ++ > drivers/gpu/drm/i915/i915_reg.h | 119 ++- > drivers/gpu/drm/i915/intel_breadcrumbs.c | 20 +- > drivers/gpu/drm/i915/intel_device_info.c
