Re: [Mesa-dev] [PATCH v3 3/9] i965: Add and use a getter for the miptree aux buffer
On Mon, Apr 23, 2018 at 11:11 AM, Nanley Cherywrote: > 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 > 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? > 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. > -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 = _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 = _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,
Re: [Mesa-dev] [PATCH v3 3/9] i965: Add and use a getter for the miptree aux buffer
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 Cherywrote: > > > 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 = _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 = _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); > > -
Re: [Mesa-dev] [PATCH v3 3/9] i965: Add and use a getter for the miptree aux buffer
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 Cherywrote: > 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 = _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 = _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
Re: [Mesa-dev] [PATCH v3 3/9] i965: Add and use a getter for the miptree aux buffer
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 = _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 = _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
Re: [Mesa-dev] [PATCH v3 3/9] i965: Add and use a getter for the miptree aux buffer
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 = _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 = _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 > >
Re: [Mesa-dev] [PATCH v3 3/9] i965: Add and use a getter for the miptree aux buffer
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 = _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 = _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> 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
Re: [Mesa-dev] [PATCH v3 3/9] i965: Add and use a getter for the miptree aux buffer
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 = _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 = _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> - 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
[Mesa-dev] [PATCH v3 3/9] i965: Add and use a getter for the miptree aux buffer
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 = _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 = _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: +