Hi, On Wed, Nov 3, 2021 at 4:40 PM Brian Norris <briannorris@xxxxxxxxxxxx> 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@xxxxxxxxxx> > Signed-off-by: Brian Norris <briannorris@xxxxxxxxxxxx> > --- > This was in part picked up from: > > https://lore.kernel.org/all/20180405095000.9756-25-enric.balletbo@xxxxxxxxxxxxx/ > [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 > implementation > - 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 > drm_atomic_get_crtc_state(). 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? -Doug
