00:00RAOF: Again, these resolve almost immediately and without me submitting any new updates.
00:04RAOF: This is not a failure familiar to me, and I can't see anything obvious that I'm doing wrong. I'm hoping this is the sort of thing that a description of the symptoms leads someone to go “Oh, yeah, you haven't $FOOd”.
00:05DemiMarie: <RAOF> "Ok, what am I misunderstanding..." <- Where did the dma-buf come from?
00:07DemiMarie: zamundaaa: On Asahi this could also be a Mesa bug, as the driver is explicit-sync-only and implicit sync is emulated.
00:10RAOF: DemiMarie: From vkgears, over linux-dmabuf protocol.
00:10DemiMarie: RAOF: which driver?
00:11RAOF: amdgpu and i915, (across a variety of hardware generations for i915).
00:11RAOF: Others see this on non-vkgears clients, but I've not managed to reproduce the required environment.
00:12soreau: RAOF: if it's oldish hw, maybe try oldish mesa to see if it solves anything?
00:12RAOF: One of the systems (not mine) displaying this is Xe (_my_ i915 system is... gen8? so might be worth trying old mesa on).
00:13RAOF: I see this on last-gen amdgpu, too.
00:13soreau: does your amdgpu case have modifier support?
00:13karolherbst: airlied: could we install a signal handler for llvmpipe compute jobs to catch sigfaults or something?
00:14karolherbst: *segfault
00:15RAOF: soreau: It does
00:23karolherbst: soo.. I think I got cross device/cross vendor SVM working 🙃
01:53DemiMarie: karolherbst: Catching segfaults seems scary! What is your reasoning for doing that?
02:10airlied: karolherbst: don't I just want it to crash? ideally I'd get to finish my debuginfo support so it could have backtraces at least
02:14airlied: probably would want userfaultfd support
03:59jannau: DemiMarie: zamundaaa[m] wasn't visible on irc. what did you respond to?
04:00DemiMarie: jannau: The statement that they were not aware how the missing sync bug could happen.
08:33MrCooper: RAOF: do you set the IN_FENCE_FD property in the atomic commit? If not, it sounds like maybe the dma-buf producer didn't populate the implicit synchronization object correctly
08:35MrCooper: or if the client uses an explicit sync protocol, you might need to handle synchronization with KMS
10:05karolherbst: airlied: that crashes also the CL CTS binary, which is kinda a pain sometimes, but yeah...
10:06karolherbst: though I think there is some benefit of reporting an API error on that anyway
10:07karolherbst: I mean.. sucks to have launched a kernel triggering a driver bug or just memory bug, but... I think for CL at least it would make sense to report an error, because it's not like with GUIs where you'd have broken rendering, but just an error
14:06karolherbst: jenatali, gfxstrand: looking at https://gitlab.freedesktop.org/mesa/mesa/-/issues/12149 and there is a bit of a problem we are running into it. When vtn creates the nir_function it takes apart structs/arrays inside vtn_cfg_handle_prepass_instruction through glsl_type_count_function_params. In vtn_emit_kernel_entry_point_wrapper that's causing kind
14:06karolherbst: of an issue, because the vtn function type only has the struct as an argument and not the destructed type, causing a count mismatch (e.g. 2 vs 4 in the issue), but it also fails to load the nir variable, because we use the struct type and not the destructed ones.
14:06karolherbst: I have... two ideas on how to fix it, but I'm not sure which I'd like to go with...
14:06karolherbst: 1. turn those into function temp pointers and special case kernels inside vtn_cfg_handle_prepass_instruction
14:07karolherbst: 2. rework vtn_emit_kernel_entry_point_wrapper enough to make it work, which... could be a bit painful
14:07karolherbst: "entry_point->num_params" is 4 as well, so the loop kinda needs to deal with the mess there
14:07karolherbst: "b->entry_point->func->type->params" is of length 2
14:08karolherbst: but maybe it wouldn't be too bad...
14:08karolherbst: any thoughts?
14:27jenatali: karolherbst: it's not valid to pass structs by value to a kernel, I thought? They get passed by pointer?
14:27karolherbst: it is, the spirv translator just turned them into pointers
14:28karolherbst: the source of OpenCL C++ in the bug, so maybe the compiler does things differently
14:28karolherbst: at least in OpenCL C it's valid, and I don't see anything in the CL spir-v env spec to disallow it
14:28karolherbst: "An OpFunctionParameter for an OpFunction that is identified with OpEntryPoint defines an OpenCL kernel argument. Allowed types for OpenCL kernel arguments are: ... OpTypeStruct"
14:29jenatali: IMO unless it's tested by the CTS I don't think we can say it's intended to be valid
14:29jenatali: But the spec does read that way I guess. What a pain
14:30karolherbst: yeah.. but the CL spec has many gaps and that wouldn't be the first one :D
14:30karolherbst: could file a CTS bug to test this
14:30karolherbst: jenatali: I'll try to write a patch later, I think I have an idea how to deal with it in a non ugly way...
15:20karolherbst: jenatali: ... yeah soo.. I've fixed vtn, but now in rusticl I have a 2 vs 4 argument conflict, because in the spir-v it's two args, in nir it's 4 args 🙃 https://gitlab.freedesktop.org/karolherbst/mesa/-/commit/effc790e070c1e8d99a3c7ffbdbf3f720cff660d
15:21karolherbst: I think at least in the wrapper we need to keep the struct
15:21karolherbst: and then we should load members individually and translate there?
15:21karolherbst: or should we make the nir functions take derefs instead?
15:22karolherbst: totally forgot that clc uses the spirv to get the args, and rusticl just uses that information...
15:22karolherbst: I suspect it's similar for you?
15:22jenatali: The C abi can do either depending on calling convention. Just a question of what convention we want here
15:23karolherbst: well.. there is the issue that the CL CTS verifies locations of parameters, not sure if checks struct members tho...
15:23karolherbst: _but_
15:23karolherbst: I think we need to ensure that the application who passes the entire struct by value ends up matching whatever we have in nir
15:23karolherbst: so maybe a struct is the better approach, because that gives us the layout guarantees
15:24jenatali: I mean at the entrypoint level they need to look the same, loading data out of the kernel arg buffer
15:24karolherbst: the question is just, should the wrapper deconstruct the struct and translate from 1 -> N parameters, or should we change vtn?
15:24karolherbst: change vtn as in, don't deconstruct struct members inside vtn_cfg_handle_prepass_instruction
15:24karolherbst: glsl_type_count_function_params and glsl_type_add_to_function_params are the functions doing that specifically
15:25karolherbst: but I suspect that could require changes all over the place
15:25jenatali: I don't feel strongly either way about what the wrapper should do tbh. MSVC for example has a small struct optimization which can decompose a struct function arg into registers, otherwise it allocates a copy and passes a poonter
15:25jenatali: We can do either and be correct
15:26karolherbst: yeah.. nir will end up promoting the entire thing to SSA values as long as there aren't funny pointers around
15:27jenatali: I didn't know that nir already decomposed structs into members for function args
15:27karolherbst: me neither
15:27karolherbst: it also does it for arrays
15:28karolherbst: but arrays are illegal as kernel args...
15:28karolherbst: but a struct with a member is 🙃
15:28karolherbst: *an array member is I think
15:28karolherbst: huh..
15:28karolherbst: actually it's not
15:29karolherbst: but yeah anyway... maybe I just make it the wrappers problem, then I don't risk regressing vulkan or whatever else
15:29jenatali: Yeah that seems fine. It needs to load from the kernel arg buffer into the individual struct members to pass them to the main kernel which expects decomposed structs
15:30karolherbst: yeah
15:30karolherbst: I'm happy to keep CL nonsense in a single place :D
15:30jenatali: Hm how would that work if the kernel tries to take the address of the struct and do pointer math?
15:31karolherbst: I suspect the compiler is doing something else then
15:31karolherbst: like the arg isn't a pointer
15:31jenatali: Would be good to verify. I suspect we'll just end up with broken nir
15:32karolherbst: well.. can you take the address of a function parameter in spirv?
15:32jenatali: I dunno
15:32karolherbst: I think the compiler would have to copy to a temporary variable and create a pointer to it
15:32karolherbst: and then risking violating the CL spec
15:33jenatali: Would be good to confirm
15:34karolherbst: yeah.. I left a comment on the issue, because I wouldn't know how to compile clcpp code anyway
15:34jenatali: Can't Clang do it?
15:35karolherbst: I have no idea
19:05karolherbst: so yeah... it seems like the spirv was generated with something entirely different 🙃 anyway... I think it's valid so we kinda need to support it (and make the CL CTS test it)
20:11Lynne: dj-death: could you investigate what happens on intel with the ffv1_vulkan encoder I wrote?
20:11Lynne: I've eliminated everything except the uint8_t BDAs that the entropy coding system uses
20:19dviola: emersion: thanks for commenting on the bug report: https://gitlab.freedesktop.org/wlroots/wlroots/-/issues/3921 -- about the mesa bug, sounds possible... in prior tests I seem to remember that /usr/lib/dri/virtio_gpu_dri.so was being loaded in the guest, I don't understand why it isn't anymore
20:20dviola: but then again I tested the previous mesa version I was using and that also gave the issue
20:53dj-death: Lynne: maybe if you had a simpler reproducer
21:12karolherbst: mhh, for the case I want to load an entire nir_variable (it's a struct) into SSA values, do we have a helper function to to a DFS over the struct fields and load them all as ssa values?
22:39karolherbst: jenatali: https://gitlab.freedesktop.org/mesa/mesa/-/issues/12149#note_2657643 wasn't too bad. I'll give it a try tomorrow through the CTS, but I think that's a fairly straight forward change. Apparently the reason the spirv-translator is using the variable approach, is due to LLVM internal ABI reasons 🙃
22:46RAOF: MrCooper: No, I do not set IN_FENCE_FD (we don't have the synchronisation proto hooked up). Good to know that implicit sync is _meant_ to be working here!
23:11jenatali: karolherbst: Yeah that looks pretty reasonable to me