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