[Mesa-dev] [PATCH 1/2] tgsi: texture types
Rob Clark
robdclark at gmail.com
Wed Jun 10 13:07:03 PDT 2015
that is starting to look more attractive, mostly just because
tgsi_transform stuff is so cumbersome..
(I did start thinking about just adding type to decl's in general,
since really it would be better to have for type information for IN's
and OUT's too.. but then decided I'd probably rather spend my time on
support in mesa st to go straight from glsl to nir and bypass tgsi,
rather than going down that rabbit hole)
BR,
-R
On Wed, Jun 10, 2015 at 3:51 PM, Marek Olšák <maraeo at gmail.com> wrote:
> There is also the option of adding the sampler type to either the SAMP
> declaration or texture instructions. This will move us further away
> from adopting SVIEW, but I don't see that happening for OpenGL anyway.
>
> Marek
>
> On Wed, Jun 10, 2015 at 8:59 PM, Rob Clark <robdclark at gmail.com> wrote:
>> 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 <sroland at 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 <maraeo at 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 <robdclark at gmail.com> wrote:
>>>>>> On Tue, Jun 9, 2015 at 9:32 AM, Roland Scheidegger <sroland at vmware.com> wrote:
>>>>>>> Am 09.06.2015 um 15:00 schrieb Rob Clark:
>>>>>>>> On Tue, Jun 9, 2015 at 5:01 AM, Jose Fonseca <jfonseca at vmware.com> wrote:
>>>>>>>>> On 09/06/15 04:03, Rob Clark wrote:
>>>>>>>>>>
>>>>>>>>>> On Mon, Jun 8, 2015 at 10:50 PM, Roland Scheidegger <sroland at vmware.com>
>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>> 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"...
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> 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
>>>>>>>
>>>>>>>
>>>
More information about the mesa-dev
mailing list