Am 09.06.2015 um 04:40 schrieb Rob Clark: > On Mon, Jun 8, 2015 at 10:36 PM, Roland Scheidegger <srol...@vmware.com> > wrote: >> Am 09.06.2015 um 04:20 schrieb Rob Clark: >>> On Mon, Jun 8, 2015 at 9:40 PM, Roland Scheidegger <srol...@vmware.com> >>> wrote: >>>> Am 09.06.2015 um 03:20 schrieb Rob Clark: >>>>> On Mon, Jun 8, 2015 at 8:35 PM, Roland Scheidegger <srol...@vmware.com> >>>>> wrote: >>>>>> Am 08.06.2015 um 20:15 schrieb Rob Clark: >>>>>>> From: Rob Clark <robcl...@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 <robcl...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
