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

[Doug Anderson]

Maxime,

On Sun, Aug 6, 2023 at 11:41 PM Maxime Ripard <mripard@xxxxxxxxxx> wrote:
>
> Hi Doug,
>
> Thanks for working on this :)
>
> On Fri, Aug 04, 2023 at 02:06:07PM -0700, Douglas Anderson wrote:
> > The goal of this file is to contain helper functions for panel drivers
> > to use. To start off with, let's add drm_panel_helper_shutdown() for
> > use by panels that want to make sure they're powered off at
> > shutdown/remove time if they happen to be powered on.
> >
> > The main goal of introducting this function is so that panel drivers
> > don't need to track the enabled/prepared state themselves.
> >
> > Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
>
> It shouldn't be necessary at all: drivers should call
> drm_atomic_helper_shutdown at removal time which will disable the
> connector (which in turn should unprepare/disable its panel).
>
> If either the driver is missing drm_atomic_helper_shutdown, or if the
> connector doesn't properly disable the panel, then I would consider that
> a bug.

Hmmm. I'm a bit hesitant here. I guess I'm less worried about the
removal time and more worried about the shutdown time.

For removal I'd be fine with just dropping the call and saying it's
the responsibility of the driver to call drm_atomic_helper_shutdown(),
as you suggest. I'd tend to believe that removal of DRM drivers is not
used anywhere in "production" code (or at least not common) and I
think it's super hard to get it right, to unregister and unbind all
the DRM components in the right order. Presumably anyone trying to
remove a DRM panel in a generic case supporting lots of different
hardware is used to it being a bit broken...  Not that it's a super
great situation to be in for remove() not to work reliably, but that's
how I think it is right now.

For shutdown, however, I'm not really OK with just blindly removing
the code that tries to power off the panel. Shutdown is called any
time you reboot a device. That means that if a DRM driver is _not_
calling drm_atomic_helper_shutdown() on the panel's behalf at shutdown
time then the panel won't be powered off properly. This feels to me
like something that might actually matter. Panels tend to be one of
those things that really care about their power sequencing and can
even get damaged (or not turn on properly next time) if sequencing is
not done properly, so just removing this code and putting the blame on
the DRM driver seems scary to me. Sure enough, a quick survey of DRM
drivers shows that many don't call drm_atomic_helper_shutdown() at
.shutdown time. A _very_ quick skim of callers to
drm_atomic_helper_shutdown():

* omapdrm/omap_drv.c - calls at remove, not shutdown
* arm/hdlcd_drv.c - calls at unbind, not shutdown
* arm/malidp_drv.c - calls at unbind, not shutdown
* armada/armada_drv.c - calls at unbind, not shutdown

...huh, actually, there are probably too many to list that don't call
it at shutdown. There are some that do, but also quite a few that
don't. I'm not sure I really want to blindly add
drm_atomic_helper_shutdown() to all those DRM driver's shutdown
callbacks... That feels like asking for someone to flame me...

...but then, what's the way forward? I think normally the panel's
shutdown() callback would happen _before_ the DRM driver's shutdown()
callback, so we can't easily write logic in the panel's shutdown like
"if the DRM panel didn't shut the panel down then print a warning and
shut down the panel". We'd have to somehow invent and register for a
"late shutdown" callback and have the panel use that to shut itself
down if the DRM driver didn't. That seems like a bad idea...

Do you have any brilliant ideas here? I could keep the function as-is
but only have panels only call it at shutdown time if you want. I
could add to the comments and/or the commit message some summary of
the above and that the call is important for panels that absolutely
need to be powered off at shutdown time even if the DRM driver doesn't
do anything special at shutdown... Any other ideas?


-Doug