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

[PATCH 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.


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                        |  2 +
 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                            | 38 ++++++++++++--
 10 files changed, 65 insertions(+), 196 deletions(-)

-- 
2.14.1.821.g8fa685d3b7-goog

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

[PATCH 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.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 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);
+		panel->prepared = false;
 		return panel->funcs->unprepare(panel);
+	}
 
 	return panel ? -ENOSYS : -EINVAL;
 }
@@ -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;
 }
-- 
2.14.1.821.g8fa685d3b7-goog

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

[PATCH 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.14.1.821.g8fa685d3b7-goog

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

[PATCH 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.14.1.821.g8fa685d3b7-goog

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

[PATCH 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.14.1.821.g8fa685d3b7-goog

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

[PATCH 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.14.1.821.g8fa685d3b7-goog

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

[PATCH 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.14.1.821.g8fa685d3b7-goog

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

[PATCH 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.14.1.821.g8fa685d3b7-goog

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

[PATCH 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.14.1.821.g8fa685d3b7-goog

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

[PATCH 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.14.1.821.g8fa685d3b7-goog

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

[PATCH 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.14.1.821.g8fa685d3b7-goog

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

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

[Andrzej Hajda]

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.

> +		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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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@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;
>>  }
>

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

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

[Eric Anholt]

[-- 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

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

[Daniel Vetter]

On Fri, Sep 22, 2017 at 09:13:53AM +0200, 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@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 :)

The tracking is just so that the drm-panel wrappers can appropriately
WARN_ON for drivers which get this wrong. Which I think makes sense, since
drm-panel needs to be glued into the driver manually in many cases (anyone
not using panel-bridge really).

I'd ack the entire series, but I'm still suffering from jetlag so this is
probably not a good idea yet. If it's not yet reviewed, feel free to ping
me in a few days ...
-Daniel

> 
> > ---
> >  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.
> 
> > +		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@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