On Tue, Jun 18, 2024 at 04:49:22PM -0700, Doug Anderson wrote: > Hi, > > On Mon, Jun 17, 2024 at 7:17 AM Daniel Vetter <daniel@xxxxxxxx> wrote: > > > > > That all being said, I'm also totally OK with any of the following: > > > > > > 1. Dropping my patch and just accepting that we will have warnings > > > printed out for all DRM drivers that do things correctly and have no > > > warnings for broken DRM drivers. > > > > Can't we just flip the warnings around? Like make the hacky cleanup > > conditional on the panel not yet being disabled/cleaned up, and complain > > in that case only. With that: > > - drivers which call shutdown shouldn't get a warning anymore, and also > > not a surplus call to drm_panel_disable/unprepare > > - drivers which are broken still get the cleanup calls > > - downside: we can't enforce this, because it's not yet enforced through > > device_link ordering > > I feel like something is getting lost in the discussion here. I'm just > not sure where to put the hacky cleanup without either having a list > like I have or fixing the device link problem so that the DRM device > shutdown gets called before the panel. Specifically, right now I think > it's possible for the panel's shutdown() callback to happen before the > DRM Device's shutdown(). Thus, we have: > > 1. Panel shutdown() checks and says "it's not shutdown yet so do my > hacky cleanup." > 2. DRM device shutdown() gets called and tries to cleanup again. > > ...and thus in step #1 we can't detect a broken DRM device. What am I missing? The above is a broken drm driver, because shutting down something that the main drm driver needs _before_ it is shut down itself is broken. That's why we need the device link. So if this case goes a bit wrong that's imo ok. That was my point that without device links, we cannot have _any_ warning at all, but we can at least make sure that correct drivers, meaning: - they make sure the panel is around for longer than the drm device - and they call drm atomic_helper_shutdown in the right places Wont either double-shutdown the panel or get the warning. It's not great, but it's at least better than the current situation where correct drivers get a warning, and some broken drivers don't. So still an improvement. That leaves us with the issue of warning for all broken drivers. We have two proposals for that: - Your explicit list, which is a pain imo, and I'm not seeing the benefit of this, because it'll encourage each driver to hack around the core code bug of not having the right device links. - Fixing this with a device link and adding the warning for everyone. This isn't a great situation, because it's likely going to be another few years without the device link situation sorted out. But that's been the case already for years so *shrug*. > > > 2. Someone else posting / landing a patch to remove the hacky "disable > > > / unprepare" for panel-simple and panel-edp and asserting that they > > > don't care if they break any DRM drivers that are still broken. I > > > don't want to be involved in authoring or landing this patch, but I > > > won't scream loudly if others want to do it. > > > > > > 3. Someone else taking over trying to solve this problem. > > > > > > ...mostly this work is janitorial and I'm trying to help move the DRM > > > framework forward and get rid of cruft, so if it's going to cause too > > > much conflict I'm fine just stepping back. > > > > My issue is that you're trading an ugly warning that hurts maintenance > > with an explicit list of working drivers, which also hurts maintenance. > > Does seem like forward progress to me, just pushing the issue around. > > IMO it at least moves things forward. If we make the warning obvious > enough then I think we could assert that, within a few kernel > versions, everyone who hit the warning would have addressed it. Once > that happens we can fully get rid of the ugly list and just make the > assumption that things are good. That feels like a clear path to me... Yeah, but your warning I think just encourages more hacks in drivers that shouldn't be there (for the ordering issue). So I'm not sure it's strictly better. And we have _tons_ of drm driver api misuse that we don't catch with warnings and checks. Sometimes that's just not possible, because the situation is too messy. If we add an explicit "I'm not broken" list for each such case, we will have an unmaintainable mess. Sometimes a "I'm the last broken driver" flag makes sense, but even there I'm cautious that it's a bright idea. Cheers, Sima -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
