Re: [PATCH 01/10] drm/panel: Keep track of enabled/prepared

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux