Date: Fri, 21 Jun 2024 18:14:28 +0200
From: Daniel Vetter <daniel@...ll.ch>
To: Doug Anderson <dianders@...omium.org>
Cc: Linus Walleij <linus.walleij@...aro.org>,
	dri-devel@...ts.freedesktop.org,
	Neil Armstrong <neil.armstrong@...aro.org>,
	Maxime Ripard <mripard@...nel.org>,
	Yuran Pereira <yuran.pereira@...mail.com>,
	Chris Morgan <macromorgan@...mail.com>,
	David Airlie <airlied@...il.com>,
	Jessica Zhang <quic_jesszhan@...cinc.com>,
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
	Thomas Zimmermann <tzimmermann@...e.de>,
	linux-kernel@...r.kernel.org,
	Saravana Kannan <saravanak@...gle.com>,
	Daniel Vetter <daniel@...ll.ch>
Subject: Re: [PATCH] drm/panel: Avoid warnings w/ panel-simple/panel-edp at
 shutdown

On Tue, Jun 18, 2024 at 04:49:32PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Mon, Jun 17, 2024 at 7:22 AM Daniel Vetter <daniel@...ll.ch> wrote:
> >
> > > I'm really not convinced that hacking with device links in order to
> > > get the shutdown notification in the right order is correct, though.
> > > The idea is that after we're confident that all DRM modeset drivers
> > > are calling shutdown properly that we should _remove_ any code
> > > handling shutdown from panel-edp and panel-simple. They should just
> > > get disabled as part of DRM's shutdown. That means that if we messed
> > > with devicelinks just to get a different shutdown order that it was
> > > just for a short term thing.
> >
> > The device links would allow us to add consistency checks to the panel
> > code to make sure that the shutdown has already happened.
> >
> > And we do kinda need the device ordering still, because if they're shut
> > down in the wrong order the panel might lose it's power already, before
> > the drm driver had a chance to have the last chat with it. Only relevant
> > for non-dumb panels like dsi, but there's cases.
> 
> My impression is that we shouldn't be relying on the driver-level
> "shutdown" call at all but should instead be relying on DRM core to
> call us at the right times. I get this impression based on the fact
> that panel drivers are encouraged _not_ to implement a shutdown()
> callback but instead to rely on the DRM driver to do an orderly power
> off of things (like via drm_atomic_helper_shutdown()) at shutdown
> time.
> 
> I would also tend to say that for suspend/resume that things are more
> complicated than the driver model can really account for, which again
> is why DRM devices have prepare() and enable() phases with complicated
> rules about the ordering that the bridge chain runs those functions
> in.
> 
> Said another way, I believe I could re-phrase your paragraph above and
> say the exact opposite and it would be just as true:
> 
> And we do kinda need the device ordering still, because if they're
> shut down in the wrong order then the DRM device might lose its power
> already, before the panel driver has a chance to use it to send a few
> last commands to the panel.
> 
> ...but that would imply the exact opposite ordering. The issue is that
> there are established rules for the order things are powered on and
> off and those rules are encoded in the orders that prepare() and
> enable() are called. Trying to replicate these rules in the driver
> model just seems like something destined to fail and probably causes
> everyone who attempts this to give up.

Both is true.

a) panels need a lot more ordering than what the pm core provides

b) we still need to make sure that all the _other_ things the pm core
manages (like power/clock domains) don't get shut down too early

If the panel is gone, there's no point in the drm driver calling unprepare
and disable in the right order, the thing is off already.

So yeah defacto this means:
- panel and bridge drivers should _not_ put anything that manages drm kms
  state into their pm callbacks. None of them. The only stuff they can do
  in there is essentially undo anything they do in ->probe. But then I
  have questions why they're wasting power even when not in use ...

- drm driver must be in charge of the entire sequence, and the
  panel/bridge drivers must do any pm management in their kms callbacks

- but we still need to make sure that the pm core doesn't shut down the
  panel/bridges too early, because there's parent resources that the pm
  core manages for us (and they might be entirely disjoint from the
  display device, e.g. if your panel is on an i2c bus, then that i2c must
  stay powered on until after the drm device has done the pm handling)

> > > That being said, one could argue that having device links between the
> > > DRM device and the panel is the right thing long term anyway and that
> > > may well be. I guess the issue is that it's not necessarily obvious
> > > how the "parent/child" or "supplier/consumer" relationship works w/
> > > DRM devices, especially panels. My instinct says that the panel
> > > logically is a "child" or "consumer" of the DRM device and thus
> > > inserting the correct long term device link would mean we'd get
> > > shutdown notification in the wrong order. It would be hard to argue
> > > that the panel is the "parent" of a DRM device, but I guess you could
> > > call it a "supplier"? ...but it's also a "consumer" of some other
> > > stuff, like the pixels being output and also (perhaps) the DP AUX bus.
> > > All this complexity is why the DRM framework tends to use its own
> > > logic for things like prepare/enable instead of using what Linux gives
> > > you. I'm sure Saravanah can also tell you about all the crazy device
> > > link circular dependencies that DRM has thrown him through...
> >
> > The panel driver provides the panel, the drm device driver consumes it.
> > I'm not really clear why you want to structure this the other way round, I
> > can't come up with another way that makes sense from a device driver
> > model. And it's device driver model stuff here, not what's really going on
> > at the hardware level when everything is set up.
> 
> ...but, at least on eDP, the DRM device driver provides the DP AUX bus
> and the panel consumes it. This is the reverse order.

Nah, still the same order like in a more separate panel driver:

- Whatever physical bus driver the panel sits on is the parent bus for the
  panel. Whether that's i2c, spi, dsi or dp aux kinda doesn't matter.

- The panel driver provides the panel for the drm_device. Note that
  drm_device is does have a struct device pointer, but that's just for
  nesting/linking/userspace device enumeration. There's _no_ pm
  relationship between the two

- It's up to the drm driver to figure out how to make sure it's shuts down
  the software drm_device _before_ any of the hardware pieces go away. For
  pci drivers this is usually pretty easy, for anything else you need a
  device link or it'll go wrong in corner cases.

> Perhaps you
> could say that you should have a separate "struct device" for the DP
> AUX bus and the panel consumes the DP AUX bus device but then the DRM
> device consumes the panel?

Yes. And I think for buses like the mipi dsi one we really should have
that. And indeed, we do:

struct mipi_dsi_device {
	struct mipi_dsi_host *host;
	struct device dev;

	...

So it all checks out.

The edp case is special because we don't need per-panel edp drivers,
because it's all specialized. Therefore the full driver model with a
full-blown struct device is overkill. But conceptually, it should be
there.

Cheers, Sima
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists