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