<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, May 31, 2017 at 9:10 AM, 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="m_-992545125936638197gmail-h5">On Wed, May 31, 2017 at 2:15 AM, Pohjolainen, Topi <span dir="ltr"><<a href="mailto:topi.pohjolainen@gmail.com" target="_blank">topi.pohjolainen@gmail.com</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, May 26, 2017 at 04:30:19PM -0700, Jason Ekstrand wrote:<br>
</span><div><div class="m_-992545125936638197gmail-m_7327797787309328826h5">> This commit adds a new unified interface for doing resolves.  The basic<br>
> format is that, prior to any surface access such as texturing or<br>
> rendering, you call intel_miptree_prepare_access.  If the surface was<br>
> written, you call intel_miptree_finish_write.  These two functions take<br>
> parameters which tell them whether or not auxiliary compression and fast<br>
> clears are supported on the surface.  Later commits will add wrappers<br>
> around these two functions for texturing, rendering, etc.<br>
> ---<br>
>  src/mesa/drivers/dri/i965/inte<wbr>l_mipmap_tree.c | 156 +++++++++++++++++++++++++-<br>
>  src/mesa/drivers/dri/i965/inte<wbr>l_mipmap_tree.h |  80 +++++++++++++<br>
>  2 files changed, 232 insertions(+), 4 deletions(-)<br>
><br>
> diff --git a/src/mesa/drivers/dri/i965/in<wbr>tel_mipmap_tree.c b/src/mesa/drivers/dri/i965/in<wbr>tel_mipmap_tree.c<br>
> index 6cd32ce..0659e75 100644<br>
> --- a/src/mesa/drivers/dri/i965/in<wbr>tel_mipmap_tree.c<br>
> +++ b/src/mesa/drivers/dri/i965/in<wbr>tel_mipmap_tree.c<br>
> @@ -2028,8 +2028,7 @@ bool<br>
>  intel_miptree_all_slices_resol<wbr>ve_hiz(struct brw_context *brw,<br>
>                                    struct intel_mipmap_tree *mt)<br>
>  {<br>
> -   return intel_miptree_depth_hiz_resolv<wbr>e(brw, mt,<br>
> -                                          0, UINT32_MAX, 0, UINT32_MAX,<br>
> +   return intel_miptree_depth_hiz_resolv<wbr>e(brw, mt, 0, UINT32_MAX, 0, UINT32_MAX,<br>
>                                            BLORP_HIZ_OP_HIZ_RESOLVE);<br>
>  }<br>
><br>
> @@ -2037,8 +2036,7 @@ bool<br>
>  intel_miptree_all_slices_resol<wbr>ve_depth(struct brw_context *brw,<br>
>                                      struct intel_mipmap_tree *mt)<br>
>  {<br>
> -   return intel_miptree_depth_hiz_resolv<wbr>e(brw, mt,<br>
> -                                          0, UINT32_MAX, 0, UINT32_MAX,<br>
> +   return intel_miptree_depth_hiz_resolv<wbr>e(brw, mt, 0, UINT32_MAX, 0, UINT32_MAX,<br>
>                                            BLORP_HIZ_OP_DEPTH_RESOLVE);<br>
>  }<br>
><br>
> @@ -2221,6 +2219,156 @@ intel_miptree_all_slices_resol<wbr>ve_color(struct brw_context *brw,<br>
>     intel_miptree_resolve_color(b<wbr>rw, mt, 0, UINT32_MAX, 0, UINT32_MAX, flags);<br>
>  }<br>
><br>
> +void<br>
> +intel_miptree_prepare_access(<wbr>struct brw_context *brw,<br>
> +                             struct intel_mipmap_tree *mt,<br>
> +                             uint32_t start_level, uint32_t num_levels,<br>
> +                             uint32_t start_layer, uint32_t num_layers,<br>
> +                             bool aux_supported, bool fast_clear_supported)<br>
<br>
</div></div>Well, I might as well throw in my preference on using enumarated flags instead<br>
of booleans. In call sites, for exmaple,<br>
<br>
intel_miptree_prepare_access(b<wbr>rw, mt, start_level, num_levels,<br>
                             start_layer, num_layers,<br>
                             INTEL_SUPPORT_AUX | INTEL_SUPPORT_FAST_CLEAR);<br>
<br>
is more informative than<br>
<br>
intel_miptree_prepare_access(b<wbr>rw, mt, start_level, num_levels,<br>
                             start_layer, num_layers,<br>
                             true, true);<br></blockquote><div><br></div></div></div><div>Yeah, it is.  I'll give it a go and see what it looks like.<br></div><span class="m_-992545125936638197gmail-"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Former also allows adding more flags without changing the signature.<br>
</blockquote></span></div><br></div><div class="gmail_extra">I'm not sure if that's actually a feature... People tend to expand flags fields without thinking about what they're doing. :-)  But I agree that labels are nice.<br></div></div>
</blockquote></div><br></div><div class="gmail_extra">I gave this a go.  Take a look at the top patch on<br><br><a href="https://cgit.freedesktop.org/~jekstrand/mesa/log/?h=wip/i965-resolve-rework-v3" target="_blank">https://cgit.freedesktop.org/~<wbr>jekstrand/mesa/log/?h=wip/<wbr>i965-resolve-rework-v3</a><br><br></div><div class="gmail_extra">I'm not sure I actually like the result.  Here's some thoughts:<br><br></div><div class="gmail_extra"> 1) It is somewhat more explicit what you support from the call.<br><br></div><div class="gmail_extra"> 2) If you don't support aux at all, then it becomes 0 which is less clear than false, false.<br><br></div><div class="gmail_extra"> 3) Very few things call prepare_access directly.<br></div><div class="gmail_extra"><br></div><div class="gmail_extra"> 4) Things are fairly firmly structured on the two bits right now.  If someone adds a third flag without thinking carefully about it and plumbing it through to everything, they will break things.  Having them be separate parameters forces you to think about it at every stage.<br><br></div><div class="gmail_extra"> 5) I was renaming parameters in the special-case prepare_access functions so, for instance, the HiZ version had hiz_supported and fast_clear_supported.  We can't do this anymore.<br><br></div><div class="gmail_extra"> 6) It's actually a bit more awkward to construct a set of flags than to construct a pair of booleans.  Not bad, but mildly annoying.<br></div><div class="gmail_extra"><br></div><div class="gmail_extra"> 7) It's unclear what to do with finish_write since it only takes one boolean.<br><br></div><div class="gmail_extra">All in all, I'm not incredibly pleased with the change.  While I agree in general that flags are better than a pile of bools, I think the bools make more sense here given how many layers of plumbing there are.  I'm willing to change it if you really want but it doesn't seem all that compelling.<br></div></div>