[Mesa-dev] [PATCH 1/2] tgsi: texture types

[Roland Scheidegger]

Am 09.06.2015 um 03:20 schrieb Rob Clark:
> On Mon, Jun 8, 2015 at 8:35 PM, Roland Scheidegger <sroland at vmware.com> wrote:
>> Am 08.06.2015 um 20:15 schrieb Rob Clark:
>>> From: Rob Clark <robclark at freedesktop.org>
>>>
>>> Freedreno needs sampler type information to deal with int/uint textures.
>>> To accomplish this, start creating sampler-view declarations, as
>>> suggested here:
>>>
>>>  http://lists.freedesktop.org/archives/mesa-dev/2014-November/071583.html
>>>
>>> create a sampler-view with index matching the sampler, to encode the
>>> texture type (ie. SINT/UINT/FLOAT).  Ie:
>>>
>>>    DCL SVIEW[n], 2D, UINT
>>>    DCL SAMP[n]
>>>    TEX OUT[1], IN[1], SAMP[n]
>>>
>>> For tgsi texture instructions which do not take an explicit SVIEW
>>> argument, the SVIEW index is implied by the SAMP index.
>> I wonder if there should be some doc update somewhere.
>>
> 
> yeah, perhaps.. I wasn't quite sure where, tgsi.rst only talks about
> SVIEW in terms of the SAMPLE and related opc's (ie. the ones which do
> take an SVIEW arg), and the way things are with this patch, other
> drivers simply need to ignore the extra SVIEW decl's and not randomly
> assert for things to continue working as before..
> 
> hypothetically if something in tree actually created SVIEW decl
> currently, and the shader also had TEX* style instructions, it would
> have to take care to not have conflicting SVIEW's.  But afaict nothing
> in-tree creates sampler view decl's currently.
> 
>>>
>>> Signed-off-by: Rob Clark <robclark at freedesktop.org>
>>> ---
>>>  src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 29 +++++++++++++++++++++++++++++
>>>  1 file changed, 29 insertions(+)
>>>
>>> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>>> index 0e60d95..ce8f2ea 100644
>>> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>>> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>>> @@ -239,6 +239,7 @@ public:
>>>     st_src_reg sampler; /**< sampler register */
>>>     int sampler_array_size; /**< 1-based size of sampler array, 1 if not array */
>>>     int tex_target; /**< One of TEXTURE_*_INDEX */
>>> +   glsl_base_type tex_type;
>>>     GLboolean tex_shadow;
>>>
>>>     st_src_reg tex_offsets[MAX_GLSL_TEXTURE_OFFSET];
>>> @@ -345,6 +346,8 @@ public:
>>>
>>>     int num_address_regs;
>>>     int samplers_used;
>>> +   glsl_base_type sampler_types[PIPE_MAX_SAMPLERS];
>>> +   int sampler_targets[PIPE_MAX_SAMPLERS];   /**< One of TGSI_TEXTURE_* */
>>>     bool indirect_addr_consts;
>>>     int wpos_transform_const;
>>>
>>> @@ -3323,6 +3326,8 @@ glsl_to_tgsi_visitor::visit(ir_texture *ir)
>>>        assert(!"Should not get here.");
>>>     }
>>>
>>> +   inst->tex_type = ir->type->base_type;
>>> +
>>>     this->result = result_src;
>>>  }
>>>
>>> @@ -3471,6 +3476,11 @@ count_resources(glsl_to_tgsi_visitor *v, gl_program *prog)
>>>           for (int i = 0; i < inst->sampler_array_size; i++) {
>>>              v->samplers_used |= 1 << (inst->sampler.index + i);
>>>
>>> +            debug_assert(i < (int)ARRAY_SIZE(v->sampler_types));
>>> +            v->sampler_types[i] = inst->tex_type;
>>> +            v->sampler_targets[i] =
>>> +               st_translate_texture_target(inst->tex_target, inst->tex_shadow);
>>> +
>>>              if (inst->tex_shadow) {
>>>                 prog->ShadowSamplers |= 1 << (inst->sampler.index + i);
>>>              }
>>> @@ -5529,7 +5539,26 @@ st_translate_program(
>>>     /* texture samplers */
>>>     for (i = 0; i < ctx->Const.Program[MESA_SHADER_FRAGMENT].MaxTextureImageUnits; i++) {
>>>        if (program->samplers_used & (1 << i)) {
>>> +         unsigned type;
>>> +
>>>           t->samplers[i] = ureg_DECL_sampler(ureg, i);
>>> +
>>> +         switch (program->sampler_types[i]) {
>>> +         case GLSL_TYPE_INT:
>>> +            type = TGSI_RETURN_TYPE_SINT;
>>> +            break;
>>> +         case GLSL_TYPE_UINT:
>>> +            type = TGSI_RETURN_TYPE_UINT;
>>> +            break;
>>> +         case GLSL_TYPE_FLOAT:
>>> +            type = TGSI_RETURN_TYPE_FLOAT;
>>> +            break;
>>> +         default:
>>> +            unreachable("not reached");
>>> +         }
>>> +
>>> +         ureg_DECL_sampler_view( ureg, i, program->sampler_targets[i],
>>> +                                 type, type, type, type );
>>>        }
>>>     }
>>>
>>>
>>
>> This indeed seems like a consistent solution. Hopefully drivers don't
>> assert when they see sampler view dcls...
> 
> well, tgsi_to_nir did before I fixed it :-(
> 
> (mostly just because of liberal debug_assert() usage)
> 
> it's at least not something I'd land right before a release branch
> point.  But I guess the fix is easy enough (ie. remove a bogus
> assert), and 10.7 branch point is quite some time from now..
> 
>> I am also somewhat worried that this only changes the glsl-to-tgsi path,
>> not other paths. llvmpipe and draw rely on either having sview
>> declarations for all (sample) instructions or for none, and with draw
>> possibly inserting tex opcodes (for aalines etc.) on its own this could
>> probably break (these draw paths aren't used with d3d10 sample opcodes
>> as it's more or less illegal to use tex and sample opcodes in the same
>> shader, but that doesn't really matter here). I think using sview
>> declarations is the right thing to do but it probably should be illegal
>> then to NOT have them in some places.
> 
> fwiw, the way I implemented it in tgsi_to_nir (and what I'd recommend
> for other drivers) is that for the "tex style" opcodes, if there is
> not a matching SVIEW decl, assume float.  That seemed more sane than
> fixing up *all* the other state trackers..
> 

Yeah but for llvmpipe/draw it's not just the type. The problem is how
they are parsing this stuff - if there's at least one sview decl they'll
construct the static texture state based on the defined sampler views,
but if there's none (up to now for gl state tracker) they'll do this
based on the defined samplers, and for some there simply won't be any
such definitions (if draw inserted some stage). llvmpipe can handle tex
opcodes without sview dcls, it should also be able to handle them if you
have them (but I wouldn't bet on it without testing as that's definitely
going to hit some code paths never seen before), but it can't handle
having "some" sview dcls (see for example lp_state_fs.c, line 3050 and
following why it can't work). This is fixable but only with some
awkwardness (since due to d3d10 sample opcodes not having 1:1 mapping
between samplers and sampler views). Hence my proposal of making it
mandatory to have sview decls instead of having them optionally. That
way could also ditch that code which distinguishes having sviews or not.

Roland