Message-ID: <CAD=FV=WBVfBZrgGay=XY2Usq3FA3m9i6y0cU4=b=w7qO6gRBFQ@mail.gmail.com>
Date: Wed, 12 Jun 2024 09:00:29 -0700
From: Doug Anderson <dianders@...omium.org>
To: Doug Anderson <dianders@...omium.org>, dri-devel@...ts.freedesktop.org, 
	Neil Armstrong <neil.armstrong@...aro.org>, Maxime Ripard <mripard@...nel.org>, 
	Linus Walleij <linus.walleij@...aro.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
Cc: Daniel Vetter <daniel@...ll.ch>
Subject: Re: [PATCH] drm/panel: Avoid warnings w/ panel-simple/panel-edp at shutdown

Hi,

On Wed, Jun 12, 2024 at 8:11 AM Daniel Vetter <daniel@...ll.ch> wrote:
>
> > 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.
>
> 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 around
> device_link_add.
>
> But the solution is definitely not to have a manually tracked list, what's
> very architectural unsound way to tackle this problem.
>
> > 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.
>
> Oh the issue is very real and known since years. It also wreaks module
> unload and driver unbinding, since currently nothing makes sure your
> drm_panel lives longer than your drm_device.

In this case I'm mostly worried about the device "shutdown" call, so
it's not quite a lifetime issue but it is definitely related.

As per my reply to Maxime, though, I'd expect that if all ordering
issues were fixed and things were perfect then we'd still have a
problem. Specifically it would seem pretty wrong to me to say that the
panel is the "parent" of the DRM device, right? So if the panel is the
"child" of the DRM device that means it'll get shutdown first and that
means that the panel's shutdown call cannot be used to tell whether
the DRM device's shutdown call behaved properly.


> > 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.
>
> You've stepped into an entire hornets nest with this device dependency
> issue unfortunately, I'm afraid :-/

As you've said, you've been working on this problem for years. Solving
the device link problem doesn't help me, but even if it did it's
really not fundamental to the problem here. The only need is to get a
warning printed out so we know for sure which DRM drivers need to be
updated before deleting the old crufty code. Blocking that on a
difficult / years-long struggle might not be the best.

That all being said, I'm also totally OK with any of the following:

1. Dropping my patch and just accepting that we will have warnings
printed out for all DRM drivers that do things correctly and have no
warnings for broken DRM drivers.

2. Someone else posting / landing a patch to remove the hacky "disable
/ unprepare" for panel-simple and panel-edp and asserting that they
don't care if they break any DRM drivers that are still broken. I
don't want to be involved in authoring or landing this patch, but I
won't scream loudly if others want to do it.

3. Someone else taking over trying to solve this problem.

...mostly this work is janitorial and I'm trying to help move the DRM
framework forward and get rid of cruft, so if it's going to cause too
much conflict I'm fine just stepping back.

-Doug

Powered by blists - more mailing lists