On Thu, May 17, 2018 at 07:44:34AM +0300, Pohjolainen, Topi wrote:
> 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>
> 

Thanks!

> > 
> > -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

Reply via email to