On Tue, 17 May 2016 18:18:27 +0200, Daniel Vetter 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). One of the problems that make things complicated is: we don't know whether there is HDMI/DP codec present before probing on the bus. SKL has a common HD-audio bus, and it might be that there is no i915 HDMI codec on it but only the onboard analog codec. However, for probing the HDMI codec itself, i915 has to be initialized beforehand. So, your assumption "if you have the sound side of hdmi/dp audio": this can't be known before i915 is loaded and the HDMI/DP audio is probed. It's a kind of chicken and egg problem. Yes, we can know at least whether i915 graphics is present on PCI bus, and we already check it for Nvidia/AMD hybrid systems. But it doesn't give a chance to know whether i915 gfx is available at all (nomodeset or whatever disablement). This is why the whole this patchset was invented. The rest -- how to bind things -- is a matter of implementation details. Once when we have a material, we can write more elegantly. But, if there is no disablement notification, EPROBE_DEFER can't work well with the fallback mechanism. > 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. Yes, I know of the pains. A better implementation is always welcome, and I'm not willing to stick with my current patches. Takashi _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
