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@xxxxxxxxxxxx>
>
> 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@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
