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

[Maxime Ripard]

On Thu, Aug 31, 2023 at 11:18:49AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Thu, Aug 31, 2023 at 12:38 AM Maxime Ripard <mripard@xxxxxxxxxx> wrote:
> >
> > If so, then I think we can implement everything by doing something like:
> >
> >   - Implement enable and disable refcounting in panels.
> >     drm_panel_prepare and drm_panel_enable would increase it,
> >     drm_panel_unprepare and drm_panel_disable would decrease it.
> >
> >   - Only actually call the disable and unprepare functions when that
> >     refcount goes to 0 in the normal case.
> 
> Just to be clear: by refcounting do you mean switching this to actual
> refcount?

Yes

> Today this is explicitly _not_ refcounting, right? It is simply
> treating double-enables as no-ops and double-disables as no-ops. With
> our current understanding, the only thing we actually need to guard
> against is double-disable but at the moment we do guard against both.
> Specifically we believe the cases that are issues:
> 
> a) At shutdown/remove time we want to disable the panel, but only if
> it was enabled (we wouldn't want to call disable if the panel was
> already off because userspace turned it off).

Yeah, and that's doable with refcounting too.

> b) At shutdown time we want to disable the panel but then we don't
> want to double-disable if the main DRM driver also causes us to get
> disabled.

That's what I want to prevent though. Eventually, I don't want to see
panels call drm_panel_unprepare/disable themselves. There's no need to
if all drivers implement the shutdown sequence properly.

> I'd rather keep it the way it is (prevent double-disable) and not
> switch it to a refcount.
>
> I'll also note that drm_panel currently already keeps track of the
> enabled/prepared state, so there's no "implement" step here, right?
> That's what landed in commit d2aacaf07395 ("drm/panel: Check for
> already prepared/enabled in drm_panel"). Just to remind ourselves of
> the history:
> 
> 1. I needed to keep track of the "prepared" state anyway to make the
> "panel follower" API work properly. See drm_panel_add_follower() where
> we immediately power on a follower if the panel they're following was
> already prepared.
> 
> 2. Since I was keeping track of the "prepared" state in the core
> anyway, it seemed like a good idea to prevent
> double-prepare/unprepare/enable/disable in the drm_panel core so that
> we could remove it from individual panels since that was always a
> point of contention in individual panels. It was asserted that, even
> in the core, we shouldn't need code to prevent
> double-prepare/unprepare/enable/disable but that as a first step
> moving this to the core and out of drivers made sense. Anyone relying
> on the core would get a warning printed out indicating that they were
> doing something wrong and this would eventually move to a WARN_ON.
> 
> 3. While trying to remove this from the drivers we ended up realizing
> some of the issues with panels wanting to power them off at shutdown /
> remove time.

Yes, I'm aware. It's not clear to me what you're confused about though.
Is there anything I said that would conflict with that?

> >   - In drm_panel_remove, if we still have a refcount > 0, then we WARN
> >     and force unprepare/disable the panel.
> 
> I think the net-net of this is that you're saying I should fold the
> code from this patch straight into drm_panel_remove() and add a WARN
> if it ever triggers, right?

Aside from the refcounting-or-not discussion, yes.

> In this patch series I'd remove the calls to
> drm_panel_helper_shutdown() and rely on drm_panel_remove() to do the
> power off at remove time.

Yep

> This might give a warning but, as discussed, removing a panel like
> this isn't likely to work well and at least we'd power sequence it
> properly. In some cases, I might have to move the call to
> drm_panel_remove() around a little bit but I think it'll work.

Yep

> The above solves the problem with panels wanting to power sequence
> themselves at remove() time, but not at shutdown() time. Thus we'd
> still have a dependency on having all drivers use
> drm_atomic_helper_shutdown() so that work becomes a dependency.

Does it? I think it can be done in parallel?

Maxime

Attachment: signature.asc
Description: PGP signature