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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx> 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

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux