On Tue, 2014-12-09 at 11:03 +0100, Daniel Vetter wrote:
On Tue, Dec 09, 2014 at 10:59:54AM +0200, Imre Deak wrote:
On Mon, 2014-12-08 at 21:14 +0100, Daniel Vetter wrote:
On Mon, Dec 08, 2014 at 06:42:04PM +0200, Imre Deak wrote:
The current hda/i915 interface to enable/disable power wells and query the CD clock rate is based on looking up the relevant i915 module symbols from the hda driver. By using the component framework we can get rid of some global state tracking in the i915 driver and pave the way to fully decouple the two drivers: once support is added to enable/disable the HDMI functionality dynamically in the hda driver, it can bind/unbind itself from the i915 component master, without the need to keep a reference on the i915 module.
This also gets rid of the problem that currently the i915 driver exposes the interface only on HSW and BDW, while it's also needed at least on VLV/CHV.
Awesome that you're tackling this, really happy to see these hacks go. Unfortunately I think it's upside down: hda should be the component master and i915 should only register a component.
Longer story: The main reason for the component helpers is to be able to magically delay registering the main/master driver until all the components are loaded. Otherwise -EDEFER doesn't work and also the suspend/resume ordering this should result in. Master here means whatever userspace eventually sees as a device node or similar, component is anything really that this userspace interfaces needs to function correctly.
EDEFER doesn't solve the suspend/resume ordering, at least not in the general async s/r case. In any case I don't see a problem in making the hda component the master and I think it's more logical that way as you said, so I changed that.
Yeah for full async s/r we're screwed as-is. But see below for some crazy ideas.
I think what we need here is then:
- Put the current azx_probe into the master_bind callback. The new azx_probe will do nothing else than register the master component and the component match list. To do so it checks the chip flag and if it needs to cooperate with i915 it registers one component match for that. The master_bind (old probe) function calls component_bind_all with the hda_intel pointer as void *data parameter.
I'm not sure this is the best way since by this the i915 module would need to be pinned even when no HDMI functionality is used. I think a better way would be to let the probe function run as now and init/register all the non-HDMI functionality. Then only the HDMI functionality would be inited/registered from the bind/unbind hooks.
Hm, I wasn't sure whether alsa allows this and thought we need i915 anyway to be able to load the driver. But if we can defer just the hdmi part.
But we could go either way even as a follow-up to this patchset.
- i915 registers a component for the i915 gfx device. It uses the component device to get at i915 sturctures and fills the dev+ops into the hda_intel pointer it gets as void *data.
Stuff we then should do on top:
- Add deferred probing to azx_probe: Only when all components are there should it actually register. This will take care of all the module load order mess.
I agree that we should only register user interfaces when everything is in place. But I'm not sure deferred probe is the best, we could do without it by registering HDMI from the component bind hook.
It's mostly a question whether alsa does support delayed addition of interface parts. DRM most definitely does not (except for recently added dp mst connector hotplug). But I guess if the current driver already delays registering the hdmi part then we're fine. I'm not sure about whether it's really safe - I spotted not a lot of locking really to make sure there's no races between i915 loading and userspace trying to access the hdmi side.
Ok, I'm not sure either. If that's not possible, I agree EDEFER would be the best and with that we could also get rid of the request_module() we need now.
It should also take care of system suspend/resume ordering and we should be able to delete all the early_resume/late_suspend trickery.
Deferred probe doesn't solve the suspend/resume ordering, we would need to have a separate HDMI device that is set as a child to the i915 device. Alternatively we could use device_pm_wait_for_dev().
I'm not sure whether there's the possibility of deadlocks with device_pm_wait_for_dev. But if that works I think a component helper to wait for all components to resume would be really useful. Or we implement the full-blown pm_ops idea laid out below for components.
Yes it could deadlock with pm_async=0 (a debug thing nowadays?) and depending on the bus address of the waiter and wait-for device. At least for us this can't happen (with the fixed PCI addresses for the gfx and audio devices), but I don't know if you can consider this a clean solution. Perhaps by adding a check for this in device_pm_wait_for_dev() and avoiding the deadlock by returning some error would be safe enough.
Imo we should have things ready up to this point to make sure this refactoring actually works.
I think we could go with this minimal patch with your change to have the hda component be master. This only adds the support for components and keeps everything else the same. We could consider the bigger changes as a follow-up.
Yeah that was my plan - if EDEFER isn't enough then we keep the early/late resume/suspend hooks for a bit longer.
Then there's some cool stuff we could do on top:
Register a i915-hda platform devices as a child of the gfx pci device. Besides shuffling around a bit with the interfaces/argument casting and the component match function this doesn't really have a functional impact. But it makes the relationship more clear since hda doesn't really need the entire pci device, but only the small part that does audio.
Replace the hand-rolled power-well interface with runtime pm on that device node.
If system suspend/resume doesn't work automatically with deferred probing (tbh I'm not sure) add pm_ops to the component master. Then add some functions as default implementations for pm_ops for components which simply refcount all component pm_ops calls and call the master pm_ops suspend on the first suspend call and resume on the last resume call. That really should take care of suspend/resume ordering for good.
Yep, these sound good. I think having an HDMI child device is the cleanest solution for the s/r ordering issue.
Ok, sounds like we have a plan. And thanks again for tackling this, I'm really happy to see this go away.
Cheers, Daniel