As talked about in commit d2aacaf07395 ("drm/panel: Check for already
prepared/enabled in drm_panel"), we want to remove needless code from
panel drivers that was storing and double-checking the
prepared/enabled state. Even if someone was relying on the
double-check before, that double-check is now in the core and not
needed in individual drivers.
This series attempts to do just that. While the original grep, AKA:
git grep 'if.*>prepared' -- drivers/gpu/drm/panel
git grep 'if.*>enabled' -- drivers/gpu/drm/panel
...still produces a few hits after my series, they are _mostly_ all
gone. The ones that are left are less trivial to fix.
One of the main reasons that many panels probably needed to store and
double-check their prepared/enabled appears to have been to handle
shutdown and/or remove. Panels drivers often wanted to force the power
off for panels in these cases and this was a good reason for the
double-check. As part of this series a new helper is added that uses
the state tracking that the drm_panel core is doing so each individual
panel driver doesn't need to do it.
This series changes a lot of drivers and obviously the author can't
test on all of them. The changes here are also not completely trivial
in all cases. Please double-check your drivers carefully to make sure
something wasn't missed. After looking at over 40 drivers I'll admit
that my eyes glazed over a little.
I've attempted to organize these patches like to group together panels
that needed similar handling. Panels that had code that didn't seem to
match anyone else got their own patch. I made judgement calls on what
I considered "similar".
As noted in individual patches, there are some cases here where I
expect behavior to change a little bit. I'm hoping these changes are
for the better and don't cause any problems. Fingers crossed.
I have at least confirmed that "allmodconfig" for arm64 doesn't fall
on its face with this series. I haven't done a ton of other testing.
Douglas Anderson (10):
drm/panel: Don't store+check prepared/enabled for simple cases
drm/panel: s6e63m0: Don't store+check prepared/enabled
drm/panel: otm8009a: Don't double check prepared/enabled
drm/panel_helper: Introduce drm_panel_helper
drm/panel: Don't store+check prepared/enabled for panels needing
shutdown
drm/panel: Don't store+check prepared/enabled for panels disabled at
shutdown
drm/panel: st7703: Don't store+check prepared
drm/panel: rm67191: Don't store+check enabled
drm/panel: sony-acx565akm: Don't double-check enabled state in disable
drm/panel: Update TODO list item for cleaning up prepared/enabled
tracking
Documentation/gpu/todo.rst | 33 +++++-------
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/drm_panel_helper.c | 37 ++++++++++++++
.../drm/panel/panel-asus-z00t-tm5p5-n35596.c | 9 ----
.../gpu/drm/panel/panel-boe-bf060y8m-aj0.c | 9 ----
drivers/gpu/drm/panel/panel-boe-himax8279d.c | 36 ++-----------
.../gpu/drm/panel/panel-boe-tv101wum-nl6.c | 16 +-----
drivers/gpu/drm/panel/panel-edp.c | 34 ++-----------
drivers/gpu/drm/panel/panel-elida-kd35t133.c | 21 +-------
drivers/gpu/drm/panel/panel-himax-hx8394.c | 21 +-------
drivers/gpu/drm/panel/panel-innolux-p079zca.c | 51 ++-----------------
drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c | 9 ----
.../gpu/drm/panel/panel-jdi-lt070me05000.c | 30 ++---------
drivers/gpu/drm/panel/panel-khadas-ts050.c | 35 ++-----------
.../drm/panel/panel-kingdisplay-kd097d04.c | 43 ++--------------
.../drm/panel/panel-leadtek-ltk050h3146w.c | 21 +-------
.../drm/panel/panel-leadtek-ltk500hd1829.c | 21 +-------
drivers/gpu/drm/panel/panel-novatek-nt35950.c | 9 ----
drivers/gpu/drm/panel/panel-novatek-nt36523.c | 12 -----
.../gpu/drm/panel/panel-novatek-nt36672a.c | 24 ++-------
.../drm/panel/panel-olimex-lcd-olinuxino.c | 45 +---------------
.../gpu/drm/panel/panel-orisetech-otm8009a.c | 17 -------
.../drm/panel/panel-osd-osd101t2587-53ts.c | 37 ++------------
.../drm/panel/panel-panasonic-vvx10f034n00.c | 42 ++-------------
drivers/gpu/drm/panel/panel-raydium-rm67191.c | 21 +-------
drivers/gpu/drm/panel/panel-raydium-rm68200.c | 38 --------------
.../gpu/drm/panel/panel-samsung-atna33xc20.c | 31 ++---------
drivers/gpu/drm/panel/panel-samsung-s6e63m0.c | 25 ---------
.../panel/panel-samsung-s6e88a0-ams452ef01.c | 10 ----
drivers/gpu/drm/panel/panel-samsung-sofef00.c | 9 ----
drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 45 ++--------------
.../gpu/drm/panel/panel-sharp-lq101r1sx01.c | 46 ++---------------
.../gpu/drm/panel/panel-sharp-ls043t1le01.c | 19 ++-----
.../gpu/drm/panel/panel-sharp-ls060t1sx01.c | 10 ----
drivers/gpu/drm/panel/panel-simple.c | 34 ++-----------
drivers/gpu/drm/panel/panel-sitronix-st7703.c | 20 +-------
drivers/gpu/drm/panel/panel-sony-acx565akm.c | 7 +--
drivers/gpu/drm/panel/panel-sony-td4353-jdi.c | 9 ----
.../panel/panel-sony-tulip-truly-nt35521.c | 18 -------
.../drm/panel/panel-startek-kd070fhfid015.c | 11 ----
drivers/gpu/drm/panel/panel-tdo-tl070wsh30.c | 19 ++-----
drivers/gpu/drm/panel/panel-truly-nt35597.c | 20 --------
drivers/gpu/drm/panel/panel-visionox-r66451.c | 16 ------
.../gpu/drm/panel/panel-visionox-rm69299.c | 8 ---
.../gpu/drm/panel/panel-visionox-vtdr6130.c | 9 ----
.../gpu/drm/panel/panel-xinpeng-xpp055c272.c | 21 +-------
include/drm/drm_panel_helper.h | 13 +++++
47 files changed, 131 insertions(+), 941 deletions(-)
create mode 100644 drivers/gpu/drm/drm_panel_helper.c
create mode 100644 include/drm/drm_panel_helper.h
--
2.41.0.585.gd2178a4bd4-goog
As talked about in commit d2aacaf07395 ("drm/panel: Check for already
prepared/enabled in drm_panel"), we want to remove needless code from
panel drivers that was storing and double-checking the
prepared/enabled state. Even if someone was relying on the
double-check before, that double-check is now in the core and not
needed in individual drivers.
This is much like the patch ("drm/panel: Don't store+check
prepared/enabled for panels needing shutdown") but this set of panels
used to _just_ call their disable() function at shutdown time. Now
both the disable() and unprepare() will be called. This is probably an
improvement and probably makes the power sequencing more correct, but
it is a change in behavior that needs to be called out. It also has
the potential to delay shutdown by a small amount of time.
As talked about in patch ("drm/panel: Don't store+check
prepared/enabled for panels needing shutdown"), this patch doesn't
attempt to validate that any remove() functions touched will actually
work properly. The main goal here is to avoid storing and
double-checking the prepared and enabled state.
Signed-off-by: Douglas Anderson <[email protected]>
---
.../gpu/drm/panel/panel-jdi-lt070me05000.c | 30 ++----------
.../drm/panel/panel-panasonic-vvx10f034n00.c | 42 ++---------------
drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 45 ++----------------
.../gpu/drm/panel/panel-sharp-lq101r1sx01.c | 46 ++-----------------
.../gpu/drm/panel/panel-sharp-ls043t1le01.c | 19 ++------
5 files changed, 16 insertions(+), 166 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
index 8f4f137a2af6..28b7ae491d12 100644
--- a/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
+++ b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
@@ -24,6 +24,7 @@
#include <drm/drm_mipi_dsi.h>
#include <drm/drm_modes.h>
#include <drm/drm_panel.h>
+#include <drm/drm_panel_helper.h>
static const char * const regulator_names[] = {
"vddp",
@@ -41,9 +42,6 @@ struct jdi_panel {
struct gpio_desc *dcdc_en_gpio;
struct backlight_device *backlight;
- bool prepared;
- bool enabled;
-
const struct drm_display_mode *mode;
};
@@ -180,13 +178,8 @@ static int jdi_panel_disable(struct drm_panel *panel)
{
struct jdi_panel *jdi = to_jdi_panel(panel);
- if (!jdi->enabled)
- return 0;
-
backlight_disable(jdi->backlight);
- jdi->enabled = false;
-
return 0;
}
@@ -196,9 +189,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);
@@ -211,8 +201,6 @@ static int jdi_panel_unprepare(struct drm_panel *panel)
gpiod_set_value(jdi->dcdc_en_gpio, 0);
- jdi->prepared = false;
-
return 0;
}
@@ -222,9 +210,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);
@@ -254,8 +239,6 @@ static int jdi_panel_prepare(struct drm_panel *panel)
goto poweroff;
}
- jdi->prepared = true;
-
return 0;
poweroff:
@@ -276,13 +259,8 @@ static int jdi_panel_enable(struct drm_panel *panel)
{
struct jdi_panel *jdi = to_jdi_panel(panel);
- if (jdi->enabled)
- return 0;
-
backlight_enable(jdi->backlight);
- jdi->enabled = true;
-
return 0;
}
@@ -487,9 +465,7 @@ static void jdi_panel_remove(struct mipi_dsi_device *dsi)
struct jdi_panel *jdi = mipi_dsi_get_drvdata(dsi);
int ret;
- ret = jdi_panel_disable(&jdi->base);
- if (ret < 0)
- dev_err(&dsi->dev, "failed to disable panel: %d\n", ret);
+ drm_panel_helper_shutdown(&jdi->base);
ret = mipi_dsi_detach(dsi);
if (ret < 0)
@@ -503,7 +479,7 @@ static void jdi_panel_shutdown(struct mipi_dsi_device *dsi)
{
struct jdi_panel *jdi = mipi_dsi_get_drvdata(dsi);
- jdi_panel_disable(&jdi->base);
+ drm_panel_helper_shutdown(&jdi->base);
}
static struct mipi_dsi_driver jdi_panel_driver = {
diff --git a/drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c b/drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c
index 8ba6d8287938..1a4121bd0ec1 100644
--- a/drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c
+++ b/drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c
@@ -18,6 +18,7 @@
#include <drm/drm_device.h>
#include <drm/drm_mipi_dsi.h>
#include <drm/drm_panel.h>
+#include <drm/drm_panel_helper.h>
/*
* When power is turned off to this panel a minimum off time of 500ms has to be
@@ -32,9 +33,6 @@ struct wuxga_nt_panel {
struct regulator *supply;
- bool prepared;
- bool enabled;
-
ktime_t earliest_wake;
const struct drm_display_mode *mode;
@@ -53,28 +51,16 @@ static int wuxga_nt_panel_on(struct wuxga_nt_panel *wuxga_nt)
static int wuxga_nt_panel_disable(struct drm_panel *panel)
{
struct wuxga_nt_panel *wuxga_nt = to_wuxga_nt_panel(panel);
- int mipi_ret, bl_ret = 0;
-
- if (!wuxga_nt->enabled)
- return 0;
-
- mipi_ret = mipi_dsi_shutdown_peripheral(wuxga_nt->dsi);
-
- wuxga_nt->enabled = false;
- return mipi_ret ? mipi_ret : bl_ret;
+ return mipi_dsi_shutdown_peripheral(wuxga_nt->dsi);
}
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;
}
@@ -85,9 +71,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
@@ -117,8 +100,6 @@ static int wuxga_nt_panel_prepare(struct drm_panel *panel)
goto poweroff;
}
- wuxga_nt->prepared = true;
-
return 0;
poweroff:
@@ -127,18 +108,6 @@ static int wuxga_nt_panel_prepare(struct drm_panel *panel)
return ret;
}
-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;
-
- wuxga_nt->enabled = true;
-
- return 0;
-}
-
static const struct drm_display_mode default_mode = {
.clock = 164402,
.hdisplay = 1920,
@@ -178,7 +147,6 @@ static const struct drm_panel_funcs wuxga_nt_panel_funcs = {
.disable = wuxga_nt_panel_disable,
.unprepare = wuxga_nt_panel_unprepare,
.prepare = wuxga_nt_panel_prepare,
- .enable = wuxga_nt_panel_enable,
.get_modes = wuxga_nt_panel_get_modes,
};
@@ -255,9 +223,7 @@ static void wuxga_nt_panel_remove(struct mipi_dsi_device *dsi)
struct wuxga_nt_panel *wuxga_nt = mipi_dsi_get_drvdata(dsi);
int ret;
- ret = drm_panel_disable(&wuxga_nt->base);
- if (ret < 0)
- dev_err(&dsi->dev, "failed to disable panel: %d\n", ret);
+ drm_panel_helper_shutdown(&wuxga_nt->base);
ret = mipi_dsi_detach(dsi);
if (ret < 0)
@@ -270,7 +236,7 @@ static void wuxga_nt_panel_shutdown(struct mipi_dsi_device *dsi)
{
struct wuxga_nt_panel *wuxga_nt = mipi_dsi_get_drvdata(dsi);
- drm_panel_disable(&wuxga_nt->base);
+ drm_panel_helper_shutdown(&wuxga_nt->base);
}
static struct mipi_dsi_driver wuxga_nt_panel_driver = {
diff --git a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c
index 658c7c040570..ca517c642674 100644
--- a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c
+++ b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c
@@ -20,6 +20,7 @@
#include <drm/drm_crtc.h>
#include <drm/drm_device.h>
#include <drm/drm_panel.h>
+#include <drm/drm_panel_helper.h>
struct seiko_panel_desc {
const struct drm_display_mode *modes;
@@ -44,8 +45,6 @@ struct seiko_panel_desc {
struct seiko_panel {
struct drm_panel base;
- bool prepared;
- bool enabled;
const struct seiko_panel_desc *desc;
struct regulator *dvdd;
struct regulator *avdd;
@@ -122,25 +121,10 @@ static int seiko_panel_get_fixed_modes(struct seiko_panel *panel,
return num;
}
-static int seiko_panel_disable(struct drm_panel *panel)
-{
- struct seiko_panel *p = to_seiko_panel(panel);
-
- if (!p->enabled)
- return 0;
-
- p->enabled = false;
-
- return 0;
-}
-
static int seiko_panel_unprepare(struct drm_panel *panel)
{
struct seiko_panel *p = to_seiko_panel(panel);
- if (!p->prepared)
- return 0;
-
gpiod_set_value_cansleep(p->enable_gpio, 0);
regulator_disable(p->avdd);
@@ -150,8 +134,6 @@ static int seiko_panel_unprepare(struct drm_panel *panel)
regulator_disable(p->dvdd);
- p->prepared = false;
-
return 0;
}
@@ -160,9 +142,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);
@@ -180,8 +159,6 @@ static int seiko_panel_prepare(struct drm_panel *panel)
gpiod_set_value_cansleep(p->enable_gpio, 1);
- p->prepared = true;
-
return 0;
disable_dvdd:
@@ -189,18 +166,6 @@ static int seiko_panel_prepare(struct drm_panel *panel)
return err;
}
-static int seiko_panel_enable(struct drm_panel *panel)
-{
- struct seiko_panel *p = to_seiko_panel(panel);
-
- if (p->enabled)
- return 0;
-
- p->enabled = true;
-
- return 0;
-}
-
static int seiko_panel_get_modes(struct drm_panel *panel,
struct drm_connector *connector)
{
@@ -228,10 +193,8 @@ static int seiko_panel_get_timings(struct drm_panel *panel,
}
static const struct drm_panel_funcs seiko_panel_funcs = {
- .disable = seiko_panel_disable,
.unprepare = seiko_panel_unprepare,
.prepare = seiko_panel_prepare,
- .enable = seiko_panel_enable,
.get_modes = seiko_panel_get_modes,
.get_timings = seiko_panel_get_timings,
};
@@ -246,8 +209,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");
@@ -283,14 +244,14 @@ static void seiko_panel_remove(struct platform_device *pdev)
struct seiko_panel *panel = platform_get_drvdata(pdev);
drm_panel_remove(&panel->base);
- drm_panel_disable(&panel->base);
+ drm_panel_helper_shutdown(&panel->base);
}
static void seiko_panel_shutdown(struct platform_device *pdev)
{
struct seiko_panel *panel = platform_get_drvdata(pdev);
- drm_panel_disable(&panel->base);
+ drm_panel_helper_shutdown(&panel->base);
}
static const struct display_timing seiko_43wvf1g_timing = {
diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
index 14851408a5e1..2a0b64c7d898 100644
--- a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
@@ -15,6 +15,7 @@
#include <drm/drm_device.h>
#include <drm/drm_mipi_dsi.h>
#include <drm/drm_panel.h>
+#include <drm/drm_panel_helper.h>
struct sharp_panel {
struct drm_panel base;
@@ -24,9 +25,6 @@ struct sharp_panel {
struct regulator *supply;
- bool prepared;
- bool enabled;
-
const struct drm_display_mode *mode;
};
@@ -85,26 +83,11 @@ static __maybe_unused int sharp_panel_read(struct sharp_panel *sharp,
return err;
}
-static int sharp_panel_disable(struct drm_panel *panel)
-{
- struct sharp_panel *sharp = to_sharp_panel(panel);
-
- if (!sharp->enabled)
- return 0;
-
- sharp->enabled = false;
-
- return 0;
-}
-
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);
@@ -119,8 +102,6 @@ static int sharp_panel_unprepare(struct drm_panel *panel)
regulator_disable(sharp->supply);
- sharp->prepared = false;
-
return 0;
}
@@ -164,9 +145,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;
@@ -235,8 +213,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);
@@ -247,18 +223,6 @@ static int sharp_panel_prepare(struct drm_panel *panel)
return err;
}
-static int sharp_panel_enable(struct drm_panel *panel)
-{
- struct sharp_panel *sharp = to_sharp_panel(panel);
-
- if (sharp->enabled)
- return 0;
-
- sharp->enabled = true;
-
- return 0;
-}
-
static const struct drm_display_mode default_mode = {
.clock = 278000,
.hdisplay = 2560,
@@ -295,10 +259,8 @@ static int sharp_panel_get_modes(struct drm_panel *panel,
}
static const struct drm_panel_funcs sharp_panel_funcs = {
- .disable = sharp_panel_disable,
.unprepare = sharp_panel_unprepare,
.prepare = sharp_panel_prepare,
- .enable = sharp_panel_enable,
.get_modes = sharp_panel_get_modes,
};
@@ -402,9 +364,7 @@ static void sharp_panel_remove(struct mipi_dsi_device *dsi)
return;
}
- err = drm_panel_disable(&sharp->base);
- if (err < 0)
- dev_err(&dsi->dev, "failed to disable panel: %d\n", err);
+ drm_panel_helper_shutdown(&sharp->base);
err = mipi_dsi_detach(dsi);
if (err < 0)
@@ -421,7 +381,7 @@ static void sharp_panel_shutdown(struct mipi_dsi_device *dsi)
if (!sharp)
return;
- drm_panel_disable(&sharp->base);
+ drm_panel_helper_shutdown(&sharp->base);
}
static struct mipi_dsi_driver sharp_panel_driver = {
diff --git a/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c b/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
index 855e64444daa..7f8c6e5c389f 100644
--- a/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
@@ -19,6 +19,7 @@
#include <drm/drm_device.h>
#include <drm/drm_mipi_dsi.h>
#include <drm/drm_panel.h>
+#include <drm/drm_panel_helper.h>
struct sharp_nt_panel {
struct drm_panel base;
@@ -26,8 +27,6 @@ struct sharp_nt_panel {
struct regulator *supply;
struct gpio_desc *reset_gpio;
-
- bool prepared;
};
static inline struct sharp_nt_panel *to_sharp_nt_panel(struct drm_panel *panel)
@@ -99,9 +98,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);
@@ -112,8 +108,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;
}
@@ -122,9 +116,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;
@@ -152,8 +143,6 @@ static int sharp_nt_panel_prepare(struct drm_panel *panel)
goto poweroff;
}
- sharp_nt->prepared = true;
-
return 0;
poweroff:
@@ -279,9 +268,7 @@ static void sharp_nt_panel_remove(struct mipi_dsi_device *dsi)
struct sharp_nt_panel *sharp_nt = mipi_dsi_get_drvdata(dsi);
int ret;
- ret = drm_panel_disable(&sharp_nt->base);
- if (ret < 0)
- dev_err(&dsi->dev, "failed to disable panel: %d\n", ret);
+ drm_panel_helper_shutdown(&sharp_nt->base);
ret = mipi_dsi_detach(dsi);
if (ret < 0)
@@ -294,7 +281,7 @@ static void sharp_nt_panel_shutdown(struct mipi_dsi_device *dsi)
{
struct sharp_nt_panel *sharp_nt = mipi_dsi_get_drvdata(dsi);
- drm_panel_disable(&sharp_nt->base);
+ drm_panel_helper_shutdown(&sharp_nt->base);
}
static const struct of_device_id sharp_nt_of_match[] = {
--
2.41.0.585.gd2178a4bd4-goog
As talked about in commit d2aacaf07395 ("drm/panel: Check for already
prepared/enabled in drm_panel"), we want to remove needless code from
panel drivers that was storing and double-checking the
prepared/enabled state. Even if someone was relying on the
double-check before, that double-check is now in the core and not
needed in individual drivers.
The acx565akm seems to do some unique stuff with the "enabled"
state. Specifically:
1. It seems to detect the enabled state based on how the bootloader
left the panel.
2. It uses the enabled state to prevent certain sysfs files from
accessing a disabled panel.
We'll leave the "enabled" state tracking for this. However, we can at
least get rid of the double-check when trying to disable. In order to
do this we use the new drm_panel_helper_shutdown() from remove() which
double-checks for us.
Signed-off-by: Douglas Anderson <[email protected]>
---
drivers/gpu/drm/panel/panel-sony-acx565akm.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-sony-acx565akm.c b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
index 3d6a286056a0..8a8326a94d72 100644
--- a/drivers/gpu/drm/panel/panel-sony-acx565akm.c
+++ b/drivers/gpu/drm/panel/panel-sony-acx565akm.c
@@ -30,6 +30,7 @@
#include <drm/drm_connector.h>
#include <drm/drm_modes.h>
#include <drm/drm_panel.h>
+#include <drm/drm_panel_helper.h>
#define CTRL_DISP_BRIGHTNESS_CTRL_ON BIT(5)
#define CTRL_DISP_AMBIENT_LIGHT_CTRL_ON BIT(4)
@@ -454,9 +455,6 @@ static int acx565akm_power_on(struct acx565akm_panel *lcd)
static void acx565akm_power_off(struct acx565akm_panel *lcd)
{
- if (!lcd->enabled)
- return;
-
acx565akm_set_display_state(lcd, 0);
acx565akm_set_sleep_mode(lcd, 1);
lcd->enabled = false;
@@ -656,8 +654,7 @@ static void acx565akm_remove(struct spi_device *spi)
if (lcd->has_bc)
acx565akm_backlight_cleanup(lcd);
- drm_panel_disable(&lcd->panel);
- drm_panel_unprepare(&lcd->panel);
+ drm_panel_helper_shutdown(&lcd->panel);
}
static const struct of_device_id acx565akm_of_match[] = {
--
2.41.0.585.gd2178a4bd4-goog
As talked about in commit d2aacaf07395 ("drm/panel: Check for already
prepared/enabled in drm_panel"), we want to remove needless code from
panel drivers that was storing and double-checking the
prepared/enabled state. Even if someone was relying on the
double-check before, that double-check is now in the core and not
needed in individual drivers.
For the st7703 panel driver this is fairly straightforward. Like with
many other panels, we need to use the new drm_panel_helper_shutdown()
function to make sure that remove() and shutdown() don't try to
disable/unprepare a panel that hasn't been prepared/enabled. One thing
that is a little different for st7703 is that it has a special
"allpixelson" debugfs file. When this file is written the driver hacks
a disable/unprepare and then a prepare/enable to try to reset the
panel. This debugfs file didn't appear to be particularly safe to use
even before this patch since it would cause a disabled/unprepared
panel to become prepared/enabled. It is nominally even less safe after
this patch since calling it on a panel that wasn't prepared/enabled
will now likely cause a regulator underflow message. If this matters
to anyone, it could be fixed in a future patch.
Signed-off-by: Douglas Anderson <[email protected]>
---
drivers/gpu/drm/panel/panel-sitronix-st7703.c | 20 ++-----------------
1 file changed, 2 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7703.c b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
index 6a3945639535..dde903e803d1 100644
--- a/drivers/gpu/drm/panel/panel-sitronix-st7703.c
+++ b/drivers/gpu/drm/panel/panel-sitronix-st7703.c
@@ -22,6 +22,7 @@
#include <drm/drm_mipi_dsi.h>
#include <drm/drm_modes.h>
#include <drm/drm_panel.h>
+#include <drm/drm_panel_helper.h>
#define DRV_NAME "panel-sitronix-st7703"
@@ -58,7 +59,6 @@ struct st7703 {
struct gpio_desc *reset_gpio;
struct regulator *vcc;
struct regulator *iovcc;
- bool prepared;
struct dentry *debugfs;
const struct st7703_panel_desc *desc;
@@ -486,13 +486,9 @@ static int st7703_unprepare(struct drm_panel *panel)
{
struct st7703 *ctx = panel_to_st7703(panel);
- if (!ctx->prepared)
- return 0;
-
gpiod_set_value_cansleep(ctx->reset_gpio, 1);
regulator_disable(ctx->iovcc);
regulator_disable(ctx->vcc);
- ctx->prepared = false;
return 0;
}
@@ -502,9 +498,6 @@ static int st7703_prepare(struct drm_panel *panel)
struct st7703 *ctx = panel_to_st7703(panel);
int ret;
- if (ctx->prepared)
- return 0;
-
dev_dbg(ctx->dev, "Resetting the panel\n");
ret = regulator_enable(ctx->vcc);
if (ret < 0) {
@@ -522,8 +515,6 @@ static int st7703_prepare(struct drm_panel *panel)
gpiod_set_value_cansleep(ctx->reset_gpio, 0);
msleep(20);
- ctx->prepared = true;
-
return 0;
disable_vcc:
@@ -665,15 +656,8 @@ static int st7703_probe(struct mipi_dsi_device *dsi)
static void st7703_shutdown(struct mipi_dsi_device *dsi)
{
struct st7703 *ctx = mipi_dsi_get_drvdata(dsi);
- int ret;
- ret = drm_panel_unprepare(&ctx->panel);
- if (ret < 0)
- dev_err(&dsi->dev, "Failed to unprepare panel: %d\n", ret);
-
- ret = drm_panel_disable(&ctx->panel);
- if (ret < 0)
- dev_err(&dsi->dev, "Failed to disable panel: %d\n", ret);
+ drm_panel_helper_shutdown(&ctx->panel);
}
static void st7703_remove(struct mipi_dsi_device *dsi)
--
2.41.0.585.gd2178a4bd4-goog
As talked about in commit d2aacaf07395 ("drm/panel: Check for already
prepared/enabled in drm_panel"), we want to remove needless code from
panel drivers that was storing and double-checking the
prepared/enabled state. Even if someone was relying on the
double-check before, that double-check is now in the core and not
needed in individual drivers.
For the s6e63m0 panel driver, this actually fixes a subtle/minor error
handling bug in s6e63m0_prepare(). In one error case s6e63m0_prepare()
called s6e63m0_unprepare() directly if there was an error. This call
to s6e63m0_unprepare() would have been a no-op since ctx->prepared
wasn't set yet.
Signed-off-by: Douglas Anderson <[email protected]>
---
drivers/gpu/drm/panel/panel-samsung-s6e63m0.c | 25 -------------------
1 file changed, 25 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c b/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
index b34fa4d5de07..a0e5698275a5 100644
--- a/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
+++ b/drivers/gpu/drm/panel/panel-samsung-s6e63m0.c
@@ -270,9 +270,6 @@ struct s6e63m0 {
struct regulator_bulk_data supplies[2];
struct gpio_desc *reset_gpio;
- bool prepared;
- bool enabled;
-
/*
* This field is tested by functions directly accessing bus before
* transfer, transfer is skipped if it is set. In case of transfer
@@ -502,9 +499,6 @@ static int s6e63m0_disable(struct drm_panel *panel)
{
struct s6e63m0 *ctx = panel_to_s6e63m0(panel);
- if (!ctx->enabled)
- return 0;
-
backlight_disable(ctx->bl_dev);
s6e63m0_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_OFF);
@@ -512,8 +506,6 @@ static int s6e63m0_disable(struct drm_panel *panel)
s6e63m0_dcs_write_seq_static(ctx, MIPI_DCS_ENTER_SLEEP_MODE);
msleep(120);
- ctx->enabled = false;
-
return 0;
}
@@ -522,17 +514,12 @@ static int s6e63m0_unprepare(struct drm_panel *panel)
struct s6e63m0 *ctx = panel_to_s6e63m0(panel);
int ret;
- if (!ctx->prepared)
- return 0;
-
s6e63m0_clear_error(ctx);
ret = s6e63m0_power_off(ctx);
if (ret < 0)
return ret;
- ctx->prepared = false;
-
return 0;
}
@@ -541,9 +528,6 @@ static int s6e63m0_prepare(struct drm_panel *panel)
struct s6e63m0 *ctx = panel_to_s6e63m0(panel);
int ret;
- if (ctx->prepared)
- return 0;
-
ret = s6e63m0_power_on(ctx);
if (ret < 0)
return ret;
@@ -564,8 +548,6 @@ static int s6e63m0_prepare(struct drm_panel *panel)
if (ret < 0)
s6e63m0_unprepare(panel);
- ctx->prepared = true;
-
return ret;
}
@@ -573,9 +555,6 @@ static int s6e63m0_enable(struct drm_panel *panel)
{
struct s6e63m0 *ctx = panel_to_s6e63m0(panel);
- if (ctx->enabled)
- return 0;
-
s6e63m0_dcs_write_seq_static(ctx, MIPI_DCS_EXIT_SLEEP_MODE);
msleep(120);
s6e63m0_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_ON);
@@ -588,8 +567,6 @@ static int s6e63m0_enable(struct drm_panel *panel)
backlight_enable(ctx->bl_dev);
- ctx->enabled = true;
-
return 0;
}
@@ -709,8 +686,6 @@ int s6e63m0_probe(struct device *dev, void *trsp,
dev_set_drvdata(dev, ctx);
ctx->dev = dev;
- ctx->enabled = false;
- ctx->prepared = false;
ret = device_property_read_u32(dev, "max-brightness", &max_brightness);
if (ret)
--
2.41.0.585.gd2178a4bd4-goog
As talked about in commit d2aacaf07395 ("drm/panel: Check for already
prepared/enabled in drm_panel"), we want to remove needless code from
panel drivers that was storing and double-checking the
prepared/enabled state. Even if someone was relying on the
double-check before, that double-check is now in the core and not
needed in individual drivers.
The conversion of the rm67191 panel driver follows many of the other
panel drivers but is separated out because it has a few differences
that need to be called out.
Like otm8009a, this panel also uses the "prepared" flag to prevent the
backlight functions from running when the panel is powered off. This
is probably not the safest thing to do but the old behavior was
preserved. See the discussion in the patch ("drm/panel: otm8009a:
Don't double check prepared/enabled"). Because of this, I've left the
driver tracking "prepared" but removed its tracking of "enabled".
This panel also used to directly call its disable/unprepare functions
at shutdown time instead of calling into drm_panel. Now we're calling
drm_panel_helper_shutdown() which in turn calls
drm_panel_unprepare()/drm_panel_disable(). That paves the way if
anyone wants to use a the panel follower APIs with this panel.
Signed-off-by: Douglas Anderson <[email protected]>
---
drivers/gpu/drm/panel/panel-raydium-rm67191.c | 21 ++-----------------
1 file changed, 2 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-raydium-rm67191.c b/drivers/gpu/drm/panel/panel-raydium-rm67191.c
index dbb1ed4efbed..fb378924c0b3 100644
--- a/drivers/gpu/drm/panel/panel-raydium-rm67191.c
+++ b/drivers/gpu/drm/panel/panel-raydium-rm67191.c
@@ -20,6 +20,7 @@
#include <drm/drm_crtc.h>
#include <drm/drm_mipi_dsi.h>
#include <drm/drm_panel.h>
+#include <drm/drm_panel_helper.h>
/* Panel specific color-format bits */
#define COL_FMT_16BPP 0x55
@@ -205,7 +206,6 @@ struct rad_panel {
unsigned int num_supplies;
bool prepared;
- bool enabled;
};
static const struct drm_display_mode default_mode = {
@@ -267,9 +267,6 @@ static int rad_panel_prepare(struct drm_panel *panel)
struct rad_panel *rad = to_rad_panel(panel);
int ret;
- if (rad->prepared)
- return 0;
-
ret = regulator_bulk_enable(rad->num_supplies, rad->supplies);
if (ret)
return ret;
@@ -291,9 +288,6 @@ static int rad_panel_unprepare(struct drm_panel *panel)
struct rad_panel *rad = to_rad_panel(panel);
int ret;
- if (!rad->prepared)
- return 0;
-
/*
* Right after asserting the reset, we need to release it, so that the
* touch driver can have an active connection with the touch controller
@@ -322,9 +316,6 @@ static int rad_panel_enable(struct drm_panel *panel)
int color_format = color_format_from_dsi_format(dsi->format);
int ret;
- if (rad->enabled)
- return 0;
-
dsi->mode_flags |= MIPI_DSI_MODE_LPM;
ret = rad_panel_push_cmd_list(dsi);
@@ -389,8 +380,6 @@ static int rad_panel_enable(struct drm_panel *panel)
backlight_enable(rad->backlight);
- rad->enabled = true;
-
return 0;
fail:
@@ -406,9 +395,6 @@ static int rad_panel_disable(struct drm_panel *panel)
struct device *dev = &dsi->dev;
int ret;
- if (!rad->enabled)
- return 0;
-
dsi->mode_flags |= MIPI_DSI_MODE_LPM;
backlight_disable(rad->backlight);
@@ -429,8 +415,6 @@ static int rad_panel_disable(struct drm_panel *panel)
return ret;
}
- rad->enabled = false;
-
return 0;
}
@@ -633,8 +617,7 @@ static void rad_panel_shutdown(struct mipi_dsi_device *dsi)
{
struct rad_panel *rad = mipi_dsi_get_drvdata(dsi);
- rad_panel_disable(&rad->panel);
- rad_panel_unprepare(&rad->panel);
+ drm_panel_helper_shutdown(&rad->panel);
}
static const struct of_device_id rad_of_match[] = {
--
2.41.0.585.gd2178a4bd4-goog
The goal of this file is to contain helper functions for panel drivers
to use. To start off with, let's add drm_panel_helper_shutdown() for
use by panels that want to make sure they're powered off at
shutdown/remove time if they happen to be powered on.
The main goal of introducting this function is so that panel drivers
don't need to track the enabled/prepared state themselves.
Signed-off-by: Douglas Anderson <[email protected]>
---
If I've misunderstood and the drm_panel_helper_shutdown() should
belong in some other file and we don't need to introduce a "helper"
for this then please le me know.
drivers/gpu/drm/Makefile | 1 +
drivers/gpu/drm/drm_panel_helper.c | 37 ++++++++++++++++++++++++++++++
include/drm/drm_panel_helper.h | 13 +++++++++++
3 files changed, 51 insertions(+)
create mode 100644 drivers/gpu/drm/drm_panel_helper.c
create mode 100644 include/drm/drm_panel_helper.h
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 215e78e79125..e811f3d68235 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -118,6 +118,7 @@ drm_kms_helper-y := \
drm_gem_framebuffer_helper.o \
drm_kms_helper_common.o \
drm_modeset_helper.o \
+ drm_panel_helper.o \
drm_plane_helper.o \
drm_probe_helper.o \
drm_rect.o \
diff --git a/drivers/gpu/drm/drm_panel_helper.c b/drivers/gpu/drm/drm_panel_helper.c
new file mode 100644
index 000000000000..85a55b5731cf
--- /dev/null
+++ b/drivers/gpu/drm/drm_panel_helper.c
@@ -0,0 +1,37 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2023 Google Inc.
+ */
+
+#include <linux/dev_printk.h>
+
+#include <drm/drm_panel.h>
+#include <drm/drm_panel_helper.h>
+
+/**
+ * drm_panel_helper_shutdown - helper for panels to use at shutdown time
+ * @panel: DRM panel
+ *
+ * Panels may call this function unconditionally at shutdown time to ensure
+ * that they are disabled and unprepared if necessary.
+ *
+ * As part of this function:
+ * - The backlight will be turned off, if it was on.
+ * - Any panel followers will be power sequenced.
+ */
+void drm_panel_helper_shutdown(struct drm_panel *panel)
+{
+ int ret;
+
+ if (panel->enabled) {
+ ret = drm_panel_disable(panel);
+ if (ret)
+ dev_warn(panel->dev, "Error disabling panel %d\n", ret);
+ }
+ if (panel->prepared) {
+ ret = drm_panel_unprepare(panel);
+ if (ret)
+ dev_warn(panel->dev, "Error unpreparing panel %d\n", ret);
+ }
+}
+EXPORT_SYMBOL_GPL(drm_panel_helper_shutdown);
diff --git a/include/drm/drm_panel_helper.h b/include/drm/drm_panel_helper.h
new file mode 100644
index 000000000000..5621482053a9
--- /dev/null
+++ b/include/drm/drm_panel_helper.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright 2023 Google Inc.
+ */
+
+#ifndef DRM_PANEL_HELPER_H
+#define DRM_PANEL_HELPER_H
+
+struct drm_panel;
+
+void drm_panel_helper_shutdown(struct drm_panel *panel);
+
+#endif /* DRM_PANEL_HELPER_H */
--
2.41.0.585.gd2178a4bd4-goog
As talked about in commit d2aacaf07395 ("drm/panel: Check for already
prepared/enabled in drm_panel"), we want to remove needless code from
panel drivers that was storing and double-checking the
prepared/enabled state. Even if someone was relying on the
double-check before, that double-check is now in the core and not
needed in individual drivers.
This pile of panel drivers appears to be simple to handle. Based on
code inspection they seemed to be using the prepared/enabled state
simply for double-checking that nothing else in the kernel called them
inconsistently. Now that the core drm_panel is doing the double
checking (and warning) it should be very clear that these devices
don't need their own double-check.
Signed-off-by: Douglas Anderson <[email protected]>
---
.../drm/panel/panel-asus-z00t-tm5p5-n35596.c | 9 -----
.../gpu/drm/panel/panel-boe-bf060y8m-aj0.c | 9 -----
drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c | 9 -----
drivers/gpu/drm/panel/panel-novatek-nt35950.c | 9 -----
drivers/gpu/drm/panel/panel-novatek-nt36523.c | 12 ------
drivers/gpu/drm/panel/panel-raydium-rm68200.c | 38 -------------------
.../panel/panel-samsung-s6e88a0-ams452ef01.c | 10 -----
drivers/gpu/drm/panel/panel-samsung-sofef00.c | 9 -----
.../gpu/drm/panel/panel-sharp-ls060t1sx01.c | 10 -----
drivers/gpu/drm/panel/panel-sony-td4353-jdi.c | 9 -----
.../panel/panel-sony-tulip-truly-nt35521.c | 18 ---------
.../drm/panel/panel-startek-kd070fhfid015.c | 11 ------
drivers/gpu/drm/panel/panel-truly-nt35597.c | 20 ----------
drivers/gpu/drm/panel/panel-visionox-r66451.c | 16 --------
.../gpu/drm/panel/panel-visionox-rm69299.c | 8 ----
.../gpu/drm/panel/panel-visionox-vtdr6130.c | 9 -----
16 files changed, 206 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c b/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c
index 075a7af81eff..bcaa63d1955f 100644
--- a/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c
+++ b/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c
@@ -16,7 +16,6 @@ struct tm5p5_nt35596 {
struct mipi_dsi_device *dsi;
struct regulator_bulk_data supplies[2];
struct gpio_desc *reset_gpio;
- bool prepared;
};
static inline struct tm5p5_nt35596 *to_tm5p5_nt35596(struct drm_panel *panel)
@@ -112,9 +111,6 @@ static int tm5p5_nt35596_prepare(struct drm_panel *panel)
struct device *dev = &ctx->dsi->dev;
int ret;
- if (ctx->prepared)
- return 0;
-
ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
if (ret < 0) {
dev_err(dev, "Failed to enable regulators: %d\n", ret);
@@ -132,7 +128,6 @@ static int tm5p5_nt35596_prepare(struct drm_panel *panel)
return ret;
}
- ctx->prepared = true;
return 0;
}
@@ -142,9 +137,6 @@ static int tm5p5_nt35596_unprepare(struct drm_panel *panel)
struct device *dev = &ctx->dsi->dev;
int ret;
- if (!ctx->prepared)
- return 0;
-
ret = tm5p5_nt35596_off(ctx);
if (ret < 0)
dev_err(dev, "Failed to un-initialize panel: %d\n", ret);
@@ -153,7 +145,6 @@ static int tm5p5_nt35596_unprepare(struct drm_panel *panel)
regulator_bulk_disable(ARRAY_SIZE(ctx->supplies),
ctx->supplies);
- ctx->prepared = false;
return 0;
}
diff --git a/drivers/gpu/drm/panel/panel-boe-bf060y8m-aj0.c b/drivers/gpu/drm/panel/panel-boe-bf060y8m-aj0.c
index 90098b753e3b..e77db8597eb7 100644
--- a/drivers/gpu/drm/panel/panel-boe-bf060y8m-aj0.c
+++ b/drivers/gpu/drm/panel/panel-boe-bf060y8m-aj0.c
@@ -34,7 +34,6 @@ struct boe_bf060y8m_aj0 {
struct mipi_dsi_device *dsi;
struct regulator_bulk_data vregs[BF060Y8M_VREG_MAX];
struct gpio_desc *reset_gpio;
- bool prepared;
};
static inline
@@ -129,9 +128,6 @@ static int boe_bf060y8m_aj0_prepare(struct drm_panel *panel)
struct device *dev = &boe->dsi->dev;
int ret;
- if (boe->prepared)
- return 0;
-
/*
* Enable EL Driving Voltage first - doing that at the beginning
* or at the end of the power sequence doesn't matter, so enable
@@ -166,7 +162,6 @@ static int boe_bf060y8m_aj0_prepare(struct drm_panel *panel)
return ret;
}
- boe->prepared = true;
return 0;
err_vci:
@@ -186,9 +181,6 @@ static int boe_bf060y8m_aj0_unprepare(struct drm_panel *panel)
struct device *dev = &boe->dsi->dev;
int ret;
- if (!boe->prepared)
- return 0;
-
ret = boe_bf060y8m_aj0_off(boe);
if (ret < 0)
dev_err(dev, "Failed to un-initialize panel: %d\n", ret);
@@ -196,7 +188,6 @@ static int boe_bf060y8m_aj0_unprepare(struct drm_panel *panel)
gpiod_set_value_cansleep(boe->reset_gpio, 1);
ret = regulator_bulk_disable(ARRAY_SIZE(boe->vregs), boe->vregs);
- boe->prepared = false;
return 0;
}
diff --git a/drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c b/drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c
index 8912757a6f42..3e0a8e0d58a0 100644
--- a/drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c
+++ b/drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c
@@ -21,7 +21,6 @@ struct jdi_fhd_r63452 {
struct drm_panel panel;
struct mipi_dsi_device *dsi;
struct gpio_desc *reset_gpio;
- bool prepared;
};
static inline struct jdi_fhd_r63452 *to_jdi_fhd_r63452(struct drm_panel *panel)
@@ -157,9 +156,6 @@ static int jdi_fhd_r63452_prepare(struct drm_panel *panel)
struct device *dev = &ctx->dsi->dev;
int ret;
- if (ctx->prepared)
- return 0;
-
jdi_fhd_r63452_reset(ctx);
ret = jdi_fhd_r63452_on(ctx);
@@ -169,7 +165,6 @@ static int jdi_fhd_r63452_prepare(struct drm_panel *panel)
return ret;
}
- ctx->prepared = true;
return 0;
}
@@ -179,16 +174,12 @@ static int jdi_fhd_r63452_unprepare(struct drm_panel *panel)
struct device *dev = &ctx->dsi->dev;
int ret;
- if (!ctx->prepared)
- return 0;
-
ret = jdi_fhd_r63452_off(ctx);
if (ret < 0)
dev_err(dev, "Failed to un-initialize panel: %d\n", ret);
gpiod_set_value_cansleep(ctx->reset_gpio, 1);
- ctx->prepared = false;
return 0;
}
diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35950.c b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
index 412ca84d0581..648ce9201426 100644
--- a/drivers/gpu/drm/panel/panel-novatek-nt35950.c
+++ b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
@@ -59,7 +59,6 @@ struct nt35950 {
int cur_mode;
u8 last_page;
- bool prepared;
};
struct nt35950_panel_mode {
@@ -431,9 +430,6 @@ static int nt35950_prepare(struct drm_panel *panel)
struct device *dev = &nt->dsi[0]->dev;
int ret;
- if (nt->prepared)
- return 0;
-
ret = regulator_enable(nt->vregs[0].consumer);
if (ret)
return ret;
@@ -460,7 +456,6 @@ static int nt35950_prepare(struct drm_panel *panel)
dev_err(dev, "Failed to initialize panel: %d\n", ret);
goto end;
}
- nt->prepared = true;
end:
if (ret < 0) {
@@ -477,9 +472,6 @@ static int nt35950_unprepare(struct drm_panel *panel)
struct device *dev = &nt->dsi[0]->dev;
int ret;
- if (!nt->prepared)
- return 0;
-
ret = nt35950_off(nt);
if (ret < 0)
dev_err(dev, "Failed to deinitialize panel: %d\n", ret);
@@ -487,7 +479,6 @@ static int nt35950_unprepare(struct drm_panel *panel)
gpiod_set_value_cansleep(nt->reset_gpio, 0);
regulator_bulk_disable(ARRAY_SIZE(nt->vregs), nt->vregs);
- nt->prepared = false;
return 0;
}
diff --git a/drivers/gpu/drm/panel/panel-novatek-nt36523.c b/drivers/gpu/drm/panel/panel-novatek-nt36523.c
index 9632b9e95b71..9b9a7eb1bc60 100644
--- a/drivers/gpu/drm/panel/panel-novatek-nt36523.c
+++ b/drivers/gpu/drm/panel/panel-novatek-nt36523.c
@@ -38,8 +38,6 @@ struct panel_info {
struct gpio_desc *reset_gpio;
struct backlight_device *backlight;
struct regulator *vddio;
-
- bool prepared;
};
struct panel_desc {
@@ -1046,9 +1044,6 @@ static int nt36523_prepare(struct drm_panel *panel)
struct panel_info *pinfo = to_panel_info(panel);
int ret;
- if (pinfo->prepared)
- return 0;
-
ret = regulator_enable(pinfo->vddio);
if (ret) {
dev_err(panel->dev, "failed to enable vddio regulator: %d\n", ret);
@@ -1064,8 +1059,6 @@ static int nt36523_prepare(struct drm_panel *panel)
return ret;
}
- pinfo->prepared = true;
-
return 0;
}
@@ -1095,14 +1088,9 @@ static int nt36523_unprepare(struct drm_panel *panel)
{
struct panel_info *pinfo = to_panel_info(panel);
- if (!pinfo->prepared)
- return 0;
-
gpiod_set_value_cansleep(pinfo->reset_gpio, 1);
regulator_disable(pinfo->vddio);
- pinfo->prepared = false;
-
return 0;
}
diff --git a/drivers/gpu/drm/panel/panel-raydium-rm68200.c b/drivers/gpu/drm/panel/panel-raydium-rm68200.c
index 5f9b340588fb..7b7fe987e292 100644
--- a/drivers/gpu/drm/panel/panel-raydium-rm68200.c
+++ b/drivers/gpu/drm/panel/panel-raydium-rm68200.c
@@ -77,8 +77,6 @@ struct rm68200 {
struct drm_panel panel;
struct gpio_desc *reset_gpio;
struct regulator *supply;
- bool prepared;
- bool enabled;
};
static const struct drm_display_mode default_mode = {
@@ -231,27 +229,12 @@ static void rm68200_init_sequence(struct rm68200 *ctx)
dcs_write_seq(ctx, MCS_CMD_MODE_SW, MCS_CMD1_UCS);
}
-static int rm68200_disable(struct drm_panel *panel)
-{
- struct rm68200 *ctx = panel_to_rm68200(panel);
-
- if (!ctx->enabled)
- return 0;
-
- ctx->enabled = false;
-
- return 0;
-}
-
static int rm68200_unprepare(struct drm_panel *panel)
{
struct rm68200 *ctx = panel_to_rm68200(panel);
struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
int ret;
- if (!ctx->prepared)
- return 0;
-
ret = mipi_dsi_dcs_set_display_off(dsi);
if (ret)
dev_warn(panel->dev, "failed to set display off: %d\n", ret);
@@ -269,8 +252,6 @@ static int rm68200_unprepare(struct drm_panel *panel)
regulator_disable(ctx->supply);
- ctx->prepared = false;
-
return 0;
}
@@ -280,9 +261,6 @@ static int rm68200_prepare(struct drm_panel *panel)
struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
int ret;
- if (ctx->prepared)
- return 0;
-
ret = regulator_enable(ctx->supply);
if (ret < 0) {
dev_err(ctx->dev, "failed to enable supply: %d\n", ret);
@@ -310,20 +288,6 @@ static int rm68200_prepare(struct drm_panel *panel)
msleep(20);
- ctx->prepared = true;
-
- return 0;
-}
-
-static int rm68200_enable(struct drm_panel *panel)
-{
- struct rm68200 *ctx = panel_to_rm68200(panel);
-
- if (ctx->enabled)
- return 0;
-
- ctx->enabled = true;
-
return 0;
}
@@ -352,10 +316,8 @@ static int rm68200_get_modes(struct drm_panel *panel,
}
static const struct drm_panel_funcs rm68200_drm_funcs = {
- .disable = rm68200_disable,
.unprepare = rm68200_unprepare,
.prepare = rm68200_prepare,
- .enable = rm68200_enable,
.get_modes = rm68200_get_modes,
};
diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e88a0-ams452ef01.c b/drivers/gpu/drm/panel/panel-samsung-s6e88a0-ams452ef01.c
index 7431cae7427e..d2df227abbea 100644
--- a/drivers/gpu/drm/panel/panel-samsung-s6e88a0-ams452ef01.c
+++ b/drivers/gpu/drm/panel/panel-samsung-s6e88a0-ams452ef01.c
@@ -18,8 +18,6 @@ struct s6e88a0_ams452ef01 {
struct mipi_dsi_device *dsi;
struct regulator_bulk_data supplies[2];
struct gpio_desc *reset_gpio;
-
- bool prepared;
};
static inline struct
@@ -115,9 +113,6 @@ static int s6e88a0_ams452ef01_prepare(struct drm_panel *panel)
struct device *dev = &ctx->dsi->dev;
int ret;
- if (ctx->prepared)
- return 0;
-
ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
if (ret < 0) {
dev_err(dev, "Failed to enable regulators: %d\n", ret);
@@ -135,7 +130,6 @@ static int s6e88a0_ams452ef01_prepare(struct drm_panel *panel)
return ret;
}
- ctx->prepared = true;
return 0;
}
@@ -145,9 +139,6 @@ static int s6e88a0_ams452ef01_unprepare(struct drm_panel *panel)
struct device *dev = &ctx->dsi->dev;
int ret;
- if (!ctx->prepared)
- return 0;
-
ret = s6e88a0_ams452ef01_off(ctx);
if (ret < 0)
dev_err(dev, "Failed to un-initialize panel: %d\n", ret);
@@ -155,7 +146,6 @@ static int s6e88a0_ams452ef01_unprepare(struct drm_panel *panel)
gpiod_set_value_cansleep(ctx->reset_gpio, 0);
regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
- ctx->prepared = false;
return 0;
}
diff --git a/drivers/gpu/drm/panel/panel-samsung-sofef00.c b/drivers/gpu/drm/panel/panel-samsung-sofef00.c
index cbf9607dd576..04ce925b3d9d 100644
--- a/drivers/gpu/drm/panel/panel-samsung-sofef00.c
+++ b/drivers/gpu/drm/panel/panel-samsung-sofef00.c
@@ -23,7 +23,6 @@ struct sofef00_panel {
struct regulator *supply;
struct gpio_desc *reset_gpio;
const struct drm_display_mode *mode;
- bool prepared;
};
static inline
@@ -113,9 +112,6 @@ static int sofef00_panel_prepare(struct drm_panel *panel)
struct device *dev = &ctx->dsi->dev;
int ret;
- if (ctx->prepared)
- return 0;
-
ret = regulator_enable(ctx->supply);
if (ret < 0) {
dev_err(dev, "Failed to enable regulator: %d\n", ret);
@@ -131,7 +127,6 @@ static int sofef00_panel_prepare(struct drm_panel *panel)
return ret;
}
- ctx->prepared = true;
return 0;
}
@@ -141,16 +136,12 @@ static int sofef00_panel_unprepare(struct drm_panel *panel)
struct device *dev = &ctx->dsi->dev;
int ret;
- if (!ctx->prepared)
- return 0;
-
ret = sofef00_panel_off(ctx);
if (ret < 0)
dev_err(dev, "Failed to un-initialize panel: %d\n", ret);
regulator_disable(ctx->supply);
- ctx->prepared = false;
return 0;
}
diff --git a/drivers/gpu/drm/panel/panel-sharp-ls060t1sx01.c b/drivers/gpu/drm/panel/panel-sharp-ls060t1sx01.c
index 68f52eaaf4fa..74c760ee0c2d 100644
--- a/drivers/gpu/drm/panel/panel-sharp-ls060t1sx01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-ls060t1sx01.c
@@ -24,7 +24,6 @@ struct sharp_ls060 {
struct regulator *avdd_supply;
struct regulator *avee_supply;
struct gpio_desc *reset_gpio;
- bool prepared;
};
static inline struct sharp_ls060 *to_sharp_ls060(struct drm_panel *panel)
@@ -101,9 +100,6 @@ static int sharp_ls060_prepare(struct drm_panel *panel)
struct device *dev = &ctx->dsi->dev;
int ret;
- if (ctx->prepared)
- return 0;
-
ret = regulator_enable(ctx->vddi_supply);
if (ret < 0)
return ret;
@@ -134,8 +130,6 @@ static int sharp_ls060_prepare(struct drm_panel *panel)
goto err_on;
}
- ctx->prepared = true;
-
return 0;
err_on:
@@ -163,9 +157,6 @@ static int sharp_ls060_unprepare(struct drm_panel *panel)
struct device *dev = &ctx->dsi->dev;
int ret;
- if (!ctx->prepared)
- return 0;
-
ret = sharp_ls060_off(ctx);
if (ret < 0)
dev_err(dev, "Failed to un-initialize panel: %d\n", ret);
@@ -181,7 +172,6 @@ static int sharp_ls060_unprepare(struct drm_panel *panel)
regulator_disable(ctx->vddi_supply);
- ctx->prepared = false;
return 0;
}
diff --git a/drivers/gpu/drm/panel/panel-sony-td4353-jdi.c b/drivers/gpu/drm/panel/panel-sony-td4353-jdi.c
index 1bde2f01786b..472195d4bbbe 100644
--- a/drivers/gpu/drm/panel/panel-sony-td4353-jdi.c
+++ b/drivers/gpu/drm/panel/panel-sony-td4353-jdi.c
@@ -36,7 +36,6 @@ struct sony_td4353_jdi {
struct regulator_bulk_data supplies[3];
struct gpio_desc *panel_reset_gpio;
struct gpio_desc *touch_reset_gpio;
- bool prepared;
int type;
};
@@ -150,9 +149,6 @@ static int sony_td4353_jdi_prepare(struct drm_panel *panel)
struct device *dev = &ctx->dsi->dev;
int ret;
- if (ctx->prepared)
- return 0;
-
ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
if (ret < 0) {
dev_err(dev, "Failed to enable regulators: %d\n", ret);
@@ -171,7 +167,6 @@ static int sony_td4353_jdi_prepare(struct drm_panel *panel)
return ret;
}
- ctx->prepared = true;
return 0;
}
@@ -181,9 +176,6 @@ static int sony_td4353_jdi_unprepare(struct drm_panel *panel)
struct device *dev = &ctx->dsi->dev;
int ret;
- if (!ctx->prepared)
- return 0;
-
ret = sony_td4353_jdi_off(ctx);
if (ret < 0)
dev_err(dev, "Failed to power off panel: %d\n", ret);
@@ -191,7 +183,6 @@ static int sony_td4353_jdi_unprepare(struct drm_panel *panel)
sony_td4353_assert_reset_gpios(ctx, 0);
regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
- ctx->prepared = false;
return 0;
}
diff --git a/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c b/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c
index ee5d20ecc577..6d44970dccd9 100644
--- a/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c
+++ b/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c
@@ -23,8 +23,6 @@ struct truly_nt35521 {
struct regulator_bulk_data supplies[2];
struct gpio_desc *reset_gpio;
struct gpio_desc *blen_gpio;
- bool prepared;
- bool enabled;
};
static inline
@@ -296,9 +294,6 @@ static int truly_nt35521_prepare(struct drm_panel *panel)
struct device *dev = &ctx->dsi->dev;
int ret;
- if (ctx->prepared)
- return 0;
-
ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
if (ret < 0) {
dev_err(dev, "Failed to enable regulators: %d\n", ret);
@@ -314,7 +309,6 @@ static int truly_nt35521_prepare(struct drm_panel *panel)
return ret;
}
- ctx->prepared = true;
return 0;
}
@@ -324,9 +318,6 @@ static int truly_nt35521_unprepare(struct drm_panel *panel)
struct device *dev = &ctx->dsi->dev;
int ret;
- if (!ctx->prepared)
- return 0;
-
ret = truly_nt35521_off(ctx);
if (ret < 0)
dev_err(dev, "Failed to un-initialize panel: %d\n", ret);
@@ -335,7 +326,6 @@ static int truly_nt35521_unprepare(struct drm_panel *panel)
regulator_bulk_disable(ARRAY_SIZE(ctx->supplies),
ctx->supplies);
- ctx->prepared = false;
return 0;
}
@@ -343,12 +333,8 @@ static int truly_nt35521_enable(struct drm_panel *panel)
{
struct truly_nt35521 *ctx = to_truly_nt35521(panel);
- if (ctx->enabled)
- return 0;
-
gpiod_set_value_cansleep(ctx->blen_gpio, 1);
- ctx->enabled = true;
return 0;
}
@@ -356,12 +342,8 @@ static int truly_nt35521_disable(struct drm_panel *panel)
{
struct truly_nt35521 *ctx = to_truly_nt35521(panel);
- if (!ctx->enabled)
- return 0;
-
gpiod_set_value_cansleep(ctx->blen_gpio, 0);
- ctx->enabled = false;
return 0;
}
diff --git a/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c b/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c
index 6e77a2d71d81..0156689f41cd 100644
--- a/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c
+++ b/drivers/gpu/drm/panel/panel-startek-kd070fhfid015.c
@@ -35,7 +35,6 @@ enum {
};
struct stk_panel {
- bool prepared;
const struct drm_display_mode *mode;
struct backlight_device *backlight;
struct drm_panel base;
@@ -145,16 +144,11 @@ static int stk_panel_unprepare(struct drm_panel *panel)
{
struct stk_panel *stk = to_stk_panel(panel);
- if (!stk->prepared)
- return 0;
-
stk_panel_off(stk);
regulator_bulk_disable(ARRAY_SIZE(stk->supplies), stk->supplies);
gpiod_set_value(stk->reset_gpio, 0);
gpiod_set_value(stk->enable_gpio, 1);
- stk->prepared = false;
-
return 0;
}
@@ -164,9 +158,6 @@ static int stk_panel_prepare(struct drm_panel *panel)
struct device *dev = &stk->dsi->dev;
int ret;
- if (stk->prepared)
- return 0;
-
gpiod_set_value(stk->reset_gpio, 0);
gpiod_set_value(stk->enable_gpio, 0);
ret = regulator_enable(stk->supplies[IOVCC].consumer);
@@ -195,8 +186,6 @@ static int stk_panel_prepare(struct drm_panel *panel)
goto poweroff;
}
- stk->prepared = true;
-
return 0;
poweroff:
diff --git a/drivers/gpu/drm/panel/panel-truly-nt35597.c b/drivers/gpu/drm/panel/panel-truly-nt35597.c
index 4f4009f9fe25..b73448cf349d 100644
--- a/drivers/gpu/drm/panel/panel-truly-nt35597.c
+++ b/drivers/gpu/drm/panel/panel-truly-nt35597.c
@@ -64,8 +64,6 @@ struct truly_nt35597 {
struct mipi_dsi_device *dsi[2];
const struct nt35597_config *config;
- bool prepared;
- bool enabled;
};
static inline struct truly_nt35597 *panel_to_ctx(struct drm_panel *panel)
@@ -313,16 +311,12 @@ static int truly_nt35597_disable(struct drm_panel *panel)
struct truly_nt35597 *ctx = panel_to_ctx(panel);
int ret;
- if (!ctx->enabled)
- return 0;
-
if (ctx->backlight) {
ret = backlight_disable(ctx->backlight);
if (ret < 0)
dev_err(ctx->dev, "backlight disable failed %d\n", ret);
}
- ctx->enabled = false;
return 0;
}
@@ -331,9 +325,6 @@ static int truly_nt35597_unprepare(struct drm_panel *panel)
struct truly_nt35597 *ctx = panel_to_ctx(panel);
int ret = 0;
- if (!ctx->prepared)
- return 0;
-
ctx->dsi[0]->mode_flags = 0;
ctx->dsi[1]->mode_flags = 0;
@@ -354,7 +345,6 @@ static int truly_nt35597_unprepare(struct drm_panel *panel)
if (ret < 0)
dev_err(ctx->dev, "power_off failed ret = %d\n", ret);
- ctx->prepared = false;
return ret;
}
@@ -367,9 +357,6 @@ static int truly_nt35597_prepare(struct drm_panel *panel)
const struct nt35597_config *config;
u32 num_cmds;
- if (ctx->prepared)
- return 0;
-
ret = truly_35597_power_on(ctx);
if (ret < 0)
return ret;
@@ -409,8 +396,6 @@ static int truly_nt35597_prepare(struct drm_panel *panel)
/* Per DSI spec wait 120ms after sending set_display_on DCS command */
msleep(120);
- ctx->prepared = true;
-
return 0;
power_off:
@@ -424,17 +409,12 @@ static int truly_nt35597_enable(struct drm_panel *panel)
struct truly_nt35597 *ctx = panel_to_ctx(panel);
int ret;
- if (ctx->enabled)
- return 0;
-
if (ctx->backlight) {
ret = backlight_enable(ctx->backlight);
if (ret < 0)
dev_err(ctx->dev, "backlight enable failed %d\n", ret);
}
- ctx->enabled = true;
-
return 0;
}
diff --git a/drivers/gpu/drm/panel/panel-visionox-r66451.c b/drivers/gpu/drm/panel/panel-visionox-r66451.c
index 00fc28ad3d07..fbb73464de33 100644
--- a/drivers/gpu/drm/panel/panel-visionox-r66451.c
+++ b/drivers/gpu/drm/panel/panel-visionox-r66451.c
@@ -22,7 +22,6 @@ struct visionox_r66451 {
struct mipi_dsi_device *dsi;
struct gpio_desc *reset_gpio;
struct regulator_bulk_data supplies[2];
- bool prepared, enabled;
};
static inline struct visionox_r66451 *to_visionox_r66451(struct drm_panel *panel)
@@ -124,9 +123,6 @@ static int visionox_r66451_prepare(struct drm_panel *panel)
struct device *dev = &dsi->dev;
int ret;
- if (ctx->prepared)
- return 0;
-
ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies),
ctx->supplies);
if (ret < 0)
@@ -144,7 +140,6 @@ static int visionox_r66451_prepare(struct drm_panel *panel)
mipi_dsi_compression_mode(ctx->dsi, true);
- ctx->prepared = true;
return 0;
}
@@ -154,9 +149,6 @@ static int visionox_r66451_unprepare(struct drm_panel *panel)
struct device *dev = &ctx->dsi->dev;
int ret;
- if (!ctx->prepared)
- return 0;
-
ret = visionox_r66451_off(ctx);
if (ret < 0)
dev_err(dev, "Failed to un-initialize panel: %d\n", ret);
@@ -164,7 +156,6 @@ static int visionox_r66451_unprepare(struct drm_panel *panel)
gpiod_set_value_cansleep(ctx->reset_gpio, 1);
regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
- ctx->prepared = false;
return 0;
}
@@ -190,9 +181,6 @@ static int visionox_r66451_enable(struct drm_panel *panel)
struct drm_dsc_picture_parameter_set pps;
int ret;
- if (ctx->enabled)
- return 0;
-
if (!dsi->dsc) {
dev_err(&dsi->dev, "DSC not attached to DSI\n");
return -ENODEV;
@@ -219,8 +207,6 @@ static int visionox_r66451_enable(struct drm_panel *panel)
}
msleep(20);
- ctx->enabled = true;
-
return 0;
}
@@ -231,8 +217,6 @@ static int visionox_r66451_disable(struct drm_panel *panel)
struct device *dev = &dsi->dev;
int ret;
- ctx->enabled = false;
-
ret = mipi_dsi_dcs_set_display_off(dsi);
if (ret < 0) {
dev_err(dev, "Failed to set display off: %d\n", ret);
diff --git a/drivers/gpu/drm/panel/panel-visionox-rm69299.c b/drivers/gpu/drm/panel/panel-visionox-rm69299.c
index c2806e4fd553..775144695283 100644
--- a/drivers/gpu/drm/panel/panel-visionox-rm69299.c
+++ b/drivers/gpu/drm/panel/panel-visionox-rm69299.c
@@ -20,8 +20,6 @@ struct visionox_rm69299 {
struct regulator_bulk_data supplies[2];
struct gpio_desc *reset_gpio;
struct mipi_dsi_device *dsi;
- bool prepared;
- bool enabled;
};
static inline struct visionox_rm69299 *panel_to_ctx(struct drm_panel *panel)
@@ -80,7 +78,6 @@ static int visionox_rm69299_unprepare(struct drm_panel *panel)
ret = visionox_rm69299_power_off(ctx);
- ctx->prepared = false;
return ret;
}
@@ -89,9 +86,6 @@ static int visionox_rm69299_prepare(struct drm_panel *panel)
struct visionox_rm69299 *ctx = panel_to_ctx(panel);
int ret;
- if (ctx->prepared)
- return 0;
-
ret = visionox_rm69299_power_on(ctx);
if (ret < 0)
return ret;
@@ -140,8 +134,6 @@ static int visionox_rm69299_prepare(struct drm_panel *panel)
/* Per DSI spec wait 120ms after sending set_display_on DCS command */
msleep(120);
- ctx->prepared = true;
-
return 0;
power_off:
diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
index bb0dfd86ea67..a23407b9f6fb 100644
--- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
+++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
@@ -20,7 +20,6 @@ struct visionox_vtdr6130 {
struct mipi_dsi_device *dsi;
struct gpio_desc *reset_gpio;
struct regulator_bulk_data supplies[3];
- bool prepared;
};
static inline struct visionox_vtdr6130 *to_visionox_vtdr6130(struct drm_panel *panel)
@@ -157,9 +156,6 @@ static int visionox_vtdr6130_prepare(struct drm_panel *panel)
struct device *dev = &ctx->dsi->dev;
int ret;
- if (ctx->prepared)
- return 0;
-
ret = regulator_bulk_enable(ARRAY_SIZE(ctx->supplies),
ctx->supplies);
if (ret < 0)
@@ -175,7 +171,6 @@ static int visionox_vtdr6130_prepare(struct drm_panel *panel)
return ret;
}
- ctx->prepared = true;
return 0;
}
@@ -185,9 +180,6 @@ static int visionox_vtdr6130_unprepare(struct drm_panel *panel)
struct device *dev = &ctx->dsi->dev;
int ret;
- if (!ctx->prepared)
- return 0;
-
ret = visionox_vtdr6130_off(ctx);
if (ret < 0)
dev_err(dev, "Failed to un-initialize panel: %d\n", ret);
@@ -196,7 +188,6 @@ static int visionox_vtdr6130_unprepare(struct drm_panel *panel)
regulator_bulk_disable(ARRAY_SIZE(ctx->supplies), ctx->supplies);
- ctx->prepared = false;
return 0;
}
--
2.41.0.585.gd2178a4bd4-goog
Now that most panels have been updated not to track/double-check their
prepared/enabled state update the TODO with next steps.
Signed-off-by: Douglas Anderson <[email protected]>
---
Documentation/gpu/todo.rst | 33 +++++++++++++--------------------
1 file changed, 13 insertions(+), 20 deletions(-)
diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 139980487ccf..c73d9dbebbf4 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -460,26 +460,19 @@ Contact: Thomas Zimmermann <[email protected]>
Level: Starter
-Clean up checks for already prepared/enabled in panels
-------------------------------------------------------
-
-In a whole pile of panel drivers, we have code to make the
-prepare/unprepare/enable/disable callbacks behave as no-ops if they've already
-been called. To get some idea of the duplicated code, try::
-
- git grep 'if.*>prepared' -- drivers/gpu/drm/panel
- git grep 'if.*>enabled' -- drivers/gpu/drm/panel
-
-In the patch ("drm/panel: Check for already prepared/enabled in drm_panel")
-we've moved this check to the core. Now we can most definitely remove the
-check from the individual panels and save a pile of code.
-
-In adition to removing the check from the individual panels, it is believed
-that even the core shouldn't need this check and that should be considered
-an error if other code ever relies on this check. The check in the core
-currently prints a warning whenever something is relying on this check with
-dev_warn(). After a little while, we likely want to promote this to a
-WARN(1) to help encourage folks not to rely on this behavior.
+Never double prepare/enable/disable/unprepare a panel
+-----------------------------------------------------
+
+As of commit d2aacaf07395 ("drm/panel: Check for already prepared/enabled in
+drm_panel"), we have a check in the drm_panel core to make sure nobody
+double-calls prepare/enable/disable/unprepare. However, that extra double-check
+shouldn't be necessary. The caller should always be matching up calls of
+prepare/unprepare and matching up calls of enable/disable.
+
+Hopefully the warning printed will encourage everyone to fix this. Eventually
+we'll likely want to change this to a WARN_ON and (perhaps) fully remove the
+check. NOTE: even if we remove the double-check, drm_panel core still needs
+to track the enabled/prepared state for its own purposes.
Contact: Douglas Anderson <[email protected]>
--
2.41.0.585.gd2178a4bd4-goog
Hi Doug,
Thanks for working on this :)
On Fri, Aug 04, 2023 at 02:06:07PM -0700, Douglas Anderson wrote:
> The goal of this file is to contain helper functions for panel drivers
> to use. To start off with, let's add drm_panel_helper_shutdown() for
> use by panels that want to make sure they're powered off at
> shutdown/remove time if they happen to be powered on.
>
> The main goal of introducting this function is so that panel drivers
> don't need to track the enabled/prepared state themselves.
>
> Signed-off-by: Douglas Anderson <[email protected]>
It shouldn't be necessary at all: drivers should call
drm_atomic_helper_shutdown at removal time which will disable the
connector (which in turn should unprepare/disable its panel).
If either the driver is missing drm_atomic_helper_shutdown, or if the
connector doesn't properly disable the panel, then I would consider that
a bug.
Maxime
On Fri, Aug 04, 2023 at 02:06:13PM -0700, Douglas Anderson wrote:
> Now that most panels have been updated not to track/double-check their
> prepared/enabled state update the TODO with next steps.
>
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
>
> Documentation/gpu/todo.rst | 33 +++++++++++++--------------------
> 1 file changed, 13 insertions(+), 20 deletions(-)
>
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 139980487ccf..c73d9dbebbf4 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -460,26 +460,19 @@ Contact: Thomas Zimmermann <[email protected]>
>
> Level: Starter
>
> -Clean up checks for already prepared/enabled in panels
> -------------------------------------------------------
> -
> -In a whole pile of panel drivers, we have code to make the
> -prepare/unprepare/enable/disable callbacks behave as no-ops if they've already
> -been called. To get some idea of the duplicated code, try::
> -
> - git grep 'if.*>prepared' -- drivers/gpu/drm/panel
> - git grep 'if.*>enabled' -- drivers/gpu/drm/panel
> -
> -In the patch ("drm/panel: Check for already prepared/enabled in drm_panel")
> -we've moved this check to the core. Now we can most definitely remove the
> -check from the individual panels and save a pile of code.
> -
> -In adition to removing the check from the individual panels, it is believed
> -that even the core shouldn't need this check and that should be considered
> -an error if other code ever relies on this check. The check in the core
> -currently prints a warning whenever something is relying on this check with
> -dev_warn(). After a little while, we likely want to promote this to a
> -WARN(1) to help encourage folks not to rely on this behavior.
> +Never double prepare/enable/disable/unprepare a panel
> +-----------------------------------------------------
> +
> +As of commit d2aacaf07395 ("drm/panel: Check for already prepared/enabled in
> +drm_panel"), we have a check in the drm_panel core to make sure nobody
> +double-calls prepare/enable/disable/unprepare. However, that extra double-check
> +shouldn't be necessary. The caller should always be matching up calls of
> +prepare/unprepare and matching up calls of enable/disable.
> +
> +Hopefully the warning printed will encourage everyone to fix this. Eventually
> +we'll likely want to change this to a WARN_ON and (perhaps) fully remove the
> +check. NOTE: even if we remove the double-check, drm_panel core still needs
> +to track the enabled/prepared state for its own purposes.
Detailing what those purposes are would be nice :)
Maxime
On Fri, Aug 4, 2023 at 11:07 PM Douglas Anderson <[email protected]> wrote:
> As talked about in commit d2aacaf07395 ("drm/panel: Check for already
> prepared/enabled in drm_panel"), we want to remove needless code from
> panel drivers that was storing and double-checking the
> prepared/enabled state. Even if someone was relying on the
> double-check before, that double-check is now in the core and not
> needed in individual drivers.
>
> This series attempts to do just that. While the original grep, AKA:
> git grep 'if.*>prepared' -- drivers/gpu/drm/panel
> git grep 'if.*>enabled' -- drivers/gpu/drm/panel
> ...still produces a few hits after my series, they are _mostly_ all
> gone. The ones that are left are less trivial to fix.
>
> One of the main reasons that many panels probably needed to store and
> double-check their prepared/enabled appears to have been to handle
> shutdown and/or remove. Panels drivers often wanted to force the power
> off for panels in these cases and this was a good reason for the
> double-check. As part of this series a new helper is added that uses
> the state tracking that the drm_panel core is doing so each individual
> panel driver doesn't need to do it.
>
> This series changes a lot of drivers and obviously the author can't
> test on all of them. The changes here are also not completely trivial
> in all cases. Please double-check your drivers carefully to make sure
> something wasn't missed. After looking at over 40 drivers I'll admit
> that my eyes glazed over a little.
>
> I've attempted to organize these patches like to group together panels
> that needed similar handling. Panels that had code that didn't seem to
> match anyone else got their own patch. I made judgement calls on what
> I considered "similar".
>
> As noted in individual patches, there are some cases here where I
> expect behavior to change a little bit. I'm hoping these changes are
> for the better and don't cause any problems. Fingers crossed.
>
> I have at least confirmed that "allmodconfig" for arm64 doesn't fall
> on its face with this series. I haven't done a ton of other testing.
The series:
Reviewed-by: Linus Walleij <[email protected]>
Please send out a non-RFC version, this is clearly the right thing to
do.
Yours,
Linus Walleij
On Fri, Aug 25, 2023 at 02:58:02PM -0700, Doug Anderson wrote:
> Maxime,
>
> On Sun, Aug 6, 2023 at 11:41 PM Maxime Ripard <[email protected]> wrote:
> >
> > Hi Doug,
> >
> > Thanks for working on this :)
> >
> > On Fri, Aug 04, 2023 at 02:06:07PM -0700, Douglas Anderson wrote:
> > > The goal of this file is to contain helper functions for panel drivers
> > > to use. To start off with, let's add drm_panel_helper_shutdown() for
> > > use by panels that want to make sure they're powered off at
> > > shutdown/remove time if they happen to be powered on.
> > >
> > > The main goal of introducting this function is so that panel drivers
> > > don't need to track the enabled/prepared state themselves.
> > >
> > > Signed-off-by: Douglas Anderson <[email protected]>
> >
> > It shouldn't be necessary at all: drivers should call
> > drm_atomic_helper_shutdown at removal time which will disable the
> > connector (which in turn should unprepare/disable its panel).
> >
> > If either the driver is missing drm_atomic_helper_shutdown, or if the
> > connector doesn't properly disable the panel, then I would consider that
> > a bug.
>
> Hmmm. I'm a bit hesitant here. I guess I'm less worried about the
> removal time and more worried about the shutdown time.
>
> For removal I'd be fine with just dropping the call and saying it's
> the responsibility of the driver to call drm_atomic_helper_shutdown(),
> as you suggest. I'd tend to believe that removal of DRM drivers is not
> used anywhere in "production" code (or at least not common) and I
> think it's super hard to get it right, to unregister and unbind all
> the DRM components in the right order.
It depends on the kind of devices. USB devices are very likely to be
removed, platform devices very unlikely, and PCIe cards somewhere in the
middle :)
I'm not sure the likelyhood of the device getting removed has much to do
with it though, and likely or not, it's definitely something we should
address and fix if an issue is to be found.
> Presumably anyone trying to remove a DRM panel in a generic case
> supporting lots of different hardware is used to it being a bit
> broken...
It's not. Most drivers might be broken, but it's totally something we
support and should strive for.
> Not that it's a super great situation to be in for remove() not to
> work reliably, but that's how I think it is right now.
>
> For shutdown, however, I'm not really OK with just blindly removing
> the code that tries to power off the panel.
I disagree with that statement. It's not "blindly removing the code",
that code is still there, in the disable hook.
> Shutdown is called any time you reboot a device. That means that if a
> DRM driver is _not_ calling drm_atomic_helper_shutdown() on the
> panel's behalf at shutdown time then the panel won't be powered off
> properly. This feels to me like something that might actually matter.
It does matter. What I disagree on is that you suggest working around
that brokenness in the core framework. What I'm saying is driver is
broken, we should keep the core framework sane and fix it in the driver.
It should be fairly easy with a coccinelle script to figure out which
panels are affected, and to add that call in remove.
> Panels tend to be one of those things that really care about their
> power sequencing and can even get damaged (or not turn on properly
> next time) if sequencing is not done properly, so just removing this
> code and putting the blame on the DRM driver seems scary to me.
Sure, it's bad. But there's no difference compared to the approach you
suggest in that patch: you created a helper, yes, but every driver will
still have to call that helper and if they don't, the panel will still
be called and it's a bug. And we would have to convert everything to
that new helper.
It's fundamentally the same discussion than what you were opposed to
above.
> Sure enough, a quick survey of DRM drivers shows that many don't call
> drm_atomic_helper_shutdown() at .shutdown time. A _very_ quick skim of
> callers to drm_atomic_helper_shutdown():
>
> * omapdrm/omap_drv.c - calls at remove, not shutdown
> * arm/hdlcd_drv.c - calls at unbind, not shutdown
> * arm/malidp_drv.c - calls at unbind, not shutdown
> * armada/armada_drv.c - calls at unbind, not shutdown
>
> ...huh, actually, there are probably too many to list that don't call
> it at shutdown. There are some that do, but also quite a few that
> don't. I'm not sure I really want to blindly add
> drm_atomic_helper_shutdown() to all those DRM driver's shutdown
> callbacks... That feels like asking for someone to flame me...
No one will flame you, and if they do, we'll take care of it. And yes,
those are bugs, so let's fix them instead of working around them?
> ...but then, what's the way forward? I think normally the panel's
> shutdown() callback would happen _before_ the DRM driver's shutdown()
> callback,
Is there such a guarantee?
> so we can't easily write logic in the panel's shutdown like "if the
> DRM panel didn't shut the panel down then print a warning and shut
> down the panel". We'd have to somehow invent and register for a "late
> shutdown" callback and have the panel use that to shut itself down if
> the DRM driver didn't. That seems like a bad idea...
>
> Do you have any brilliant ideas here? I could keep the function as-is
> but only have panels only call it at shutdown time if you want. I
> could add to the comments and/or the commit message some summary of
> the above and that the call is important for panels that absolutely
> need to be powered off at shutdown time even if the DRM driver doesn't
> do anything special at shutdown... Any other ideas?
Brilliant ideas to do what exactly?
I disagree that the solution to our problem is to disable the panel
resources at shutdown time and we should only do it at disable time.
Your helper does exactly that though, so I don't think the helper is a
good idea.
Maxime
On Mon, Aug 28, 2023 at 09:06:29AM -0700, Doug Anderson wrote:
> > > Shutdown is called any time you reboot a device. That means that if a
> > > DRM driver is _not_ calling drm_atomic_helper_shutdown() on the
> > > panel's behalf at shutdown time then the panel won't be powered off
> > > properly. This feels to me like something that might actually matter.
> >
> > It does matter. What I disagree on is that you suggest working around
> > that brokenness in the core framework. What I'm saying is driver is
> > broken, we should keep the core framework sane and fix it in the driver.
> >
> > It should be fairly easy with a coccinelle script to figure out which
> > panels are affected, and to add that call in remove.
>
> I think I'm confused here. I've already figured out which panels are
> affected in my patch series, right? It's the set of panels that today
> try to power the panel off in their shutdown call, right? ...but I
> think we can't add the call you're suggesting,
> drm_atomic_helper_shutdown(), to the _panel_'s shutdown callback, can
> we? We need to add it to the shutdown callback of the top-level DRM
> driver, right?
I have no idea what happens if we just unbind the panel device from its
driver.
If we can't, then it's all fine. If we can, then we need figure out how
to unregister the DRM device (or block the unbinding from happening).
> > > Panels tend to be one of those things that really care about their
> > > power sequencing and can even get damaged (or not turn on properly
> > > next time) if sequencing is not done properly, so just removing this
> > > code and putting the blame on the DRM driver seems scary to me.
> >
> > Sure, it's bad. But there's no difference compared to the approach you
> > suggest in that patch: you created a helper, yes, but every driver will
> > still have to call that helper and if they don't, the panel will still
> > be called and it's a bug. And we would have to convert everything to
> > that new helper.
> >
> > It's fundamentally the same discussion than what you were opposed to
> > above.
>
> I think the key difference here is that, if I understand correctly,
> drm_atomic_helper_shutdown() needs to be added to the top-level DRM
> driver, not to the panel itself. I guess I'm worried that I'll either
> miss a case or that simply adding a call to
> drm_atomic_helper_shutdown() in the top-level DRM driver will break
> something. Well, I suppose I can try it and see what happens...
The more I think about this discussion, the more I think that the
original intent of the prepared/enabled flags were precisely there to
prevent a double-disable for drivers with drm_atomic_helper_shutdown(),
while still shutting down the panel resources when the panel is used
with a driver that doesn't call it.
Honestly, I think the right thing to do here is to make every driver
call shutdown, and then you don't need the reference counting anymore.
I had a shot at a (possibly very suboptimal) coccinelle script to look
for drivers that are KMS drivers but don't call
drm_atomic_helper_shutdown() at shutdown.
https://paste.ack.tf/bb42e6@raw
The result is:
$ make coccicheck COCCI=./test-drm-shutdown.cocci MODE=report
...
./drivers/gpu/drm/sti/sti_drv.c:262:30-49: ERROR: KMS driver sti_platform_driver is missing shutdown implementation
./drivers/gpu/drm/armada/armada_drv.c:245:30-56: ERROR: KMS driver armada_drm_platform_driver is missing shutdown implementation
./drivers/gpu/drm/vmwgfx/vmwgfx_drv.c:1637:25-39: ERROR: KMS driver vmw_pci_driver is missing shutdown implementation
./drivers/gpu/drm/vboxvideo/vbox_drv.c:163:25-40: ERROR: KMS driver vbox_pci_driver is missing shutdown implementation
./drivers/gpu/drm/tiny/arcpgu.c:424:30-52: ERROR: KMS driver arcpgu_platform_driver is missing shutdown implementation
./drivers/gpu/drm/omapdrm/omap_drv.c:856:30-34: ERROR: KMS driver pdev is missing shutdown implementation
./drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c:361:30-57: ERROR: KMS driver fsl_dcu_drm_platform_driver is missing shutdown implementation
./drivers/gpu/drm/tve200/tve200_drv.c:261:30-43: ERROR: KMS driver tve200_driver is missing shutdown implementation
./drivers/gpu/drm/stm/drv.c:235:30-53: ERROR: KMS driver stm_drm_platform_driver is missing shutdown implementation
./drivers/gpu/drm/qxl/qxl_drv.c:267:25-39: ERROR: KMS driver qxl_pci_driver is missing shutdown implementation
./drivers/gpu/drm/logicvc/logicvc_drm.c:494:30-57: ERROR: KMS driver logicvc_drm_platform_driver is missing shutdown implementation
./drivers/gpu/drm/sun4i/sun4i_drv.c:439:30-55: ERROR: KMS driver sun4i_drv_platform_driver is missing shutdown implementation
./drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c:827:30-60: ERROR: KMS driver atmel_hlcdc_dc_platform_driver is missing shutdown implementation
./drivers/gpu/drm/mcde/mcde_drv.c:471:30-41: ERROR: KMS driver mcde_driver is missing shutdown implementation
./drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c:366:25-41: ERROR: KMS driver hibmc_pci_driver is missing shutdown implementation
./drivers/gpu/drm/aspeed/aspeed_gfx_drv.c:365:30-56: ERROR: KMS driver aspeed_gfx_platform_driver is missing shutdown implementation
./drivers/gpu/drm/ingenic/ingenic-drm-drv.c:1607:30-48: ERROR: KMS driver ingenic_drm_driver is missing shutdown implementation
./drivers/gpu/drm/arm/malidp_drv.c:982:30-52: ERROR: KMS driver malidp_platform_driver is missing shutdown implementation
./drivers/gpu/drm/arm/hdlcd_drv.c:400:30-51: ERROR: KMS driver hdlcd_platform_driver is missing shutdown implementation
./drivers/gpu/drm/kmb/kmb_drv.c:622:30-49: ERROR: KMS driver kmb_platform_driver is missing shutdown implementation
./drivers/gpu/drm/exynos/exynos_drm_drv.c:356:30-56: ERROR: KMS driver exynos_drm_platform_driver is missing shutdown implementation
./drivers/gpu/drm/tiny/bochs.c:718:25-41: ERROR: KMS driver bochs_pci_driver is missing shutdown implementation
./drivers/gpu/drm/tiny/cirrus.c:746:25-42: ERROR: KMS driver cirrus_pci_driver is missing shutdown implementation
./drivers/gpu/drm/mediatek/mtk_drm_drv.c:954:30-53: ERROR: KMS driver mtk_drm_platform_driver is missing shutdown implementation
It's a significant number of drivers, but it's not the end of the world,
really.
Then, once the expectation is that all drivers are calling shutdown, we
don't have to worry about the refcounting at all in the panels, or
resources not being free'd anymore. And we have a single path to test
(disable) instead of two including one that might be difficult to test
properly.
> I'll try to cook up a v2 and we'll see what people say. I might keep
> the RFC tag for v2 just because I expect it to still be touching a lot
> of stuff.
Awesome, thanks
Maxime
On Thu, Aug 31, 2023 at 11:18:49AM -0700, Doug Anderson wrote:
> Hi,
>
> On Thu, Aug 31, 2023 at 12:38 AM Maxime Ripard <[email protected]> wrote:
> >
> > If so, then I think we can implement everything by doing something like:
> >
> > - Implement enable and disable refcounting in panels.
> > drm_panel_prepare and drm_panel_enable would increase it,
> > drm_panel_unprepare and drm_panel_disable would decrease it.
> >
> > - Only actually call the disable and unprepare functions when that
> > refcount goes to 0 in the normal case.
>
> Just to be clear: by refcounting do you mean switching this to actual
> refcount?
Yes
> Today this is explicitly _not_ refcounting, right? It is simply
> treating double-enables as no-ops and double-disables as no-ops. With
> our current understanding, the only thing we actually need to guard
> against is double-disable but at the moment we do guard against both.
> Specifically we believe the cases that are issues:
>
> a) At shutdown/remove time we want to disable the panel, but only if
> it was enabled (we wouldn't want to call disable if the panel was
> already off because userspace turned it off).
Yeah, and that's doable with refcounting too.
> b) At shutdown time we want to disable the panel but then we don't
> want to double-disable if the main DRM driver also causes us to get
> disabled.
That's what I want to prevent though. Eventually, I don't want to see
panels call drm_panel_unprepare/disable themselves. There's no need to
if all drivers implement the shutdown sequence properly.
> I'd rather keep it the way it is (prevent double-disable) and not
> switch it to a refcount.
>
> I'll also note that drm_panel currently already keeps track of the
> enabled/prepared state, so there's no "implement" step here, right?
> That's what landed in commit d2aacaf07395 ("drm/panel: Check for
> already prepared/enabled in drm_panel"). Just to remind ourselves of
> the history:
>
> 1. I needed to keep track of the "prepared" state anyway to make the
> "panel follower" API work properly. See drm_panel_add_follower() where
> we immediately power on a follower if the panel they're following was
> already prepared.
>
> 2. Since I was keeping track of the "prepared" state in the core
> anyway, it seemed like a good idea to prevent
> double-prepare/unprepare/enable/disable in the drm_panel core so that
> we could remove it from individual panels since that was always a
> point of contention in individual panels. It was asserted that, even
> in the core, we shouldn't need code to prevent
> double-prepare/unprepare/enable/disable but that as a first step
> moving this to the core and out of drivers made sense. Anyone relying
> on the core would get a warning printed out indicating that they were
> doing something wrong and this would eventually move to a WARN_ON.
>
> 3. While trying to remove this from the drivers we ended up realizing
> some of the issues with panels wanting to power them off at shutdown /
> remove time.
Yes, I'm aware. It's not clear to me what you're confused about though.
Is there anything I said that would conflict with that?
> > - In drm_panel_remove, if we still have a refcount > 0, then we WARN
> > and force unprepare/disable the panel.
>
> I think the net-net of this is that you're saying I should fold the
> code from this patch straight into drm_panel_remove() and add a WARN
> if it ever triggers, right?
Aside from the refcounting-or-not discussion, yes.
> In this patch series I'd remove the calls to
> drm_panel_helper_shutdown() and rely on drm_panel_remove() to do the
> power off at remove time.
Yep
> This might give a warning but, as discussed, removing a panel like
> this isn't likely to work well and at least we'd power sequence it
> properly. In some cases, I might have to move the call to
> drm_panel_remove() around a little bit but I think it'll work.
Yep
> The above solves the problem with panels wanting to power sequence
> themselves at remove() time, but not at shutdown() time. Thus we'd
> still have a dependency on having all drivers use
> drm_atomic_helper_shutdown() so that work becomes a dependency.
Does it? I think it can be done in parallel?
Maxime
On Wed, Aug 30, 2023 at 04:10:20PM -0700, Doug Anderson wrote:
> Hi,
>
> On Tue, Aug 29, 2023 at 1:38 AM Maxime Ripard <[email protected]> wrote:
> >
> > On Mon, Aug 28, 2023 at 09:06:29AM -0700, Doug Anderson wrote:
> > > > > Shutdown is called any time you reboot a device. That means that if a
> > > > > DRM driver is _not_ calling drm_atomic_helper_shutdown() on the
> > > > > panel's behalf at shutdown time then the panel won't be powered off
> > > > > properly. This feels to me like something that might actually matter.
> > > >
> > > > It does matter. What I disagree on is that you suggest working around
> > > > that brokenness in the core framework. What I'm saying is driver is
> > > > broken, we should keep the core framework sane and fix it in the driver.
> > > >
> > > > It should be fairly easy with a coccinelle script to figure out which
> > > > panels are affected, and to add that call in remove.
> > >
> > > I think I'm confused here. I've already figured out which panels are
> > > affected in my patch series, right? It's the set of panels that today
> > > try to power the panel off in their shutdown call, right? ...but I
> > > think we can't add the call you're suggesting,
> > > drm_atomic_helper_shutdown(), to the _panel_'s shutdown callback, can
> > > we? We need to add it to the shutdown callback of the top-level DRM
> > > driver, right?
> >
> > I have no idea what happens if we just unbind the panel device from its
> > driver.
> >
> > If we can't, then it's all fine. If we can, then we need figure out how
> > to unregister the DRM device (or block the unbinding from happening).
>
> Nothing prevents unbinding the panel driver directly. I just confirmed
> on my sc7180-lazor:
>
> cd /sys/bus/dp-aux/drivers/panel-simple-dp-aux
> echo aux-2-0008 > unbind
>
> ...no errors happened and the panel unbound. I think this is as
> expected since I'm not aware of a way to prevent unbinding a driver. I
> think the relevant function is unbind_store() in bus.c, right? There
> is no failure code returned by device_driver_detach(). Presumably this
> is by design?
Well, that sucks :(
If I'm reading the docs right, then we could add a device link from the
panel (provider) to the KMS device (consumer), and unbinding the panel
would unbind the KMS driver.
https://www.kernel.org/doc/html/latest/driver-api/device_link.html#state-machine
"""
Before a supplier's driver is removed, links to consumers that are
not bound to a driver are updated to DL_STATE_SUPPLIER_UNBIND. (Call to
device_links_busy() from __device_release_driver().) This prevents the
consumers from binding. (Call to device_links_check_suppliers() from
really_probe().) Consumers that are bound are freed from their driver;
consumers that are probing are waited for until they are done. (Call to
device_links_unbind_consumers() from __device_release_driver().) Once
all links to consumers are in DL_STATE_SUPPLIER_UNBIND state, the
supplier driver is released and the links revert to DL_STATE_DORMANT.
(Call to device_links_driver_cleanup() from __device_release_driver().)
"""
It would be painful to rebind the whole thing, but at least we would be
safe.
> FWIW, also trying to re-bind didn't work, also as expected (I think).
> I think the whole bridge chain would need to be resolved again and
> nothing I'm aware of makes that happen. Maybe I'm simply missing
> something in my understanding, of course.
Yeah, I would expect rebinding the entire thing would solve this too. We
never really supported partial devices (which is also why the panel or
bridges going away sucks), so switching to something more dynamic is
probably going to be a lot of work for not much gains.
> If you want to attempt to tackle some of these issues then I'd be more
> than happy. I'm already waist deep in yak hair and I think this is too
> big of a task for me to add on.
I don't think we need to fix this in this series, or even now. Maybe we
should add a TODO note so that we don't forget about it, but I'd rather
get the original problem fixed :)
> Should this issue block my work, then?
No, definitely not. It's orthogonal to me. The only thing we could learn
from that experiment for this series is that calling
drm_atomic_helper_shutdown() is probably just wishful thinking at this
point.
> Today, panels will at least cleanly power themselves off if someone
> tries to unbind them like that. If I remove the clean power off at
> driver unbind time and rely on the DRM driver to power panels off
> cleanly then if someone directly unbinds a panel like I've done above
> then it won't be power sequenced properly, right?
Yeah... The kernel also probably just blew up because it followed a
dangling pointer to what used to be our panel but got freed when it was
unbound.
So, if we go back to the original intent of this series and sums things
up.
Our goals are:
1) We want to have the panel disabled at KMS shutdown and remove
(Doug, Maxime);
2) but we want them to be disabled once to avoid any double-free or
related issue (Doug, Maxime);
3) doing so should work out of the box so that we close this once and
for all, so no boilerplate or function to call in the panel driver
(Maxime);
For 3) to happen, it means that all drivers should call
drm_atomic_helper_shutdown() both in remove/unbind and at shutdown. It's
something that drivers should do anymay, but a very significant number
of them don't.
However, panels and bridges, when they are unbound/removed, don't notify
the KMS driver that they are connected to and thus we end up with
dangling pointers and the panel still powered on if 3 is implemented as
is.
We shouldn't try to fix panel unbinding at the moment, so we have to
take it into account.
Is that a good summary?
If so, then I think we can implement everything by doing something like:
- Implement enable and disable refcounting in panels.
drm_panel_prepare and drm_panel_enable would increase it,
drm_panel_unprepare and drm_panel_disable would decrease it.
- Only actually call the disable and unprepare functions when that
refcount goes to 0 in the normal case.
- In drm_panel_remove, if we still have a refcount > 0, then we WARN
and force unprepare/disable the panel.
And in parallel, we convert all drivers to use
drm_atomic_helper_shutdown() so that, in the most common case, we don't
rely on the drm_panel_remove safety net. And we'll want a bunch of TODO
items to eventually remove that safety net and the refcounting entirely
once we fix the unbinding issue.
Does that make sense?
> > > > > Panels tend to be one of those things that really care about their
> > > > > power sequencing and can even get damaged (or not turn on properly
> > > > > next time) if sequencing is not done properly, so just removing this
> > > > > code and putting the blame on the DRM driver seems scary to me.
> > > >
> > > > Sure, it's bad. But there's no difference compared to the approach you
> > > > suggest in that patch: you created a helper, yes, but every driver will
> > > > still have to call that helper and if they don't, the panel will still
> > > > be called and it's a bug. And we would have to convert everything to
> > > > that new helper.
> > > >
> > > > It's fundamentally the same discussion than what you were opposed to
> > > > above.
> > >
> > > I think the key difference here is that, if I understand correctly,
> > > drm_atomic_helper_shutdown() needs to be added to the top-level DRM
> > > driver, not to the panel itself. I guess I'm worried that I'll either
> > > miss a case or that simply adding a call to
> > > drm_atomic_helper_shutdown() in the top-level DRM driver will break
> > > something. Well, I suppose I can try it and see what happens...
> >
> > The more I think about this discussion, the more I think that the
> > original intent of the prepared/enabled flags were precisely there to
> > prevent a double-disable for drivers with drm_atomic_helper_shutdown(),
> > while still shutting down the panel resources when the panel is used
> > with a driver that doesn't call it.
>
> Sure, that makes sense.
And we would also catch that and complain loudly so that if we ever miss
drivers in the conversion, it's clear that it needs to be fixed.
> > Honestly, I think the right thing to do here is to make every driver
> > call shutdown, and then you don't need the reference counting anymore.
> >
> > I had a shot at a (possibly very suboptimal) coccinelle script to look
> > for drivers that are KMS drivers but don't call
> > drm_atomic_helper_shutdown() at shutdown.
> >
> > https://paste.ack.tf/bb42e6@raw
>
> Your paste seems to have expired. Maybe you used the default and had
> it expire in 1 day? Maybe you could just paste the script in email so
> it'll be archived for posterity on lore?
Argh, sorry. The coccinelle script was massive so I didn't want to
inline it, but here you go:
----8<----
virtual report
@ has_driver @
identifier driver;
position p;
@@
(
struct pci_driver driver@p = {
...
};
|
struct platform_driver driver@p = {
...
};
|
struct spi_driver driver@p = {
...
};
)
@ has_probe @
identifier has_driver.driver;
identifier probe_f;
@@
(
struct pci_driver driver = {
.probe = probe_f,
};
|
struct platform_driver driver = {
.probe = probe_f,
};
|
struct spi_driver driver = {
.probe = probe_f,
};
)
@ has_shutdown @
identifier has_driver.driver;
identifier shutdown_f;
@@
(
struct pci_driver driver = {
.shutdown = shutdown_f,
};
|
struct platform_driver driver = {
.shutdown = shutdown_f,
};
|
struct spi_driver driver = {
.shutdown = shutdown_f,
};
)
@ has_kms_atomic @
identifier kms_driver;
@@
struct drm_driver kms_driver = {
.driver_features = DRIVER_ATOMIC | ...,
};
@ is_kms_probe depends on has_probe && has_kms_atomic @
identifier has_probe.probe_f;
identifier has_kms_atomic.kms_driver;
@@
probe_f(...)
{
<+...
(
drm_dev_alloc(&kms_driver, ...)
|
devm_drm_dev_alloc(..., &kms_driver, ...)
)
...+>
}
@ uses_component @
identifier ops;
identifier bind_f;
@@
struct component_master_ops ops = {
.bind = bind_f,
};
@ registers_bind depends on has_probe && uses_component @
identifier uses_component.ops;
identifier has_probe.probe_f;
@@
probe_f(...)
{
<+...
(
component_master_add_with_match(..., &ops, ...)
|
drm_of_component_probe(..., &ops)
)
...+>
}
@ is_kms_component depends on uses_component && registers_bind && has_kms_atomic @
identifier uses_component.bind_f;
identifier has_kms_atomic.kms_driver;
@@
bind_f(...)
{
<+...
(
drm_dev_alloc(&kms_driver, ...)
|
devm_drm_dev_alloc(..., &kms_driver, ...)
)
...+>
}
@ has_kms_init_f depends on has_kms_atomic && !(is_kms_probe || is_kms_component) @
identifier has_kms_atomic.kms_driver;
identifier init_f;
@@
init_f(...)
{
<+...
(
drm_dev_alloc(&kms_driver, ...)
|
devm_drm_dev_alloc(..., &kms_driver, ...)
)
...+>
}
@ is_kms_probe_subf depends on has_probe && has_kms_init_f @
identifier has_kms_init_f.init_f;
identifier has_probe.probe_f;
@@
probe_f(...)
{
<+...
init_f(...)
...+>
}
@ is_kms_bind_subf depends on uses_component && has_kms_init_f @
identifier has_kms_init_f.init_f;
identifier uses_component.bind_f;
@@
bind_f(...)
{
<+...
init_f(...)
...+>
}
@ shuts_down depends on has_shutdown @
identifier has_shutdown.shutdown_f;
@@
shutdown_f(...)
{
<+...
drm_atomic_helper_shutdown(...)
...+>
}
@ script:python depends on report && (is_kms_probe || is_kms_probe_subf || is_kms_component || is_kms_bind_subf) && !has_shutdown @
ops << has_driver.driver;
p << has_driver.p;
@@
coccilib.report.print_report(p[0], "ERROR: KMS driver %s is missing shutdown implementation" % (ops))
@ script:python depends on report && (is_kms_probe || is_kms_probe_subf || is_kms_component || is_kms_bind_subf) && (has_shutdown && !shuts_down) @
ops << has_driver.driver;
p << has_driver.p;
@@
coccilib.report.print_report(p[0], "ERROR: KMS driver %s shutdown implementation is missing a call to drm_atomic_helper_shutdown()" % (ops))
----8<----
>
> > The result is:
> >
> > $ make coccicheck COCCI=./test-drm-shutdown.cocci MODE=report
> >
> > ...
> >
> > ./drivers/gpu/drm/sti/sti_drv.c:262:30-49: ERROR: KMS driver sti_platform_driver is missing shutdown implementation
> > ./drivers/gpu/drm/armada/armada_drv.c:245:30-56: ERROR: KMS driver armada_drm_platform_driver is missing shutdown implementation
> > ./drivers/gpu/drm/vmwgfx/vmwgfx_drv.c:1637:25-39: ERROR: KMS driver vmw_pci_driver is missing shutdown implementation
> > ./drivers/gpu/drm/vboxvideo/vbox_drv.c:163:25-40: ERROR: KMS driver vbox_pci_driver is missing shutdown implementation
> > ./drivers/gpu/drm/tiny/arcpgu.c:424:30-52: ERROR: KMS driver arcpgu_platform_driver is missing shutdown implementation
> > ./drivers/gpu/drm/omapdrm/omap_drv.c:856:30-34: ERROR: KMS driver pdev is missing shutdown implementation
> > ./drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c:361:30-57: ERROR: KMS driver fsl_dcu_drm_platform_driver is missing shutdown implementation
> > ./drivers/gpu/drm/tve200/tve200_drv.c:261:30-43: ERROR: KMS driver tve200_driver is missing shutdown implementation
> > ./drivers/gpu/drm/stm/drv.c:235:30-53: ERROR: KMS driver stm_drm_platform_driver is missing shutdown implementation
> > ./drivers/gpu/drm/qxl/qxl_drv.c:267:25-39: ERROR: KMS driver qxl_pci_driver is missing shutdown implementation
> > ./drivers/gpu/drm/logicvc/logicvc_drm.c:494:30-57: ERROR: KMS driver logicvc_drm_platform_driver is missing shutdown implementation
> > ./drivers/gpu/drm/sun4i/sun4i_drv.c:439:30-55: ERROR: KMS driver sun4i_drv_platform_driver is missing shutdown implementation
> > ./drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c:827:30-60: ERROR: KMS driver atmel_hlcdc_dc_platform_driver is missing shutdown implementation
> > ./drivers/gpu/drm/mcde/mcde_drv.c:471:30-41: ERROR: KMS driver mcde_driver is missing shutdown implementation
> > ./drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c:366:25-41: ERROR: KMS driver hibmc_pci_driver is missing shutdown implementation
> > ./drivers/gpu/drm/aspeed/aspeed_gfx_drv.c:365:30-56: ERROR: KMS driver aspeed_gfx_platform_driver is missing shutdown implementation
> > ./drivers/gpu/drm/ingenic/ingenic-drm-drv.c:1607:30-48: ERROR: KMS driver ingenic_drm_driver is missing shutdown implementation
> > ./drivers/gpu/drm/arm/malidp_drv.c:982:30-52: ERROR: KMS driver malidp_platform_driver is missing shutdown implementation
> > ./drivers/gpu/drm/arm/hdlcd_drv.c:400:30-51: ERROR: KMS driver hdlcd_platform_driver is missing shutdown implementation
> > ./drivers/gpu/drm/kmb/kmb_drv.c:622:30-49: ERROR: KMS driver kmb_platform_driver is missing shutdown implementation
> > ./drivers/gpu/drm/exynos/exynos_drm_drv.c:356:30-56: ERROR: KMS driver exynos_drm_platform_driver is missing shutdown implementation
> > ./drivers/gpu/drm/tiny/bochs.c:718:25-41: ERROR: KMS driver bochs_pci_driver is missing shutdown implementation
> > ./drivers/gpu/drm/tiny/cirrus.c:746:25-42: ERROR: KMS driver cirrus_pci_driver is missing shutdown implementation
> > ./drivers/gpu/drm/mediatek/mtk_drm_drv.c:954:30-53: ERROR: KMS driver mtk_drm_platform_driver is missing shutdown implementation
> >
> > It's a significant number of drivers, but it's not the end of the world,
> > really.
>
> My own analysis shows more than that, actually. I can't look at your
> script since it's expired, but as one example your list seems to have
> neither imx/ipuv3 nor imx/dcss. imx/ipuv3 seems to be missing
> drm_atomic_helper_shutdown() at both unbind and system shutdown time.
Yeah, this was due to the script ignoring drm_of_component_probe that
ipuv3 uses. It's fixed now.
> imx/dcss seems to be missing drm_atomic_helper_shutdown() at system
> shutdown time, though it does have it at driver remove time.
And I'm wondering if this one is due to the fact that probe only calls a
function that isn't in the same file, which confuses my script. I'm not
entirely sure how to address that though.
> Did I mess up in identifying those two drivers as ones that need
> fixing?
No, both look like legit issues indeed.
> I can't give you an exact list because I don't have a great search
> that identifies the problem. I'm mostly looking for all instances of
> drm_dev_register() where the driver is "DRIVER_MODESET" and then
> manually checking their use of drm_atomic_helper_shutdown(). Until I
> get through them all I won't know the count, and it's a manual
> process.
I think my coccinnelle script will match with most of them, but we might
still miss a few indeed.
Maxime
Hi,
On Fri, Sep 1, 2023 at 1:15 AM Maxime Ripard <[email protected]> wrote:
>
> On Thu, Aug 31, 2023 at 11:18:49AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Aug 31, 2023 at 12:38 AM Maxime Ripard <[email protected]> wrote:
> > >
> > > If so, then I think we can implement everything by doing something like:
> > >
> > > - Implement enable and disable refcounting in panels.
> > > drm_panel_prepare and drm_panel_enable would increase it,
> > > drm_panel_unprepare and drm_panel_disable would decrease it.
> > >
> > > - Only actually call the disable and unprepare functions when that
> > > refcount goes to 0 in the normal case.
> >
> > Just to be clear: by refcounting do you mean switching this to actual
> > refcount?
>
> Yes
>
> > Today this is explicitly _not_ refcounting, right? It is simply
> > treating double-enables as no-ops and double-disables as no-ops. With
> > our current understanding, the only thing we actually need to guard
> > against is double-disable but at the moment we do guard against both.
> > Specifically we believe the cases that are issues:
> >
> > a) At shutdown/remove time we want to disable the panel, but only if
> > it was enabled (we wouldn't want to call disable if the panel was
> > already off because userspace turned it off).
>
> Yeah, and that's doable with refcounting too.
I don't understand the benefit of switching to refcounting, though. We
don't ever expect the "prepare" or "enable" function to be called more
than once and all we're guarding against is a double-unprepare and a
double-enable. Switching this to refcounting would make the reader
think that there was a legitimate case for things to be prepared or
enabled twice. As far as I know, there isn't.
In any case, I don't think there's any need to switch this to
refcounting as part of this effort. Someone could, in theory, do it as
a separate patch series.
> > The above solves the problem with panels wanting to power sequence
> > themselves at remove() time, but not at shutdown() time. Thus we'd
> > still have a dependency on having all drivers use
> > drm_atomic_helper_shutdown() so that work becomes a dependency.
>
> Does it? I think it can be done in parallel?
I don't think it can be in parallel. While it makes sense for panels
to call drm_panel_remove() at remove time, it doesn't make sense for
them to call it at shutdown time. That means that the trick of having
the panel get powered off in drm_panel_remove() won't help for
shutdown. For shutdown, which IMO is the more important case, we need
to wait until all drm drivers call drm_atomic_helper_shutdown()
properly.
-Doug
Hi,
On Fri, Sep 01, 2023 at 06:42:42AM -0700, Doug Anderson wrote:
> On Fri, Sep 1, 2023 at 1:15 AM Maxime Ripard <[email protected]> wrote:
> > On Thu, Aug 31, 2023 at 11:18:49AM -0700, Doug Anderson wrote:
> > > Today this is explicitly _not_ refcounting, right? It is simply
> > > treating double-enables as no-ops and double-disables as no-ops. With
> > > our current understanding, the only thing we actually need to guard
> > > against is double-disable but at the moment we do guard against both.
> > > Specifically we believe the cases that are issues:
> > >
> > > a) At shutdown/remove time we want to disable the panel, but only if
> > > it was enabled (we wouldn't want to call disable if the panel was
> > > already off because userspace turned it off).
> >
> > Yeah, and that's doable with refcounting too.
>
> I don't understand the benefit of switching to refcounting, though. We
> don't ever expect the "prepare" or "enable" function to be called more
> than once and all we're guarding against is a double-unprepare and a
> double-enable. Switching this to refcounting would make the reader
> think that there was a legitimate case for things to be prepared or
> enabled twice. As far as I know, there isn't.
Sure, eventually we'll want to remove it.
I even said it as such here:
https://lore.kernel.org/dri-devel/wwzbd7dt5qyimshnd7sbgkf5gxk7tq5dxtrerz76uw5p6s7tzt@cbiezkfeuqqn/
However, we have a number of panels following various anti-patterns
where disable and unprepare would be called multiple times. A boolean
would just ignore the second, refcounting would warn over it, and that's
what we want.
And that's exactly because there isn't a legitimate case for things to
be disabled or unprepared twice, but yet many panel driver do it anyway.
> In any case, I don't think there's any need to switch this to
> refcounting as part of this effort. Someone could, in theory, do it as
> a separate patch series.
I'm sorry, but I'll insist on getting a solution that will warn panels
that call drm_atomic_helper_shutdown or drm_panel_disable/unprepare by
hand. It doesn't have to be refcounting though if you have a better idea
in mind.
> > > The above solves the problem with panels wanting to power sequence
> > > themselves at remove() time, but not at shutdown() time. Thus we'd
> > > still have a dependency on having all drivers use
> > > drm_atomic_helper_shutdown() so that work becomes a dependency.
> >
> > Does it? I think it can be done in parallel?
>
> I don't think it can be in parallel. While it makes sense for panels
> to call drm_panel_remove() at remove time, it doesn't make sense for
> them to call it at shutdown time. That means that the trick of having
> the panel get powered off in drm_panel_remove() won't help for
> shutdown. For shutdown, which IMO is the more important case, we need
> to wait until all drm drivers call drm_atomic_helper_shutdown()
> properly.
Right, my point was more that drivers that already don't disable the
panel in their shutdown implementation will still not do it. And drivers
that do will still do it, so there's no regression.
We obviously want to tend to having all drivers call
drm_atomic_helper_shutdown(), but not having it will not introduce any
regression.
Maxime
On Tue, Sep 05, 2023 at 12:12:58PM -0700, Doug Anderson wrote:
> Hi,
>
> On Tue, Sep 5, 2023 at 9:45 AM Doug Anderson <[email protected]> wrote:
> >
> > As per our discussion, in V2 we will make drm_panel_remove() actually
> > unprepare / disable a panel if it was left enabled. This would
> > essentially fold in the drm_panel_helper_shutdown() from my RFC patch.
> > This would make tdo_tl070wsh30_panel_remove() behave the same as it
> > did before. Ugh, though I may have to think about this more when I get
> > to implementation since I don't think there's a guarantee of the
> > ordering of shutdown calls between the DRM driver and the panel.
> > Anyway, something to discuss later.
>
> Ugh, ignore the above paragraph. I managed to confuse myself and was
> thinking about shutdown but talking about remove. Sigh. :( Instead,
> pretend the above paragraph said:
>
> As per our discussion, in V2 we will make drm_panel_remove() actually
> unprepare / disable a panel (and print a warning) if it was left
> enabled. This would essentially fold in the
> drm_panel_helper_shutdown() from my RFC patch (but add a warning).
> This would make tdo_tl070wsh30_panel_remove() behave the same as it
> did before with the addition of a warning if someone tries to remove a
> currently powered panel.
Ok, that works for me :)
Maxime
On Tue, Sep 05, 2023 at 09:45:49AM -0700, Doug Anderson wrote:
> > > In any case, I don't think there's any need to switch this to
> > > refcounting as part of this effort. Someone could, in theory, do it as
> > > a separate patch series.
> >
> > I'm sorry, but I'll insist on getting a solution that will warn panels
> > that call drm_atomic_helper_shutdown or drm_panel_disable/unprepare by
> > hand. It doesn't have to be refcounting though if you have a better idea
> > in mind.
>
> As per above, I think this already happens with the boolean? Won't you
> see an error message like this:
>
> dev_warn(panel->dev, "Skipping unprepare of already unprepared panel\n");
>
> ...from drm_panel_unprepare()
Urgh. I missed that part, sorry for dragging you into that refcounting
discussion then.
> > > > > The above solves the problem with panels wanting to power sequence
> > > > > themselves at remove() time, but not at shutdown() time. Thus we'd
> > > > > still have a dependency on having all drivers use
> > > > > drm_atomic_helper_shutdown() so that work becomes a dependency.
> > > >
> > > > Does it? I think it can be done in parallel?
> > >
> > > I don't think it can be in parallel. While it makes sense for panels
> > > to call drm_panel_remove() at remove time, it doesn't make sense for
> > > them to call it at shutdown time. That means that the trick of having
> > > the panel get powered off in drm_panel_remove() won't help for
> > > shutdown. For shutdown, which IMO is the more important case, we need
> > > to wait until all drm drivers call drm_atomic_helper_shutdown()
> > > properly.
> >
> > Right, my point was more that drivers that already don't disable the
> > panel in their shutdown implementation will still not do it. And drivers
> > that do will still do it, so there's no regression.
> >
> > We obviously want to tend to having all drivers call
> > drm_atomic_helper_shutdown(), but not having it will not introduce any
> > regression.
>
> I'm still confused here too. The next patch in this patch series here
> that we're talking about is:
>
> https://lore.kernel.org/dri-devel/20230804140605.RFC.5.Icc3238e91bc726d4b04c51a4acf67f001ec453d7@changeid/
>
> Let's look at an arbitrary concrete part of that patch: the
> modification to "panel-tdo-tl070wsh30.c". For that panel, my RFC patch
> removed the tracking of "prepared" and and then did this:
>
> @@ -220,16 +209,14 @@ static void tdo_tl070wsh30_panel_remove(struct
> mipi_dsi_device *dsi)
> dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", err);
>
> drm_panel_remove(&tdo_tl070wsh30->base);
> - drm_panel_disable(&tdo_tl070wsh30->base);
> - drm_panel_unprepare(&tdo_tl070wsh30->base);
> + drm_panel_helper_shutdown(&tdo_tl070wsh30->base);
> }
>
> static void tdo_tl070wsh30_panel_shutdown(struct mipi_dsi_device *dsi)
> {
> struct tdo_tl070wsh30_panel *tdo_tl070wsh30 = mipi_dsi_get_drvdata(dsi);
>
> - drm_panel_disable(&tdo_tl070wsh30->base);
> - drm_panel_unprepare(&tdo_tl070wsh30->base);
> + drm_panel_helper_shutdown(&tdo_tl070wsh30->base);
> }
>
> As per our discussion, the plan is to send a V2 of my patch series
> where I _don't_ create the call drm_panel_helper_shutdown() and thus I
> won't call it. That means that the V2 of the patch for
> "panel-tdo-tl070wsh30.c" will remove the calls to drm_panel_disable()
> and drm_panel_unprepare() and not replace them with anything.
Right, that's what we should do.
> As per our discussion, in V2 we will make drm_panel_remove() actually
> unprepare / disable a panel if it was left enabled. This would
> essentially fold in the drm_panel_helper_shutdown() from my RFC patch.
> This would make tdo_tl070wsh30_panel_remove() behave the same as it
> did before.
Not really? You would get a warning from drm_panel_remove(), but our
discussion very much implied still disabling it...
> Ugh, though I may have to think about this more when I get to
> implementation since I don't think there's a guarantee of the ordering
> of shutdown calls between the DRM driver and the panel. Anyway,
> something to discuss later.
>
> However... tdo_tl070wsh30_panel_shutdown() will no longer power the
> panel off properly _unless_ the main DRM driver properly calls
> drm_atomic_helper_shutdown().
... with the expectation that all KMS drivers should call
drm_atomic_helper_shutdown() anyway (thanks to your other series) and
thus we wouldn't trigger that remove warning anyway.
> Did I get anything above incorrect? Assuming I got it correct, that
> means that our choices are:
>
> a) Block landing the change to "panel-tdo-tl070wsh30.c" until after
> all drivers properly call drm_atomic_helper_shutdown().
I don't think we care about all drivers here. Only the driver it's
enabled with would be a blocker. Like, let's say sun4i hasn't been
converted but that panel is only used with rockchip anyway, we don't
really care that sun4i shutdown patch hasn't been merged (yet).
> b) Add a hacky call to drm_panel_remove() in
> tdo_tl070wsh30_panel_shutdown() to get the old behavior. This would
> work, but IMO it's ugly and I'm pretty strongly against it.
Yeah, it's ugly.
> c) Ignore the regression and just say that this panel won't get power
> sequenced properly until its DRM driver is updated. I'm strongly
> against this.
That would work too, but the first one looks like the best trade-off to
me.
Maxime
Hi,
On Fri, Aug 4, 2023 at 2:07 PM Douglas Anderson <[email protected]> wrote:
>
> As talked about in commit d2aacaf07395 ("drm/panel: Check for already
> prepared/enabled in drm_panel"), we want to remove needless code from
> panel drivers that was storing and double-checking the
> prepared/enabled state. Even if someone was relying on the
> double-check before, that double-check is now in the core and not
> needed in individual drivers.
>
> This pile of panel drivers appears to be simple to handle. Based on
> code inspection they seemed to be using the prepared/enabled state
> simply for double-checking that nothing else in the kernel called them
> inconsistently. Now that the core drm_panel is doing the double
> checking (and warning) it should be very clear that these devices
> don't need their own double-check.
>
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
>
> .../drm/panel/panel-asus-z00t-tm5p5-n35596.c | 9 -----
> .../gpu/drm/panel/panel-boe-bf060y8m-aj0.c | 9 -----
> drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c | 9 -----
> drivers/gpu/drm/panel/panel-novatek-nt35950.c | 9 -----
> drivers/gpu/drm/panel/panel-novatek-nt36523.c | 12 ------
> drivers/gpu/drm/panel/panel-raydium-rm68200.c | 38 -------------------
> .../panel/panel-samsung-s6e88a0-ams452ef01.c | 10 -----
> drivers/gpu/drm/panel/panel-samsung-sofef00.c | 9 -----
> .../gpu/drm/panel/panel-sharp-ls060t1sx01.c | 10 -----
> drivers/gpu/drm/panel/panel-sony-td4353-jdi.c | 9 -----
> .../panel/panel-sony-tulip-truly-nt35521.c | 18 ---------
> .../drm/panel/panel-startek-kd070fhfid015.c | 11 ------
> drivers/gpu/drm/panel/panel-truly-nt35597.c | 20 ----------
> drivers/gpu/drm/panel/panel-visionox-r66451.c | 16 --------
> .../gpu/drm/panel/panel-visionox-rm69299.c | 8 ----
> .../gpu/drm/panel/panel-visionox-vtdr6130.c | 9 -----
> 16 files changed, 206 deletions(-)
In response to the cover letter [1], I proposed landing patches #1-#3
directly from here while we resolve the issues talked about in
response to patch #4 [2]. I didn't hear any complaints, so I took
Linus W's review tag from the cover letter and pushed this to
drm-misc-next.
f8c37b88092e drm/panel: Don't store+check prepared/enabled for simple cases
[1] https://lore.kernel.org/r/CAD=FV=UFuUsrrZmkL8_RL5WLvkJryDwRSAy_PWTa-hX_p0dF+Q@mail.gmail.com
[2] https://lore.kernel.org/r/20230804140605.RFC.4.I930069a32baab6faf46d6b234f89613b5cec0f14@changeid/
Hi,
On Thu, Sep 7, 2023 at 7:15 AM Maxime Ripard <[email protected]> wrote:
>
> > a) Block landing the change to "panel-tdo-tl070wsh30.c" until after
> > all drivers properly call drm_atomic_helper_shutdown().
>
> I don't think we care about all drivers here. Only the driver it's
> enabled with would be a blocker. Like, let's say sun4i hasn't been
> converted but that panel is only used with rockchip anyway, we don't
> really care that sun4i shutdown patch hasn't been merged (yet).
Yeah, I suppose that would work, though it would require a bit of
research. Some other things have popped onto my plate recently, but
for now I'm going to focus on seeing how much of the drivers can get
their shutdown fixed. When that stalls out then we can see if we can
unblock some of the panels by digging into which DRM drivers they're
used with.
Also, as my proposal in the cover letter [1], I'm leaving a breadcrumb
here that I landed the first 3 patches in this series just to get them
out of the way. Those 3 patches didn't depend on the resolution of the
issues discussed here.
[1] https://lore.kernel.org/lkml/CAD=FV=UFuUsrrZmkL8_RL5WLvkJryDwRSAy_PWTa-hX_p0dF+Q@mail.gmail.com/
Hi,
On Fri, Aug 4, 2023 at 2:07 PM Douglas Anderson <[email protected]> wrote:
>
> As talked about in commit d2aacaf07395 ("drm/panel: Check for already
> prepared/enabled in drm_panel"), we want to remove needless code from
> panel drivers that was storing and double-checking the
> prepared/enabled state. Even if someone was relying on the
> double-check before, that double-check is now in the core and not
> needed in individual drivers.
>
> For the s6e63m0 panel driver, this actually fixes a subtle/minor error
> handling bug in s6e63m0_prepare(). In one error case s6e63m0_prepare()
> called s6e63m0_unprepare() directly if there was an error. This call
> to s6e63m0_unprepare() would have been a no-op since ctx->prepared
> wasn't set yet.
>
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
>
> drivers/gpu/drm/panel/panel-samsung-s6e63m0.c | 25 -------------------
> 1 file changed, 25 deletions(-)
In response to the cover letter [1], I proposed landing patches #1-#3
directly from here while we resolve the issues talked about in
response to patch #4 [2]. I didn't hear any complaints, so I took
Linus W's review tag from the cover letter and pushed this to
drm-misc-next.
d43f0fe153dc drm/panel: s6e63m0: Don't store+check prepared/enabled
[1] https://lore.kernel.org/r/CAD=FV=UFuUsrrZmkL8_RL5WLvkJryDwRSAy_PWTa-hX_p0dF+Q@mail.gmail.com
[2] https://lore.kernel.org/r/20230804140605.RFC.4.I930069a32baab6faf46d6b234f89613b5cec0f14@changeid/