Hi, On Wed, Jul 26, 2023 at 5:41 AM Maxime Ripard <mripard@xxxxxxxxxx> wrote: > > Hi, > > 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? 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. -Doug
