On Fri, Jan 06, 2023 at 07:20:40PM +0100, Daniel Vetter wrote: > On Fri, Jan 06, 2023 at 10:08:53AM -0800, Brian Norris wrote: > > OK! Then that sounds like it at least ACKs my general idea for this > > series. (Michel and I poked at a few ideas in the thread at [1] and > > landed on approx. this solution, or else a fake/timer like you suggest.) > > Yeah once I stopped looking at this the wrong way round it does make sense > what you're doing. See my other reply, I think with just this series here > the vblanks still stall out? Or does your hw actually keep generating > vblank irq with the display off? I might not be understanding all of the IGT tests that I've run through, but the display is not actually off -- it's sitting on a still frame. But yes, IRQs still come, and I see no hangs. This is also ref'd in patch 2: bed030a49f3e drm/rockchip: Don't fully disable vop on self refresh So, we're not even doing that much to power-down the CRTC/VOP. That's probably why IRQs are still active, contrary to your expectation? NB: this is how Rockchip Chromebooks implemented PSR from essentially day 1. > > On Fri, Jan 06, 2023 at 06:53:49PM +0100, Daniel Vetter wrote: > > > We might need a few more helpers. Also, probably more igt, or is this > > > something igt testing has uncovered? If so, please cite the igt testcase > > > which hits this. > > > > The current patch only fixes a warning that comes when I try to do the > > second patch. The second patch is a direct product of an IGT test > > failure (a few of kms_vblank's subtests), and I linked [1] the KernelCI > > report there. > > Ah yeah that makes sense. Would be good to cite that in this patch too, > because I guess the same kms_vblank tests can also hit this warning here? Well, before this series: no, those tests don't hit this warning. The warning is only uncovered after patch 2. If I do just patch 2, it's super-trivial to hit the warning. You just have to turn the display on and go idle long enough for PSR to activate once. I suppose that can count as "caught by a test", with a little stretch of the imagination. At any rate, I'll improve this description to refer precisely to the "next patch" (as Greg suggested already), and that might involve describing this test case a little. Brian
