[Intel-gfx] [PATCH v2 1/3] drm/i915/hda: Add audio component stub

[Takashi Iwai]

On Tue, 17 May 2016 18:23:40 +0200,
Daniel Vetter wrote:
> 
> On Tue, May 17, 2016 at 6:18 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
> > On Tue, May 17, 2016 at 3:20 PM, Takashi Iwai <tiwai at suse.de> 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