Message-ID: <Zmm6_27GikpmT3HQ@phenom.ffwll.local>
Date: Wed, 12 Jun 2024 17:13:03 +0200
From: Daniel Vetter <daniel@...ll.ch>
To: Doug Anderson <dianders@...omium.org>
Cc: Maxime Ripard <mripard@...nel.org>, dri-devel@...ts.freedesktop.org,
	Neil Armstrong <neil.armstrong@...aro.org>,
	Linus Walleij <linus.walleij@...aro.org>,
	Yuran Pereira <yuran.pereira@...mail.com>,
	Chris Morgan <macromorgan@...mail.com>,
	Daniel Vetter <daniel@...ll.ch>, 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
Subject: Re: [PATCH] drm/panel: Avoid warnings w/ panel-simple/panel-edp at
 shutdown

On Wed, Jun 12, 2024 at 07:39:01AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Wed, Jun 12, 2024 at 1:09 AM Maxime Ripard <mripard@...nel.org> wrote:
> >
> > Hi,
> >
> > On Tue, Jun 11, 2024 at 07:48:51AM GMT, 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@...omium.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 don't think it's the right approach, even more so since we're so close
> > now to having it in every driver.
> >
> > I ran the coccinelle script we started with, and here are the results:
> >
> > ./drivers/gpu/drm/vmwgfx/vmwgfx_drv.c:1640:25-39: ERROR: KMS driver vmw_pci_driver is missing shutdown implementation
> > ./drivers/gpu/drm/kmb/kmb_drv.c:621:30-49: ERROR: KMS driver kmb_platform_driver is missing shutdown implementation
> > ./drivers/gpu/drm/tiny/arcpgu.c:422:30-52: ERROR: KMS driver arcpgu_platform_driver is missing shutdown implementation
> 
> Sure, although I think we agreed even back when we talked about this
> last that your coccinelle script wasn't guaranteed to catch every
> driver. ...so I guess the question is: are we willing to accept that
> we'll stop disabling panels at shutdown for any drivers that might
> were missed. For instance, looking at it by hand (which also could
> miss things), I previously thought that we also might need:
> 
> * nouveau
> * tegra
> * amdgpu
> * sprd
> * gma500
> * radeon
> 
> I sent patches for those drivers but they don't go through drm-misc
> and some of the drivers had a lot of abstraction layers and were hard
> to reason about. I'm also not 100% confident that all of those drivers
> really are affected--they'd have to be used with panel-simple or
> panel-edp...

Aside from amdgpu and radeon they're all in -misc now, and Alex is
generally fairly responsive.

> In any case, having some sort of warning that would give us a
> definitive answer would be nice. My proposed patch would give us that
> warning. I could even jump to a WARN_ON right from the start.

Yeah we defo want some warning to at least check this at runtime.

> My proposed patch is self-admittedly super ugly and is also designed
> to be temporary, so I don't think of this as giving up right before
> crossing the finish line but instead accepting a tiny bit of temporary
> ugliness to make sure that we don't accidentally regress anyone. I
> would really hope it would be obvious to anyone writing / reviewing
> drivers that the function I introduced isn't intended for anyone but
> panel-simple and panel-edp.

See my other reply for the proper design fix, and my apologies for what
you stepped into here :-/
-Sima
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists