On Wed, May 09, 2018 at 10:47:24AM -0700, Nanley Chery wrote:
> v2: Inline the switch statement (Jason)
>
> Reviewed-by: Jason Ekstrand <ja...@jlekstrand.net>
> ---
> src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 90 ++++++++-----------
> 1 file changed, 38 insertions(+), 52 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index f78b862a702..b5d7d691ecc 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -1777,30 +1777,37 @@ intel_miptree_level_enable_hiz(struct brw_context
> *brw,
> }
>
>
> -/* Returns true iff all params are successfully filled. */
> -static bool
> -get_aux_buf_params(const struct brw_context *brw,
> - const struct intel_mipmap_tree *mt,
> - enum isl_aux_state *initial_state,
> - uint8_t *memset_value,
> - struct isl_surf *aux_surf)
> +/**
> + * Allocate the initial aux surface for a miptree based on mt->aux_usage
> + *
> + * Since MCS, HiZ, and CCS_E can compress more than just clear color, we
> + * create the auxiliary surfaces up-front. CCS_D, on the other hand, can
> only
> + * compress clear color so we wait until an actual fast-clear to allocate it.
> + */
> +bool
> +intel_miptree_alloc_aux(struct brw_context *brw,
> + struct intel_mipmap_tree *mt)
> {
> - assert(initial_state && memset_value && aux_surf);
> + assert(mt->aux_buf == NULL);
> +
> + /* Get the aux buf allocation parameters for this miptree. */
> + enum isl_aux_state initial_state;
> + uint8_t memset_value;
> + struct isl_surf aux_surf;
> + bool aux_surf_ok;
>
> switch (mt->aux_usage) {
> case ISL_AUX_USAGE_NONE:
> - aux_surf->size = 0;
> - return true;
> + aux_surf.size = 0;
> + aux_surf_ok = true;
> + break;
> case ISL_AUX_USAGE_HIZ:
> assert(!_mesa_is_format_color_format(mt->format));
>
> - *initial_state = ISL_AUX_STATE_AUX_INVALID;
> - {
> - MAYBE_UNUSED bool ok =
> - isl_surf_get_hiz_surf(&brw->isl_dev, &mt->surf, aux_surf);
> - assert(ok);
> - }
> - return true;
> + initial_state = ISL_AUX_STATE_AUX_INVALID;
> + aux_surf_ok = isl_surf_get_hiz_surf(&brw->isl_dev, &mt->surf,
> &aux_surf);
> + assert(aux_surf_ok);
> + break;
> case ISL_AUX_USAGE_MCS:
> assert(_mesa_is_format_color_format(mt->format));
> assert(brw->screen->devinfo.gen >= 7); /* MCS only used on Gen7+ */
> @@ -1817,14 +1824,11 @@ get_aux_buf_params(const struct brw_context *brw,
> * Note: the clear value for MCS buffers is all 1's, so we memset to
> * 0xff.
> */
> - *initial_state = ISL_AUX_STATE_CLEAR;
> - *memset_value = 0xFF;
> - {
> - MAYBE_UNUSED bool ok =
> - isl_surf_get_mcs_surf(&brw->isl_dev, &mt->surf, aux_surf);
> - assert(ok);
> - }
> - return true;
> + initial_state = ISL_AUX_STATE_CLEAR;
> + memset_value = 0xFF;
> + aux_surf_ok = isl_surf_get_mcs_surf(&brw->isl_dev, &mt->surf,
> &aux_surf);
> + assert(aux_surf_ok);
> + break;
> case ISL_AUX_USAGE_CCS_D:
> case ISL_AUX_USAGE_CCS_E:
> assert(_mesa_is_format_color_format(mt->format));
> @@ -1839,36 +1843,18 @@ get_aux_buf_params(const struct brw_context *brw,
> * A CCS value of 0 indicates that the corresponding block is in the
> * pass-through state which is what we want.
> *
> - * For CCS_D, do the same thing. On gen9+, this avoids having any
> undefined
> - * bits in the aux buffer.
> + * For CCS_D, do the same thing. On gen9+, this avoids having any
> + * undefined bits in the aux buffer.
> */
> - *initial_state = ISL_AUX_STATE_PASS_THROUGH;
> - *memset_value = 0;
> - return isl_surf_get_ccs_surf(&brw->isl_dev, &mt->surf, aux_surf, 0);
> + initial_state = ISL_AUX_STATE_PASS_THROUGH;
> + memset_value = 0;
> + aux_surf_ok =
> + isl_surf_get_ccs_surf(&brw->isl_dev, &mt->surf, &aux_surf, 0);
Should we assert here also?
assert(aux_surf_ok);
> + break;
> }
>
> - unreachable("Invalid aux usage");
> -}
> -
> -
> -/**
> - * Allocate the initial aux surface for a miptree based on mt->aux_usage
> - *
> - * Since MCS, HiZ, and CCS_E can compress more than just clear color, we
> - * create the auxiliary surfaces up-front. CCS_D, on the other hand, can
> only
> - * compress clear color so we wait until an actual fast-clear to allocate it.
> - */
> -bool
> -intel_miptree_alloc_aux(struct brw_context *brw,
> - struct intel_mipmap_tree *mt)
> -{
> - assert(mt->aux_buf == NULL);
> -
> - /* Get the aux buf allocation parameters for this miptree. */
> - enum isl_aux_state initial_state;
> - uint8_t memset_value;
> - struct isl_surf aux_surf;
> - if (!get_aux_buf_params(brw, mt, &initial_state, &memset_value,
> &aux_surf))
> + /* Ensure we have a valid aux_surf. */
> + if (aux_surf_ok == false)
> return false;
I started wondering when is this allowed to fail, for NONE/HIZ/MCS there is
assert above. So that leaves CCS_D/CCS_E, but shouldn't they be treated the
same as the rest?
>
> /* No work is needed for a zero-sized auxiliary buffer. */
> --
> 2.17.0
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
