linux-kernel - Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 1 Sep 2023 06:42:42 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Maxime Ripard <mripard@...nel.org>
Cc:     dri-devel@...ts.freedesktop.org,
        Linus Walleij <linus.walleij@...aro.org>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...il.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 04/10] drm/panel_helper: Introduce drm_panel_helper

Hi,

On Fri, Sep 1, 2023 at 1:15 AM Maxime Ripard <mripard@...nel.org> wrote:
>
> On Thu, Aug 31, 2023 at 11:18:49AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Aug 31, 2023 at 12:38 AM Maxime Ripard <mripard@...nel.org> wrote:
> > >
> > > If so, then I think we can implement everything by doing something like:
> > >
> > >   - Implement enable and disable refcounting in panels.
> > >     drm_panel_prepare and drm_panel_enable would increase it,
> > >     drm_panel_unprepare and drm_panel_disable would decrease it.
> > >
> > >   - Only actually call the disable and unprepare functions when that
> > >     refcount goes to 0 in the normal case.
> >
> > Just to be clear: by refcounting do you mean switching this to actual
> > refcount?
>
> Yes
>
> > Today this is explicitly _not_ refcounting, right? It is simply
> > treating double-enables as no-ops and double-disables as no-ops. With
> > our current understanding, the only thing we actually need to guard
> > against is double-disable but at the moment we do guard against both.
> > Specifically we believe the cases that are issues:
> >
> > a) At shutdown/remove time we want to disable the panel, but only if
> > it was enabled (we wouldn't want to call disable if the panel was
> > already off because userspace turned it off).
>
> Yeah, and that's doable with refcounting too.

I don't understand the benefit of switching to refcounting, though. We
don't ever expect the "prepare" or "enable" function to be called more
than once and all we're guarding against is a double-unprepare and a
double-enable. Switching this to refcounting would make the reader
think that there was a legitimate case for things to be prepared or
enabled twice. As far as I know, there isn't.

In any case, I don't think there's any need to switch this to
refcounting as part of this effort. Someone could, in theory, do it as
a separate patch series.

> > The above solves the problem with panels wanting to power sequence
> > themselves at remove() time, but not at shutdown() time. Thus we'd
> > still have a dependency on having all drivers use
> > drm_atomic_helper_shutdown() so that work becomes a dependency.
>
> Does it? I think it can be done in parallel?

I don't think it can be in parallel. While it makes sense for panels
to call drm_panel_remove() at remove time, it doesn't make sense for
them to call it at shutdown time. That means that the trick of having
the panel get powered off in drm_panel_remove() won't help for
shutdown. For shutdown, which IMO is the more important case, we need
to wait until all drm drivers call drm_atomic_helper_shutdown()
properly.

-Doug