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

Hi,

On Wed, Jun 12, 2024 at 8:03=E2=80=AFAM Maxime Ripard <mripard@kernel.org> =
wrote:
>
> > > 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 int=
o the
> > > 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.
>
> Based on a discussion we had today With Sima on IRC, I think there's
> another way forward.
>
> We were actually discussing refcount'ing the panels to avoid lifetime
> issues. It would require some API overhaul to have a function to
> allocate the drm_panel structure and init'ing the refcount, plus some to
> get / put the references.
>
> Having this refcount would mean that we also get a release function now,
> called when the panel is free'd.
>
> Could we warn if the panel is still prepared/enabled and is about to be
> freed?
>
> It would require to switch panel-simple and panel-edp to that new API,
> but it should be easy enough.

I think there are two problems here:

1. The problem is at shutdown here. Memory isn't freed at shutdown
time. This isn't a lifetime issue. No release functions are involved
in shutdown and we don't free memory then.

2. As I tried to point out, even if we were guaranteed the correct
order it still doesn't help us. In other words: if all device links
were perfect and all ordering was proper then the panel should get
shutdown _before_ the DRM device. That means we can't put a check in
the panel code to see if the DRM device has been shutdown.


-Doug