| Message ID | 20170921170638.27031-2-seanpaul@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 21.09.2017 19:06, 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. > > Signed-off-by: Sean Paul <seanpaul@chromium.org> I wonder if the tracking is needed at all, panels power states are usually the same as states of encoders they are connected to. But it is just remark to consider, no strong opposition :) > --- > drivers/gpu/drm/drm_panel.c | 2 ++ > include/drm/drm_panel.h | 38 ++++++++++++++++++++++++++++++++++---- > 2 files changed, 36 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > index 308d442a531b..9515219d3d2c 100644 > --- a/drivers/gpu/drm/drm_panel.c > +++ b/drivers/gpu/drm/drm_panel.c > @@ -48,6 +48,8 @@ static LIST_HEAD(panel_list); > void drm_panel_init(struct drm_panel *panel) > { > INIT_LIST_HEAD(&panel->list); > + panel->enabled = false; > + panel->prepared = false; > } > EXPORT_SYMBOL(drm_panel_init); > > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h > index 14ac240a1f64..b9a86a4cf29c 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,8 @@ struct drm_panel_funcs { > * @dev: parent device of the panel > * @funcs: operations that can be performed on the panel > * @list: panel entry in registry > + * @enabled: keeps track of the panel enabled status > + * @prepared: keeps track of the panel prepared status > */ > struct drm_panel { > struct drm_device *drm; > @@ -93,6 +96,9 @@ struct drm_panel { > const struct drm_panel_funcs *funcs; > > struct list_head list; > + > + bool enabled; > + bool prepared; > }; > > /** > @@ -104,12 +110,18 @@ 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) > + if (panel && panel->funcs && panel->funcs->unprepare) { > + WARN_ON(!panel->prepared); WARN_ON(!panel->prepared || panel->enabled); Similar double checks should be used in other places. > + panel->prepared = false; > return panel->funcs->unprepare(panel); ret = panel->funcs->unprepare(panel); if (!ret) panel->prepared = false; return ret; Again this pattern should be repeated in other places. Different thing is meaning of unsuccessful unprepare/disable? But this is other issue. > + } > > return panel ? -ENOSYS : -EINVAL; I think tracking should be performed also for no-op case. Regards Andrzej > } > @@ -122,12 +134,18 @@ 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) > + if (panel && panel->funcs && panel->funcs->disable) { > + WARN_ON(!panel->enabled); > + panel->enabled = false; > return panel->funcs->disable(panel); > + } > > return panel ? -ENOSYS : -EINVAL; > } > @@ -140,12 +158,18 @@ 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) > + if (panel && panel->funcs && panel->funcs->prepare) { > + WARN_ON(panel->prepared); > + panel->prepared = true; > return panel->funcs->prepare(panel); > + } > > return panel ? -ENOSYS : -EINVAL; > } > @@ -158,12 +182,18 @@ 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) > + if (panel && panel->funcs && panel->funcs->enable) { > + WARN_ON(panel->enabled); > + panel->enabled = true; > return panel->funcs->enable(panel); > + } > > return panel ? -ENOSYS : -EINVAL; > }
On 22.09.2017 09:13, Andrzej Hajda wrote: > On 21.09.2017 19:06, 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. >> >> Signed-off-by: Sean Paul <seanpaul@chromium.org> > I wonder if the tracking is needed at all, panels power states are > usually the same as states of encoders they are connected to. > But it is just remark to consider, no strong opposition :) > >> --- >> drivers/gpu/drm/drm_panel.c | 2 ++ >> include/drm/drm_panel.h | 38 ++++++++++++++++++++++++++++++++++---- >> 2 files changed, 36 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c >> index 308d442a531b..9515219d3d2c 100644 >> --- a/drivers/gpu/drm/drm_panel.c >> +++ b/drivers/gpu/drm/drm_panel.c >> @@ -48,6 +48,8 @@ static LIST_HEAD(panel_list); >> void drm_panel_init(struct drm_panel *panel) >> { >> INIT_LIST_HEAD(&panel->list); >> + panel->enabled = false; >> + panel->prepared = false; >> } >> EXPORT_SYMBOL(drm_panel_init); >> >> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h >> index 14ac240a1f64..b9a86a4cf29c 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,8 @@ struct drm_panel_funcs { >> * @dev: parent device of the panel >> * @funcs: operations that can be performed on the panel >> * @list: panel entry in registry >> + * @enabled: keeps track of the panel enabled status >> + * @prepared: keeps track of the panel prepared status >> */ >> struct drm_panel { >> struct drm_device *drm; >> @@ -93,6 +96,9 @@ struct drm_panel { >> const struct drm_panel_funcs *funcs; >> >> struct list_head list; >> + >> + bool enabled; >> + bool prepared; I think better would be to use enum {disabled, prepared, enabled} here, the checks will be more readable, and will fit better to panel state machine :), just to consider. Regards Andrzej >> }; >> >> /** >> @@ -104,12 +110,18 @@ 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) >> + if (panel && panel->funcs && panel->funcs->unprepare) { >> + WARN_ON(!panel->prepared); > WARN_ON(!panel->prepared || panel->enabled); > > Similar double checks should be used in other places. > >> + panel->prepared = false; >> return panel->funcs->unprepare(panel); > ret = panel->funcs->unprepare(panel); > if (!ret) > panel->prepared = false; > return ret; > > Again this pattern should be repeated in other places. > > Different thing is meaning of unsuccessful unprepare/disable? But this > is other issue. > >> + } >> >> return panel ? -ENOSYS : -EINVAL; > I think tracking should be performed also for no-op case. > > Regards > Andrzej > >> } >> @@ -122,12 +134,18 @@ 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) >> + if (panel && panel->funcs && panel->funcs->disable) { >> + WARN_ON(!panel->enabled); >> + panel->enabled = false; >> return panel->funcs->disable(panel); >> + } >> >> return panel ? -ENOSYS : -EINVAL; >> } >> @@ -140,12 +158,18 @@ 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) >> + if (panel && panel->funcs && panel->funcs->prepare) { >> + WARN_ON(panel->prepared); >> + panel->prepared = true; >> return panel->funcs->prepare(panel); >> + } >> >> return panel ? -ENOSYS : -EINVAL; >> } >> @@ -158,12 +182,18 @@ 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) >> + if (panel && panel->funcs && panel->funcs->enable) { >> + WARN_ON(panel->enabled); >> + panel->enabled = true; >> return panel->funcs->enable(panel); >> + } >> >> return panel ? -ENOSYS : -EINVAL; >> } >
Andrzej Hajda <a.hajda@samsung.com> writes: > On 21.09.2017 19:06, 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. >> >> Signed-off-by: Sean Paul <seanpaul@chromium.org> > > I wonder if the tracking is needed at all, panels power states are > usually the same as states of encoders they are connected to. > But it is just remark to consider, no strong opposition :) > >> --- >> drivers/gpu/drm/drm_panel.c | 2 ++ >> include/drm/drm_panel.h | 38 ++++++++++++++++++++++++++++++++++---- >> 2 files changed, 36 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c >> index 308d442a531b..9515219d3d2c 100644 >> --- a/drivers/gpu/drm/drm_panel.c >> +++ b/drivers/gpu/drm/drm_panel.c >> @@ -48,6 +48,8 @@ static LIST_HEAD(panel_list); >> void drm_panel_init(struct drm_panel *panel) >> { >> INIT_LIST_HEAD(&panel->list); >> + panel->enabled = false; >> + panel->prepared = false; >> } >> EXPORT_SYMBOL(drm_panel_init); >> >> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h >> index 14ac240a1f64..b9a86a4cf29c 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,8 @@ struct drm_panel_funcs { >> * @dev: parent device of the panel >> * @funcs: operations that can be performed on the panel >> * @list: panel entry in registry >> + * @enabled: keeps track of the panel enabled status >> + * @prepared: keeps track of the panel prepared status >> */ >> struct drm_panel { >> struct drm_device *drm; >> @@ -93,6 +96,9 @@ struct drm_panel { >> const struct drm_panel_funcs *funcs; >> >> struct list_head list; >> + >> + bool enabled; >> + bool prepared; >> }; >> >> /** >> @@ -104,12 +110,18 @@ 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) >> + if (panel && panel->funcs && panel->funcs->unprepare) { >> + WARN_ON(!panel->prepared); > > WARN_ON(!panel->prepared || panel->enabled); > > Similar double checks should be used in other places. I like this idea! I feel like I've seen enough drivers that didn't balance prepare/unprepare and enable/disable because for their one panel they supported it didn't matter.
On Fri, Sep 22, 2017 at 09:13:53AM +0200, Andrzej Hajda wrote: > On 21.09.2017 19:06, 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. > > > > Signed-off-by: Sean Paul <seanpaul@chromium.org> > > I wonder if the tracking is needed at all, panels power states are > usually the same as states of encoders they are connected to. > But it is just remark to consider, no strong opposition :) The tracking is just so that the drm-panel wrappers can appropriately WARN_ON for drivers which get this wrong. Which I think makes sense, since drm-panel needs to be glued into the driver manually in many cases (anyone not using panel-bridge really). I'd ack the entire series, but I'm still suffering from jetlag so this is probably not a good idea yet. If it's not yet reviewed, feel free to ping me in a few days ... -Daniel > > > --- > > drivers/gpu/drm/drm_panel.c | 2 ++ > > include/drm/drm_panel.h | 38 ++++++++++++++++++++++++++++++++++---- > > 2 files changed, 36 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c > > index 308d442a531b..9515219d3d2c 100644 > > --- a/drivers/gpu/drm/drm_panel.c > > +++ b/drivers/gpu/drm/drm_panel.c > > @@ -48,6 +48,8 @@ static LIST_HEAD(panel_list); > > void drm_panel_init(struct drm_panel *panel) > > { > > INIT_LIST_HEAD(&panel->list); > > + panel->enabled = false; > > + panel->prepared = false; > > } > > EXPORT_SYMBOL(drm_panel_init); > > > > diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h > > index 14ac240a1f64..b9a86a4cf29c 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,8 @@ struct drm_panel_funcs { > > * @dev: parent device of the panel > > * @funcs: operations that can be performed on the panel > > * @list: panel entry in registry > > + * @enabled: keeps track of the panel enabled status > > + * @prepared: keeps track of the panel prepared status > > */ > > struct drm_panel { > > struct drm_device *drm; > > @@ -93,6 +96,9 @@ struct drm_panel { > > const struct drm_panel_funcs *funcs; > > > > struct list_head list; > > + > > + bool enabled; > > + bool prepared; > > }; > > > > /** > > @@ -104,12 +110,18 @@ 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) > > + if (panel && panel->funcs && panel->funcs->unprepare) { > > + WARN_ON(!panel->prepared); > > WARN_ON(!panel->prepared || panel->enabled); > > Similar double checks should be used in other places. > > > + panel->prepared = false; > > return panel->funcs->unprepare(panel); > > ret = panel->funcs->unprepare(panel); > if (!ret) > panel->prepared = false; > return ret; > > Again this pattern should be repeated in other places. > > Different thing is meaning of unsuccessful unprepare/disable? But this > is other issue. > > > + } > > > > return panel ? -ENOSYS : -EINVAL; > > I think tracking should be performed also for no-op case. > > Regards > Andrzej > > > } > > @@ -122,12 +134,18 @@ 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) > > + if (panel && panel->funcs && panel->funcs->disable) { > > + WARN_ON(!panel->enabled); > > + panel->enabled = false; > > return panel->funcs->disable(panel); > > + } > > > > return panel ? -ENOSYS : -EINVAL; > > } > > @@ -140,12 +158,18 @@ 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) > > + if (panel && panel->funcs && panel->funcs->prepare) { > > + WARN_ON(panel->prepared); > > + panel->prepared = true; > > return panel->funcs->prepare(panel); > > + } > > > > return panel ? -ENOSYS : -EINVAL; > > } > > @@ -158,12 +182,18 @@ 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) > > + if (panel && panel->funcs && panel->funcs->enable) { > > + WARN_ON(panel->enabled); > > + panel->enabled = true; > > return panel->funcs->enable(panel); > > + } > > > > return panel ? -ENOSYS : -EINVAL; > > } > > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c index 308d442a531b..9515219d3d2c 100644 --- a/drivers/gpu/drm/drm_panel.c +++ b/drivers/gpu/drm/drm_panel.c @@ -48,6 +48,8 @@ static LIST_HEAD(panel_list); void drm_panel_init(struct drm_panel *panel) { INIT_LIST_HEAD(&panel->list); + panel->enabled = false; + panel->prepared = false; } EXPORT_SYMBOL(drm_panel_init); diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h index 14ac240a1f64..b9a86a4cf29c 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,8 @@ struct drm_panel_funcs { * @dev: parent device of the panel * @funcs: operations that can be performed on the panel * @list: panel entry in registry + * @enabled: keeps track of the panel enabled status + * @prepared: keeps track of the panel prepared status */ struct drm_panel { struct drm_device *drm; @@ -93,6 +96,9 @@ struct drm_panel { const struct drm_panel_funcs *funcs; struct list_head list; + + bool enabled; + bool prepared; }; /** @@ -104,12 +110,18 @@ 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) + if (panel && panel->funcs && panel->funcs->unprepare) { + WARN_ON(!panel->prepared); + panel->prepared = false; return panel->funcs->unprepare(panel); + } return panel ? -ENOSYS : -EINVAL; } @@ -122,12 +134,18 @@ 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) + if (panel && panel->funcs && panel->funcs->disable) { + WARN_ON(!panel->enabled); + panel->enabled = false; return panel->funcs->disable(panel); + } return panel ? -ENOSYS : -EINVAL; } @@ -140,12 +158,18 @@ 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) + if (panel && panel->funcs && panel->funcs->prepare) { + WARN_ON(panel->prepared); + panel->prepared = true; return panel->funcs->prepare(panel); + } return panel ? -ENOSYS : -EINVAL; } @@ -158,12 +182,18 @@ 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) + if (panel && panel->funcs && panel->funcs->enable) { + WARN_ON(panel->enabled); + panel->enabled = true; return panel->funcs->enable(panel); + } 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. Signed-off-by: Sean Paul <seanpaul@chromium.org> --- drivers/gpu/drm/drm_panel.c | 2 ++ include/drm/drm_panel.h | 38 ++++++++++++++++++++++++++++++++++---- 2 files changed, 36 insertions(+), 4 deletions(-)