Yes, I think nowadays everybody uses tgsi_transform_shader() to do that, so that should probably work.
Roland Am 10.06.2015 um 20:59 schrieb Rob Clark: > So, afaiu, everything that might insert a sampler is using > tgsi_transform_shader()? There aren't too many of those, and I think > I can fix them up to not violate that constraint. > > (It does occur to me that I may end up needing to fix u_blitter to > differentiate between blitting float vs int to avoid some regressions > in freedreno, since I'd no longer be using shader variants based on > bound samplers.. but I guess that is unrelated and a separate patch) > > BR, > -R > > On Wed, Jun 10, 2015 at 2:55 PM, Roland Scheidegger <srol...@vmware.com> > wrote: >> My biggest problem with that is the initial case I saw as a problem: >> draw is going to modify these shaders in some cases (aaline stage for >> example), adding its own sampler, and it doesn't know anything about >> distinguishing shaders with sampler views or without. >> The same goes for any other potential code which may modify shaders >> similarly - needs to be modified not just to always use sampler views >> but use them based on if the incoming shader already uses them or not. >> Which conceptually looks worse to me. But otherwise I agree this should >> work. >> >> Roland >> >> >> Am 10.06.2015 um 20:30 schrieb Rob Clark: >>> Hmm, at least tgsi_text_translate() doesn't appear to use tgsi_ureg.. >>> and there are still a number of users of tgsi_text_translate().. I >>> guess handling this in tgsi_ureg would avoid fixing all the tgsi_ureg >>> users, but that still leaves a lot of others. Changing them all still >>> seems to be too intrusive to me. >>> >>> (And also, I have a large collection of saved tgsi shaders that I use >>> for standalone testing of my shader compiler and don't really like the >>> idea of fixing up 700 or 800 tgsi shaders by hand :-P) >>> >>> That said, looking at the code like llvmpipe where Roland/Jose where >>> thinking we might have problems.. by making the assumption that we >>> never mix TEX* and SAMPLE* opc's, I think we can loosen the >>> restriction to: >>> >>> for TEX* instructions, the tgsi must either *not* include SVIEW, or >>> *must* include a matching SVIEW[idx] for every SAMP[idx] >>> >>> Which is a restriction that glsl_to_tgsi follows. >>> >>> If you follow this restriction, then for TEX* shaders which have >>> SVIEW's, file_max[TGSI_FILE_SAMPLER_VIEW] == >>> file_max[TGSI_FILE_SAMPLER] and file_mask[TGSI_FILE_SAMPLER_VIEW] == >>> file_mask[TGSI_FILE_SAMPLER].. so code takes a different but >>> equivalent path. And for TEX* shaders which don't have SVIEW's >>> everything continues to work as before. >>> >>> With this approach, we don't have to fix up everything to create >>> SVIEW[idx] for every SAMP[idx], as long as glsl_to_tgsi always creates >>> SVIEW[idx] for each SAMP[idx], and any other tgsi generator that later >>> adds SVIEW support for TEX* instructions follows the same pattern. >>> >>> So, tl;dr: I think really all I need to add to this patch is add blurb >>> in tgsi.rst to explain this restriction and usage of SVIEW for TEX* >>> >>> Thoughts? >>> >>> BR, >>> -R >>> >>> On Tue, Jun 9, 2015 at 1:20 PM, Marek Olšák <mar...@gmail.com> wrote: >>>> If you only want to modify TGSI and not all the users, you only have >>>> to fix tgsi_ureg. tgsi_ureg is a layer that can hide a lot of small >>>> ugly details if needed, including sampler view declarations when the >>>> users don't even know about them. >>>> >>>> Marek >>>> >>>> >>>> >>>> >>>> >>>> On Tue, Jun 9, 2015 at 6:01 PM, Rob Clark <robdcl...@gmail.com> wrote: >>>>> On Tue, Jun 9, 2015 at 9:32 AM, Roland Scheidegger <srol...@vmware.com> >>>>> wrote: >>>>>> Am 09.06.2015 um 15:00 schrieb Rob Clark: >>>>>>> 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*) >>>>>>> >>>>>> >>>>>> That doesn't tell you how many dcls (either samplers or sampler views) >>>>>> you actually have. Even with GL reusing the same texture unit with a >>>>>> different instruction is of course possible (and quite frequently done). >>>>>> Though you are right that because both sample and tex opcodes can't be >>>>>> used in the same shader, scanning the shader could determine if it's >>>>>> either tex or sample and then use the appropriate shader file if we'd >>>>>> want to keep unused views out of the shader. That may also be ok as an >>>>>> interim measure. >>>>> >>>>> yeah, that was basically my idea.. not so much caring about the count, >>>>> as much of which one is non-zero.. >>>>> >>>>> BR, >>>>> -R >>>>> >>>>>> Roland >>>>>> >>>>>> >> _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
