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

[Daniel Vetter <daniel@ffwll.ch>]

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
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch