Message-ID: <CAD=FV=WUNcS0uAUro-oXEksHcKMb37kF2NMfd4pE9FahT7jXVA@mail.gmail.com>
Date: Tue, 18 Jun 2024 16:49:32 -0700
From: Doug Anderson <dianders@...omium.org>
To: Doug Anderson <dianders@...omium.org>, 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>
Cc: Daniel Vetter <daniel@...ll.ch>
Subject: Re: [PATCH] drm/panel: Avoid warnings w/ panel-simple/panel-edp at shutdown

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.


> > 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. 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?

-Doug

Powered by blists - more mailing lists