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

[Andrzej Hajda]

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 at 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;
>>  }
>