On Wed, Jun 14, 2017 at 12:13 AM, Pohjolainen, Topi < topi.pohjolai...@gmail.com> wrote:
> On Tue, Jun 13, 2017 at 04:28:53PM -0700, Jason Ekstrand wrote: > > On Tue, Jun 13, 2017 at 7:53 AM, Topi Pohjolainen < > > topi.pohjolai...@gmail.com> wrote: > > > > > Reviewed-by: Jason Ekstrand <ja...@jlekstrand.net> > > > Signed-off-by: Topi Pohjolainen <topi.pohjolai...@intel.com> > > > --- > > > src/mesa/drivers/dri/i965/brw_blorp.c | 2 + > > > src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 8 +- > > > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 103 > > > ++++------------------- > > > 3 files changed, 25 insertions(+), 88 deletions(-) > > > > > > diff --git a/src/mesa/drivers/dri/i965/brw_blorp.c > > > b/src/mesa/drivers/dri/i965/brw_blorp.c > > > index b722454703..3d8f24b304 100644 > > > --- a/src/mesa/drivers/dri/i965/brw_blorp.c > > > +++ b/src/mesa/drivers/dri/i965/brw_blorp.c > > > @@ -168,6 +168,8 @@ blorp_surf_for_miptree(struct brw_context *brw, > > > struct isl_surf *aux_surf; > > > if (brw->gen == 6 && mt->hiz_buf) { > > > aux_surf = &mt->hiz_buf->aux_base.surf; > > > + } else if (mt->mcs_buf) { > > > + aux_surf = &mt->mcs_buf->surf; > > > } else { > > > aux_surf = &tmp_surfs[1]; > > > intel_miptree_get_aux_isl_surf(brw, mt, surf->aux_usage, > aux_surf); > > > 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 5aa0c7c809..d13302d03e 100644 > > > --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c > > > +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c > > > @@ -143,13 +143,17 @@ brw_emit_surface_state(struct brw_context *brw, > > > if ((mt->mcs_buf || intel_miptree_sample_with_hiz(brw, mt)) && > > > !(flags & INTEL_AUX_BUFFER_DISABLED)) { > > > aux_usage = intel_miptree_get_aux_isl_usage(brw, mt); > > > - intel_miptree_get_aux_isl_surf(brw, mt, aux_usage, > &aux_surf_s); > > > - aux_surf = &aux_surf_s; > > > > > > if (mt->mcs_buf) { > > > + aux_surf = &mt->mcs_buf->surf; > > > > > > > This doesn't work unless we're going to convert MCS and CCS at the same > > time. Again, I think just making miptree_get_aux_isl_surf do the right > > thing would help. > > Yeah, after reading your question in the summary, I realized had "do not > re-calculate for ccs either" hidden in this patch. I'm happy to add > something > to the commit message or split into another patch. I'd like to keep > miptree_get_aux_isl_surf() as it is as I'm going to drop that altogether > once > hiz is converted. > Right. It's very confusing. That said, I think I've convinced myself that it's ok though I would like something added to the commit message of this patch. Assuming all of the other trivial little comments get addressed, the series is Reviewed-by: Jason Ekstrand <ja...@jlekstrand.net> > > > > > > > + > > > + assert(mt->mcs_buf->offset == 0); > > > aux_bo = mt->mcs_buf->bo; > > > aux_offset = mt->mcs_buf->bo->offset64 + mt->mcs_buf->offset; > > > } else { > > > + intel_miptree_get_aux_isl_surf(brw, mt, aux_usage, > &aux_surf_s); > > > + aux_surf = &aux_surf_s; > > > + > > > aux_bo = mt->hiz_buf->aux_base.bo; > > > aux_offset = mt->hiz_buf->aux_base.bo->offset64; > > > } > > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > > index 0a86d3401d..f51bbd9cda 100644 > > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > > @@ -1599,56 +1599,6 @@ intel_miptree_init_mcs(struct brw_context *brw, > > > } > > > > > > static struct intel_miptree_aux_buffer * > > > -intel_mcs_miptree_buf_create(struct brw_context *brw, > > > - struct intel_mipmap_tree *mt, > > > - mesa_format format, > > > - unsigned mcs_width, > > > - unsigned mcs_height, > > > - uint32_t layout_flags) > > > -{ > > > - struct intel_miptree_aux_buffer *buf = calloc(sizeof(*buf), 1); > > > - struct intel_mipmap_tree *temp_mt; > > > - > > > - if (!buf) > > > - return NULL; > > > - > > > - /* From the Ivy Bridge PRM, Vol4 Part1 p76, "MCS Base Address": > > > - * > > > - * "The MCS surface must be stored as Tile Y." > > > - */ > > > - layout_flags |= MIPTREE_LAYOUT_TILING_Y; > > > - temp_mt = miptree_create(brw, > > > - mt->target, > > > - format, > > > - mt->first_level, > > > - mt->last_level, > > > - mcs_width, > > > - mcs_height, > > > - mt->logical_depth0, > > > - 0 /* num_samples */, > > > - layout_flags); > > > - if (!temp_mt) { > > > - free(buf); > > > - return NULL; > > > - } > > > - > > > - buf->bo = temp_mt->bo; > > > - buf->offset = temp_mt->offset; > > > - buf->size = temp_mt->total_height * temp_mt->pitch; > > > - buf->pitch = temp_mt->pitch; > > > - buf->qpitch = temp_mt->qpitch; > > > - > > > - /* Just hang on to the BO which backs the AUX buffer; the rest of > the > > > miptree > > > - * structure should go away. We use miptree create simply as a > means > > > to make > > > - * sure all the constraints for the buffer are satisfied. > > > - */ > > > - brw_bo_reference(temp_mt->bo); > > > - intel_miptree_release(&temp_mt); > > > - > > > - return buf; > > > -} > > > - > > > -static struct intel_miptree_aux_buffer * > > > intel_alloc_aux_buffer(struct brw_context *brw, > > > const char *name, > > > const struct isl_surf *main_surf, > > > @@ -1679,6 +1629,7 @@ intel_alloc_aux_buffer(struct brw_context *brw, > > > } > > > > > > assert(pitch == buf->pitch); > > > + buf->surf = *aux_surf; > Does this hunk belong in the patch which adds intel_alloc_aux_buffer? I think it does. > > > > > > return buf; > > > } > > > @@ -1692,36 +1643,6 @@ intel_miptree_alloc_mcs(struct brw_context *brw, > > > assert(mt->mcs_buf == NULL); > > > assert((mt->aux_disable & INTEL_AUX_DISABLE_MCS) == 0); > > > > > > - /* Choose the correct format for the MCS buffer. All that really > > > matters > > > - * is that we allocate the right buffer size, since we'll always be > > > - * accessing this miptree using MCS-specific hardware mechanisms, > which > > > - * infer the correct format based on num_samples. > > > - */ > > > - mesa_format format; > > > - switch (num_samples) { > > > - case 2: > > > - case 4: > > > - /* 8 bits/pixel are required for MCS data when using 4x MSAA (2 > > > bits for > > > - * each sample). > > > - */ > > > - format = MESA_FORMAT_R_UNORM8; > > > - break; > > > - case 8: > > > - /* 32 bits/pixel are required for MCS data when using 8x MSAA (3 > > > bits > > > - * for each sample, plus 8 padding bits). > > > - */ > > > - format = MESA_FORMAT_R_UINT32; > > > - break; > > > - case 16: > > > - /* 64 bits/pixel are required for MCS data when using 16x MSAA > (4 > > > bits > > > - * for each sample). > > > - */ > > > - format = MESA_FORMAT_RG_UINT32; > > > - break; > > > - default: > > > - unreachable("Unrecognized sample count in > intel_miptree_alloc_mcs"); > > > - }; > > > - > > > /* Multisampled miptrees are only supported for single level. */ > > > assert(mt->first_level == 0); > > > enum isl_aux_state **aux_state = > > > @@ -1729,12 +1650,22 @@ intel_miptree_alloc_mcs(struct brw_context > *brw, > > > if (!aux_state) > > > return false; > > > > > > - mt->mcs_buf = > > > - intel_mcs_miptree_buf_create(brw, mt, > > > - format, > > > - mt->logical_width0, > > > - mt->logical_height0, > > > - MIPTREE_LAYOUT_ACCELERATED_UP > LOAD); > > > + struct isl_surf temp_main_surf; > > > + struct isl_surf temp_mcs_surf; > > > + > > > + /* Create first an ISL presentation for the main color surface and > let > > > ISL > > > + * calculate equivalent MCS surface against it. > > > + */ > > > + intel_miptree_get_isl_surf(brw, mt, &temp_main_surf); > > > + isl_surf_get_mcs_surf(&brw->isl_dev, &temp_main_surf, > &temp_mcs_surf); > > > + > > > + assert(temp_mcs_surf.size && > > > + (temp_mcs_surf.size % temp_mcs_surf.row_pitch == 0)); > > > > > > > The better thing to do here would be to assert that isl_surf_get_mcs_surf > > returns true. > > Agreed. > > > > > > > > + > > > + const uint32_t alloc_flags = BO_ALLOC_FOR_RENDER; > > > + mt->mcs_buf = intel_alloc_aux_buffer(brw, "mcs-miptree", > > > + &temp_main_surf, > &temp_mcs_surf, > > > + alloc_flags, mt); > > > > > > > I like this new helper. :-) > > > > > > > if (!mt->mcs_buf) { > > > free(aux_state); > > > return false; > > > -- > > > 2.11.0 > > > > > > _______________________________________________ > > > 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