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

Tue Jun 27 18:18:56 UTC 2017

On Wed, Jun 21, 2017 at 05:51:00PM -0700, Jason Ekstrand wrote:
> On Fri, Jun 16, 2017 at 3:41 PM, Jason Ekstrand <jason at jlekstrand.net>
> 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.

Suggestion to revise the commit message in patch 12.

Typo in the subject in patch 19.

In 25 there is a question about incoming aux data - it looks that were are
taking it as containing meaningful data in some cases.

Question in patch 27.

Typo in patch 28.

Given we have those sorted, patches 12-29 are:

Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>

> >
> > 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
> >
> 
> I've rebased on top of Topi's ISL reworks and force-pushed that branch.
> You can see the result of the rebase there.
> 
> 
> > 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
> >
> > --
> > 2.5.0.400.gff86faf
> >
> >

> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev