On Fri, Apr 20, 2018 at 03:51:34PM -0700, Rafael Antognolli wrote:
> On Fri, Apr 20, 2018 at 02:38:37PM -0700, Nanley Chery wrote:
> > On Fri, Apr 20, 2018 at 09:58:38AM -0700, Rafael Antognolli wrote:
> > > Nice, I was planning to do something like this later but didn't want to
> > > include many more changes on my ongoing series. This looks great, just a
> > > little comment below.
> > >
> > > On Wed, Apr 11, 2018 at 01:42:20PM -0700, Nanley Chery wrote:
> > > > Make the next patch easier to read by eliminating most of the would-be
> > > > duplicate field accesses now.
> > > > ---
> > > > 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.
> > > > - */
> > >
> > > It looks like the above comment, and the equivalent one about HiZ, just
> > > disappear. Unless there's a reason for that, I think you should still
> > > keep them, maybe like "Any pending HiZ/MCS/CCS operations...".
> > >
> > > With that, this patch is
> > >
> > > Reviewed-by: Rafael Antognolli <rafael.antogno...@intel.com>
> > >
> >
> > Thanks for the reviews thus far! I deleted this comment because I
> > couldn't really make sense of it. After looking into it's origin (commit
> > de0b0a3a9cfd25ac5082223322002710a23da8a), I think it was placed above
> > the wrong lines of code.
> >
> > It should be placed above the for loop which sets has_hiz to false. I
> > believe it's that loop which fixes the bug in the mentioned commit
> > because that field was (and still is) checked to see if HiZ was/is
> > enabled without first checking for the presence of the hiz_buf.
>
> Ah, makes sense. I didn't fully understand that comment, but removing it
> without mentioning why looked accidental. Thanks for the explanation.
>
No problem. Thanks for bringing it up. I probably should have looked
into it more and mentioned why before I deleted it.
-Nanley
> >
> > I could keep the HiZ comment and reword it to say:
> >
> > /* Make future calls of intel_miptree_level_has_hiz() return false. */
> >
> > Thoughts?
>
> This looks like a good idea.
>
> > -Nanley
> >
> > > > - 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@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
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
