02:16 luc: does anyone have any idea what those dri2_format_mappings in dri2_format_table (in dri_helpers.c:424) are based on? i wonder e.g. DRM_FORMAT_ARGB8888 maps to PIPE_FORMAT_BGRA8888_UNORM rather than PIPE_FORMAT_ARGB8888_UNORM
05:26 llyyr: luc: DRM_FORMAT describes the channel order while PIPE_FORMAT describes the memory layout
05:27 llyyr: because of little endian, ARGB is stored as BGRA in memory. or that's my understanding of it is anyway
06:26 luc: llyyr: According to dri2_format_table, does it mean that there is no such channel order like BGRA in practice？
07:15 pq: llyyr, do you mean that DRM_FORMAT_* does not describe memory layout? How so?
07:32 llyyr: pq: the drm format describes the order of the components in a native type on a little endian system, not the exact byte order in the memory
07:33 llyyr: DRM_FORMAT_ARGB8888 is stored as B, G, R, A in memory. PIPE_FORMAT describes the memory order instead, so it needs to be reversed
07:34 llyyr: or well that's my understanding of it at least, I've seen quite a few comments talk about it in mesa codebase
07:39 pq: llyyr, yes, DRM_FORMAT describes the meaning of bits in a word. Hard to describe non-8-bit formats otherwise. How is that not a memory layout though?
07:40 pq: it's not "on a little-endian system" but the format itself is little-endian regardless of the system
07:40 pq: which means the byte-by-byte memory content of a format is the same on both little-endian and big-endian systems.
07:48 llyyr: yes, the PIPE_FORMAT describes the actual byte order in memory while DRM_FORMAT is bit placement inside a native type
07:49 llyyr: from what I understand, the cpu endianess will only affect how the values are interpreted, so the PIPE_FORMAT needs to be reversed
07:50 llyyr: perhaps someone else could explain it better
07:52 sima: llyyr, drm_format is supposed to be always little endian, which means it really should describe a format irrespective of that
07:53 sima: reality is that too few people care about big endian, so it's just broken everywhere
07:53 pq: llyyr, the "reversed" only works with 8/16/32-bpc formats, because otherwise re-ordering channels cannot express the difference between little- and big-endian words. E.g. 1010102 formats or RGB565 cannot be converted by channel re-ordering.
07:54 sima: yeah there's a separate endianess bit in DRM_FORMAT_
07:56 pq: llyyr, GL "solved" the problem using both ways: GL_UNSIGNED_BYTE vs. GL_UNSIGNED_INT.
07:56 pq: INT_2_10_10_10 or whatever
07:59 jfalempe: I fixed some GBM_FORMAT <=> PIPE_FORMAT in mesa recently with https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/31707 But as there are less and less big endian systems, it tends to break often this days.
07:59 pq: DRM_FORMAT_* endianess ignoring the CPU endianess means that words need to be byte-swapped to CPU-endian before word-wide bit ops can be used to extract individual channels.
08:00 pq: jfalempe, and in the old days it tended to be broken, every component in the stack tended to just byte-swap until "it looked fine to me with this combination of sw&hw"? :-)
08:00 pq: +because
08:03 luc: sima: "reality is that too few people care about big endian, so it's just broken everywhere", is that why DRM_FORMAT_BGRA8888 is not listed in dri2_format_table (dri_helpers.c:424) in mesa codebase?
08:03 jfalempe: yes adding a byte swap is the lazy fix to make it work on big endian.
08:04 sima: luc, no idea, but essentially what pq said that we just applied byte-swapping until it looked correct and called it done
08:05 sima: but yeah ADDFB2 and drm_format stuff is most likely just busted to no end on big endian
08:05 sima: we've tried to internally in the kernel at least map ADDFB correctly to DRM_FORMAT taking endianess into account
08:05 sima: but then the virtual drivers I think get it all wrong
08:05 sima: it's a mess
08:06 sima: jfalempe, yeah my take is that big endian will just die out eventually, and until then we can just apply more hacks to keep it afloat
08:06 sima: but all pragmatism aside, I think DRM_FORMAT should be endian independent like PIPE_FORMAT
08:07 sima: for a very weak version of "should" unfortunately
08:07 jfalempe: sima: at least mutter + virtio-gpu should work on big endian (another fix I've made in mutter https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/4088)
08:08 sima: jfalempe, oh, very nice stuff :-D
08:09 sima: and yeah realistically I guess for big endian you get bgrax8888 and that's it, everything else is too hard
08:09 jfalempe: But yeah, even with that some apps have still color issues. (like when browsing with firefox, some images are inverted, depending on the source format).
08:22 jfalempe: sima: on another topic, if you have no objections, I will push this https://patchwork.freedesktop.org/patch/642839/?series=146251&rev=1
08:24 MrCooper: luc: the bigger issue in dri2_format_table is that DRM_FORMAT_*8888 are endianness-independent (as discussed above), whereas __DRI_IMAGE_FORMAT_*8888 & PIPE_FORMAT_*8888_UNORM are endianness-dependent, which means all 8888 entries in the table are wrong on big endian hosts
08:24 sima: jfalempe, I guess kmap_local is fully panic safe, including nmi and all that?
08:25 sima: otherwise I guess we'd need a kmap_try_nmi_safe or so, which bails out if it's not possible
08:25 jfalempe: sima: Yes, at least it claims to be ;)
08:25 MrCooper: luc: the users of this table should directly map between DRM_FORMAT_*8888 & PIPE_FORMAT_*8*8*8*8_UNORM (which are also endianness-independent) instead
08:26 sima: jfalempe, hm where did you find that?
08:26 sima: like without HIGHMEM they're obviously safe
08:27 sima: so in reality we really don't care much if they're not I guess
08:27 jfalempe: https://elixir.bootlin.com/linux/v6.13.6/source/include/linux/highmem.h#L68
08:27 jfalempe: * Can be invoked from any context, including interrupts.
08:27 sima: jfalempe, interrupts isn't enough
08:27 sima: nmi is _very_ nasty
08:29 sima: jfalempe, only other comment I have is that augmenting the kerneldoc for drm_scanout_buffer.pages that it cannot be allocated from @get_scanout_buffer would be good
08:29 sima: to make it clear that drivers must prepare that array as part of their ->prepare_framebuffer callback as part of normal atomic flow
08:30 sima: jfalempe, for the kmap_local_page I guess we could #ifndef HIGHMEM that for now, to get this going
08:30 sima: since it's a bit an implementation detail
08:30 sima: but it's something I think we should chase with printk people
08:30 sima: and -rt folks in general, they know how this all works really well
08:30 sima: maybe in #linux-rt
08:31 sima: I think just failing if the mapping doesn't exist yet is ok
08:31 jfalempe: sima: ok, I can add the comment for page allocation. but usually the framebuffer is allocated, so it should have an array of pages somewhere.
08:31 sima: jfalempe, these details aside I think this looks good from design pov
08:31 sima: jfalempe, yeah but not all drivers have that, at least i915-gem didn't
08:32 sima: but I think drm shmem helpers and ttm both have that, so should be ok
08:32 sima: just feels like something worth documenting, since panic context is so tricky
08:32 jfalempe: ok thanks, yes that's primary for virtio-gpu.
08:33 sima: so just add a paragraph there explaining the constraint and pointing at the prepare_framebuffer hook and what should be done there to make sure you do have a pages array that you can just assign
08:36 sima: jfalempe, I've asked over on #linux-rt, we might need a _try version of this which can fail in nmi if we're unlucky
08:37 jfalempe: I'm looking at the HIGHMEM version of kmap_local_page(), and the main difference I see is that it calls migrate_disable() and preempt_disable() https://elixir.bootlin.com/linux/v6.13.6/source/mm/highmem.c#L559
08:37 jfalempe: which should be already disabled in the panic handler.
08:41 luc: MrCooper: thanks to you all, I learned a lot.
08:43 sima: jfalempe, https://paste.debian.net/hidden/b6605b51/ something like this maybe
08:49 jfalempe: sima: Yes, I can even put that function in drm_panic.c, as it's probably the only user.
08:50 sima: jfalempe, eh drm_panic. doesn't sound right for this, we want the kmap/core people to review this
08:50 sima: but jogness over on #linux-rt isn't even sure if kmap_local is ok in hardware interrupt context
08:50 sima: eh in #printk
08:52 sima: jfalempe, oh also I think since this gets more complicated, kunit tests to drive this and make sure the book-keeping is correct would be great
08:53 sima: since this is just for very few drivers, and panic code really has to work
08:54 jfalempe: sima: I can take a look at how kunit works, I will see if I can do something about it.
08:54 sima: jfalempe, https://paste.debian.net/hidden/9e4725c9/ I think this is the guaranteed safe version, we might want to start with that?
08:54 sima: as an rfc patch set and cc: all the relevant core people
08:55 sima: and use that in your next version
08:55 jfalempe: sure, I can sent a v2 with that ;)
08:57 pq: sima, what does "endian independent" mean for a format defined as bits in a word? CPU-endian? GPU-endian? Display-chip-endian?
08:57 sima: little endian always for DRM_FORMAT
08:57 emersion: ^
08:57 sima: at least that's what's in the docs
08:58 emersion: i made a website to document pixel formats, might be useful https://pixfmtdb.emersion.fr/
08:58 MrCooper: pq: doesn't matter, the point is it doesn't depend on endianness
08:58 sima: jfalempe, for testing we might want to allow NULL entries, so that you can test the failure path in your kunit test
08:58 sima: so maybe an additiona if (!page) return NULL; in there
09:00 jfalempe: ok, that's true I assume the page array has the required size, and that all pages are not NULL.
09:02 pq: sima, that's what DRM_FORMAT already is - you sounded like you wished it was something else that what it is today.
09:02 pq: *than
09:16 MrCooper: pq: DRM formats are actually defined independently from endianness, the "little endian" language is a bit confusing
10:29 mlankhorst: sima: did you write that page of notes yet? :)
10:35 sima: mlankhorst, it's here on a piece of paper
10:36 sima: since I didn't hear anything I didn't do more, plus still backlogged on m-l traffic
10:36 sima: mlankhorst, mripard should I transcribe it as a fairly unstructured mess as a reply to the latest dmem rfc?
10:36 sima: it wasn't clear to me what I should be doing with it
10:38 mlankhorst: I think it makes sense to put it on dri-devel and with the mm ml as well
10:38 mlankhorst: Perhaps cgroup too, so we can continue the discussion
10:39 mlankhorst: What's also complicating things is eviction from VRAM will likely charge the cgroup evicting, instead of the cgroup allocating
10:42 sima: mlankhorst, yeah need to sort that out
10:42 sima: also evicting from vram means you need a memcg aware shrinker for system memory
10:42 sima: or it just all falls apart in very unfunny ways
10:43 mlankhorst: Yeah exactly
11:47 pq: MrCooper, yes, that's exactly what I'm saying.
11:49 pq: MrCooper, what I'm confused about is what sima said: "but all pragmatism aside, I think DRM_FORMAT should be endian independent like PIPE_FORMAT". Should be - so it isn't yet?
11:52 pq: as I think DRM_FORMAT are endian independent already, "endian independent" must mean something else
13:40 MrCooper: pq: "should be" can also mean "I'm pretty sure are", which I suspect was meant there
13:41 MrCooper: sima: FYI, there are endianness-dependant variants of 8888 PIPE_FORMATs (and the Mesa table which started this discussion currently uses those, which is a problem)
14:10 sima: MrCooper, pq yeah was a bit a mix of what should be and what is and what is the pragmatic approach
14:11 sima: MrCooper, I think we have the same issue with DRM_FORMAT, where there's essentially dupes&aliases due to the endian flag
14:11 sima: plus that flag doesn't make sense for a lot of formats
14:11 sima: and even less if you throw in modifiers
14:11 sima: I think defacto the endian flag has only a meaning for the 8888 formats, and maybe for the 16bit ones too
14:46 eric_engestrom: jljusten: good catch on the docs, they should have been updated when the bug was known but not fixed; I just had a look though and the fix for `backport-to:` was actually pretty simple, it's only the parser that needs to be updated: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/34117
14:46 eric_engestrom: having multiple `fixes:` tags has a similar bug though, same effect where only the first one gets parsed and the rest is ignored, but for that one it's not just the parser that needs to be updated: the way we store that information only has a single field, not a list, and making that change in a backport-able way is going to be interesting
14:46 eric_engestrom: I'm really hoping the things that's currently sinking most of my time will be over by next week, and I can have some free time to look into this (before I forget about it again ^^)
14:47 eric_engestrom: actually, for multiple `fixes:`, does it mean "needs to be backported to any branch that has any of these commits", or "... that has all of these commits"?
14:48 eric_engestrom: (any vs all, to be clear)
15:20 MrCooper: sima: my impression is DRM_FORMAT_BIG_ENDIAN was an afterthought, I doubt anyone really thought it through
15:23 sima: MrCooper, we might just have copy-pasted it from some other fourcc.h with no thought actually
15:23 sima: or yeah an "oops, maybe we should pretend to think about big endian" moment, but yeah not really more
15:27 eric_engestrom: oh btw Venemo you were asking if gitlab had a way to automatically delete old pipelines, it turns out there is: go to https://gitlab.freedesktop.org/$USERNAME/mesa/-/settings/ci_cd#js-general-pipeline-settings (replacing `$USERNAME` with your username) and set the retention you want in "Automatic pipeline cleanup"
15:27 eric_engestrom: and I have a script (https://tmp.engestrom.ch/fdo-gitlab-clean-old-pipelines.py) that I use to automatically delete pipelines for branches that no longer exist (eg. merged and auto-deleted, or manually deleted), or old pipelines when another one exists for a newer version of the same branch; it's tailored to my usage, make sure you change it the way you want to use the ci :)
15:27 eric_engestrom: (posting it here instead of DM because others might be interested in either of these)
15:29 Venemo: thank you eric_engestrom I just set the automatic cleanup to 3 months, let's hope this helps our infra save some space
15:32 Lynne: zzoon: av1 vulkan decode on anv got broken recently
15:33 Lynne: keyframes decode fine, but inter frames decode to garbage
17:19 mlankhorst: sima: How should we pin VRAM btw, should that be done through cgroups?
17:48 yrlf: hi! is there way to force a GPU reset on amdgpu?
17:48 yrlf: I'm currently testing some changes to sway's handling of those and it would be really useful to manually trigger one so I don't have to wait for the unlikely case that one occurs
17:50 pixelcluster: read (!) from /sys/kernel/debug/dri/<pci id>/amdgpu_gpu_recover
18:06 yrlf: pixelcluster: that doesn't appear to trigger glGetGraphicsResetStatusKHR(); is there another way to trigger such a GPU reset or do I need to fake this in order to test it?
18:07 pixelcluster: that's weird
18:07 pixelcluster: I'm not aware of any other method to trigger a gpu reset other than writing deliberately invalid apps
18:10 Company: I know that it works (unless it broke at some point?) because I've accidentally crashed Mutter a few times when using cat(1) too liberally
18:33 yrlf: it definitely does something, since I see a visible modeset and the brightness gets set back to the default
18:37 yrlf: I recently had an actual gpu reset due to some hang and in that case glGetGraphicsResetStatusKHR() did report that the context was lost
18:38 yrlf: i also didn't find any way in opengl to fake loss of a context, so I'll probably just fake it completely for testing and hope real context loss cases behave similarly
19:14 pac85: is that a dedicated or mobile amd gpu?
19:15 pac85: Perhaps on mobile chips only in flight contexts are marked as invalid since you don't lose "vram"?
20:14 Company: yrlf: maybe llvmpipe allows faking it somehow?