On Thu, Oct 12, 2017 at 01:55:28PM -0400, 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
>
> Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
I really like this. Bunch of (hopefully) non-jetlagged comments below.
With those address (obviously for all the functions, I only commented on
drm_panel_unprepare):
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
> drivers/gpu/drm/drm_panel.c | 1 +
> include/drm/drm_panel.h | 60 +++++++++++++++++++++++++++++++++++++++------
> 2 files changed, 53 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index 308d442a531b..e5957e7da768 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -48,6 +48,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);
>
> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> index 14ac240a1f64..425461c4c574 100644
> --- a/include/drm/drm_panel.h
> +++ b/include/drm/drm_panel.h
> @@ -24,6 +24,7 @@
> #ifndef __DRM_PANEL_H__
> #define __DRM_PANEL_H__
>
> +#include <linux/bug.h>
> #include <linux/errno.h>
> #include <linux/list.h>
>
> @@ -84,6 +85,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
> + * @power_state: keeps track of the panel power status
> */
> struct drm_panel {
> struct drm_device *drm;
> @@ -93,6 +95,12 @@ struct drm_panel {
> const struct drm_panel_funcs *funcs;
>
> struct list_head list;
> +
> + enum {
> + DRM_PANEL_POWER_OFF,
> + DRM_PANEL_POWER_PREPARED,
> + DRM_PANEL_POWER_ENABLED
> + } state;
> };
>
> /**
> @@ -104,12 +112,21 @@ struct drm_panel {
> * is usually no longer possible to communicate with the panel until another
> * call to drm_panel_prepare().
> *
> + * Atomic framework should ensure that prepare/unprepare are properly balanced.
> + * If this is not the case, a WARNING will be issued.
s/should/will/ to make it slightly more authoritative sounding?
> + *
> * Return: 0 on success or a negative error code on failure.
> */
> static inline int drm_panel_unprepare(struct drm_panel *panel)
Given that the functions have grown quite a bit I'm not sure it's
reasonable to have them as static inlines anymore ...
> {
> - if (panel && panel->funcs && panel->funcs->unprepare)
> - return panel->funcs->unprepare(panel);
> + WARN_ON(panel->state != DRM_PANEL_POWER_PREPARED);
> +
> + if (panel && panel->funcs && panel->funcs->unprepare) {
> + int ret = panel->funcs->unprepare(panel);
> + if (!ret)
> + panel->state = DRM_PANEL_POWER_OFF;
Do you really want to only update the status if the driver provides a
callback? I think this should be moved out of the if
(panel->funcs->disable), but not ouf of the if (panel).
Could probably make the if (panel) an early return, for clean code.
-Daniel
> + return ret;
> + }
>
> return panel ? -ENOSYS : -EINVAL;
> }
> @@ -122,12 +139,21 @@ static inline int drm_panel_unprepare(struct drm_panel *panel)
> * drivers. For smart panels it should still be possible to communicate with
> * the integrated circuitry via any command bus after this call.
> *
> + * Atomic framework should 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.
> */
> static inline int drm_panel_disable(struct drm_panel *panel)
> {
> - if (panel && panel->funcs && panel->funcs->disable)
> - return panel->funcs->disable(panel);
> + WARN_ON(panel->state != DRM_PANEL_POWER_ENABLED);
> +
> + if (panel && panel->funcs && panel->funcs->disable) {
> + int ret = panel->funcs->disable(panel);
> + if (!ret)
> + panel->state = DRM_PANEL_POWER_PREPARED;
> + return ret;
> + }
>
> return panel ? -ENOSYS : -EINVAL;
> }
> @@ -140,12 +166,21 @@ static inline int drm_panel_disable(struct drm_panel *panel)
> * the panel. After this has completed it is possible to communicate with any
> * integrated circuitry via a command bus.
> *
> + * Atomic framework should 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.
> */
> static inline int drm_panel_prepare(struct drm_panel *panel)
> {
> - if (panel && panel->funcs && panel->funcs->prepare)
> - return panel->funcs->prepare(panel);
> + WARN_ON(panel->state != DRM_PANEL_POWER_OFF);
> +
> + if (panel && panel->funcs && panel->funcs->prepare) {
> + int ret = panel->funcs->prepare(panel);
> + if (!ret)
> + panel->state = DRM_PANEL_POWER_PREPARED;
> + return ret;
> + }
>
> return panel ? -ENOSYS : -EINVAL;
> }
> @@ -158,12 +193,21 @@ static inline int drm_panel_prepare(struct drm_panel *panel)
> * and the backlight to be enabled. Content will be visible on screen after
> * this call completes.
> *
> + * Atomic framework should 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.
> */
> static inline int drm_panel_enable(struct drm_panel *panel)
> {
> - if (panel && panel->funcs && panel->funcs->enable)
> - return panel->funcs->enable(panel);
> + WARN_ON(panel->state != DRM_PANEL_POWER_PREPARED);
> +
> + if (panel && panel->funcs && panel->funcs->enable) {
> + int ret = panel->funcs->enable(panel);
> + if (!ret)
> + panel->state = DRM_PANEL_POWER_ENABLED;
> + return ret;
> + }
>
> return panel ? -ENOSYS : -EINVAL;
> }
> --
> 2.15.0.rc0.271.g36b669edcc-goog
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
