diff mbox series

[RFC,03/10] drm/panel: otm8009a: Don't double check prepared/enabled

Message ID 20230804140605.RFC.3.I6a4a3c81c78acf5acdc2e5b5d936e19bf57ec07a@changeid
State New
Headers
Series drm/panel: Remove most store/double-check of prepared/enabled state |

Commit Message

Doug Anderson Aug. 4, 2023, 9:06 p.m. UTC 
  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(-)

Comments

Doug Anderson Sept. 13, 2023, 6:20 p.m. UTC | #1 
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/
diff mbox series

Patch

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;
 }