| Message ID | 20211103164002.2.Ie6c485320b35b89fd49e15a73f0a68e3bb49eef9@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm: Support input-boosted panel self-refresh exit | expand |
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
Hi Doug, On Fri, Nov 12, 2021 at 4:52 PM Doug Anderson <dianders@chromium.org> wrote: > On Wed, Nov 3, 2021 at 4:40 PM Brian Norris <briannorris@chromium.org> wrote: ... > > 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. > > 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... 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
On Thu, Nov 4, 2021 at 12:40 AM 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. > > Inspired-by: Kristian H. Kristensen <hoegsberg@google.com> > Signed-off-by: Brian Norris <briannorris@chromium.org> Can you pls resend with dri-devel on cc? scripts/get_maintainers.pl should pick this up, you have all the maintainers but not the list. -Daniel > --- > 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(). > > drivers/gpu/drm/drm_self_refresh_helper.c | 54 ++++++++++++++++++++--- > 1 file changed, 48 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_self_refresh_helper.c b/drivers/gpu/drm/drm_self_refresh_helper.c > index dd33fec5aabd..dcab061cc90a 100644 > --- a/drivers/gpu/drm/drm_self_refresh_helper.c > +++ b/drivers/gpu/drm/drm_self_refresh_helper.c > @@ -15,6 +15,7 @@ > #include <drm/drm_connector.h> > #include <drm/drm_crtc.h> > #include <drm/drm_device.h> > +#include <drm/drm_input_helper.h> > #include <drm/drm_mode_config.h> > #include <drm/drm_modeset_lock.h> > #include <drm/drm_print.h> > @@ -58,17 +59,17 @@ DECLARE_EWMA(psr_time, 4, 4) > struct drm_self_refresh_data { > struct drm_crtc *crtc; > struct delayed_work entry_work; > + struct work_struct exit_work; > + struct drm_input_handler input_handler; > > struct mutex avg_mutex; > struct ewma_psr_time entry_avg_ms; > struct ewma_psr_time exit_avg_ms; > }; > > -static void drm_self_refresh_helper_entry_work(struct work_struct *work) > +static void drm_self_refresh_transition(struct drm_self_refresh_data *sr_data, > + bool enable) > { > - struct drm_self_refresh_data *sr_data = container_of( > - to_delayed_work(work), > - struct drm_self_refresh_data, entry_work); > struct drm_crtc *crtc = sr_data->crtc; > struct drm_device *dev = crtc->dev; > struct drm_modeset_acquire_ctx ctx; > @@ -95,6 +96,9 @@ static void drm_self_refresh_helper_entry_work(struct work_struct *work) > goto out; > } > > + if (crtc->state->self_refresh_active == enable) > + goto out; > + > if (!crtc_state->enable) > goto out; > > @@ -107,8 +111,8 @@ static void drm_self_refresh_helper_entry_work(struct work_struct *work) > goto out; > } > > - crtc_state->active = false; > - crtc_state->self_refresh_active = true; > + crtc_state->active = !enable; > + crtc_state->self_refresh_active = enable; > > ret = drm_atomic_commit(state); > if (ret) > @@ -129,6 +133,15 @@ static void drm_self_refresh_helper_entry_work(struct work_struct *work) > drm_modeset_acquire_fini(&ctx); > } > > +static void drm_self_refresh_helper_entry_work(struct work_struct *work) > +{ > + struct drm_self_refresh_data *sr_data = container_of( > + to_delayed_work(work), > + struct drm_self_refresh_data, entry_work); > + > + drm_self_refresh_transition(sr_data, true); > +} > + > /** > * drm_self_refresh_helper_update_avg_times - Updates a crtc's SR time averages > * @state: the state which has just been applied to hardware > @@ -223,6 +236,20 @@ void drm_self_refresh_helper_alter_state(struct drm_atomic_state *state) > } > EXPORT_SYMBOL(drm_self_refresh_helper_alter_state); > > +static void drm_self_refresh_helper_exit_work(struct work_struct *work) > +{ > + struct drm_self_refresh_data *sr_data = container_of( > + work, struct drm_self_refresh_data, exit_work); > + > + drm_self_refresh_transition(sr_data, false); > +} > + > +static void drm_self_refresh_input_event(void *data) > +{ > + struct drm_self_refresh_data *sr_data = data; > + > + schedule_work(&sr_data->exit_work); > +} > /** > * drm_self_refresh_helper_init - Initializes self refresh helpers for a crtc > * @crtc: the crtc which supports self refresh supported displays > @@ -232,6 +259,7 @@ EXPORT_SYMBOL(drm_self_refresh_helper_alter_state); > int drm_self_refresh_helper_init(struct drm_crtc *crtc) > { > struct drm_self_refresh_data *sr_data = crtc->self_refresh_data; > + int ret; > > /* Helper is already initialized */ > if (WARN_ON(sr_data)) > @@ -243,6 +271,7 @@ int drm_self_refresh_helper_init(struct drm_crtc *crtc) > > INIT_DELAYED_WORK(&sr_data->entry_work, > drm_self_refresh_helper_entry_work); > + INIT_WORK(&sr_data->exit_work, drm_self_refresh_helper_exit_work); > sr_data->crtc = crtc; > mutex_init(&sr_data->avg_mutex); > ewma_psr_time_init(&sr_data->entry_avg_ms); > @@ -256,8 +285,19 @@ int drm_self_refresh_helper_init(struct drm_crtc *crtc) > ewma_psr_time_add(&sr_data->entry_avg_ms, SELF_REFRESH_AVG_SEED_MS); > ewma_psr_time_add(&sr_data->exit_avg_ms, SELF_REFRESH_AVG_SEED_MS); > > + sr_data->input_handler.callback = drm_self_refresh_input_event; > + sr_data->input_handler.priv = sr_data; > + ret = drm_input_handle_register(crtc->dev, &sr_data->input_handler); > + if (ret) > + goto err; > + > crtc->self_refresh_data = sr_data; > + > return 0; > + > +err: > + kfree(sr_data); > + return ret; > } > EXPORT_SYMBOL(drm_self_refresh_helper_init); > > @@ -275,7 +315,9 @@ void drm_self_refresh_helper_cleanup(struct drm_crtc *crtc) > > crtc->self_refresh_data = NULL; > > + drm_input_handle_unregister(&sr_data->input_handler); > cancel_delayed_work_sync(&sr_data->entry_work); > + cancel_work_sync(&sr_data->exit_work); > kfree(sr_data); > } > EXPORT_SYMBOL(drm_self_refresh_helper_cleanup); > -- > 2.34.0.rc0.344.g81b53c2807-goog >
On Wed, Nov 17, 2021 at 11:12 AM Daniel Vetter <daniel@ffwll.ch> wrote: > Can you pls resend with dri-devel on cc? scripts/get_maintainers.pl > should pick this up, you have all the maintainers but not the list. Oops, I don't know how that happened. I guess I sometimes have to trim get_maintainer output, since it likes to hoover up a bunch of barely-relevant previous committers. I must have been too aggressive. I'll plan on sending v2 to dri-devel, but let me know (privately if you'd like) if you'd prefer a pure RESEND of v1. Brian
On Wed, Nov 17, 2021 at 8:37 PM Brian Norris <briannorris@chromium.org> wrote: > > On Wed, Nov 17, 2021 at 11:12 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > Can you pls resend with dri-devel on cc? scripts/get_maintainers.pl > > should pick this up, you have all the maintainers but not the list. > > Oops, I don't know how that happened. I guess I sometimes have to trim > get_maintainer output, since it likes to hoover up a bunch of > barely-relevant previous committers. I must have been too aggressive. > > I'll plan on sending v2 to dri-devel, but let me know (privately if > you'd like) if you'd prefer a pure RESEND of v1. Nah just for next version is fine, assuming you include all the context in in-patch changelog and all that so new readers can catch up. -Daniel
diff --git a/drivers/gpu/drm/drm_self_refresh_helper.c b/drivers/gpu/drm/drm_self_refresh_helper.c index dd33fec5aabd..dcab061cc90a 100644 --- a/drivers/gpu/drm/drm_self_refresh_helper.c +++ b/drivers/gpu/drm/drm_self_refresh_helper.c @@ -15,6 +15,7 @@ #include <drm/drm_connector.h> #include <drm/drm_crtc.h> #include <drm/drm_device.h> +#include <drm/drm_input_helper.h> #include <drm/drm_mode_config.h> #include <drm/drm_modeset_lock.h> #include <drm/drm_print.h> @@ -58,17 +59,17 @@ DECLARE_EWMA(psr_time, 4, 4) struct drm_self_refresh_data { struct drm_crtc *crtc; struct delayed_work entry_work; + struct work_struct exit_work; + struct drm_input_handler input_handler; struct mutex avg_mutex; struct ewma_psr_time entry_avg_ms; struct ewma_psr_time exit_avg_ms; }; -static void drm_self_refresh_helper_entry_work(struct work_struct *work) +static void drm_self_refresh_transition(struct drm_self_refresh_data *sr_data, + bool enable) { - struct drm_self_refresh_data *sr_data = container_of( - to_delayed_work(work), - struct drm_self_refresh_data, entry_work); struct drm_crtc *crtc = sr_data->crtc; struct drm_device *dev = crtc->dev; struct drm_modeset_acquire_ctx ctx; @@ -95,6 +96,9 @@ static void drm_self_refresh_helper_entry_work(struct work_struct *work) goto out; } + if (crtc->state->self_refresh_active == enable) + goto out; + if (!crtc_state->enable) goto out; @@ -107,8 +111,8 @@ static void drm_self_refresh_helper_entry_work(struct work_struct *work) goto out; } - crtc_state->active = false; - crtc_state->self_refresh_active = true; + crtc_state->active = !enable; + crtc_state->self_refresh_active = enable; ret = drm_atomic_commit(state); if (ret) @@ -129,6 +133,15 @@ static void drm_self_refresh_helper_entry_work(struct work_struct *work) drm_modeset_acquire_fini(&ctx); } +static void drm_self_refresh_helper_entry_work(struct work_struct *work) +{ + struct drm_self_refresh_data *sr_data = container_of( + to_delayed_work(work), + struct drm_self_refresh_data, entry_work); + + drm_self_refresh_transition(sr_data, true); +} + /** * drm_self_refresh_helper_update_avg_times - Updates a crtc's SR time averages * @state: the state which has just been applied to hardware @@ -223,6 +236,20 @@ void drm_self_refresh_helper_alter_state(struct drm_atomic_state *state) } EXPORT_SYMBOL(drm_self_refresh_helper_alter_state); +static void drm_self_refresh_helper_exit_work(struct work_struct *work) +{ + struct drm_self_refresh_data *sr_data = container_of( + work, struct drm_self_refresh_data, exit_work); + + drm_self_refresh_transition(sr_data, false); +} + +static void drm_self_refresh_input_event(void *data) +{ + struct drm_self_refresh_data *sr_data = data; + + schedule_work(&sr_data->exit_work); +} /** * drm_self_refresh_helper_init - Initializes self refresh helpers for a crtc * @crtc: the crtc which supports self refresh supported displays @@ -232,6 +259,7 @@ EXPORT_SYMBOL(drm_self_refresh_helper_alter_state); int drm_self_refresh_helper_init(struct drm_crtc *crtc) { struct drm_self_refresh_data *sr_data = crtc->self_refresh_data; + int ret; /* Helper is already initialized */ if (WARN_ON(sr_data)) @@ -243,6 +271,7 @@ int drm_self_refresh_helper_init(struct drm_crtc *crtc) INIT_DELAYED_WORK(&sr_data->entry_work, drm_self_refresh_helper_entry_work); + INIT_WORK(&sr_data->exit_work, drm_self_refresh_helper_exit_work); sr_data->crtc = crtc; mutex_init(&sr_data->avg_mutex); ewma_psr_time_init(&sr_data->entry_avg_ms); @@ -256,8 +285,19 @@ int drm_self_refresh_helper_init(struct drm_crtc *crtc) ewma_psr_time_add(&sr_data->entry_avg_ms, SELF_REFRESH_AVG_SEED_MS); ewma_psr_time_add(&sr_data->exit_avg_ms, SELF_REFRESH_AVG_SEED_MS); + sr_data->input_handler.callback = drm_self_refresh_input_event; + sr_data->input_handler.priv = sr_data; + ret = drm_input_handle_register(crtc->dev, &sr_data->input_handler); + if (ret) + goto err; + crtc->self_refresh_data = sr_data; + return 0; + +err: + kfree(sr_data); + return ret; } EXPORT_SYMBOL(drm_self_refresh_helper_init); @@ -275,7 +315,9 @@ void drm_self_refresh_helper_cleanup(struct drm_crtc *crtc) crtc->self_refresh_data = NULL; + drm_input_handle_unregister(&sr_data->input_handler); cancel_delayed_work_sync(&sr_data->entry_work); + cancel_work_sync(&sr_data->exit_work); kfree(sr_data); } EXPORT_SYMBOL(drm_self_refresh_helper_cleanup);
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. 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(). drivers/gpu/drm/drm_self_refresh_helper.c | 54 ++++++++++++++++++++--- 1 file changed, 48 insertions(+), 6 deletions(-)