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@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 :)
>
>> ---
>> 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;
>> }
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
