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

[Eric Anholt <eric@anholt.net>]

[-- Attachment #1.1: Type: text/plain, Size: 3432 bytes --]

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel