renesas/master bisection: igt-kms-rockchip.kms_vblank.pipe-A-wait-forked on rk3399-gru-kevin

[Brian Norris]

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