On Tue, Sep 05, 2023 at 09:45:49AM -0700, Doug Anderson wrote: > > > 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. > > As per above, I think this already happens with the boolean? Won't you > see an error message like this: > > dev_warn(panel->dev, "Skipping unprepare of already unprepared panel\n"); > > ...from drm_panel_unprepare() Urgh. I missed that part, sorry for dragging you into that refcounting discussion then. > > > > > 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. > > I'm still confused here too. The next patch in this patch series here > that we're talking about is: > > https://lore.kernel.org/dri-devel/20230804140605.RFC.5.Icc3238e91bc726d4b04c51a4acf67f001ec453d7@changeid/ > > Let's look at an arbitrary concrete part of that patch: the > modification to "panel-tdo-tl070wsh30.c". For that panel, my RFC patch > removed the tracking of "prepared" and and then did this: > > @@ -220,16 +209,14 @@ static void tdo_tl070wsh30_panel_remove(struct > mipi_dsi_device *dsi) > dev_err(&dsi->dev, "failed to detach from DSI host: %d\n", err); > > drm_panel_remove(&tdo_tl070wsh30->base); > - drm_panel_disable(&tdo_tl070wsh30->base); > - drm_panel_unprepare(&tdo_tl070wsh30->base); > + drm_panel_helper_shutdown(&tdo_tl070wsh30->base); > } > > static void tdo_tl070wsh30_panel_shutdown(struct mipi_dsi_device *dsi) > { > struct tdo_tl070wsh30_panel *tdo_tl070wsh30 = mipi_dsi_get_drvdata(dsi); > > - drm_panel_disable(&tdo_tl070wsh30->base); > - drm_panel_unprepare(&tdo_tl070wsh30->base); > + drm_panel_helper_shutdown(&tdo_tl070wsh30->base); > } > > As per our discussion, the plan is to send a V2 of my patch series > where I _don't_ create the call drm_panel_helper_shutdown() and thus I > won't call it. That means that the V2 of the patch for > "panel-tdo-tl070wsh30.c" will remove the calls to drm_panel_disable() > and drm_panel_unprepare() and not replace them with anything. Right, that's what we should do. > As per our discussion, in V2 we will make drm_panel_remove() actually > unprepare / disable a panel if it was left enabled. This would > essentially fold in the drm_panel_helper_shutdown() from my RFC patch. > This would make tdo_tl070wsh30_panel_remove() behave the same as it > did before. Not really? You would get a warning from drm_panel_remove(), but our discussion very much implied still disabling it... > Ugh, though I may have to think about this more when I get to > implementation since I don't think there's a guarantee of the ordering > of shutdown calls between the DRM driver and the panel. Anyway, > something to discuss later. > > However... tdo_tl070wsh30_panel_shutdown() will no longer power the > panel off properly _unless_ the main DRM driver properly calls > drm_atomic_helper_shutdown(). ... with the expectation that all KMS drivers should call drm_atomic_helper_shutdown() anyway (thanks to your other series) and thus we wouldn't trigger that remove warning anyway. > Did I get anything above incorrect? Assuming I got it correct, that > means that our choices are: > > a) Block landing the change to "panel-tdo-tl070wsh30.c" until after > all drivers properly call drm_atomic_helper_shutdown(). I don't think we care about all drivers here. Only the driver it's enabled with would be a blocker. Like, let's say sun4i hasn't been converted but that panel is only used with rockchip anyway, we don't really care that sun4i shutdown patch hasn't been merged (yet). > b) Add a hacky call to drm_panel_remove() in > tdo_tl070wsh30_panel_shutdown() to get the old behavior. This would > work, but IMO it's ugly and I'm pretty strongly against it. Yeah, it's ugly. > c) Ignore the regression and just say that this panel won't get power > sequenced properly until its DRM driver is updated. I'm strongly > against this. That would work too, but the first one looks like the best trade-off to me. Maxime
Attachment:
signature.asc
Description: PGP signature
