[Mesa-dev] [PATCH 14/30] intel/isl: Add an enum for describing auxiliary compression state

[Jason Ekstrand]

On Tue, Jun 6, 2017 at 5:41 PM, Chad Versace <chad at kiwitree.net> wrote:

> On Tue 06 Jun 2017, Jason Ekstrand wrote:
> > On Tue, Jun 6, 2017 at 1:22 PM, Chad Versace <chadversary at chromium.org>
> > wrote:
> >
> > > On Fri 26 May 2017, Jason Ekstrand wrote:
> > > > This enum describes all of the states that a auxiliary compressed
> > > > surface can have.  All of the states as well as normative language
> for
> > > > referring to each of the compression operations is provided in the
> > > > truly colossal comment for the new isl_aux_state enum.  There is also
> > > > a diagram showing how surfaces move between the different states.
> > > > ---
> > > >  src/intel/isl/isl.h | 142 ++++++++++++++++++++++++++++++
> > > ++++++++++++++++++++++
> > > >  1 file changed, 142 insertions(+)
> > > >
> > > > diff --git a/src/intel/isl/isl.h b/src/intel/isl/isl.h
> > > > index b9d8fa8..df6d3e3 100644
> > > > --- a/src/intel/isl/isl.h
> > > > +++ b/src/intel/isl/isl.h
> > > > @@ -560,6 +560,148 @@ enum isl_aux_usage {
> > > >     ISL_AUX_USAGE_CCS_E,
> > > >  };
> > > >
> > > > +/**
> > > > + * Enum for keeping track of the state an auxiliary compressed
> surface.
> > >
> > > This is really nice and helpful for everyone.
> > >
> > > I also learned something new from it: that a resolve on CCS_E also
> > > ambiguates the aux surface. Do you have any insight on why the hardware
> > > does that?
> > >
> > > > + *
> > > > + * For any given auxiliary surface compression format (HiZ, CCS, or
> > > MCS), any
> > > > + * given slice (lod + array layer) can be in one of the six states
> > > described
> > > > + * by this enum.  Draw and resolve operations may cause the slice to
> > > change
> > > > + * from one state to another.  The six valid states are:
> > >
> > > I have one suggestion: please carefully distinguish between CCS_D and
> > > CCS_E in the documentation. In my experience, muddy thinking where the
> > > two are not cleanly distinguished leads to confused minds and confusing
> > > code.
> > >
> > > For someone who already has a firm grasp on aux state, the ambiguous
> > > term "CCS" poses no problem. That wise person automatically infers from
> > > context if "CCS" applies to CCS_D, to CCS_E, or to both. But for
> someone
> > > who's understanding of aux isn't as solid, the term "CCS" can lead to
> > > incorrect inferences.
> > >
> > > For example, below you say that the partial resolve "operation is only
> > > available for CCS". That's misleading. It should say "only available
> for
> > > CCS_E".
> > >
> > > Another benefit: It becomes possible to document that
> > > ISL_AUX_STATE_COMPRESSED_NO_CLEAR is valid only for CCS_E and HIZ, but
> > > not valid for CCS_D and MCS.
> > >
> >
> > It is valid for MCS.  If you don't fast-clear but only render, then
> you're
> > in that state.  It's only invalid for CCS_D.
>
> Oops. You're right. compressed-no-clear is the "normal" state for MCS
> compression blocks.
>
> >
> >
> > > Other than the CCS_D/CCS_E distinction, the patch looks good to me.
> This
> > > is a really nice addition to the driver.
> > >
> >
> > How about a section after the auxiliary compression ops section which
> goes
> > into detail on each of the compression types and discusses which states
> are
> > valid etc.
>
> That sounds good, as long as there's not too much duplication between
> the two sections.
>

How about this:

https://cgit.freedesktop.org/~jekstrand/mesa/commit/?h=wip/i965-resolve-rework-v3&id=8478b102c99e3ec43ec687b3f4e52acb9acbd5ba
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170606/d9554437/attachment.html>