[Mesa-dev] [PATCH 1/2] tgsi: texture types
Roland Scheidegger
sroland at vmware.com
Mon Jun 8 19:50:21 PDT 2015
Am 09.06.2015 um 04:40 schrieb Rob Clark:
> On Mon, Jun 8, 2015 at 10:36 PM, Roland Scheidegger <sroland at vmware.com> wrote:
>> Am 09.06.2015 um 04:20 schrieb Rob Clark:
>>> On Mon, Jun 8, 2015 at 9:40 PM, Roland Scheidegger <sroland at vmware.com> wrote:
>>>> 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.
>>>
>>> hmm, I could squash in something like:
>>>
>>> ----
>>> diff --git a/src/gallium/drivers/llvmpipe/lp_state_fs.c
>>> b/src/gallium/drivers/llvmpipe/lp_state_fs.c
>>> index b5ce868..2878c49 100644
>>> --- a/src/gallium/drivers/llvmpipe/lp_state_fs.c
>>> +++ b/src/gallium/drivers/llvmpipe/lp_state_fs.c
>>> @@ -3059,27 +3059,11 @@ make_variant_key(struct llvmpipe_context *lp,
>>> }
>>> }
>>>
>>> - /*
>>> - * XXX If TGSI_FILE_SAMPLER_VIEW exists assume all texture opcodes
>>> - * are dx10-style? Can't really have mixed opcodes, at least not
>>> - * if we want to skip the holes here (without rescanning tgsi).
>>> - */
>>> - if (shader->info.base.file_max[TGSI_FILE_SAMPLER_VIEW] != -1) {
>>> - key->nr_sampler_views =
>>> shader->info.base.file_max[TGSI_FILE_SAMPLER_VIEW] + 1;
>>> - for(i = 0; i < key->nr_sampler_views; ++i) {
>>> - if(shader->info.base.file_mask[TGSI_FILE_SAMPLER_VIEW] & (1 << i)) {
>>> - lp_sampler_static_texture_state(&key->state[i].texture_state,
>>> -
>>> lp->sampler_views[PIPE_SHADER_FRAGMENT][i]);
>>> - }
>>> - }
>>> - }
>>> - else {
>>> - key->nr_sampler_views = key->nr_samplers;
>>> - for(i = 0; i < key->nr_sampler_views; ++i) {
>>> - if(shader->info.base.file_mask[TGSI_FILE_SAMPLER] & (1 << i)) {
>>> - lp_sampler_static_texture_state(&key->state[i].texture_state,
>>> -
>>> lp->sampler_views[PIPE_SHADER_FRAGMENT][i]);
>>> - }
>>> + key->nr_sampler_views = key->nr_samplers;
>>> + for(i = 0; i < key->nr_sampler_views; ++i) {
>>> + if(shader->info.base.file_mask[TGSI_FILE_SAMPLER] & (1 << i)) {
>>> + lp_sampler_static_texture_state(&key->state[i].texture_state,
>>> +
>>> lp->sampler_views[PIPE_SHADER_FRAGMENT][i]);
>>> }
>>> }
>>> }
>> No that won't work for d3d10 state trackers, where you typically have more
>> views than samplers (though the opposite is possible as well and you can
>> have holes in both). I think it really would look nicer if you could
>> just rely on sampler views being present.
>
> what d3d10 state tracker?
>
Make that "some non-public code which uses the sample opcodes"...
Roland
More information about the mesa-dev
mailing list