LinuxLists.cc - [PATCH v3 00/24] drm/panel: Remove most store/double-check of prepared/enabled state

2024-06-05 00:24:23

Subject: [PATCH v3 00/24] drm/panel: Remove most store/double-check of prepared/enabled state

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.

In response to my V1 series [1] we had much discussion of what to
do. The conclusion was that as long as DRM modeset drivers properly
called drm_atomic_helper_shutdown() that we should be able to remove
the explicit shutdown/remove handling in the panel drivers. Most of
the patches to improve DRM modeset drivers [2] [3] [4] have now
landed.

In contrast to my V1 series, I broke the V2 series up a lot
more. Since a few of the panel drivers in V1 already landed, we had
fewer total drivers and so we could devote a patch to each panel.
Also, since we were now relying on DRM modeset drivers I felt like we
should split the patches for each panel into two: one that's
definitely safe and one that could be reverted if we found a
problematic DRM modeset driver that we couldn't fix.

Many of the patches in the V2 series [5] landed, so this V3 series is
the patches that are left plus one new bonus patch. At this point, we
may want to just land the patches that are left since it seems like
nobody is going to test/review them and they've all been Acked by
Linus and Maxime.

NOTE: this touches _a lot_ of drivers, is repetitive, and is not
really possible to generate automatically. That means it's entirely
possible that my eyes glazed over and I did something wrong. Please
double-check me and don't assume that I got everything perfect, though
I did my best. 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.

[1] https://lore.kernel.org/r/20230804140605.RFC.4.I930069a32baab6faf46d6b234f89613b5cec0f14@changeid
[2] https://lore.kernel.org/r/[email protected]
[3] https://lore.kernel.org/r/[email protected]
[4] https://lore.kernel.org/r/[email protected]
[5] https://lore.kernel.org/r/[email protected]/

Changes in v3:
- drm/panel: Avoid warnings w/ panel-simple/panel-edp at shutdown

Changes in v2:
- Only handle 1 panel per patch.
- Split removal of prepared/enabled from handling of remove/shutdown.

Douglas Anderson (24):
drm/panel: boe-himax8279d: Stop tracking prepared/enabled
drm/panel: boe-himax8279d: Don't call unprepare+disable at
shutdown/remove
drm/panel: khadas-ts050: Stop tracking prepared/enabled
drm/panel: khadas-ts050: Don't call unprepare+disable at
shutdown/remove
drm/panel: olimex-lcd-olinuxino: Stop tracking prepared/enabled
drm/panel: olimex-lcd-olinuxino: Don't call unprepare+disable at
remove
drm/panel: osd-osd101t2587-53ts: Stop tracking prepared/enabled
drm/panel: osd-osd101t2587-53ts: Don't call unprepare+disable at
shutdown/remove
drm/panel: tdo-tl070wsh30: Stop tracking prepared
drm/panel: tdo-tl070wsh30: Don't call unprepare+disable at
shutdown/remove
drm/panel: jdi-lt070me05000: Stop tracking prepared/enabled
drm/panel: jdi-lt070me05000: Don't call disable at shutdown/remove
drm/panel: panasonic-vvx10f034n00: Stop tracking prepared/enabled
drm/panel: panasonic-vvx10f034n00: Don't call disable at
shutdown/remove
drm/panel: seiko-43wvf1g: Stop tracking prepared/enabled
drm/panel: seiko-43wvf1g: Don't call disable at shutdown/remove
drm/panel: sharp-lq101r1sx01: Stop tracking prepared/enabled
drm/panel: sharp-lq101r1sx01: Don't call disable at shutdown/remove
drm/panel: sharp-ls043t1le01: Stop tracking prepared
drm/panel: sharp-ls043t1le01: Don't call disable at shutdown/remove
drm/panel: raydium-rm67191: Stop tracking enabled
drm/panel: raydium-rm67191: Don't call unprepare+disable at shutdown
drm/panel: Update TODO list item for cleaning up prepared/enabled
tracking
drm/panel: Avoid warnings w/ panel-simple/panel-edp at shutdown

Documentation/gpu/todo.rst | 47 +++---
drivers/gpu/drm/drm_panel.c | 12 ++
drivers/gpu/drm/panel/panel-boe-himax8279d.c | 40 -----
.../gpu/drm/panel/panel-drm-shutdown-check.h | 151 ++++++++++++++++++
drivers/gpu/drm/panel/panel-edp.c | 19 +--
.../gpu/drm/panel/panel-jdi-lt070me05000.c | 35 ----
drivers/gpu/drm/panel/panel-khadas-ts050.c | 39 -----
.../drm/panel/panel-olimex-lcd-olinuxino.c | 44 -----
.../drm/panel/panel-osd-osd101t2587-53ts.c | 41 +----
.../drm/panel/panel-panasonic-vvx10f034n00.c | 47 +-----
drivers/gpu/drm/panel/panel-raydium-rm67191.c | 26 ---
drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 49 ------
.../gpu/drm/panel/panel-sharp-lq101r1sx01.c | 63 +-------
.../gpu/drm/panel/panel-sharp-ls043t1le01.c | 24 ---
drivers/gpu/drm/panel/panel-simple.c | 19 +--
drivers/gpu/drm/panel/panel-tdo-tl070wsh30.c | 23 ---
16 files changed, 199 insertions(+), 480 deletions(-)
create mode 100644 drivers/gpu/drm/panel/panel-drm-shutdown-check.h

--
2.45.1.288.g0e0cd299f1-goog

2024-06-05 00:24:29

by Doug Anderson

[permalink] [raw]

Subject: [PATCH v3 01/24] drm/panel: boe-himax8279d: Stop tracking prepared/enabled

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.

NOTE: as part of this, transition the panel's direct calls to its
disable/unprepare functions in shutdown/remove to call through DRM
panel.

Cc: Jerry Han <[email protected]>
Cc: Jitao Shi <[email protected]>
Cc: Rock Wang <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Acked-by: Maxime Ripard <[email protected]>
Signed-off-by: Douglas Anderson <[email protected]>
---

(no changes since v2)

Changes in v2:
- Only handle 1 panel per patch.
- Split removal of prepared/enabled from handling of remove/shutdown.

drivers/gpu/drm/panel/panel-boe-himax8279d.c | 31 +++-----------------
1 file changed, 4 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-boe-himax8279d.c b/drivers/gpu/drm/panel/panel-boe-himax8279d.c
index e225840b0d67..12e14615298b 100644
--- a/drivers/gpu/drm/panel/panel-boe-himax8279d.c
+++ b/drivers/gpu/drm/panel/panel-boe-himax8279d.c
@@ -47,9 +47,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 +83,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 +97,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 +110,6 @@ static int boe_panel_unprepare(struct drm_panel *panel)

disable_gpios(pinfo);

- pinfo->prepared = false;
-
return 0;
}

@@ -131,9 +118,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 +164,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 +176,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 +184,6 @@ static int boe_panel_enable(struct drm_panel *panel)
return ret;
}

- pinfo->enabled = true;
-
return 0;
}

@@ -917,11 +894,11 @@ 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);
+ err = drm_panel_disable(&pinfo->base);
if (err < 0)
dev_err(&dsi->dev, "failed to disable panel: %d\n", err);

- err = boe_panel_unprepare(&pinfo->base);
+ err = drm_panel_unprepare(&pinfo->base);
if (err < 0)
dev_err(&dsi->dev, "failed to unprepare panel: %d\n", err);

@@ -936,8 +913,8 @@ 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_disable(&pinfo->base);
+ drm_panel_unprepare(&pinfo->base);
}

static struct mipi_dsi_driver panel_driver = {
--
2.45.1.288.g0e0cd299f1-goog

2024-06-05 00:24:50

by Doug Anderson

[permalink] [raw]

Subject: [PATCH v3 02/24] drm/panel: boe-himax8279d: Don't call unprepare+disable at shutdown/remove

It's the responsibility of a correctly written DRM modeset driver to
call drm_atomic_helper_shutdown() at shutdown time and that should be
disabling / unpreparing the panel if needed. Panel drivers shouldn't
be calling these functions themselves.

A recent effort was made to fix as many DRM modeset drivers as
possible [1] [2] [3] and most drivers are fixed now.

Unfortunately, grepping mainline for this panel's compatible string
shows no hits, so we can't be 100% sure if the DRM modeset driver used
with this panel has been fixed. If it is found that the DRM modeset
driver hasn't been fixed then this patch could be temporarily reverted
until it is.

[1] https://lore.kernel.org/r/[email protected]
[2] https://lore.kernel.org/r/[email protected]
[3] https://lore.kernel.org/r/[email protected]

Cc: Jerry Han <[email protected]>
Cc: Jitao Shi <[email protected]>
Cc: Rock Wang <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Acked-by: Maxime Ripard <[email protected]>
Signed-off-by: Douglas Anderson <[email protected]>
---

(no changes since v2)

Changes in v2:
- Only handle 1 panel per patch.
- Split removal of prepared/enabled from handling of remove/shutdown.

drivers/gpu/drm/panel/panel-boe-himax8279d.c | 17 -----------------
1 file changed, 17 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-boe-himax8279d.c b/drivers/gpu/drm/panel/panel-boe-himax8279d.c
index 12e14615298b..df746baae301 100644
--- a/drivers/gpu/drm/panel/panel-boe-himax8279d.c
+++ b/drivers/gpu/drm/panel/panel-boe-himax8279d.c
@@ -894,14 +894,6 @@ static void panel_remove(struct mipi_dsi_device *dsi)
struct panel_info *pinfo = mipi_dsi_get_drvdata(dsi);
int err;

- err = drm_panel_disable(&pinfo->base);
- if (err < 0)
- dev_err(&dsi->dev, "failed to disable panel: %d\n", err);
-
- err = drm_panel_unprepare(&pinfo->base);
- if (err < 0)
- dev_err(&dsi->dev, "failed to unprepare panel: %d\n", err);
-
err = mipi_dsi_detach(dsi);
if (err < 0)
dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", err);
@@ -909,14 +901,6 @@ static void panel_remove(struct mipi_dsi_device *dsi)
drm_panel_remove(&pinfo->base);
}

-static void panel_shutdown(struct mipi_dsi_device *dsi)
-{
- struct panel_info *pinfo = mipi_dsi_get_drvdata(dsi);
-
- drm_panel_disable(&pinfo->base);
- drm_panel_unprepare(&pinfo->base);
-}
-
static struct mipi_dsi_driver panel_driver = {
.driver = {
.name = "panel-boe-himax8279d",
@@ -924,7 +908,6 @@ static struct mipi_dsi_driver panel_driver = {
},
.probe = panel_probe,
.remove = panel_remove,
- .shutdown = panel_shutdown,
};
module_mipi_dsi_driver(panel_driver);

--
2.45.1.288.g0e0cd299f1-goog

2024-06-05 00:25:05

by Doug Anderson

[permalink] [raw]

Subject: [PATCH v3 03/24] drm/panel: khadas-ts050: Stop tracking prepared/enabled

2024-06-05 00:25:35

by Doug Anderson

[permalink] [raw]

Subject: [PATCH v3 05/24] drm/panel: olimex-lcd-olinuxino: Stop tracking prepared/enabled

2024-06-05 00:25:48

by Doug Anderson

[permalink] [raw]

Subject: [PATCH v3 06/24] drm/panel: olimex-lcd-olinuxino: Don't call unprepare+disable at remove

2024-06-05 00:26:04

by Doug Anderson

[permalink] [raw]

Subject: [PATCH v3 07/24] drm/panel: osd-osd101t2587-53ts: Stop tracking prepared/enabled

2024-06-05 00:26:07

by Doug Anderson

[permalink] [raw]

Subject: [PATCH v3 04/24] drm/panel: khadas-ts050: Don't call unprepare+disable at shutdown/remove

It's the responsibility of a correctly written DRM modeset driver to
call drm_atomic_helper_shutdown() at shutdown time and that should be
disabling / unpreparing the panel if needed. Panel drivers shouldn't
be calling these functions themselves.

A recent effort was made to fix as many DRM modeset drivers as
possible [1] [2] [3] and most drivers are fixed now.

Unfortunately, grepping mainline for this panel's compatible string
shows no hits, so we can't be 100% sure if the DRM modeset driver used
with this panel has been fixed. If it is found that the DRM modeset
driver hasn't been fixed then this patch could be temporarily reverted
until it is.

[1] https://lore.kernel.org/r/[email protected]
[2] https://lore.kernel.org/r/[email protected]
[3] https://lore.kernel.org/r/[email protected]

Cc: Jacobe Zang <[email protected]>
Cc: Nicolas Belin <[email protected]>
Cc: Neil Armstrong <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Acked-by: Maxime Ripard <[email protected]>
Signed-off-by: Douglas Anderson <[email protected]>
---

(no changes since v2)

Changes in v2:
- Only handle 1 panel per patch.
- Split removal of prepared/enabled from handling of remove/shutdown.

drivers/gpu/drm/panel/panel-khadas-ts050.c | 11 -----------
1 file changed, 11 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-khadas-ts050.c b/drivers/gpu/drm/panel/panel-khadas-ts050.c
index e35762ebdbd1..14932cb3defc 100644
--- a/drivers/gpu/drm/panel/panel-khadas-ts050.c
+++ b/drivers/gpu/drm/panel/panel-khadas-ts050.c
@@ -880,16 +880,6 @@ 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);
-}
-
-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);
}

static struct mipi_dsi_driver khadas_ts050_panel_driver = {
@@ -899,7 +889,6 @@ static struct mipi_dsi_driver khadas_ts050_panel_driver = {
},
.probe = khadas_ts050_panel_probe,
.remove = khadas_ts050_panel_remove,
- .shutdown = khadas_ts050_panel_shutdown,
};
module_mipi_dsi_driver(khadas_ts050_panel_driver);

--
2.45.1.288.g0e0cd299f1-goog

2024-06-05 00:27:00

by Doug Anderson

[permalink] [raw]

Subject: [PATCH v3 11/24] drm/panel: jdi-lt070me05000: Stop tracking prepared/enabled

2024-06-05 00:27:16

by Doug Anderson

[permalink] [raw]

Subject: [PATCH v3 12/24] drm/panel: jdi-lt070me05000: Don't call disable at shutdown/remove

It's the responsibility of a correctly written DRM modeset driver to
call drm_atomic_helper_shutdown() at shutdown time and that should be
disabling / unpreparing the panel if needed. Panel drivers shouldn't
be calling these functions themselves.

A recent effort was made to fix as many DRM modeset drivers as
possible [1] [2] [3] and most drivers are fixed now.

A grep through mainline for compatible strings used by this driver
indicates that it is used by Qualcomm boards. The Qualcomm driver
appears to be correctly calling drm_atomic_helper_shutdown() so we can
remove the calls.

[1] https://lore.kernel.org/r/[email protected]
[2] https://lore.kernel.org/r/[email protected]
[3] https://lore.kernel.org/r/[email protected]

Cc: Vinay Simha BN <[email protected]>
Cc: Sumit Semwal <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Acked-by: Maxime Ripard <[email protected]>
Signed-off-by: Douglas Anderson <[email protected]>
---

(no changes since v2)

Changes in v2:
- Only handle 1 panel per patch.
- Split removal of prepared/enabled from handling of remove/shutdown.

drivers/gpu/drm/panel/panel-jdi-lt070me05000.c | 12 ------------
1 file changed, 12 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
index 4ddddee6fa1e..b1ce186de261 100644
--- a/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
+++ b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
@@ -452,10 +452,6 @@ static void jdi_panel_remove(struct mipi_dsi_device *dsi)
struct jdi_panel *jdi = mipi_dsi_get_drvdata(dsi);
int ret;

- ret = drm_panel_disable(&jdi->base);
- if (ret < 0)
- dev_err(&dsi->dev, "failed to disable panel: %d\n", ret);
-
ret = mipi_dsi_detach(dsi);
if (ret < 0)
dev_err(&dsi->dev, "failed to detach from DSI host: %d\n",
@@ -464,13 +460,6 @@ static void jdi_panel_remove(struct mipi_dsi_device *dsi)
jdi_panel_del(jdi);
}

-static void jdi_panel_shutdown(struct mipi_dsi_device *dsi)
-{
- struct jdi_panel *jdi = mipi_dsi_get_drvdata(dsi);
-
- drm_panel_disable(&jdi->base);
-}
-
static struct mipi_dsi_driver jdi_panel_driver = {
.driver = {
.name = "panel-jdi-lt070me05000",
@@ -478,7 +467,6 @@ static struct mipi_dsi_driver jdi_panel_driver = {
},
.probe = jdi_panel_probe,
.remove = jdi_panel_remove,
- .shutdown = jdi_panel_shutdown,
};
module_mipi_dsi_driver(jdi_panel_driver);

--
2.45.1.288.g0e0cd299f1-goog

2024-06-05 00:27:28

by Doug Anderson

[permalink] [raw]

Subject: [PATCH v3 13/24] drm/panel: panasonic-vvx10f034n00: Stop tracking prepared/enabled

2024-06-05 00:27:44

by Doug Anderson

[permalink] [raw]

Subject: [PATCH v3 08/24] drm/panel: osd-osd101t2587-53ts: Don't call unprepare+disable at shutdown/remove

It's the responsibility of a correctly written DRM modeset driver to
call drm_atomic_helper_shutdown() at shutdown time and that should be
disabling / unpreparing the panel if needed. Panel drivers shouldn't
be calling these functions themselves.

A recent effort was made to fix as many DRM modeset drivers as
possible [1] [2] [3] and most drivers are fixed now.

A grep through mainline for compatible strings used by this driver
indicates that it is used by TI OMAP boards. The OMAP driver appears
to be correctly calling drm_atomic_helper_shutdown() so we can remove
the calls.

[1] https://lore.kernel.org/r/[email protected]
[2] https://lore.kernel.org/r/[email protected]
[3] https://lore.kernel.org/r/[email protected]

Cc: Peter Ujfalusi <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Acked-by: Maxime Ripard <[email protected]>
Signed-off-by: Douglas Anderson <[email protected]>
---

(no changes since v2)

Changes in v2:
- Only handle 1 panel per patch.
- Split removal of prepared/enabled from handling of remove/shutdown.

drivers/gpu/drm/panel/panel-osd-osd101t2587-53ts.c | 14 --------------
1 file changed, 14 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-osd-osd101t2587-53ts.c b/drivers/gpu/drm/panel/panel-osd-osd101t2587-53ts.c
index c0da7d9512e8..dbea84f51514 100644
--- a/drivers/gpu/drm/panel/panel-osd-osd101t2587-53ts.c
+++ b/drivers/gpu/drm/panel/panel-osd-osd101t2587-53ts.c
@@ -186,11 +186,6 @@ 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_remove(&osd101t2587->base);

ret = mipi_dsi_detach(dsi);
@@ -198,14 +193,6 @@ static void osd101t2587_panel_remove(struct mipi_dsi_device *dsi)
dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret);
}

-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);
-}
-
static struct mipi_dsi_driver osd101t2587_panel_driver = {
.driver = {
.name = "panel-osd-osd101t2587-53ts",
@@ -213,7 +200,6 @@ static struct mipi_dsi_driver osd101t2587_panel_driver = {
},
.probe = osd101t2587_panel_probe,
.remove = osd101t2587_panel_remove,
- .shutdown = osd101t2587_panel_shutdown,
};
module_mipi_dsi_driver(osd101t2587_panel_driver);

--
2.45.1.288.g0e0cd299f1-goog

2024-06-05 00:27:44

by Doug Anderson

[permalink] [raw]

Subject: [PATCH v3 14/24] drm/panel: panasonic-vvx10f034n00: Don't call disable at shutdown/remove

It's the responsibility of a correctly written DRM modeset driver to
call drm_atomic_helper_shutdown() at shutdown time and that should be
disabling / unpreparing the panel if needed. Panel drivers shouldn't
be calling these functions themselves.

A recent effort was made to fix as many DRM modeset drivers as
possible [1] [2] [3] and most drivers are fixed now.

Unfortunately, grepping mainline for this panel's compatible string
shows no hits, so we can't be 100% sure if the DRM modeset driver used
with this panel has been fixed. If it is found that the DRM modeset
driver hasn't been fixed then this patch could be temporarily reverted
until it is.

[1] https://lore.kernel.org/r/[email protected]
[2] https://lore.kernel.org/r/[email protected]
[3] https://lore.kernel.org/r/[email protected]

Cc: Werner Johansson <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Acked-by: Maxime Ripard <[email protected]>
Signed-off-by: Douglas Anderson <[email protected]>
---

(no changes since v2)

Changes in v2:
- Only handle 1 panel per patch.
- Split removal of prepared/enabled from handling of remove/shutdown.

drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c | 12 ------------
1 file changed, 12 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c b/drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c
index 822ca2f971eb..d1c5c9bc3c56 100644
--- a/drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c
+++ b/drivers/gpu/drm/panel/panel-panasonic-vvx10f034n00.c
@@ -222,10 +222,6 @@ 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);
-
ret = mipi_dsi_detach(dsi);
if (ret < 0)
dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret);
@@ -233,13 +229,6 @@ static void wuxga_nt_panel_remove(struct mipi_dsi_device *dsi)
wuxga_nt_panel_del(wuxga_nt);
}

-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);
-}
-
static struct mipi_dsi_driver wuxga_nt_panel_driver = {
.driver = {
.name = "panel-panasonic-vvx10f034n00",
@@ -247,7 +236,6 @@ static struct mipi_dsi_driver wuxga_nt_panel_driver = {
},
.probe = wuxga_nt_panel_probe,
.remove = wuxga_nt_panel_remove,
- .shutdown = wuxga_nt_panel_shutdown,
};
module_mipi_dsi_driver(wuxga_nt_panel_driver);

--
2.45.1.288.g0e0cd299f1-goog

2024-06-05 00:27:55

by Doug Anderson

[permalink] [raw]

Subject: [PATCH v3 09/24] drm/panel: tdo-tl070wsh30: Stop tracking prepared

2024-06-05 00:28:08

by Doug Anderson

[permalink] [raw]

Subject: [PATCH v3 10/24] drm/panel: tdo-tl070wsh30: Don't call unprepare+disable at shutdown/remove

It's the responsibility of a correctly written DRM modeset driver to
call drm_atomic_helper_shutdown() at shutdown time and that should be
disabling / unpreparing the panel if needed. Panel drivers shouldn't
be calling these functions themselves.

A recent effort was made to fix as many DRM modeset drivers as
possible [1] [2] [3] and most drivers are fixed now.

Unfortunately, grepping mainline for this panel's compatible string
shows no hits, so we can't be 100% sure if the DRM modeset driver used
with this panel has been fixed. If it is found that the DRM modeset
driver hasn't been fixed then this patch could be temporarily reverted
until it is.

[1] https://lore.kernel.org/r/[email protected]
[2] https://lore.kernel.org/r/[email protected]
[3] https://lore.kernel.org/r/[email protected]

Cc: Neil Armstrong <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Acked-by: Maxime Ripard <[email protected]>
Signed-off-by: Douglas Anderson <[email protected]>
---

(no changes since v2)

Changes in v2:
- Only handle 1 panel per patch.
- Split removal of prepared/enabled from handling of remove/shutdown.

drivers/gpu/drm/panel/panel-tdo-tl070wsh30.c | 11 -----------
1 file changed, 11 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-tdo-tl070wsh30.c b/drivers/gpu/drm/panel/panel-tdo-tl070wsh30.c
index 36f27c664b69..227f97f9b136 100644
--- a/drivers/gpu/drm/panel/panel-tdo-tl070wsh30.c
+++ b/drivers/gpu/drm/panel/panel-tdo-tl070wsh30.c
@@ -208,16 +208,6 @@ 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);
-}
-
-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);
}

static struct mipi_dsi_driver tdo_tl070wsh30_panel_driver = {
@@ -227,7 +217,6 @@ static struct mipi_dsi_driver tdo_tl070wsh30_panel_driver = {
},
.probe = tdo_tl070wsh30_panel_probe,
.remove = tdo_tl070wsh30_panel_remove,
- .shutdown = tdo_tl070wsh30_panel_shutdown,
};
module_mipi_dsi_driver(tdo_tl070wsh30_panel_driver);

--
2.45.1.288.g0e0cd299f1-goog

2024-06-05 00:28:54

by Doug Anderson

[permalink] [raw]

Subject: [PATCH v3 17/24] drm/panel: sharp-lq101r1sx01: Stop tracking prepared/enabled

2024-06-05 00:28:59

by Doug Anderson

[permalink] [raw]

Subject: [PATCH v3 19/24] drm/panel: sharp-ls043t1le01: Stop tracking prepared

2024-06-05 00:29:01

by Doug Anderson

[permalink] [raw]

Subject: [PATCH v3 18/24] drm/panel: sharp-lq101r1sx01: Don't call disable at shutdown/remove

It's the responsibility of a correctly written DRM modeset driver to
call drm_atomic_helper_shutdown() at shutdown time and that should be
disabling / unpreparing the panel if needed. Panel drivers shouldn't
be calling these functions themselves.

A recent effort was made to fix as many DRM modeset drivers as
possible [1] [2] [3] and most drivers are fixed now.

A grep through mainline for compatible strings used by this driver
indicates that it is used by Tegra boards. The Tegra driver appears to
be correctly calling drm_atomic_helper_shutdown() so we can remove the
calls.

[1] https://lore.kernel.org/r/[email protected]
[2] https://lore.kernel.org/r/[email protected]
[3] https://lore.kernel.org/r/[email protected]

Cc: Thierry Reding <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Acked-by: Maxime Ripard <[email protected]>
Signed-off-by: Douglas Anderson <[email protected]>
---

(no changes since v2)

Changes in v2:
- Only handle 1 panel per patch.
- Split removal of prepared/enabled from handling of remove/shutdown.

.../gpu/drm/panel/panel-sharp-lq101r1sx01.c | 24 ++-----------------
1 file changed, 2 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
index 8f6c21b99522..edc9425bb143 100644
--- a/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-lq101r1sx01.c
@@ -357,32 +357,13 @@ static void sharp_panel_remove(struct mipi_dsi_device *dsi)
struct sharp_panel *sharp = mipi_dsi_get_drvdata(dsi);
int err;

- /* only detach from host for the DSI-LINK2 interface */
- if (!sharp) {
- mipi_dsi_detach(dsi);
- return;
- }
-
- err = drm_panel_disable(&sharp->base);
- if (err < 0)
- dev_err(&dsi->dev, "failed to disable panel: %d\n", err);
-
err = mipi_dsi_detach(dsi);
if (err < 0)
dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", err);

- sharp_panel_del(sharp);
-}
-
-static void sharp_panel_shutdown(struct mipi_dsi_device *dsi)
-{
- struct sharp_panel *sharp = mipi_dsi_get_drvdata(dsi);
-
- /* nothing to do for DSI-LINK2 */
+ /* only detach from host for the DSI-LINK2 interface */
if (!sharp)
- return;
-
- drm_panel_disable(&sharp->base);
+ sharp_panel_del(sharp);
}

static struct mipi_dsi_driver sharp_panel_driver = {
@@ -392,7 +373,6 @@ static struct mipi_dsi_driver sharp_panel_driver = {
},
.probe = sharp_panel_probe,
.remove = sharp_panel_remove,
- .shutdown = sharp_panel_shutdown,
};
module_mipi_dsi_driver(sharp_panel_driver);

--
2.45.1.288.g0e0cd299f1-goog

2024-06-05 00:29:12

by Doug Anderson

[permalink] [raw]

Subject: [PATCH v3 20/24] drm/panel: sharp-ls043t1le01: Don't call disable at shutdown/remove

It's the responsibility of a correctly written DRM modeset driver to
call drm_atomic_helper_shutdown() at shutdown time and that should be
disabling / unpreparing the panel if needed. Panel drivers shouldn't
be calling these functions themselves.

A recent effort was made to fix as many DRM modeset drivers as
possible [1] [2] [3] and most drivers are fixed now.

A grep through mainline for compatible strings used by this driver
indicates that it is used by Qualcomm boards. The Qualcomm driver
appears to be correctly calling drm_atomic_helper_shutdown() so we can
remove the calls.

[1] https://lore.kernel.org/r/[email protected]
[2] https://lore.kernel.org/r/[email protected]
[3] https://lore.kernel.org/r/[email protected]

Cc: Werner Johansson <[email protected]>
Cc: Dmitry Baryshkov <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Acked-by: Maxime Ripard <[email protected]>
Signed-off-by: Douglas Anderson <[email protected]>
---

(no changes since v2)

Changes in v2:
- Only handle 1 panel per patch.
- Split removal of prepared/enabled from handling of remove/shutdown.

drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c | 12 ------------
1 file changed, 12 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c b/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
index c86337954ad7..729cbb0d8403 100644
--- a/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
+++ b/drivers/gpu/drm/panel/panel-sharp-ls043t1le01.c
@@ -267,10 +267,6 @@ 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);
-
ret = mipi_dsi_detach(dsi);
if (ret < 0)
dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", ret);
@@ -278,13 +274,6 @@ static void sharp_nt_panel_remove(struct mipi_dsi_device *dsi)
sharp_nt_panel_del(sharp_nt);
}

-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);
-}
-
static const struct of_device_id sharp_nt_of_match[] = {
{ .compatible = "sharp,ls043t1le01-qhd", },
{ }
@@ -298,7 +287,6 @@ static struct mipi_dsi_driver sharp_nt_panel_driver = {
},
.probe = sharp_nt_panel_probe,
.remove = sharp_nt_panel_remove,
- .shutdown = sharp_nt_panel_shutdown,
};
module_mipi_dsi_driver(sharp_nt_panel_driver);

--
2.45.1.288.g0e0cd299f1-goog

2024-06-05 00:29:13

by Doug Anderson

[permalink] [raw]

Subject: [PATCH v3 16/24] drm/panel: seiko-43wvf1g: Don't call disable at shutdown/remove

It's the responsibility of a correctly written DRM modeset driver to
call drm_atomic_helper_shutdown() at shutdown time and that should be
disabling / unpreparing the panel if needed. Panel drivers shouldn't
be calling these functions themselves.

A recent effort was made to fix as many DRM modeset drivers as
possible [1] [2] [3] and most drivers are fixed now.

A grep through mainline for compatible strings used by this driver
indicates that it is used by IMX boards. As far as I can tell, all IMX
boards are now correctly calling drm_atomic_helper_shutdown() so we
can remove the calls.

[1] https://lore.kernel.org/r/[email protected]
[2] https://lore.kernel.org/r/[email protected]
[3] https://lore.kernel.org/r/[email protected]

Cc: Marco Franchi <[email protected]>
Cc: Fabio Estevam <[email protected]>
Cc: Laurentiu Palcu <[email protected]>
Cc: Pengutronix Kernel Team <[email protected]>
Cc: [email protected]
Acked-by: Linus Walleij <[email protected]>
Acked-by: Maxime Ripard <[email protected]>
Signed-off-by: Douglas Anderson <[email protected]>
---

(no changes since v2)

Changes in v2:
- Only handle 1 panel per patch.
- Split removal of prepared/enabled from handling of remove/shutdown.

drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 9 ---------
1 file changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c
index 98480904126c..8a3fe531c641 100644
--- a/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c
+++ b/drivers/gpu/drm/panel/panel-seiko-43wvf1g.c
@@ -243,14 +243,6 @@ 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);
-}
-
-static void seiko_panel_shutdown(struct platform_device *pdev)
-{
- struct seiko_panel *panel = platform_get_drvdata(pdev);
-
- drm_panel_disable(&panel->base);
}

static const struct display_timing seiko_43wvf1g_timing = {
@@ -306,7 +298,6 @@ static struct platform_driver seiko_panel_platform_driver = {
},
.probe = seiko_panel_platform_probe,
.remove_new = seiko_panel_remove,
- .shutdown = seiko_panel_shutdown,
};
module_platform_driver(seiko_panel_platform_driver);

--
2.45.1.288.g0e0cd299f1-goog

2024-06-05 00:29:42

by Doug Anderson

[permalink] [raw]

Subject: [PATCH v3 22/24] drm/panel: raydium-rm67191: Don't call unprepare+disable at shutdown

It's the responsibility of a correctly written DRM modeset driver to
call drm_atomic_helper_shutdown() at shutdown time and that should be
disabling / unpreparing the panel if needed. Panel drivers shouldn't
be calling these functions themselves.

A recent effort was made to fix as many DRM modeset drivers as
possible [1] [2] [3] and most drivers are fixed now.

A grep through mainline for compatible strings used by this driver
indicates that it is used by IMX boards. The IMX driver appears to be
correctly calling drm_atomic_helper_shutdown() so we can remove the
calls.

[1] https://lore.kernel.org/r/[email protected]
[2] https://lore.kernel.org/r/[email protected]
[3] https://lore.kernel.org/r/[email protected]

Cc: Robert Chiras <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Acked-by: Maxime Ripard <[email protected]>
Signed-off-by: Douglas Anderson <[email protected]>
---

(no changes since v2)

Changes in v2:
- Split removal of prepared/enabled from handling of remove/shutdown.

drivers/gpu/drm/panel/panel-raydium-rm67191.c | 9 ---------
1 file changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-raydium-rm67191.c b/drivers/gpu/drm/panel/panel-raydium-rm67191.c
index fa9bf89d3bb5..b2029e035635 100644
--- a/drivers/gpu/drm/panel/panel-raydium-rm67191.c
+++ b/drivers/gpu/drm/panel/panel-raydium-rm67191.c
@@ -612,14 +612,6 @@ static void rad_panel_remove(struct mipi_dsi_device *dsi)
drm_panel_remove(&rad->panel);
}

-static void rad_panel_shutdown(struct mipi_dsi_device *dsi)
-{
- struct rad_panel *rad = mipi_dsi_get_drvdata(dsi);
-
- drm_panel_disable(&rad->panel);
- drm_panel_unprepare(&rad->panel);
-}
-
static const struct of_device_id rad_of_match[] = {
{ .compatible = "raydium,rm67191", },
{ /* sentinel */ }
@@ -633,7 +625,6 @@ static struct mipi_dsi_driver rad_panel_driver = {
},
.probe = rad_panel_probe,
.remove = rad_panel_remove,
- .shutdown = rad_panel_shutdown,
};
module_mipi_dsi_driver(rad_panel_driver);

--
2.45.1.288.g0e0cd299f1-goog

2024-06-05 00:29:56

by Doug Anderson

[permalink] [raw]

Subject: [PATCH v3 23/24] drm/panel: Update TODO list item for cleaning up prepared/enabled tracking

Now that most panels have been updated not to track/double-check their
prepared/enabled state update the TODO with next steps.

Acked-by: Linus Walleij <[email protected]>
Acked-by: Maxime Ripard <[email protected]>
Signed-off-by: Douglas Anderson <[email protected]>
---

(no changes since v1)

Documentation/gpu/todo.rst | 47 +++++++++++++++++++++-----------------
1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 2734b8a34541..2ea6ffc9b22b 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -469,30 +469,35 @@ Contact: Thomas Zimmermann <[email protected]>

Level: Starter

-Clean up checks for already prepared/enabled in panels
-------------------------------------------------------
-
-In a whole pile of panel drivers, we have code to make the
-prepare/unprepare/enable/disable callbacks behave as no-ops if they've already
-been called. To get some idea of the duplicated code, try::
-
- git grep 'if.*>prepared' -- drivers/gpu/drm/panel
- git grep 'if.*>enabled' -- drivers/gpu/drm/panel
-
-In the patch ("drm/panel: Check for already prepared/enabled in drm_panel")
-we've moved this check to the core. Now we can most definitely remove the
-check from the individual panels and save a pile of code.
-
-In adition to removing the check from the individual panels, it is believed
-that even the core shouldn't need this check and that should be considered
-an error if other code ever relies on this check. The check in the core
-currently prints a warning whenever something is relying on this check with
-dev_warn(). After a little while, we likely want to promote this to a
-WARN(1) to help encourage folks not to rely on this behavior.
+Remove disable/unprepare in remove/shutdown in panel-simple and panel-edp
+-------------------------------------------------------------------------
+
+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. Eventually that should probably
+be turned into a WARN_ON() or somehow made louder, but right now we actually
+expect it to trigger and so we don't want it to be too loud.
+
+Specifically, that warning will trigger for panel-edp and panel-simple at
+shutdown time because those panels hardcode a call to drm_panel_disable()
+and drm_panel_unprepare() at shutdown and remove time that they call regardless
+of panel state. On systems with a properly coded DRM modeset driver that
+calls drm_atomic_helper_shutdown() this is pretty much guaranteed to cause
+the warning to fire.
+
+Unfortunately we can't safely remove the calls in panel-edp and panel-simple
+until we're sure that all DRM modeset drivers that are used with those panels
+properly call drm_atomic_helper_shutdown(). This TODO item is to validate
+that all DRM modeset drivers used with panel-edp and panel-simple properly
+call drm_atomic_helper_shutdown() and then remove the calls to
+disable/unprepare from those panels. Alternatively, this TODO item could be
+removed by convincing stakeholders that those calls are fine and downgrading
+the error message in drm_panel_disable() / drm_panel_unprepare() to a
+debug-level message.

Contact: Douglas Anderson <[email protected]>

-Level: Starter/Intermediate
+Level: Intermediate

Transition away from using mipi_dsi_*_write_seq()
-------------------------------------------------
--
2.45.1.288.g0e0cd299f1-goog

2024-06-05 00:30:13

by Doug Anderson

[permalink] [raw]

Subject: [PATCH v3 24/24] drm/panel: Avoid warnings w/ panel-simple/panel-edp at shutdown

At shutdown if you've got a _properly_ coded DRM modeset driver then
you'll get these two warnings at shutdown time:

Skipping disable of already disabled panel
Skipping unprepare of already unprepared panel

These warnings are ugly and sound concerning, but they're actually a
sign of a properly working system. That's not great.

It's not easy to get rid of these warnings. Until we know that all DRM
modeset drivers used with panel-simple and panel-edp are properly
calling drm_atomic_helper_shutdown() or drm_helper_force_disable_all()
then the panel drivers _need_ to disable/unprepare themselves in order
to power off the panel cleanly. However, there are lots of DRM modeset
drivers used with panel-edp and panel-simple and it's hard to know
when we've got them all. Since the warning happens only on the drivers
that _are_ updated there's nothing to encourage broken DRM modeset
drivers to get fixed.

In order to flip the warning to the proper place, we need to know
which modeset drivers are going to shutdown properly. Though ugly, do
this by creating a list of everyone that shuts down properly. This
allows us to generate a warning for the correct case and also lets us
get rid of the warning for drivers that are shutting down properly.

Maintaining this list is ugly, but the idea is that it's only short
term. Once everyone is converted we can delete the list and call it
done. The list is ugly enough and adding to it is annoying enough that
people should push to make this happen.

Implement this all in a shared "header" file included by the two panel
drivers that need it. This avoids us adding an new exports while still
allowing the panel drivers to be modules. The code waste should be
small and, as per above, the whole solution is temporary.

Signed-off-by: Douglas Anderson <[email protected]>
---
I came up with this idea to help us move forward since otherwise I
couldn't see how we were ever going to fix panel-simple and panel-edp
since they're used by so many DRM Modeset drivers. It's a bit ugly but
I don't hate it. What do others think?

This is at the end of the series so even if folks hate it we could
still land the rest of the series.

Changes in v3:
- New

drivers/gpu/drm/drm_panel.c | 12 ++
.../gpu/drm/panel/panel-drm-shutdown-check.h | 151 ++++++++++++++++++
drivers/gpu/drm/panel/panel-edp.c | 19 +--
drivers/gpu/drm/panel/panel-simple.c | 19 +--
4 files changed, 169 insertions(+), 32 deletions(-)
create mode 100644 drivers/gpu/drm/panel/panel-drm-shutdown-check.h

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index cfbe020de54e..df3f15f4625e 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -161,6 +161,12 @@ int drm_panel_unprepare(struct drm_panel *panel)
if (!panel)
return -EINVAL;

+ /*
+ * If you're seeing this warning, you either need to add your driver
+ * to "drm_drivers_that_shutdown" (if you're seeing it with panel-edp
+ * or panel-simple) or you need to remove the manual call to
+ * drm_panel_unprepare() in your panel driver.
+ */
if (!panel->prepared) {
dev_warn(panel->dev, "Skipping unprepare of already unprepared panel\n");
return 0;
@@ -245,6 +251,12 @@ int drm_panel_disable(struct drm_panel *panel)
if (!panel)
return -EINVAL;

+ /*
+ * If you're seeing this warning, you either need to add your driver
+ * to "drm_drivers_that_shutdown" (if you're seeing it with panel-edp
+ * or panel-simple) or you need to remove the manual call to
+ * drm_panel_disable() in your panel driver.
+ */
if (!panel->enabled) {
dev_warn(panel->dev, "Skipping disable of already disabled panel\n");
return 0;
diff --git a/drivers/gpu/drm/panel/panel-drm-shutdown-check.h b/drivers/gpu/drm/panel/panel-drm-shutdown-check.h
new file mode 100644
index 000000000000..b5164490d6e7
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-drm-shutdown-check.h
@@ -0,0 +1,151 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright 2024 Google Inc.
+ *
+ * This header is a temporary solution and is intended to be included
+ * directly by panel-edp.c and panel-simple.c.
+ *
+ * This header is needed because panel-edp and panel-simple are used by a
+ * wide variety of DRM drivers and it's hard to know for sure if all of the
+ * DRM drivers used by those panel drivers are properly calling
+ * drm_atomic_helper_shutdown() or drm_helper_force_disable_all() at
+ * shutdown/remove time.
+ *
+ * The plan for this header file:
+ * - Land it and hope that the warning print will encourage DRM drivers to
+ * get fixed.
+ * - Eventually move to a WARN() splat for extra encouragement.
+ * - Assume that everyone has been fixed and remove this header file.
+ */
+
+#ifndef __PANEL_DRM_SHUTDOWN_CHECK_H__
+#define __PANEL_DRM_SHUTDOWN_CHECK_H__
+
+#include <drm/drm_bridge.h>
+#include <drm/drm_drv.h>
+
+/*
+ * This is a list of all DRM drivers that appear to properly call
+ * drm_atomic_helper_shutdown() or drm_helper_force_disable_all() at
+ * shutdown and remove time.
+ *
+ * We can't detect this dynamically and are stuck with a list because the panel
+ * driver's shutdown() call might be called _before_ the DRM driver's
+ * shutdown() call.
+ *
+ * NOTE: no verification has been done to confirm that the below drivers
+ * are actually _used_ with panel-simple or panel-edp, only that these drivers
+ * appear to be shutting down properly. It doesn't hurt to have extra drivers
+ * listed here as long as the list doesn't contain any drivers that are
+ * missing the shutdown calls.
+ */
+static const char * const drm_drivers_that_shutdown[] = {
+ "armada-drm",
+ "aspeed-gfx-drm",
+ "ast",
+ "atmel-hlcdc",
+ "bochs-drm",
+ "cirrus",
+ "exynos",
+ "fsl-dcu-drm",
+ "gm12u320",
+ "gud",
+ "hdlcd",
+ "hibmc",
+ "hx8357d",
+ "hyperv_drm",
+ "ili9163",
+ "ili9225",
+ "ili9341",
+ "ili9486",
+ "imx-dcss",
+ "imx-drm",
+ "imx-lcdc",
+ "imx-lcdif",
+ "ingenic-drm",
+ "kirin",
+ "komeda",
+ "logicvc-drm",
+ "loongson",
+ "mali-dp",
+ "mcde",
+ "meson",
+ "mgag200",
+ "mi0283qt",
+ "msm",
+ "mxsfb-drm",
+ "omapdrm",
+ "panel-mipi-dbi",
+ "pl111",
+ "qxl",
+ "rcar-du",
+ "repaper",
+ "rockchip",
+ "rzg2l-du",
+ "ssd130x",
+ "st7586",
+ "st7735r",
+ "sti",
+ "stm",
+ "sun4i-drm",
+ "tidss",
+ "tilcdc",
+ "tve200",
+ "vboxvideo",
+ "zynqmp-dpsub",
+ ""
+};
+
+static void panel_shutdown_if_drm_driver_needs_fixing(struct drm_panel *panel)
+{
+ struct drm_bridge *bridge;
+ const struct drm_driver *driver;
+ const char * const *driver_name;
+
+ /*
+ * Look for a bridge that shares the DT node of this panel. That only
+ * works if we've been linked up with a panel_bridge.
+ */
+ bridge = of_drm_find_bridge(panel->dev->of_node);
+ if (bridge && bridge->dev && bridge->dev->driver) {
+ /*
+ * If the DRM driver for the bridge is known to be fine then
+ * we're done.
+ */
+ driver = bridge->dev->driver;
+ for (driver_name = drm_drivers_that_shutdown; *driver_name; driver_name++) {
+ if (strcmp(*driver_name, driver->name) == 0)
+ return;
+ }
+
+ /*
+ * If you see the message below then:
+ * 1. Make sure your DRM driver is properly calling
+ * drm_atomic_helper_shutdown() or drm_helper_force_disable_all()
+ * at shutdown time.
+ * 2. Add your driver to the list.
+ */
+ dev_warn(panel->dev,
+ "DRM driver appears buggy; manually disable/unprepare\n");
+ } else {
+ /*
+ * If you see the message below then your setup needs to
+ * be moved to using a panel_bridge. This often happens
+ * by calling devm_drm_of_get_bridge(). Having a panel without
+ * an associated panel_bridge is deprecated.
+ */
+ dev_warn(panel->dev,
+ "Can't't find DRM driver; manually disable/unprepare\n");
+ }
+
+ /*
+ * If we don't know if a DRM driver is properly shutting things down
+ * then we'll manually call the disable/unprepare. This is always a
+ * safe thing to do (in that it won't cause you to crash), but it
+ * does generate a warning.
+ */
+ drm_panel_disable(panel);
+ drm_panel_unprepare(panel);
+}
+
+#endif
diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
index 67ab6915d6e4..26f89858df9d 100644
--- a/drivers/gpu/drm/panel/panel-edp.c
+++ b/drivers/gpu/drm/panel/panel-edp.c
@@ -42,6 +42,8 @@
#include <drm/drm_edid.h>
#include <drm/drm_panel.h>

+#include "panel-drm-shutdown-check.h"
+
/**
* struct panel_delay - Describes delays for a simple panel.
*/
@@ -948,22 +950,7 @@ static void panel_edp_shutdown(struct device *dev)
{
struct panel_edp *panel = dev_get_drvdata(dev);

- /*
- * NOTE: the following two calls don't really belong here. It is the
- * responsibility of a correctly written DRM modeset driver to call
- * drm_atomic_helper_shutdown() at shutdown time and that should
- * cause the panel to be disabled / unprepared if needed. For now,
- * however, we'll keep these calls due to the sheer number of
- * different DRM modeset drivers used with panel-edp. The fact that
- * we're calling these and _also_ the drm_atomic_helper_shutdown()
- * will try to disable/unprepare means that we can get a warning about
- * trying to disable/unprepare an already disabled/unprepared panel,
- * but that's something we'll have to live with until we've confirmed
- * that all DRM modeset drivers are properly calling
- * drm_atomic_helper_shutdown().
- */
- drm_panel_disable(&panel->base);
- drm_panel_unprepare(&panel->base);
+ panel_shutdown_if_drm_driver_needs_fixing(&panel->base);
}

static void panel_edp_remove(struct device *dev)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 9b9e078ec8aa..36b29c473609 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -42,6 +42,8 @@
#include <drm/drm_panel.h>
#include <drm/drm_of.h>

+#include "panel-drm-shutdown-check.h"
+
/**
* struct panel_desc - Describes a simple panel.
*/
@@ -720,22 +722,7 @@ static void panel_simple_shutdown(struct device *dev)
{
struct panel_simple *panel = dev_get_drvdata(dev);

- /*
- * NOTE: the following two calls don't really belong here. It is the
- * responsibility of a correctly written DRM modeset driver to call
- * drm_atomic_helper_shutdown() at shutdown time and that should
- * cause the panel to be disabled / unprepared if needed. For now,
- * however, we'll keep these calls due to the sheer number of
- * different DRM modeset drivers used with panel-simple. The fact that
- * we're calling these and _also_ the drm_atomic_helper_shutdown()
- * will try to disable/unprepare means that we can get a warning about
- * trying to disable/unprepare an already disabled/unprepared panel,
- * but that's something we'll have to live with until we've confirmed
- * that all DRM modeset drivers are properly calling
- * drm_atomic_helper_shutdown().
- */
- drm_panel_disable(&panel->base);
- drm_panel_unprepare(&panel->base);
+ panel_shutdown_if_drm_driver_needs_fixing(&panel->base);
}

static void panel_simple_remove(struct device *dev)
--
2.45.1.288.g0e0cd299f1-goog

2024-06-05 00:30:13

by Doug Anderson

[permalink] [raw]

Subject: [PATCH v3 21/24] drm/panel: raydium-rm67191: Stop tracking enabled

2024-06-05 00:33:16

by Doug Anderson

[permalink] [raw]

Subject: [PATCH v3 15/24] drm/panel: seiko-43wvf1g: Stop tracking prepared/enabled

2024-06-11 07:56:53

by Neil Armstrong

[permalink] [raw]

Subject: Re: [PATCH v3 00/24] drm/panel: Remove most store/double-check of prepared/enabled state

Hi,

On 05/06/2024 02:22, Douglas Anderson 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.
>
> In response to my V1 series [1] we had much discussion of what to
> do. The conclusion was that as long as DRM modeset drivers properly
> called drm_atomic_helper_shutdown() that we should be able to remove
> the explicit shutdown/remove handling in the panel drivers. Most of
> the patches to improve DRM modeset drivers [2] [3] [4] have now
> landed.
>
> In contrast to my V1 series, I broke the V2 series up a lot
> more. Since a few of the panel drivers in V1 already landed, we had
> fewer total drivers and so we could devote a patch to each panel.
> Also, since we were now relying on DRM modeset drivers I felt like we
> should split the patches for each panel into two: one that's
> definitely safe and one that could be reverted if we found a
> problematic DRM modeset driver that we couldn't fix.
>
> Many of the patches in the V2 series [5] landed, so this V3 series is
> the patches that are left plus one new bonus patch. At this point, we
> may want to just land the patches that are left since it seems like
> nobody is going to test/review them and they've all been Acked by
> Linus and Maxime.
>
> NOTE: this touches _a lot_ of drivers, is repetitive, and is not
> really possible to generate automatically. That means it's entirely
> possible that my eyes glazed over and I did something wrong. Please
> double-check me and don't assume that I got everything perfect, though
> I did my best. 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.
>
> [1] https://lore.kernel.org/r/20230804140605.RFC.4.I930069a32baab6faf46d6b234f89613b5cec0f14@changeid
> [2] https://lore.kernel.org/r/[email protected]
> [3] https://lore.kernel.org/r/[email protected]
> [4] https://lore.kernel.org/r/[email protected]
> [5] https://lore.kernel.org/r/[email protected]/
>
> Changes in v3:
> - drm/panel: Avoid warnings w/ panel-simple/panel-edp at shutdown
>
> Changes in v2:
> - Only handle 1 panel per patch.
> - Split removal of prepared/enabled from handling of remove/shutdown.
>
> Douglas Anderson (24):
> drm/panel: boe-himax8279d: Stop tracking prepared/enabled
> drm/panel: boe-himax8279d: Don't call unprepare+disable at
> shutdown/remove
> drm/panel: khadas-ts050: Stop tracking prepared/enabled
> drm/panel: khadas-ts050: Don't call unprepare+disable at
> shutdown/remove
> drm/panel: olimex-lcd-olinuxino: Stop tracking prepared/enabled
> drm/panel: olimex-lcd-olinuxino: Don't call unprepare+disable at
> remove
> drm/panel: osd-osd101t2587-53ts: Stop tracking prepared/enabled
> drm/panel: osd-osd101t2587-53ts: Don't call unprepare+disable at
> shutdown/remove
> drm/panel: tdo-tl070wsh30: Stop tracking prepared
> drm/panel: tdo-tl070wsh30: Don't call unprepare+disable at
> shutdown/remove
> drm/panel: jdi-lt070me05000: Stop tracking prepared/enabled
> drm/panel: jdi-lt070me05000: Don't call disable at shutdown/remove
> drm/panel: panasonic-vvx10f034n00: Stop tracking prepared/enabled
> drm/panel: panasonic-vvx10f034n00: Don't call disable at
> shutdown/remove
> drm/panel: seiko-43wvf1g: Stop tracking prepared/enabled
> drm/panel: seiko-43wvf1g: Don't call disable at shutdown/remove
> drm/panel: sharp-lq101r1sx01: Stop tracking prepared/enabled
> drm/panel: sharp-lq101r1sx01: Don't call disable at shutdown/remove
> drm/panel: sharp-ls043t1le01: Stop tracking prepared
> drm/panel: sharp-ls043t1le01: Don't call disable at shutdown/remove
> drm/panel: raydium-rm67191: Stop tracking enabled
> drm/panel: raydium-rm67191: Don't call unprepare+disable at shutdown
> drm/panel: Update TODO list item for cleaning up prepared/enabled
> tracking
> drm/panel: Avoid warnings w/ panel-simple/panel-edp at shutdown
>
> Documentation/gpu/todo.rst | 47 +++---
> drivers/gpu/drm/drm_panel.c | 12 ++
> drivers/gpu/drm/panel/panel-boe-himax8279d.c | 40 -----
> .../gpu/drm/panel/panel-drm-shutdown-check.h | 151 ++++++++++++++++++
> drivers/gpu/drm/panel/panel-edp.c | 19 +--
> .../gpu/drm/panel/panel-jdi-lt070me05000.c | 35 ----
> drivers/gpu/drm/panel/panel-khadas-ts050.c | 39 -----
> .../drm/panel/panel-olimex-lcd-olinuxino.c | 44 -----
> .../drm/panel/panel-osd-osd101t2587-53ts.c | 41 +----
> .../drm/panel/panel-panasonic-vvx10f034n00.c | 47 +-----
> drivers/gpu/drm/panel/panel-raydium-rm67191.c | 26 ---
> drivers/gpu/drm/panel/panel-seiko-43wvf1g.c | 49 ------
> .../gpu/drm/panel/panel-sharp-lq101r1sx01.c | 63 +-------
> .../gpu/drm/panel/panel-sharp-ls043t1le01.c | 24 ---
> drivers/gpu/drm/panel/panel-simple.c | 19 +--
> drivers/gpu/drm/panel/panel-tdo-tl070wsh30.c | 23 ---
> 16 files changed, 199 insertions(+), 480 deletions(-)
> create mode 100644 drivers/gpu/drm/panel/panel-drm-shutdown-check.h
>

Reviewed-by: Neil Armstrong <[email protected]>

for all patches, let me apply all but the last one and let us a more
week to review it. Could you resend it as standalone patch?

Thanks,
Neil

2024-06-11 08:26:21

by Neil Armstrong

[permalink] [raw]

Subject: Re: (subset) [PATCH v3 00/24] drm/panel: Remove most store/double-check of prepared/enabled state