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

From: Doug Anderson <dianders@chromium.org>
To: Brian Norris <briannorris@chromium.org>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
	Maxime Ripard <mripard@kernel.org>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Andrzej Hajda <andrzej.hajda@intel.com>,
	Dmitry Torokhov <dmitry.torokhov@gmail.com>,
	linux-kernel@vger.kernel.org, linux-input@vger.kernel.org,
	David Airlie <airlied@linux.ie>,
	linux-rockchip@lists.infradead.org,
	"Kristian H . Kristensen" <hoegsberg@google.com>,
	Rob Clark <robdclark@chromium.org>,
	Rob Clark <robdclark@gmail.com>, Daniel Vetter <daniel@ffwll.ch>
Subject: Re: [PATCH 2/2] drm/self_refresh: Disable self-refresh on input events
Date: Fri, 12 Nov 2021 16:52:03 -0800	[thread overview]
Message-ID: <CAD=FV=WDRDHVSiFW+yxaR=Z+mNdKnUY_eF_CFqKeQhcKmdag5g@mail.gmail.com> (raw)
In-Reply-To: <20211103164002.2.Ie6c485320b35b89fd49e15a73f0a68e3bb49eef9@changeid>

Hi,

On Wed, Nov 3, 2021 at 4:40 PM Brian Norris <briannorris@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@google.com>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> This was in part picked up from:
>
>   https://lore.kernel.org/all/20180405095000.9756-25-enric.balletbo@collabora.com/
>   [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