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