| Message ID | 20171012175546.21323-2-seanpaul@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@samsung.com> > Signed-off-by: Sean Paul <seanpaul@chromium.org> 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@ffwll.ch> > --- > 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 12.10.2017 19:55, 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@samsung.com> > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > 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. > + * > * Return: 0 on success or a negative error code on failure. > */ > static inline int drm_panel_unprepare(struct drm_panel *panel) > { > - 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; > + return ret; > + } > > return panel ? -ENOSYS : -EINVAL; State check and state change should be performed also for panels without callbacks, otherwise it will work only with panels having full set of callbacks. Another thing is that missing callback does not mean function-not-implemented (ENOSYS), it is rather nothing-to-do, in such case function should return rather 0, this will simplify also error handling on callers side. And one thing just to consider: all functions have almost the same body, maybe it would be good to use some helper, for example: #define drm_panel_state_change(panel, from, to, func) ... int drm_panel_unprepare(struct drm_panel *panel) { return drm_panel_state_change(panel, DRM_PANEL_POWER_PREPARED, DRM_PANEL_POWER_OFF, unprepare); } Regards Andrzej > } > @@ -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; > }
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. + * * Return: 0 on success or a negative error code on failure. */ static inline int drm_panel_unprepare(struct drm_panel *panel) { - 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; + 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; }
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@samsung.com> Signed-off-by: Sean Paul <seanpaul@chromium.org> --- drivers/gpu/drm/drm_panel.c | 1 + include/drm/drm_panel.h | 60 +++++++++++++++++++++++++++++++++++++++------ 2 files changed, 53 insertions(+), 8 deletions(-)