Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper

From: Doug Anderson <dianders@chromium.org>
To: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>,
	linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org
Subject: Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper
Date: Mon, 28 Aug 2023 09:06:29 -0700	[thread overview]
Message-ID: <CAD=FV=XUhzguFCC=aKzHFMV0bBnZzkHXP_tx+P=PNkVr=8SnTA@mail.gmail.com> (raw)
In-Reply-To: <lhd6ai7d6swlxhisjhikytguor7pptrymo3bmfwej4k7zqrnv4@hp2gvhw7mh3m>

Hi,

On Mon, Aug 28, 2023 at 12:45 AM Maxime Ripard <mripard@kernel.org> wrote:
>
> > For removal I'd be fine with just dropping the call and saying it's
> > the responsibility of the driver to call drm_atomic_helper_shutdown(),
> > as you suggest. I'd tend to believe that removal of DRM drivers is not
> > used anywhere in "production" code (or at least not common) and I
> > think it's super hard to get it right, to unregister and unbind all
> > the DRM components in the right order.
>
> It depends on the kind of devices. USB devices are very likely to be
> removed, platform devices very unlikely, and PCIe cards somewhere in the
> middle :)

Good point. Obviously those cases need to work. I guess I've just
spent lots of time on the SoC case where all the pieces coming
together are very intertwined with each other...

> > Shutdown is called any time you reboot a device. That means that if a
> > DRM driver is _not_ calling drm_atomic_helper_shutdown() on the
> > panel's behalf at shutdown time then the panel won't be powered off
> > properly. This feels to me like something that might actually matter.
>
> It does matter. What I disagree on is that you suggest working around
> that brokenness in the core framework. What I'm saying is driver is
> broken, we should keep the core framework sane and fix it in the driver.
>
> It should be fairly easy with a coccinelle script to figure out which
> panels are affected, and to add that call in remove.

I think I'm confused here. I've already figured out which panels are
affected in my patch series, right? It's the set of panels that today
try to power the panel off in their shutdown call, right? ...but I
think we can't add the call you're suggesting,
drm_atomic_helper_shutdown(), to the _panel_'s shutdown callback, can
we? We need to add it to the shutdown callback of the top-level DRM
driver, right?

> > Panels tend to be one of those things that really care about their
> > power sequencing and can even get damaged (or not turn on properly
> > next time) if sequencing is not done properly, so just removing this
> > code and putting the blame on the DRM driver seems scary to me.
>
> Sure, it's bad. But there's no difference compared to the approach you
> suggest in that patch: you created a helper, yes, but every driver will
> still have to call that helper and if they don't, the panel will still
> be called and it's a bug. And we would have to convert everything to
> that new helper.
>
> It's fundamentally the same discussion than what you were opposed to
> above.

I think the key difference here is that, if I understand correctly,
drm_atomic_helper_shutdown() needs to be added to the top-level DRM
driver, not to the panel itself. I guess I'm worried that I'll either
miss a case or that simply adding a call to
drm_atomic_helper_shutdown() in the top-level DRM driver will break
something. Well, I suppose I can try it and see what happens...

I'll try to cook up a v2 and we'll see what people say. I might keep
the RFC tag for v2 just because I expect it to still be touching a lot
of stuff.

-Doug