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

[Pohjolainen, Topi]

On Wed, May 06, 2015 at 02:56:53PM +0300, Francisco Jerez wrote:
> 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.

I'm just looking at the end result. Here we don't need to introduce new entry
to the jump table, the changes are kept to the minimum and we both get
applicable interface. I didn't really intentionally choose between the
interfaces - this was the outcome of trying to keep it as unintrusive as I
could.