On 17.10.2017 23:13, Sean Paul wrote:
> This patch adds state tracking to the drm_panel functions which keep
> track of enabled and prepared. If the calls are unbalanced, a WARNING is
> issued.
>
> The motivation for this change is that a number of panel drivers
> (including panel-simple) all do this to protect their regulator
> refcounts. The atomic framework ensures the calls are balanced, and
> there aren't any panel drivers being used by legacy drivers. As such,
> these checks are useless, but let's add a WARNING just in case something
> crazy happens (like a legacy driver using a panel).
>
> Less code == better.
>
> Changes in v2:
> - Reduced enabled/prepared bools to one enum (Andrzej)
> - Don't update state if the operation fails (Andrzej)
> - Issue warning even if operation is not implemented
>
> Changes in v3:
> - Functions no longer static inline (Daniel)
> - Added a helper function to share state change code (Andrzej)
> - Only warn when a panel is present (Daniel)
> - Update state if function is not implemented (Andrzej)
>
> Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
> ---
> drivers/gpu/drm/drm_panel.c | 63 ++++++++++++++++++++++++++++++++++++---------
> include/drm/drm_panel.h | 15 +++++++++++
> 2 files changed, 66 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index 6f28598a5d0e..6547850b5e80 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -21,6 +21,7 @@
> * DEALINGS IN THE SOFTWARE.
> */
>
> +#include <linux/bug.h>
> #include <linux/err.h>
> #include <linux/module.h>
>
> @@ -48,6 +49,7 @@ static LIST_HEAD(panel_list);
> void drm_panel_init(struct drm_panel *panel)
> {
> INIT_LIST_HEAD(&panel->list);
> + panel->state = DRM_PANEL_POWER_OFF;
> }
> EXPORT_SYMBOL(drm_panel_init);
>
> @@ -126,6 +128,23 @@ int drm_panel_detach(struct drm_panel *panel)
> }
> EXPORT_SYMBOL(drm_panel_detach);
>
> +static int drm_panel_change_state(struct drm_panel *panel,
> + enum drm_panel_state cur_state,
> + enum drm_panel_state new_state,
> + int (*hook)(struct drm_panel *panel))
> +{
> + int ret;
> +
> + WARN_ON(panel->state != cur_state);
> + if (hook)
> + ret = hook(panel);
> + else
> + ret = -ENOSYS;
You have missed my 2 comments:
- do not change state on callback error,
- return 0 in case there is no callback.
Regards
Andrzej
> +
> + panel->state = new_state;
> + return ret;
> +}
> +
> /**
> * drm_panel_unprepare - power off a panel
> * @panel: DRM panel
> @@ -135,14 +154,19 @@ EXPORT_SYMBOL(drm_panel_detach);
> * is usually no longer possible to communicate with the panel until another
> * call to drm_panel_prepare().
> *
> + * Atomic framework will ensure that prepare/unprepare are properly balanced.
> + * If this is not the case, a WARNING will be issued.
> + *
> * Return: 0 on success or a negative error code on failure.
> */
> int drm_panel_unprepare(struct drm_panel *panel)
> {
> - if (panel && panel->funcs && panel->funcs->unprepare)
> - return panel->funcs->unprepare(panel);
> + if (!panel)
> + return -EINVAL;
>
> - return panel ? -ENOSYS : -EINVAL;
> + return drm_panel_change_state(panel, DRM_PANEL_POWER_PREPARED,
> + DRM_PANEL_POWER_OFF,
> + panel->funcs ? panel->funcs->unprepare : NULL);
> }
> EXPORT_SYMBOL(drm_panel_unprepare);
>
> @@ -154,14 +178,19 @@ EXPORT_SYMBOL(drm_panel_unprepare);
> * drivers. For smart panels it should still be possible to communicate with
> * the integrated circuitry via any command bus after this call.
> *
> + * Atomic framework will ensure that enable/disable are properly balanced.
> + * If this is not the case, a WARNING will be issued.
> + *
> * Return: 0 on success or a negative error code on failure.
> */
> int drm_panel_disable(struct drm_panel *panel)
> {
> - if (panel && panel->funcs && panel->funcs->disable)
> - return panel->funcs->disable(panel);
> + if (!panel)
> + return -EINVAL;
>
> - return panel ? -ENOSYS : -EINVAL;
> + return drm_panel_change_state(panel, DRM_PANEL_POWER_ENABLED,
> + DRM_PANEL_POWER_PREPARED,
> + panel->funcs ? panel->funcs->disable : NULL);
> }
> EXPORT_SYMBOL(drm_panel_disable);
>
> @@ -173,14 +202,19 @@ EXPORT_SYMBOL(drm_panel_disable);
> * the panel. After this has completed it is possible to communicate with any
> * integrated circuitry via a command bus.
> *
> + * Atomic framework will ensure that prepare/unprepare are properly balanced.
> + * If this is not the case, a WARNING will be issued.
> + *
> * Return: 0 on success or a negative error code on failure.
> */
> int drm_panel_prepare(struct drm_panel *panel)
> {
> - if (panel && panel->funcs && panel->funcs->prepare)
> - return panel->funcs->prepare(panel);
> + if (!panel)
> + return -EINVAL;
>
> - return panel ? -ENOSYS : -EINVAL;
> + return drm_panel_change_state(panel, DRM_PANEL_POWER_OFF,
> + DRM_PANEL_POWER_PREPARED,
> + panel->funcs ? panel->funcs->prepare: NULL);
> }
> EXPORT_SYMBOL(drm_panel_prepare);
>
> @@ -192,14 +226,19 @@ EXPORT_SYMBOL(drm_panel_prepare);
> * and the backlight to be enabled. Content will be visible on screen after
> * this call completes.
> *
> + * Atomic framework will ensure that enable/disable are properly balanced.
> + * If this is not the case, a WARNING will be issued.
> + *
> * Return: 0 on success or a negative error code on failure.
> */
> int drm_panel_enable(struct drm_panel *panel)
> {
> - if (panel && panel->funcs && panel->funcs->enable)
> - return panel->funcs->enable(panel);
> + if (!panel)
> + return -EINVAL;
>
> - return panel ? -ENOSYS : -EINVAL;
> + return drm_panel_change_state(panel, DRM_PANEL_POWER_PREPARED,
> + DRM_PANEL_POWER_ENABLED,
> + panel->funcs ? panel->funcs->enable: NULL);
> }
> EXPORT_SYMBOL(drm_panel_enable);
>
> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> index 0a9442069d3c..7e6a552005d7 100644
> --- a/include/drm/drm_panel.h
> +++ b/include/drm/drm_panel.h
> @@ -77,6 +77,18 @@ struct drm_panel_funcs {
> struct display_timing *timings);
> };
>
> +/**
> + * enum drm_panel_state - describes the state of a panel
> + * @DRM_PANEL_POWER_OFF: panel is currently off (accessible from PREPARED)
> + * @DRM_PANEL_POWER_PREPARED: panel is prepared (accessible from OFF || ENABLED)
> + * @DRM_PANEL_POWER_ENABLED: panel is enabled (accessible from PREPARED)
> + */
> +enum drm_panel_state {
> + DRM_PANEL_POWER_OFF,
> + DRM_PANEL_POWER_PREPARED,
> + DRM_PANEL_POWER_ENABLED
> +};
> +
> /**
> * struct drm_panel - DRM panel object
> * @drm: DRM device owning the panel
> @@ -84,6 +96,7 @@ struct drm_panel_funcs {
> * @dev: parent device of the panel
> * @funcs: operations that can be performed on the panel
> * @list: panel entry in registry
> + * @state: keeps track of the panel power status
> */
> struct drm_panel {
> struct drm_device *drm;
> @@ -93,6 +106,8 @@ struct drm_panel {
> const struct drm_panel_funcs *funcs;
>
> struct list_head list;
> +
> + enum drm_panel_state state;
> };
>
> /**
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
