On Tue, May 17, 2016 at 02:47:44PM +0200, Takashi Iwai wrote: > On Tue, 17 May 2016 14:34:21 +0200, > Daniel Vetter wrote: > > > > On Tue, May 17, 2016 at 03:11:02PM +0300, Imre Deak wrote: > > > On ti, 2016-05-17 at 13:10 +0200, Daniel Vetter wrote: > > > > On Tue, May 17, 2016 at 01:22:52PM +0300, Imre Deak wrote: > > > > > On ti, 2016-05-17 at 11:59 +0200, Takashi Iwai wrote: > > > > > > On Tue, 17 May 2016 11:42:17 +0200, > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > > On Tue, May 17, 2016 at 09:37:12AM +0200, Takashi Iwai wrote: > > > > > > > > On Tue, 17 May 2016 09:20:48 +0200, > > > > > > > > Daniel Vetter wrote: > > > > > > > > > > > > > > > > > > On Thu, May 12, 2016 at 09:06:53PM +0300, Imre Deak wrote: > > > > > > > > > > User may pass nomodeset or i915.modeset=0 option to > > > > > > > > > > disable i915 KMS > > > > > > > > > > explicitly. Although this itself works fine, it breaks > > > > > > > > > > the weak > > > > > > > > > > dependency the HD-audio driver requires, and it's the > > > > > > > > > > reason the > > > > > > > > > > delayed component binding isn't implemented in HD- > > > > > > > > > > audio. Since i915 > > > > > > > > > > doesn't notify its disablement, HD-audio would be blocked > > > > > > > > > > unnecessarily endlessly, waiting for the bind with i915. > > > > > > > > > > > > > > > > > > > > This patch introduces a stub audio component binding when > > > > > > > > > > i915 driver > > > > > > > > > > is loaded with KMS off like the boot options above. Then > > > > > > > > > > i915 driver > > > > > > > > > > still registers the slave component but with the new > > > > > > > > > > "disabled" ops > > > > > > > > > > flag, so that the master component (HD-audio) can know > > > > > > > > > > clearly the > > > > > > > > > > slave state. > > > > > > > > > > > > > > > > > > > > v2: > > > > > > > > > > - Fail the probe in case component registration fails, > > > > > > > > > > instead of > > > > > > > > > > suppressing the error. (Ville) > > > > > > > > > > - Register the component only for the real PCI device > > > > > > > > > > function. > > > > > > > > > > > > > > > > > > > > CC: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > > > > > > > > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx> > > > > > > > > > > Signed-off-by: Takashi Iwai <tiwai@xxxxxxx> > > > > > > > > > > > > > > > > > > We don't support not running with modesetting. Why do we > > > > > > > > > suddenly care? > > > > > > > > > > > > > > > > This is needed for the patch 2 and 3. Right now we have no > > > > > > > > blocking > > > > > > > > or deferred component binding, so far, in HD-audio > > > > > > > > side. This caused > > > > > > > > problems when async module probe was done. > > > > > > > > > > > > > > > > So, the patch 3 implements the blocking behavior of HD-audio > > > > > > > > side. It > > > > > > > > would lead to another regression when i915 doesn't notify its > > > > > > > > disabled > > > > > > > > state by this patch. Otherwise the HD-audio driver will be > > > > > > > > blocked > > > > > > > > endlessly of unnecessarily long. > > > > > > > > > > > > > > > > > Same for users creating a .config that fails to boot or > > > > > > > > > work ... > > > > > > > > > > > > > > > > The config isn't cared much, but the problem is about the > > > > > > > > runtime boot > > > > > > > > option. > > > > > > > > > > > > > > > > > If HDA needs to coporate with gfx to get things done, then > > > > > > > > > imo we should > > > > > > > > > just require that i915.ko is there. > > > > > > > > > > > > > > > > Other way round: we do already require i915 in HD-audio > > > > > > > > side. But in > > > > > > > > this case, we do *not* want to require i915 when it's > > > > > > > > disabled in > > > > > > > > runtime. > > > > > > > > > > > > > > That's what I mean: If you boot with i915.nomodeset you're > > > > > > > explicitly fine > > > > > > > with a somewhat non-useable system - that option is for > > > > > > > debugging only > > > > > > > really. If that means audio also doesn't work, then I think > > > > > > > that's ok. > > > > > > > > > > > > It's not only "it doesn't work". The module load gets stuck. So > > > > > > we > > > > > > need some notification for the blocked component binding. > > > > > > > > > > > > > Adding complexity for this case (which means more error paths > > > > > > > we don't > > > > > > > ever test and hence _will_ break) seems over the top. > > > > > > > > > > > > > > I'm quite opposed to adding error handling for every condition > > > > > > > in general > > > > > > > because the combinatorial testing madness just can't be > > > > > > > handled. The one > > > > > > > exception in the i915.ko driver is that when the render side > > > > > > > died > > > > > > > (terminal gpu hang) we'll try our best to keep the display > > > > > > > alive. But > > > > > > > that's it, and the justification for that is "we want users to > > > > > > > be able to > > > > > > > get the bug report out". I don't see a justification of that > > > > > > > magnitude for > > > > > > > this feature here at all. > > > > > > > > > > > > Well, actually the patchset was proposed just because Intel CI > > > > > > tests > > > > > > failed due to async module probes. If Intel is happy with > > > > > > continued > > > > > > CI test failures, I'm also happy with the current situation ;) > > > > > > > > > > It's not only due to those particular failures. That was caused by > > > > > a > > > > > kmod bug and as such would be good to not depend on that mechanism. > > > > > But > > > > > things will fail atm even in the normal case when audio is built-in > > > > > and > > > > > i915 is a module. This patchset would solve that too. > > > > > > > > I'm not against patch 3 from this series, which seems to be the > > > > bugfix we > > > > want. I'm against the kludge in patch 1 here only (and maybe patch 2, > > > > not > > > > sure about that). Patch 1 here looks like a workaround purely for the > > > > i915.modeset=0 case. And I don't want special code for debug knobs if > > > > we > > > > can avoid it. > > > > > > We have discussed this in more detail, see: > > > https://bugs.freedesktop.org/show_bug.cgi?id=94566#c26 > > > > I disagree with Takashi here. modeset=0 is a debug option, and we should > > probably mark it as _unsafe. > > Well, even if it's a debug option, making the system unusable isn't a > nice move. With the blocking component binding, the audio side stalls > unexpectedly (and silently). How user would know it? > > I'm no big fan of the current implementation, but I really would like > to avoid users hitting the unexpected results. 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. > > > The point from Takashi in that discussion was that in order to do the > > > probing via the component bind hook as we want and as patch 3 does we > > > need to make sure basic audio functionality works even with the > > > nomodeset option. For that you need the kludge in patch 1. I think of > > > this as a similar kludge we have for not failing modprobing i915 in > > > case of nomodeset. Legacy userspace depends on this for basic graphics > > > functionality and in the same way some other userspace bits may depend > > > on basic audio functionality. > > > > nomodeset isn't supported really anymore in upstream, we've thrown out the > > Kconfig option needed by distros for that: > > > > commit fd930478fb797e4cbaa799d9ddd970e9a1fa1b4a > > Author: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> > > Date: Fri Jun 19 20:27:27 2015 +0100 > > > > drm/i915: Remove KMS Kconfig option > > > > That was almost a year ago, and no one complained. I don't think modeset=0 > > is a use-case any more, and hence I don't think we need special code to > > support it. If some users scream that we broke their setup we can fix this > > later on, but right now I'm firmly in the "nope" territory. > > Hmm, I thought nomodeset option still keeps working? > i915 checks vgacon_text_force(), and nomodeset is set there. Yes, but a debug option only. We have lots of those, and many of them break your system if you touch them and don't know what to you're doing ;-) -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
