Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper

[Maxime Ripard]

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