Re: [PATCH 2/2] drm/self_refresh: Disable self-refresh on input events

[Doug Anderson]

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