[Mesa-dev] i965: Revision of texture surface setup refactoring

[Francisco Jerez]

Hi!

Topi Pohjolainen <topi.pohjolainen at intel.com> writes:

> This series moves all the decision making of values into common
> hardware independent dispatcher while leaving the hardware specific
> logic to deal with formatting only.
>
> Curro needed a similar refactor for gen7 and gen8. However, that
> makes it a harder to apply the changes I needed that expand all the
> way to gen4. Ken helped me to notice that my refactoring can in
> fact address both relatively easily.
>
> For context, I added the patch from Curro that makes use of the
> texture surface setup logic along with a small patch making it
> compatible with the surface state refactoring found here.
>
> Curro, what do you think? I'm not too happy with reverting your
> work but overall this way it becomes cleaner, I think.
>

*Shrug*, it seems weird to me that you opted to revert my patches even
though they are closer to where you want to get at than it was before my
patches.

This is the current interface:
      void (*emit_texture_surface_state)(struct brw_context *brw,
                                         struct intel_mipmap_tree *mt,
                                         GLenum target,
                                         unsigned min_layer,
                                         unsigned max_layer,
                                         unsigned min_level,
                                         unsigned max_level,
                                         unsigned format,
                                         unsigned swizzle,
                                         uint32_t *surf_offset,
                                         bool rw, bool for_gather);

This is the old interface we both wanted to get rid of:
      void (*update_texture_surface)(struct gl_context *ctx,
                                     unsigned unit,
                                     uint32_t *surf_offset,
                                     bool for_gather);
                                           

This is the interface introduced by this series:
        void (*update_texture_surface)(struct brw_context *brw,
                                       const struct intel_mipmap_tree *mt,
                                       GLenum target, uint32_t effective_depth,
                                       uint32_t min_layer,
                                       uint32_t min_lod, uint32_t mip_count,
                                       uint32_t tex_format, int swizzle,
                                       uint32_t *surf_offset,
                                       bool for_gather);

AFAIK the only difference between your proposal and mine is the name
(IMHO emit_texture_surface_state is more consistent with the other
emit_*_surface_state hooks with similar semantics), the ordering of
arguments (and I find the ordering and naming of your "effective_depth",
"min_layer", "min_lod" and "mip_count" arguments rather asymmetric, they
are both pairs determining an interval of either layers or levels, it
doesn't make much sense to me that they are named and ordered
inconsistently in your series), the fact that you're using a min
level/layer index + count instead of half-open intervals like I did, and
the fact that you're missing an "rw" argument which is required for
ARB_shader_image_load_store support.

I fail to see why a revert is justified or desirable, and I fail to see
how your proposal will work better on Gen4, since the difference between
the two interfaces mostly cosmetic.

> CC: Francisco Jerez <currojerez at riseup.net>
>
> Francisco Jerez (1):
>   i965: Implement surface state set-up for shader images.
>
> Topi Pohjolainen (11):
>   Revert "i965/gen8: Factor out texture surface state set-up from
>     gen8_update_texture_surface()."
>   Revert "i965/gen7: Factor out texture surface state set-up from
>     gen7_update_texture_surface()."
>   i965/gen7: Check mip-tree for integer formats in surface setup
>   i965: Move texture buffer dispatch into single location
>   i965/gen8: Use miptree format in the surface setup
>   i965: Move tex miptree and format resolving into dispatcher
>   i965: Move texture swizzle resolving into dispatcher
>   i965: Refactor effective depth calculation
>   i965: Pass texture target as parameter for surface setup
>   i965: Pass slice details as parameters for surface setup
>   i965/SQUASH: Implement surface state set-up for shader images.
>
>  src/mesa/drivers/dri/i965/brw_context.h           |  24 +--
>  src/mesa/drivers/dri/i965/brw_surface_formats.c   | 112 +++++++++++++
>  src/mesa/drivers/dri/i965/brw_wm_surface_state.c  | 196 ++++++++++++++++++----
>  src/mesa/drivers/dri/i965/gen7_wm_surface_state.c |  92 +++-------
>  src/mesa/drivers/dri/i965/gen8_surface_state.c    |  96 +++--------
>  5 files changed, 325 insertions(+), 195 deletions(-)
>
> -- 
> 1.9.3
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150506/61c7c477/attachment.sig>