<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jun 6, 2017 at 1:32 PM, Jason Ekstrand <span dir="ltr"><<a href="mailto:jason@jlekstrand.net" target="_blank">jason@jlekstrand.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="gmail-h5">On Tue, Jun 6, 2017 at 1:22 PM, Chad Versace <span dir="ltr"><<a href="mailto:chadversary@chromium.org" target="_blank">chadversary@chromium.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span>On Fri 26 May 2017, Jason Ekstrand wrote:<br>
> This enum describes all of the states that a auxiliary compressed<br>
> surface can have. All of the states as well as normative language for<br>
> referring to each of the compression operations is provided in the<br>
> truly colossal comment for the new isl_aux_state enum. There is also<br>
> a diagram showing how surfaces move between the different states.<br>
> ---<br>
> src/intel/isl/isl.h | 142 ++++++++++++++++++++++++++++++<wbr>++++++++++++++++++++++<br>
> 1 file changed, 142 insertions(+)<br>
><br>
> diff --git a/src/intel/isl/isl.h b/src/intel/isl/isl.h<br>
> index b9d8fa8..df6d3e3 100644<br>
> --- a/src/intel/isl/isl.h<br>
> +++ b/src/intel/isl/isl.h<br>
> @@ -560,6 +560,148 @@ enum isl_aux_usage {<br>
> ISL_AUX_USAGE_CCS_E,<br>
> };<br>
><br>
> +/**<br>
> + * Enum for keeping track of the state an auxiliary compressed surface.<br>
<br>
</span>This is really nice and helpful for everyone.<br>
<br>
I also learned something new from it: that a resolve on CCS_E also<br>
ambiguates the aux surface. Do you have any insight on why the hardware<br>
does that?<br>
<span><br>
> + *<br>
> + * For any given auxiliary surface compression format (HiZ, CCS, or MCS), any<br>
> + * given slice (lod + array layer) can be in one of the six states described<br>
> + * by this enum. Draw and resolve operations may cause the slice to change<br>
> + * from one state to another. The six valid states are:<br>
<br>
</span>I have one suggestion: please carefully distinguish between CCS_D and<br>
CCS_E in the documentation. In my experience, muddy thinking where the<br>
two are not cleanly distinguished leads to confused minds and confusing<br>
code.<br>
<br>
For someone who already has a firm grasp on aux state, the ambiguous<br>
term "CCS" poses no problem. That wise person automatically infers from<br>
context if "CCS" applies to CCS_D, to CCS_E, or to both. But for someone<br>
who's understanding of aux isn't as solid, the term "CCS" can lead to<br>
incorrect inferences.<br>
<br>
For example, below you say that the partial resolve "operation is only<br>
available for CCS". That's misleading. It should say "only available for<br>
CCS_E".<br>
<br>
Another benefit: It becomes possible to document that<br>
ISL_AUX_STATE_COMPRESSED_NO_CL<wbr>EAR is valid only for CCS_E and HIZ, but<br>
not valid for CCS_D and MCS.<br></blockquote><div><br></div></div></div><div>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.<br></div><span class="gmail-"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Other than the CCS_D/CCS_E distinction, the patch looks good to me. This<br>
is a really nice addition to the driver.<br></blockquote><div><br></div></span><div>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.<br></div></div></div></div></blockquote><div><br></div><div>How does this look:<br><br><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><br><br></div><div>I'll squash it in if you like it.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div><div class="gmail-h5"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
One more comment at the end...<br>
<div><div class="gmail-m_1765984308297186245h5"><br>
> + *<br>
> + * 1) Clear: In this state, each block in the auxiliary surface contains a<br>
> + * magic value that indicates that the block is in the clear state. If<br>
> + * a block is in the clear state, it's values in the primary surface are<br>
> + * ignored and the color of the samples in the block is taken either the<br>
> + * RENDER_SURFACE_STATE packet for color or 3DSTATE_CLEAR_PARAMS for<br>
> + * depth. Since neither the primary surface nor the auxiliary surface<br>
> + * contains the clear value, the surface can be cleared to a different<br>
> + * color by simply changing the clear color without modifying either<br>
> + * surface.<br>
> + *<br>
> + * 2) Compressed w/ Clear: In this state, neither the auxiliary surface<br>
> + * nor the primary surface has a complete representation of the data.<br>
> + * Instead, both surfaces must be used together or else rendering<br>
> + * corruption may occur. Depending on the auxiliary compression format<br>
> + * and the data, any given block in the primary surface may contain all,<br>
> + * some, or none of the data required to reconstruct the actual sample<br>
> + * values. Blocks may also be in the clear state (see Clear) and have<br>
> + * their value taken from outside the surface.<br>
> + *<br>
> + * 3) Compressed w/o Clear: This state is identical to the state above<br>
> + * except that no blocks are in the clear state. In this state, all of<br>
> + * the data required to reconstruct the final sample values is contained<br>
> + * in the auxiliary and primary surface and the clear value is not<br>
> + * considered.<br>
> + *<br>
> + * 4) Resolved: In this state, the primary surface contains 100% of the<br>
> + * data. The auxiliary surface is also valid so the surface can be<br>
> + * validly used with or without aux enabled. The auxiliary surface may,<br>
> + * however, contain non-trivial data and any update to the primary<br>
> + * surface with aux disabled will cause the two to get out of sync.<br>
> + *<br>
> + * 5) Pass-through: In this state, the primary surface contains 100% of the<br>
> + * data and every block in the auxiliary surface contains a magic value<br>
> + * which indicates that the auxiliary surface should be ignored and the<br>
> + * only the primary surface should be considered. Updating the primary<br>
> + * surface without aux works fine and can be done repeatedly in this<br>
> + * mode. Writing to a surface in pass-through mode with aux enabled may<br>
> + * cause the auxiliary buffer to contain non-trivial data and no longer<br>
> + * be in the pass-through state.<br>
> + *<br>
> + * 5) Aux Invalid: In this state, the primary surface contains 100% of the<br>
> + * data and the auxiliary surface is completely bogus. Any attempt to<br>
> + * use the auxiliary surface is liable to result in rendering<br>
> + * corruption. The only thing that one can do to re-enable aux once<br>
> + * this state is reached is to use an ambiguate pass to transition into<br>
> + * the pass-through state.<br>
> + *<br>
> + * Drawing with or without aux enabled may implicitly cause the surface to<br>
> + * transition between these states. There are also four types of "resolve"<br>
> + * operations which cause an explicit transition:<br>
> + *<br>
> + * 1) Fast Clear: This operation writes the magic "clear" value to the<br>
> + * auxiliary surface. This operation will safely transition any slice<br>
> + * of a surface from any state to the clear state so long as the entire<br>
> + * slice is fast cleared at once.<br>
> + *<br>
> + * 2) Full Resolve: This operation combines the auxiliary surface data<br>
> + * with the primary surface data and writes the result to the primary.<br>
> + * For CCS resolves, this operation is destructive in the sense that it<br>
> + * also sets the auxiliary surface to the pass-through mode. For HiZ,<br>
> + * it is not destructive.<br>
> + *<br>
> + * 3) Partial Resolve: This operation considers blocks which are in the<br>
> + * "clear" state and writes the clear value directly into the primary or<br>
> + * auxiliary surface. Once this operation completes, the surface is<br>
> + * still compressed but no longer references the clear color. This<br>
> + * operation is only available for CCS.<br>
> + *<br>
> + * 4) Ambiguate: This operation throws away the current auxiliary data and<br>
> + * replaces it with the magic pass-through value. If an ambiguate<br>
> + * operation is performed when the primary surface does not contain 100%<br>
> + * of the data, data will be lost. This operation is only implemented<br>
> + * in hardware for depth where it is called a HiZ resolve.<br>
> + *<br>
> + * Not all operations are valid or useful in all states. The diagram below<br>
> + * contains a complete description of the states and all valid and useful<br>
> + * transitions except clear.<br>
> + *<br>
> + * Draw w/ Aux<br>
> + * +----------+<br>
> + * | |<br>
> + * | +-------------+ Draw w/ Aux +-------------+<br>
> + * +------>| Compressed |<---------------------| Clear |<br>
> + * | w/ Clear | | |<br>
> + * +-------------+ +-------------+<br>
> + * | | |<br>
> + * Partial | | |<br>
> + * Resolve | | Full Resolve |<br>
> + * | +----------------------------<wbr>+ | Full<br>
> + * | | | Resolve<br>
> + * Draw w/ aux | | |<br>
> + * +----------+ | | |<br>
> + * | | \|/ \|/ \|/<br>
> + * | +-------------+ Full Resolve +-------------+<br>
> + * +------>| Compressed |--------------------->| Resolved |<br>
> + * | w/o Clear |<---------------------| |<br>
> + * +-------------+ Draw w/ Aux +-------------+<br>
> + * /|\ | |<br>
> + * | Draw | | Draw<br>
> + * | w/ Aux | | w/o Aux<br>
> + * | Ambiguate | |<br>
> + * | +----------------------------<wbr>+ |<br>
> + * Draw w/o Aux | | | Draw w/o Aux<br>
> + * +----------+ | | | +----------+<br>
> + * | | | \|/ \|/ | |<br>
> + * | +-------------+ Ambiguate +-------------+ |<br>
> + * +------>| Pass- |<---------------------| Aux |<------+<br>
> + * | through | | Invalid |<br>
> + * +-------------+ +-------------+<br>
> + *<br>
> + *<br>
> + * As referenced in the description of the different operations above, not all<br>
> + * auxiliary surface formats actually support all of the above modes. With<br>
> + * HiZ, for instance, does not have a partial resolve operation so the two<br>
> + * "compressed" modes are the same. With CCS, the resolve operation is<br>
> + * destructive and takes you directly to passthrough so the "resolved" state<br>
> + * doesn't really exist. However, if you consider the CCS resolve operation<br>
> + * as doing a resolve and then an ambiguate, the diagram is still accurate.<br>
> + */<br>
> +enum isl_aux_state {<br>
<br>
</div></div>One last quibble: I think the code is cleaner with the below comments<br>
removed. They don't add much, as they basically just restart the each<br>
enum's name.<br>
<div class="gmail-m_1765984308297186245HOEnZb"><div class="gmail-m_1765984308297186245h5"><br>
> + /** Describes the Clear state */<br>
> + ISL_AUX_STATE_CLEAR = 0,<br>
> + /** Describes the Compressed w/ Clear state */<br>
> + ISL_AUX_STATE_COMPRESSED_CLEA<wbr>R,<br>
> + /** Describes the Compressed w/o Clear state */<br>
> + ISL_AUX_STATE_COMPRESSED_NO_C<wbr>LEAR,<br>
> + /** Describes the Resolved state */<br>
> + ISL_AUX_STATE_RESOLVED,<br>
> + /** Describes the Pass-through state */<br>
> + ISL_AUX_STATE_PASS_THROUGH,<br>
> + /** Describes the Aux Invalid state */<br>
> + ISL_AUX_STATE_AUX_INVALID,<br>
> +};<br>
</div></div></blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div>