On Tue 06 Jun 2017, Jason Ekstrand wrote:
> On Tue, Jun 6, 2017 at 1:22 PM, Chad Versace <chadvers...@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.
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
