On Wed, May 16, 2018 at 09:33:34AM -0700, Nanley Chery wrote:
> 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.
Sounds good, in the meanwhile you can add my r-b here:
Reviewed-by: Topi Pohjolainen <topi.pohjolai...@intel.com>
>
> -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
