On Tue, Jun 9, 2015 at 5:01 AM, Jose Fonseca <jfons...@vmware.com> wrote: > On 09/06/15 04:03, Rob Clark wrote: >> >> On Mon, Jun 8, 2015 at 10:50 PM, Roland Scheidegger <srol...@vmware.com> >> wrote: >>> >>> 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"... >> >> >> well, not to be rude, but I can't really be expected to fix / >> avoid-breaking code that doesn't exist (in upstream mesa).. > > > Rob, > > Of course. Nobody expects or asked you to do such thing. > > Roland's just explaining why your proposed llvmpipe workaround is not > sustainable. Breaking our closed state tracker is OK and happens all the > time FYI. What we can't have is irreparable breakage. In short, we just > need to workout a solution that doesn't leave us with our hands tied to fix > it. > > And in fact, Roland's suggestion of requiring all TGSI producers including > SVIEW dcl does not require fixing any closed-source state tracker but rather > just fix TGSI generators in Mesa to put the SVIEW.
Hmm, well there are a *lot* of things in mesa which generate tgsi (vl, gallium9, etc).. fixing them all, esp. when I don't necessarily know all the other state trackers well enough to know what they *should* be doing, didn't seem reasonable. I was hoping we could just leave SVIEW decl as optional to avoid having to fix up everything. It just means that something which does use SVIEW/SAMPLE* needs to avoid having SVIEW index's which conflict with SAMP decl indexes. Although maybe w/ the draw stuff, if it is potentially inserting SAMP/TEX* into shaders already using SVIEW/SAMPLE*, maybe it needs to take care to insert SAMP decl's with #'s that start above max SVIEW index. I'm not sure, I haven't used the aux/draw stuff yet, so I'm not terribly familiar with it. Not sure if it would help, but I could add an shader cap and for now only insert the SVIEW decl for drivers that want them. Which I think, at the moment, is just tgsi_to_nir (and currently the two tgsi_to_nir users, vc4 and freedreno, use any of the aux/draw stuff). > Anyway I really like your approach, appreciate your perseverance so far, and > want to see it through. So please just bear with us a bit more. > fair enough.. I was mostly just trying to avoid having to fix massive amounts of state trackers, etc, which I am not necessarily terribly familiar with or even have a way to test. > > Roland, would it be possible to swap the > `shader->info.base.file_max[TGSI_FILE_SAMPLER_VIEW] != -1` condition with > something else? Just temporarily, so that we can decouple things, allowing > to commit this as is, then enforce the SVIEW everywhere on a 2nd round. > > Specially because enforcing this everywhere will require some work, > sepcially on u_blit / u_mipmap_gen helpers, which need yet another key. > > > For example, what about actually using the TGSI_OPCODE_SAMPLE_* & friends > opcode count? In fact, we could go even farther, and compute the opcount > for TGSI_OPCODE_TEX & friends and ensure that they are never non-zero > simultanouesly. > hmm, possibly something I didn't understand properly.. if you never have both TEX* and SAMPLE* at same time, this sounds much easier.. Ie. just use # of opcodes instead of file_max. Could even just add a trivial thing to tgsi_scan to count up # of opcodes of each category (ie. TEX* and SAMPLE*) BR, -R > > Jose > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
