[Mesa-dev] [PATCH 00/32] i965: Enabale CCS_E for sRGB render buffers

Sat Jul 22 09:32:05 UTC 2017

On Wed, Jul 19, 2017 at 02:01:26PM -0700, Jason Ekstrand wrote:
> Gen9 hardware has this annoying little corner where CCS_E is not allowed
> for any sRGB formats.  This is fixed on gen10 but on gen9 there's nothing
> we can do; it just doesn't work.  The old approach to working around this
> was to just disable CCS_E the moment we saw sRGB.  This is bad because GLX
> gives out sRGB-capable visuals by default and you can easily get one
> through EGL as well even if you never enable sRGB encode.  This means that
> users who have sRGB visuals and don't care about sRGB encode are getting
> unnecessarily punished.  This isn't a huge problem today because you also
> can't do CCS_E on X-tiled images but it will be a problem the moment we
> start seeing the Y-tiling modifier through the window system.
> 
> Also, I think sRGB + CCS was plain broken for the less likely case of
> rendering to a texture-backed framebuffer.  Our tracking for sRGB was based
> on piles of sRGBEnabled checks that I'm not at all sure added up to correct
> code.  When trying to better test the CCS_E modifier, I patched waffle to
> start using modifiers whenever the GBM back-end was in-use.  When I ran
> piglit with these waffle patches and my old CCS_E series, our pass rate was
> under 50%.  I think part of that was due to bugs with sRGB and part of it
> was due to not having a plan for falling back to CCS_D once the CCS_E
> modifier gets used.  This series is that plan.
> 
> The first 5 patches are a couple of bugfixes and the removal of a couple of
> bogus restrictions.  In particular, we were disabling CCS_E on all
> renderbuffers for no good reason.  Patch 3 fixes bugs exposed by patch 2
> related to glBlitFramebuffers with both color and depth bits specified.
> 
> The next 5 add a partial resolve pass for MCS and hook it up so that we can
> handle clear colors with texture views correctly.
> 
> The last 22 patches rework things so that we can properly fall back to
> CCS_D whenever we can't render with CCS_E.  This requires adding a seventh
> value to the isl_aux_state enum to describe a "partial clear" state which
> is the state a CCS_E image is in when it's been fast-cleared and then
> rendered to using CCS_D.  Tracking this additional state allows us to turn
> on CCS_E even when we have an sRGB visual and then just silently resolve if
> we ever need to render with sRGB encode enabled.  If they just turn on sRGB
> encode and leave it on, then they don't get a resolve because our tracking
> code knows that you can do CCS_D rendering on a CCS_E surface that is in
> the CLEAR state and the end result is the PARTIAL_CLEAR state.
> 
> We need to land this series before we flip on the CCS_E modifier and I'd
> like to land it in time for the 17.2 release if we can.
> 
> Happy Reviewing!

There were only small comments now and there most of which you already
covered. I looked at your wip/i965-srgb-ccs branch, patch 15 got updated but
patch 24 looked the same as in the list. I'd like to have a look at 24 once
you have it. Otherwise series:

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

> 
> Jason Ekstrand (32):
>   i965/surface_state: Use the minified depth for number of image layers
>   i965/blorp: Use the renderbuffer format for clears
>   i965/blorp: Do flushes around depth resolves
>   i965/miptree: Stop setting FOR_SCANOUT for renderbuffers
>   i965/miptree: Remove some unneeded restrictions
>   intel/blorp: Add a partial resolve pass for MCS
>   i965/miptree: Make layer_range_length return locical layers
>   i965/miptree: Tighten up finish_mcs_write
>   i965/miptree: Add support for partially resolving MCS
>   i965/miptree: Partially resolve MCS for texture views
>   i965/miptree: Add a helper for getting the aux usage for texturing
>   i965/miptree: Rework prepare/finish_render to be in terms of aux_usage
>   i965/blorp: Do prepare/finsh manually
>   i965/blorp: Use texture/render_aux_usage for blits
>   i965/blorp: Be more accurate about aux usage in blorp_copy
>   i965/blorp: Use render_aux_usage for color clears
>   i965/blorp: Use prepare/finish_depth for depth clears
>   i965/miptree: Refactor some things to use mt->aux_usage
>   i965/miptree: Take an aux_usage in prepare/finish
>   intel/isl: Add an aux state for "partial clear"
>   i965/miptree: Use ISL_AUX_STATE_PARTIAL_CLEAR for CCS_D
>   i965/miptree: Allow for accessing a CCS_E image as CCS_D
>   i965/miptree: Use miptree range helpers in has_color_unresolved
>   i965/miptree: Take an isl_format in prepare_texture
>   i965/surface_state: Take an isl_aux_usage in emit_surface_state
>   i965/surface_state: Get the aux usage from the miptree code
>   intel/isl/format: Dedent the template in gen_format_layout.py
>   intel/isl/format: Add an srgb_to_linear helper
>   i965: Weaken the texture view rules for formats slightly
>   intel/blorp: Allow blorp_copy on sRGB formats
>   intel/isl: Add a helper for determining if a color is 0/1
>   i965: Enable regular fast-clears (CCS_D) on gen9+
> 
>  src/intel/blorp/blorp.h                          |   6 +
>  src/intel/blorp/blorp_blit.c                     |  18 +-
>  src/intel/blorp/blorp_clear.c                    | 105 ++++-
>  src/intel/blorp/blorp_nir_builder.h              | 102 +++++
>  src/intel/blorp/blorp_priv.h                     |   1 +
>  src/intel/isl/gen_format_layout.py               | 161 +++++---
>  src/intel/isl/isl.c                              |  27 ++
>  src/intel/isl/isl.h                              |  99 +++--
>  src/mesa/drivers/dri/i965/brw_blorp.c            | 349 ++++++++++------
>  src/mesa/drivers/dri/i965/brw_blorp.h            |   5 +
>  src/mesa/drivers/dri/i965/brw_clear.c            |   3 +-
>  src/mesa/drivers/dri/i965/brw_draw.c             |  14 +-
>  src/mesa/drivers/dri/i965/brw_meta_util.c        |  11 -
>  src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 160 +++----
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c    | 504 +++++++++++++++--------
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.h    |  20 +-
>  16 files changed, 1058 insertions(+), 527 deletions(-)
>  create mode 100644 src/intel/blorp/blorp_nir_builder.h
> 
> -- 
> 2.5.0.400.gff86faf
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev