Hi Sean.
Small things here and there. Did not stare at this long enough to
understand the code, but added some feedback anyway.
Sam
>
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index d36feb4a62330..9d630a28a7880 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -43,7 +43,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_dsc.o drm_probe_helper
> drm_simple_kms_helper.o drm_modeset_helper.o \
> drm_scdc_helper.o drm_gem_framebuffer_helper.o \
> drm_atomic_state_helper.o drm_damage_helper.o \
> - drm_format_helper.o
> + drm_format_helper.o drm_self_refresh_helper.o
We really should have only one .o file per line and sort them.
The current way to do things is asking for conflicts.
Anyway, that battle is for another day.
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index e58be69960692..bc43cc08cdc11 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -33,6 +33,7 @@
> #include <drm/drm_damage_helper.h>
> #include <drm/drm_device.h>
> #include <drm/drm_plane_helper.h>
> +#include <drm/drm_self_refresh_helper.h>
> #include <drm/drm_print.h>
> #include <drm/drm_vblank.h>
> #include <drm/drm_writeback.h>
Please add header file in alphabetic order.
It goes below drm_print.h
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_self_refresh_helper.c
> @@ -0,0 +1,213 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright (C) 2019 Google, Inc.
> + *
> + * Authors:
> + * Sean Paul <seanpaul@xxxxxxxxxxxx>
> + */
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_connector.h>
> +#include <drm/drm_crtc.h>
> +#include <drm/drm_device.h>
> +#include <drm/drm_mode_config.h>
> +#include <drm/drm_modeset_lock.h>
> +#include <drm/drm_print.h>
> +#include <drm/drm_self_refresh_helper.h>
> +#include <linux/bitops.h>
> +#include <linux/slab.h>
> +#include <linux/workqueue.h>
Nit:
Typical include order is:
#include <linux/*>
#include <drm/*>
#include "*"
With an empty lines between the blocks.
And sorted within the block.
> +
> +/**
> + * DOC: overview
> + *
> + * This helper library provides an easy way for drivers to leverage the atomic
> + * framework to implement panel self refresh (SR) support. Drivers are
> + * responsible for initializing and cleaning up the SR helpers on load/unload
> + * (see &drm_self_refresh_helper_init/&drm_self_refresh_helper_cleanup).
> + * The connector is responsible for setting
> + * &drm_connector_state.self_refresh_aware to true at runtime if it is SR-aware
> + * (meaning it knows how to initiate self refresh on the panel).
> + *
> + * Once a crtc has enabled SR using &drm_self_refresh_helper_init, the
> + * helpers will monitor activity and call back into the driver to enable/disable
> + * SR as appropriate. The best way to think about this is that it's a DPMS
> + * on/off request with &drm_crtc_state.self_refresh_active set in crtc state
> + * that tells you to disable/enable SR on the panel instead of power-cycling it.
> + *
> + * During SR, drivers may choose to fully disable their crtc/encoder/bridge
> + * hardware (in which case no driver changes are necessary), or they can inspect
> + * &drm_crtc_state.self_refresh_active if they want to enter low power mode
> + * without full disable (in case full disable/enable is too slow).
> + *
> + * SR will be deactivated if there are any atomic updates affecting the
> + * pipe that is in SR mode. If a crtc is driving multiple connectors, all
> + * connectors must be SR aware and all will enter/exit SR mode at the same time.
> + *
> + * If the crtc and connector are SR aware, but the panel connected does not
> + * support it (or is otherwise unable to enter SR), the driver should fail
> + * atomic_check when &drm_crtc_state.self_refresh_active is true.
> + */
> +
> +struct drm_self_refresh_data {
> + struct drm_crtc *crtc;
> + struct delayed_work entry_work;
> + struct drm_atomic_state *save_state;
> + unsigned int entry_delay_ms;
> +};
> +
> +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);
> + struct drm_crtc *crtc = sr_data->crtc;
> + struct drm_device *dev = crtc->dev;
> + struct drm_modeset_acquire_ctx ctx;
> + struct drm_atomic_state *state;
> + struct drm_connector *conn;
> + struct drm_connector_state *conn_state;
> + struct drm_crtc_state *crtc_state;
> + int i, ret;
This function is called from a workqueue.
Just wondering if this require any locking?
(Maybe I missed it, browsed the code without a detailed review)
> +
> + drm_modeset_acquire_init(&ctx, 0);
> +
> + state = drm_atomic_state_alloc(dev);
> + if (!state) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> +retry:
> + state->acquire_ctx = &ctx;
> +
> + crtc_state = drm_atomic_get_crtc_state(state, crtc);
> + if (IS_ERR(crtc_state)) {
> + ret = PTR_ERR(crtc_state);
> + goto out;
> + }
> +
> + if (!crtc_state->enable)
> + goto out;
> +
> + ret = drm_atomic_add_affected_connectors(state, crtc);
> + if (ret)
> + goto out;
> +
> + for_each_new_connector_in_state(state, conn, conn_state, i) {
> + if (!conn_state->self_refresh_aware)
> + goto out;
> + }
> +
> + crtc_state->active = false;
> + crtc_state->self_refresh_active = true;
> +
> + ret = drm_atomic_commit(state);
> + if (ret)
> + goto out;
> +
> +out:
> + if (ret == -EDEADLK) {
> + drm_atomic_state_clear(state);
> + ret = drm_modeset_backoff(&ctx);
> + if (!ret)
> + goto retry;
> + }
> +
> + drm_atomic_state_put(state);
> + drm_modeset_drop_locks(&ctx);
> + drm_modeset_acquire_fini(&ctx);
> +}
> +
> +
> +/**
> + * drm_self_refresh_helper_init - Initializes self refresh helpers for a crtc
> + * @crtc: the crtc which supports self refresh supported displays
> + * @entry_delay_ms: amount of inactivity to wait before entering self refresh
> + */
> +int drm_self_refresh_helper_init(struct drm_crtc *crtc,
> + unsigned int entry_delay_ms)
Missing documentation of return code.
> +{
> + struct drm_self_refresh_data *sr_data = crtc->self_refresh_data;
> +
> + /* Helper is already initialized */
> + if (WARN_ON(sr_data))
> + return -EINVAL;
> +
> + sr_data = kzalloc(sizeof(*sr_data), GFP_KERNEL);
> + if (!sr_data)
> + return -ENOMEM;
> +
> + INIT_DELAYED_WORK(&sr_data->entry_work,
> + drm_self_refresh_helper_entry_work);
> + sr_data->entry_delay_ms = entry_delay_ms;
> + sr_data->crtc = crtc;
> +
> + crtc->self_refresh_data = sr_data;
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_self_refresh_helper_init);
> +
>
> /**
> diff --git a/include/drm/drm_self_refresh_helper.h b/include/drm/drm_self_refresh_helper.h
> new file mode 100644
> index 0000000000000..405e86fb8461e
> --- /dev/null
> +++ b/include/drm/drm_self_refresh_helper.h
> @@ -0,0 +1,22 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright (C) 2019 Google, Inc.
> + *
> + * Authors:
> + * Sean Paul <seanpaul@xxxxxxxxxxxx>
> + */
> +#ifndef DRM_SELF_REFRESH_HELPER_H_
> +#define DRM_SELF_REFRESH_HELPER_H_
> +
> +struct drm_atomic_state;
OK
> +struct drm_connector;
> +struct drm_device;
> +struct drm_modeset_acquire_ctx;
The three above are not needed.
struct drm_crtc;
Missing
> +
> +void drm_self_refresh_helper_alter_state(struct drm_atomic_state *state);
> +
> +int drm_self_refresh_helper_init(struct drm_crtc *crtc,
> + unsigned int entry_delay_ms);
> +
> +void drm_self_refresh_helper_cleanup(struct drm_crtc *crtc);
> +#endif
> --
> Sean Paul, Software Engineer, Google / Chromium OS
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
