<jfalempe>
I've seen the kernel bot report, but missed your patch, maybe I need better mail filter rules on dri-devel
<javierm>
tzimmermann: that one looks good to me as well. Why sparse reports it for ARCH=i386 but not for ARCH=x86 ?
lynxeye has joined #dri-devel
<tzimmermann>
javierm, no idea. it runs on debian. maybe it is debian's way of naming things? what actually is 'i386' in this context? the kernel has long lost the ability to use i386 IIRC
<tzimmermann>
jfalempe. ha! i know. my spam filter has a habit of constantly filtering out mails from the same people
<javierm>
tzimmermann: dunno, maybe x86-32?
<javierm>
the reported .config has CONFIG_X86=y and CONFIG_X86_32=y
<javierm>
tzimmermann: yeah. They could just use ARCH=x86 and set CONFIG_X86_32=y
JohnnyonFlame has quit [Ping timeout: 480 seconds]
<javierm>
anyways, just wondered why sparse complained for x86_32 since grep'ing the code it seems that shouldn't affect the __iomem type modifier
pcercuei has joined #dri-devel
<emersion>
MrCooper: iirc you said GNOME now waits for client buffers to be ready? how do you account for amdgpu using POLLOUT instead of POLLIN?
<MrCooper>
that was fixed a while ago
<emersion>
oh so POLLIN works now?
<emersion>
that's good to know
<MrCooper>
yep, so not handling it at all :)
<emersion>
do you know in which kernel version?
<MrCooper>
not offhand, but I can look it up
<emersion>
i can grerp the commit log, but not sure what to search for
<emersion>
was it like 1 year ago? or less?
<javierm>
tzimmermann: I was today years old when I noticed the samples/ directory :)
<tzimmermann>
javierm, same here. i never noticed it before
maxzor__ has quit [Ping timeout: 480 seconds]
<MrCooper>
emersion: AFAICT it was 5.15
<emersion>
thanks!
JohnnyonFlame has joined #dri-devel
<MrCooper>
np
xypron has quit [Quit: xypron]
xypron has joined #dri-devel
xypron has quit []
xypron has joined #dri-devel
xypron has quit []
xypron has joined #dri-devel
<javierm>
tzimmermann: is great to see the benefits of the common aperture infra that allows now to remove the FBINFO_MISC_FIRMWARE flag
JohnnyonF has joined #dri-devel
xypron has quit []
<tzimmermann>
javierm, yeah. but to be honest, this patchset is something like the third hard u-turn within the rabbit hole. i was actually out to fix something entirely different
<tzimmermann>
and cleaning up first, made some sense
xypron has joined #dri-devel
<javierm>
tzimmermann: I see
<tzimmermann>
javierm, thanks for the r-bs. i'll merge it early in january
<javierm>
tzimmermann: cool. As mentioned in the ML, I think we need an ack from kvm/virt folks before for patch #9
<tzimmermann>
ok, np
<javierm>
unlikely that there will be merge conflicts but just to be good citizens
JohnnyonFlame has quit [Ping timeout: 480 seconds]
tobiasjakobi has joined #dri-devel
tobiasjakobi has quit []
kts has quit [Quit: Leaving]
<tzimmermann>
javierm, oh cool! thanks for reviewing those other patches as well
<javierm>
tzimmermann: no worries, those were easy to review :)
<javierm>
I'll also go through your "drm: Fix color-format selection in fbdev emulation" series
<javierm>
I believe that's the last one that you wanted me to review?
<tzimmermann>
javierm, there will be a v2 of the series. you may want to wait
<javierm>
tzimmermann: ah Ok. But I thought that was only to change some of the kunit tests as reported by Jose?
<tzimmermann>
after reading jose's comment, i think the tests need some fixes wrt endianess
<javierm>
tzimmermann: that's what I understood. But if is only about fixing the tests, feel free to carry my r-b's
<tzimmermann>
if you like, you can review the rest of the patchset, of course
<tzimmermann>
that shouldn't change much from the test fixes
<javierm>
yeah
<javierm>
tzimmermann: I prefer to do things in batch because I'm a terrible context switcher :)
<tzimmermann>
:)
jkrzyszt has joined #dri-devel
<javierm>
tzimmermann: I'm confused about DRM_FORMAT_BGRX8888, that's not DRM_FORMAT_HOST_XRGB8888 | DRM_FORMAT_BIG_ENDIAN, right?
<javierm>
but then you also have include/drm/drm_fourcc.h:# define DRM_FORMAT_HOST_XRGB8888 DRM_FORMAT_BGRX8888
<tzimmermann>
javierm, from what i understand, the bgrx format was there first and the big-endian flag came later
<tzimmermann>
?
<tzimmermann>
the data is the same for bgrx and xrgb|be
<javierm>
tzimmermann: right, that's what I was thinking. Hence what I meant in my first comment actually DRM_FORMAT_BGRX8888 == (DRM_FORMAT_XRGB8888 | DRM_FORMAT_BIG_ENDIAN)
kts has joined #dri-devel
<javierm>
tzimmermann: I'm trying to figure out if we can somehow simplify the else if statements by using DRM_FORMAT_HOST_XRGB8888
<javierm>
and avoid doing the drm_fb_swab() unless is necessary
<javierm>
tzimmermann: anyway, maybe that could be done as a follow-up anyways if is possible to even simplify drm_fb_blit() more
YuGiOhJCJ has quit [Quit: YuGiOhJCJ]
JohnnyonF has quit [Ping timeout: 480 seconds]
devilhorns has joined #dri-devel
srslypascal has quit [Ping timeout: 480 seconds]
ahajda_ has quit [Ping timeout: 480 seconds]
srslypascal has joined #dri-devel
ahajda_ has joined #dri-devel
<javierm>
tzimmermann: patch #8 is awesome. I always found confusing that the drivers formats array mixed both emulated and native formats
ahajda_ has quit []
bmodem1 has joined #dri-devel
bmodem has quit [Ping timeout: 480 seconds]
ahajda_ has joined #dri-devel
<tzimmermann>
thanks :)
JohnnyonFlame has joined #dri-devel
deathmist1 has joined #dri-devel
deathmist has quit [Ping timeout: 480 seconds]
jagan_ has quit [Remote host closed the connection]
apinheiro has quit [Ping timeout: 480 seconds]
maxzor has joined #dri-devel
jagan_ has joined #dri-devel
pjakobsson_ has quit [Remote host closed the connection]
JohnnyonFlame has quit [Ping timeout: 480 seconds]
kts has joined #dri-devel
cef has joined #dri-devel
djbw has quit [Read error: Connection reset by peer]
devilhorns has quit []
<lina>
I just ran into something that looks very broken in kmsro...
<lina>
I'm having issues with KWin breaking badly with whole screen capture.
<DavidHeidelberg[m]>
shadeslayer: I have branch WIP for that
<lina>
The screen resource is allocated with renderonly_create_gpu_import_for_resource, which then gets imported into the GPU
<lina>
KWin then exports that fd again and re-imports it as a texture, which then gets re-imported into the display controller with renderonly_create_gpu_import_for_resource
<lina>
But that returns the same GBM handle, since there is only one unique name for any given buffer, and there is no reference counting mechanism here
<lina>
So when it's done with the texture import, renderonly_scanout_destroy gets called and destroys the handle on the display controller side
<lina>
And then next time KWin wants to present that frame, there's no buffer any more
<emersion>
lina, drivers are responsible for ref'counting
<lynxeye>
emersion: seems lina is right, renderonly is broken in that case. Drivers do the desuplication and refcounting on the GPU BO level, but there is nothing in renderonly that would prevent a double import of the same BO on the KMS side, so you get non-refcounted handles
<lynxeye>
renderonly needs a handle hashtable for the KMS side to do proper refcounting.
<shadeslayer>
DavidHeidelberg[m]: ah thanks, I guess I'll have to wait for that :(
<DavidHeidelberg[m]>
shadeslayer: you can apply and re-run (if it isn't flake)
<lina>
lynxeye: Yeah, I'm implementing that now (following the way drivers do it)
<emersion>
renderonly needs to die :S
bgs has joined #dri-devel
<shadeslayer>
I'll have to have a look over xmas :)
<lynxeye>
emersion: welcome to the club ;)
maxzor has quit [Remote host closed the connection]
maxzor has joined #dri-devel
<danvet>
lynxeye, you need to de-dupe in libdrm/userspace
<danvet>
and it's intentional
<danvet>
so needs an import cache to do the de-duping
<lynxeye>
danvet: sure, that's what all the drivers do for the GPU side, but it's missing for the KMS side of renderonly
<danvet>
lynxeye, oh in the compositor?
<lynxeye>
danvet: No, renderonly helpers in Mesa.
<danvet>
ah
<danvet>
well guess those should get fixed or something
<danvet>
maybe the entire import cache shared as much as possible
<danvet>
tzimmermann, looked at your patch, cried in Xorg
<danvet>
lynxeye, emersion does kmsro fully own the kms side gem id space? or do we have an unfixable bug in there ...
<danvet>
this feels vaguely similar to some of the other reimport fun we've discussed in the past
<lynxeye>
danvet: I guess everyone is using this KMS handle space through the GBM device sitting on top of the renderonly screen, so should be okay to fix this at the renderonly level.
<lina>
I guess that could break if clients are doing their own BO allocs/exports directly though, but I'm not sure if anyone does that.
Company has joined #dri-devel
<tzimmermann>
danvet, cried?
<danvet>
tzimmermann, why does xorg have hard coded list of all this stuff
<tzimmermann>
danvet, because the alternative is to redesign the xserver's backend code :p i'm already glad that the workaround is easy
<tzimmermann>
but i get your point
JohnnyonFlame has joined #dri-devel
<emersion>
lina: clients specifically are forbidden to do it on a FD shared with GBM, because that breaks the ref'counting
<danvet>
lina, very quick read of all this, but the HandleToFD side seems missing?
<danvet>
you get the aliasing for any shared buffer
<danvet>
and it might come back somehow
<danvet>
biggest mistake we didn't do this in libdrm :-/
Haaninjo has joined #dri-devel
<lynxeye>
danvet: biggest mistake was to not refcount handles in the kernel. You can always add userspace caching on top to avoid the syscalls, but not having refcounts at this level was just setting up a trap for everyone.
<emersion>
indeed
<lina>
danvet: I don't think we care about HandleToFD, do we? The FD is its own reference, and then if/when it comes back either we still have the original reference or we import it again fresh.
<lina>
It doesn't matter if the BO gets destroyed while the FD still exists, because that means we aren't actually using it in the kms device since there is no direct reference.
<lina>
And then if it comes back it'll create a fresh handle when imported.
<emersion>
lina: the import de-duplicates handles
<emersion>
ah, eh
<emersion>
misread, sorry
heat_ has joined #dri-devel
<lynxeye>
lina: danvet: FD export doesn't go via renderonly. If you ask for the FD all the renderonly drivers will give you the FD exported from the GPU side BO, which is already properly deduped and refcounted, not the KMS that is handled by renderonly. IOW: it's fine.
ybogdano has joined #dri-devel
<danvet>
lina, if the gem bo of an exporterd buffer survives until you re-import, you get the same gem bo id back
<danvet>
so the exact same unrefcounted gem bo id problem
<danvet>
lynxeye, I only read one line, and that called HandleToFD on the kms_fd
<danvet>
hence why I asked
<danvet>
but if it's all good, then good :-)
tursulin has quit [Ping timeout: 480 seconds]
<danvet>
lynxeye, hm thinking about this some more, can't you still end up in a loop
<danvet>
hm, I guess I'm just confused maybe
gouchi has joined #dri-devel
<lina>
danvet: If the gem bo is alive, we have an object for it, so then on re-import we get back the same bo ID and find the object that way (and increase the refcount).
gouchi has quit []
<lina>
The path that calls HandleToFD is right after BO creation, which already allocated the BO out of the map
ybogdano has quit [Ping timeout: 480 seconds]
<danvet>
lina, yeah I'm just confused :-)
<lynxeye>
danvet: one of the primary virtues of renderonly is its ability to confuse people
<danvet>
it's pretty good at that, I admit
<lina>
^^
bmodem1 has quit [Ping timeout: 480 seconds]
<danvet>
lina, still trying to make a fool of myself ... :-) so I don't think it's a real issue, but it looks a bit strange that you're adding the array entry in renderonly_create_kms_dumb_buffer_for_resource() before the refcount and everything is set up
<danvet>
not an issue in mesa, but it freaks me out with my kernel hat somewhere :-)
Major_Biscuit has quit [Ping timeout: 480 seconds]
<danvet>
also doesn't the FDToHandle outside of the mutex potentially race with a concurrent destroy?
<danvet>
destroy&re-create of a different buffer but same gem bo_id
<Lynne>
lina: I think kwin is faulty for giving pw an fd it doesn't really have ownership over
ybogdano has joined #dri-devel
<Lynne>
no proper synchronization either, if pw maps it in userspace and starts copying while the compositor renders another frame to it
<Lynne>
IIRC pw only supports linear buffers last I took a look
<danvet>
pw some screen grabber?
frieder has quit [Remote host closed the connection]
<Lynne>
pipewire
<jadahl>
Lynne: it supports modifiers (explicit/implicit), as long as both sides of the pipewire stream does
<jadahl>
any copying happens while copyer holds on to the buffer, and it won't be overwritten until its released
<Lynne>
I believe most clients want linear buffers though, because they map them for transfer, and a badly behaving client could slow down a compositor
<jadahl>
if clients intend to mmap they shouldn't ask for dmabufs to begin with
<jadahl>
if they go via egl/vulkan/.. then don't think linear is necessary?
<Lynne>
well, the protocol started out as linear-only and host-visible
kts has quit [Remote host closed the connection]
kts has joined #dri-devel
tzimmermann has quit [Quit: Leaving]
<lina>
Lynne: This has nothing to do with PW directly, this is just KWin re-importing its own framebuffer as a texture itself (to blit it into the buffer that it then gives PW).
<lina>
Also this works with modifiers after another bugfix in Kwin, PW supports that now and I verified OBS is getting fancy compressed framebuffers ^^
<lina>
danvet: But the way util_sparse_array works is that it allocates stuff for you at a given slot when necessary... there's no way to initialize things before adding them to the array, it's always the other way around ^^
<lina>
In that particular case it's a fresh BO so we're guaranteed to have a fresh handle and fresh array slot so we're the only possible owner (note the assert that the refcnt is zero indicating a free slot), so the lock doesn't need to cover initialization later on.
<lina>
That was about the first comment, re FDToHandle you're right, there's a race (and it's probably in a bunch of drivers too!)
<danvet>
yeah that case is ok (wouldn't be in the kernel, there you need to extend the lock or inverse the sequence because kernel needs to be robust against userspace guessing ids before the syscall has finisehd)
<danvet>
it just freaked me out because kernel :-)
<lina>
Yeah, but in the kernel you probably wouldn't use that kind of data structure ^^
<danvet>
xarray is pretty much the same thing
<lina>
It's kind of funky, apparently it's inherently threadsafe? But we still need the lock to tie it to refcounting anyway I think so that doesn't buy us much.
<danvet>
just slightly different interface, as in caller allocates
<lina>
Yeah, so you can initialize it first
<danvet>
which you want anyway because GFP_ hierarchy
<danvet>
yup
<lina>
I need to write Rust abstractions for that ~next up, since I'm adding command queues to the UAPI (for compute) real soon now and that means I finally need to start keeping track of objects other than GBM BOs.
<danvet>
yeah xarray is also rcu safe in most things, (plus spinlock for when it isn't)
<danvet>
but in practice it's simpler to just use the spinlock to also protect what callers need instead of scratching your head about rcu
<danvet>
oh no xarray rust wrapper yet?
<lina>
Nope!
<lina>
(Honestly, the existing Rust support is... very barebones, even downstream. I discovered right before release that there was no module device ID alias generation support...)
<danvet>
oops
<lina>
(I think I'm the only person actually using any of this in a production driver with real users ^^;;)
<lina>
But that's probably good since it's actually getting everything usable slowly!
<danvet>
I thought the nvme driver was fairly real?
<lina>
Real yes but I'm not sure it has any users... considering tne module alias stuff.
<danvet>
that would need mod alias stuff, or did they simply use the PCI_ macros for that
<danvet>
hm I guess you can bind it through sysfs at runtime if it's just for playing around
<lina>
They probably just only ever tested it built-in, or specified a single alias manually (which was the only thing that worked)
<lina>
It works built in, the alias generation is just for module autoloading
<danvet>
sysfs has an interface to add at least pci bindings at runtime
<danvet>
oh right, once loaded it's pci_register_driver or whatever it was
<lina>
Yeah
<lina>
I'm not sure if they have the actual PCI match tables at all either, but if that isn't in Rust4Linux downstream I'm sure it's in a branch someone has somewhere (R4L has OpenFirmware match support, it's not hard to adapt that to other bus types)
<lina>
IIRC device DMA/etc isn't merged into r4l either, so yeah, there's lots of moving parts still work in progress...
<lina>
I think I'm not stepping on anyone's toes since I mostly stuck to DRM. The only core stuff I touched was some bugfixing, minor details, and general OF/Device Tree support.
<lina>
GPUs are special snowflakes enough they tend not to overlap too much with "normal" drivers...
<lina>
Oh yeah, I did add the IOMMU pgtable ops abstractions, but same thing, I doubt anyone else will need that for quite a while...
<danvet>
yeah bypassing dma-api and directly wrestling iommu is not something many drivers do
<danvet>
kvm and some vfio stuff and that's about it I think
<lina>
Oh we still hit dma-api behind the scenes I think (in DRM), it's just that there's no actual IOMMU for it to interact with (though there could be one...), and instead the GPU has its own MMU layer that just happens to reuse the IOMMU pagetable code.
<lina>
I had an interesting conversation with the Rust folks about what is safe and what is unsafe... technically, those IOMMU pagetable abstractions are 100% safe!
<Lynne>
Venemo: latest mesh shared changes break mpv, btw, I had noticed this when it was still a draft, but thought it would be fixed
<Lynne>
segfault in radv_amdgpu_get_bo_list
<lina>
Sure, you can ask it to map any random GPU VA to any random "physical" address... but all it's doing is changing some data in safely allocated memory (page tables).
<lina>
It's up to the GPU driver to actually install the base pointer into the IOMMU and make it all scary and real, and that's the only unsafe operation ^^
<lina>
Safety is quite interesting (and hard) to define when it comes to device drivers.
rsjw has joined #dri-devel
K0bin has joined #dri-devel
kts has quit [Quit: Leaving]
alatiera8 has joined #dri-devel
alatiera has quit [Ping timeout: 480 seconds]
ajax has quit [Remote host closed the connection]
djbw has joined #dri-devel
JohnnyonFlame has quit [Ping timeout: 480 seconds]
K0bin has quit [Quit: K0bin]
jkrzyszt has quit [Remote host closed the connection]
jkrzyszt has joined #dri-devel
ahajda_ has quit []
lynxeye has quit [Quit: Leaving.]
<DemiMarie>
lina: that is why one makes a type of base pointers that can always be safely inserted into the IOMMU and which always point to a well-formed page table. Functions that modify the page tables are then marked `unsafe` unless they guarantee all needed invariants.
maxzor has quit [Remote host closed the connection]
bgs has quit [Remote host closed the connection]
lemonzest has quit [Quit: WeeChat 3.6]
rasterman has quit [Quit: Gettin' stinky!]
K0bin has joined #dri-devel
Haaninjo has quit [Quit: Ex-Chat]
<Venemo>
Lynne: which mesh shader changes? and how do I reproduce the issue?
<Venemo>
Lynne: this seems to be unrelated to num_extra_cs, line 796 is: handles[i].bo_handle = extra_bo_array[i]->bo_handle; this line is not changed by that patch
<Venemo>
actually the backtrace seems to not match the code, the second frame points to an empty line
<Lynne>
you're on the commit rather than HEAD, right?
<Venemo>
I'm on 55df7ad571470562ffa3f6d71c32787f11b61b14 (latest main)
<Venemo>
which commit are you on?
<Lynne>
the bad commit
<Lynne>
I think the issue is that before, initial_preamble_cs could be NULL
<Venemo>
ok gimme a sec, will take a look at that
Akari has joined #dri-devel
<Lynne>
the fix is simple, instead of hardcoding 1 on function calls, it should be !!initial_preamble_cs
<Venemo>
I'm trying to think of a use case when it can be NULL and honestly I'm not aware of any
rsjw has quit [Ping timeout: 480 seconds]
<Lynne>
making that change in the 3 calls fixes it
<Venemo>
I don't understand how the preamble can be NULL
<Venemo>
Lynne: if you go to frame #4 radv_queue_submit_normal, is submit.initial_preamble_cs really NULL?
<Lynne>
yup
<Venemo>
Lynne: what path does the code take in radv_update_preambles?
<Venemo>
does it take the early exit?
<Lynne>
seems to take the queue->qf == RADV_QUEUE_TRANSFER path
<Venemo>
hm
<Venemo>
interesting, I wonder why it does that on sway and not on gnome
<Lynne>
hmm, well, it's got a vulkan renderer
<Lynne>
I guess a difference is that AFAIK, application's semaphores are given to the compositor now