On Tue, 17 May 2016 18:23:40 +0200, Daniel Vetter wrote: > > On Tue, May 17, 2016 at 6:18 PM, Daniel Vetter <daniel@xxxxxxxx> wrote: > > On Tue, May 17, 2016 at 3:20 PM, Takashi Iwai <tiwai@xxxxxxx> wrote: > >>> Ok, I looked at patch 3, and that indeed would lead to trouble without > >>> patch 1. But the real trouble is the unconditional wait_completion in > >>> there - blocking for another driver to complete loading from a driver load > >>> function is a no-go. The correct way to do this is to bail out with > >>> EPROBE_DEFER if not all the parts are available there. Also, throw out > >>> that request_module. > >>> > >>> By bailing out with EPROBE_DEFER you avoid deadlocks, and the driver core > >>> also knows what's going on. Which is incidentally what's used to > >>> implicitly order suspend/resume. Driver core will restart your probe as > >>> soon as some new devices/drivers have registers (assuming that hopefully > >>> then you're unblocking), but if you're unlucky your driver can go through > >>> that loop a few times. > >>> > >>> But that was just a very quick look, we definitely shouldn't need any > >>> wait_completion in driver load to handle cross-module depencies. > >> > >> Yeah, I admit that wait_completion() is hackish. OTOH, EPROBE_DEFER > >> doesn't work in the case of HD-audio because we want to give up > >> binding and continue without i915 but only with onboard audio, instead > >> of endlessly reprobing for the never-appearing component. The i915 > >> binding is no hard dependency; i.e. it isn't (always) mandatory, and > >> EPROBE_DEFER can't handle such a fallback, AFAIK. > >> > >> If there is a good way to deal with it, please let me know. I'd love > >> to rewrite to a cleaner way. > > > > The only way to deal with that is to split the driver into two, and > > hotplug them individually. Fundamentally any approach where you need > > to know whether i915 shows up or not and act accordingly is just plain > > flawed, there's no way around it. That's also why EPROBE_DEFER doesn't > > bother dealing with it. > > > > Imo if you have the sound side of hdmi/dp audio, then just > > EPROBE_DEFER until i915 is loaded (assuming it's not disabled through > > nomodeset or Kconfig). If it's not there then continue without it (and > > without hdmi/dp audio ofc). Trying to be clever just means we need to > > hand roll things all over the place all the time. We have some code on > > earlier platforms for runtime clock adjustements (on ironlake) in > > i915.ko, and I really don't want that kind of hacks any more. > > In case you don't believe me that your hack is broken: I often boot > with i915 blacklisted, so that I can set up netconsole and other > instrumenting and then load it again with modeset. Until that's done > snd-hda will be stuck in that wait_completion. Yes, but it has a fallback after some long timeout. Then at least user would see if binding failure happens and going without HDMI/DP. > You really can't ever > know when userspace or the user decides to finally load the driver, > and the only reasonable thing to do is to defer until everything you > need is there. Except of course when the user told you it's not going > to show up through nomodeset or Kconfig knobs, but that's kinda the > exception. > > Imo the best course forward would be: > - Implement EPROBE_DEFER correctly in snd-hda (i.e. no > wait_completion, no deferred work or anything like that, just return > -EPROBE_DEFER when i915 isn't there yet). We actually don't need EPROBE_DEFER, either. The component master bind would take care of that already. I used wait_for_completion() just because I didn't want the hda-i915 binder API function. If it takes the function to be continued, master binder can call it after binding with i915 properly. > - Add a bail-out option if CONFIG_DRM_I915 isn't enabled to snd-hda. You can forget about Kconfig. It's already handled. No relevant code will be executed when CONFIG_DRM_I915 isn't set. > - Add a runtime bail-out for nomodeset/vgacon_text_force() to snd-hda. Yes, that's an easy part. > That should cover everyone's debug needs, while giving us a clean > architecture moving forward. Thoughts? But this doesn't cover all cases. As you mentioned, what if user would blacklist i915, or set i915.modeset=0? In my patch, HD-audio tries to continue at least some timeout. thanks, Takashi _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
