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

Wed May 18 09:06:48 UTC 2016

On Tue, 17 May 2016 23:11:10 +0200,
Daniel Vetter wrote:
> 
> On Tue, May 17, 2016 at 08:19:32PM +0200, Takashi Iwai wrote:
> > 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.
> 
> Well component does a deferred probe internally afaik.

Really?  I see that component_*() functions just call the ops
directly...

But in our case, it doesn't matter, since the component binding itself
is already done in the deferred work.

In anyway, my point is that EPROBE_DEFER can't handle the timeout, and
EPROBE_DEFER is superfluous when the component binding is used.

> > > - 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.
> 
> My opinion is that you can't decide correctly. Either the timeout is too
> long, and you have users complaining that audio doesn't work because the
> driver doesn't complete loading. Or it's too short, and you break
> EDEFER_PROBING. I guess if you want you can code up a timeout in snd-hda,
> but since patch 1 doesn't fix anything when a user outright blacklist i915
> I guess we can drop that one at least?

Surely there can't be any 100% workable solution.  But if i915 driver
knows that it's disabled, it'd be still helpful to notify it if any
others expect i915 getting up.

Imagine you know that you can't attend a party you are invited in this
evening.  What's the best act?  A cancel notification would make
things working better than just let people waiting.

In this analogy, people also shouldn't wait endlessly for you,
either.  Setting a timeout is very reasonable.

> I just don't want these kinds of hacks to leak into i915, if you're fine
> with that I don't mind.

Well, this can be seen as a lack in component binding mechanism, too.
There is no cancellation notification there.  So, Imre came up with a
stub ops implementation.  It's hacky, yes.

Maybe vgacon_text_force() check and some timeout without i915 side
changes would be enough as a start, until some good way for cancel
notification is implemented.  User can still reload the sound module
after i915 if needed, too.  The timeout can be an adjustable module
option, if it really matters.

thanks,

Takashi