Re: [PATCH v3 03/12] drm/panel: Keep track of enabled/prepared

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

 



On 17.10.2017 23:13, 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
>
> Changes in v3:
>  - Functions no longer static inline (Daniel)
>  - Added a helper function to share state change code (Andrzej)
>  - Only warn when a panel is present (Daniel)
>  - Update state if function is not implemented (Andrzej)
>
> Cc: Andrzej Hajda <a.hajda@xxxxxxxxxxx>
> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/drm_panel.c | 63 ++++++++++++++++++++++++++++++++++++---------
>  include/drm/drm_panel.h     | 15 +++++++++++
>  2 files changed, 66 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index 6f28598a5d0e..6547850b5e80 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -21,6 +21,7 @@
>   * DEALINGS IN THE SOFTWARE.
>   */
>  
> +#include <linux/bug.h>
>  #include <linux/err.h>
>  #include <linux/module.h>
>  
> @@ -48,6 +49,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);
>  
> @@ -126,6 +128,23 @@ int drm_panel_detach(struct drm_panel *panel)
>  }
>  EXPORT_SYMBOL(drm_panel_detach);
>  
> +static int drm_panel_change_state(struct drm_panel *panel,
> +				  enum drm_panel_state cur_state,
> +				  enum drm_panel_state new_state,
> +				  int (*hook)(struct drm_panel *panel))
> +{
> +	int ret;
> +
> +	WARN_ON(panel->state != cur_state);
> +	if (hook)
> +		ret = hook(panel);
> +	else
> +		ret = -ENOSYS;

You have missed my 2 comments:
- do not change state on callback error,
- return 0 in case there is no callback.

Regards
Andrzej

> +
> +	panel->state = new_state;
> +	return ret;
> +}
> +
>  /**
>   * drm_panel_unprepare - power off a panel
>   * @panel: DRM panel
> @@ -135,14 +154,19 @@ EXPORT_SYMBOL(drm_panel_detach);
>   * is usually no longer possible to communicate with the panel until another
>   * call to drm_panel_prepare().
>   *
> + * Atomic framework will 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.
>   */
>  int drm_panel_unprepare(struct drm_panel *panel)
>  {
> -	if (panel && panel->funcs && panel->funcs->unprepare)
> -		return panel->funcs->unprepare(panel);
> +	if (!panel)
> +		return -EINVAL;
>  
> -	return panel ? -ENOSYS : -EINVAL;
> +	return drm_panel_change_state(panel, DRM_PANEL_POWER_PREPARED,
> +		      DRM_PANEL_POWER_OFF,
> +		      panel->funcs ? panel->funcs->unprepare : NULL);
>  }
>  EXPORT_SYMBOL(drm_panel_unprepare);
>  
> @@ -154,14 +178,19 @@ EXPORT_SYMBOL(drm_panel_unprepare);
>   * drivers. For smart panels it should still be possible to communicate with
>   * the integrated circuitry via any command bus after this call.
>   *
> + * Atomic framework will 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.
>   */
>  int drm_panel_disable(struct drm_panel *panel)
>  {
> -	if (panel && panel->funcs && panel->funcs->disable)
> -		return panel->funcs->disable(panel);
> +	if (!panel)
> +		return -EINVAL;
>  
> -	return panel ? -ENOSYS : -EINVAL;
> +	return drm_panel_change_state(panel, DRM_PANEL_POWER_ENABLED,
> +		      DRM_PANEL_POWER_PREPARED,
> +		      panel->funcs ? panel->funcs->disable : NULL);
>  }
>  EXPORT_SYMBOL(drm_panel_disable);
>  
> @@ -173,14 +202,19 @@ EXPORT_SYMBOL(drm_panel_disable);
>   * the panel. After this has completed it is possible to communicate with any
>   * integrated circuitry via a command bus.
>   *
> + * Atomic framework will 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.
>   */
>  int drm_panel_prepare(struct drm_panel *panel)
>  {
> -	if (panel && panel->funcs && panel->funcs->prepare)
> -		return panel->funcs->prepare(panel);
> +	if (!panel)
> +		return -EINVAL;
>  
> -	return panel ? -ENOSYS : -EINVAL;
> +	return drm_panel_change_state(panel, DRM_PANEL_POWER_OFF,
> +		      DRM_PANEL_POWER_PREPARED,
> +		      panel->funcs ? panel->funcs->prepare: NULL);
>  }
>  EXPORT_SYMBOL(drm_panel_prepare);
>  
> @@ -192,14 +226,19 @@ EXPORT_SYMBOL(drm_panel_prepare);
>   * and the backlight to be enabled. Content will be visible on screen after
>   * this call completes.
>   *
> + * Atomic framework will 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.
>   */
>  int drm_panel_enable(struct drm_panel *panel)
>  {
> -	if (panel && panel->funcs && panel->funcs->enable)
> -		return panel->funcs->enable(panel);
> +	if (!panel)
> +		return -EINVAL;
>  
> -	return panel ? -ENOSYS : -EINVAL;
> +	return drm_panel_change_state(panel, DRM_PANEL_POWER_PREPARED,
> +		      DRM_PANEL_POWER_ENABLED,
> +		      panel->funcs ? panel->funcs->enable: NULL);
>  }
>  EXPORT_SYMBOL(drm_panel_enable);
>  
> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> index 0a9442069d3c..7e6a552005d7 100644
> --- a/include/drm/drm_panel.h
> +++ b/include/drm/drm_panel.h
> @@ -77,6 +77,18 @@ struct drm_panel_funcs {
>  			   struct display_timing *timings);
>  };
>  
> +/**
> + * enum drm_panel_state - describes the state of a panel
> + * @DRM_PANEL_POWER_OFF: panel is currently off (accessible from PREPARED)
> + * @DRM_PANEL_POWER_PREPARED: panel is prepared (accessible from OFF || ENABLED)
> + * @DRM_PANEL_POWER_ENABLED: panel is enabled  (accessible from PREPARED)
> + */
> +enum drm_panel_state {
> +	DRM_PANEL_POWER_OFF,
> +	DRM_PANEL_POWER_PREPARED,
> +	DRM_PANEL_POWER_ENABLED
> +};
> +
>  /**
>   * struct drm_panel - DRM panel object
>   * @drm: DRM device owning the panel
> @@ -84,6 +96,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
> + * @state: keeps track of the panel power status
>   */
>  struct drm_panel {
>  	struct drm_device *drm;
> @@ -93,6 +106,8 @@ struct drm_panel {
>  	const struct drm_panel_funcs *funcs;
>  
>  	struct list_head list;
> +
> +	enum drm_panel_state state;
>  };
>  
>  /**


_______________________________________________
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