Hi Michel, Thanks for your thoughts. I'll give my attempt at weighing my and your options, with the caveat that I'm still not much of a DRM expert. On Tue, Jan 03, 2023 at 07:04:00PM +0100, Michel Dänzer wrote: > On 12/21/22 23:02, Brian Norris wrote: > > So how to fix this? > > > > 1. ignore it; it's "just" a test failure, IIUC [1] Probably discarded, per Michel's notes. > > 2. change the test, to skip this test if the device has PSR Doesn't seem great, and not just because PSR is not directly detectable in user space. > > 3. leave vblank enabled even in the presence of PSR I'm leaning toward this. > > 4. otherwise modify the vblank ioctl interface, such that one can "wait" > > for a vblank that won't come (because the display is in self-refresh > > / there are no new frames or vblanks) That doesn't sound so great of an API, to essentially induce hangs, pending other activity. (Assuming I understand the DRM APIs here correctly.) > FWIW, a couple more alternatives: > > 5. Go/stay out of PSR while vblank interrupts are enabled (probably want to make sure vblankoffdelay is set up such that vblank interrupts are disabled ASAP) That seems not extremely nice, to waste power. Based on the earlier downstream implementation (which left vblank interrupts enabled), I'd wager there's a much larger power win from PSR (on the display-transport and memory-bandwidth costs), relative to the power cost of vblank interrupts. Also, messing with vblankoffdelay sounds likely to trigger the races mentioned in the drm_vblank.c kerneldoc. > 6. Use fallback timers for vblank events while in PSR (there might be some infrastructure for this, since some drivers don't have real vblank interrupts) There's drm_atomic_helper_fake_vblank(), but I don't think that's really hooked up to a timer. That's potentially promising though. > > [1] Or is it? I don't really know the DRM_IOCTL_WAIT_VBLANK ABI > > contract in the presence of PSR, but I believe the upstream PSR > > support has always worked this way. I could imagine > > DRM_IOCTL_WAIT_VBLANK users might not love seeing EINVAL here > > though. > > Yeah, that's pretty likely to cause issues with existing real-world user space. OK. Any hints as to what kind of user space uses this? A cursory look doesn't show that any of the ChromeOS user space uses it, which may be why it was overlooked in the initial PSR development and upstreaming. I'm in part simply curious, but I'm also wondering what the error-handling and reliability (e.g., what if vblanks don't come?) expectations might be in practice. All in all, it's seeming like maybe option 3 or 6 are best. I'd lean toward 3, assuming I can hack my way through "driver forgot to call drm_crtc_vblank_off()" and similar problems, in part because it's ground that has partially been tread before. I hope that doesn't complicate the DRM state machine even more though, now that I'll have to better coordinate the "enabled"/"self_refresh_active" states with "vblank enabled"... Thoughts still welcome of course. Brian
