Message-ID: <ZnWkcodVWXe7gPVa@phenom.ffwll.local>
Date: Fri, 21 Jun 2024 18:04:02 +0200
From: Daniel Vetter <daniel@...ll.ch>
To: Doug Anderson <dianders@...omium.org>
Cc: 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, Daniel Vetter <daniel@...ll.ch>
Subject: Re: [PATCH] drm/panel: Avoid warnings w/ panel-simple/panel-edp at
 shutdown

On Tue, Jun 18, 2024 at 04:49:22PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Mon, Jun 17, 2024 at 7:17 AM Daniel Vetter <daniel@...ll.ch> wrote:
> >
> > > 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.
> >
> > Can't we just flip the warnings around? Like make the hacky cleanup
> > conditional on the panel not yet being disabled/cleaned up, and complain
> > in that case only. With that:
> > - drivers which call shutdown shouldn't get a warning anymore, and also
> >   not a surplus call to drm_panel_disable/unprepare
> > - drivers which are broken still get the cleanup calls
> > - downside: we can't enforce this, because it's not yet enforced through
> >   device_link ordering
> 
> I feel like something is getting lost in the discussion here. I'm just
> not sure where to put the hacky cleanup without either having a list
> like I have or fixing the device link problem so that the DRM device
> shutdown gets called before the panel. Specifically, right now I think
> it's possible for the panel's shutdown() callback to happen before the
> DRM Device's shutdown(). Thus, we have:
> 
> 1. Panel shutdown() checks and says "it's not shutdown yet so do my
> hacky cleanup."
> 2. DRM device shutdown() gets called and tries to cleanup again.
> 
> ...and thus in step #1 we can't detect a broken DRM device. What am I missing?

The above is a broken drm driver, because shutting down something that the
main drm driver needs _before_ it is shut down itself is broken. That's
why we need the device link.

So if this case goes a bit wrong that's imo ok. That was my point that
without device links, we cannot have _any_ warning at all, but we can at
least make sure that correct drivers, meaning:
- they make sure the panel is around for longer than the drm device
- and they call drm atomic_helper_shutdown in the right places

Wont either double-shutdown the panel or get the warning.

It's not great, but it's at least better than the current situation where
correct drivers get a warning, and some broken drivers don't. So still an
improvement.

That leaves us with the issue of warning for all broken drivers. We have
two proposals for that:

- Your explicit list, which is a pain imo, and I'm not seeing the benefit
  of this, because it'll encourage each driver to hack around the core
  code bug of not having the right device links.

- Fixing this with a device link and adding the warning for everyone.

This isn't a great situation, because it's likely going to be another few
years without the device link situation sorted out. But that's been the
case already for years so *shrug*.

> > > 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.
> >
> > My issue is that you're trading an ugly warning that hurts maintenance
> > with an explicit list of working drivers, which also hurts maintenance.
> > Does seem like forward progress to me, just pushing the issue around.
> 
> IMO it at least moves things forward. If we make the warning obvious
> enough then I think we could assert that, within a few kernel
> versions, everyone who hit the warning would have addressed it. Once
> that happens we can fully get rid of the ugly list and just make the
> assumption that things are good. That feels like a clear path to me...

Yeah, but your warning I think just encourages more hacks in drivers that
shouldn't be there (for the ordering issue). So I'm not sure it's strictly
better.

And we have _tons_ of drm driver api misuse that we don't catch with
warnings and checks. Sometimes that's just not possible, because the
situation is too messy. If we add an explicit "I'm not broken" list for
each such case, we will have an unmaintainable mess. Sometimes a "I'm the
last broken driver" flag makes sense, but even there I'm cautious that
it's a bright idea.

Cheers, Sima
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists