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

Hi Doug,

On Fri, Nov 12, 2021 at 4:52 PM Doug Anderson [off-list ref] wrote:

On Wed, Nov 3, 2021 at 4:40 PM Brian Norris [off-list ref] wrote:

...

quoted

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?

Sure, I think so. This feature is unfortunately on the borderline of
"policy" (which we normally avoid baking into the kernel), so having
some control over it is probably a good idea -- e.g., module
parameter, CONFIG_*, or both.

I suppose that would make sense to be a "self_refresh"-level control,
and not a "drm_input_helper"-level control? Because different
applications (PSR, GPU boost, etc.) may have different characteristics
and reasons for leveraging this or not.

quoted

Inspired-by: Kristian H. Kristensen [off-list ref]
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/ (local)
  [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...

Thanks ;)

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?

I did think about that option (calling mod_timer to delay the next PSR
entry), but thought it was a bit excessive, at least in terms of
calling it a "race" -- the race between steps #5 and #6 are
essentially equivalent to the natural (unsolvable) race between #1 and
#2 (we can't really read the future about input events).

But rereading your explanation and thinking again, I see that you're
pointing out less of a "race" in the traditional sense, and more of a
missing part of this feature: I think what you're really saying is
that input events should not only exit PSR, but they should delay PSR
(re)entry for some time. With my current patch, input events only
enforce any delay time window if we were already in PSR.

I'll try to factor that into the next version. Thanks!

Brian