[Mesa-dev] [PATCH v3 3/9] i965: Add and use a getter for the miptree aux buffer

Nanley Chery nanleychery at gmail.com
Mon Apr 23 18:11:24 UTC 2018


On Mon, Apr 23, 2018 at 11:01:12AM -0700, Jason Ekstrand wrote:
> Why is this useful in light of patch 4?  I don't mean to be overly critical
> but the main purpose of this helper seems to be to deal with the fact that
> we have two different fields.  If it's just one field, why the helper?
> 
> --Jason
> 
> On Wed, Apr 11, 2018 at 1:42 PM, Nanley Chery <nanleychery at gmail.com> wrote:
> 
> > Make the next patch easier to read by eliminating most of the would-be
> > duplicate field accesses now.

No worries, honest questions are welcome :) The rationale is mentioned
here in the commit message. By the way, I leave the function behind in
patch 4 because I'm currently working under the (possibly naive)
assumption that getters are a good thing in general. That said, I
haven't thought about it much and I'm not opposed to deleting it. Maybe
I shouldn't introduce two ways of getting at the same field?

-Nanley

> > ---
> >  src/mesa/drivers/dri/i965/brw_blorp.c            |  8 ++------
> >  src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 16 +---------------
> >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c    | 24
> > ++++--------------------
> >  src/mesa/drivers/dri/i965/intel_mipmap_tree.h    | 17 +++++++++++++++++
> >  4 files changed, 24 insertions(+), 41 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_blorp.c
> > b/src/mesa/drivers/dri/i965/brw_blorp.c
> > index 5dcd95e9f44..962a316c5cf 100644
> > --- a/src/mesa/drivers/dri/i965/brw_blorp.c
> > +++ b/src/mesa/drivers/dri/i965/brw_blorp.c
> > @@ -154,12 +154,6 @@ blorp_surf_for_miptree(struct brw_context *brw,
> >        .aux_usage = aux_usage,
> >     };
> >
> > -   struct intel_miptree_aux_buffer *aux_buf = NULL;
> > -   if (mt->mcs_buf)
> > -      aux_buf = mt->mcs_buf;
> > -   else if (mt->hiz_buf)
> > -      aux_buf = mt->hiz_buf;
> > -
> >     if (mt->format == MESA_FORMAT_S_UINT8 && is_render_target &&
> >         devinfo->gen <= 7)
> >        mt->r8stencil_needs_update = true;
> > @@ -174,6 +168,8 @@ blorp_surf_for_miptree(struct brw_context *brw,
> >         */
> >        surf->clear_color = mt->fast_clear_color;
> >
> > +      struct intel_miptree_aux_buffer *aux_buf =
> > +         intel_miptree_get_aux_buffer(mt);
> >        surf->aux_surf = &aux_buf->surf;
> >        surf->aux_addr = (struct blorp_address) {
> >           .reloc_flags = is_render_target ? EXEC_OBJECT_WRITE : 0,
> > diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > index 3fb101bf68b..06f739faf61 100644
> > --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> > @@ -155,21 +155,7 @@ brw_emit_surface_state(struct brw_context *brw,
> >     struct brw_bo *aux_bo = NULL;
> >     struct isl_surf *aux_surf = NULL;
> >     uint64_t aux_offset = 0;
> > -   struct intel_miptree_aux_buffer *aux_buf = NULL;
> > -   switch (aux_usage) {
> > -   case ISL_AUX_USAGE_MCS:
> > -   case ISL_AUX_USAGE_CCS_D:
> > -   case ISL_AUX_USAGE_CCS_E:
> > -      aux_buf = mt->mcs_buf;
> > -      break;
> > -
> > -   case ISL_AUX_USAGE_HIZ:
> > -      aux_buf = mt->hiz_buf;
> > -      break;
> > -
> > -   case ISL_AUX_USAGE_NONE:
> > -      break;
> > -   }
> > +   struct intel_miptree_aux_buffer *aux_buf =
> > intel_miptree_get_aux_buffer(mt);
> >
> >     if (aux_usage != ISL_AUX_USAGE_NONE) {
> >        aux_surf = &aux_buf->surf;
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > index d95128de119..ba5b02bc0aa 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > @@ -1249,8 +1249,7 @@ intel_miptree_release(struct intel_mipmap_tree **mt)
> >        brw_bo_unreference((*mt)->bo);
> >        intel_miptree_release(&(*mt)->stencil_mt);
> >        intel_miptree_release(&(*mt)->r8stencil_mt);
> > -      intel_miptree_aux_buffer_free((*mt)->hiz_buf);
> > -      intel_miptree_aux_buffer_free((*mt)->mcs_buf);
> > +      intel_miptree_aux_buffer_free(intel_miptree_get_aux_buffer(*mt));
> >        free_aux_state_map((*mt)->aux_state);
> >
> >        intel_miptree_release(&(*mt)->plane[0]);
> > @@ -2876,31 +2875,16 @@ intel_miptree_make_shareable(struct brw_context
> > *brw,
> >                                  0, INTEL_REMAINING_LAYERS,
> >                                  ISL_AUX_USAGE_NONE, false);
> >
> > -   if (mt->mcs_buf) {
> > -      intel_miptree_aux_buffer_free(mt->mcs_buf);
> > +   struct intel_miptree_aux_buffer *aux_buf =
> > intel_miptree_get_aux_buffer(mt);
> > +   if (aux_buf) {
> > +      intel_miptree_aux_buffer_free(aux_buf);
> >        mt->mcs_buf = NULL;
> > -
> > -      /* Any pending MCS/CCS operations are no longer needed. Trying to
> > -       * execute any will likely crash due to the missing aux buffer. So
> > let's
> > -       * delete all pending ops.
> > -       */
> > -      free(mt->aux_state);
> > -      mt->aux_state = NULL;
> > -      brw->ctx.NewDriverState |= BRW_NEW_AUX_STATE;
> > -   }
> > -
> > -   if (mt->hiz_buf) {
> > -      intel_miptree_aux_buffer_free(mt->hiz_buf);
> >        mt->hiz_buf = NULL;
> >
> >        for (uint32_t l = mt->first_level; l <= mt->last_level; ++l) {
> >           mt->level[l].has_hiz = false;
> >        }
> >
> > -      /* Any pending HiZ operations are no longer needed. Trying to
> > execute
> > -       * any will likely crash due to the missing aux buffer. So let's
> > delete
> > -       * all pending ops.
> > -       */
> >        free(mt->aux_state);
> >        mt->aux_state = NULL;
> >        brw->ctx.NewDriverState |= BRW_NEW_AUX_STATE;
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > index 2f754427fc5..8fe5c4add67 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > @@ -485,6 +485,23 @@ enum isl_dim_layout
> >  get_isl_dim_layout(const struct gen_device_info *devinfo,
> >                     enum isl_tiling tiling, GLenum target);
> >
> > +static inline struct intel_miptree_aux_buffer *
> > +intel_miptree_get_aux_buffer(const struct intel_mipmap_tree *mt)
> > +{
> > +   switch (mt->aux_usage) {
> > +   case ISL_AUX_USAGE_MCS:
> > +   case ISL_AUX_USAGE_CCS_D:
> > +   case ISL_AUX_USAGE_CCS_E:
> > +      return mt->mcs_buf;
> > +   case ISL_AUX_USAGE_HIZ:
> > +      return mt->hiz_buf;
> > +   case ISL_AUX_USAGE_NONE:
> > +      return NULL;
> > +   default:
> > +      unreachable("Invalid aux_usage!\n");
> > +   }
> > +}
> > +
> >  void
> >  intel_get_image_dims(struct gl_texture_image *image,
> >                       int *width, int *height, int *depth);
> > --
> > 2.16.2
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >


More information about the mesa-dev mailing list