<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Jun 14, 2017 at 12:13 AM, Pohjolainen, Topi <<a href="mailto:topi.pohjolainen@gmail.com" target="_blank">topi.pohjolainen@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="m_-7983647689082039336HOEnZb"><div class="m_-7983647689082039336h5">On Tue, Jun 13, 2017 at 04:28:53PM -0700, Jason Ekstrand wrote: > On Tue, Jun 13, 2017 at 7:53 AM, Topi Pohjolainen < > <a href="mailto:topi.pohjolainen@gmail.com" target="_blank">topi.pohjolainen@gmail.com</a>> wrote: > > > Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>> > > Signed-off-by: Topi Pohjolainen <<a href="mailto:topi.pohjolainen@intel.com" target="_blank">topi.pohjolainen@intel.com</a>> > > --- > > 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. </div></div>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. <div><div class="m_-7983647689082039336h5"></div></div></blockquote><div> </div><div>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 </div><div>Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="m_-7983647689082039336h5"> > > > > + > > + 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-><a href="http://aux_base.bo" rel="noreferrer" target="_blank">aux_base.bo</a>; > > 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; </div></div></blockquote><div> </div><div>Does this hunk belong in the patch which adds intel_alloc_aux_buffer? I think it does. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="m_-7983647689082039336h5"> > > > > 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_UPLOAD); > > + 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. </div></div>Agreed. <div class="m_-7983647689082039336HOEnZb"><div class="m_-7983647689082039336h5"> > > > > + > > + 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 > > <a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">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>