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 <[email protected]> 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

Attachments:

(No filename) (3.06 kB)
signature.asc (235.00 B)
Download all attachments

2023-07-31 16:56:35

by Chris Morgan

[permalink] [raw]

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

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 <[email protected]> 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 <[email protected]> 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

2023-07-31 17:15:46

On Wed, 2 Aug 2023 at 18:26, Chris Morgan <[email protected]> wrote:
>
> * Spam *
> On Mon, Jul 31, 2023 at 07:03:07PM +0200, Maxime Ripard wrote:
> > Hi,
> >
> > On Mon, Jul 31, 2023 at 11:33:22AM -0500, Chris Morgan wrote:
> > > In my case a few different panel drivers disable the regulators in the
> > > unprepare/disable routines.
> >
> > And that's totally fine.
> >
> > > For at least the Rockchip DSI implementations for some reason the
> > > panel gets unprepared more than once, which triggers an unbalanced
> > > regulator disable.
> >
> > "For some reason" being that DW-DSI apparently finds it ok to bypass any
> > kind of abstraction and randomly calling panel functions by itself:
> >
> > https://elixir.bootlin.com/linux/v6.4.7/source/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c#L868
> >
> > It looks like it's fixed it current drm-misc-next though.
>
> Good, when I get a chance I will test it out with the existing panels
> I have at my disposal and submit some patches to clean them up.
>
> >
> > > 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.
> >
> > Like I said we already have a commit on the way to fix that, so it
> > shouldn't be an issue anymore.
> >
> > I stand by what I was saying earlier though, I think it's mostly
> > cargo-cult or drivers being very wrong. If anything, the DW-DSI stuff
> > made me even more convinced that we shouldn't even entertain that idea
> > :)

DW-DSI is hacking around the fact that DSI panels may want to send DCS
commands in unprepare, however the DSI host driver shuts down the
controller in the DSI bridge post_disable which gets called first.

That ordering can now be reversed with pre_enable_prev_first flag in
struct drm_bridge, or prepare_prev_first in drm_panel, hence no need
for the DSI controller to jump around the bridge chain.

Dave

> > Maxime
>
> Thank you, and yes if a driver is doing something it shouldn't we
> shouldn't be patching around that, we should be fixing things. Thanks
> for providing me with the additional info.
>
> Chris
>