Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper

[Doug Anderson]

Hi,

On Mon, Sep 4, 2023 at 8:33 AM Maxime Ripard <mripard@xxxxxxxxxx> 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