<div dir="ltr">I think I've now reviewed everything except 2 patches. For the "Update a few assertions" patch, you said you would run a test but never reported back the results. The other is the patch for re-enabling sRGB fast-clears. That one isn't needed for enabling and I'm not yet convinced that it's removing enough code. I need to understand Sky lake sRGB myself before I can really review anything. Also, at Chad's request, I'll probably be adjusting the way that code works to be based on the ISL table at which point, re-enabling sRGB will just naturally fall out as a result of the ISL format table update. </div><div class="gmail_extra"> <div class="gmail_quote">On Mon, Jun 5, 2017 at 10:04 AM, Anuj Phogat <<a href="mailto:anuj.phogat@gmail.com" target="_blank">anuj.phogat@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">For your reference, here is a list of patches pending review in this series: 3, 18, 19, 22, 23, 24.5. Thanks Anuj <div class="HOEnZb"><div class="h5"> On Fri, Jun 2, 2017 at 5:48 PM, Anuj Phogat <<a href="mailto:anuj.phogat@gmail.com">anuj.phogat@gmail.com</a>> wrote: > On Fri, Jun 2, 2017 at 4:48 PM, Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> wrote: >> On Mon, May 22, 2017 at 9:32 AM, Anuj Phogat <<a href="mailto:anuj.phogat@gmail.com">anuj.phogat@gmail.com</a>> wrote: >>> >>> On Fri, May 12, 2017 at 4:38 PM, Anuj Phogat <<a href="mailto:anuj.phogat@gmail.com">anuj.phogat@gmail.com</a>> >>> wrote: >>> > This series adds support for Cannonlake. >>> > >>> > Changes from V1 to V2: >>> > - Incorporated the review comments from V1. >>> > - Rebased 8 months old CNL branch on top of master >>> > - Wired up Linux and Android build files for gen10 >>> > - Replaced the use of few gen9 functions with gen10 specific functions. >>> > - Squashed few patches, dropped few and created new patches. >>> > >>> Thanks to Jason and Ken who have reviewed few patches in this series. >>> Rest of them are still waiting for the review. I really want to land this >>> series >>> (at least first 15-16 patches) soon. There are some very easy patches any >>> one can review like enabling Mesa to build for gen10 etc. Please take a >>> look at them. Thanks :). >> >> >> Finally got around to looking at these again... >> >> Now that we're switching everything over to genxml, I think it's a good idea >> to be a bit more intentional in the way we write new platform patches. >> There are a number of patches in this series that are much harder to review >> than they need to be because they're written more-or-less in order of code >> development and not in a logical reviewable order. In particular, there's a >> patch which updates a pile of switch statements to get rid of asserts but it >> just moves them all over to gen9. Moving stuff to gen10 is in a different >> patch. The result is that it's very hard, without squashing things >> together, to tell whether or not we missed anything when we switched them >> over to actual gen10 functions. This isn't really a criticism of Anuj and >> Ben. They've done a lot of rebasaing on top of a lot of driver architecture >> changes. I think this will be much easier to do better in the future. >> >> In my view, the ideal platform enabling patch series would look something >> like this: >> >> 1) Add genN.xml > Does it make sense to break it down in to few patches based on manual > changes we make to an auto generated genN.xml ? or send out just one > patch and reviewer can diff it with previous gen and verify the changes ? > >> 2) Add the #defines and #includes to genxml and the build system stuff to >> generate the packing headers >> 3) Update stuff in src/intel/common such as URB configuration changes >> 4) Update ISL: >> a) Any needed generic ISL changes such as adding new layaouts. (Cannon >> lake doesn't add anything, so nothing to do here). This may be multiple >> patches. >> b) Get ISL surface state emit code building for the new hardware. This >> includes updating the autotools and Android makefiles, adding function >> prototypes, updating switch statements, etc. If changes are needed in >> isl_surface_state.c or isl_depth_stencil.c, they should be minimal bug still >> enough that the end result is correct. >> 5) Update BLORP as needed for the new platform. Sadly, there's no way to >> build-test this without the next step since BLORP doesn't build its own genX >> files. >> 6) Get GL driver genxml state-upload and blorp code building and hooked in. >> Core blorp changes should go in their own patch (above) but this will >> include the build system changes for blorp as well. This also includes >> updating switch statements. >> 8) Implement workarounds, features, etc. >> >> The important part is that we keep related changes together. In order to >> review this series, I had to squash patches 6, 9, 14, and 15 together. >> Again, I'm not casting any blame here. The series makes a lot of historical >> sense. It's just hard to review as-is. >> > I agree with you that the series could have been structured better for ease > of review. Thanks for the reviews and all the great suggestions above. I'll > keep them in mind for future h/w enabling patches. > >> --Jason >> >>> >>> > What's remaining: >>> > - Add missing gen10 bits in Vulkan driver. >>> > - Fix failing piglit, cts tests for GL and Vulkan. >>> > >>> > You can also find this series at: >>> > <a href="https://github.com/aphogat/mesa.git" rel="noreferrer" target="_blank">https://github.com/aphogat/mesa.git</a> >>> > branch: reviews >>> > >>> > Anuj Phogat (18): >>> > i965/cnl: Define genX(x) and GENX(x) for gen10 >>> > i965/cnl: Include gen10_pack.h >>> >>> > i965/cnl: Add gen10 specific function declarations >>> > i965/cnl: Update the script generating genX_bits.h >>> > i965/cnl: Add isl_gen10 header and source files >>> > i965/cnl: Wire up Mesa build files for gen10 >>> > i965/cnl: Wire up android Mesa build files for gen10 >>> > i965/cnl: Add pci id for INTEL_DEVID_OVERRIDE >>> > i965/cnl: Add cnl bits in aubinator >>> > i965/cnl: Update few assertions >>> > i965/cnl: Handle gen10 in switch cases across the driver >>> > i965/cnl: Start using CNL MOCS defines >>> > i965/cnl: Start using gen10 specific functions >>> > i965/cnl: Don't resolve single sampled color rb in case of sRGB >>> > formats >>> > i965/cnl: Make URB {VS, GS, HS, DS} sizes non multiple of 3 >>> > i965/cnl: Reformat surface_format_info table to accomodate gen10+ >>> > i965/cnl: Enable CCS_E and RT support for few formats >>> > i965: Simplify get_l3_way_size() function >>> > >>> > Ben Widawsky (5): >>> > i965: Make feature macros gen8 based >>> > i965/cnl: Add a preliminary device for Cannonlake >>> > i965/cnl: Implement new pipe control workaround >>> > i965/cnl: Implement depth count workaround >>> > i965/cnl: Restore lossless compression for sRGB formats >>> > >>> > Jason Ekstrand (1): >>> > i965/cnl: Add gen10.xml >>> > >>> > include/pci_ids/i965_pci_ids.h | 12 + >>> > src/intel/<a href="http://Android.genxml.mk" rel="noreferrer" target="_blank">Android.genxml.mk</a> | 5 + >>> > src/intel/<a href="http://Android.isl.mk" rel="noreferrer" target="_blank">Android.isl.mk</a> | 20 + >>> > src/intel/<a href="http://Android.vulkan.mk" rel="noreferrer" target="_blank">Android.vulkan.mk</a> | 21 + >>> > src/intel/<a href="http://Makefile.isl.am" rel="noreferrer" target="_blank">Makefile.isl.am</a> | 4 + >>> > src/intel/Makefile.sources | 12 +- >>> > src/intel/<a href="http://Makefile.vulkan.am" rel="noreferrer" target="_blank">Makefile.vulkan.am</a> | 7 +- >>> > src/intel/common/gen_device_info.c | 71 +- >>> > src/intel/common/gen_device_info.h | 1 + >>> > src/intel/common/gen_l3_config.c | 11 +- >>> > src/intel/compiler/brw_compiler.h | 2 +- >>> > src/intel/compiler/brw_eu.c | 2 + >>> > src/intel/compiler/brw_eu_compact.c | 1 + >>> > src/intel/genxml/gen10.xml | 3563 >>> > ++++++++++++++++++++++ >>> > src/intel/genxml/genX_pack.h | 2 + >>> > src/intel/genxml/gen_bits_header.py | 6 +- >>> > src/intel/genxml/gen_macros.h | 3 + >>> > src/intel/isl/isl.c | 9 + >>> > src/intel/isl/isl_format.c | 498 +-- >>> > src/intel/isl/isl_gen10.c | 41 + >>> > src/intel/isl/isl_gen10.h | 45 + >>> > src/intel/isl/isl_priv.h | 12 + >>> > src/intel/tools/aubinator.c | 8 +- >>> > src/intel/vulkan/anv_cmd_buffer.c | 1 + >>> > src/intel/vulkan/anv_device.c | 1 + >>> > src/intel/vulkan/anv_entrypoints_gen.py | 1 + >>> > src/mesa/drivers/dri/i965/Android.mk | 24 +- >>> > src/mesa/drivers/dri/i965/Makefile.am | 6 +- >>> > src/mesa/drivers/dri/i965/Makefile.sources | 4 + >>> > src/mesa/drivers/dri/i965/brw_blorp.c | 6 + >>> > src/mesa/drivers/dri/i965/brw_blorp.h | 2 + >>> > src/mesa/drivers/dri/i965/brw_context.c | 2 +- >>> > src/mesa/drivers/dri/i965/brw_formatquery.c | 1 + >>> > src/mesa/drivers/dri/i965/brw_pipe_control.c | 11 + >>> > src/mesa/drivers/dri/i965/brw_program.c | 2 +- >>> > src/mesa/drivers/dri/i965/brw_queryobj.c | 8 + >>> > src/mesa/drivers/dri/i965/brw_state.h | 9 + >>> > src/mesa/drivers/dri/i965/brw_state_upload.c | 4 +- >>> > src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 2 + >>> > src/mesa/drivers/dri/i965/gen7_urb.c | 12 + >>> > src/mesa/drivers/dri/i965/genX_state_upload.c | 4 +- >>> > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 2 +- >>> > src/mesa/drivers/dri/i965/intel_screen.c | 2 + >>> > 43 files changed, 4180 insertions(+), 280 deletions(-) >>> > create mode 100644 src/intel/genxml/gen10.xml >>> > create mode 100644 src/intel/isl/isl_gen10.c >>> > create mode 100644 src/intel/isl/isl_gen10.h >>> > >>> > -- >>> > 2.9.3 >>> > >>> _______________________________________________ >>> mesa-dev mailing list >>> <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> >>> <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> >> >> </div></div></blockquote></div> </div>