[v3,02/10] drm/panel: Check for already prepared/enabled in drm_panel

Commit Message

Doug Anderson July 25, 2023, 8:34 p.m. UTC

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. It's silly to have this code duplicated
everywhere. Add it to the core instead so that we can eventually
delete it from all the drivers. Note: 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

NOTE: arguably, the right thing to do here is actually to skip this
patch and simply remove all the extra checks from the individual
drivers. Perhaps the checks were needed at some point in time in the
past but maybe they no longer are? Certainly as we continue
transitioning over to "panel_bridge" then we expect there to be much
less variety in how these calls are made. When we're called as part of
the bridge chain, things should be pretty simple. In fact, there was
some discussion in the past about these checks [1], including a
discussion about whether the checks were needed and whether the calls
ought to be refcounted. At the time, I decided not to mess with it
because it felt too risky.

Looking closer at it now, I'm fairly certain that nothing in the
existing codebase is expecting these calls to be refcounted. The only
real question is whether someone is already doing something to ensure
prepare()/unprepare() match and enabled()/disable() match. I would say
that, even if there is something else ensuring that things match,
there's enough complexity that adding an extra bool and an extra
double-check here is a good idea. Let's add a drm_warn() to let people
know that it's considered a minor error to take advantage of
drm_panel's double-checking but we'll still make things work fine.

[1] https://lore.kernel.org/r/20210416153909.v4.27.I502f2a92ddd36c3d28d014dd75e170c2d405a0a5@changeid

Acked-by: Neil Armstrong <[e-mail]>
Signed-off-by: Douglas Anderson <[e-mail]>
---
This has Neil's Ack and I could commit it to drm-misc-next, but for
now I'm holding off to see where this series ends up. If the series
ends up looking good we'll have to coordinate landing the various bits
between the drm and the hid trees and the second drm patch in my
series depends on this one.

If my series implodes I'll land this one on its own. In any case, once
this lands somewhere I'll take an AI to cleanup the panels.

(no changes since v1)

 drivers/gpu/drm/drm_panel.c | 49 ++++++++++++++++++++++++++++++++-----
 include/drm/drm_panel.h     | 14 +++++++++++
 2 files changed, 57 insertions(+), 6 deletions(-)

Comments

Maxime Ripard July 26, 2023, 12:41 p.m. UTC | #1

Hi,

On Tue, Jul 25, 2023 at 01:34:37PM -0700, Douglas Anderson wrote:
> NOTE: arguably, the right thing to do here is actually to skip this
> patch and simply remove all the extra checks from the individual
> drivers. Perhaps the checks were needed at some point in time in the
> past but maybe they no longer are? Certainly as we continue
> transitioning over to "panel_bridge" then we expect there to be much
> less variety in how these calls are made. When we're called as part of
> the bridge chain, things should be pretty simple. In fact, there was
> some discussion in the past about these checks [1], including a
> discussion about whether the checks were needed and whether the calls
> ought to be refcounted. At the time, I decided not to mess with it
> because it felt too risky.

Yeah, I'd agree here too. I've never found evidence that it was actually
needed and it really looks like cargo cult to me.

And if it was needed, then I'm not sure we need refcounting either. We
don't have refcounting for atomic_enable / disable, we have a sound API
design that makes sure we don't fall into that trap :)

> Looking closer at it now, I'm fairly certain that nothing in the
> existing codebase is expecting these calls to be refcounted. The only
> real question is whether someone is already doing something to ensure
> prepare()/unprepare() match and enabled()/disable() match. I would say
> that, even if there is something else ensuring that things match,
> there's enough complexity that adding an extra bool and an extra
> double-check here is a good idea. Let's add a drm_warn() to let people
> know that it's considered a minor error to take advantage of
> drm_panel's double-checking but we'll still make things work fine.

I'm ok with this, if we follow-up in a couple of releases and remove it
and all the calls.

Could you add a TODO item so that we can keep a track of it? A follow-up
is fine if you don't send a new version of that series.

Maxime

Doug Anderson July 26, 2023, 3:10 p.m. UTC | #2

Hi,

On Wed, Jul 26, 2023 at 5:41 AM Maxime Ripard <[e-mail]> wrote:
>
> Hi,
>
> On Tue, Jul 25, 2023 at 01:34:37PM -0700, Douglas Anderson wrote:
> > NOTE: arguably, the right thing to do here is actually to skip this
> > patch and simply remove all the extra checks from the individual
> > drivers. Perhaps the checks were needed at some point in time in the
> > past but maybe they no longer are? Certainly as we continue
> > transitioning over to "panel_bridge" then we expect there to be much
> > less variety in how these calls are made. When we're called as part of
> > the bridge chain, things should be pretty simple. In fact, there was
> > some discussion in the past about these checks [1], including a
> > discussion about whether the checks were needed and whether the calls
> > ought to be refcounted. At the time, I decided not to mess with it
> > because it felt too risky.
>
> Yeah, I'd agree here too. I've never found evidence that it was actually
> needed and it really looks like cargo cult to me.
>
> And if it was needed, then I'm not sure we need refcounting either. We
> don't have refcounting for atomic_enable / disable, we have a sound API
> design that makes sure we don't fall into that trap :)
>
> > Looking closer at it now, I'm fairly certain that nothing in the
> > existing codebase is expecting these calls to be refcounted. The only
> > real question is whether someone is already doing something to ensure
> > prepare()/unprepare() match and enabled()/disable() match. I would say
> > that, even if there is something else ensuring that things match,
> > there's enough complexity that adding an extra bool and an extra
> > double-check here is a good idea. Let's add a drm_warn() to let people
> > know that it's considered a minor error to take advantage of
> > drm_panel's double-checking but we'll still make things work fine.
>
> I'm ok with this, if we follow-up in a couple of releases and remove it
> and all the calls.
>
> Could you add a TODO item so that we can keep a track of it? A follow-up
> is fine if you don't send a new version of that series.

By this, I think you mean to add a "TODO" comment inline in the code?

Also: I was thinking that we'd keep the check in "drm_panel.c" with
the warning message indefinitely. You think it should be eventually
removed? If we are truly thinking of removing it eventually, this
feels like it should be a more serious warning message like a WARN(1,
...) to make it really obvious to people that they're relying on
behavior that will eventually go away.


-Doug

Maxime Ripard July 27, 2023, 6:37 a.m. UTC | #3

Hi,

On Wed, Jul 26, 2023 at 08:10:33AM -0700, Doug Anderson wrote:
> On Wed, Jul 26, 2023 at 5:41 AM Maxime Ripard <[e-mail]> wrote:
> > On Tue, Jul 25, 2023 at 01:34:37PM -0700, Douglas Anderson wrote:
> > > NOTE: arguably, the right thing to do here is actually to skip this
> > > patch and simply remove all the extra checks from the individual
> > > drivers. Perhaps the checks were needed at some point in time in the
> > > past but maybe they no longer are? Certainly as we continue
> > > transitioning over to "panel_bridge" then we expect there to be much
> > > less variety in how these calls are made. When we're called as part of
> > > the bridge chain, things should be pretty simple. In fact, there was
> > > some discussion in the past about these checks [1], including a
> > > discussion about whether the checks were needed and whether the calls
> > > ought to be refcounted. At the time, I decided not to mess with it
> > > because it felt too risky.
> >
> > Yeah, I'd agree here too. I've never found evidence that it was actually
> > needed and it really looks like cargo cult to me.
> >
> > And if it was needed, then I'm not sure we need refcounting either. We
> > don't have refcounting for atomic_enable / disable, we have a sound API
> > design that makes sure we don't fall into that trap :)
> >
> > > Looking closer at it now, I'm fairly certain that nothing in the
> > > existing codebase is expecting these calls to be refcounted. The only
> > > real question is whether someone is already doing something to ensure
> > > prepare()/unprepare() match and enabled()/disable() match. I would say
> > > that, even if there is something else ensuring that things match,
> > > there's enough complexity that adding an extra bool and an extra
> > > double-check here is a good idea. Let's add a drm_warn() to let people
> > > know that it's considered a minor error to take advantage of
> > > drm_panel's double-checking but we'll still make things work fine.
> >
> > I'm ok with this, if we follow-up in a couple of releases and remove it
> > and all the calls.
> >
> > Could you add a TODO item so that we can keep a track of it? A follow-up
> > is fine if you don't send a new version of that series.
> 
> By this, I think you mean to add a "TODO" comment inline in the code?

No, sorry, I meant an entry in our TODO list: Documentation/gpu/todo.rst

> Also: I was thinking that we'd keep the check in "drm_panel.c" with
> the warning message indefinitely. You think it should be eventually
> removed? If we are truly thinking of removing it eventually, this
> feels like it should be a more serious warning message like a WARN(1,
> ...) to make it really obvious to people that they're relying on
> behavior that will eventually go away.

Yeah, it really feels like this is cargo-cult to me. Your approach seems
like a good short-term thing to do to warn everyone but eventually we'll
want it to go away.

So promoting it to a WARN could be a good thing, or let's say we do a
drm_warn for now, WARN next release, and gone in two?

Maxime

Chris Morgan July 31, 2023, 4:33 p.m. UTC | #4

In my case a few different panel drivers disable the regulators in the
unprepare/disable routines. For at least the Rockchip DSI
implementations for some reason the panel gets unprepared more than
once, which triggers an unbalanced regulator disable. Obviously though
the correct course of action is to fix the reason why the panel is
disabled more than once, but that's at least the root cause of this
behavior on the few panels I've worked with.

Thank you.

On Thu, Jul 27, 2023 at 1:38 AM Maxime Ripard <[e-mail]> wrote:
>
> Hi,
>
> On Wed, Jul 26, 2023 at 08:10:33AM -0700, Doug Anderson wrote:
> > On Wed, Jul 26, 2023 at 5:41 AM Maxime Ripard <[e-mail]> wrote:
> > > On Tue, Jul 25, 2023 at 01:34:37PM -0700, Douglas Anderson wrote:
> > > > NOTE: arguably, the right thing to do here is actually to skip this
> > > > patch and simply remove all the extra checks from the individual
> > > > drivers. Perhaps the checks were needed at some point in time in the
> > > > past but maybe they no longer are? Certainly as we continue
> > > > transitioning over to "panel_bridge" then we expect there to be much
> > > > less variety in how these calls are made. When we're called as part of
> > > > the bridge chain, things should be pretty simple. In fact, there was
> > > > some discussion in the past about these checks [1], including a
> > > > discussion about whether the checks were needed and whether the calls
> > > > ought to be refcounted. At the time, I decided not to mess with it
> > > > because it felt too risky.
> > >
> > > Yeah, I'd agree here too. I've never found evidence that it was actually
> > > needed and it really looks like cargo cult to me.
> > >
> > > And if it was needed, then I'm not sure we need refcounting either. We
> > > don't have refcounting for atomic_enable / disable, we have a sound API
> > > design that makes sure we don't fall into that trap :)
> > >
> > > > Looking closer at it now, I'm fairly certain that nothing in the
> > > > existing codebase is expecting these calls to be refcounted. The only
> > > > real question is whether someone is already doing something to ensure
> > > > prepare()/unprepare() match and enabled()/disable() match. I would say
> > > > that, even if there is something else ensuring that things match,
> > > > there's enough complexity that adding an extra bool and an extra
> > > > double-check here is a good idea. Let's add a drm_warn() to let people
> > > > know that it's considered a minor error to take advantage of
> > > > drm_panel's double-checking but we'll still make things work fine.
> > >
> > > I'm ok with this, if we follow-up in a couple of releases and remove it
> > > and all the calls.
> > >
> > > Could you add a TODO item so that we can keep a track of it? A follow-up
> > > is fine if you don't send a new version of that series.
> >
> > By this, I think you mean to add a "TODO" comment inline in the code?
>
> No, sorry, I meant an entry in our TODO list: Documentation/gpu/todo.rst
>
> > Also: I was thinking that we'd keep the check in "drm_panel.c" with
> > the warning message indefinitely. You think it should be eventually
> > removed? If we are truly thinking of removing it eventually, this
> > feels like it should be a more serious warning message like a WARN(1,
> > ...) to make it really obvious to people that they're relying on
> > behavior that will eventually go away.
>
> Yeah, it really feels like this is cargo-cult to me. Your approach seems
> like a good short-term thing to do to warn everyone but eventually we'll
> want it to go away.
>
> So promoting it to a WARN could be a good thing, or let's say we do a
> drm_warn for now, WARN next release, and gone in two?
>
> Maxime

Maxime Ripard July 31, 2023, 5:03 p.m. UTC | #5

Hi,

On Mon, Jul 31, 2023 at 11:33:22AM -0500, Chris Morgan wrote:
> In my case a few different panel drivers disable the regulators in the
> unprepare/disable routines.

And that's totally fine.

> For at least the Rockchip DSI implementations for some reason the
> panel gets unprepared more than once, which triggers an unbalanced
> regulator disable.

"For some reason" being that DW-DSI apparently finds it ok to bypass any
kind of abstraction and randomly calling panel functions by itself:

https://elixir.bootlin.com/linux/v6.4.7/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L868

It looks like it's fixed it current drm-misc-next though.

> Obviously though the correct course of action is to fix the reason why
> the panel is disabled more than once, but that's at least the root
> cause of this behavior on the few panels I've worked with.

Like I said we already have a commit on the way to fix that, so it
shouldn't be an issue anymore.

I stand by what I was saying earlier though, I think it's mostly
cargo-cult or drivers being very wrong. If anything, the DW-DSI stuff
made me even more convinced that we shouldn't even entertain that idea
:)

Maxime

Chris Morgan Aug. 2, 2023, 5:25 p.m. UTC | #6

On Mon, Jul 31, 2023 at 07:03:07PM +0200, Maxime Ripard wrote:
> Hi,
> 
> On Mon, Jul 31, 2023 at 11:33:22AM -0500, Chris Morgan wrote:
> > In my case a few different panel drivers disable the regulators in the
> > unprepare/disable routines.
> 
> And that's totally fine.
> 
> > For at least the Rockchip DSI implementations for some reason the
> > panel gets unprepared more than once, which triggers an unbalanced
> > regulator disable.
> 
> "For some reason" being that DW-DSI apparently finds it ok to bypass any
> kind of abstraction and randomly calling panel functions by itself:
> 
> https://elixir.bootlin.com/linux/v6.4.7/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L868
> 
> It looks like it's fixed it current drm-misc-next though.

Good, when I get a chance I will test it out with the existing panels
I have at my disposal and submit some patches to clean them up.

> 
> > Obviously though the correct course of action is to fix the reason why
> > the panel is disabled more than once, but that's at least the root
> > cause of this behavior on the few panels I've worked with.
> 
> Like I said we already have a commit on the way to fix that, so it
> shouldn't be an issue anymore.
> 
> I stand by what I was saying earlier though, I think it's mostly
> cargo-cult or drivers being very wrong. If anything, the DW-DSI stuff
> made me even more convinced that we shouldn't even entertain that idea
> :)
> 
> Maxime

Thank you, and yes if a driver is doing something it shouldn't we
shouldn't be patching around that, we should be fixing things. Thanks
for providing me with the additional info.

Chris

Dave Stevenson Aug. 2, 2023, 5:50 p.m. UTC | #7

On Wed, 2 Aug 2023 at 18:26, Chris Morgan <[e-mail]> wrote:
>
> * Spam *
> On Mon, Jul 31, 2023 at 07:03:07PM +0200, Maxime Ripard wrote:
> > Hi,
> >
> > On Mon, Jul 31, 2023 at 11:33:22AM -0500, Chris Morgan wrote:
> > > In my case a few different panel drivers disable the regulators in the
> > > unprepare/disable routines.
> >
> > And that's totally fine.
> >
> > > For at least the Rockchip DSI implementations for some reason the
> > > panel gets unprepared more than once, which triggers an unbalanced
> > > regulator disable.
> >
> > "For some reason" being that DW-DSI apparently finds it ok to bypass any
> > kind of abstraction and randomly calling panel functions by itself:
> >
> > https://elixir.bootlin.com/linux/v6.4.7/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L868
> >
> > It looks like it's fixed it current drm-misc-next though.
>
> Good, when I get a chance I will test it out with the existing panels
> I have at my disposal and submit some patches to clean them up.
>
> >
> > > Obviously though the correct course of action is to fix the reason why
> > > the panel is disabled more than once, but that's at least the root
> > > cause of this behavior on the few panels I've worked with.
> >
> > Like I said we already have a commit on the way to fix that, so it
> > shouldn't be an issue anymore.
> >
> > I stand by what I was saying earlier though, I think it's mostly
> > cargo-cult or drivers being very wrong. If anything, the DW-DSI stuff
> > made me even more convinced that we shouldn't even entertain that idea
> > :)

DW-DSI is hacking around the fact that DSI panels may want to send DCS
commands in unprepare, however the DSI host driver shuts down the
controller in the DSI bridge post_disable which gets called first.

That ordering can now be reversed with pre_enable_prev_first flag in
struct drm_bridge, or prepare_prev_first in drm_panel, hence no need
for the DSI controller to jump around the bridge chain.

  Dave

> > Maxime
>
> Thank you, and yes if a driver is doing something it shouldn't we
> shouldn't be patching around that, we should be fixing things. Thanks
> for providing me with the additional info.
>
> Chris
>

Patch

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index f634371c717a..4e1c4e42575b 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -105,11 +105,22 @@  EXPORT_SYMBOL(drm_panel_remove);
  */
 int drm_panel_prepare(struct drm_panel *panel)
 {
+	int ret;
+
 	if (!panel)
 		return -EINVAL;
 
-	if (panel->funcs && panel->funcs->prepare)
-		return panel->funcs->prepare(panel);
+	if (panel->prepared) {
+		dev_warn(panel->dev, "Skipping prepare of already prepared panel\n");
+		return 0;
+	}
+
+	if (panel->funcs && panel->funcs->prepare) {
+		ret = panel->funcs->prepare(panel);
+		if (ret < 0)
+			return ret;
+	}
+	panel->prepared = true;
 
 	return 0;
 }
@@ -128,11 +139,22 @@  EXPORT_SYMBOL(drm_panel_prepare);
  */
 int drm_panel_unprepare(struct drm_panel *panel)
 {
+	int ret;
+
 	if (!panel)
 		return -EINVAL;
 
-	if (panel->funcs && panel->funcs->unprepare)
-		return panel->funcs->unprepare(panel);
+	if (!panel->prepared) {
+		dev_warn(panel->dev, "Skipping unprepare of already unprepared panel\n");
+		return 0;
+	}
+
+	if (panel->funcs && panel->funcs->unprepare) {
+		ret = panel->funcs->unprepare(panel);
+		if (ret < 0)
+			return ret;
+	}
+	panel->prepared = false;
 
 	return 0;
 }
@@ -155,11 +177,17 @@  int drm_panel_enable(struct drm_panel *panel)
 	if (!panel)
 		return -EINVAL;
 
+	if (panel->enabled) {
+		dev_warn(panel->dev, "Skipping enable of already enabled panel\n");
+		return 0;
+	}
+
 	if (panel->funcs && panel->funcs->enable) {
 		ret = panel->funcs->enable(panel);
 		if (ret < 0)
 			return ret;
 	}
+	panel->enabled = true;
 
 	ret = backlight_enable(panel->backlight);
 	if (ret < 0)
@@ -187,13 +215,22 @@  int drm_panel_disable(struct drm_panel *panel)
 	if (!panel)
 		return -EINVAL;
 
+	if (!panel->enabled) {
+		dev_warn(panel->dev, "Skipping disable of already disabled panel\n");
+		return 0;
+	}
+
 	ret = backlight_disable(panel->backlight);
 	if (ret < 0)
 		DRM_DEV_INFO(panel->dev, "failed to disable backlight: %d\n",
 			     ret);
 
-	if (panel->funcs && panel->funcs->disable)
-		return panel->funcs->disable(panel);
+	if (panel->funcs && panel->funcs->disable) {
+		ret = panel->funcs->disable(panel);
+		if (ret < 0)
+			return ret;
+	}
+	panel->enabled = false;
 
 	return 0;
 }
diff --git a/include/drm/drm_panel.h b/include/drm/drm_panel.h
index 432fab2347eb..c6cf75909389 100644
--- a/include/drm/drm_panel.h
+++ b/include/drm/drm_panel.h
@@ -198,6 +198,20 @@  struct drm_panel {
 	 * the panel is powered up.
 	 */
 	bool prepare_prev_first;
+
+	/**
+	 * @prepared:
+	 *
+	 * If true then the panel has been prepared.
+	 */
+	bool prepared;
+
+	/**
+	 * @enabled:
+	 *
+	 * If true then the panel has been enabled.
+	 */
+	bool enabled;
 };
 
 void drm_panel_init(struct drm_panel *panel, struct device *dev,