On Tue, May 08, 2018 at 04:52:09PM -0700, Jason Ekstrand wrote: > On Thu, May 3, 2018 at 12:03 PM, Nanley Chery <nanleych...@gmail.com> 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) > > > > I think it'd be better if we inlined this into the function below. Having > it split out isn't really buying us much and separates the logic into two > pieces. > >
Topi commented on it being split out as well. I'll inline it. > > +{ > > + 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 = > > > > It might be cleaner if we declared this little helper bool outside the > switch so we could drop the braces. > > Will do. -Nanley > > + 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)); > > + 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)); > > > > - 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 > > mesa-dev@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev