<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Mon, Apr 23, 2018 at 11:11 AM, Nanley Chery <<a href="mailto:nanleychery@gmail.com" target="_blank">nanleychery@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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 <<a href="mailto:nanleychery@gmail.com">nanleychery@gmail.com</a>> 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? </blockquote><div> </div><div>I'm not sure if getters are good in general or not. I think they frequently can be if the thing you're getting is complicated or if you want to do a bunch of asserting around it. I don't think they do any good if it's feasable and common (which it is) to get at it directly. In that case, I think they just provide a false sense of security. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> -Nanley <div class="HOEnZb"><div class="h5"> > > --- > > 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 > > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > </div></div></blockquote></div> </div></div>