Received-SPF: pass (google.com: domain of linux-kernel+bounces-211722-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240611074846.1.Ieb287c2c3ee3f6d3b0d5f49b29f746b93621749c@changeid>
 <ZmljNHteJ9L5EdE9@phenom.ffwll.local>
In-Reply-To: <ZmljNHteJ9L5EdE9@phenom.ffwll.local>
From: Doug Anderson <dianders@chromium.org>
Date: Wed, 12 Jun 2024 07:49:31 -0700
Message-ID: <CAD=FV=V4C1AYVqG4gig+SiQr4n_mAPVASxneDDZT1a=7AY3Hzw@mail.gmail.com>
Subject: Re: [PATCH] drm/panel: Avoid warnings w/ panel-simple/panel-edp at shutdown
To: Douglas Anderson <dianders@chromium.org>, dri-devel@lists.freedesktop.org, 
	Neil Armstrong <neil.armstrong@linaro.org>, Maxime Ripard <mripard@kernel.org>, 
	Linus Walleij <linus.walleij@linaro.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
Cc: Daniel Vetter <daniel@ffwll.ch>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,

On Wed, Jun 12, 2024 at 1:58=E2=80=AFAM Daniel Vetter <daniel@ffwll.ch> wro=
te:
>
> On Tue, Jun 11, 2024 at 07:48:51AM -0700, Douglas Anderson wrote:
> > At shutdown if you've got a _properly_ coded DRM modeset driver then
> > you'll get these two warnings at shutdown time:
> >
> >   Skipping disable of already disabled panel
> >   Skipping unprepare of already unprepared panel
> >
> > These warnings are ugly and sound concerning, but they're actually a
> > sign of a properly working system. That's not great.
> >
> > It's not easy to get rid of these warnings. Until we know that all DRM
> > modeset drivers used with panel-simple and panel-edp are properly
> > calling drm_atomic_helper_shutdown() or drm_helper_force_disable_all()
> > then the panel drivers _need_ to disable/unprepare themselves in order
> > to power off the panel cleanly. However, there are lots of DRM modeset
> > drivers used with panel-edp and panel-simple and it's hard to know
> > when we've got them all. Since the warning happens only on the drivers
> > that _are_ updated there's nothing to encourage broken DRM modeset
> > drivers to get fixed.
> >
> > In order to flip the warning to the proper place, we need to know
> > which modeset drivers are going to shutdown properly. Though ugly, do
> > this by creating a list of everyone that shuts down properly. This
> > allows us to generate a warning for the correct case and also lets us
> > get rid of the warning for drivers that are shutting down properly.
> >
> > Maintaining this list is ugly, but the idea is that it's only short
> > term. Once everyone is converted we can delete the list and call it
> > done. The list is ugly enough and adding to it is annoying enough that
> > people should push to make this happen.
> >
> > Implement this all in a shared "header" file included by the two panel
> > drivers that need it. This avoids us adding an new exports while still
> > allowing the panel drivers to be modules. The code waste should be
> > small and, as per above, the whole solution is temporary.
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > I came up with this idea to help us move forward since otherwise I
> > couldn't see how we were ever going to fix panel-simple and panel-edp
> > since they're used by so many DRM Modeset drivers. It's a bit ugly but
> > I don't hate it. What do others think?
>
> I think it's terrible :-)

Well, we're in agreement. It is terrible. However, in my opinion it's
still a reasonable way to move forward.


> Why does something like this now work?
>
> drm_panel_shutdown_fixup(panel)
> {
>         /* if you get warnings here, fix your main drm driver to call
>          * drm_atomic_helper_shutdown()
>          */
>         if (WARN_ON(panel->enabled))
>                 drm_panel_disable(panel);
>
>         if (WARN_ON(panel->prepared))
>                 drm_panel_unprepare(panel);
> }
>
> And then call that little helper in the relevant panel drivers? Also feel
> free to bikeshed the name and maybe put a more lengthly explainer into th=
e
> kerneldoc for that ...
>
> Or am I completely missing the point here?

The problem is that the ordering is wrong, I think. Even if the OS was
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.

:-P

Certainly if I goofed and the above is wrong then let me know--I did
my experiments on this many months ago and didn't try repeating them
again now.

In any case, the only way I could figure out around this was some sort
of list. As mentioned in the commit message, it's super ugly and
intended to be temporary. Once we solve all the current in-tree
drivers, I'd imagine that long term for new DRM drivers this kind of
thing would be discovered during bringup of new boards. Usually during
bringup of new boards EEs measure timing signals and complain if
they're not right. If some EE cared and said we weren't disabling the
panel correctly at shutdown time then we'd know there was a problem.

-Doug