Received-SPF: pass (google.com: domain of linux-kernel+bounces-211996-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240611074846.1.Ieb287c2c3ee3f6d3b0d5f49b29f746b93621749c@changeid>
 <ZmljNHteJ9L5EdE9@phenom.ffwll.local> <CAD=FV=V4C1AYVqG4gig+SiQr4n_mAPVASxneDDZT1a=7AY3Hzw@mail.gmail.com>
 <Zmm6i6iQOdP613w3@phenom.ffwll.local> <CACRpkdbb5OdizDLSRW3bFEJJhrQ7Fs8Pb=Q2yxBog62Z1m-bOQ@mail.gmail.com>
In-Reply-To: <CACRpkdbb5OdizDLSRW3bFEJJhrQ7Fs8Pb=Q2yxBog62Z1m-bOQ@mail.gmail.com>
From: Doug Anderson <dianders@chromium.org>
Date: Wed, 12 Jun 2024 10:22:49 -0700
Message-ID: <CAD=FV=Vm==ngDo_bZ+xqV4Ojj0SSOO3ZWaxAbWA906h6-e8FMA@mail.gmail.com>
Subject: Re: [PATCH] drm/panel: Avoid warnings w/ panel-simple/panel-edp at shutdown
To: Linus Walleij <linus.walleij@linaro.org>
Cc: dri-devel@lists.freedesktop.org, 
	Neil Armstrong <neil.armstrong@linaro.org>, Maxime Ripard <mripard@kernel.org>, 
	Yuran Pereira <yuran.pereira@hotmail.com>, Chris Morgan <macromorgan@hotmail.com>, 
	David Airlie <airlied@gmail.com>, Jessica Zhang <quic_jesszhan@quicinc.com>, 
	Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Thomas Zimmermann <tzimmermann@suse.de>, 
	linux-kernel@vger.kernel.org, Saravana Kannan <saravanak@google.com>, 
	Daniel Vetter <daniel@ffwll.ch>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Wed, Jun 12, 2024 at 9:47=E2=80=AFAM Linus Walleij <linus.walleij@linaro=
.org> wrote:
>
> On Wed, Jun 12, 2024 at 5:11=E2=80=AFPM Daniel Vetter <daniel@ffwll.ch> w=
rote:
> > On Wed, Jun 12, 2024 at 07:49:31AM -0700, Doug Anderson wrote:
> (...)
> > > The problem is that the ordering is wrong, I think. Even if the OS wa=
s
> > > calling driver shutdown functions in the perfect order (which I'm not
> > > convinced about since panels aren't always child "struct device"s of
> > > the DRM device), the OS should be calling panel shutdown _before_
> > > shutting down the DRM device. That means that with your suggestion:
> > >
> > > 1. Shutdown starts and panel is on.
> > >
> > > 2. OS calls panel shutdown call, which prints warnings because panel
> > > is still on.
> > >
> > > 3. OS calls DRM driver shutdown call, which prints warnings because
> > > someone else turned the panel off.
> >
> > Uh, that's a _much_ more fundamental issue.
> >
> > The fix for that is telling the driver core about this dependency with
> > device_link_add. Unfortuantely, despite years of me trying to push for
> > this, drm_bridge and drm_panel still don't automatically add these,
> > because the situation is a really complex mess.
> >
> > Probably need to read dri-devel archives for all the past attempts arou=
nd
> > device_link_add.
>
> I think involving Saravana Kannan in the discussions around this
> is the right thing to do, because he knows how to get devicelinks
> to do the right thing.
>
> If we can describe what devicelink needs to do to get this ordering
> right, I'm pretty sure Saravana can tell us how to do it.

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.

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

In any case, I guess I'll continue asserting that I'm not going to try
to solve this problem. If folks don't like my patch and there's no
suggestion other than solving years-old problems then I'm happy to
live with the way things are and hope that someone eventually comes
along and solves it.

-Doug