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

[Francisco Jerez]

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

> 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.

I've rebased your series on top of master.  In fact the rebased version
is a lot less churn, two of your patches (PATCH 3 and 5) that were
re-applying changes you had previously reverted become empty, and the
diffstat goes down from +131/-195 to +89/-143.

There were a number of subtle differences between the two interfaces
that weren't obvious at all by looking at the "end result", and I only
noticed while looking at the actual diff between master (without
reverts) and your branch, namely:

 - Your "mip_count" argument expects the number of mipmap levels minus
   one, instead of the actual number of mipmap levels (we already
   discussed this earlier today to some extent).

 - Your "min_lod" argument isn't the absolute starting mipmap level,
   instead it's relative to mt->first_level.  This could have bitten us
   in the future if some caller forgets to take this offset into
   account.

 - gen7_emit_texture_surface_state wasn't taking into account the
   work-around for the R32G32_FLOAT format in the texture gather case,
   instead it was relying on update_texture_surface to do it.  I have
   fixed this so we can get rid of the update_texture_surface hook.

I'll reply to this thread with the rebased series.  The changes are
split up in the exact same way you did, but I've changed some of the
arguments passed to update_texture_surface so the prototype looks more
like emit_texture_surface_state and by the end of the series the Gen7
and Gen8 implementations are simplified to a single call to
emit_texture_surface_state forwarding the arguments as-is.  I've added
an additional patch at the end that removes the update_texture_surface
hook and replaces all users with emit_texture_surface_state.

I've preserved your authorship and added R-b tags of my own to all your
commits.
-------------- 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/20150507/de21b02d/attachment-0001.sig>