<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jun 6, 2017 at 5:41 PM, Chad Versace <<a href="mailto:chad@kiwitree.net" target="_blank">chad@kiwitree.net</a>> wrote: <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5">On Tue 06 Jun 2017, Jason Ekstrand wrote: > On Tue, Jun 6, 2017 at 1:22 PM, Chad Versace <<a href="mailto:chadversary@chromium.org">chadversary@chromium.org</a>> > 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. </div></div>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. </blockquote></div> </div><div class="gmail_extra">How about this: <a href="https://cgit.freedesktop.org/~jekstrand/mesa/commit/?h=wip/i965-resolve-rework-v3&id=8478b102c99e3ec43ec687b3f4e52acb9acbd5ba">https://cgit.freedesktop.org/~jekstrand/mesa/commit/?h=wip/i965-resolve-rework-v3&id=8478b102c99e3ec43ec687b3f4e52acb9acbd5ba</a> </div></div>