01:36tarceri: zmike: yeah there are a few remaining issues I'm fixing up that I missed in my original testing
08:50Ermine: Are igt-gpu-tools supposed to work on anything besides x86_64 ?
09:54javierm: tzimmermann: hi, I would like to merge https://lists.freedesktop.org/archives/dri-devel/2025-July/516249.html but wanted to know if you have any objections on patch #5
09:54javierm: tzimmermann: https://patchwork.kernel.org/project/dri-devel/patch/20250721-st7571-format-v2-5-159f4134098c@gmail.com/
10:47tzimmermann: javierm, hey! how's going? i'll look through it. i took vacation in July and probably just missed the series
10:49mlankhorst: airlied: do you have a new version of memcg?
10:56javierm: tzimmermann: sure, no worries. I took vacation on aug so I'm just going through my email backlog
10:56pq: zamundaaa, uhh, sorry, I think I mistook those mesa MR threads as open.
13:08dolphin: sima, airlied: sent drm-intel-gt-next PR, mostly fixes only
13:54tzimmermann: sima, hi. i've annoyed by the drm_dev_enter/_exit() pairs everwhere. can't we do them unconditionally in drm_atomic_helper_commit_tail() ? https://elixir.bootlin.com/linux/v6.16.4/source/drivers/gpu/drm/drm_atomic_helper.c#L1782
14:30sima: tzimmermann, maybe, it's kinda a big change
14:30sima: and I'm not sure whether that won't screw up locking in some drivers
14:39sima: tzimmermann, I did chat with a bunch of other maintainers though who are very firmly of the opinion that hotunplug locking is supposed to be the subsystem's job
14:40sima: so seems to be fairly sound idea
14:40sima: I'm just not sure we won't run into some issues in some driver
14:42tzimmermann: sima, sounds great. commit_tail is also easy to adapt for any 'odd' drivers
14:42sima: yeah that's probably how we could roll this out, if it' all stays within what drivers can overwrite if needed
14:44tzimmermann: i've been looking over it briefly. there a number of calls in the helper (and it's _rpm variant) the first 3 (_modeset_disable, _commit_planes and _enable) would be wrapped in _enter/_exit; the rest of the calls have to be unconditional
14:45tzimmermann: and in _commit_planes() there's a call to ->end_fb_access. this needs to go out of the unplug handle (somewhere before he_done)
14:45tzimmermann: for most of the drivers, it seems manageable
14:47tzimmermann: sima, by 'other maintainers', you mean maintainers of subsystems besides drm, right?
14:48sima: yup
14:48sima: and what do you mean with ->end_fb_access?
14:49sima: I'm not parsing the "go out of the unplug handle"
14:49javierm: tzimmermann: I see that drivers also use drm_dev_enter/_exit() on their drm_plane_helper_funcs.atomic_disable callback to clear up the screen
14:50javierm: tzimmermann: that will also be covered by drm_atomic_helper_commit_tail() ?
14:51javierm: or did you mean only for the drm_plane_helper_funcs.atomic_update callbacks ?
14:56tzimmermann: javierm, all the HW-updates; including plane and crtc
14:57javierm: tzimmermann: got it
14:57tzimmermann: and (from a firt look) they should all happen within the first free calles within drm_atomic_helper_commit_tail()
14:58javierm: it would be a nice cleanup indeed if can just be done there instead of having it in every driver
14:58tzimmermann: sima, by ->end_fb_access i mean this loop: https://elixir.bootlin.com/linux/v6.16.4/source/drivers/gpu/drm/drm_atomic_helper.c#L2865
14:59tzimmermann: it does things like the vunmap() for shadow-plane helpers. so it needs to run a but later
14:59tzimmermann: outside the unplug CS
15:01tzimmermann: s/but/bit
15:12sima: tzimmermann, hm ...
15:12sima: one complication is that the dma api blows up after hotunplug, but I'm not sure how we can sort out that mess properly
15:13sima: tzimmermann, also one reason for letting drivers do this is that it allows you to be a lot more fine-grained with bailing out
15:13sima: because pci timeouts are slow, so for pci drivers you probably want more nested sections
15:13sima: but they can nest (I think so at least), so we should be fine
15:14sima: tzimmermann, I still don't get why you need to move ->end_fb_access outside the critical section, that shouldn't hurt?
15:15sima: we should have a drm_atomic_helper_shutdown anyway in the ->remove path, so we'll have that vunmap in our critical path no matter what we do for driver unload
15:15sima: so not sure what you're gaining by pulling it out of drm_dev_enter/exit?
15:16tzimmermann: because begin_fb_access runs during atomic_check. it usually vmap()s on the plane's gem BOs. we need a corresponding vunmap even if the device got unplugged
15:16sima: oh right
15:16sima: hm
15:16tzimmermann: but it's a trivial change
15:17sima: that might actually break drivers in other ways, if they expect symmetry but it's not there anymore if we do this big wrapping
15:17sima: that = the general issue of normal atomic helper guarantees no longer applying
15:17sima: e.g. pretty sure you'll splat on the crtc_state.event checks if you disable all of commit_planes
15:17sima: at least in most drivers
15:18sima: there's probably a lot more, at least in more complex drivers
15:18javierm: sima: but those drivers usually have their own drm_mode_config_helper_funcs.atomic_commit_tail anyways I think?
15:18sima: javierm, yeah but everyone copypastes the helpers
15:18tzimmermann: i guess we could find a way for doing it incrementally
15:18javierm: sima: hmm, I see
15:19sima: plus handling crtc_state.event is not optional, and that's something drivers have to do
15:19tzimmermann: javierm, drm_atomic_helper_commit_tail() is the default
15:19sima: also not sure whether that won't blow up the drm_vblank_on/off tracking?
15:19tzimmermann: drivers usually don't set their own
15:19sima: that's another one that is mostly sw tracking, and we cannot disable sw stuff
15:19sima: and it's another one (like crtc_state.event) that drivers have to do in their callbacks at the right time
15:20sima: at least if they support vblank events
15:20tzimmermann: some vblank handling is often in atomic_flush. could become an issue
15:20sima: tzimmermann, so with a bit more pondering, really not sure this works beyond the most trivial drivers :-(
15:21sima: tzimmermann, other way round would be some pcibar access helpers that wrap drm_dev_enter/exit at the lowest levels
15:21sima: and convert pci drivers over to use those
15:21tzimmermann: but if the HW device is unplugged there won't be any vblank interrupts. we could fake the event in the commit_tail helper
15:22sima: tzimmermann, it's not about the hw events, but about the sw tracking that goes loopsided
15:22sima: similar to how vunmap needs to be done or things go awry
15:22sima: you explaining the vunmap case made me remember all the others we have :-/
15:22sima: s/all/some/ since I expect there's a bunch more
15:23tzimmermann: sima, moving this into HW-access helpers sounds like even more hassle to me
15:24tzimmermann: like touching all pci drivers
15:24sima: yeah I don't have good ideas here :-/
15:24tzimmermann: and there's no clear way of handling register reads
15:25tzimmermann: no worries. i guess we could start small for a dedicated commit_tail helper for the simple drivers and go step-by-step from there
15:26sima: pretty sure they all break due to crtc_state.event, unless you handle that somehow
15:27sima: and it would break for any drivers using the drm_crtc_vblank* functions even more
15:28tzimmermann: :(
15:28sima: but worth a shot I guess, we're low on good ideas here :-(
15:30sima: tzimmermann, meaning if you handle crtc_state.event in the else case for when drm_dev_enter fails, that /might/ work for some drivers
15:30sima: anyway gtg now, heading out for dinner
15:30tzimmermann: sima, sure. thanks