[Mesa-dev] [PATCH 00/30] i965: Add support for I915_FORMAT_MOD_Y_TILED_CCS

Mon Jun 19 12:49:59 UTC 2017

Hi,

I run this on few GEN9 machines and SynMark v7 DeferredAA test improved 
~3%, so it seems multisample fast-clears do help. :-)

	- Eero

On 17.06.2017 01:41, Jason Ekstrand wrote:
> This series is a rework of Ben's series to enable the CCS format modifier.
> It started as an attempt to rebase his original patches on top of my
> resolve reworks inside the miptree code.  However, as I started to dive
> deeper, I found a number of subtle issues:
>
>  1) Thanks to the terrible set of INTEL_AUX_DISABLE_* flags that we use to
>     choose what aux buffers to use, we were set up to never use CCS_E for
>     any external buffers.  Even when Y-tiled on gen9, the most they would
>     ever get was CCS_D.
>
>  2) Whether to do a full or partial resolve or not was based on is_scanout
>     and not on the actual modifier.  If we use I915_FORMAT_MOD_Y_TILED (not
>     the CCS modifier) and choose to use CCS_E with it, it would only get
>     partial resolves and not full resolves.  Of course, this wasn't
>     actually a problem thanks to problem 1 above.
>
>  3) If a user ever imported an image with I915_FORMAT_MOD_Y_TILED_CCS
>     through EGL or GBM and did glClear on it, they would get a fast clear
>     with no way to force a resolve before handing it off to the other
>     process.  Since the other process doesn't know the clear color, this
>     means that any blocks in the clear state in the surface will get
>     whatever random clear color process B thinks it has.
>
>  4) There were three different places where we computed the pitch of the
>     CCS and they all did so differently.  When we go to create the image,
>     we would allocate the CCS with the same pitch as the main surface.  We
>     would then calculate the CCS pitch with ISL when we created mt->mcs_buf.
>     Finally, we had a different mechanism to compute the pitch when we pass
>     it back to the user.  Fortunately, the first only caused us to over-
>     allocate and I think the last two were equivalent (at least for the
>     simple case) so nothing exploded.
>
>  5) Thanks again to our confusing aux enable/disable, we haven't been doing
>     multisample fast-clears since cec30a666930ddb8476a9452a89364a24979ff62
>     around a year ago.
>
> This series takes a bit more round-about approach to enabling the CCS
> modifier that should fix these issues:
>
>  * Patches 1-5 do a bit of refactoring and then rework the way we choose
>    the type of aux compression to use.  They move us away from the crazy
>    enable/disable system to a simple choice system.  This fixes (1) and (5)
>    above.
>
>  * Patches 6-15 refactor things so that we have only one path for going
>    from a __DRIimage to an intel_mipmap_tree.  This was rather painful
>    because we have to be careful to take into account the differences
>    between window system images regular images.
>
>  * Patches 16-22 rework image creation and import to use ISL to do their
>    surface layout calculations.  Previously, all of the surface layout
>    calculations were simply hand-rolled here.  In the particular case of
>    images, the hand-rolling was fairly safe because they were only ever
>    simple 2D non-array images.  However, with the addition of CCS, things
>    were going to get a bit tricky.
>
>  * Patches 23-30 add support for I915_FORMAT_MOD_Y_TILED.
>
> I've tested this series on our Jenkins system which runs piglit as well as
> the OpenGL and OpenGL ES test suites.  Both piglit and the OpenGL ES suite
> have some number of EGL tests which I hope have tested some of this.  I've
> also tested with kmscube and have verified that I get basically the same
> bandwidth numbers as Ben got on his original series, so I think CCS is
> working properly.
>
> This series can be found here:
>
> https://cgit.freedesktop.org/~jekstrand/mesa/log/?h=review/i965-ccs-mod
>
> Cc: Ben Widawsky <ben at bwidawsk.net>
> Cc: Daniel Stone <daniels at collabora.com>
> Cc: Varad Gautam <varad.gautam at collabora.com>
> Cc: Chad Versace <chadversary at chromium.org>
> Cc: Topi Pohjolainen <topi.pohjolainen at intel.com>
>
> Ben Widawsky (6):
>   i965/miptree: Add a return for updating of winsys
>   i965/miptree: Allocate mt earlier in update winsys
>   i965: Support images with aux buffers
>   i965/miptree: Allocate mcs_buf for an image's CCS
>   i965: Pretend that CCS modified images are two planes
>   i965: Advertise the CCS modifier
>
> Jason Ekstrand (24):
>   i965/miptree: Delete the layered rendering resolve
>   i965/miptree: Rename the non_msrt_mcs functions to _ccs
>   i965: Don't bother with HiZ in renderbuffer_move_to_temp
>   i965: Clamp clear colors to the representable range
>   i965/miptree: Rework aux enabling
>   i965: Move the DRIimage -> miptree code to intel_mipmap_tree.c
>   i965/miptree: Pass the offset into create_for_bo in
>     create_for_dri_image
>   i965/miptree: Add tile_x/y to total_width/height
>   i965/miptree: Set level_x/h in create_for_dri_image
>   i965: Use miptree_create_for_dri_image in
>     image_target_renderbuffer_storage
>   i965/miptree: Add an explicit format parameter to create_for_dri_image
>   i965/miptree: Add support for window system images to
>     create_for_dri_image
>   i965: Use create_for_dri_image in intel_update_image_buffer
>   i965/miptree: Move CCS allocation into create_for_dri_image
>   i965: Add an isl_device to intel_screen
>   intel/isl: Add basic modifier introspection
>   intel/isl: Add a helper to convert tilings fro ISL to i915
>   i965/screen: Use ISL for allocating image BOs
>   i965/screen: Use ISL for doing image import checks
>   i965/screen: Drop get_tiled_height
>   intel/isl: Add support for I915_FORMAT_MOD_Y_TILED_CCS
>   intel/isl: Add a row_pitch parameter to surf_get_ccs_surf
>   i965/screen: Support import and export of surfaces with CCS
>   i965/miptree: More conservatively resolve external images
>
>  src/intel/Makefile.am                         |   1 +
>  src/intel/Makefile.sources                    |   1 +
>  src/intel/isl/isl.c                           |   4 +-
>  src/intel/isl/isl.h                           |  28 +-
>  src/intel/isl/isl_drm.c                       |  93 +++++
>  src/intel/vulkan/anv_image.c                  |   2 +-
>  src/mesa/drivers/dri/i965/brw_blorp.c         |   4 +-
>  src/mesa/drivers/dri/i965/brw_context.c       |  44 ++-
>  src/mesa/drivers/dri/i965/brw_meta_util.c     |  40 +++
>  src/mesa/drivers/dri/i965/intel_fbo.c         |  30 +-
>  src/mesa/drivers/dri/i965/intel_image.h       |   6 +
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 496 ++++++++++++++++++--------
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.h |  77 ++--
>  src/mesa/drivers/dri/i965/intel_screen.c      | 216 +++++++----
>  src/mesa/drivers/dri/i965/intel_screen.h      |   4 +
>  src/mesa/drivers/dri/i965/intel_tex_image.c   |  98 +----
>  16 files changed, 767 insertions(+), 377 deletions(-)
>  create mode 100644 src/intel/isl/isl_drm.c
>