03:03 roboporo: Hello,
03:04 roboporo: is there a reason why the atomic KMS API doesn't allow for changes to the props of a cursor plane (ex. crtc_x) in an asnyc/immediate commit?
03:08 roboporo: I am trying to get tearing (async page flips) and hardware cursors working together using the atomic API, but it doesn't work even though I know it does in X11
04:29 kode54: roboporo: I already posted a question about this to a bunch of mailing lists and people, too
04:30 kode54: specifically, it's asking why the allowance check is even performed when the write is redundant
04:30 kode54: because both kwin and wlroots will spam fb_id=0 and crtc_id=0 every commit that the cursor is disabled
04:31 kode54: I also expressed that I attempted to make the atomic code only write properties that changed, dropping them from the commit if they'd be no-op writes
04:31 kode54: turns out that breaks things :D
04:31 kode54: the appropriate mailing list is indeed the dri-devel one
04:32 kode54: but I started on amd-gfx because my initial patch was against amd code
04:32 kode54: let me dig up the full thread on lore for amd-gfx
04:33 kode54: roboporo: https://lore.kernel.org/amd-gfx/DARA1U86AS72.QOIEVZWCFPYC@kode54.net/
04:33 kode54: that particular patch should seal it up for you
04:33 kode54: not a properly formatted commit patch, because I was asking for feedback, and what a proper commit message should be
04:34 kode54: this patch makes it so that only the redundancy check is applied to the majority of properties
04:34 kode54: and so that the three that we want to allow, also pass a redundancy check
04:35 kode54: the reason I don't use the function for that, is because I don't want the alert message, since a fail will hit its own message in that path
04:36 roboporo: kode54: nice I guess I got lucky you asked already heh
04:36 kode54: yeah
04:36 roboporo: how would I go about trying the patch? I haven't really messed with kernel patches
04:36 kode54: which distribution are you using?
04:36 roboporo: arch
04:36 kode54: arch packages tend to make it easy to add patches to the build
04:37 kode54: pick your kernel, copy the patch file into the package root, add its filename to the sources list, updpkgsums after you have all the other sources
04:37 kode54: they usually have a loop in the prepare function to patch -Np1 anything named *.patch in the sources list
04:37 roboporo: in the PKGBUILD right?
04:38 kode54: yes
04:38 roboporo: ok nice I'll try it then
04:39 roboporo: hmmm wait now that I look at it wouldn't the crtc_x property just hit the drm_atomic_check_prop_changes path and fail?
04:40 kode54: that's a plane update
04:40 kode54: this function has to do with plane properties
04:40 kode54: crtc_x would hit that top function
04:41 kode54: hmm
04:41 kode54: weird, maybe it's spamming all plane properties even though it's a disabled cursor?
04:41 kode54: I need to check tearing on KDE again
04:41 roboporo: ?
04:41 roboporo: no no I'm trying to use the hw cursor at the same time as the async page flip
04:42 roboporo: but only moving it around using the crtc_x and crtc_y properties of the cursor plane
04:42 kode54: yeah, cursor doesn't support async intentionally on atomic
04:43 roboporo: from what I heard it just hadn't been implemented yet no?
04:43 kode54: someone slyly suggested using the legacy path just to poke the cursor, much to the hatred of atomic devs
04:43 kode54: as in, using the legacy function in the middle of an atomic display
04:44 roboporo: that's so cursed lol
04:44 roboporo: both apis at the same time
04:44 kode54: you could also chime in on that topic about whether it should be allowed or not
04:44 kode54: the last message or two is in dri-devel too
04:45 kode54: my workaround attempts to fix the fact two compositors are poking fb_id and crtc_id with disable settings every commit
04:46 roboporo: why not do an initial non-async commit to disable them and then continue on?
04:58 kode54: I mean
04:58 kode54: it posts the minimal state to every plane on every commit
04:58 kode54: on enabled commits, it pushes every property for the plane
04:59 kode54: I attempted to dedup property writes, which resulted in commits not working and breaking the compositor
06:43 glehmann: cmarcelo: will you continue to review https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/35668 ? I'm actually not quite sure about is_scalar thing myself.
06:56 cmarcelo: glehmann: yeah, I can take a look tomorrow
09:01 DragoonAethis: FYI: The Intel kernel CI infra is going down for the weekend due to the lab power shutdown. It'll be back online on Monday, more details in emails.
10:24 zamundaaa[m]: <roboporo> "why not do an initial non-..." <- Because that is not required by the API, and tracking atomic state on the compositor side just makes the compositor more complicated and makes bugs a lot more likely
10:24 zamundaaa[m]: Or to put it another way, kernel regressions just have to be fixed in the kernel
11:53 kode54: I attempted to track the atomic state in the compositor library
11:53 kode54: it broke spectacularly :D
11:54 kode54: clearly this should be handled in the kernel
11:58 sima: hm did I miss a fun discussion with this "track atomic state" thing?
12:06 daniels: sima: async commits disallowing any property set outside of the golden ones, even if they're the same as prior values
12:08 emersion: these are allowed if they're the same as the previous ones
12:38 sima: daniels, uh ...
12:38 sima: async is very funny anyway, lots of special casing
12:40 pac85: One thing I don't get is why cursor updates are disallowed. The commit that introduces the check talks about modesetting being a problem but updating the cursor wouldn't cause modesetting right?
12:41 pac85: though perhaps cursors is an amd limitation? Not sure
12:42 vsyrjala: intel hw cursors can't do async. and moving a plane is a sync operation anyway
12:42 emersion: there was a patch to allow cursors on AMD
12:42 emersion: not sure what the status is
12:42 pac85: ah
12:43 pac85: anyway with legacy ig. none of this is an issue because cursor is handled completely separately so even if it is a sync update it doesn't affect page flipping?
12:53 MrCooper: vsyrjala: couldn't the HW/driver use the latest available cursor plane position for the next scanout cycle though?
13:01 pac85: yeah basically a mailbox
13:08 MrCooper: something like that has to be happening anyway for the legacy cursor ioctl, isn't it?
14:19 roboporo: MrCooper: yeah that's what I'm interested in. Is it possible to add that path into the atomic api?
14:19 emersion: i've been discussing an AMEND thing numerous times in the past
14:19 roboporo: Having hardware cursors with async commits is definitley a feature missing in atomic vs legacy
14:21 roboporo: emersion: an AMEND thing?
14:25 MrCooper: not sure what AMEND would buy over ASYNC for the cursor plane
14:26 MrCooper: roboporo: a flag to amend a pending atomic commit instead of creating a new one
14:28 roboporo: Ah I see
14:29 emersion: MrCooper: i wouldn't want my cursor to tear, but with ASYNC it can happen
14:30 emersion: IMHO overloading ASYNC semantics isn't a good way forward
14:32 zamundaaa[m]: Yeah, I'd also prefer a separate amend flag
14:32 zamundaaa[m]: Even if the kernel internally confuses the two a lot
14:32 pac85: Yeah, mailbox has different semantics than tearing. If async can be either under the hood and you replace fb_id basically userspace would have no idea which buffer is being scanned out so it can't know which buffer is free to draw to I think. Overloading would only be acceptable for properties like x and y offset ig. However for cursor you also need to say "tear the plane I'm updating the fb onn but mailbox the cursor update"
14:33 pac85: or I guess you could do two commits with different flags?
14:34 roboporo: pac85: I actually tried just doing a seperate commit that only changed the cursor plane position
14:34 MrCooper: emersion: I don't necessarily disagree, the cursor plane needs to be usable with ASYNC anyway though (or ASYNC is kind of unusable with the cursor plane on), and maybe no tearing could be achieved differently, even just a gentlemen's agreement (is there any case where tearing is actually desirable for the cursor plane?)
14:35 roboporo: but it seemed to limit the pageflip rate to the vsync rate instead of allowing a higher "tearing" frame rate
14:35 pac85: MrCooper: if cursor can be updated with an amend flag then you'd just stop updating the cursor in the same commit as the flip perhaops?
14:35 pac85: bascially back to how legacy worked
14:36 MrCooper: roboporo: yep, any commit without ASYNC syncs to vblank
14:37 roboporo: yeah maybe the AMEND thing would just allow the seperate commit to not have to sync to vblank, but just change the cursor plane stuff
14:37 roboporo: I guess basically the cursor plane updates at vblank while the primary does async?
14:39 MrCooper: pac85: hmm, I guess that could kind of work, though it leaves a non-0 chance for the cursor amend commit to miss the next scanout cycle
14:42 pac85: MrCooper: If you want the cursor to not tear but the image to tear, intrinsically you are updating them not atomically. Even if you do it in one commit the image update may still happen ealier because it is allowed to tear whereas the cursor needs to be latched
14:43 MrCooper: one downside of AMEND is that it strictly speaking doesn't allow the driver to apply the cursor plane change during the ongoing scanout cycle, which some HW can do without tearing in some cases
14:43 pac85: uhm
14:44 pac85: I kinda feel like having an ioctl for cursor is the best option if it really has this hw dependent behavior that only applies to cursors
14:44 pac85: and the nice thing is, the cursor ioctl doesn't give you an event back so the change in semantics aren't visible to user space
14:45 MrCooper: could call it DRM_IOCTL_MODE_CURSOR(2) ;)
14:45 pac85: another important different in semantics is wgt vrr
14:45 pac85: I think rn if you update the cursor in a commit it triggers scanout right?
14:46 pac85: that's a huge problem imho
14:46 pac85: and amend wouldn't solve it either
14:47 MrCooper: right (at least that's supposed to happen, I understand there are driver bugs though), OTOH cursor plane moves should be applied for any LFC scanout cycles though
14:49 pac85: yeah, legacy api behavior on amd. In practice it totally eliminates the whole cursor with vrr issue sine the panel is always refreshing at some decent rate due to lfc
14:53 MrCooper: ASYNC seems pretty close, except for tearing and triggering scanout with VRR
14:56 pac85: I mean, it is further than amend
14:56 pac85: I'm not sure how amend is specified would be nice to see the proposal
14:57 MrCooper: let's settle on both having one issue in common and another one each :)
15:02 pac85: lol yeah
15:02 pac85: but yeah if you want the legacy iotcl back I agree with you that's the best solution as it solves all three things very simply
15:05 pac85: the alternative would be a pair of flags perhaps: a flag to do commits that don't touch vrr and a flag that makes the semantics non defined. Sounds like more work
15:39 sima: pac85, yeah async is supposed to be atomic, but with tearing allowed, which is extremely hw specific of what you can and cannot do
15:43 pac85: yeah I see
15:59 MrCooper: does AMEND really help for that though? E.g. if an ASYNC commit for the primary plane is followed by an AMEND commit for the cursor plane, doesn't the latter effectively become part of the ASYNC commit as well?
16:05 pac85: where are the semantics of the amend proposal specified (if anywhere)
16:05 pac85: curious to take a look
16:09 zmike: mareko: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/35769 for common parts
17:55 cmarcelo: glehmann: done.
20:55 glehmann: cmarcelo: thanks
21:29 kode54: the case for the kwin and wlroots compositors
21:30 kode54: even if you force software cursors, it's spamming fb_id=0 and crtc_id=0 every commit