[Mesa-dev] [PATCH 15/30] i965/miptree: Add new entrypoints for resolve management

[Jason Ekstrand]

On Wed, May 31, 2017 at 9:10 AM, Jason Ekstrand <jason at jlekstrand.net>
wrote:

> On Wed, May 31, 2017 at 2:15 AM, Pohjolainen, Topi <
> topi.pohjolainen at gmail.com> wrote:
>
>> On Fri, May 26, 2017 at 04:30:19PM -0700, Jason Ekstrand wrote:
>> > This commit adds a new unified interface for doing resolves.  The basic
>> > format is that, prior to any surface access such as texturing or
>> > rendering, you call intel_miptree_prepare_access.  If the surface was
>> > written, you call intel_miptree_finish_write.  These two functions take
>> > parameters which tell them whether or not auxiliary compression and fast
>> > clears are supported on the surface.  Later commits will add wrappers
>> > around these two functions for texturing, rendering, etc.
>> > ---
>> >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 156
>> +++++++++++++++++++++++++-
>> >  src/mesa/drivers/dri/i965/intel_mipmap_tree.h |  80 +++++++++++++
>> >  2 files changed, 232 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>> > index 6cd32ce..0659e75 100644
>> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
>> > @@ -2028,8 +2028,7 @@ bool
>> >  intel_miptree_all_slices_resolve_hiz(struct brw_context *brw,
>> >                                    struct intel_mipmap_tree *mt)
>> >  {
>> > -   return intel_miptree_depth_hiz_resolve(brw, mt,
>> > -                                          0, UINT32_MAX, 0, UINT32_MAX,
>> > +   return intel_miptree_depth_hiz_resolve(brw, mt, 0, UINT32_MAX, 0,
>> UINT32_MAX,
>> >                                            BLORP_HIZ_OP_HIZ_RESOLVE);
>> >  }
>> >
>> > @@ -2037,8 +2036,7 @@ bool
>> >  intel_miptree_all_slices_resolve_depth(struct brw_context *brw,
>> >                                      struct intel_mipmap_tree *mt)
>> >  {
>> > -   return intel_miptree_depth_hiz_resolve(brw, mt,
>> > -                                          0, UINT32_MAX, 0, UINT32_MAX,
>> > +   return intel_miptree_depth_hiz_resolve(brw, mt, 0, UINT32_MAX, 0,
>> UINT32_MAX,
>> >                                            BLORP_HIZ_OP_DEPTH_RESOLVE);
>> >  }
>> >
>> > @@ -2221,6 +2219,156 @@ intel_miptree_all_slices_resolve_color(struct
>> brw_context *brw,
>> >     intel_miptree_resolve_color(brw, mt, 0, UINT32_MAX, 0, UINT32_MAX,
>> flags);
>> >  }
>> >
>> > +void
>> > +intel_miptree_prepare_access(struct brw_context *brw,
>> > +                             struct intel_mipmap_tree *mt,
>> > +                             uint32_t start_level, uint32_t num_levels,
>> > +                             uint32_t start_layer, uint32_t num_layers,
>> > +                             bool aux_supported, bool
>> fast_clear_supported)
>>
>> Well, I might as well throw in my preference on using enumarated flags
>> instead
>> of booleans. In call sites, for exmaple,
>>
>> intel_miptree_prepare_access(brw, mt, start_level, num_levels,
>>                              start_layer, num_layers,
>>                              INTEL_SUPPORT_AUX |
>> INTEL_SUPPORT_FAST_CLEAR);
>>
>> is more informative than
>>
>> intel_miptree_prepare_access(brw, mt, start_level, num_levels,
>>                              start_layer, num_layers,
>>                              true, true);
>>
>
> Yeah, it is.  I'll give it a go and see what it looks like.
>
>
>> Former also allows adding more flags without changing the signature.
>>
>
> 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.
>

I gave this a go.  Take a look at the top patch on

https://cgit.freedesktop.org/~jekstrand/mesa/log/?h=wip/
i965-resolve-rework-v3

I'm not sure I actually like the result.  Here's some thoughts:

 1) It is somewhat more explicit what you support from the call.

 2) If you don't support aux at all, then it becomes 0 which is less clear
than false, false.

 3) Very few things call prepare_access directly.

 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.

 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.

 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.

 7) It's unclear what to do with finish_write since it only takes one
boolean.

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170531/9fb29a37/attachment-0001.html>