[PATCH v2 00/10] drm/panel: Remove unnecessary enabled/prepared state

[PATCH v2 00/10] drm/panel: Remove unnecessary enabled/prepared state

[Sean Paul]

A number of panel drivers track enabled/prepared state (I suspect to protect
regulator refcounts). However, the atomic framework already ensures that
prepare/unprepare and enable/disable calls are balanced. This series removes all
independent tracking from the drivers and adds a WARNING to the core in case
someone uses a panel with a legacy driver.

Changes in v2:
	- Addressed review comments in first patch
	- Since the initial set didn't get much action, this is a partial RESEND


Sean Paul (10):
  drm/panel: Keep track of enabled/prepared
  drm/panel: vvx10f034n00: Remove enabled/prepared state
  drm/panel: lt070me05000: Remove enabled/prepared state
  drm/panel: lq101r1sx01: Remove enabled/prepared state
  drm/panel: otm8009a: Remove enabled state
  drm/panel: otm8009a: Properly sequence [un]prepare with backlight
  drm/panel: 43wvf1g: Remove enabled/prepared state
  drm/panel: simple: Remove enabled/prepared state
  drm/panel: p079zca: Remove enabled/prepared state
  drm/panel: ls043t1le01: Remove enabled/prepared state

 drivers/gpu/drm/drm_panel.c                        |  1 +
 drivers/gpu/drm/panel/panel-innolux-p079zca.c      | 23 ---------
 drivers/gpu/drm/panel/panel-jdi-lt070me05000.c     | 23 ---------
 drivers/gpu/drm/panel/panel-orisetech-otm8009a.c   | 59 +++++++++++----------
 .../gpu/drm/panel/panel-panasonic-vvx10f034n00.c   | 22 --------
 drivers/gpu/drm/panel/panel-seiko-43wvf1g.c        | 24 ---------
 drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c    | 23 ---------
 drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c    | 23 ---------
 drivers/gpu/drm/panel/panel-simple.c               | 24 ---------
 include/drm/drm_panel.h                            | 60 +++++++++++++++++++---
 10 files changed, 82 insertions(+), 200 deletions(-)

-- 
2.15.0.rc0.271.g36b669edcc-goog

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

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

[Sean Paul]

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

Cc: Andrzej Hajda <a.hajda@samsung.com>
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/drm_panel.c |  1 +
 include/drm/drm_panel.h     | 60 +++++++++++++++++++++++++++++++++++++++------
 2 files changed, 53 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 308d442a531b..e5957e7da768 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -48,6 +48,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);
 
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index 14ac240a1f64..425461c4c574 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,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
+ * @power_state: keeps track of the panel power status
  */
 struct drm_panel {
 	struct drm_device *drm;
@@ -93,6 +95,12 @@ struct drm_panel {
 	const struct drm_panel_funcs *funcs;
 
 	struct list_head list;
+
+	enum {
+		DRM_PANEL_POWER_OFF,
+		DRM_PANEL_POWER_PREPARED,
+		DRM_PANEL_POWER_ENABLED
+	} state;
 };
 
 /**
@@ -104,12 +112,21 @@ 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)
-		return panel->funcs->unprepare(panel);
+	WARN_ON(panel->state != DRM_PANEL_POWER_PREPARED);
+
+	if (panel && panel->funcs && panel->funcs->unprepare) {
+		int ret = panel->funcs->unprepare(panel);
+		if (!ret)
+			panel->state = DRM_PANEL_POWER_OFF;
+		return ret;
+	}
 
 	return panel ? -ENOSYS : -EINVAL;
 }
@@ -122,12 +139,21 @@ 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)
-		return panel->funcs->disable(panel);
+	WARN_ON(panel->state != DRM_PANEL_POWER_ENABLED);
+
+	if (panel && panel->funcs && panel->funcs->disable) {
+		int ret = panel->funcs->disable(panel);
+		if (!ret)
+			panel->state = DRM_PANEL_POWER_PREPARED;
+		return ret;
+	}
 
 	return panel ? -ENOSYS : -EINVAL;
 }
@@ -140,12 +166,21 @@ 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)
-		return panel->funcs->prepare(panel);
+	WARN_ON(panel->state != DRM_PANEL_POWER_OFF);
+
+	if (panel && panel->funcs && panel->funcs->prepare) {
+		int ret = panel->funcs->prepare(panel);
+		if (!ret)
+			panel->state = DRM_PANEL_POWER_PREPARED;
+		return ret;
+	}
 
 	return panel ? -ENOSYS : -EINVAL;
 }
@@ -158,12 +193,21 @@ 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)
-		return panel->funcs->enable(panel);
+	WARN_ON(panel->state != DRM_PANEL_POWER_PREPARED);
+
+	if (panel && panel->funcs && panel->funcs->enable) {
+		int ret = panel->funcs->enable(panel);
+		if (!ret)
+			panel->state = DRM_PANEL_POWER_ENABLED;
+		return ret;
+	}
 
 	return panel ? -ENOSYS : -EINVAL;
 }
-- 
2.15.0.rc0.271.g36b669edcc-goog

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

[PATCH v2 02/10] drm/panel: vvx10f034n00: Remove enabled/prepared state

[Sean Paul]

They're not necessary for atomic drivers, and drm_panel will WARN if
the calls are unbalanced.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 .../gpu/drm/panel/panel-panasonic-vvx10f034n00.c   | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c b/drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c
index 7f915f706fa6..e7efa097151c 100644
--- a/drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c
+++ b/drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c
@@ -44,9 +44,6 @@ struct wuxga_nt_panel {
 	struct backlight_device *backlight;
 	struct regulator *supply;
 
-	bool prepared;
-	bool enabled;
-
 	ktime_t earliest_wake;
 
 	const struct drm_display_mode *mode;
@@ -73,9 +70,6 @@ static int wuxga_nt_panel_disable(struct drm_panel *panel)
 {
 	struct wuxga_nt_panel *wuxga_nt = to_wuxga_nt_panel(panel);
 
-	if (!wuxga_nt->enabled)
-		return 0;
-
 	mipi_dsi_shutdown_peripheral(wuxga_nt->dsi);
 
 	if (wuxga_nt->backlight) {
@@ -84,8 +78,6 @@ static int wuxga_nt_panel_disable(struct drm_panel *panel)
 		backlight_update_status(wuxga_nt->backlight);
 	}
 
-	wuxga_nt->enabled = false;
-
 	return 0;
 }
 
@@ -93,12 +85,8 @@ static int wuxga_nt_panel_unprepare(struct drm_panel *panel)
 {
 	struct wuxga_nt_panel *wuxga_nt = to_wuxga_nt_panel(panel);
 
-	if (!wuxga_nt->prepared)
-		return 0;
-
 	regulator_disable(wuxga_nt->supply);
 	wuxga_nt->earliest_wake = ktime_add_ms(ktime_get_real(), MIN_POFF_MS);
-	wuxga_nt->prepared = false;
 
 	return 0;
 }
@@ -109,9 +97,6 @@ static int wuxga_nt_panel_prepare(struct drm_panel *panel)
 	int ret;
 	s64 enablewait;
 
-	if (wuxga_nt->prepared)
-		return 0;
-
 	/*
 	 * If the user re-enabled the panel before the required off-time then
 	 * we need to wait the remaining period before re-enabling regulator
@@ -141,8 +126,6 @@ static int wuxga_nt_panel_prepare(struct drm_panel *panel)
 		goto poweroff;
 	}
 
-	wuxga_nt->prepared = true;
-
 	return 0;
 
 poweroff:
@@ -155,17 +138,12 @@ static int wuxga_nt_panel_enable(struct drm_panel *panel)
 {
 	struct wuxga_nt_panel *wuxga_nt = to_wuxga_nt_panel(panel);
 
-	if (wuxga_nt->enabled)
-		return 0;
-
 	if (wuxga_nt->backlight) {
 		wuxga_nt->backlight->props.power = FB_BLANK_UNBLANK;
 		wuxga_nt->backlight->props.state &= ~BL_CORE_FBBLANK;
 		backlight_update_status(wuxga_nt->backlight);
 	}
 
-	wuxga_nt->enabled = true;
-
 	return 0;
 }
 
-- 
2.15.0.rc0.271.g36b669edcc-goog

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

[PATCH v2 03/10] drm/panel: lt070me05000: Remove enabled/prepared state

[Sean Paul]

They're not necessary for atomic drivers, and drm_panel will
WARN if the calls are unbalanced.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/panel/panel-jdi-lt070me05000.c | 23 -----------------------
 1 file changed, 23 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
index 5b2340ef74ed..2f2455650258 100644
--- a/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
+++ b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
@@ -50,9 +50,6 @@ struct jdi_panel {
 	struct gpio_desc *dcdc_en_gpio;
 	struct backlight_device *backlight;
 
-	bool prepared;
-	bool enabled;
-
 	const struct drm_display_mode *mode;
 };
 
@@ -189,14 +186,9 @@ static int jdi_panel_disable(struct drm_panel *panel)
 {
 	struct jdi_panel *jdi = to_jdi_panel(panel);
 
-	if (!jdi->enabled)
-		return 0;
-
 	jdi->backlight->props.power = FB_BLANK_POWERDOWN;
 	backlight_update_status(jdi->backlight);
 
-	jdi->enabled = false;
-
 	return 0;
 }
 
@@ -206,9 +198,6 @@ static int jdi_panel_unprepare(struct drm_panel *panel)
 	struct device *dev = &jdi->dsi->dev;
 	int ret;
 
-	if (!jdi->prepared)
-		return 0;
-
 	jdi_panel_off(jdi);
 
 	ret = regulator_bulk_disable(ARRAY_SIZE(jdi->supplies), jdi->supplies);
@@ -221,8 +210,6 @@ static int jdi_panel_unprepare(struct drm_panel *panel)
 
 	gpiod_set_value(jdi->dcdc_en_gpio, 0);
 
-	jdi->prepared = false;
-
 	return 0;
 }
 
@@ -232,9 +219,6 @@ static int jdi_panel_prepare(struct drm_panel *panel)
 	struct device *dev = &jdi->dsi->dev;
 	int ret;
 
-	if (jdi->prepared)
-		return 0;
-
 	ret = regulator_bulk_enable(ARRAY_SIZE(jdi->supplies), jdi->supplies);
 	if (ret < 0) {
 		dev_err(dev, "regulator enable failed, %d\n", ret);
@@ -264,8 +248,6 @@ static int jdi_panel_prepare(struct drm_panel *panel)
 		goto poweroff;
 	}
 
-	jdi->prepared = true;
-
 	return 0;
 
 poweroff:
@@ -286,14 +268,9 @@ static int jdi_panel_enable(struct drm_panel *panel)
 {
 	struct jdi_panel *jdi = to_jdi_panel(panel);
 
-	if (jdi->enabled)
-		return 0;
-
 	jdi->backlight->props.power = FB_BLANK_UNBLANK;
 	backlight_update_status(jdi->backlight);
 
-	jdi->enabled = true;
-
 	return 0;
 }
 
-- 
2.15.0.rc0.271.g36b669edcc-goog

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

[PATCH v2 04/10] drm/panel: lq101r1sx01: Remove enabled/prepared state

[Sean Paul]

They're not necessary for atomic drivers, and drm_panel will
WARN if the calls are unbalanced.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c | 23 -----------------------
 1 file changed, 23 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
index 3cce3ca19601..9b447b0bfcd0 100644
--- a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
@@ -28,9 +28,6 @@ struct sharp_panel {
 	struct backlight_device *backlight;
 	struct regulator *supply;
 
-	bool prepared;
-	bool enabled;
-
 	const struct drm_display_mode *mode;
 };
 
@@ -93,16 +90,11 @@ static int sharp_panel_disable(struct drm_panel *panel)
 {
 	struct sharp_panel *sharp = to_sharp_panel(panel);
 
-	if (!sharp->enabled)
-		return 0;
-
 	if (sharp->backlight) {
 		sharp->backlight->props.power = FB_BLANK_POWERDOWN;
 		backlight_update_status(sharp->backlight);
 	}
 
-	sharp->enabled = false;
-
 	return 0;
 }
 
@@ -111,9 +103,6 @@ static int sharp_panel_unprepare(struct drm_panel *panel)
 	struct sharp_panel *sharp = to_sharp_panel(panel);
 	int err;
 
-	if (!sharp->prepared)
-		return 0;
-
 	sharp_wait_frames(sharp, 4);
 
 	err = mipi_dsi_dcs_set_display_off(sharp->link1);
@@ -128,8 +117,6 @@ static int sharp_panel_unprepare(struct drm_panel *panel)
 
 	regulator_disable(sharp->supply);
 
-	sharp->prepared = false;
-
 	return 0;
 }
 
@@ -173,9 +160,6 @@ static int sharp_panel_prepare(struct drm_panel *panel)
 	u8 format = MIPI_DCS_PIXEL_FMT_24BIT;
 	int err;
 
-	if (sharp->prepared)
-		return 0;
-
 	err = regulator_enable(sharp->supply);
 	if (err < 0)
 		return err;
@@ -244,8 +228,6 @@ static int sharp_panel_prepare(struct drm_panel *panel)
 		goto poweroff;
 	}
 
-	sharp->prepared = true;
-
 	/* wait for 6 frames before continuing */
 	sharp_wait_frames(sharp, 6);
 
@@ -260,16 +242,11 @@ static int sharp_panel_enable(struct drm_panel *panel)
 {
 	struct sharp_panel *sharp = to_sharp_panel(panel);
 
-	if (sharp->enabled)
-		return 0;
-
 	if (sharp->backlight) {
 		sharp->backlight->props.power = FB_BLANK_UNBLANK;
 		backlight_update_status(sharp->backlight);
 	}
 
-	sharp->enabled = true;
-
 	return 0;
 }
 
-- 
2.15.0.rc0.271.g36b669edcc-goog

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

[PATCH v2 05/10] drm/panel: otm8009a: Remove enabled state

[Sean Paul]

It's not necessary for atomic drivers, and drm_panel will
WARN if the calls are unbalanced.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/panel/panel-orisetech-otm8009a.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
index c189cd6329c8..0a5898fd4502 100644
--- a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
+++ b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
@@ -63,7 +63,6 @@ struct otm8009a {
 	struct backlight_device *bl_dev;
 	struct gpio_desc *reset_gpio;
 	bool prepared;
-	bool enabled;
 };
 
 static const struct drm_display_mode default_mode = {
@@ -243,9 +242,6 @@ static int otm8009a_disable(struct drm_panel *panel)
 	struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
 	int ret;
 
-	if (!ctx->enabled)
-		return 0; /* This is not an issue so we return 0 here */
-
 	/* Power off the backlight. Note: end-user still controls brightness */
 	ctx->bl_dev->props.power = FB_BLANK_POWERDOWN;
 	ret = backlight_update_status(ctx->bl_dev);
@@ -262,8 +258,6 @@ static int otm8009a_disable(struct drm_panel *panel)
 
 	msleep(120);
 
-	ctx->enabled = false;
-
 	return 0;
 }
 
@@ -316,15 +310,6 @@ static int otm8009a_prepare(struct drm_panel *panel)
 	return 0;
 }
 
-static int otm8009a_enable(struct drm_panel *panel)
-{
-	struct otm8009a *ctx = panel_to_otm8009a(panel);
-
-	ctx->enabled = true;
-
-	return 0;
-}
-
 static int otm8009a_get_modes(struct drm_panel *panel)
 {
 	struct drm_display_mode *mode;
@@ -352,7 +337,6 @@ static const struct drm_panel_funcs otm8009a_drm_funcs = {
 	.disable   = otm8009a_disable,
 	.unprepare = otm8009a_unprepare,
 	.prepare   = otm8009a_prepare,
-	.enable    = otm8009a_enable,
 	.get_modes = otm8009a_get_modes,
 };
 
-- 
2.15.0.rc0.271.g36b669edcc-goog

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

[PATCH v2 06/10] drm/panel: otm8009a: Properly sequence [un]prepare with backlight

[Sean Paul]

I noticed while removing the enabled flag that backlight update checks
prepared in such a way that could race with hardware turning on/off.
This patch adds a mutex to ensure these races don't happen.

In addition to the lock, this patch also renames prepared to initialized
to better reflect what it means when used in the backlight hook.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/panel/panel-orisetech-otm8009a.c | 43 ++++++++++++++++--------
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
index 0a5898fd4502..d099af3c91df 100644
--- a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
+++ b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
@@ -11,6 +11,7 @@
 #include <drm/drm_panel.h>
 #include <linux/backlight.h>
 #include <linux/gpio/consumer.h>
+#include <linux/mutex.h>
 #include <video/mipi_display.h>
 
 #define DRV_NAME "orisetech_otm8009a"
@@ -62,7 +63,9 @@ struct otm8009a {
 	struct drm_panel panel;
 	struct backlight_device *bl_dev;
 	struct gpio_desc *reset_gpio;
-	bool prepared;
+
+	struct mutex lock;
+	bool initialized;
 };
 
 static const struct drm_display_mode default_mode = {
@@ -265,26 +268,30 @@ static int otm8009a_unprepare(struct drm_panel *panel)
 {
 	struct otm8009a *ctx = panel_to_otm8009a(panel);
 
-	if (!ctx->prepared)
-		return 0;
+	mutex_lock(&ctx->lock);
+	if (!ctx->initialized)
+		goto out;
 
 	if (ctx->reset_gpio) {
 		gpiod_set_value_cansleep(ctx->reset_gpio, 1);
 		msleep(20);
 	}
 
-	ctx->prepared = false;
+	ctx->initialized = false;
 
+out:
+	mutex_unlock(&ctx->lock);
 	return 0;
 }
 
 static int otm8009a_prepare(struct drm_panel *panel)
 {
 	struct otm8009a *ctx = panel_to_otm8009a(panel);
-	int ret;
+	int ret = 0;
 
-	if (ctx->prepared)
-		return 0;
+	mutex_lock(&ctx->lock);
+	if (ctx->initialized)
+		goto out;
 
 	if (ctx->reset_gpio) {
 		gpiod_set_value_cansleep(ctx->reset_gpio, 0);
@@ -296,18 +303,20 @@ static int otm8009a_prepare(struct drm_panel *panel)
 
 	ret = otm8009a_init_sequence(ctx);
 	if (ret)
-		return ret;
+		goto out;
 
-	ctx->prepared = true;
+	ctx->initialized = true;
 
 	/*
 	 * Power on the backlight. Note: end-user still controls brightness
-	 * Note: ctx->prepared must be true before updating the backlight.
+	 * Note: ctx->initialized must be true before updating the backlight.
 	 */
 	ctx->bl_dev->props.power = FB_BLANK_UNBLANK;
 	backlight_update_status(ctx->bl_dev);
 
-	return 0;
+out:
+	mutex_unlock(&ctx->lock);
+	return ret;
 }
 
 static int otm8009a_get_modes(struct drm_panel *panel)
@@ -348,10 +357,13 @@ static int otm8009a_backlight_update_status(struct backlight_device *bd)
 {
 	struct otm8009a *ctx = bl_get_data(bd);
 	u8 data[2];
+	int ret = 0;
 
-	if (!ctx->prepared) {
+	mutex_lock(&ctx->lock);
+	if (!ctx->initialized) {
 		DRM_DEBUG("lcd not ready yet for setting its backlight!\n");
-		return -ENXIO;
+		ret = -ENXIO;
+		goto out;
 	}
 
 	if (bd->props.power <= FB_BLANK_NORMAL) {
@@ -375,7 +387,9 @@ static int otm8009a_backlight_update_status(struct backlight_device *bd)
 	data[0] = MIPI_DCS_WRITE_CONTROL_DISPLAY;
 	otm8009a_dcs_write_buf(ctx, data, ARRAY_SIZE(data));
 
-	return 0;
+out:
+	mutex_unlock(&ctx->lock);
+	return ret;
 }
 
 static const struct backlight_ops otm8009a_backlight_ops = {
@@ -401,6 +415,7 @@ static int otm8009a_probe(struct mipi_dsi_device *dsi)
 	mipi_dsi_set_drvdata(dsi, ctx);
 
 	ctx->dev = dev;
+	mutex_init(&ctx->lock);
 
 	dsi->lanes = 2;
 	dsi->format = MIPI_DSI_FMT_RGB888;
-- 
2.15.0.rc0.271.g36b669edcc-goog

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

[PATCH v2 07/10] drm/panel: 43wvf1g: Remove enabled/prepared state

[Sean Paul]

They're not necessary for atomic drivers, and drm_panel will
WARN if the calls are unbalanced

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 24 ------------------------
 1 file changed, 24 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c
index 71c09ed436ae..51785774efd1 100644
--- a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c
+++ b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c
@@ -44,8 +44,6 @@ struct seiko_panel_desc {
 
 struct seiko_panel {
 	struct drm_panel base;
-	bool prepared;
-	bool enabled;
 	const struct seiko_panel_desc *desc;
 	struct backlight_device *backlight;
 	struct regulator *dvdd;
@@ -126,17 +124,12 @@ static int seiko_panel_disable(struct drm_panel *panel)
 {
 	struct seiko_panel *p = to_seiko_panel(panel);
 
-	if (!p->enabled)
-		return 0;
-
 	if (p->backlight) {
 		p->backlight->props.power = FB_BLANK_POWERDOWN;
 		p->backlight->props.state |= BL_CORE_FBBLANK;
 		backlight_update_status(p->backlight);
 	}
 
-	p->enabled = false;
-
 	return 0;
 }
 
@@ -144,9 +137,6 @@ static int seiko_panel_unprepare(struct drm_panel *panel)
 {
 	struct seiko_panel *p = to_seiko_panel(panel);
 
-	if (!p->prepared)
-		return 0;
-
 	regulator_disable(p->avdd);
 
 	/* Add a 100ms delay as per the panel datasheet */
@@ -154,8 +144,6 @@ static int seiko_panel_unprepare(struct drm_panel *panel)
 
 	regulator_disable(p->dvdd);
 
-	p->prepared = false;
-
 	return 0;
 }
 
@@ -164,9 +152,6 @@ static int seiko_panel_prepare(struct drm_panel *panel)
 	struct seiko_panel *p = to_seiko_panel(panel);
 	int err;
 
-	if (p->prepared)
-		return 0;
-
 	err = regulator_enable(p->dvdd);
 	if (err < 0) {
 		dev_err(panel->dev, "failed to enable dvdd: %d\n", err);
@@ -182,8 +167,6 @@ static int seiko_panel_prepare(struct drm_panel *panel)
 		goto disable_dvdd;
 	}
 
-	p->prepared = true;
-
 	return 0;
 
 disable_dvdd:
@@ -195,17 +178,12 @@ static int seiko_panel_enable(struct drm_panel *panel)
 {
 	struct seiko_panel *p = to_seiko_panel(panel);
 
-	if (p->enabled)
-		return 0;
-
 	if (p->backlight) {
 		p->backlight->props.state &= ~BL_CORE_FBBLANK;
 		p->backlight->props.power = FB_BLANK_UNBLANK;
 		backlight_update_status(p->backlight);
 	}
 
-	p->enabled = true;
-
 	return 0;
 }
 
@@ -254,8 +232,6 @@ static int seiko_panel_probe(struct device *dev,
 	if (!panel)
 		return -ENOMEM;
 
-	panel->enabled = false;
-	panel->prepared = false;
 	panel->desc = desc;
 
 	panel->dvdd = devm_regulator_get(dev, "dvdd");
-- 
2.15.0.rc0.271.g36b669edcc-goog

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

[PATCH v2 08/10] drm/panel: simple: Remove enabled/prepared state

[Sean Paul]

They're not necessary for atomic drivers, and drm_panel will
WARN if the calls are unbalanced

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/panel/panel-simple.c | 24 ------------------------
 1 file changed, 24 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index a3c96d2ea41c..0e1fbca811a0 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -77,8 +77,6 @@ struct panel_desc {
 
 struct panel_simple {
 	struct drm_panel base;
-	bool prepared;
-	bool enabled;
 
 	const struct panel_desc *desc;
 
@@ -163,9 +161,6 @@ static int panel_simple_disable(struct drm_panel *panel)
 {
 	struct panel_simple *p = to_panel_simple(panel);
 
-	if (!p->enabled)
-		return 0;
-
 	if (p->backlight) {
 		p->backlight->props.power = FB_BLANK_POWERDOWN;
 		p->backlight->props.state |= BL_CORE_FBBLANK;
@@ -175,8 +170,6 @@ static int panel_simple_disable(struct drm_panel *panel)
 	if (p->desc->delay.disable)
 		msleep(p->desc->delay.disable);
 
-	p->enabled = false;
-
 	return 0;
 }
 
@@ -184,9 +177,6 @@ static int panel_simple_unprepare(struct drm_panel *panel)
 {
 	struct panel_simple *p = to_panel_simple(panel);
 
-	if (!p->prepared)
-		return 0;
-
 	gpiod_set_value_cansleep(p->enable_gpio, 0);
 
 	regulator_disable(p->supply);
@@ -194,8 +184,6 @@ static int panel_simple_unprepare(struct drm_panel *panel)
 	if (p->desc->delay.unprepare)
 		msleep(p->desc->delay.unprepare);
 
-	p->prepared = false;
-
 	return 0;
 }
 
@@ -204,9 +192,6 @@ static int panel_simple_prepare(struct drm_panel *panel)
 	struct panel_simple *p = to_panel_simple(panel);
 	int err;
 
-	if (p->prepared)
-		return 0;
-
 	err = regulator_enable(p->supply);
 	if (err < 0) {
 		dev_err(panel->dev, "failed to enable supply: %d\n", err);
@@ -218,8 +203,6 @@ static int panel_simple_prepare(struct drm_panel *panel)
 	if (p->desc->delay.prepare)
 		msleep(p->desc->delay.prepare);
 
-	p->prepared = true;
-
 	return 0;
 }
 
@@ -227,9 +210,6 @@ static int panel_simple_enable(struct drm_panel *panel)
 {
 	struct panel_simple *p = to_panel_simple(panel);
 
-	if (p->enabled)
-		return 0;
-
 	if (p->desc->delay.enable)
 		msleep(p->desc->delay.enable);
 
@@ -239,8 +219,6 @@ static int panel_simple_enable(struct drm_panel *panel)
 		backlight_update_status(p->backlight);
 	}
 
-	p->enabled = true;
-
 	return 0;
 }
 
@@ -301,8 +279,6 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
 	if (!panel)
 		return -ENOMEM;
 
-	panel->enabled = false;
-	panel->prepared = false;
 	panel->desc = desc;
 
 	panel->supply = devm_regulator_get(dev, "power");
-- 
2.15.0.rc0.271.g36b669edcc-goog

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

[PATCH v2 09/10] drm/panel: p079zca: Remove enabled/prepared state

[Sean Paul]

They're not necessary for atomic drivers, and drm_panel will
WARN if the calls are unbalanced

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/panel/panel-innolux-p079zca.c | 23 -----------------------
 1 file changed, 23 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
index 6ba93449fcfb..38b19c8de9e1 100644
--- a/drivers/gpu/drm/panel/panel-innolux-p079zca.c
+++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
@@ -27,9 +27,6 @@ struct innolux_panel {
 	struct backlight_device *backlight;
 	struct regulator *supply;
 	struct gpio_desc *enable_gpio;
-
-	bool prepared;
-	bool enabled;
 };
 
 static inline struct innolux_panel *to_innolux_panel(struct drm_panel *panel)
@@ -42,9 +39,6 @@ static int innolux_panel_disable(struct drm_panel *panel)
 	struct innolux_panel *innolux = to_innolux_panel(panel);
 	int err;
 
-	if (!innolux->enabled)
-		return 0;
-
 	innolux->backlight->props.power = FB_BLANK_POWERDOWN;
 	backlight_update_status(innolux->backlight);
 
@@ -53,8 +47,6 @@ static int innolux_panel_disable(struct drm_panel *panel)
 		DRM_DEV_ERROR(panel->dev, "failed to set display off: %d\n",
 			      err);
 
-	innolux->enabled = false;
-
 	return 0;
 }
 
@@ -63,9 +55,6 @@ static int innolux_panel_unprepare(struct drm_panel *panel)
 	struct innolux_panel *innolux = to_innolux_panel(panel);
 	int err;
 
-	if (!innolux->prepared)
-		return 0;
-
 	err = mipi_dsi_dcs_enter_sleep_mode(innolux->link);
 	if (err < 0) {
 		DRM_DEV_ERROR(panel->dev, "failed to enter sleep mode: %d\n",
@@ -82,8 +71,6 @@ static int innolux_panel_unprepare(struct drm_panel *panel)
 	if (err < 0)
 		return err;
 
-	innolux->prepared = false;
-
 	return 0;
 }
 
@@ -92,9 +79,6 @@ static int innolux_panel_prepare(struct drm_panel *panel)
 	struct innolux_panel *innolux = to_innolux_panel(panel);
 	int err, regulator_err;
 
-	if (innolux->prepared)
-		return 0;
-
 	gpiod_set_value_cansleep(innolux->enable_gpio, 0);
 
 	err = regulator_enable(innolux->supply);
@@ -129,8 +113,6 @@ static int innolux_panel_prepare(struct drm_panel *panel)
 	/* T7: 5ms */
 	usleep_range(5000, 6000);
 
-	innolux->prepared = true;
-
 	return 0;
 
 poweroff:
@@ -148,9 +130,6 @@ static int innolux_panel_enable(struct drm_panel *panel)
 	struct innolux_panel *innolux = to_innolux_panel(panel);
 	int ret;
 
-	if (innolux->enabled)
-		return 0;
-
 	innolux->backlight->props.power = FB_BLANK_UNBLANK;
 	ret = backlight_update_status(innolux->backlight);
 	if (ret) {
@@ -159,8 +138,6 @@ static int innolux_panel_enable(struct drm_panel *panel)
 		return ret;
 	}
 
-	innolux->enabled = true;
-
 	return 0;
 }
 
-- 
2.15.0.rc0.271.g36b669edcc-goog

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

[PATCH v2 10/10] drm/panel: ls043t1le01: Remove enabled/prepared state

[Sean Paul]

They're not necessary for atomic drivers, and drm_panel will
WARN if the calls are unbalanced

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c | 23 -----------------------
 1 file changed, 23 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c b/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
index 3aeb0bda4947..8d7843248556 100644
--- a/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
@@ -39,9 +39,6 @@ struct sharp_nt_panel {
 	struct regulator *supply;
 	struct gpio_desc *reset_gpio;
 
-	bool prepared;
-	bool enabled;
-
 	const struct drm_display_mode *mode;
 };
 
@@ -114,16 +111,11 @@ static int sharp_nt_panel_disable(struct drm_panel *panel)
 {
 	struct sharp_nt_panel *sharp_nt = to_sharp_nt_panel(panel);
 
-	if (!sharp_nt->enabled)
-		return 0;
-
 	if (sharp_nt->backlight) {
 		sharp_nt->backlight->props.power = FB_BLANK_POWERDOWN;
 		backlight_update_status(sharp_nt->backlight);
 	}
 
-	sharp_nt->enabled = false;
-
 	return 0;
 }
 
@@ -132,9 +124,6 @@ static int sharp_nt_panel_unprepare(struct drm_panel *panel)
 	struct sharp_nt_panel *sharp_nt = to_sharp_nt_panel(panel);
 	int ret;
 
-	if (!sharp_nt->prepared)
-		return 0;
-
 	ret = sharp_nt_panel_off(sharp_nt);
 	if (ret < 0) {
 		dev_err(panel->dev, "failed to set panel off: %d\n", ret);
@@ -145,8 +134,6 @@ static int sharp_nt_panel_unprepare(struct drm_panel *panel)
 	if (sharp_nt->reset_gpio)
 		gpiod_set_value(sharp_nt->reset_gpio, 0);
 
-	sharp_nt->prepared = false;
-
 	return 0;
 }
 
@@ -155,9 +142,6 @@ static int sharp_nt_panel_prepare(struct drm_panel *panel)
 	struct sharp_nt_panel *sharp_nt = to_sharp_nt_panel(panel);
 	int ret;
 
-	if (sharp_nt->prepared)
-		return 0;
-
 	ret = regulator_enable(sharp_nt->supply);
 	if (ret < 0)
 		return ret;
@@ -185,8 +169,6 @@ static int sharp_nt_panel_prepare(struct drm_panel *panel)
 		goto poweroff;
 	}
 
-	sharp_nt->prepared = true;
-
 	return 0;
 
 poweroff:
@@ -200,16 +182,11 @@ static int sharp_nt_panel_enable(struct drm_panel *panel)
 {
 	struct sharp_nt_panel *sharp_nt = to_sharp_nt_panel(panel);
 
-	if (sharp_nt->enabled)
-		return 0;
-
 	if (sharp_nt->backlight) {
 		sharp_nt->backlight->props.power = FB_BLANK_UNBLANK;
 		backlight_update_status(sharp_nt->backlight);
 	}
 
-	sharp_nt->enabled = true;
-
 	return 0;
 }
 
-- 
2.15.0.rc0.271.g36b669edcc-goog

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

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

[Daniel Vetter]

On Thu, Oct 12, 2017 at 01:55:28PM -0400, 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
> 
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>

I really like this. Bunch of (hopefully) non-jetlagged comments below.

With those address (obviously for all the functions, I only commented on
drm_panel_unprepare):

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/drm_panel.c |  1 +
>  include/drm/drm_panel.h     | 60 +++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 53 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index 308d442a531b..e5957e7da768 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -48,6 +48,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);
>  
> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> index 14ac240a1f64..425461c4c574 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,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
> + * @power_state: keeps track of the panel power status
>   */
>  struct drm_panel {
>  	struct drm_device *drm;
> @@ -93,6 +95,12 @@ struct drm_panel {
>  	const struct drm_panel_funcs *funcs;
>  
>  	struct list_head list;
> +
> +	enum {
> +		DRM_PANEL_POWER_OFF,
> +		DRM_PANEL_POWER_PREPARED,
> +		DRM_PANEL_POWER_ENABLED
> +	} state;
>  };
>  
>  /**
> @@ -104,12 +112,21 @@ 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.

s/should/will/ to make it slightly more authoritative sounding?

> + *
>   * Return: 0 on success or a negative error code on failure.
>   */
>  static inline int drm_panel_unprepare(struct drm_panel *panel)

Given that the functions have grown quite a bit I'm not sure it's
reasonable to have them as static inlines anymore ...

>  {
> -	if (panel && panel->funcs && panel->funcs->unprepare)
> -		return panel->funcs->unprepare(panel);
> +	WARN_ON(panel->state != DRM_PANEL_POWER_PREPARED);
> +
> +	if (panel && panel->funcs && panel->funcs->unprepare) {
> +		int ret = panel->funcs->unprepare(panel);
> +		if (!ret)
> +			panel->state = DRM_PANEL_POWER_OFF;

Do you really want to only update the status if the driver provides a
callback? I think this should be moved out of the if
(panel->funcs->disable), but not ouf of the if (panel).

Could probably make the if (panel) an early return, for clean code.
-Daniel

> +		return ret;
> +	}
>  
>  	return panel ? -ENOSYS : -EINVAL;
>  }
> @@ -122,12 +139,21 @@ 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)
> -		return panel->funcs->disable(panel);
> +	WARN_ON(panel->state != DRM_PANEL_POWER_ENABLED);
> +
> +	if (panel && panel->funcs && panel->funcs->disable) {
> +		int ret = panel->funcs->disable(panel);
> +		if (!ret)
> +			panel->state = DRM_PANEL_POWER_PREPARED;
> +		return ret;
> +	}
>  
>  	return panel ? -ENOSYS : -EINVAL;
>  }
> @@ -140,12 +166,21 @@ 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)
> -		return panel->funcs->prepare(panel);
> +	WARN_ON(panel->state != DRM_PANEL_POWER_OFF);
> +
> +	if (panel && panel->funcs && panel->funcs->prepare) {
> +		int ret = panel->funcs->prepare(panel);
> +		if (!ret)
> +			panel->state = DRM_PANEL_POWER_PREPARED;
> +		return ret;
> +	}
>  
>  	return panel ? -ENOSYS : -EINVAL;
>  }
> @@ -158,12 +193,21 @@ 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)
> -		return panel->funcs->enable(panel);
> +	WARN_ON(panel->state != DRM_PANEL_POWER_PREPARED);
> +
> +	if (panel && panel->funcs && panel->funcs->enable) {
> +		int ret = panel->funcs->enable(panel);
> +		if (!ret)
> +			panel->state = DRM_PANEL_POWER_ENABLED;
> +		return ret;
> +	}
>  
>  	return panel ? -ENOSYS : -EINVAL;
>  }
> -- 
> 2.15.0.rc0.271.g36b669edcc-goog
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Andrzej Hajda]

On 12.10.2017 19:55, 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
>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---
>  drivers/gpu/drm/drm_panel.c |  1 +
>  include/drm/drm_panel.h     | 60 +++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 53 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index 308d442a531b..e5957e7da768 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -48,6 +48,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);
>  
> diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
> index 14ac240a1f64..425461c4c574 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,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
> + * @power_state: keeps track of the panel power status
>   */
>  struct drm_panel {
>  	struct drm_device *drm;
> @@ -93,6 +95,12 @@ struct drm_panel {
>  	const struct drm_panel_funcs *funcs;
>  
>  	struct list_head list;
> +
> +	enum {
> +		DRM_PANEL_POWER_OFF,
> +		DRM_PANEL_POWER_PREPARED,
> +		DRM_PANEL_POWER_ENABLED
> +	} state;
>  };
>  
>  /**
> @@ -104,12 +112,21 @@ 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)
> -		return panel->funcs->unprepare(panel);
> +	WARN_ON(panel->state != DRM_PANEL_POWER_PREPARED);
> +
> +	if (panel && panel->funcs && panel->funcs->unprepare) {
> +		int ret = panel->funcs->unprepare(panel);
> +		if (!ret)
> +			panel->state = DRM_PANEL_POWER_OFF;
> +		return ret;
> +	}
>  
>  	return panel ? -ENOSYS : -EINVAL;

State check and state change should be performed also for panels without
callbacks, otherwise it will work only with panels having full set of
callbacks.
Another thing is that missing callback does not mean
function-not-implemented (ENOSYS), it is rather nothing-to-do, in such
case function should return rather 0, this will simplify also error
handling on callers side.

And one thing just to consider: all functions have almost the same body,
maybe it would be good to use some helper, for example:

    #define drm_panel_state_change(panel, from, to, func) ...

    int drm_panel_unprepare(struct drm_panel *panel)
    {
        return drm_panel_state_change(panel, DRM_PANEL_POWER_PREPARED,
DRM_PANEL_POWER_OFF, unprepare);
    }


Regards
Andrzej
>  }
> @@ -122,12 +139,21 @@ 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)
> -		return panel->funcs->disable(panel);
> +	WARN_ON(panel->state != DRM_PANEL_POWER_ENABLED);
> +
> +	if (panel && panel->funcs && panel->funcs->disable) {
> +		int ret = panel->funcs->disable(panel);
> +		if (!ret)
> +			panel->state = DRM_PANEL_POWER_PREPARED;
> +		return ret;
> +	}
>  
>  	return panel ? -ENOSYS : -EINVAL;
>  }
> @@ -140,12 +166,21 @@ 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)
> -		return panel->funcs->prepare(panel);
> +	WARN_ON(panel->state != DRM_PANEL_POWER_OFF);
> +
> +	if (panel && panel->funcs && panel->funcs->prepare) {
> +		int ret = panel->funcs->prepare(panel);
> +		if (!ret)
> +			panel->state = DRM_PANEL_POWER_PREPARED;
> +		return ret;
> +	}
>  
>  	return panel ? -ENOSYS : -EINVAL;
>  }
> @@ -158,12 +193,21 @@ 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)
> -		return panel->funcs->enable(panel);
> +	WARN_ON(panel->state != DRM_PANEL_POWER_PREPARED);
> +
> +	if (panel && panel->funcs && panel->funcs->enable) {
> +		int ret = panel->funcs->enable(panel);
> +		if (!ret)
> +			panel->state = DRM_PANEL_POWER_ENABLED;
> +		return ret;
> +	}
>  
>  	return panel ? -ENOSYS : -EINVAL;
>  }


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