On Wed, May 16, 2018 at 09:11:38AM +0300, Pohjolainen, Topi wrote:
> 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);
>
We can't at the moment. One example that would cause
isl_surf_get_ccs_surf() to return false is where we're on gen8 and we
attempt to fast-clear level 0 of a 3D texture.
> > + 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?
>
Great observation. It think we can assert for CCS_E but not CCS_D.
We can't assert for CCS_D because we have this restriction in
isl_surf_get_ccs_surf():
if (ISL_DEV_GEN(dev) <= 8 && surf->dim != ISL_SURF_DIM_2D)
return false;
But we don't have an equivalent restriction in
intel_miptree_supports_ccs(). Would you like me to send out some patches
to add the restriction and assert for all aux surfaces? They won't be a
part of this series.
-Nanley
> >
> > /* 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
