[Mesa-dev] [PATCH 4/8] i965/miptree: Use isl for mcs layouts
Jason Ekstrand
jason at jlekstrand.net
Wed Jun 14 17:11:32 UTC 2017
On Wed, Jun 14, 2017 at 12:13 AM, Pohjolainen, Topi <
topi.pohjolainen at 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.pohjolainen at gmail.com> wrote:
> >
> > > Reviewed-by: Jason Ekstrand <jason at jlekstrand.net>
> > > Signed-off-by: Topi Pohjolainen <topi.pohjolainen at 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 <jason at 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 at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> > >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170614/97226a39/attachment-0001.html>
More information about the mesa-dev
mailing list