03:20fdobridge: <gfxstrand> I have opined.
05:03fdobridge: <airlied> bleh add debug, can't reproduce pain
13:09fdobridge: <zmike.> https://media.giphy.com/media/CaiVJuZGvR8HK/giphy.gif
13:33fdobridge: <dwlsalmeida> hey guys, can the push_bo be accessed simultaneously from different threads? i.e.: can two threads record commands to the same command buffer at the same time?
13:34fdobridge: <karolherbst🐧🦀> no
13:34fdobridge: <karolherbst🐧🦀> even if the data structure it self might allow it, you'd just run into state tracking problems
13:35fdobridge: <karolherbst🐧🦀> as both threads would have to agree on the state they think is build through the commands
13:35fdobridge: <karolherbst🐧🦀> so you'd need to lock/unlock anyway
13:35fdobridge: <nanokatze> just wondering, why do you want that?
13:35fdobridge: <dwlsalmeida> I don't want that, no, I am porting this to Rust
13:35fdobridge: <karolherbst🐧🦀> and also.. you kinda need to push a block at a time
13:36fdobridge: <dwlsalmeida> but to have a mutable reference, you must ensure nobody else can touch that memory at the same time
13:36fdobridge: <karolherbst🐧🦀> yeah
13:36fdobridge: <dwlsalmeida> (from C, since C has a pointer to the BO)
13:36fdobridge: <dwlsalmeida> hence the question
13:36fdobridge: <karolherbst🐧🦀> well
13:36fdobridge: <karolherbst🐧🦀> sooo
13:36fdobridge: <karolherbst🐧🦀> just because you have a *mut pointer, doesn't mean you need a &mut ref
13:37fdobridge: <karolherbst🐧🦀> but... it also depends on where you actually lock
13:38fdobridge: <karolherbst🐧🦀> like.. to manipulate anything within a `Mutex` you don't need a `&mut` ref
13:38fdobridge: <dwlsalmeida> well, it's more convenient to have a &mut slice, because that will automatically protect us on the Rust side without any locking
13:38fdobridge: <karolherbst🐧🦀> yeah
13:38fdobridge: <karolherbst🐧🦀> but at some point you'll need to lock somewhere if you share anything
13:39fdobridge: <karolherbst🐧🦀> you can of course say that certain structs are not thread safe and hence operate on `&mut` refs
13:39fdobridge: <dwlsalmeida> to build a slice with slice_from_raw_parts_mut, it will ask us to not read or write from that memory from any other location (including C) while the slice is alive
13:39fdobridge: <karolherbst🐧🦀> right
13:40fdobridge: <karolherbst🐧🦀> but it won't magically expend on the pointer
13:40fdobridge: <karolherbst🐧🦀> *expand
13:40fdobridge: <dwlsalmeida> wdym?
13:40fdobridge: <karolherbst🐧🦀> like.. you can call `slice_from_raw_parts_mut` multiple times on the same pointer without any errors
13:41fdobridge: <dwlsalmeida> ah..
13:41fdobridge: <karolherbst🐧🦀> so before calling `slice_from_raw_parts_mut` on a pointer you kinda need to guarantee you are the only one operating on the pointer in the first place
13:42fdobridge: <karolherbst🐧🦀> normally you'd just wrap that pointer and claim unique ownership
13:43fdobridge: <karolherbst🐧🦀> and have an `unsafe` on `from_ptr` or something
13:43fdobridge: <karolherbst🐧🦀> so at `from_ptr` type you guarantee those fules
13:43fdobridge: <karolherbst🐧🦀> *rules
13:43fdobridge: <karolherbst🐧🦀> *tiem
13:43fdobridge: <karolherbst🐧🦀> *time
13:44fdobridge: <dwlsalmeida> well, I guess the only way is to also port the bo allocation and submission to Rust, but that's...a lot more code. My point being, if you allocate the BO from Rust and then immediately hand it to some `RustPush` or whatever, then yes you're guaranteed to be the only user.
13:44fdobridge: <karolherbst🐧🦀> yeah
13:44fdobridge: <karolherbst🐧🦀> it can be a trivial wrapper for now
13:45fdobridge: <karolherbst🐧🦀> potentially with a `drop` so it also gets deallocated properly
13:46fdobridge: <dwlsalmeida> > it can be a trivial wrapper for now
13:46fdobridge: <dwlsalmeida>
13:46fdobridge: <dwlsalmeida> What are you referring to? A wrapper over nv_push, or over the BOs?
13:46fdobridge: <dwlsalmeida> there's nothing to drop for `RustPush`
13:46fdobridge: <karolherbst🐧🦀> over the bo
13:46fdobridge: <dwlsalmeida> I see
13:47fdobridge: <karolherbst🐧🦀> and then the `RustPush` keeps an internal Bo
13:47fdobridge: <karolherbst🐧🦀> or so
13:47fdobridge: <karolherbst🐧🦀> and then you can just call `slice_from_raw_parts_mut` as long as you do it inside a `&mut` method of `RustPush` as at the time of creationg of `RustPush` you ensure unique ownership
13:49fdobridge: <dwlsalmeida> I noticed that there seems to be some sharing of the push buffer between different functions, i.e.:
13:49fdobridge: <dwlsalmeida>
13:49fdobridge: <dwlsalmeida> ```
13:49fdobridge: <dwlsalmeida> uint32_t *push_bo_limit;
13:49fdobridge: <dwlsalmeida> struct nv_push push;
13:49fdobridge: <dwlsalmeida> ```
13:49fdobridge: <dwlsalmeida>
13:49fdobridge: <dwlsalmeida> Then apparently `push` gets written until it's full, then it's flushed. Also there seems to be this array that is used when the allocation fails:
13:49fdobridge: <dwlsalmeida>
13:49fdobridge: <dwlsalmeida> ````
13:49fdobridge: <dwlsalmeida> /* If we ever fail to allocate a push, we use this */
13:49fdobridge: <dwlsalmeida> static uint32_t push_runout[NVK_CMD_BUFFER_MAX_PUSH];
13:49fdobridge: <dwlsalmeida> ```
13:49fdobridge: <dwlsalmeida> I wonder if this is a blocker to this new design somehow
13:52fdobridge: <karolherbst🐧🦀> right... but it might also make sense to make the bo allocation part of the push API or so? dunno, I don't know for sure how it all works at the moment. But normally it's simpler to wrap things and guarantee unique ownership, rather than to try to be careful within each method
13:55fdobridge: <dwlsalmeida> well, I guess we can only know for sure with real code at hand, so I will just keep going and see how this works out
13:56fdobridge: <dwlsalmeida> @karolherbst mind reviewing this later?
13:56fdobridge: <karolherbst🐧🦀> uhh... maybe in two weeks :blobcatnotlikethis:
13:56fdobridge: <karolherbst🐧🦀> I'm sure I won't be able to touch any code this week, nor next week
13:56fdobridge: <dwlsalmeida> that's fine, I don't expect it to be ready sooner anyways xD
13:56fdobridge: <karolherbst🐧🦀> cool
14:03fdobridge: <dwlsalmeida> one last question: does anybody have anything against boxing `RustPush`? That's so that:
14:03fdobridge: <dwlsalmeida>
14:03fdobridge: <dwlsalmeida> a) we can hide its internals from C
14:03fdobridge: <dwlsalmeida> b) we can get away with a non `repr(C)` struct, which is the case for `RustPush` because slices are not `repr(C)`
14:03fdobridge: <dwlsalmeida>
14:03fdobridge: <dwlsalmeida> So my plan is to forward declare it, at a cost of a heap allocation per instance of `RustPush`
14:09fdobridge: <zmike.> @gfxstrand so for clip/cull distance, your opinion is that compact arrays should not be used and they should be vec4s?
14:12fdobridge: <zmike.> this is massive :fullheadache: since now I'm going to have to (again) do some dumbass nir conversion to rewrite correct nir to some alternate form (vec4 array instead of float array) and then convert it back to the correct form for use with spirv
14:13fdobridge: <zmike.> which feels like some real stubbornness considering the compact array stuff is already 99% to working in the existing code
14:33fdobridge: <gfxstrand> No
14:34fdobridge: <gfxstrand> We should do compact
14:34fdobridge: <gfxstrand> I mean, compact is useful
14:34fdobridge: <gfxstrand> We shouldn't make them an array of vec4s
14:36fdobridge: <zmike.> oh
14:36fdobridge: <zmike.> I guess I misread then since none of your comments explicitly say that
14:39fdobridge: <zmike.> so to be clear, you are in favor of supporting compact clip/cull distances as array<float> ?
15:32fdobridge: <gfxstrand> I think so?
15:33fdobridge: <zmike.> tremendous
15:33fdobridge: <zmike.> are you planning to fix/merge your nir compact MR soon?
15:33fdobridge: <zmike.> it's blocking me
16:35fdobridge: <gfxstrand> My compact MR worked only because I was tossing `nir_io_semantics` to the wind and lowering directly to byte offsets in NVIDIA I/O space. With `nir_io_semantics`, everything is deeply intertwined and it's not going to be a quick fix.
17:16fdobridge: <zmike.> ?
17:16fdobridge: <zmike.> the only change that's relevant to me is fixing num_slots during io lowering
17:16fdobridge: <zmike.> which your MR certainly can do
17:31fdobridge: <zmike.> I left a comment on your MR about it, and that would be enough to unblock me
17:34fdobridge: <gfxstrand> Hrm... If all we care about is compact and not array-deref-of-vec, maybe my MR is pretty close.
17:34fdobridge: <zmike.> all I care about is fixing compact arrays, which is just clip/cull dist and tess levels
17:34fdobridge: <zmike.> and the only remaining issue there is that `num_slots` is broken during lower_io
17:34fdobridge: <zmike.> everything else works fine
17:35fdobridge: <zmike.> though it's also complicated
17:35fdobridge: <zmike.> and now that I think about it my comment is wrong
17:35fdobridge: <zmike.> because I still need the num_slots to be the full array size
17:35fdobridge: <zmike.> :stressheadache:
17:39fdobridge: <zmike.> in summary, I guess disregard
17:39fdobridge: <zmike.> https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/28465 is the actual fix that I need still
17:40fdobridge: <zmike.> and remains the only actual bug with compact arrays
17:40fdobridge: <zmike.> ...I think
18:12fdobridge: <zmike.> there, that'll do it https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/28554
20:59ad__: hi all, i am trying to fix nouveau backlight control on a lenovo pro 5 (ADA LOVELACE)
21:00ad__: workinig on upstream driver, 6.9.0-rc1
21:01ad__: using nouveau driver, screen is black
21:01ad__: i could setup a temporary patch to have backlight on, but it's just a workaround
21:02ad__: So trying to see if i can help for a better fix.
21:04ad__: From what i se in nouveau_backlight.c, gpu-controlled backlight init is implemented till NV_DEVICE_INFO_V0_AMPERE, so NV_DEVICE_INFO_V0_ADA is not considered.
21:04ad__: I tried to add it, using nv50_backlight_init, but if calls are failing
21:05ad__: Then there is a fall back to acpi backlight that does not work either (vendor or video).
21:06ad__: So if any help on this, welcome, in the meantime i study on the acpi part, that maybe is easier to be fixed.
21:11karolherbst: Lyude might be able to help out with that
21:17ad__: karolherbst: thanks, i keep an eye here
21:42Lyude: ad__ yeah if you could file a bug on gitlab with `drm.debug=0x116 log_buf_len=50M nouveau.debug=disp=debug` added to your kernel command line
21:43Lyude: Do you know if it's the backlight specifically? I know that there's some laptops that need to use nvidia-wmi-backlight
21:51ad__: Lyude: well, issue is the backlight, i could have it on with a small patch (workaround), driver works fine, tested a 3d game, perfect
21:51ad__: how can i test nvidia-wmi-backlight ?
21:52Lyude: oh - if a patch fixed it maybe it is a problem with nouveau then o:. and to be honest I'm not sure - I think if you already have it built for your kernel you should either just be able to load it, or it should load automatically
21:52Lyude: If you've got a patch though feel free to send it to the ML and I can take a look tomorrow
21:53ad__: Lyude: this is the workaround i found https://forums.lenovo.com/download/V4V7bki6mmU
21:54ad__: i am just disabling ACPI in a way that backlight stays fixed on (not tunable)
21:54Lyude: eek, disabling ACPI comes with a lot of issues though (also - that link doesn't seem to work
21:55ad__: https://pastecode.dev/s/z7vbfcg5
21:56ad__: disabled only in one file, this makes the backlight come back on after kernel boot
21:56Lyude: ....huh. that means whatever codepath you're hitting without that macro defined works. you said "not tunable", did you mean you can't change the brightness of the backlight?
21:56ad__: right
21:57ad__: the backlight, i think, stays 100% always
21:58ad__: ok, let me see if nvidia-wmi-backlight is set and if i can test it
22:24ad__: nvidia-wmi-ec-backlight seems the right module i shopuld use, since this laptop has dual switchable gpu, amd and nvidia.
22:26ad__: but as of now, it does not load, just tried acpi_backlight=nvidia_wmi_ec
22:38ad__: ok, now i can have nvidia_wmi_ec_backlight loaded, but i still see only /sys/class/backlight/acpi_video0 and black screen
23:34Lyude: ad__: nvidia_wmi_ec_backlight isn't just for all hybrid machines - I think it's specifically oens that use nvidia stuff in the backlight for some reason
23:34Lyude: iirc I think it might be for laptops with als sensors or something like that?
23:36Lyude: anyway - it sounds like it's probably not the right driver for your machine, so I guess there is something we might be doing wrong. does the nvidia blob do the right thing?
23:36ad__: Lyude: ok, looks like doesn't have any effect
23:36ad__: Lyude: yes, propertary stuff works
23:37ad__: is the backlight part code visible ?
23:37Lyude: yes it should be - was one of the reasons I was curious if it works or not
23:37ad__: yes it works fine
23:37ad__: is it the proper way to send the backlight commands through the nvif ?
23:38Lyude: It sounds like we're doing something wrong with how we interact with ACPI so I'll have to take a look at both drivers and see if I can find anything in their backlight code that we might be missing, do you think you could open up an issue on gitlab so I can keep track of this?
23:39ad__: Yes, will do. Now is 01:38 am here, so tomorrow. Anyway, from some debugging, looks like, using nv50 backlight, the outp object is set as 0x0000
23:39Lyude: and ad__ - nvif is sort of a relic, but I -think- the code we'd be looking at would either be on the nvkm side in drivers/gpu/drm/nouveau/nvkm/engine/disp/r535.c and/or in nouveau_backlight.c
23:40Lyude: in openrm (nvidia's driver) I'm not totally sure where the backlight code lives, I'd have to do a bit of searching
23:41ad__: no probs, tomorrow will fill the bug report, and or next week will continue debugging
23:41Lyude: it may also be some gsp callback we're doing incorrectly/just totally missing
23:41Lyude: and sgtm
23:42ad__: this is what i have seen till now
23:43ad__: https://snipboard.io/dHn8rc.jpg
23:43ad__: after V0_AMPERE, was missing V0_ADA
23:43ad__: i tried it
23:44ad__: but nv50_backlight init fails
23:46ad__: fails in
23:46ad__: if (nvif_outp_bl_get(&nv_encoder->outp) < 0 ||
23:46ad__: outp is 0
23:46ad__: also, that code looks changed quite recently
23:47ad__: it was if (!nvif_rd32(device, NV50_PDISP_SOR_PWM_CTL(ffs(nv_encoder->dcb->or) - 1)) ||
23:48ad__: so tomorrow will fill the bug report and continue investigating. Just let me know if it's ok to go on looking ion the gpu.controlled backlight (nv50_backlight_init)
23:48ad__: or better to look into the acpi-controlled fallback stuff
23:55Lyude: tbh maybe it's better to ignore the acpi stuff? I'm not sure honestly, it's been a while since I've looked at tis sort of thing
23:55Lyude: thinking about it more it's probably some sort of gsp call that we're missing, or possibly nto paying attention to the parameters of
23:56Lyude: (also - the nvif_rd32() stuff i realized doesn't actually get used anymore here, since we're not really interacting with the backlight registers directly on gsp)
23:56airlied: might be worth trying a distro kernel and seeing if the NVIDIA driver works with it
23:56Lyude: yeah they did already
23:57Lyude: it does seem to work so I'm probably going to be checking a bit on that the next chance I get
23:57airlied: oh if openrm works that is a bit easier to figure out maybe
23:57Lyude: it could be something we're just missing like with the link RT delay stuff
23:57airlied: what backlight path does the openrm driver use? acpi or something else?
23:58Lyude: ad__ would have to check that unless you meant generaly