<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">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 class=""><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_<wbr>CLEAR is valid only for CCS_E and HIZ, but<br>
not valid for CCS_D and MCS.<br></blockquote><div><br></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><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;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><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><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
One more comment at the end...<br>
<div><div class="h5"><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>
> + *Â Â Â Â Â Â Â Â Â |Â Â +----------------------------+Â Â |Â 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>
> + *Â Â Â Â Â Â Â Â Â |Â Â +----------------------------+Â Â |<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="HOEnZb"><div class="h5"><br>
> +Â Â /** Describes the Clear state */<br>
> +Â Â ISL_AUX_STATE_CLEAR = 0,<br>
> +Â Â /** Describes the Compressed w/ Clear state */<br>
> +Â Â ISL_AUX_STATE_COMPRESSED_<wbr>CLEAR,<br>
> +Â Â /** Describes the Compressed w/o Clear state */<br>
> +Â Â ISL_AUX_STATE_COMPRESSED_NO_<wbr>CLEAR,<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><br></div></div>