On Mon, May 07, 2018 at 11:30:12AM -0700, Nanley Chery wrote:
> On Mon, May 07, 2018 at 04:12:24PM +0300, Pohjolainen, Topi wrote:
> > On Thu, May 03, 2018 at 12:03:56PM -0700, Nanley Chery wrote:
> > > There isn't much that changes between the aux allocation functions.
> > > Remove the duplicated code.
> > > ---
> > > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 227
> > > +++++++++++---------------
> > > src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 9 -
> > > 2 files changed, 95 insertions(+), 141 deletions(-)
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > index 84be7c07c6f..08976d2680f 100644
> > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > @@ -1736,95 +1736,9 @@ intel_alloc_aux_buffer(struct brw_context *brw,
> > > return buf;
> > > }
> > >
> > > -static bool
> > > -intel_miptree_alloc_mcs(struct brw_context *brw,
> > > - struct intel_mipmap_tree *mt,
> > > - GLuint num_samples)
> > > -{
> > > - assert(brw->screen->devinfo.gen >= 7); /* MCS only used on Gen7+ */
> > > - assert(mt->aux_buf == NULL);
> > > - assert(mt->aux_usage == ISL_AUX_USAGE_MCS);
> > > -
> > > - /* Multisampled miptrees are only supported for single level. */
> > > - assert(mt->first_level == 0);
> > > - enum isl_aux_state **aux_state =
> > > - create_aux_state_map(mt, ISL_AUX_STATE_CLEAR);
> > > - if (!aux_state)
> > > - return false;
> > > -
> > > - struct isl_surf temp_mcs_surf;
> > > -
> > > - MAYBE_UNUSED bool ok =
> > > - isl_surf_get_mcs_surf(&brw->isl_dev, &mt->surf, &temp_mcs_surf);
> > > - assert(ok);
> > > -
> > > - /* From the Ivy Bridge PRM, Vol 2 Part 1 p326:
> > > - *
> > > - * When MCS buffer is enabled and bound to MSRT, it is required
> > > that it
> > > - * is cleared prior to any rendering.
> > > - *
> > > - * Since we don't use the MCS buffer for any purpose other than
> > > rendering,
> > > - * it makes sense to just clear it immediately upon allocation.
> > > - *
> > > - * Note: the clear value for MCS buffers is all 1's, so we memset to
> > > 0xff.
> > > - */
> > > - mt->aux_buf = intel_alloc_aux_buffer(brw, &temp_mcs_surf, true, 0xFF);
> > > - if (!mt->aux_buf) {
> > > - free(aux_state);
> > > - return false;
> > > - }
> > > -
> > > - mt->aux_state = aux_state;
> > > -
> > > - return true;
> > > -}
> > > -
> > > -static bool
> > > -intel_miptree_alloc_ccs(struct brw_context *brw,
> > > - struct intel_mipmap_tree *mt)
> > > -{
> > > - assert(mt->aux_buf == NULL);
> > > - assert(mt->aux_usage == ISL_AUX_USAGE_CCS_E ||
> > > - mt->aux_usage == ISL_AUX_USAGE_CCS_D);
> > > -
> > > - struct isl_surf temp_ccs_surf;
> > > -
> > > - if (!isl_surf_get_ccs_surf(&brw->isl_dev, &mt->surf, &temp_ccs_surf,
> > > 0))
> > > - return false;
> > > -
> > > - assert(temp_ccs_surf.size &&
> > > - (temp_ccs_surf.size % temp_ccs_surf.row_pitch == 0));
> > > -
> > > - enum isl_aux_state **aux_state =
> > > - create_aux_state_map(mt, ISL_AUX_STATE_PASS_THROUGH);
> > > - if (!aux_state)
> > > - return false;
> > > -
> > > - /* When CCS_E is used, we need to ensure that the CCS starts off in a
> > > valid
> > > - * state. From the Sky Lake PRM, "MCS Buffer for Render Target(s)":
> > > - *
> > > - * "If Software wants to enable Color Compression without Fast
> > > clear,
> > > - * Software needs to initialize MCS with zeros."
> > > - *
> > > - * 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.
> > > - */
> > > - mt->aux_buf = intel_alloc_aux_buffer(brw, &temp_ccs_surf, true, 0);
> > > - if (!mt->aux_buf) {
> > > - free(aux_state);
> > > - return false;
> > > - }
> > > -
> > > - mt->aux_state = aux_state;
> > > -
> > > - return true;
> > > -}
> > >
> > > /**
> > > - * Helper for intel_miptree_alloc_hiz() that sets
> > > + * Helper for intel_miptree_alloc_aux() that sets
> > > * \c mt->level[level].has_hiz. Return true if and only if
> > > * \c has_hiz was set.
> > > */
> > > @@ -1859,37 +1773,78 @@ intel_miptree_level_enable_hiz(struct brw_context
> > > *brw,
> > > return true;
> > > }
> > >
> > > -bool
> > > -intel_miptree_alloc_hiz(struct brw_context *brw,
> > > - struct intel_mipmap_tree *mt)
> > > -{
> > > - assert(mt->aux_buf == NULL);
> > > - assert(mt->aux_usage == ISL_AUX_USAGE_HIZ);
> > >
> > > - enum isl_aux_state **aux_state =
> > > - create_aux_state_map(mt, ISL_AUX_STATE_AUX_INVALID);
> > > - if (!aux_state)
> > > - return false;
> > > +/* 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)
> > > +{
> > > + assert(initial_state && memset_value && aux_surf);
> > >
> > > - struct isl_surf temp_hiz_surf;
> > > + switch (mt->aux_usage) {
> > > + case ISL_AUX_USAGE_NONE:
> > > + aux_surf->size = 0;
> > > + return true;
> > > + case ISL_AUX_USAGE_HIZ:
> > > + assert(!_mesa_is_format_color_format(mt->format));
> > >
> > > - MAYBE_UNUSED bool ok =
> > > - isl_surf_get_hiz_surf(&brw->isl_dev, &mt->surf, &temp_hiz_surf);
> > > - assert(ok);
> > > + *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;
> > > + case ISL_AUX_USAGE_MCS:
> > > + assert(_mesa_is_format_color_format(mt->format));
> >
> > You could preserve one more assert here:
> >
> > assert(mt->surf.samples > 1);
> >
>
> This assert is found in isl_surf_get_mcs_surf(), so I didn't think
> anyone would mind. I'm actually hoping that we can move the one below
> into ISL as well.
That is fine then, and in that case I would drop the other as well.
>
> > > + assert(brw->screen->devinfo.gen >= 7); /* MCS only used on Gen7+ */
> > >
> > > - mt->aux_buf = intel_alloc_aux_buffer(brw, &temp_hiz_surf, false, 0);
> > > + /* From the Ivy Bridge PRM, Vol 2 Part 1 p326:
> > > + *
> > > + * When MCS buffer is enabled and bound to MSRT, it is
> > > required that
> > > + * it is cleared prior to any rendering.
> > > + *
> > > + * Since we don't use the MCS buffer for any purpose other than
> > > + * rendering, it makes sense to just clear it immediately upon
> > > + * allocation.
> > > + *
> > > + * 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;
> > > + case ISL_AUX_USAGE_CCS_D:
> > > + case ISL_AUX_USAGE_CCS_E:
> > > + assert(_mesa_is_format_color_format(mt->format));
> >
> > And here:
> >
> > assert(mt->surf.samples == 1);
> >
>
> Like the assertion above, this one is present in
> isl_surf_get_ccs_surf().
>
> > Otherwise I couldn't spot anything that was missing/changed:
> >
> > Reviewed-by: Topi Pohjolainen <[email protected]>
> >
>
> Thanks!
>
> > There doesn't seem to be too much code left in intel_miptree_alloc_aux()
> > itself and I was wondering if you tried having the body of
> > get_aux_buf_params() inlined.
> >
>
> When I was writing this series, this function started off inlined, but I
> found it to be better as a seperate function. For example:
> * We can just return isl_surf_get_ccs_surf instead of checking if it failed.
> * We get to keep the unreachable.
> * IMO it's easier to see the high-level algorithm involved with
> allocating the aux buffer.
>
> I had in mind that we may want to inline some parts of
> intel_alloc_aux_buffer() in the future, but it's not completely clear to
> me how that should look just yet.
Makes sense.
>
> -Nanley
>
> > >
> > > - if (!mt->aux_buf) {
> > > - free(aux_state);
> > > - return false;
> > > + /* When CCS_E is used, we need to ensure that the CCS starts off
> > > in a
> > > + * valid state. From the Sky Lake PRM, "MCS Buffer for Render
> > > + * Target(s)":
> > > + *
> > > + * "If Software wants to enable Color Compression without Fast
> > > + * clear, Software needs to initialize MCS with zeros."
> > > + *
> > > + * 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.
> > > + */
> > > + *initial_state = ISL_AUX_STATE_PASS_THROUGH;
> > > + *memset_value = 0;
> > > + return isl_surf_get_ccs_surf(&brw->isl_dev, &mt->surf, aux_surf,
> > > 0);
> > > }
> > >
> > > - for (unsigned level = mt->first_level; level <= mt->last_level;
> > > ++level)
> > > - intel_miptree_level_enable_hiz(brw, mt, level);
> > > -
> > > - mt->aux_state = aux_state;
> > > -
> > > - return true;
> > > + unreachable("Invalid aux usage");
> > > }
> > >
> > >
> > > @@ -1904,33 +1859,41 @@ bool
> > > intel_miptree_alloc_aux(struct brw_context *brw,
> > > struct intel_mipmap_tree *mt)
> > > {
> > > - switch (mt->aux_usage) {
> > > - case ISL_AUX_USAGE_NONE:
> > > - return true;
> > > + assert(mt->aux_buf == NULL);
> > >
> > > - case ISL_AUX_USAGE_HIZ:
> > > - assert(!_mesa_is_format_color_format(mt->format));
> > > - if (!intel_miptree_alloc_hiz(brw, mt))
> > > - return false;
> > > - return true;
> > > + /* 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))
> > > + return false;
> > >
> > > - case ISL_AUX_USAGE_MCS:
> > > - assert(_mesa_is_format_color_format(mt->format));
> > > - assert(mt->surf.samples > 1);
> > > - if (!intel_miptree_alloc_mcs(brw, mt, mt->surf.samples))
> > > - return false;
> > > + /* No work is needed for a zero-sized auxiliary buffer. */
> > > + if (aux_surf.size == 0)
> > > return true;
> > >
> > > - case ISL_AUX_USAGE_CCS_D:
> > > - case ISL_AUX_USAGE_CCS_E:
> > > - assert(_mesa_is_format_color_format(mt->format));
> > > - assert(mt->surf.samples == 1);
> > > - if (!intel_miptree_alloc_ccs(brw, mt))
> > > - return false;
> > > - return true;
> > > + /* Create the aux_state for the auxiliary buffer. */
> > > + mt->aux_state = create_aux_state_map(mt, initial_state);
> > > + if (mt->aux_state == NULL)
> > > + return false;
> > > +
> > > + /* Allocate the auxiliary buffer. */
> > > + const bool needs_memset = initial_state != ISL_AUX_STATE_AUX_INVALID;
> > > + mt->aux_buf = intel_alloc_aux_buffer(brw, &aux_surf, needs_memset,
> > > + memset_value);
> > > + if (mt->aux_buf == NULL) {
> > > + free_aux_state_map(mt->aux_state);
> > > + mt->aux_state = NULL;
> > > + return false;
> > > }
> > >
> > > - unreachable("Invalid aux usage");
> > > + /* Perform aux_usage-specific initialization. */
> > > + if (mt->aux_usage == ISL_AUX_USAGE_HIZ) {
> > > + for (unsigned level = mt->first_level; level <= mt->last_level;
> > > ++level)
> > > + intel_miptree_level_enable_hiz(brw, mt, level);
> > > + }
> > > +
> > > + return true;
> > > }
> > >
> > >
> > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > > index 9a5d97bf348..d8d69698f54 100644
> > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > > @@ -536,15 +536,6 @@ intel_miptree_copy_teximage(struct brw_context *brw,
> > > * functions on a miptree without HiZ. In that case, each function is a
> > > no-op.
> > > */
> > >
> > > -/**
> > > - * \brief Allocate the miptree's embedded HiZ miptree.
> > > - * \see intel_mipmap_tree:hiz_mt
> > > - * \return false if allocation failed
> > > - */
> > > -bool
> > > -intel_miptree_alloc_hiz(struct brw_context *brw,
> > > - struct intel_mipmap_tree *mt);
> > > -
> > > bool
> > > intel_miptree_level_has_hiz(const struct intel_mipmap_tree *mt, uint32_t
> > > level);
> > >
> > > --
> > > 2.16.2
> > >
> > > _______________________________________________
> > > mesa-dev mailing list
> > > [email protected]
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
