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

[Chad Versace]

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.