On Wed, Jan 04, 2023 at 10:11:46AM +0100, Michel Dänzer wrote:
> On 1/4/23 03:11, Brian Norris wrote:
> > On Tue, Jan 03, 2023 at 07:04:00PM +0100, Michel Dänzer wrote:
> >> On 12/21/22 23:02, Brian Norris wrote:
> >
> >>> 3. leave vblank enabled even in the presence of PSR
> >
> > I'm leaning toward this.
>
> If this means vblank interrupts will arrive as expected even while in PSR,
> that may be the ideal solution indeed.
Yes. And I think I have a working patchset for this now. It passes all
the igt-gpu-tools/kms_vblank tests for me now, and I don't think it
causes any other issues. I'll send it out shortly, after a bit more
testing.
Side note: I believe this vblank-disabled issue actually came in as an
upstream regression at commit 6c836d965bad ("drm/rockchip: Use the
helpers for PSR"); before that, we were doing this very differently, and
didn't touch vblank at all for PSR, similar to the "downstream" stuff I
mentioned earlier.
> >> 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.
>
> Not sure how likely that is, quite a few drivers are setting
> dev->vblank_disable_immediate = true.
>
> With that, vblank interrupts should generally be enabled only while there are
> screen updates as well[0], in which case PSR shouldn't be possible anyway.
>
> [0] There may be user space which uses the vblank ioctls even while there are
> no screen updates though, which would prevent PSR in this case.
OK. I'm just reading docs here; definitely not an expert.
> >>> [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?
>
> I don't have any specific example, just thinking about how user space could
> respond to the vblank ioctls returning an error, and it would seem to be
> generally either of:
>
> * Just run non-throttled, which might negate any energy savings from PSR
> * Fail to work altogether
>
>
> > 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.
>
> If vblank events never come, user space might freeze.
Thanks. If my patchset works out like I expect, this should no longer be
noticeable to user space, so I don't really have to test out your
educated guesses :)
Thanks for the idea-bouncing.
Brian
