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
