[PATCH 2/2] drm/self_refresh: Disable self-refresh on input events
dianders at chromium.org
Fri Nov 12 16:52:03 PST 2021
On Wed, Nov 3, 2021 at 4:40 PM Brian Norris <briannorris at chromium.org> wrote:
> To improve panel self-refresh exit latency, we speculatively start
> exiting when we
> receive input events. Occasionally, this may lead to false positives,
> but most of the time we get a head start on coming out of PSR. Depending
> on how userspace takes to produce a new frame in response to the event,
> this can completely hide the exit latency.
> In local tests on Chrome OS (Rockchip RK3399 eDP), we've found that the
> input notifier gives us about a 50ms head start over the
> fb-update-initiated exit.
> Leverage a new drm_input_helper library to get easy access to
> likely-relevant input event callbacks.
So IMO this is a really useful thing and I'm in support of it landing.
It's not much code and it clearly gives a big benefit. However, I
would request a CONFIG option to control this so that if someone
really finds some use case where it isn't needed or if they find a
good way to do this in userspace without latency problems then they
can turn it off. Does that sound reasonable?
> Inspired-by: Kristian H. Kristensen <hoegsberg at google.com>
> Signed-off-by: Brian Norris <briannorris at chromium.org>
> This was in part picked up from:
> [PATCH v6 24/30] drm/rockchip: Disable PSR on input events
> with significant rewrites/reworks:
> - moved to common drm_input_helper and drm_self_refresh_helper
> - track state only through crtc->state->self_refresh_active
> Note that I'm relatively unfamiliar with DRM locking expectations, but I
> believe access to drm_crtc->state (which helps us track redundant
> transitions) is OK under the locking provided by
Yeah, I'm no expert here either. I gave a review a shot anyway since
it's been all quiet, but adult supervision is probably required...
I can believe that you are safe from corrupting things, but I think
you still have locking problems, don't you? What about this:
1. PSR is _not_ active but we're 1 microsecond away from entering PSR
2. Input event comes through.
3. Start executing drm_self_refresh_transition(false).
4. PSR timer expires and starts executing drm_self_refresh_transition(true).
5. Input event "wins the race" but sees that PSR is already disabled => noop
6. PSR timer gets the lock now. Starts PSR transition.
Wouldn't it be better to cancel / reschedule any PSR entry as soon as
you see the input event?
More information about the Linux-rockchip