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

[Doug Anderson]

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? 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).

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.

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.


>   - 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? 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. 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.

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.


> > I can't give you an exact list because I don't have a great search
> > that identifies the problem. I'm mostly looking for all instances of
> > drm_dev_register() where the driver is "DRIVER_MODESET" and then
> > manually checking their use of drm_atomic_helper_shutdown(). Until I
> > get through them all I won't know the count, and it's a manual
> > process.
>
> I think my coccinnelle script will match with most of them, but we might
> still miss a few indeed.

I'll just plug through with my original list for now, then I can try
your script when I'm done and see if it catches anything I missed.

-Doug