* [RFC PATCH 01/10] drm/panel: Don't store+check prepared/enabled for simple cases
2023-08-04 21:06 [RFC PATCH 00/10] drm/panel: Remove most store/double-check of prepared/enabled state Douglas Anderson
@ 2023-08-04 21:06 ` Douglas Anderson
2023-09-13 18:20 ` Doug Anderson
2023-08-04 21:06 ` [RFC PATCH 02/10] drm/panel: s6e63m0: Don't store+check prepared/enabled Douglas Anderson
` (9 subsequent siblings)
10 siblings, 1 reply; 34+ messages in thread
From: Douglas Anderson @ 2023-08-04 21:06 UTC (permalink / raw)
To: dri-devel, Maxime Ripard
Cc: Neil Armstrong, Sam Ravnborg, Douglas Anderson, linux-kernel,
Jianhua Lu
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 <dianders@chromium.org>
---
.../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
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [RFC PATCH 01/10] drm/panel: Don't store+check prepared/enabled for simple cases
2023-08-04 21:06 ` [RFC PATCH 01/10] drm/panel: Don't store+check prepared/enabled for simple cases Douglas Anderson
@ 2023-09-13 18:20 ` Doug Anderson
0 siblings, 0 replies; 34+ messages in thread
From: Doug Anderson @ 2023-09-13 18:20 UTC (permalink / raw)
To: dri-devel, Maxime Ripard
Cc: Neil Armstrong, linux-kernel, Jianhua Lu, Sam Ravnborg
Hi,
On Fri, Aug 4, 2023 at 2:07 PM Douglas Anderson <dianders@chromium.org> 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 <dianders@chromium.org>
> ---
>
> .../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/
^ permalink raw reply [flat|nested] 34+ messages in thread
* [RFC PATCH 02/10] drm/panel: s6e63m0: Don't store+check prepared/enabled
2023-08-04 21:06 [RFC PATCH 00/10] drm/panel: Remove most store/double-check of prepared/enabled state Douglas Anderson
2023-08-04 21:06 ` [RFC PATCH 01/10] drm/panel: Don't store+check prepared/enabled for simple cases Douglas Anderson
@ 2023-08-04 21:06 ` Douglas Anderson
2023-09-13 18:20 ` Doug Anderson
2023-08-04 21:06 ` [RFC PATCH 03/10] drm/panel: otm8009a: Don't double check prepared/enabled Douglas Anderson
` (8 subsequent siblings)
10 siblings, 1 reply; 34+ messages in thread
From: Douglas Anderson @ 2023-08-04 21:06 UTC (permalink / raw)
To: dri-devel, Maxime Ripard
Cc: Neil Armstrong, Douglas Anderson, linux-kernel, Sam Ravnborg
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 <dianders@chromium.org>
---
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
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [RFC PATCH 02/10] drm/panel: s6e63m0: Don't store+check prepared/enabled
2023-08-04 21:06 ` [RFC PATCH 02/10] drm/panel: s6e63m0: Don't store+check prepared/enabled Douglas Anderson
@ 2023-09-13 18:20 ` Doug Anderson
0 siblings, 0 replies; 34+ messages in thread
From: Doug Anderson @ 2023-09-13 18:20 UTC (permalink / raw)
To: dri-devel, Maxime Ripard; +Cc: Neil Armstrong, linux-kernel, Sam Ravnborg
Hi,
On Fri, Aug 4, 2023 at 2:07 PM Douglas Anderson <dianders@chromium.org> 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 <dianders@chromium.org>
> ---
>
> 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/
^ permalink raw reply [flat|nested] 34+ messages in thread
* [RFC PATCH 03/10] drm/panel: otm8009a: Don't double check prepared/enabled
2023-08-04 21:06 [RFC PATCH 00/10] drm/panel: Remove most store/double-check of prepared/enabled state Douglas Anderson
2023-08-04 21:06 ` [RFC PATCH 01/10] drm/panel: Don't store+check prepared/enabled for simple cases Douglas Anderson
2023-08-04 21:06 ` [RFC PATCH 02/10] drm/panel: s6e63m0: Don't store+check prepared/enabled Douglas Anderson
@ 2023-08-04 21:06 ` Douglas Anderson
2023-09-13 18:20 ` Doug Anderson
2023-08-04 21:06 ` [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper Douglas Anderson
` (7 subsequent siblings)
10 siblings, 1 reply; 34+ messages in thread
From: Douglas Anderson @ 2023-08-04 21:06 UTC (permalink / raw)
To: dri-devel, Maxime Ripard
Cc: Neil Armstrong, Douglas Anderson, linux-kernel, Sam Ravnborg
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 "otm8009a" driver we fully remove the storing of the "enabled"
state and we remove the double-checking, but we still keep the storing
of the "prepared" state since the backlight code in the driver checks
it. This backlight code may not be perfectly safe since there doesn't
appear to be sufficient synchronization between the backlight driver
(which userspace can call into directly) and the code that's
unpreparing the panel. However, this lack of safety is not new and can
be addressed in a future patch.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
From quick inspection, I think the right way to handle the backlight
properly is:
1. Start calling backlight_get_brightness() instead of directly
getting "bd->props.brightness" and bd->props.power. This should
return 0 for a disabled (or blanked or powered off) backlight.
2. Cache the backlight level in "struct otm8009a"
3. If the backlight isn't changing compared to the cached value, make
otm8009a_backlight_update_status() a no-op.
4. Remove the caching of the "prepared" value.
That should work and always be safe because we always enable/disable
the backlight in the panel's enable() and disable() functions. The
backlight core has proper locking in this case. A disabled backlight
will always return a level of 0 which will always make the backlight's
update_status a no-op when the panel is disabled and keep us from
trying to talk to the panel when it's off. Userspace can't directly
cause a backlight to be enabled/disabled, it can only affect the other
blanking modes.
.../gpu/drm/panel/panel-orisetech-otm8009a.c | 17 -----------------
1 file changed, 17 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
index 898b892f1143..93183f30d7d6 100644
--- a/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
+++ b/drivers/gpu/drm/panel/panel-orisetech-otm8009a.c
@@ -70,7 +70,6 @@ struct otm8009a {
struct gpio_desc *reset_gpio;
struct regulator *supply;
bool prepared;
- bool enabled;
};
static const struct drm_display_mode modes[] = {
@@ -267,9 +266,6 @@ static int otm8009a_disable(struct drm_panel *panel)
struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
int ret;
- if (!ctx->enabled)
- return 0; /* This is not an issue so we return 0 here */
-
backlight_disable(ctx->bl_dev);
ret = mipi_dsi_dcs_set_display_off(dsi);
@@ -282,8 +278,6 @@ static int otm8009a_disable(struct drm_panel *panel)
msleep(120);
- ctx->enabled = false;
-
return 0;
}
@@ -291,9 +285,6 @@ static int otm8009a_unprepare(struct drm_panel *panel)
{
struct otm8009a *ctx = panel_to_otm8009a(panel);
- if (!ctx->prepared)
- return 0;
-
if (ctx->reset_gpio) {
gpiod_set_value_cansleep(ctx->reset_gpio, 1);
msleep(20);
@@ -311,9 +302,6 @@ static int otm8009a_prepare(struct drm_panel *panel)
struct otm8009a *ctx = panel_to_otm8009a(panel);
int ret;
- if (ctx->prepared)
- return 0;
-
ret = regulator_enable(ctx->supply);
if (ret < 0) {
dev_err(panel->dev, "failed to enable supply: %d\n", ret);
@@ -341,13 +329,8 @@ static int otm8009a_enable(struct drm_panel *panel)
{
struct otm8009a *ctx = panel_to_otm8009a(panel);
- if (ctx->enabled)
- return 0;
-
backlight_enable(ctx->bl_dev);
- ctx->enabled = true;
-
return 0;
}
--
2.41.0.585.gd2178a4bd4-goog
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [RFC PATCH 03/10] drm/panel: otm8009a: Don't double check prepared/enabled
2023-08-04 21:06 ` [RFC PATCH 03/10] drm/panel: otm8009a: Don't double check prepared/enabled Douglas Anderson
@ 2023-09-13 18:20 ` Doug Anderson
0 siblings, 0 replies; 34+ messages in thread
From: Doug Anderson @ 2023-09-13 18:20 UTC (permalink / raw)
To: dri-devel, Maxime Ripard; +Cc: Neil Armstrong, linux-kernel, Sam Ravnborg
Hi,
On Fri, Aug 4, 2023 at 2:07 PM Douglas Anderson <dianders@chromium.org> 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 "otm8009a" driver we fully remove the storing of the "enabled"
> state and we remove the double-checking, but we still keep the storing
> of the "prepared" state since the backlight code in the driver checks
> it. This backlight code may not be perfectly safe since there doesn't
> appear to be sufficient synchronization between the backlight driver
> (which userspace can call into directly) and the code that's
> unpreparing the panel. However, this lack of safety is not new and can
> be addressed in a future patch.
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> From quick inspection, I think the right way to handle the backlight
> properly is:
> 1. Start calling backlight_get_brightness() instead of directly
> getting "bd->props.brightness" and bd->props.power. This should
> return 0 for a disabled (or blanked or powered off) backlight.
> 2. Cache the backlight level in "struct otm8009a"
> 3. If the backlight isn't changing compared to the cached value, make
> otm8009a_backlight_update_status() a no-op.
> 4. Remove the caching of the "prepared" value.
>
> That should work and always be safe because we always enable/disable
> the backlight in the panel's enable() and disable() functions. The
> backlight core has proper locking in this case. A disabled backlight
> will always return a level of 0 which will always make the backlight's
> update_status a no-op when the panel is disabled and keep us from
> trying to talk to the panel when it's off. Userspace can't directly
> cause a backlight to be enabled/disabled, it can only affect the other
> blanking modes.
Note: I'm not planning to take on the cleanup of making the backlight
of this driver work better. Ideally someone who uses / maintains the
affected hardware could give it a shot.
> .../gpu/drm/panel/panel-orisetech-otm8009a.c | 17 -----------------
> 1 file changed, 17 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.
1e0465eb16a4 drm/panel: otm8009a: Don't double 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/
^ permalink raw reply [flat|nested] 34+ messages in thread
* [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper
2023-08-04 21:06 [RFC PATCH 00/10] drm/panel: Remove most store/double-check of prepared/enabled state Douglas Anderson
` (2 preceding siblings ...)
2023-08-04 21:06 ` [RFC PATCH 03/10] drm/panel: otm8009a: Don't double check prepared/enabled Douglas Anderson
@ 2023-08-04 21:06 ` Douglas Anderson
2023-08-07 6:41 ` Maxime Ripard
2023-08-04 21:06 ` [RFC PATCH 05/10] drm/panel: Don't store+check prepared/enabled for panels needing shutdown Douglas Anderson
` (6 subsequent siblings)
10 siblings, 1 reply; 34+ messages in thread
From: Douglas Anderson @ 2023-08-04 21:06 UTC (permalink / raw)
To: dri-devel, Maxime Ripard
Cc: Thomas Zimmermann, Douglas Anderson, linux-kernel
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 <dianders@chromium.org>
---
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
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper
2023-08-04 21:06 ` [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper Douglas Anderson
@ 2023-08-07 6:41 ` Maxime Ripard
2023-08-25 21:58 ` Doug Anderson
0 siblings, 1 reply; 34+ messages in thread
From: Maxime Ripard @ 2023-08-07 6:41 UTC (permalink / raw)
To: Douglas Anderson; +Cc: Thomas Zimmermann, linux-kernel, dri-devel
[-- Attachment #1: Type: text/plain, Size: 931 bytes --]
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 <dianders@chromium.org>
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper
2023-08-07 6:41 ` Maxime Ripard
@ 2023-08-25 21:58 ` Doug Anderson
2023-08-28 7:45 ` Maxime Ripard
0 siblings, 1 reply; 34+ messages in thread
From: Doug Anderson @ 2023-08-25 21:58 UTC (permalink / raw)
To: Maxime Ripard; +Cc: Thomas Zimmermann, linux-kernel, dri-devel
Maxime,
On Sun, Aug 6, 2023 at 11:41 PM Maxime Ripard <mripard@kernel.org> 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 <dianders@chromium.org>
>
> 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. 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... 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. 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. 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 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...
...but then, what's the way forward? I think normally the panel's
shutdown() callback would happen _before_ the DRM driver's shutdown()
callback, 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?
-Doug
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper
2023-08-25 21:58 ` Doug Anderson
@ 2023-08-28 7:45 ` Maxime Ripard
2023-08-28 16:06 ` Doug Anderson
0 siblings, 1 reply; 34+ messages in thread
From: Maxime Ripard @ 2023-08-28 7:45 UTC (permalink / raw)
To: Doug Anderson; +Cc: Thomas Zimmermann, linux-kernel, dri-devel
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 <mripard@kernel.org> 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 <dianders@chromium.org>
> >
> > 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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper
2023-08-28 7:45 ` Maxime Ripard
@ 2023-08-28 16:06 ` Doug Anderson
2023-08-29 8:38 ` Maxime Ripard
0 siblings, 1 reply; 34+ messages in thread
From: Doug Anderson @ 2023-08-28 16:06 UTC (permalink / raw)
To: Maxime Ripard; +Cc: Thomas Zimmermann, linux-kernel, dri-devel
Hi,
On Mon, Aug 28, 2023 at 12:45 AM Maxime Ripard <mripard@kernel.org> wrote:
>
> > 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 :)
Good point. Obviously those cases need to work. I guess I've just
spent lots of time on the SoC case where all the pieces coming
together are very intertwined with each other...
> > 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?
> > 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...
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.
-Doug
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper
2023-08-28 16:06 ` Doug Anderson
@ 2023-08-29 8:38 ` Maxime Ripard
2023-08-30 23:10 ` Doug Anderson
0 siblings, 1 reply; 34+ messages in thread
From: Maxime Ripard @ 2023-08-29 8:38 UTC (permalink / raw)
To: Doug Anderson; +Cc: Thomas Zimmermann, linux-kernel, dri-devel
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper
2023-08-29 8:38 ` Maxime Ripard
@ 2023-08-30 23:10 ` Doug Anderson
2023-08-31 7:38 ` Maxime Ripard
0 siblings, 1 reply; 34+ messages in thread
From: Doug Anderson @ 2023-08-30 23:10 UTC (permalink / raw)
To: Maxime Ripard; +Cc: Thomas Zimmermann, linux-kernel, dri-devel
Hi,
On Tue, Aug 29, 2023 at 1:38 AM Maxime Ripard <mripard@kernel.org> 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?
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.
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.
Should this issue block my work, then? 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?
> > > > 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.
> 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?
> 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.
imx/dcss seems to be missing drm_atomic_helper_shutdown() at system
shutdown time, though it does have it at driver remove time. Did I
mess up in identifying those two drivers as ones that need fixing?
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.
-Doug
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper
2023-08-30 23:10 ` Doug Anderson
@ 2023-08-31 7:38 ` Maxime Ripard
2023-08-31 18:18 ` Doug Anderson
0 siblings, 1 reply; 34+ messages in thread
From: Maxime Ripard @ 2023-08-31 7:38 UTC (permalink / raw)
To: Doug Anderson; +Cc: Thomas Zimmermann, linux-kernel, dri-devel
[-- Attachment #1: Type: text/plain, Size: 17065 bytes --]
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 <mripard@kernel.org> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper
2023-08-31 7:38 ` Maxime Ripard
@ 2023-08-31 18:18 ` Doug Anderson
2023-09-01 8:15 ` Maxime Ripard
0 siblings, 1 reply; 34+ messages in thread
From: Doug Anderson @ 2023-08-31 18:18 UTC (permalink / raw)
To: Maxime Ripard; +Cc: Thomas Zimmermann, linux-kernel, dri-devel
Hi,
On Thu, Aug 31, 2023 at 12:38 AM Maxime Ripard <mripard@kernel.org> 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? 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).
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.
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.
> - 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? 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. 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.
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.
> > 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.
I'll just plug through with my original list for now, then I can try
your script when I'm done and see if it catches anything I missed.
-Doug
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper
2023-08-31 18:18 ` Doug Anderson
@ 2023-09-01 8:15 ` Maxime Ripard
2023-09-01 13:42 ` Doug Anderson
0 siblings, 1 reply; 34+ messages in thread
From: Maxime Ripard @ 2023-09-01 8:15 UTC (permalink / raw)
To: Doug Anderson; +Cc: Thomas Zimmermann, linux-kernel, dri-devel
[-- Attachment #1: Type: text/plain, Size: 4330 bytes --]
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 <mripard@kernel.org> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper
2023-09-01 8:15 ` Maxime Ripard
@ 2023-09-01 13:42 ` Doug Anderson
2023-09-01 23:44 ` Doug Anderson
2023-09-04 15:33 ` Maxime Ripard
0 siblings, 2 replies; 34+ messages in thread
From: Doug Anderson @ 2023-09-01 13:42 UTC (permalink / raw)
To: Maxime Ripard; +Cc: Thomas Zimmermann, linux-kernel, dri-devel
Hi,
On Fri, Sep 1, 2023 at 1:15 AM Maxime Ripard <mripard@kernel.org> 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 <mripard@kernel.org> 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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper
2023-09-01 13:42 ` Doug Anderson
@ 2023-09-01 23:44 ` Doug Anderson
2023-09-04 15:33 ` Maxime Ripard
1 sibling, 0 replies; 34+ messages in thread
From: Doug Anderson @ 2023-09-01 23:44 UTC (permalink / raw)
To: Maxime Ripard; +Cc: Thomas Zimmermann, linux-kernel, dri-devel
Hi,
On Fri, Sep 1, 2023 at 6:42 AM Doug Anderson <dianders@chromium.org> wrote:
>
> > > 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.
FWIW, it was a bit of a slog, but I've managed to put together a RFT
series. Good thing it's Friday and beer-o-clock. Please enjoy
reviewing.
Ugh. It's now two series because there are too many recipients. Email
is fun. OK, these should be at:
* https://lore.kernel.org/r/20230901234015.566018-1-dianders@chromium.org
- patches targeting drm-misc
* https://lore.kernel.org/r/20230901234202.566951-1-dianders@chromium.org
- patches targeting something that's not drm-misc
-Doug
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper
2023-09-01 13:42 ` Doug Anderson
2023-09-01 23:44 ` Doug Anderson
@ 2023-09-04 15:33 ` Maxime Ripard
2023-09-05 16:45 ` Doug Anderson
1 sibling, 1 reply; 34+ messages in thread
From: Maxime Ripard @ 2023-09-04 15:33 UTC (permalink / raw)
To: Doug Anderson; +Cc: Thomas Zimmermann, linux-kernel, dri-devel
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 <mripard@kernel.org> 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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper
2023-09-04 15:33 ` Maxime Ripard
@ 2023-09-05 16:45 ` Doug Anderson
2023-09-05 19:12 ` Doug Anderson
2023-09-07 14:14 ` Maxime Ripard
0 siblings, 2 replies; 34+ messages in thread
From: Doug Anderson @ 2023-09-05 16:45 UTC (permalink / raw)
To: Maxime Ripard; +Cc: Thomas Zimmermann, linux-kernel, dri-devel
Hi,
On Mon, Sep 4, 2023 at 8:33 AM Maxime Ripard <mripard@kernel.org> wrote:
>
> > 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.
Can you provide a concrete example of a case where refcounting would
give a better error? I'm still having a hard time understanding why
you are saying that refcounting is better and something concrete would
help me. Can you point at a driver you have in mind that follows an
anti-pattern where refcount would be better?
I'll try to be more concrete too and maybe you can point out where I'm
confused. As far as I understand the only difference between the
boolean and the refcount would be if someone _enabled_ or _prepared_
more than once, right? That would cause a refcount to increment to 2
but the boolean would stay at "true". I'm not aware of anyone calling
enable or prepare more than once, but maybe you are? ...or maybe
there's some other difference that I'm not aware of?
Said another way...
With a boolean and _not_ having more than one enable:
1. enable() => set "enabled" to true and enable panel.
2. disable() => set "enabled" to false and disable panel.
3. disable() => WARN, leave "enabled" as false.
4. disable() => WARN, leave "enabled" as false
With a refcount and _not_ having more than one enable:
1. enable() => refcount becomes 1 and enable panel
2. disable() => refcount becomes 0 and disable panel
3. disable() => WARN, refcount stays 0
4. disable() => WARN, refcount stays 0
So with only one enable the behavior is the same.
With a boolean and more than one enable:
1. enable() => set "enabled" to true and enable panel.
2. enable() => WARN, leave "enabled" as true
3. disable() => set "enabled" to false and disable panel.
4. disable() => WARN, leave "enabled" as false.
5. disable() => WARN, leave "enabled" as false
With a refcount and more than one enable:
1. enable() => refcount becomes 1 and enable panel
2. enable() => refcount becomes 2
3. disable() => refcount becomes 1
4. disable() => refcount becomes 0 and disable panel
5. disable() => WARN, refcount stays 0
In the case that there is more than one enable, I think the "boolean"
is better. Specifically:
a) It doesn't change behavior from today. Perhaps you can show me a
counterexample, but lacking that I'd assert that everyone today is
expecting things to work like the "boolean" case. If we change to a
refcount I think it could break someone. Even if nobody is relying on
things working like the "boolean" case today, I would assert that I
don't think anyone is expecting things to work like the "refcount"
case.
b) With a boolean we WARN at more appropriate times. Sure we could add
a warning when the refcount becomes 2, but at that point why aren't we
just using a boolean?
c) The "boolean" case is already implemented today so no patches need
to be sent for it.
> > 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()
> > > > 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.
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.
However... tdo_tl070wsh30_panel_shutdown() will no longer power the
panel off properly _unless_ the main DRM driver properly calls
drm_atomic_helper_shutdown().
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().
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.
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.
...unless you are aware of another choice, the only choice I'm willing
to go with is "a)" which is why I say we are blocked on getting all
drivers to properly call drm_atomic_helper_shutdown().
NOTE: I could still land patches #1-3 of this series and I'm actually
included to do so. I'll respond to the cover letter proposing this.
-Doug
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper
2023-09-05 16:45 ` Doug Anderson
@ 2023-09-05 19:12 ` Doug Anderson
2023-09-07 14:16 ` Maxime Ripard
2023-09-07 14:14 ` Maxime Ripard
1 sibling, 1 reply; 34+ messages in thread
From: Doug Anderson @ 2023-09-05 19:12 UTC (permalink / raw)
To: Maxime Ripard; +Cc: Thomas Zimmermann, linux-kernel, dri-devel
Hi,
On Tue, Sep 5, 2023 at 9:45 AM Doug Anderson <dianders@chromium.org> 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.
-Doug
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper
2023-09-05 19:12 ` Doug Anderson
@ 2023-09-07 14:16 ` Maxime Ripard
0 siblings, 0 replies; 34+ messages in thread
From: Maxime Ripard @ 2023-09-07 14:16 UTC (permalink / raw)
To: Doug Anderson; +Cc: Thomas Zimmermann, linux-kernel, dri-devel
[-- Attachment #1: Type: text/plain, Size: 1367 bytes --]
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 <dianders@chromium.org> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper
2023-09-05 16:45 ` Doug Anderson
2023-09-05 19:12 ` Doug Anderson
@ 2023-09-07 14:14 ` Maxime Ripard
2023-09-13 18:28 ` Doug Anderson
1 sibling, 1 reply; 34+ messages in thread
From: Maxime Ripard @ 2023-09-07 14:14 UTC (permalink / raw)
To: Doug Anderson; +Cc: Thomas Zimmermann, linux-kernel, dri-devel
[-- Attachment #1: Type: text/plain, Size: 5509 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper
2023-09-07 14:14 ` Maxime Ripard
@ 2023-09-13 18:28 ` Doug Anderson
0 siblings, 0 replies; 34+ messages in thread
From: Doug Anderson @ 2023-09-13 18:28 UTC (permalink / raw)
To: Maxime Ripard; +Cc: Thomas Zimmermann, linux-kernel, dri-devel
Hi,
On Thu, Sep 7, 2023 at 7:15 AM Maxime Ripard <mripard@kernel.org> 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/
^ permalink raw reply [flat|nested] 34+ messages in thread
* [RFC PATCH 05/10] drm/panel: Don't store+check prepared/enabled for panels needing shutdown
2023-08-04 21:06 [RFC PATCH 00/10] drm/panel: Remove most store/double-check of prepared/enabled state Douglas Anderson
` (3 preceding siblings ...)
2023-08-04 21:06 ` [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper Douglas Anderson
@ 2023-08-04 21:06 ` Douglas Anderson
2023-08-04 21:06 ` [RFC PATCH 06/10] drm/panel: Don't store+check prepared/enabled for panels disabled at shutdown Douglas Anderson
` (5 subsequent siblings)
10 siblings, 0 replies; 34+ messages in thread
From: Douglas Anderson @ 2023-08-04 21:06 UTC (permalink / raw)
To: dri-devel, Maxime Ripard
Cc: Neil Armstrong, Stefan Mavrodiev, Sam Ravnborg, Jerry Han,
Douglas Anderson, Javier Martinez Canillas, Ondrej Jirman,
Sumit Semwal, linux-kernel
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.
A number of panels seemed to need the extra double-checking of the
prepared/enabled state to handle driver remove and/or shutdown. This
set of drivers was easy to transform and used to call
drm_panel_unprepare() and drm_panel_disable(). It's easy to move them
to call the drm_panel_helper_shutdown() that does the double-checking
for the panels.
NOTE: this patch doesn't attempt to sanitize the shutdown or remove
functions of these panels, it merely tries to preserve the old
behavior while removing the need for the panels to track
prepared/enabled state themselves. Specifically it can be noted that
removing an in-use panel is not necessarily straightfoward and may not
be correct in most panels.
ALSO NOTE: some of the panels touched in this path used to not
complain about disable/unprepare error at shutdown time. Now that
we're using the drm_panel_helper_shutdown() function we'll
consistently warn about these errors.
THIRDLY NOTE: One of these panels, "boe-himax8279d", used to call its
unprepare() and disable() functions directly instead of calling
drm_panel_unprepare() and drm_panel_disable(). I believe that the only
difference is that "boe-himax8279d" will now turn off its backlight at
shutdown/remove. This will also pave the way if anyone wants to use
this panel w/ the new "panel follower" APIs.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
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-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 +-------
.../gpu/drm/panel/panel-novatek-nt36672a.c | 24 ++-------
.../drm/panel/panel-olimex-lcd-olinuxino.c | 45 +---------------
.../drm/panel/panel-osd-osd101t2587-53ts.c | 37 ++------------
.../gpu/drm/panel/panel-samsung-atna33xc20.c | 31 ++---------
drivers/gpu/drm/panel/panel-simple.c | 34 ++-----------
drivers/gpu/drm/panel/panel-tdo-tl070wsh30.c | 19 ++-----
.../gpu/drm/panel/panel-xinpeng-xpp055c272.c | 21 +-------
17 files changed, 45 insertions(+), 465 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-boe-himax8279d.c b/drivers/gpu/drm/panel/panel-boe-himax8279d.c
index 11b64acbe8a9..cccf9400fa99 100644
--- a/drivers/gpu/drm/panel/panel-boe-himax8279d.c
+++ b/drivers/gpu/drm/panel/panel-boe-himax8279d.c
@@ -18,6 +18,7 @@
#include <drm/drm_mipi_dsi.h>
#include <drm/drm_modes.h>
#include <drm/drm_panel.h>
+#include <drm/drm_panel_helper.h>
#include <video/mipi_display.h>
@@ -47,9 +48,6 @@ struct panel_info {
struct gpio_desc *enable_gpio;
struct gpio_desc *pp33_gpio;
struct gpio_desc *pp18_gpio;
-
- bool prepared;
- bool enabled;
};
static inline struct panel_info *to_panel_info(struct drm_panel *panel)
@@ -86,17 +84,12 @@ static int boe_panel_disable(struct drm_panel *panel)
struct panel_info *pinfo = to_panel_info(panel);
int err;
- if (!pinfo->enabled)
- return 0;
-
err = mipi_dsi_dcs_set_display_off(pinfo->link);
if (err < 0) {
dev_err(panel->dev, "failed to set display off: %d\n", err);
return err;
}
- pinfo->enabled = false;
-
return 0;
}
@@ -105,9 +98,6 @@ static int boe_panel_unprepare(struct drm_panel *panel)
struct panel_info *pinfo = to_panel_info(panel);
int err;
- if (!pinfo->prepared)
- return 0;
-
err = mipi_dsi_dcs_set_display_off(pinfo->link);
if (err < 0)
dev_err(panel->dev, "failed to set display off: %d\n", err);
@@ -121,8 +111,6 @@ static int boe_panel_unprepare(struct drm_panel *panel)
disable_gpios(pinfo);
- pinfo->prepared = false;
-
return 0;
}
@@ -131,9 +119,6 @@ static int boe_panel_prepare(struct drm_panel *panel)
struct panel_info *pinfo = to_panel_info(panel);
int err;
- if (pinfo->prepared)
- return 0;
-
gpiod_set_value(pinfo->pp18_gpio, 1);
/* T1: 5ms - 6ms */
usleep_range(5000, 6000);
@@ -180,8 +165,6 @@ static int boe_panel_prepare(struct drm_panel *panel)
/* T7: 20ms - 21ms */
usleep_range(20000, 21000);
- pinfo->prepared = true;
-
return 0;
poweroff:
@@ -194,9 +177,6 @@ static int boe_panel_enable(struct drm_panel *panel)
struct panel_info *pinfo = to_panel_info(panel);
int ret;
- if (pinfo->enabled)
- return 0;
-
usleep_range(120000, 121000);
ret = mipi_dsi_dcs_set_display_on(pinfo->link);
@@ -205,8 +185,6 @@ static int boe_panel_enable(struct drm_panel *panel)
return ret;
}
- pinfo->enabled = true;
-
return 0;
}
@@ -923,14 +901,7 @@ static void panel_remove(struct mipi_dsi_device *dsi)
struct panel_info *pinfo = mipi_dsi_get_drvdata(dsi);
int err;
- err = boe_panel_disable(&pinfo->base);
- if (err < 0)
- dev_err(&dsi->dev, "failed to disable panel: %d\n", err);
-
- err = boe_panel_unprepare(&pinfo->base);
- if (err < 0)
- dev_err(&dsi->dev, "failed to unprepare panel: %d\n", err);
-
+ drm_panel_helper_shutdown(&pinfo->base);
err = mipi_dsi_detach(dsi);
if (err < 0)
dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", err);
@@ -942,8 +913,7 @@ static void panel_shutdown(struct mipi_dsi_device *dsi)
{
struct panel_info *pinfo = mipi_dsi_get_drvdata(dsi);
- boe_panel_disable(&pinfo->base);
- boe_panel_unprepare(&pinfo->base);
+ drm_panel_helper_shutdown(&pinfo->base);
}
static struct mipi_dsi_driver panel_driver = {
diff --git a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
index 5ac926281d2c..72cb9a10fe43 100644
--- a/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
+++ b/drivers/gpu/drm/panel/panel-boe-tv101wum-nl6.c
@@ -14,6 +14,7 @@
#include <drm/drm_crtc.h>
#include <drm/drm_mipi_dsi.h>
#include <drm/drm_panel.h>
+#include <drm/drm_panel_helper.h>
#include <video/mipi_display.h>
@@ -50,8 +51,6 @@ struct boe_panel {
struct regulator *avee;
struct regulator *avdd;
struct gpio_desc *enable_gpio;
-
- bool prepared;
};
enum dsi_cmd_type {
@@ -1792,9 +1791,6 @@ static int boe_panel_unprepare(struct drm_panel *panel)
{
struct boe_panel *boe = to_boe_panel(panel);
- if (!boe->prepared)
- return 0;
-
if (boe->desc->discharge_on_disable) {
regulator_disable(boe->avee);
regulator_disable(boe->avdd);
@@ -1813,8 +1809,6 @@ static int boe_panel_unprepare(struct drm_panel *panel)
regulator_disable(boe->pp3300);
}
- boe->prepared = false;
-
return 0;
}
@@ -1823,9 +1817,6 @@ static int boe_panel_prepare(struct drm_panel *panel)
struct boe_panel *boe = to_boe_panel(panel);
int ret;
- if (boe->prepared)
- return 0;
-
gpiod_set_value(boe->enable_gpio, 0);
usleep_range(1000, 1500);
@@ -1865,8 +1856,6 @@ static int boe_panel_prepare(struct drm_panel *panel)
goto poweroff;
}
- boe->prepared = true;
-
return 0;
poweroff:
@@ -2292,8 +2281,7 @@ static void boe_panel_shutdown(struct mipi_dsi_device *dsi)
{
struct boe_panel *boe = mipi_dsi_get_drvdata(dsi);
- drm_panel_disable(&boe->base);
- drm_panel_unprepare(&boe->base);
+ drm_panel_helper_shutdown(&boe->base);
}
static void boe_panel_remove(struct mipi_dsi_device *dsi)
diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
index feb665df35a1..fd8987702140 100644
--- a/drivers/gpu/drm/panel/panel-edp.c
+++ b/drivers/gpu/drm/panel/panel-edp.c
@@ -41,6 +41,7 @@
#include <drm/drm_device.h>
#include <drm/drm_edid.h>
#include <drm/drm_panel.h>
+#include <drm/drm_panel_helper.h>
/**
* struct panel_delay - Describes delays for a simple panel.
@@ -207,11 +208,8 @@ struct edp_panel_entry {
struct panel_edp {
struct drm_panel base;
- bool enabled;
bool no_hpd;
- bool prepared;
-
ktime_t prepared_time;
ktime_t unprepared_time;
@@ -361,14 +359,9 @@ static int panel_edp_disable(struct drm_panel *panel)
{
struct panel_edp *p = to_panel_edp(panel);
- if (!p->enabled)
- return 0;
-
if (p->desc->delay.disable)
msleep(p->desc->delay.disable);
- p->enabled = false;
-
return 0;
}
@@ -385,18 +378,12 @@ static int panel_edp_suspend(struct device *dev)
static int panel_edp_unprepare(struct drm_panel *panel)
{
- struct panel_edp *p = to_panel_edp(panel);
int ret;
- /* Unpreparing when already unprepared is a no-op */
- if (!p->prepared)
- return 0;
-
pm_runtime_mark_last_busy(panel->dev);
ret = pm_runtime_put_autosuspend(panel->dev);
if (ret < 0)
return ret;
- p->prepared = false;
return 0;
}
@@ -504,21 +491,14 @@ static int panel_edp_resume(struct device *dev)
static int panel_edp_prepare(struct drm_panel *panel)
{
- struct panel_edp *p = to_panel_edp(panel);
int ret;
- /* Preparing when already prepared is a no-op */
- if (p->prepared)
- return 0;
-
ret = pm_runtime_get_sync(panel->dev);
if (ret < 0) {
pm_runtime_put_autosuspend(panel->dev);
return ret;
}
- p->prepared = true;
-
return 0;
}
@@ -527,9 +507,6 @@ static int panel_edp_enable(struct drm_panel *panel)
struct panel_edp *p = to_panel_edp(panel);
unsigned int delay;
- if (p->enabled)
- return 0;
-
delay = p->desc->delay.enable;
/*
@@ -558,8 +535,6 @@ static int panel_edp_enable(struct drm_panel *panel)
panel_edp_wait(p->prepared_time, p->desc->delay.prepare_to_enable);
- p->enabled = true;
-
return 0;
}
@@ -807,7 +782,6 @@ static int panel_edp_probe(struct device *dev, const struct panel_desc *desc,
if (!panel)
return -ENOMEM;
- panel->enabled = false;
panel->prepared_time = 0;
panel->desc = desc;
panel->aux = aux;
@@ -908,8 +882,7 @@ static void panel_edp_remove(struct device *dev)
struct panel_edp *panel = dev_get_drvdata(dev);
drm_panel_remove(&panel->base);
- drm_panel_disable(&panel->base);
- drm_panel_unprepare(&panel->base);
+ drm_panel_helper_shutdown(&panel->base);
pm_runtime_dont_use_autosuspend(dev);
pm_runtime_disable(dev);
@@ -924,8 +897,7 @@ static void panel_edp_shutdown(struct device *dev)
{
struct panel_edp *panel = dev_get_drvdata(dev);
- drm_panel_disable(&panel->base);
- drm_panel_unprepare(&panel->base);
+ drm_panel_helper_shutdown(&panel->base);
}
static const struct display_timing auo_b101ean01_timing = {
diff --git a/drivers/gpu/drm/panel/panel-elida-kd35t133.c b/drivers/gpu/drm/panel/panel-elida-kd35t133.c
index e7be15b68102..b76dd85d41b4 100644
--- a/drivers/gpu/drm/panel/panel-elida-kd35t133.c
+++ b/drivers/gpu/drm/panel/panel-elida-kd35t133.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>
/* Manufacturer specific Commands send via DSI */
#define KD35T133_CMD_INTERFACEMODECTRL 0xb0
@@ -43,7 +44,6 @@ struct kd35t133 {
struct regulator *vdd;
struct regulator *iovcc;
enum drm_panel_orientation orientation;
- bool prepared;
};
static inline struct kd35t133 *panel_to_kd35t133(struct drm_panel *panel)
@@ -91,9 +91,6 @@ static int kd35t133_unprepare(struct drm_panel *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 < 0)
dev_err(ctx->dev, "failed to set display off: %d\n", ret);
@@ -107,8 +104,6 @@ static int kd35t133_unprepare(struct drm_panel *panel)
regulator_disable(ctx->iovcc);
regulator_disable(ctx->vdd);
- ctx->prepared = false;
-
return 0;
}
@@ -118,9 +113,6 @@ static int kd35t133_prepare(struct drm_panel *panel)
struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
int ret;
- if (ctx->prepared)
- return 0;
-
dev_dbg(ctx->dev, "Resetting the panel\n");
ret = regulator_enable(ctx->vdd);
if (ret < 0) {
@@ -164,8 +156,6 @@ static int kd35t133_prepare(struct drm_panel *panel)
msleep(50);
- ctx->prepared = true;
-
return 0;
disable_iovcc:
@@ -302,15 +292,8 @@ static int kd35t133_probe(struct mipi_dsi_device *dsi)
static void kd35t133_shutdown(struct mipi_dsi_device *dsi)
{
struct kd35t133 *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 kd35t133_remove(struct mipi_dsi_device *dsi)
diff --git a/drivers/gpu/drm/panel/panel-himax-hx8394.c b/drivers/gpu/drm/panel/panel-himax-hx8394.c
index c73243d85de7..3e75d48aae0a 100644
--- a/drivers/gpu/drm/panel/panel-himax-hx8394.c
+++ b/drivers/gpu/drm/panel/panel-himax-hx8394.c
@@ -23,6 +23,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-himax-hx8394"
@@ -68,7 +69,6 @@ struct hx8394 {
struct gpio_desc *reset_gpio;
struct regulator *vcc;
struct regulator *iovcc;
- bool prepared;
const struct hx8394_panel_desc *desc;
};
@@ -262,16 +262,11 @@ static int hx8394_unprepare(struct drm_panel *panel)
{
struct hx8394 *ctx = panel_to_hx8394(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;
}
@@ -280,9 +275,6 @@ static int hx8394_prepare(struct drm_panel *panel)
struct hx8394 *ctx = panel_to_hx8394(panel);
int ret;
- if (ctx->prepared)
- return 0;
-
gpiod_set_value_cansleep(ctx->reset_gpio, 1);
ret = regulator_enable(ctx->vcc);
@@ -301,8 +293,6 @@ static int hx8394_prepare(struct drm_panel *panel)
msleep(180);
- ctx->prepared = true;
-
return 0;
disable_vcc:
@@ -404,15 +394,8 @@ static int hx8394_probe(struct mipi_dsi_device *dsi)
static void hx8394_shutdown(struct mipi_dsi_device *dsi)
{
struct hx8394 *ctx = mipi_dsi_get_drvdata(dsi);
- int ret;
-
- ret = drm_panel_disable(&ctx->panel);
- if (ret < 0)
- dev_err(&dsi->dev, "Failed to disable panel: %d\n", ret);
- ret = drm_panel_unprepare(&ctx->panel);
- if (ret < 0)
- dev_err(&dsi->dev, "Failed to unprepare panel: %d\n", ret);
+ drm_panel_helper_shutdown(&ctx->panel);
}
static void hx8394_remove(struct mipi_dsi_device *dsi)
diff --git a/drivers/gpu/drm/panel/panel-innolux-p079zca.c b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
index 485178a99910..cc2a05a38972 100644
--- a/drivers/gpu/drm/panel/panel-innolux-p079zca.c
+++ b/drivers/gpu/drm/panel/panel-innolux-p079zca.c
@@ -16,6 +16,7 @@
#include <drm/drm_mipi_dsi.h>
#include <drm/drm_modes.h>
#include <drm/drm_panel.h>
+#include <drm/drm_panel_helper.h>
struct panel_init_cmd {
size_t len;
@@ -51,9 +52,6 @@ struct innolux_panel {
struct regulator_bulk_data *supplies;
struct gpio_desc *enable_gpio;
-
- bool prepared;
- bool enabled;
};
static inline struct innolux_panel *to_innolux_panel(struct drm_panel *panel)
@@ -61,26 +59,11 @@ static inline struct innolux_panel *to_innolux_panel(struct drm_panel *panel)
return container_of(panel, struct innolux_panel, base);
}
-static int innolux_panel_disable(struct drm_panel *panel)
-{
- struct innolux_panel *innolux = to_innolux_panel(panel);
-
- if (!innolux->enabled)
- return 0;
-
- innolux->enabled = false;
-
- return 0;
-}
-
static int innolux_panel_unprepare(struct drm_panel *panel)
{
struct innolux_panel *innolux = to_innolux_panel(panel);
int err;
- if (!innolux->prepared)
- return 0;
-
err = mipi_dsi_dcs_set_display_off(innolux->link);
if (err < 0)
dev_err(panel->dev, "failed to set display off: %d\n", err);
@@ -104,8 +87,6 @@ static int innolux_panel_unprepare(struct drm_panel *panel)
if (err < 0)
return err;
- innolux->prepared = false;
-
return 0;
}
@@ -114,9 +95,6 @@ static int innolux_panel_prepare(struct drm_panel *panel)
struct innolux_panel *innolux = to_innolux_panel(panel);
int err;
- if (innolux->prepared)
- return 0;
-
gpiod_set_value_cansleep(innolux->enable_gpio, 0);
err = regulator_bulk_enable(innolux->desc->num_supplies,
@@ -178,8 +156,6 @@ static int innolux_panel_prepare(struct drm_panel *panel)
/* T7: 5ms */
usleep_range(5000, 6000);
- innolux->prepared = true;
-
return 0;
poweroff:
@@ -189,18 +165,6 @@ static int innolux_panel_prepare(struct drm_panel *panel)
return err;
}
-static int innolux_panel_enable(struct drm_panel *panel)
-{
- struct innolux_panel *innolux = to_innolux_panel(panel);
-
- if (innolux->enabled)
- return 0;
-
- innolux->enabled = true;
-
- return 0;
-}
-
static const char * const innolux_p079zca_supply_names[] = {
"power",
};
@@ -407,10 +371,8 @@ static int innolux_panel_get_modes(struct drm_panel *panel,
}
static const struct drm_panel_funcs innolux_panel_funcs = {
- .disable = innolux_panel_disable,
.unprepare = innolux_panel_unprepare,
.prepare = innolux_panel_prepare,
- .enable = innolux_panel_enable,
.get_modes = innolux_panel_get_modes,
};
@@ -510,13 +472,7 @@ static void innolux_panel_remove(struct mipi_dsi_device *dsi)
struct innolux_panel *innolux = mipi_dsi_get_drvdata(dsi);
int err;
- err = drm_panel_unprepare(&innolux->base);
- if (err < 0)
- dev_err(&dsi->dev, "failed to unprepare panel: %d\n", err);
-
- err = drm_panel_disable(&innolux->base);
- if (err < 0)
- dev_err(&dsi->dev, "failed to disable panel: %d\n", err);
+ drm_panel_helper_shutdown(&innolux->base);
err = mipi_dsi_detach(dsi);
if (err < 0)
@@ -529,8 +485,7 @@ static void innolux_panel_shutdown(struct mipi_dsi_device *dsi)
{
struct innolux_panel *innolux = mipi_dsi_get_drvdata(dsi);
- drm_panel_unprepare(&innolux->base);
- drm_panel_disable(&innolux->base);
+ drm_panel_helper_shutdown(&innolux->base);
}
static struct mipi_dsi_driver innolux_panel_driver = {
diff --git a/drivers/gpu/drm/panel/panel-khadas-ts050.c b/drivers/gpu/drm/panel/panel-khadas-ts050.c
index b942a0162274..ed46b1d19e7c 100644
--- a/drivers/gpu/drm/panel/panel-khadas-ts050.c
+++ b/drivers/gpu/drm/panel/panel-khadas-ts050.c
@@ -17,6 +17,7 @@
#include <drm/drm_mipi_dsi.h>
#include <drm/drm_modes.h>
#include <drm/drm_panel.h>
+#include <drm/drm_panel_helper.h>
struct khadas_ts050_panel {
struct drm_panel base;
@@ -25,9 +26,6 @@ struct khadas_ts050_panel {
struct regulator *supply;
struct gpio_desc *reset_gpio;
struct gpio_desc *enable_gpio;
-
- bool prepared;
- bool enabled;
};
struct khadas_ts050_panel_cmd {
@@ -584,9 +582,6 @@ static int khadas_ts050_panel_prepare(struct drm_panel *panel)
unsigned int i;
int err;
- if (khadas_ts050->prepared)
- return 0;
-
gpiod_set_value_cansleep(khadas_ts050->enable_gpio, 0);
err = regulator_enable(khadas_ts050->supply);
@@ -649,8 +644,6 @@ static int khadas_ts050_panel_prepare(struct drm_panel *panel)
usleep_range(10000, 11000);
- khadas_ts050->prepared = true;
-
return 0;
poweroff:
@@ -667,11 +660,6 @@ static int khadas_ts050_panel_unprepare(struct drm_panel *panel)
struct khadas_ts050_panel *khadas_ts050 = to_khadas_ts050_panel(panel);
int err;
- if (!khadas_ts050->prepared)
- return 0;
-
- khadas_ts050->prepared = false;
-
err = mipi_dsi_dcs_enter_sleep_mode(khadas_ts050->link);
if (err < 0)
dev_err(panel->dev, "failed to enter sleep mode: %d\n", err);
@@ -688,31 +676,17 @@ static int khadas_ts050_panel_unprepare(struct drm_panel *panel)
return 0;
}
-static int khadas_ts050_panel_enable(struct drm_panel *panel)
-{
- struct khadas_ts050_panel *khadas_ts050 = to_khadas_ts050_panel(panel);
-
- khadas_ts050->enabled = true;
-
- return 0;
-}
-
static int khadas_ts050_panel_disable(struct drm_panel *panel)
{
struct khadas_ts050_panel *khadas_ts050 = to_khadas_ts050_panel(panel);
int err;
- if (!khadas_ts050->enabled)
- return 0;
-
err = mipi_dsi_dcs_set_display_off(khadas_ts050->link);
if (err < 0)
dev_err(panel->dev, "failed to set display off: %d\n", err);
usleep_range(10000, 11000);
- khadas_ts050->enabled = false;
-
return 0;
}
@@ -756,7 +730,6 @@ static int khadas_ts050_panel_get_modes(struct drm_panel *panel,
static const struct drm_panel_funcs khadas_ts050_panel_funcs = {
.prepare = khadas_ts050_panel_prepare,
.unprepare = khadas_ts050_panel_unprepare,
- .enable = khadas_ts050_panel_enable,
.disable = khadas_ts050_panel_disable,
.get_modes = khadas_ts050_panel_get_modes,
};
@@ -840,16 +813,14 @@ static void khadas_ts050_panel_remove(struct mipi_dsi_device *dsi)
dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", err);
drm_panel_remove(&khadas_ts050->base);
- drm_panel_disable(&khadas_ts050->base);
- drm_panel_unprepare(&khadas_ts050->base);
+ drm_panel_helper_shutdown(&khadas_ts050->base);
}
static void khadas_ts050_panel_shutdown(struct mipi_dsi_device *dsi)
{
struct khadas_ts050_panel *khadas_ts050 = mipi_dsi_get_drvdata(dsi);
- drm_panel_disable(&khadas_ts050->base);
- drm_panel_unprepare(&khadas_ts050->base);
+ drm_panel_helper_shutdown(&khadas_ts050->base);
}
static struct mipi_dsi_driver khadas_ts050_panel_driver = {
diff --git a/drivers/gpu/drm/panel/panel-kingdisplay-kd097d04.c b/drivers/gpu/drm/panel/panel-kingdisplay-kd097d04.c
index 17f8d80cf2b3..b86f24875985 100644
--- a/drivers/gpu/drm/panel/panel-kingdisplay-kd097d04.c
+++ b/drivers/gpu/drm/panel/panel-kingdisplay-kd097d04.c
@@ -16,6 +16,7 @@
#include <drm/drm_mipi_dsi.h>
#include <drm/drm_modes.h>
#include <drm/drm_panel.h>
+#include <drm/drm_panel_helper.h>
struct kingdisplay_panel {
struct drm_panel base;
@@ -23,9 +24,6 @@ struct kingdisplay_panel {
struct regulator *supply;
struct gpio_desc *enable_gpio;
-
- bool prepared;
- bool enabled;
};
struct kingdisplay_panel_cmd {
@@ -185,15 +183,10 @@ static int kingdisplay_panel_disable(struct drm_panel *panel)
struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel);
int err;
- if (!kingdisplay->enabled)
- return 0;
-
err = mipi_dsi_dcs_set_display_off(kingdisplay->link);
if (err < 0)
dev_err(panel->dev, "failed to set display off: %d\n", err);
- kingdisplay->enabled = false;
-
return 0;
}
@@ -202,9 +195,6 @@ static int kingdisplay_panel_unprepare(struct drm_panel *panel)
struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel);
int err;
- if (!kingdisplay->prepared)
- return 0;
-
err = mipi_dsi_dcs_enter_sleep_mode(kingdisplay->link);
if (err < 0) {
dev_err(panel->dev, "failed to enter sleep mode: %d\n", err);
@@ -220,8 +210,6 @@ static int kingdisplay_panel_unprepare(struct drm_panel *panel)
if (err < 0)
return err;
- kingdisplay->prepared = false;
-
return 0;
}
@@ -231,9 +219,6 @@ static int kingdisplay_panel_prepare(struct drm_panel *panel)
int err, regulator_err;
unsigned int i;
- if (kingdisplay->prepared)
- return 0;
-
gpiod_set_value_cansleep(kingdisplay->enable_gpio, 0);
err = regulator_enable(kingdisplay->supply);
@@ -275,8 +260,6 @@ static int kingdisplay_panel_prepare(struct drm_panel *panel)
/* T7: 10ms */
usleep_range(10000, 11000);
- kingdisplay->prepared = true;
-
return 0;
poweroff:
@@ -289,18 +272,6 @@ static int kingdisplay_panel_prepare(struct drm_panel *panel)
return err;
}
-static int kingdisplay_panel_enable(struct drm_panel *panel)
-{
- struct kingdisplay_panel *kingdisplay = to_kingdisplay_panel(panel);
-
- if (kingdisplay->enabled)
- return 0;
-
- kingdisplay->enabled = true;
-
- return 0;
-}
-
static const struct drm_display_mode default_mode = {
.clock = 229000,
.hdisplay = 1536,
@@ -341,7 +312,6 @@ static const struct drm_panel_funcs kingdisplay_panel_funcs = {
.disable = kingdisplay_panel_disable,
.unprepare = kingdisplay_panel_unprepare,
.prepare = kingdisplay_panel_prepare,
- .enable = kingdisplay_panel_enable,
.get_modes = kingdisplay_panel_get_modes,
};
@@ -420,13 +390,7 @@ static void kingdisplay_panel_remove(struct mipi_dsi_device *dsi)
struct kingdisplay_panel *kingdisplay = mipi_dsi_get_drvdata(dsi);
int err;
- err = drm_panel_unprepare(&kingdisplay->base);
- if (err < 0)
- dev_err(&dsi->dev, "failed to unprepare panel: %d\n", err);
-
- err = drm_panel_disable(&kingdisplay->base);
- if (err < 0)
- dev_err(&dsi->dev, "failed to disable panel: %d\n", err);
+ drm_panel_helper_shutdown(&kingdisplay->base);
err = mipi_dsi_detach(dsi);
if (err < 0)
@@ -439,8 +403,7 @@ static void kingdisplay_panel_shutdown(struct mipi_dsi_device *dsi)
{
struct kingdisplay_panel *kingdisplay = mipi_dsi_get_drvdata(dsi);
- drm_panel_unprepare(&kingdisplay->base);
- drm_panel_disable(&kingdisplay->base);
+ drm_panel_helper_shutdown(&kingdisplay->base);
}
static struct mipi_dsi_driver kingdisplay_panel_driver = {
diff --git a/drivers/gpu/drm/panel/panel-leadtek-ltk050h3146w.c b/drivers/gpu/drm/panel/panel-leadtek-ltk050h3146w.c
index d41482d3a34f..ae07d23e26f9 100644
--- a/drivers/gpu/drm/panel/panel-leadtek-ltk050h3146w.c
+++ b/drivers/gpu/drm/panel/panel-leadtek-ltk050h3146w.c
@@ -16,6 +16,7 @@
#include <drm/drm_mipi_dsi.h>
#include <drm/drm_modes.h>
#include <drm/drm_panel.h>
+#include <drm/drm_panel_helper.h>
struct ltk050h3146w_cmd {
char cmd;
@@ -35,7 +36,6 @@ struct ltk050h3146w {
struct regulator *vci;
struct regulator *iovcc;
const struct ltk050h3146w_desc *panel_desc;
- bool prepared;
};
static const struct ltk050h3146w_cmd page1_cmds[] = {
@@ -432,9 +432,6 @@ static int ltk050h3146w_unprepare(struct drm_panel *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 < 0) {
dev_err(ctx->dev, "failed to set display off: %d\n", ret);
@@ -450,8 +447,6 @@ static int ltk050h3146w_unprepare(struct drm_panel *panel)
regulator_disable(ctx->iovcc);
regulator_disable(ctx->vci);
- ctx->prepared = false;
-
return 0;
}
@@ -461,9 +456,6 @@ static int ltk050h3146w_prepare(struct drm_panel *panel)
struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
int ret;
- if (ctx->prepared)
- return 0;
-
dev_dbg(ctx->dev, "Resetting the panel\n");
ret = regulator_enable(ctx->vci);
if (ret < 0) {
@@ -504,8 +496,6 @@ static int ltk050h3146w_prepare(struct drm_panel *panel)
msleep(50);
- ctx->prepared = true;
-
return 0;
disable_iovcc:
@@ -608,15 +598,8 @@ static int ltk050h3146w_probe(struct mipi_dsi_device *dsi)
static void ltk050h3146w_shutdown(struct mipi_dsi_device *dsi)
{
struct ltk050h3146w *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 ltk050h3146w_remove(struct mipi_dsi_device *dsi)
diff --git a/drivers/gpu/drm/panel/panel-leadtek-ltk500hd1829.c b/drivers/gpu/drm/panel/panel-leadtek-ltk500hd1829.c
index 39e408c9f762..8f26f0b51a06 100644
--- a/drivers/gpu/drm/panel/panel-leadtek-ltk500hd1829.c
+++ b/drivers/gpu/drm/panel/panel-leadtek-ltk500hd1829.c
@@ -20,6 +20,7 @@
#include <drm/drm_mipi_dsi.h>
#include <drm/drm_modes.h>
#include <drm/drm_panel.h>
+#include <drm/drm_panel_helper.h>
struct ltk500hd1829 {
struct device *dev;
@@ -27,7 +28,6 @@ struct ltk500hd1829 {
struct gpio_desc *reset_gpio;
struct regulator *vcc;
struct regulator *iovcc;
- bool prepared;
};
struct ltk500hd1829_cmd {
@@ -272,9 +272,6 @@ static int ltk500hd1829_unprepare(struct drm_panel *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 < 0)
dev_err(panel->dev, "failed to set display off: %d\n", ret);
@@ -290,8 +287,6 @@ static int ltk500hd1829_unprepare(struct drm_panel *panel)
regulator_disable(ctx->iovcc);
regulator_disable(ctx->vcc);
- ctx->prepared = false;
-
return 0;
}
@@ -302,9 +297,6 @@ static int ltk500hd1829_prepare(struct drm_panel *panel)
unsigned int i;
int ret;
- if (ctx->prepared)
- return 0;
-
ret = regulator_enable(ctx->vcc);
if (ret < 0) {
dev_err(ctx->dev, "Failed to enable vci supply: %d\n", ret);
@@ -348,8 +340,6 @@ static int ltk500hd1829_prepare(struct drm_panel *panel)
goto disable_iovcc;
}
- ctx->prepared = true;
-
return 0;
disable_iovcc:
@@ -466,15 +456,8 @@ static int ltk500hd1829_probe(struct mipi_dsi_device *dsi)
static void ltk500hd1829_shutdown(struct mipi_dsi_device *dsi)
{
struct ltk500hd1829 *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 ltk500hd1829_remove(struct mipi_dsi_device *dsi)
diff --git a/drivers/gpu/drm/panel/panel-novatek-nt36672a.c b/drivers/gpu/drm/panel/panel-novatek-nt36672a.c
index 33fb3d715e54..8447cc9887d2 100644
--- a/drivers/gpu/drm/panel/panel-novatek-nt36672a.c
+++ b/drivers/gpu/drm/panel/panel-novatek-nt36672a.c
@@ -25,6 +25,7 @@
#include <drm/drm_mipi_dsi.h>
#include <drm/drm_modes.h>
#include <drm/drm_panel.h>
+#include <drm/drm_panel_helper.h>
#include <video/mipi_display.h>
@@ -72,8 +73,6 @@ struct nt36672a_panel {
struct regulator_bulk_data supplies[ARRAY_SIZE(nt36672a_regulator_names)];
struct gpio_desc *reset_gpio;
-
- bool prepared;
};
static inline struct nt36672a_panel *to_nt36672a_panel(struct drm_panel *panel)
@@ -119,9 +118,6 @@ static int nt36672a_panel_unprepare(struct drm_panel *panel)
struct nt36672a_panel *pinfo = to_nt36672a_panel(panel);
int ret;
- if (!pinfo->prepared)
- return 0;
-
/* send off cmds */
ret = nt36672a_send_cmds(panel, pinfo->desc->off_cmds,
pinfo->desc->num_off_cmds);
@@ -147,8 +143,6 @@ static int nt36672a_panel_unprepare(struct drm_panel *panel)
if (ret < 0)
dev_err(panel->dev, "power_off failed ret = %d\n", ret);
- pinfo->prepared = false;
-
return ret;
}
@@ -179,9 +173,6 @@ static int nt36672a_panel_prepare(struct drm_panel *panel)
struct nt36672a_panel *pinfo = to_nt36672a_panel(panel);
int err;
- if (pinfo->prepared)
- return 0;
-
err = nt36672a_panel_power_on(pinfo);
if (err < 0)
goto poweroff;
@@ -221,8 +212,6 @@ static int nt36672a_panel_prepare(struct drm_panel *panel)
msleep(120);
- pinfo->prepared = true;
-
return 0;
poweroff:
@@ -673,13 +662,7 @@ static void nt36672a_panel_remove(struct mipi_dsi_device *dsi)
struct nt36672a_panel *pinfo = mipi_dsi_get_drvdata(dsi);
int err;
- err = drm_panel_unprepare(&pinfo->base);
- if (err < 0)
- dev_err(&dsi->dev, "failed to unprepare panel: %d\n", err);
-
- err = drm_panel_disable(&pinfo->base);
- if (err < 0)
- dev_err(&dsi->dev, "failed to disable panel: %d\n", err);
+ drm_panel_helper_shutdown(&pinfo->base);
err = mipi_dsi_detach(dsi);
if (err < 0)
@@ -692,8 +675,7 @@ static void nt36672a_panel_shutdown(struct mipi_dsi_device *dsi)
{
struct nt36672a_panel *pinfo = mipi_dsi_get_drvdata(dsi);
- drm_panel_disable(&pinfo->base);
- drm_panel_unprepare(&pinfo->base);
+ drm_panel_helper_shutdown(&pinfo->base);
}
static const struct of_device_id tianma_fhd_video_of_match[] = {
diff --git a/drivers/gpu/drm/panel/panel-olimex-lcd-olinuxino.c b/drivers/gpu/drm/panel/panel-olimex-lcd-olinuxino.c
index 4819ada69482..ee89f0c99986 100644
--- a/drivers/gpu/drm/panel/panel-olimex-lcd-olinuxino.c
+++ b/drivers/gpu/drm/panel/panel-olimex-lcd-olinuxino.c
@@ -20,6 +20,7 @@
#include <drm/drm_device.h>
#include <drm/drm_modes.h>
#include <drm/drm_panel.h>
+#include <drm/drm_panel_helper.h>
#define LCD_OLINUXINO_HEADER_MAGIC 0x4F4CB727
#define LCD_OLINUXINO_DATA_LEN 256
@@ -64,9 +65,6 @@ struct lcd_olinuxino {
struct i2c_client *client;
struct mutex mutex;
- bool prepared;
- bool enabled;
-
struct regulator *supply;
struct gpio_desc *enable_gpio;
@@ -78,30 +76,13 @@ static inline struct lcd_olinuxino *to_lcd_olinuxino(struct drm_panel *panel)
return container_of(panel, struct lcd_olinuxino, panel);
}
-static int lcd_olinuxino_disable(struct drm_panel *panel)
-{
- struct lcd_olinuxino *lcd = to_lcd_olinuxino(panel);
-
- if (!lcd->enabled)
- return 0;
-
- lcd->enabled = false;
-
- return 0;
-}
-
static int lcd_olinuxino_unprepare(struct drm_panel *panel)
{
struct lcd_olinuxino *lcd = to_lcd_olinuxino(panel);
- if (!lcd->prepared)
- return 0;
-
gpiod_set_value_cansleep(lcd->enable_gpio, 0);
regulator_disable(lcd->supply);
- lcd->prepared = false;
-
return 0;
}
@@ -110,27 +91,11 @@ static int lcd_olinuxino_prepare(struct drm_panel *panel)
struct lcd_olinuxino *lcd = to_lcd_olinuxino(panel);
int ret;
- if (lcd->prepared)
- return 0;
-
ret = regulator_enable(lcd->supply);
if (ret < 0)
return ret;
gpiod_set_value_cansleep(lcd->enable_gpio, 1);
- lcd->prepared = true;
-
- return 0;
-}
-
-static int lcd_olinuxino_enable(struct drm_panel *panel)
-{
- struct lcd_olinuxino *lcd = to_lcd_olinuxino(panel);
-
- if (lcd->enabled)
- return 0;
-
- lcd->enabled = true;
return 0;
}
@@ -195,10 +160,8 @@ static int lcd_olinuxino_get_modes(struct drm_panel *panel,
}
static const struct drm_panel_funcs lcd_olinuxino_funcs = {
- .disable = lcd_olinuxino_disable,
.unprepare = lcd_olinuxino_unprepare,
.prepare = lcd_olinuxino_prepare,
- .enable = lcd_olinuxino_enable,
.get_modes = lcd_olinuxino_get_modes,
};
@@ -264,9 +227,6 @@ static int lcd_olinuxino_probe(struct i2c_client *client)
lcd->eeprom.num_modes = 4;
}
- lcd->enabled = false;
- lcd->prepared = false;
-
lcd->supply = devm_regulator_get(dev, "power");
if (IS_ERR(lcd->supply))
return PTR_ERR(lcd->supply);
@@ -293,8 +253,7 @@ static void lcd_olinuxino_remove(struct i2c_client *client)
drm_panel_remove(&panel->panel);
- drm_panel_disable(&panel->panel);
- drm_panel_unprepare(&panel->panel);
+ drm_panel_helper_shutdown(&panel->panel);
}
static const struct of_device_id lcd_olinuxino_of_ids[] = {
diff --git a/drivers/gpu/drm/panel/panel-osd-osd101t2587-53ts.c b/drivers/gpu/drm/panel/panel-osd-osd101t2587-53ts.c
index 493e0504f6f7..b299e999d5b5 100644
--- a/drivers/gpu/drm/panel/panel-osd-osd101t2587-53ts.c
+++ b/drivers/gpu/drm/panel/panel-osd-osd101t2587-53ts.c
@@ -12,6 +12,7 @@
#include <drm/drm_device.h>
#include <drm/drm_mipi_dsi.h>
#include <drm/drm_panel.h>
+#include <drm/drm_panel_helper.h>
#include <video/mipi_display.h>
@@ -21,9 +22,6 @@ struct osd101t2587_panel {
struct regulator *supply;
- bool prepared;
- bool enabled;
-
const struct drm_display_mode *default_mode;
};
@@ -37,13 +35,8 @@ static int osd101t2587_panel_disable(struct drm_panel *panel)
struct osd101t2587_panel *osd101t2587 = ti_osd_panel(panel);
int ret;
- if (!osd101t2587->enabled)
- return 0;
-
ret = mipi_dsi_shutdown_peripheral(osd101t2587->dsi);
- osd101t2587->enabled = false;
-
return ret;
}
@@ -51,11 +44,7 @@ static int osd101t2587_panel_unprepare(struct drm_panel *panel)
{
struct osd101t2587_panel *osd101t2587 = ti_osd_panel(panel);
- if (!osd101t2587->prepared)
- return 0;
-
regulator_disable(osd101t2587->supply);
- osd101t2587->prepared = false;
return 0;
}
@@ -63,16 +52,8 @@ static int osd101t2587_panel_unprepare(struct drm_panel *panel)
static int osd101t2587_panel_prepare(struct drm_panel *panel)
{
struct osd101t2587_panel *osd101t2587 = ti_osd_panel(panel);
- int ret;
-
- if (osd101t2587->prepared)
- return 0;
- ret = regulator_enable(osd101t2587->supply);
- if (!ret)
- osd101t2587->prepared = true;
-
- return ret;
+ return regulator_enable(osd101t2587->supply);
}
static int osd101t2587_panel_enable(struct drm_panel *panel)
@@ -80,15 +61,10 @@ static int osd101t2587_panel_enable(struct drm_panel *panel)
struct osd101t2587_panel *osd101t2587 = ti_osd_panel(panel);
int ret;
- if (osd101t2587->enabled)
- return 0;
-
ret = mipi_dsi_turn_on_peripheral(osd101t2587->dsi);
if (ret)
return ret;
- osd101t2587->enabled = true;
-
return ret;
}
@@ -211,11 +187,7 @@ static void osd101t2587_panel_remove(struct mipi_dsi_device *dsi)
struct osd101t2587_panel *osd101t2587 = mipi_dsi_get_drvdata(dsi);
int ret;
- ret = drm_panel_disable(&osd101t2587->base);
- if (ret < 0)
- dev_warn(&dsi->dev, "failed to disable panel: %d\n", ret);
-
- drm_panel_unprepare(&osd101t2587->base);
+ drm_panel_helper_shutdown(&osd101t2587->base);
drm_panel_remove(&osd101t2587->base);
ret = mipi_dsi_detach(dsi);
@@ -227,8 +199,7 @@ static void osd101t2587_panel_shutdown(struct mipi_dsi_device *dsi)
{
struct osd101t2587_panel *osd101t2587 = mipi_dsi_get_drvdata(dsi);
- drm_panel_disable(&osd101t2587->base);
- drm_panel_unprepare(&osd101t2587->base);
+ drm_panel_helper_shutdown(&osd101t2587->base);
}
static struct mipi_dsi_driver osd101t2587_panel_driver = {
diff --git a/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c b/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
index 5703f4712d96..e46eb8a0b1e9 100644
--- a/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
+++ b/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
@@ -18,6 +18,7 @@
#include <drm/display/drm_dp_helper.h>
#include <drm/drm_edid.h>
#include <drm/drm_panel.h>
+#include <drm/drm_panel_helper.h>
/* T3 VCC to HPD high is max 200 ms */
#define HPD_MAX_MS 200
@@ -25,8 +26,6 @@
struct atana33xc20_panel {
struct drm_panel base;
- bool prepared;
- bool enabled;
bool el3_was_on;
bool no_hpd;
@@ -137,13 +136,8 @@ static int atana33xc20_disable(struct drm_panel *panel)
{
struct atana33xc20_panel *p = to_atana33xc20(panel);
- /* Disabling when already disabled is a no-op */
- if (!p->enabled)
- return 0;
-
gpiod_set_value_cansleep(p->el_on3_gpio, 0);
p->el_on3_off_time = ktime_get_boottime();
- p->enabled = false;
/*
* Keep track of the fact that EL_ON3 was on but we haven't power
@@ -167,10 +161,6 @@ static int atana33xc20_enable(struct drm_panel *panel)
{
struct atana33xc20_panel *p = to_atana33xc20(panel);
- /* Enabling when already enabled is a no-op */
- if (p->enabled)
- return 0;
-
/*
* Once EL_ON3 drops we absolutely need a power cycle before the next
* enable or the backlight will never come on again. The code ensures
@@ -189,20 +179,14 @@ static int atana33xc20_enable(struct drm_panel *panel)
atana33xc20_wait(p->powered_on_time, 400);
gpiod_set_value_cansleep(p->el_on3_gpio, 1);
- p->enabled = true;
return 0;
}
static int atana33xc20_unprepare(struct drm_panel *panel)
{
- struct atana33xc20_panel *p = to_atana33xc20(panel);
int ret;
- /* Unpreparing when already unprepared is a no-op */
- if (!p->prepared)
- return 0;
-
/*
* Purposely do a put_sync, don't use autosuspend. The panel's tcon
* seems to sometimes crash when you stop giving it data and this is
@@ -214,26 +198,19 @@ static int atana33xc20_unprepare(struct drm_panel *panel)
ret = pm_runtime_put_sync_suspend(panel->dev);
if (ret < 0)
return ret;
- p->prepared = false;
return 0;
}
static int atana33xc20_prepare(struct drm_panel *panel)
{
- struct atana33xc20_panel *p = to_atana33xc20(panel);
int ret;
- /* Preparing when already prepared is a no-op */
- if (p->prepared)
- return 0;
-
ret = pm_runtime_get_sync(panel->dev);
if (ret < 0) {
pm_runtime_put_autosuspend(panel->dev);
return ret;
}
- p->prepared = true;
return 0;
}
@@ -337,8 +314,7 @@ static void atana33xc20_remove(struct dp_aux_ep_device *aux_ep)
struct atana33xc20_panel *panel = dev_get_drvdata(dev);
drm_panel_remove(&panel->base);
- drm_panel_disable(&panel->base);
- drm_panel_unprepare(&panel->base);
+ drm_panel_helper_shutdown(&panel->base);
kfree(panel->edid);
}
@@ -348,8 +324,7 @@ static void atana33xc20_shutdown(struct dp_aux_ep_device *aux_ep)
struct device *dev = &aux_ep->dev;
struct atana33xc20_panel *panel = dev_get_drvdata(dev);
- drm_panel_disable(&panel->base);
- drm_panel_unprepare(&panel->base);
+ drm_panel_helper_shutdown(&panel->base);
}
static const struct of_device_id atana33xc20_dt_match[] = {
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 72cef64441a6..6ccf0f6727b7 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -40,6 +40,7 @@
#include <drm/drm_edid.h>
#include <drm/drm_mipi_dsi.h>
#include <drm/drm_panel.h>
+#include <drm/drm_panel_helper.h>
/**
* struct panel_desc - Describes a simple panel.
@@ -137,9 +138,6 @@ struct panel_desc {
struct panel_simple {
struct drm_panel base;
- bool enabled;
-
- bool prepared;
ktime_t unprepared_time;
@@ -289,14 +287,9 @@ static int panel_simple_disable(struct drm_panel *panel)
{
struct panel_simple *p = to_panel_simple(panel);
- if (!p->enabled)
- return 0;
-
if (p->desc->delay.disable)
msleep(p->desc->delay.disable);
- p->enabled = false;
-
return 0;
}
@@ -316,18 +309,12 @@ static int panel_simple_suspend(struct device *dev)
static int panel_simple_unprepare(struct drm_panel *panel)
{
- struct panel_simple *p = to_panel_simple(panel);
int ret;
- /* Unpreparing when already unprepared is a no-op */
- if (!p->prepared)
- return 0;
-
pm_runtime_mark_last_busy(panel->dev);
ret = pm_runtime_put_autosuspend(panel->dev);
if (ret < 0)
return ret;
- p->prepared = false;
return 0;
}
@@ -355,21 +342,14 @@ static int panel_simple_resume(struct device *dev)
static int panel_simple_prepare(struct drm_panel *panel)
{
- struct panel_simple *p = to_panel_simple(panel);
int ret;
- /* Preparing when already prepared is a no-op */
- if (p->prepared)
- return 0;
-
ret = pm_runtime_get_sync(panel->dev);
if (ret < 0) {
pm_runtime_put_autosuspend(panel->dev);
return ret;
}
- p->prepared = true;
-
return 0;
}
@@ -377,14 +357,9 @@ static int panel_simple_enable(struct drm_panel *panel)
{
struct panel_simple *p = to_panel_simple(panel);
- if (p->enabled)
- return 0;
-
if (p->desc->delay.enable)
msleep(p->desc->delay.enable);
- p->enabled = true;
-
return 0;
}
@@ -562,7 +537,6 @@ static int panel_simple_probe(struct device *dev, const struct panel_desc *desc)
if (!panel)
return -ENOMEM;
- panel->enabled = false;
panel->desc = desc;
panel->supply = devm_regulator_get(dev, "power");
@@ -694,8 +668,7 @@ static void panel_simple_remove(struct device *dev)
struct panel_simple *panel = dev_get_drvdata(dev);
drm_panel_remove(&panel->base);
- drm_panel_disable(&panel->base);
- drm_panel_unprepare(&panel->base);
+ drm_panel_helper_shutdown(&panel->base);
pm_runtime_dont_use_autosuspend(dev);
pm_runtime_disable(dev);
@@ -707,8 +680,7 @@ static void panel_simple_shutdown(struct device *dev)
{
struct panel_simple *panel = dev_get_drvdata(dev);
- drm_panel_disable(&panel->base);
- drm_panel_unprepare(&panel->base);
+ drm_panel_helper_shutdown(&panel->base);
}
static const struct drm_display_mode ampire_am_1280800n3tzqw_t00h_mode = {
diff --git a/drivers/gpu/drm/panel/panel-tdo-tl070wsh30.c b/drivers/gpu/drm/panel/panel-tdo-tl070wsh30.c
index d8487bc6d611..07d5917f1549 100644
--- a/drivers/gpu/drm/panel/panel-tdo-tl070wsh30.c
+++ b/drivers/gpu/drm/panel/panel-tdo-tl070wsh30.c
@@ -17,6 +17,7 @@
#include <drm/drm_mipi_dsi.h>
#include <drm/drm_modes.h>
#include <drm/drm_panel.h>
+#include <drm/drm_panel_helper.h>
struct tdo_tl070wsh30_panel {
struct drm_panel base;
@@ -24,8 +25,6 @@ struct tdo_tl070wsh30_panel {
struct regulator *supply;
struct gpio_desc *reset_gpio;
-
- bool prepared;
};
static inline
@@ -39,9 +38,6 @@ static int tdo_tl070wsh30_panel_prepare(struct drm_panel *panel)
struct tdo_tl070wsh30_panel *tdo_tl070wsh30 = to_tdo_tl070wsh30_panel(panel);
int err;
- if (tdo_tl070wsh30->prepared)
- return 0;
-
err = regulator_enable(tdo_tl070wsh30->supply);
if (err < 0)
return err;
@@ -74,8 +70,6 @@ static int tdo_tl070wsh30_panel_prepare(struct drm_panel *panel)
msleep(20);
- tdo_tl070wsh30->prepared = true;
-
return 0;
}
@@ -84,9 +78,6 @@ static int tdo_tl070wsh30_panel_unprepare(struct drm_panel *panel)
struct tdo_tl070wsh30_panel *tdo_tl070wsh30 = to_tdo_tl070wsh30_panel(panel);
int err;
- if (!tdo_tl070wsh30->prepared)
- return 0;
-
err = mipi_dsi_dcs_set_display_off(tdo_tl070wsh30->link);
if (err < 0)
dev_err(panel->dev, "failed to set display off: %d\n", err);
@@ -103,8 +94,6 @@ static int tdo_tl070wsh30_panel_unprepare(struct drm_panel *panel)
regulator_disable(tdo_tl070wsh30->supply);
- tdo_tl070wsh30->prepared = false;
-
return 0;
}
@@ -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);
}
static struct mipi_dsi_driver tdo_tl070wsh30_panel_driver = {
diff --git a/drivers/gpu/drm/panel/panel-xinpeng-xpp055c272.c b/drivers/gpu/drm/panel/panel-xinpeng-xpp055c272.c
index 8670386498a4..27a66694be06 100644
--- a/drivers/gpu/drm/panel/panel-xinpeng-xpp055c272.c
+++ b/drivers/gpu/drm/panel/panel-xinpeng-xpp055c272.c
@@ -12,6 +12,7 @@
#include <drm/drm_mipi_dsi.h>
#include <drm/drm_modes.h>
#include <drm/drm_panel.h>
+#include <drm/drm_panel_helper.h>
#include <video/display_timing.h>
#include <video/mipi_display.h>
@@ -52,7 +53,6 @@ struct xpp055c272 {
struct gpio_desc *reset_gpio;
struct regulator *vci;
struct regulator *iovcc;
- bool prepared;
};
static inline struct xpp055c272 *panel_to_xpp055c272(struct drm_panel *panel)
@@ -136,9 +136,6 @@ static int xpp055c272_unprepare(struct drm_panel *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 < 0)
dev_err(ctx->dev, "failed to set display off: %d\n", ret);
@@ -152,8 +149,6 @@ static int xpp055c272_unprepare(struct drm_panel *panel)
regulator_disable(ctx->iovcc);
regulator_disable(ctx->vci);
- ctx->prepared = false;
-
return 0;
}
@@ -163,9 +158,6 @@ static int xpp055c272_prepare(struct drm_panel *panel)
struct mipi_dsi_device *dsi = to_mipi_dsi_device(ctx->dev);
int ret;
- if (ctx->prepared)
- return 0;
-
dev_dbg(ctx->dev, "Resetting the panel\n");
ret = regulator_enable(ctx->vci);
if (ret < 0) {
@@ -209,8 +201,6 @@ static int xpp055c272_prepare(struct drm_panel *panel)
msleep(50);
- ctx->prepared = true;
-
return 0;
disable_iovcc:
@@ -320,15 +310,8 @@ static int xpp055c272_probe(struct mipi_dsi_device *dsi)
static void xpp055c272_shutdown(struct mipi_dsi_device *dsi)
{
struct xpp055c272 *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 xpp055c272_remove(struct mipi_dsi_device *dsi)
--
2.41.0.585.gd2178a4bd4-goog
^ permalink raw reply related [flat|nested] 34+ messages in thread* [RFC PATCH 06/10] drm/panel: Don't store+check prepared/enabled for panels disabled at shutdown
2023-08-04 21:06 [RFC PATCH 00/10] drm/panel: Remove most store/double-check of prepared/enabled state Douglas Anderson
` (4 preceding siblings ...)
2023-08-04 21:06 ` [RFC PATCH 05/10] drm/panel: Don't store+check prepared/enabled for panels needing shutdown Douglas Anderson
@ 2023-08-04 21:06 ` Douglas Anderson
2023-08-04 21:06 ` [RFC PATCH 07/10] drm/panel: st7703: Don't store+check prepared Douglas Anderson
` (4 subsequent siblings)
10 siblings, 0 replies; 34+ messages in thread
From: Douglas Anderson @ 2023-08-04 21:06 UTC (permalink / raw)
To: dri-devel, Maxime Ripard
Cc: Neil Armstrong, Douglas Anderson, linux-kernel, Sam Ravnborg
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 <dianders@chromium.org>
---
.../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
^ permalink raw reply related [flat|nested] 34+ messages in thread* [RFC PATCH 07/10] drm/panel: st7703: Don't store+check prepared
2023-08-04 21:06 [RFC PATCH 00/10] drm/panel: Remove most store/double-check of prepared/enabled state Douglas Anderson
` (5 preceding siblings ...)
2023-08-04 21:06 ` [RFC PATCH 06/10] drm/panel: Don't store+check prepared/enabled for panels disabled at shutdown Douglas Anderson
@ 2023-08-04 21:06 ` Douglas Anderson
2023-08-04 21:06 ` [RFC PATCH 08/10] drm/panel: rm67191: Don't store+check enabled Douglas Anderson
` (3 subsequent siblings)
10 siblings, 0 replies; 34+ messages in thread
From: Douglas Anderson @ 2023-08-04 21:06 UTC (permalink / raw)
To: dri-devel, Maxime Ripard
Cc: Ondrej Jirman, Neil Armstrong, Purism Kernel Team, Sam Ravnborg,
Douglas Anderson, linux-kernel, Guido Günther
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 <dianders@chromium.org>
---
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
^ permalink raw reply related [flat|nested] 34+ messages in thread* [RFC PATCH 08/10] drm/panel: rm67191: Don't store+check enabled
2023-08-04 21:06 [RFC PATCH 00/10] drm/panel: Remove most store/double-check of prepared/enabled state Douglas Anderson
` (6 preceding siblings ...)
2023-08-04 21:06 ` [RFC PATCH 07/10] drm/panel: st7703: Don't store+check prepared Douglas Anderson
@ 2023-08-04 21:06 ` Douglas Anderson
2023-08-04 21:06 ` [RFC PATCH 09/10] drm/panel: sony-acx565akm: Don't double-check enabled state in disable Douglas Anderson
` (2 subsequent siblings)
10 siblings, 0 replies; 34+ messages in thread
From: Douglas Anderson @ 2023-08-04 21:06 UTC (permalink / raw)
To: dri-devel, Maxime Ripard
Cc: Neil Armstrong, Sam Ravnborg, Douglas Anderson, linux-kernel,
Robert Chiras
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 <dianders@chromium.org>
---
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
^ permalink raw reply related [flat|nested] 34+ messages in thread* [RFC PATCH 09/10] drm/panel: sony-acx565akm: Don't double-check enabled state in disable
2023-08-04 21:06 [RFC PATCH 00/10] drm/panel: Remove most store/double-check of prepared/enabled state Douglas Anderson
` (7 preceding siblings ...)
2023-08-04 21:06 ` [RFC PATCH 08/10] drm/panel: rm67191: Don't store+check enabled Douglas Anderson
@ 2023-08-04 21:06 ` Douglas Anderson
2023-08-04 21:06 ` [RFC PATCH 10/10] drm/panel: Update TODO list item for cleaning up prepared/enabled tracking Douglas Anderson
2023-08-10 8:23 ` [RFC PATCH 00/10] drm/panel: Remove most store/double-check of prepared/enabled state Linus Walleij
10 siblings, 0 replies; 34+ messages in thread
From: Douglas Anderson @ 2023-08-04 21:06 UTC (permalink / raw)
To: dri-devel, Maxime Ripard
Cc: Neil Armstrong, Douglas Anderson, linux-kernel, Sam Ravnborg
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 <dianders@chromium.org>
---
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
^ permalink raw reply related [flat|nested] 34+ messages in thread* [RFC PATCH 10/10] drm/panel: Update TODO list item for cleaning up prepared/enabled tracking
2023-08-04 21:06 [RFC PATCH 00/10] drm/panel: Remove most store/double-check of prepared/enabled state Douglas Anderson
` (8 preceding siblings ...)
2023-08-04 21:06 ` [RFC PATCH 09/10] drm/panel: sony-acx565akm: Don't double-check enabled state in disable Douglas Anderson
@ 2023-08-04 21:06 ` Douglas Anderson
2023-08-07 6:41 ` Maxime Ripard
2023-08-10 8:23 ` [RFC PATCH 00/10] drm/panel: Remove most store/double-check of prepared/enabled state Linus Walleij
10 siblings, 1 reply; 34+ messages in thread
From: Douglas Anderson @ 2023-08-04 21:06 UTC (permalink / raw)
To: dri-devel, Maxime Ripard
Cc: Thomas Zimmermann, Jonathan Corbet, linux-doc, Douglas Anderson,
linux-kernel
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 <dianders@chromium.org>
---
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 <tzimmermann@suse.de>
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 <dianders@chromium.org>
--
2.41.0.585.gd2178a4bd4-goog
^ permalink raw reply related [flat|nested] 34+ messages in thread* Re: [RFC PATCH 10/10] drm/panel: Update TODO list item for cleaning up prepared/enabled tracking
2023-08-04 21:06 ` [RFC PATCH 10/10] drm/panel: Update TODO list item for cleaning up prepared/enabled tracking Douglas Anderson
@ 2023-08-07 6:41 ` Maxime Ripard
0 siblings, 0 replies; 34+ messages in thread
From: Maxime Ripard @ 2023-08-07 6:41 UTC (permalink / raw)
To: Douglas Anderson
Cc: Thomas Zimmermann, Jonathan Corbet, linux-doc, linux-kernel,
dri-devel
[-- Attachment #1: Type: text/plain, Size: 2701 bytes --]
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 <dianders@chromium.org>
> ---
>
> 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 <tzimmermann@suse.de>
>
> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [RFC PATCH 00/10] drm/panel: Remove most store/double-check of prepared/enabled state
2023-08-04 21:06 [RFC PATCH 00/10] drm/panel: Remove most store/double-check of prepared/enabled state Douglas Anderson
` (9 preceding siblings ...)
2023-08-04 21:06 ` [RFC PATCH 10/10] drm/panel: Update TODO list item for cleaning up prepared/enabled tracking Douglas Anderson
@ 2023-08-10 8:23 ` Linus Walleij
2023-09-05 19:15 ` Doug Anderson
10 siblings, 1 reply; 34+ messages in thread
From: Linus Walleij @ 2023-08-10 8:23 UTC (permalink / raw)
To: Douglas Anderson
Cc: Ondrej Jirman, Neil Armstrong, Purism Kernel Team,
Thomas Zimmermann, Jonathan Corbet, Sam Ravnborg,
Guido Günther, Jerry Han, Javier Martinez Canillas,
Maxime Ripard, linux-doc, Stefan Mavrodiev, Jianhua Lu, dri-devel,
Robert Chiras, Ondrej Jirman, Sumit Semwal, linux-kernel
On Fri, Aug 4, 2023 at 11:07 PM Douglas Anderson <dianders@chromium.org> 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 <linus.walleij@linaro.org>
Please send out a non-RFC version, this is clearly the right thing to
do.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 34+ messages in thread* Re: [RFC PATCH 00/10] drm/panel: Remove most store/double-check of prepared/enabled state
2023-08-10 8:23 ` [RFC PATCH 00/10] drm/panel: Remove most store/double-check of prepared/enabled state Linus Walleij
@ 2023-09-05 19:15 ` Doug Anderson
0 siblings, 0 replies; 34+ messages in thread
From: Doug Anderson @ 2023-09-05 19:15 UTC (permalink / raw)
To: Linus Walleij
Cc: Ondrej Jirman, Neil Armstrong, Purism Kernel Team,
Thomas Zimmermann, Jonathan Corbet, Sam Ravnborg,
Guido Günther, Jerry Han, Javier Martinez Canillas,
Maxime Ripard, linux-doc, Stefan Mavrodiev, Jianhua Lu, dri-devel,
Robert Chiras, Ondrej Jirman, Sumit Semwal, linux-kernel
Hi,
On Thu, Aug 10, 2023 at 1:23 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Fri, Aug 4, 2023 at 11:07 PM Douglas Anderson <dianders@chromium.org> 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 <linus.walleij@linaro.org>
>
> Please send out a non-RFC version, this is clearly the right thing to
> do.
As per the long discussion in response to patch #4, I think there are
still open questions about the later patches in this series. However,
I could land patches #1 - #3 if there are no concerns. Would anyone
object if I just landed them straight from this series with Linus's
review, or would I need to repost just patches #1 - #3 without the
"RFC" tag?
Thanks!
-Doug
^ permalink raw reply [flat|nested] 34+ messages in thread