Hi, On Fri, Sep 01, 2023 at 06:42:42AM -0700, Doug Anderson wrote: > On Fri, Sep 1, 2023 at 1:15 AM Maxime Ripard <mripard@xxxxxxxxxx> wrote: > > On Thu, Aug 31, 2023 at 11:18:49AM -0700, Doug Anderson wrote: > > > 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. > > 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. And that's exactly because there isn't a legitimate case for things to be disabled or unprepared twice, but yet many panel driver do it anyway. > 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. > > > 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. Maxime
