00:10airlied: mripard: did you do anything different with the misc-fixes pull, patchwork didn't seem to find it
00:11airlied: wierdly the intel-gfx pw did
00:11airlied: ivyl: ^?
04:05RAOF: So, if I understand the contributing doc correctly, if I want to cherry-pick https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/4294 into the next 20.0 stable release I should branch staging/20.0, cherry-pick that commit, and then PR a merge of that onto staging/20.0?
04:20jekstrand: RAOF: That should work, yes
09:03ivyl: airlied: oh, can I have the msg id? It may have something to do with mialman deduplication too - mailing lists are recognized not basing on To/Cc but on List-Id header.
09:38airlied: ivyl: https://firstname.lastname@example.org/T/#u
09:44EdB: Hello. Is there someone willing to review a MR for Piglit regarding OpenCL test ? What is the formal process (I didn't manage to find a doc) ?
11:21ivyl: EdB: last time I have contributed there was still using a miling list, and this is described here: https://gitlab.freedesktop.org/mesa/piglit/-/blob/master/HACKING#L166
11:22ivyl: but I wouldn't be surprised if this is out of date and they take only MRs nowadays
11:25daniels: yeah, you will want to submit a merge request using gitlab instead
11:26ivyl: EdB: and you can open an issue about HACKING needing some updates :-)
11:42EdB: daniels: I already did, but not sure if it get notified if unassigned :)
11:49EdB: ivyl: Done :) https://gitlab.freedesktop.org/mesa/piglit/-/issues/37
19:19jenatali: karolherbst I'm looking at adding support for ConstantSampler in vtn for CL. It looks quite a bit different from a regular sampler. The OpSampledImage takes the result of OpConstantSampler directly, rather than a pointer coming from a variable declaration. I'm debating between keeping the NIR looking closer to the SPIR-V and letting the sampler in
19:19jenatali: the sampled_image be a constant instead of a pointer, or immediately promoting it to a variable with constant initializer. Any opinions?
19:20jenatali: jekstrand might have opinions too :)
19:21karolherbst: jenatali: well a sampler is just a bitfield of parameters in CLC, right?
19:22karolherbst: yeah... I never looked into it though :) The only thing I really looked into was passing the image reference into the kernel
19:22jenatali: I've got something functional there now, gotten kernel-passed samplers working too
19:23jekstrand: jenatali: It would make spirv_to_nir easier if it was a variable.
19:23karolherbst: and with clover I decided to just enumerate the kernel args and have a constant texture_index value
19:23jenatali: jekstrand: Cool, 'cause I'd have to promote it to a variable before trying to convert it to DXIL anyway
19:23jekstrand: But it seems like the "right" thing to do is to eventually have the NIR have a new nir_tex_src_sampler_imm
19:23jekstrand: There is hardware that really does have immediate samplers
19:23jekstrand: Like radeon
19:24karolherbst: jekstrand: what do you mean my immediate samplers?
19:24jekstrand: But I'm happy to let them write that lowering pass
19:24karolherbst: you mean having the params in the instruction encoding instead of in a sampler table?
19:24jekstrand: karolherbst: On radeon, the entire sampler state fits in a vec4 and they read it from the descriptor set and pass it through to the sample message.
19:24jekstrand: karolherbst: So you can embed that directly in the shader as an immediate.
19:24karolherbst: ahh I see
19:24karolherbst: afaik nv can't :)
19:25jekstrand: But, like I said, they can write that lowering pass
19:25jenatali: Makes sense, D3D's got expression of that but it'd still have to come all the way out to the API, so it'd still have to be a variable even if we wanted to use that feature
19:25jekstrand: Yeah, Vulkan and D3D both have a concept of "immutable samplers" that are baked into the PSO
19:25jekstrand: Which sort-of exist so that drivers can bake them into the shader binary as an optimization.
19:26jekstrand: But when going from CL to D3D or Vulkan, you really need to have it in a variable somehow.
19:26karolherbst: jenatali: what I am still wondering about is on how we really want to deal with images/samplers. I think we agree that images are opaque types and we can just iterate them like we do with GLSL and samplers are just a 32 bit value or something.
19:26karolherbst: but.. mhh
19:26karolherbst: jenatali: samplers need to be compile time evaluated constats, right?
19:27jenatali: karolherbst: What I've got right now is keeping the images/samplers as input variables, and just creating deref instructions
19:27karolherbst: or can they be runtime variable
19:27jenatali: They can be runtime variable too
19:27karolherbst: jenatali: yeah.. but that leads to an indirection not everybody wants to have
19:27karolherbst: I mean.. clover right now also does this runtime image reg thing with LLVM
19:27jenatali: karolherbst: Sure, but that's an easy lowering away
19:27karolherbst: but that's super pointless
19:28karolherbst: the first image is 0, and the second is 1 :)
19:28karolherbst: we already know that at compile time
19:28jenatali: Yeah, CLOn12's going to require that to look quite a bit different :)
19:28karolherbst: why though?
19:28jenatali: The first image might be 0, 1, and 2 depending on how it's accessed in the shader
19:29karolherbst: uhh, why would that matter?
19:29jenatali: (At the abstracted DXIL register space, it's only one actual descriptor once the D3D12 driver collapses it back down to hardware)
19:29jenatali: DXIL texture declarations need to have type encoded in them
19:29jenatali: At least at the granularity of float vs int vs uint
19:29karolherbst: ohhh, I see
19:29karolherbst: yeah.. mhh
19:29jenatali: So, keeping them derefs in vtn lets us write that lowering pass
19:30karolherbst: okay, sure
19:30karolherbst: when I was thinking of that I was never thinking about having to go back to a IR with different restrictions
19:30jenatali: Yep, makes sense
19:31karolherbst: anyway, I create the constants in the kernel wrapper anyway
19:31karolherbst: the functions itself need to deal with runtime variable values
19:31karolherbst: and then constant folding can fold the constants in
19:31karolherbst: uhm.. algebraicopt in nit :)
19:31karolherbst: always forget it's the same
19:32jenatali: Yeah, I saw that. I did things a bit differently in what we've merged into the *on12 dev tree
19:32jenatali: https://gitlab.freedesktop.org/kusma/mesa/-/merge_requests/24/diffs?commit_id=f7fd80a63ab07a2af1ca504fec79d16f394492b7 and https://gitlab.freedesktop.org/kusma/mesa/-/merge_requests/24/diffs?commit_id=581e76153c133df3d53199a9a1284661fcd1b4f2
19:33jenatali: jekstrand: Thanks for the opinion, I think promoting to a variable with const initializer is probably the easier thing to do, so I'll go for that
19:39jekstrand: jenatali: I don't know that we want to make it a "normal" const initializer though
19:39jekstrand: jenatali: We may need to do something a bit special for it
19:40jekstrand: jenatali: Mostly because, last I knew, samplers didn't have a defined layout to use for nir_constant
19:41jenatali: jekstrand: I was just planning to make it a vec3 with the 3 literals that come from the SPIR-V
19:41jekstrand: jenatali: Oh, that might work.
19:41jekstrand: jenatali: You'll likely need to do some plumbing in things like nir_[de]serialize for that though
19:42jenatali: jekstrand: Ah, thanks for the heads up. Not too familiar with that though, I'll have to look at how to actually test that
19:43jekstrand: jenatali: NIR_TEST_CLONE=1
19:43jekstrand: and NIR_TEST_SERIALIZE=1
19:43jenatali: jekstrand: Cool, thanks
20:45jenatali: jekstrand: Thanks for pointing out serialize/deserialize/clone, looks like I introduced a problem there previously
20:50jekstrand: jenatali: Whenever you're modifying core NIR data structures beyond just adding an opcode or something, it's good to test with that.
20:50jekstrand: It's really easy to miss stuff
20:50alyssa: I think I just found a bug in glmark.
20:50jenatali: jekstrand: Makes sense. I don't think I've actually done that though so I'm not sure why it broke. Pretty sure the only thing I've done is add constant indices
20:51jekstrand: Maybe you added one too many?
20:51jenatali: Shouldn't have :)
20:51jekstrand: I don't know off-hand then
20:52jenatali: Don't worry, I'll find it
20:54alyssa: krh: Does `glmark2-es2 -bfunction` work on freedreno with fp16?
20:54jenatali: jekstrand: Hm... I'm actually wondering if this is a Windows/MSVC bug rather than something I introduced. Serializing a tex whose dest type is float (128) gets deserialized as -128
20:54alyssa: (robclark: ^)
20:54jekstrand: jenatali: It's possible
20:55airlied: jekstrand: sounds like a char/unsigned char mixup
20:55airlied: jenatali: ^
20:55alyssa: krh: The shader does `gl_FragCoord.x * gl_FragCoord.y * 0.00001`. But with mediump, that first multiplication overflows the range of fp16
20:55airlied: oops wrong autocorrect
20:55jekstrand: jenatali: Yeah, it's probably the packed_tex_data bitfield. Maybe if we made dest_type "unsigned" instead of "nir_alu_type" it would work.
20:55jenatali: jekstrand: Yeah, let me give it a shot
20:56HdkR: alyssa: Maybe it was expected to only run in a 320x240 window :P
20:56jenatali: I'm guessing sampler_dims should probably also be explicitly unsigned rather than the enum type...
20:56alyssa: krh: I'm not familiar enough with mediump conversion rules to know if the bug is in mesa's precision lowering or glmark's shader. FWIW gl_FragCoord is highp.
20:57alyssa: krh: So I'd think `gl_FragCoord.x * gl_FragCoord.y * 0.00001` is parsed as `highp * highp * dontcare`, which should still be highp. But mesa is inserting f2f16's.
20:58robclark: alyssa: well, it renders differently from the gl version..
20:59alyssa: builtin_variables L1288 seems to select mediump for gl_FragCoord if the version isn't (0, 300)
20:59krh: alyssa: we also found that some benchmarks or demos were probably written and tested on desktop gpus that don't lower mediump and just are buggy when run on mobile gpus thatdo lower
21:00alyssa: krh: incredible :p
21:00krh: this one, for example: https://webglsamples.org/fishtank/fishtank.html
21:00jekstrand: Oh, no, the fish!
21:00robclark: glmark2 was written w/ gles in mind..
21:00robclark: (back in the gles2 days)
21:00krh: jekstrand: the fish are fine, don't worry, but the skybox does get enough precision
21:01jekstrand: krh: Since when did the fish tank have sharks with lasers?
21:02alyssa: thanks mr. evil
21:02krh: there are multiple webgl fish tanks
21:02alyssa: robclark: Oh, even better - gl_FragCoord is mediump in ES2 but highp in ES3, so it looks like mesa is totally correct, glmark is just broken on es2.
21:03alyssa: I do wonder if we would rather force highp gl_FragCoord everywhere tho
21:04robclark: hmm, probably not?
21:04jekstrand: alyssa: Why? It's a pretty small number most of the time
21:04jekstrand: And multiplying gl_FragCoord by itself is silly
21:05jekstrand: alyssa: Also, it's on github. Submit a merge request.
21:05alyssa: jekstrand: Well, yes, but it's already highp on desktop GL or any ES3 context, this only affects drivers which expose fp16 but only ES2, without any ES3.
21:06jekstrand: alyssa: Yes, but that doesn't mean all drivers should start adding workarounds for it.
21:07jekstrand: There's absolutely no reason to put in driver workarounds for fixable apps
21:07alyssa: okay, yeah, let's fix it.
21:07alyssa: (upstream, I mean)
21:08jekstrand: If it weren't for pesky users, I'd go for app shaming over driver workarounds for unfixable things too. :-)
21:10jenatali: jekstrand: Found another pre-existing bug in serialize/deserialize for NIR coming from CL: sampled_type in a sampler only has 2 bits, not enough to store void
21:13jekstrand: Ooh, that's bad. That doesn't work for Vulkan either.
21:15alyssa: To be sure - (gl_FragCoord.x * 0.01) * (gl_FragCoord.y * 0.01) is ok, in the sense that a driver cannot in spec commute the multiplications and const fold back to where it started?
21:16alyssa: I know GLES2 allows reassociating but..
21:17jekstrand: I'm not sure that's safe
21:17alyssa: Gah gles
21:17jekstrand: But "highp float f = gl_FragCoord.x * gl_FragCoord.y; thing = f * 0.00001;" should be.
21:18jekstrand: Orh, you might be able to re-declare gl_FragCoord highp
21:18alyssa: unfortunately, gles2 only requires (-2^16, 2^16) integer range for *highp*
21:19alyssa: so that still overflows in theory.
21:20jenatali: jekstrand: How much do we care about loading from textual NIR? I hooked up printing for sampler with const initializer, not sure if I should try to support loading too
21:20alyssa: We can load textual NIR?
21:21jenatali: alyssa: Some of it at least
21:21jekstrand: alyssa: Actually, that's probably ok for this, though, since you're working with floats.
21:21alyssa: jenatali: Neat :o
21:21jenatali: alyssa: nir_shader_from_string
21:21jekstrand: jenatali: I, personally don't care at all. I've always found that endeavour to be a bit silly.
21:22jekstrand: But someone seems to think it's useful because they put it in the tree
21:22jenatali: Yeah, some of our DXIL tests use textual NIR as input just to isolate the DXIL bits
21:23jenatali: So I've already had to fix it up at least once to add bare sampler support
21:23jenatali: I'll wait til someone complains this time I guess
21:23alyssa: jekstrand: Oh, yeah, you're right, read the worng spec column
21:23jekstrand: alyssa: What's especially great about highp integers in GLES is that you can't use int16_t for them. You can, however, use int17_t if you had such a thing. :-)
21:24jekstrand: Clearly, it was designed to run on the PDP-1
21:26alyssa: jekstrand: Does VK fix that? :p
21:27jekstrand: Yeah, I think so. All VK has is RelaxedPrecision which gives you a 16-bit thing stored in 32-bit
21:27jekstrand: None of the others exist
21:27jekstrand: And I'm not sure if you're allowed to apply it to integers or not
21:31alyssa: My github account is here somewhere...
21:35alyssa: , _, _,
21:35alyssa: Oops, sorry