Re: [PATCH v3 02/10] drm/panel: Check for already prepared/enabled in drm_panel

[Chris Morgan <macroalpha82@gmail.com>]

In my case a few different panel drivers disable the regulators in the
unprepare/disable routines. For at least the Rockchip DSI
implementations for some reason the panel gets unprepared more than
once, which triggers an unbalanced regulator disable. Obviously though
the correct course of action is to fix the reason why the panel is
disabled more than once, but that's at least the root cause of this
behavior on the few panels I've worked with.

Thank you.

On Thu, Jul 27, 2023 at 1:38 AM Maxime Ripard <mripard@kernel.org> wrote:
>
> Hi,
>
> On Wed, Jul 26, 2023 at 08:10:33AM -0700, Doug Anderson wrote:
> > On Wed, Jul 26, 2023 at 5:41 AM Maxime Ripard <mripard@kernel.org> wrote:
> > > On Tue, Jul 25, 2023 at 01:34:37PM -0700, Douglas Anderson wrote:
> > > > NOTE: arguably, the right thing to do here is actually to skip this
> > > > patch and simply remove all the extra checks from the individual
> > > > drivers. Perhaps the checks were needed at some point in time in the
> > > > past but maybe they no longer are? Certainly as we continue
> > > > transitioning over to "panel_bridge" then we expect there to be much
> > > > less variety in how these calls are made. When we're called as part of
> > > > the bridge chain, things should be pretty simple. In fact, there was
> > > > some discussion in the past about these checks [1], including a
> > > > discussion about whether the checks were needed and whether the calls
> > > > ought to be refcounted. At the time, I decided not to mess with it
> > > > because it felt too risky.
> > >
> > > Yeah, I'd agree here too. I've never found evidence that it was actually
> > > needed and it really looks like cargo cult to me.
> > >
> > > And if it was needed, then I'm not sure we need refcounting either. We
> > > don't have refcounting for atomic_enable / disable, we have a sound API
> > > design that makes sure we don't fall into that trap :)
> > >
> > > > Looking closer at it now, I'm fairly certain that nothing in the
> > > > existing codebase is expecting these calls to be refcounted. The only
> > > > real question is whether someone is already doing something to ensure
> > > > prepare()/unprepare() match and enabled()/disable() match. I would say
> > > > that, even if there is something else ensuring that things match,
> > > > there's enough complexity that adding an extra bool and an extra
> > > > double-check here is a good idea. Let's add a drm_warn() to let people
> > > > know that it's considered a minor error to take advantage of
> > > > drm_panel's double-checking but we'll still make things work fine.
> > >
> > > I'm ok with this, if we follow-up in a couple of releases and remove it
> > > and all the calls.
> > >
> > > Could you add a TODO item so that we can keep a track of it? A follow-up
> > > is fine if you don't send a new version of that series.
> >
> > By this, I think you mean to add a "TODO" comment inline in the code?
>
> No, sorry, I meant an entry in our TODO list: Documentation/gpu/todo.rst
>
> > Also: I was thinking that we'd keep the check in "drm_panel.c" with
> > the warning message indefinitely. You think it should be eventually
> > removed? If we are truly thinking of removing it eventually, this
> > feels like it should be a more serious warning message like a WARN(1,
> > ...) to make it really obvious to people that they're relying on
> > behavior that will eventually go away.
>
> Yeah, it really feels like this is cargo-cult to me. Your approach seems
> like a good short-term thing to do to warn everyone but eventually we'll
> want it to go away.
>
> So promoting it to a WARN could be a good thing, or let's say we do a
> drm_warn for now, WARN next release, and gone in two?
>
> Maxime